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

### 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 @@ class CWalletTx
         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 @@ class CWalletTx
     // 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 @@ class CWalletTx
     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 @@ def test_alternate_witness_tx(self):
         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()