Refactor: Redefine CTransaction equality to include witness data #32723

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.

<!--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/32723.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

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

<!--5faf32d7da4f0f540f40219e4f7537a3-->

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

CWalletTx::IsEquivalentTo needs to consider malleated txs as equivalent, so the original behavior of ignoring the scriptSigs and scriptWitness is intended.

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.

ACK 6efbd1e1dcdfbe9eae2d5c22abab3ee616a75ff2

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

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

struct EqualsOptions{
 bool include_script_sig{true};
 bool include_witness_data{true};
};

review ACK 6efbd1e1dcdfbe9eae2d5c22abab3ee616a75ff2 🦋

<details><summary>Show signature</summary>

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 6efbd1e1dcdfbe9eae2d5c22abab3ee616a75ff2 🦋
N0uT90G8PsnnIuFw4bOkG8Wa6pwZI9a2+iJm9OgP0xXhBPuVjz+8o9x6kcelzHO6Qk+Dn1fShRbsXI88RS4NAA==

</details>

ACK 6efbd1e1dcdfbe9eae2d5c22abab3ee616a75ff2

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

diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h
index ad81727094..7eb8ded479 100644
--- a/src/primitives/transaction.h
+++ b/src/primitives/transaction.h
@@ -289,6 +289,10 @@ inline CAmount CalculateOutputValue(const TxType& tx)
 }
 
 
+struct EqualsOptions {
+    bool include_script_sig{true};
+    bool include_witness_data{true};
+};
 /** The basic transaction that is broadcasted on the network and contained in
  * blocks.  A transaction can contain multiple inputs and outputs.
  */
@@ -358,14 +362,14 @@ public:
         return (vin.size() == 1 && vin[0].prevout.IsNull());
     }
 
-    friend bool operator==(const CTransaction& a, const CTransaction& b)
-    {
-        return a.GetWitnessHash() == b.GetWitnessHash();
-    }
-
-    friend bool operator!=(const CTransaction& a, const CTransaction& b)
+    bool Equals(const CTransaction& other, const EqualsOptions opts = {}) const
     {
-        return !operator==(a, b);
+        return nLockTime == other.nLockTime && version == other.version && vout == other.vout &&
+               std::ranges::equal(vin, other.vin, [&opts](const CTxIn self, const CTxIn other) {
+                   return self.prevout == other.prevout && (opts.include_script_sig ? self.scriptSig == other.scriptSig : true) &&
+                          self.nSequence == other.nSequence &&
+                          (opts.include_witness_data ? self.scriptWitness.stack == other.scriptWitness.stack : true);
+               });
     }
 
     std::string ToString() const;
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 9c26e5c733..c6662220ba 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -401,7 +401,7 @@ static RPCHelpMan getrawtransaction()
     }
 
     CTxUndo* undoTX {nullptr};
-    auto it = std::find_if(block.vtx.begin(), block.vtx.end(), [tx](CTransactionRef t){ return *t == *tx; });
+    auto it = std::find_if(block.vtx.begin(), block.vtx.end(), [tx](CTransactionRef t){ return t->Equals(*tx); });
     if (it != block.vtx.end()) {
         // -1 as blockundo does not have coinbase tx
         undoTX = &blockUndo.vtxundo.at(it - block.vtx.begin() - 1);
diff --git a/src/test/fuzz/primitives_transaction.cpp b/src/test/fuzz/primitives_transaction.cpp
index 64f1c8cf8d..7bc774b5e1 100644
--- a/src/test/fuzz/primitives_transaction.cpp
+++ b/src/test/fuzz/primitives_transaction.cpp
@@ -29,6 +29,6 @@ FUZZ_TARGET(primitives_transaction)
     if (mutable_tx_1 && mutable_tx_2) {
         const CTransaction tx_1{*mutable_tx_1};
         const CTransaction tx_2{*mutable_tx_2};
-        assert((tx_1 == tx_2) != (tx_1 != tx_2));
+        (void)tx_1.Equals(tx_2);
     }
 }
diff --git a/src/test/serialize_tests.cpp b/src/test/serialize_tests.cpp
index 33a9133d2e..e56a0be73b 100644
--- a/src/test/serialize_tests.cpp
+++ b/src/test/serialize_tests.cpp
@@ -57,7 +57,7 @@ public:
                boolval == rhs.boolval &&
                stringval == rhs.stringval &&
                strcmp(charstrval, rhs.charstrval) == 0 &&
