Merge bitcoin/bitcoin#32723: Refactor: Redefine CTransaction equality to include witness data

6efbd1e1dc refactor: CTransaction equality should consider witness data (Cory Fields)
cbf9b2dab1 mempool: codify existing assumption about duplicate txids during removal (Cory Fields)
e9331cd6ab wallet: IsEquivalentTo should strip witness data in addition to scriptsigs (Cory Fields)

Pull request description:

  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.

ACKs for top commit:
  maflcko:
    review ACK 6efbd1e1dc 🦋
  achow101:
    ACK 6efbd1e1dc
  glozow:
    ACK 6efbd1e1dc

Tree-SHA512: 89be424889f49e7e26dd2bdab7fbc8b2def59bf002ae8b94989b349ce97245f007d6c96e409a626cbf0de9df83ae2485b4815b40a70f7aa5b6c720eb34a6c017

src/primitives/transaction.h

@@ -360,12 +360,12 @@ public:
 
     friend bool operator==(const CTransaction& a, const CTransaction& b)
     {
-        return a.hash == b.hash;
+        return a.GetWitnessHash() == b.GetWitnessHash();
     }
 
     friend bool operator!=(const CTransaction& a, const CTransaction& b)
     {
-        return a.hash != b.hash;
+        return !operator==(a, b);
     }
 
     std::string ToString() const;

src/txmempool.cpp

@@ -652,7 +652,7 @@ void CTxMemPool::removeConflicts(const CTransaction &tx)
         auto it = mapNextTx.find(txin.prevout);
         if (it != mapNextTx.end()) {
             const CTransaction &txConflict = *it->second;
-            if (txConflict != tx)
+            if (Assume(txConflict.GetHash() != tx.GetHash()))
             {
                 ClearPrioritisation(txConflict.GetHash());
                 removeRecursive(txConflict, MemPoolRemovalReason::CONFLICT);

src/wallet/transaction.cpp

@@ -13,8 +13,14 @@ bool CWalletTx::IsEquivalentTo(const CWalletTx& _tx) const
 {
         CMutableTransaction tx1 {*this->tx};
         CMutableTransaction tx2 {*_tx.tx};
-        for (auto& txin : tx1.vin) txin.scriptSig = CScript();
-        for (auto& txin : tx2.vin) txin.scriptSig = CScript();
+        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);
 }