doc: Fix CWalletTx::Confirmation doc #23332

MarcoFalke commented at 1:37 PM on October 21, 2021: member

Follow-up to:

commit 700c42b85d20e624bef4228eef062c93084efab5, which replaced pIndex with block_hash in AddToWalletIfInvolvingMe.
commit 9700fcb47feca9d78e005b8d18b41148c8f6b25f, which replaced posInBlock with confirm.nIndex.

MarcoFalke force-pushed on Oct 21, 2021

MarcoFalke added the label Docs on Oct 21, 2021

MarcoFalke added the label Wallet on Oct 21, 2021

in src/wallet/transaction.h:165 in faf9a9f508 outdated

 161 | @@ -162,7 +162,8 @@ class CWalletTx
 162 |          int block_height;
 163 |          uint256 hashBlock;
 164 |          int nIndex;
 165 | -        Confirmation(Status s = UNCONFIRMED, int b = 0, uint256 h = uint256(), int i = 0) : status(s), block_height(b), hashBlock(h), nIndex(i) {}
 166 | +        Confirmation(Status s = UNCONFIRMED, int block_height = 0, uint256 block_hash = uint256(), int block_index = 0)

laanwj commented at 6:56 PM on October 21, 2021:

Could rename s to status here as well

MarcoFalke commented at 8:14 PM on October 21, 2021:

Done

doc: Fix CWalletTx::Confirmation doc

Follow-up to:
* commit 700c42b85d20e624bef4228eef062c93084efab5, which replaced pIndex
  with block_hash in AddToWalletIfInvolvingMe.

* commit 9700fcb47feca9d78e005b8d18b41148c8f6b25f, which replaced
  posInBlock with confirm.nIndex.

fa8fef6ef2

in src/wallet/wallet.h:288 in faf9a9f508 outdated

 284 | @@ -285,8 +285,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
 285 |  
 286 |      void SyncMetaData(std::pair<TxSpends::iterator, TxSpends::iterator>) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 287 |  
 288 | -    /* Used by TransactionAddedToMemorypool/BlockConnected/Disconnected/ScanForWalletTransactions.
 289 | -     * Should be called with non-zero block_hash and posInBlock if this is for a transaction that is included in a block. */
 290 | +    /** Should be called with non-zero block_hash and block_index if this is for a transaction that is included in a block. */

laanwj commented at 7:02 PM on October 21, 2021:

I think this comment is slightly confusing. There are no block_hash and block_index parameters. Maybe add confirm. in front?

MarcoFalke commented at 8:14 PM on October 21, 2021:

I've removed the comment. CWalletTx::Confirmation is self-explanatory in that regard.

MarcoFalke force-pushed on Oct 21, 2021

DrahtBot commented at 1:32 PM on October 22, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj commented at 2:52 PM on October 22, 2021: member

Code review ACK fa8fef6ef2287cd36ae14fbf10e852ddef7e62ac

MarcoFalke commented at 3:03 PM on October 22, 2021: member

cc Ryan @ryanofsky

in src/wallet/wallet.h:289 in fa8fef6ef2

 284 | @@ -285,8 +285,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
 285 |  
 286 |      void SyncMetaData(std::pair<TxSpends::iterator, TxSpends::iterator>) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 287 |  
 288 | -    /* Used by TransactionAddedToMemorypool/BlockConnected/Disconnected/ScanForWalletTransactions.
 289 | -     * Should be called with non-zero block_hash and posInBlock if this is for a transaction that is included in a block. */

ryanofsky commented at 4:24 PM on October 25, 2021:

In commit "doc: Fix CWalletTx::Confirmation doc" (fa8fef6ef2287cd36ae14fbf10e852ddef7e62ac)

Minor: This comment does seem useful to me and I'd probably s/non-zero block_hash and posInBlock/confirmation/ instead of dropping it.

MarcoFalke commented at 9:43 AM on October 26, 2021:

This is already explained in the Confirmation doc:

/** Confirmation includes tx status and a triplet of {block height/block hash/tx index in block} ...

MarcoFalke commented at 9:46 AM on October 26, 2021:

If you think this is important to explain, it might be better to put the comment to TxStateConfirmed from #21206?

ryanofsky approved

ryanofsky commented at 4:26 PM on October 25, 2021: member

Code review ACK fa8fef6ef2287cd36ae14fbf10e852ddef7e62ac

This is probably ready for merge, but if anybody else reviews this they may also be interested in #21206 which changes the same code and does a broader cleanup.

MarcoFalke merged this on Oct 26, 2021

MarcoFalke closed this on Oct 26, 2021

MarcoFalke deleted the branch on Oct 26, 2021

sidhujag referenced this in commit 0a88a7e3cf on Oct 26, 2021

DrahtBot locked this on Oct 30, 2022