Merge bitcoin/bitcoin#35930: wallet: post-#35501 cleanups in CWalletTx

4ca182ca40 doc: clarify alternate_wtxids is empty when only one witness variant (pablomartin4btc)
fa48b5d28e test: assert listsinceblock "removed" reports current canonical wtxid (pablomartin4btc)
9b96ee1288 wallet, test: add unit test for variant txid validation in CWalletTx deserializer (pablomartin4btc)
9de6543cb5 wallet: post-#35501 cleanup in CWalletTx (pablomartin4btc)

Pull request description:

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

  Commit breakdown:

  1. _post-[#35501](https://github.com/bitcoin/bitcoin/pull/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](https://github.com/bitcoin/bitcoin/pull/35501#pullrequestreview-4854519083) 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](https://github.com/bitcoin/bitcoin/pull/35501#discussion_r3632044472) by w0xlt.

  4. _clarify_ `alternate_wtxids` _is empty when only one witness variant_
     [Suggested](https://github.com/bitcoin/bitcoin/pull/35501#discussion_r3632113003) by polespinasa.

ACKs for top commit:
  jeanpablojp:
    re-ACK 4ca182ca40
  achow101:
    ACK 4ca182ca40
  polespinasa:
    ACK 4ca182ca40

Tree-SHA512: 64eadeb11372d904c79edbfd264c4d8dc1b4fe4ce5e3acc301bfeba9e556efb5dce2c684f0e58c7b74e2c687cc7dd77389970662b3fd629ed034a97bcfdfb71c

doc/release-notes-35501.md

@@ -1,4 +1,4 @@
 RPC
 ---
 
-- `gettransaction`, `listtransactions`, and `listsinceblock` now have an `alternate_wtxids` field which lists the wtxids of all transactions that have the same txid.
+- `gettransaction`, `listtransactions`, and `listsinceblock` now have an `alternate_wtxids` field which lists the wtxids of all transactions that have the same txid. When there is only one known witness variant the field is an empty array, analogous to `walletconflicts` and `mempoolconflicts`.

src/wallet/test/wallet_transaction_tests.cpp

@@ -4,6 +4,9 @@
 
 #include <wallet/transaction.h>
 
+#include <primitives/transaction.h>
+#include <serialize.h>
+#include <streams.h>
 #include <test/util/common.h>
 #include <wallet/test/wallet_test_fixture.h>
 
@@ -23,5 +26,34 @@ BOOST_AUTO_TEST_CASE(roundtrip)
     }
 }
 
+BOOST_AUTO_TEST_CASE(deserialize_rejects_mismatched_variant_txid)
+{
+    // Build tx_a and serialise it as a CWalletTx.
+    // Needs at least one input: a zero-input tx serialises vin_count as 0x00,
+    // which the witness-aware deserialiser misreads as the segwit marker byte.
+    CMutableTransaction mtx_a;
+    mtx_a.vin.emplace_back(COutPoint{Txid::FromUint256(uint256::ONE), 0});
+    mtx_a.vout.emplace_back(COIN, CScript() << OP_TRUE);
+    CTransactionRef tx_a = MakeTransactionRef(std::move(mtx_a));
+    CWalletTx wtx_a{tx_a, TxStateInactive{}};
+    DataStream ss;
+    ss << wtx_a;
+
+    // Build tx_b with a different txid to use as a bogus variant.
+    CMutableTransaction mtx_b;
+    mtx_b.vout.emplace_back(2 * COIN, CScript() << OP_TRUE);
+    CTransactionRef tx_b = MakeTransactionRef(std::move(mtx_b));
+    BOOST_REQUIRE(tx_b->GetHash() != tx_a->GetHash());
+
+    // A variant whose txid doesn't match the canonical txid must be rejected.
+    std::map<Wtxid, CTransactionRef> bad_variants{{tx_b->GetWitnessHash(), tx_b}};
+    try {
+        CWalletTx(deserialize, ss, bad_variants);
+        BOOST_FAIL("expected std::runtime_error was not thrown");
+    } catch (const std::runtime_error& e) {
+        BOOST_CHECK_EQUAL(std::string(e.what()), "variant txid does not match wallet txid");
+    }
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 } // namespace wallet

src/wallet/transaction.cpp

@@ -104,24 +104,8 @@ void CWalletTx::RecomputeCanonical()
     // the least weight.
     Assert(!m_txs.empty());
 
