Refactor: Redefine CTransaction equality to include witness data

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.

wallet: IsEquivalentTo should strip witness data in addition to scriptsigs e9331cd6ab

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

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

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, maflcko, achow101

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

DrahtBot added the label Refactoring on Jun 10, 2025

theuni commented at 10:17 pm on June 10, 2025: member

Ping @achow101 and @glozow to check my assumptions about the wallet/mempool uses.

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.

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.

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 :)

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

ACK 6efbd1e1dcdfbe9eae2d5c22abab3ee616a75ff2

luke-jr commented at 11:22 pm on June 24, 2025: member

Seems like it might be best to forbid operator== entirely and require comparisons to be explicit about whether witness data is compared or not.

maflcko commented at 9:24 am on June 25, 2025: member

lgtm. Also, seems fine to remove it and add a Equals(const CTransaction& other, const EqualsOptions = {}), where

0struct EqualsOptions{
1 bool include_script_sig{true};
2 bool include_witness_data{true};
3};

review ACK 6efbd1e1dcdfbe9eae2d5c22abab3ee616a75ff2 🦋

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 6efbd1e1dcdfbe9eae2d5c22abab3ee616a75ff2 🦋
3N0uT90G8PsnnIuFw4bOkG8Wa6pwZI9a2+iJm9OgP0xXhBPuVjz+8o9x6kcelzHO6Qk+Dn1fShRbsXI88RS4NAA==

achow101 commented at 9:35 pm on July 1, 2025: member

ACK 6efbd1e1dcdfbe9eae2d5c22abab3ee616a75ff2

achow101 merged this on Jul 1, 2025

achow101 closed this on Jul 1, 2025

maflcko commented at 2:32 pm on July 2, 2025: member

Just for reference and fun, not as a pull request, the EqualsOptions patch would look something like:

  0diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h
  1index ad81727094..7eb8ded479 100644
  2--- a/src/primitives/transaction.h
  3+++ b/src/primitives/transaction.h
  4@@ -289,6 +289,10 @@ inline CAmount CalculateOutputValue(const TxType& tx)
  5 }
  6 
  7 
  8+struct EqualsOptions {
  9+    bool include_script_sig{true};
 10+    bool include_witness_data{true};
 11+};
 12 /** The basic transaction that is broadcasted on the network and contained in
 13  * blocks.  A transaction can contain multiple inputs and outputs.
 14  */
 15@@ -358,14 +362,14 @@ public:
 16         return (vin.size() == 1 && vin[0].prevout.IsNull());
 17     }
 18 
 19-    friend bool operator==(const CTransaction& a, const CTransaction& b)
 20-    {
 21-        return a.GetWitnessHash() == b.GetWitnessHash();
 22-    }
 23-
 24-    friend bool operator!=(const CTransaction& a, const CTransaction& b)
 25+    bool Equals(const CTransaction& other, const EqualsOptions opts = {}) const
 26     {
 27-        return !operator==(a, b);
 28+        return nLockTime == other.nLockTime && version == other.version && vout == other.vout &&
 29+               std::ranges::equal(vin, other.vin, [&opts](const CTxIn self, const CTxIn other) {
 30+                   return self.prevout == other.prevout && (opts.include_script_sig ? self.scriptSig == other.scriptSig : true) &&
 31+                          self.nSequence == other.nSequence &&
 32+                          (opts.include_witness_data ? self.scriptWitness.stack == other.scriptWitness.stack : true);
 33+               });
 34     }
 35 
 36     std::string ToString() const;
 37diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
 38index 9c26e5c733..c6662220ba 100644
 39--- a/src/rpc/rawtransaction.cpp
 40+++ b/src/rpc/rawtransaction.cpp
 41@@ -401,7 +401,7 @@ static RPCHelpMan getrawtransaction()
 42     }
 43 
 44     CTxUndo* undoTX {nullptr};
 45-    auto it = std::find_if(block.vtx.begin(), block.vtx.end(), [tx](CTransactionRef t){ return *t == *tx; });
 46+    auto it = std::find_if(block.vtx.begin(), block.vtx.end(), [tx](CTransactionRef t){ return t->Equals(*tx); });
 47     if (it != block.vtx.end()) {
 48         // -1 as blockundo does not have coinbase tx
 49         undoTX = &blockUndo.vtxundo.at(it - block.vtx.begin() - 1);
 50diff --git a/src/test/fuzz/primitives_transaction.cpp b/src/test/fuzz/primitives_transaction.cpp
 51index 64f1c8cf8d..7bc774b5e1 100644
 52--- a/src/test/fuzz/primitives_transaction.cpp
 53+++ b/src/test/fuzz/primitives_transaction.cpp
 54@@ -29,6 +29,6 @@ FUZZ_TARGET(primitives_transaction)
 55     if (mutable_tx_1 && mutable_tx_2) {
 56         const CTransaction tx_1{*mutable_tx_1};
 57         const CTransaction tx_2{*mutable_tx_2};
 58-        assert((tx_1 == tx_2) != (tx_1 != tx_2));
 59+        (void)tx_1.Equals(tx_2);
 60     }
 61 }
 62diff --git a/src/test/serialize_tests.cpp b/src/test/serialize_tests.cpp
 63index 33a9133d2e..e56a0be73b 100644
 64--- a/src/test/serialize_tests.cpp
 65+++ b/src/test/serialize_tests.cpp
 66@@ -57,7 +57,7 @@ public:
 67                boolval == rhs.boolval &&
 68                stringval == rhs.stringval &&
 69                strcmp(charstrval, rhs.charstrval) == 0 &&
 70-               *txval == *rhs.txval;
 71+               txval->Equals(*rhs.txval);
 72     }
 73 };
 74 
 75diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
 76index 54b3216a4c..e745b08d39 100644
 77--- a/src/test/transaction_tests.cpp
 78+++ b/src/test/transaction_tests.cpp
 79@@ -738,7 +738,7 @@ BOOST_AUTO_TEST_CASE(test_witness)
 80     CheckWithFlag(output1, input1, SCRIPT_VERIFY_NONE, false);
 81     CreateCreditAndSpend(keystore2, scriptMulti, output2, input2, false);
 82     CheckWithFlag(output2, input2, SCRIPT_VERIFY_NONE, false);
 83-    BOOST_CHECK(*output1 == *output2);
 84+    BOOST_CHECK(output1->Equals(*output2));
 85     UpdateInput(input1.vin[0], CombineSignatures(input1, input2, output1));
 86     CheckWithFlag(output1, input1, STANDARD_SCRIPT_VERIFY_FLAGS, true);
 87 
 88@@ -749,7 +749,7 @@ BOOST_AUTO_TEST_CASE(test_witness)
 89     CreateCreditAndSpend(keystore2, GetScriptForDestination(ScriptHash(scriptMulti)), output2, input2, false);
 90     CheckWithFlag(output2, input2, SCRIPT_VERIFY_NONE, true);
 91     CheckWithFlag(output2, input2, SCRIPT_VERIFY_P2SH, false);
 92-    BOOST_CHECK(*output1 == *output2);
 93+    BOOST_CHECK(output1->Equals(*output2));
 94     UpdateInput(input1.vin[0], CombineSignatures(input1, input2, output1));
 95     CheckWithFlag(output1, input1, SCRIPT_VERIFY_P2SH, true);
 96     CheckWithFlag(output1, input1, STANDARD_SCRIPT_VERIFY_FLAGS, true);
 97@@ -761,7 +761,7 @@ BOOST_AUTO_TEST_CASE(test_witness)
 98     CreateCreditAndSpend(keystore2, destination_script_multi, output2, input2, false);
 99     CheckWithFlag(output2, input2, SCRIPT_VERIFY_NONE, true);
