Refactor: Redefine CTransaction equality to include witness data #32723

1. 
   theuni commented at 10:16 pm on June 10, 2025: member

   I stumbled upon the CTransaction comparison operators while refactoring some nearby code. I found it surprising and not at all obvious that two transactions would test equal even if their witness data differed. It seems like an unnecessary potential footgun. Fix that by comparing against wtxid rather than txid.

   Outside of tests, there were only 3 users of these functions in the code-base:

   • Its use in the mempool has been replaced with an explicit txid comparison, as that’s a tighter constraint and matches the old behavior. glozow suggested also upgrading this to an Assume().
   • Its use in the wallet was accidentally doing the correct thing by ignoring witness data. I’ve changed that to an explicit witness removal so that IsEquivalentTo continues to work as-intended.
   • Its use in getrawtransaction is indifferent to the change.
2. wallet: IsEquivalentTo should strip witness data in addition to scriptsigs e9331cd6ab
3. mempool: codify existing assumption about duplicate txids during removal

   Also explicitly check for txid equality rather than transaction equality as the
   former is a tighter constraint if witness data is included when comparing the
   full transactions.
   
   Co-authored-by: glozow <gloriajzhao@gmail.com>

   cbf9b2dab1
4. refactor: CTransaction equality should consider witness data

   It is not at all obvious that two transactions with differing witness data
   should test equal to each other.
   
   There was only a single instance of a caller relying on this behavior, and that
   one appears accidental (left-over from before segwit). That caller (in the
   wallet) has been fixed.
   
   Change the definition of transaction equality (and inequality) to use the wtxid
   instead.

   6efbd1e1dc
5. 
   DrahtBot commented at 10:16 pm on June 10, 2025: contributor

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

   Code Coverage & Benchmarks

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	glozow

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

6. DrahtBot added the label Refactoring on Jun 10, 2025
7. 
   theuni commented at 10:17 pm on June 10, 2025: member

   Ping @achow101 and @glozow to check my assumptions about the wallet/mempool uses.
8. 
   achow101 commented at 11:21 pm on June 10, 2025: member

   CWalletTx::IsEquivalentTo needs to consider malleated txs as equivalent, so the original behavior of ignoring the scriptSigs and scriptWitness is intended.
9. 
   theuni commented at 1:31 pm on June 11, 2025: member

   CWalletTx::IsEquivalentTo needs to consider malleated txs as equivalent, so the original behavior of ignoring the scriptSigs and scriptWitness is intended. @achow101 Thanks for confirming. Indeed there should be no functional change here from the original behavior. It currently works by virtue of the equality operator comparing txids, but that’s not at all obvious just from looking at IsEquivalentTo. I figured it just kinda accidentally kept working correctly post-segwit, but I guess you were aware of that quirk :)

   Manually stripping the witness scripts as well is much more straightforward imo.

10. in src/primitives/transaction.h:363 in 6efbd1e1dc

    359@@ -360,12 +360,12 @@ class CTransaction
    360 
    361     friend bool operator==(const CTransaction& a, const CTransaction& b)
    362     {
    363-        return a.hash == b.hash;
    364+        return a.GetWitnessHash() == b.GetWitnessHash();
    

    ---

    ---

    

    glozow commented at 4:23 pm on June 11, 2025:

    nit: can use m_witness_hash

    ---

    

    theuni commented at 5:33 pm on June 11, 2025:

    I have a follow-up PR that will change the representation of the hashes (to allow txid and wtxid to be calculated at the same time without double serialization, so unless you feel strongly I’d prefer to leave this as-is to avoid more churn.

    ---

    

    l0rinc commented at 5:39 pm on June 11, 2025:

    I also prefer the method instead, an unrelated experiment I did yesterday (investigating making these hash computations lazy) also needs methods here. Not critical, just mentioning a possible usecase. Funny timing :)
11. 
    glozow commented at 4:23 pm on June 11, 2025: member

    ACK 6efbd1e1dcdfbe9eae2d5c22abab3ee616a75ff2

Contributors
theuni DrahtBot achow101 glozow l0rinc

Labels
Refactoring