wallet: post-#35501 cleanups in CWalletTx #35930

1. 
   pablomartin4btc commented at 2:04 PM on August 7, 2026: member

   Follow-up cleanups and clarifications after #35501 was merged.

   Commit breakdown:

   1. post-#35501 cleanup in CWalletTx

      • Rename arg_state → new_state in Update() for consistency
      • Simplify RecomputeCanonical() using std::ranges::min_element with a projection lambda (14 lines → 3 lines)
      • Add variant txid validation in the CWalletTx deserialise constructor: throws std::runtime_error if any variant's txid doesn't match the canonical txid deserialized from the stream
      • Move Init() to private and extend it to clear m_txs and reset m_canonical_wtxid, so a full re-deserialise via Unserialize() starts from a clean state

      All suggested by ajtowns.

   2. add unit test for variant txid validation in CWalletTx deserializer

   3. assert listsinceblock "removed" reports current canonical wtxid Documents that removed entries reflect the wallet's current CWalletTx state, not a snapshot of the detached block. A future followup could improve this (requires per-block tracking of which witness variant was included). Suggested by w0xlt.

   4. clarify alternate_wtxids is empty when only one witness variant Suggested by polespinasa.

2. DrahtBot added the label Wallet on Aug 7, 2026
3. 
   DrahtBot commented at 2:04 PM on August 7, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35930.

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline and AI policy for information on the review process.

   Type	Reviewers
   ACK	polespinasa, jeanpablojp, achow101, vicjuma

   If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #35935 (wallet: Avoid unnecessary wtxvariant rewrites by achow101)
   • #35511 (RFC: consensus: Make CAmount a class by hodlinator)
   • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)

   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.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

4. 
   achow101 commented at 11:19 PM on August 7, 2026: member

   ACK d5c0228f3df83fa60849a3ac24bedf31c9b9c40f

5. 
   jeanpablojp commented at 11:42 AM on August 8, 2026: none

   ACK d5c0228f3df83fa60849a3ac24bedf31c9b9c40f

   I have tested the code. The min_element rewrite preserves the old ordering, ties included: both take the first minimum, and !HasWitness() first in the pair keeps witnessed ahead of witnessless. I modelled both selections over 400k random maps with forced ties and they never diverged. Built the merge with master (05a7c47) with wallet and tests enabled: no build errors, unit suite and wallet_listtransactions.py pass.

6. in src/wallet/transaction.h:254 in d5c0228f3d outdated

    258 |          // Merge witness variants
    259 |          m_txs.insert(variants.begin(), variants.end());
    260 |          Assert(m_txs.contains(GetWitnessHash()));
    261 |      }
    262 |  
    263 | -    void Init()
   

   ---

   ---

   

   vicjuma commented at 12:28 AM on August 11, 2026:

   why not just rename it to eg. setdefaults then make the constructor its caller?

   ---

   

   pablomartin4btc commented at 2:06 AM on August 11, 2026:

   Done in 9de6543cb55ff43aea950712a3148fed9d4a7e13, thanks!

7. in src/wallet/transaction.h:242 in d5c0228f3d

    238 | @@ -239,27 +239,26 @@ class CWalletTx
    239 |          Assert(tx);
    240 |          m_canonical_wtxid = tx->GetWitnessHash();
    241 |          m_txs.emplace(tx->GetWitnessHash(), std::move(tx));
    242 | -        Init();
    243 | +        nTimeReceived = 0;
   

   ---

   ---

   

   vicjuma commented at 12:40 AM on August 11, 2026:

   Init(); will probably not fit here with the addition of m_txs.clear(); and m_canonical_wtxid = Wtxid{}; but encapsulating the initial values in a helper function is much cleaner IMO

   ---

   

   pablomartin4btc commented at 2:07 AM on August 11, 2026:

   Done in 9de6543cb55ff43aea950712a3148fed9d4a7e13, thanks!

8. in src/wallet/transaction.cpp:107 in d5c0228f3d outdated

    121 | -            best = &it->second;
    122 | -            best_wtxid = it->first;
    123 | -        }
    124 | -    }
    125 | -    m_canonical_wtxid = best_wtxid;
    126 | +    m_canonical_wtxid = std::ranges::min_element(m_txs, std::less{}, [](const auto& entry) {
   

   ---

   ---

   

   vicjuma commented at 12:52 AM on August 11, 2026:

   Much cleaner as per the comment

9. 
   vicjuma commented at 12:58 AM on August 11, 2026: contributor

   ACK

10. wallet: post-#35501 cleanup in CWalletTx

    - Rename arg_state to new_state in Update() declaration to match implementation
    - Replace RecomputeCanonical manual loop with std::ranges::min_element
    - Add variant txid validation in the deserialize constructor
    - Make Init() private and have it clear all members including m_txs
    
    Co-authored-by: Anthony Towns <aj@erisian.com.au>

    9de6543cb5
11. wallet, test: add unit test for variant txid validation in CWalletTx deserializer 9b96ee1288
12. test: assert listsinceblock "removed" reports current canonical wtxid

    When a block is detached, listsinceblock "removed" entries reflect the
    wallet's current CWalletTx rather than a snapshot of the variant that
    was actually in the detached block. Add assertions to make this
    behaviour explicit. A future followup could improve listsinceblock to
    track and report the specific witness variant that was in the
    disconnected block (requires per-block tracking of which witness variant was included).
    
    Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>

    fa48b5d28e
13. doc: clarify alternate_wtxids is empty when only one witness variant

    When there is only one known witness variant for a transaction,
    alternate_wtxids is an empty array, analogous to walletconflicts and
    mempoolconflicts.
    
    Suggested-by: polespinasa

    4ca182ca40
14. pablomartin4btc force-pushed on Aug 11, 2026
15. 
    pablomartin4btc commented at 2:12 AM on August 11, 2026: member

    -<ins>Updates</ins>:

    • Addressed @vicjuma's feedback:
      • Extract SetDefaults() private helper for the 5 metadata fields;
      • Have the constructor and Init() both call SetDefaults() to avoid duplication (DRY).
16. 
    polespinasa commented at 10:07 AM on August 11, 2026: member

    ACK 4ca182ca4028b9e681d65ec21f79fd7fed3ce215

    lgtm

17. DrahtBot requested review from vicjuma on Aug 11, 2026
18. DrahtBot requested review from achow101 on Aug 11, 2026
19. 
    jeanpablojp commented at 3:15 PM on August 11, 2026: none

    re-ACK 4ca182ca4028b9e681d65ec21f79fd7fed3ce215

20. 
    achow101 commented at 5:48 PM on August 11, 2026: member

    ACK 4ca182ca4028b9e681d65ec21f79fd7fed3ce215

21. 
    vicjuma commented at 6:04 PM on August 11, 2026: contributor

    ACK 4ca182ca4028b9e681d65ec21f79fd7fed3ce215

22. achow101 merged this on Aug 11, 2026
23. achow101 closed this on Aug 11, 2026

    ---

Contributors

pablomartin4btcDrahtBotachow101jeanpablojpvicjumapolespinasa

Review Requested
achow101vicjuma