100     CheckWithFlag(output2, input2, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, false);
101-    BOOST_CHECK(*output1 == *output2);
102+    BOOST_CHECK(output1->Equals(*output2));
103     UpdateInput(input1.vin[0], CombineSignatures(input1, input2, output1));
104     CheckWithFlag(output1, input1, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true);
105     CheckWithFlag(output1, input1, STANDARD_SCRIPT_VERIFY_FLAGS, true);
106@@ -773,7 +773,7 @@ BOOST_AUTO_TEST_CASE(test_witness)
107     CreateCreditAndSpend(keystore2, GetScriptForDestination(ScriptHash(destination_script_multi)), output2, input2, false);
108     CheckWithFlag(output2, input2, SCRIPT_VERIFY_P2SH, true);
109     CheckWithFlag(output2, input2, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, false);
110-    BOOST_CHECK(*output1 == *output2);
111+    BOOST_CHECK(output1->Equals(*output2));
112     UpdateInput(input1.vin[0], CombineSignatures(input1, input2, output1));
113     CheckWithFlag(output1, input1, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true);
114     CheckWithFlag(output1, input1, STANDARD_SCRIPT_VERIFY_FLAGS, true);
115diff --git a/src/wallet/transaction.cpp b/src/wallet/transaction.cpp
116index a611a034d9..13ef690403 100644
117--- a/src/wallet/transaction.cpp
118+++ b/src/wallet/transaction.cpp
119@@ -11,17 +11,7 @@ using interfaces::FoundBlock;
120 namespace wallet {
121 bool CWalletTx::IsEquivalentTo(const CWalletTx& _tx) const
122 {
123-        CMutableTransaction tx1 {*this->tx};
124-        CMutableTransaction tx2 {*_tx.tx};
125-        for (auto& txin : tx1.vin) {
126-            txin.scriptSig = CScript();
127-            txin.scriptWitness.SetNull();
128-        }
129-        for (auto& txin : tx2.vin) {
130-            txin.scriptSig = CScript();
131-            txin.scriptWitness.SetNull();
132-        }
133-        return CTransaction(tx1) == CTransaction(tx2);
134+    return tx->Equals(*_tx.tx, {.include_script_sig = false, .include_witness_data = false});
135 }
136 
137 bool CWalletTx::InMempool() const

Refactor: Redefine CTransaction equality to include witness data #32723

Code Coverage & Benchmarks

Reviews