-    // Returns true if 'a' should be preferred over 'b'
-    auto is_better = [](const CTransactionRef& a, const CTransactionRef& b) {
-        // A witnessed variant always beats a witnessless one
-        if (a->HasWitness() != b->HasWitness()) return a->HasWitness();
-        // Otherwise the lighter one wins
-        return GetTransactionWeight(*a) < GetTransactionWeight(*b);
-    };
-
-    auto it = m_txs.begin();
-    auto best_wtxid = it->first;
-    const CTransactionRef* best = &it->second;
-    it = std::next(it);
-    for (; it != m_txs.end(); it = std::next(it)) {
-        if (is_better(it->second, *best)) {
-            best = &it->second;
-            best_wtxid = it->first;
-        }
-    }
-    m_canonical_wtxid = best_wtxid;
+    m_canonical_wtxid = std::ranges::min_element(m_txs, std::less{}, [](const auto& entry) {
+                            return std::make_pair(!entry.second->HasWitness(), GetTransactionWeight(*entry.second));
+                        })->first;
 }
 } // namespace wallet

src/wallet/transaction.h

@@ -239,27 +239,22 @@ public:
         Assert(tx);
         m_canonical_wtxid = tx->GetWitnessHash();
         m_txs.emplace(tx->GetWitnessHash(), std::move(tx));
-        Init();
+        SetDefaults();
     }
 
     template <typename Stream>
     CWalletTx(deserialize_type, Stream& s, const std::map<Wtxid, CTransactionRef>& variants) : m_state(TxStateInactive{})
     {
         Unserialize(s);
+        const Txid& canonical_txid = GetHash();
+        for (const auto& [wtxid, tx] : variants) {
+            if (tx->GetHash() != canonical_txid) throw std::runtime_error("variant txid does not match wallet txid");
+        }
         // Merge witness variants
         m_txs.insert(variants.begin(), variants.end());
         Assert(m_txs.contains(GetWitnessHash()));
     }
 
-    void Init()
-    {
-        nTimeReceived = 0;
-        nTimeSmart = 0;
-        fChangeCached = false;
-        nChangeCached = 0;
-        nOrderPos = -1;
-    }
-
     TxState m_state;
 
     // Set of mempool transactions that conflict
@@ -357,7 +352,7 @@ public:
     // If the given transaction has a different wtxid, the transaction is stored if it has not been seen before.
     // The canonical wtxid is also updated. The tx that is confirmed becomes canonical. For unconfirmed txs,
     // those with witnesses are preferred, followed by least weight.
-    bool Update(CTransactionRef tx, const TxState& arg_state);
+    bool Update(CTransactionRef tx, const TxState& new_state);
 
     //! make sure balances are recalculated
     void MarkDirty()
@@ -405,6 +400,22 @@ public:
     CWalletTx(CWalletTx&&) = default;
 
 private:
+    void SetDefaults()
+    {
+        nTimeReceived = 0;
+        nTimeSmart = 0;
+        fChangeCached = false;
+        nChangeCached = 0;
+        nOrderPos = -1;
+    }
+
+    void Init()
+    {
+        m_txs.clear();
+        m_canonical_wtxid = Wtxid{};
+        SetDefaults();
+    }
+
     Wtxid m_canonical_wtxid;
     std::map<Wtxid, CTransactionRef> m_txs;
 

test/functional/wallet_listtransactions.py

@@ -356,6 +356,16 @@ class ListTransactionsTest(BitcoinTestFramework):
         assert_equal(wallet.gettransaction(txid)["confirmations"], 0)
         self.check_tx_variants(wallet, txid, key_path_tx, key_path_wtxid, alternate_wtxids=[script_path_wtxid])
 
+        # listsinceblock "removed" entries reflect the wallet's current CWalletTx, not a
+        # snapshot of the detached block. The detached block contained the heavier script
+        # path variant, but "wtxid" reports the current canonical (key path) variant and
+        # the script path variant appears under "alternate_wtxids". A future improvement
+        # could track which specific variant was in the detached block and report that.
+        removed = next(e for e in wallet.listsinceblock(block)["removed"] if e["txid"] == txid)
+        assert_equal(removed["confirmations"], 0)
+        assert_equal(removed["wtxid"], key_path_wtxid)
+        assert_equal(removed["alternate_wtxids"], [script_path_wtxid])
+
 
 if __name__ == '__main__':
     ListTransactionsTest(__file__).main()