-               *txval == *rhs.txval;
+               txval->Equals(*rhs.txval);
     }
 };
 
diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
index 54b3216a4c..e745b08d39 100644
--- a/src/test/transaction_tests.cpp
+++ b/src/test/transaction_tests.cpp
@@ -738,7 +738,7 @@ BOOST_AUTO_TEST_CASE(test_witness)
     CheckWithFlag(output1, input1, SCRIPT_VERIFY_NONE, false);
     CreateCreditAndSpend(keystore2, scriptMulti, output2, input2, false);
     CheckWithFlag(output2, input2, SCRIPT_VERIFY_NONE, false);
-    BOOST_CHECK(*output1 == *output2);
+    BOOST_CHECK(output1->Equals(*output2));
     UpdateInput(input1.vin[0], CombineSignatures(input1, input2, output1));
     CheckWithFlag(output1, input1, STANDARD_SCRIPT_VERIFY_FLAGS, true);
 
@@ -749,7 +749,7 @@ BOOST_AUTO_TEST_CASE(test_witness)
     CreateCreditAndSpend(keystore2, GetScriptForDestination(ScriptHash(scriptMulti)), output2, input2, false);
     CheckWithFlag(output2, input2, SCRIPT_VERIFY_NONE, true);
     CheckWithFlag(output2, input2, SCRIPT_VERIFY_P2SH, false);
-    BOOST_CHECK(*output1 == *output2);
+    BOOST_CHECK(output1->Equals(*output2));
     UpdateInput(input1.vin[0], CombineSignatures(input1, input2, output1));
     CheckWithFlag(output1, input1, SCRIPT_VERIFY_P2SH, true);
     CheckWithFlag(output1, input1, STANDARD_SCRIPT_VERIFY_FLAGS, true);
@@ -761,7 +761,7 @@ BOOST_AUTO_TEST_CASE(test_witness)
     CreateCreditAndSpend(keystore2, destination_script_multi, output2, input2, false);
     CheckWithFlag(output2, input2, SCRIPT_VERIFY_NONE, true);
     CheckWithFlag(output2, input2, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, false);
-    BOOST_CHECK(*output1 == *output2);
+    BOOST_CHECK(output1->Equals(*output2));
     UpdateInput(input1.vin[0], CombineSignatures(input1, input2, output1));
     CheckWithFlag(output1, input1, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true);
     CheckWithFlag(output1, input1, STANDARD_SCRIPT_VERIFY_FLAGS, true);
@@ -773,7 +773,7 @@ BOOST_AUTO_TEST_CASE(test_witness)
     CreateCreditAndSpend(keystore2, GetScriptForDestination(ScriptHash(destination_script_multi)), output2, input2, false);
     CheckWithFlag(output2, input2, SCRIPT_VERIFY_P2SH, true);
     CheckWithFlag(output2, input2, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, false);
-    BOOST_CHECK(*output1 == *output2);
+    BOOST_CHECK(output1->Equals(*output2));
     UpdateInput(input1.vin[0], CombineSignatures(input1, input2, output1));
     CheckWithFlag(output1, input1, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true);
     CheckWithFlag(output1, input1, STANDARD_SCRIPT_VERIFY_FLAGS, true);
diff --git a/src/wallet/transaction.cpp b/src/wallet/transaction.cpp
index a611a034d9..13ef690403 100644
--- a/src/wallet/transaction.cpp
+++ b/src/wallet/transaction.cpp
@@ -11,17 +11,7 @@ using interfaces::FoundBlock;
 namespace wallet {
 bool CWalletTx::IsEquivalentTo(const CWalletTx& _tx) const
 {
-        CMutableTransaction tx1 {*this->tx};
-        CMutableTransaction tx2 {*_tx.tx};
-        for (auto& txin : tx1.vin) {
-            txin.scriptSig = CScript();
-            txin.scriptWitness.SetNull();
-        }
-        for (auto& txin : tx2.vin) {
-            txin.scriptSig = CScript();
-            txin.scriptWitness.SetNull();
-        }
-        return CTransaction(tx1) == CTransaction(tx2);
+    return tx->Equals(*_tx.tx, {.include_script_sig = false, .include_witness_data = false});
 }
 
 bool CWalletTx::InMempool() const