doc: Fix CWalletTx::Confirmation doc #23332

1. 
   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.

2. MarcoFalke force-pushed on Oct 21, 2021
3. MarcoFalke added the label Docs on Oct 21, 2021
4. MarcoFalke added the label Wallet on Oct 21, 2021
5. 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
6. 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
7. 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.
8. MarcoFalke force-pushed on Oct 21, 2021
9. 
   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.

10. 
    laanwj commented at 2:52 pm on October 22, 2021: member

    Code review ACK fa8fef6ef2287cd36ae14fbf10e852ddef7e62ac
11. 
    MarcoFalke commented at 3:03 pm on October 22, 2021: member

    cc Ryan @ryanofsky
12. 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?
13. ryanofsky approved
14. 
    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.

15. MarcoFalke merged this on Oct 26, 2021
16. MarcoFalke closed this on Oct 26, 2021

    ---

17. MarcoFalke deleted the branch on Oct 26, 2021
18. sidhujag referenced this in commit 0a88a7e3cf on Oct 26, 2021
19. DrahtBot locked this on Oct 30, 2022

Contributors
MarcoFalke laanwj DrahtBot ryanofsky

Labels
Docs Wallet