From 9de6543cb55ff43aea950712a3148fed9d4a7e13 Mon Sep 17 00:00:00 2001 From: pablomartin4btc Date: Thu, 6 Aug 2026 23:30:07 -0300 Subject: [PATCH 1/4] wallet: post-#35501 cleanup in CWalletTx - Rename arg_state to new_state in Update() declaration to match implementation - Replace RecomputeCanonical manual loop with std::ranges::min_element - Add variant txid validation in the deserialize constructor - Make Init() private and have it clear all members including m_txs Co-authored-by: Anthony Towns --- src/wallet/transaction.cpp | 22 +++------------------- src/wallet/transaction.h | 33 ++++++++++++++++++++++----------- 2 files changed, 25 insertions(+), 30 deletions(-) diff --git a/src/wallet/transaction.cpp b/src/wallet/transaction.cpp index 10746d09c7b..26cbf55fe1b 100644 --- a/src/wallet/transaction.cpp +++ b/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 diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h index d57f24acf1b..c00be2877ba 100644 --- a/src/wallet/transaction.h +++ b/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 CWalletTx(deserialize_type, Stream& s, const std::map& 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 m_txs; From 9b96ee12881ef38a221f34ce26a3b4fcb5622b4b Mon Sep 17 00:00:00 2001 From: pablomartin4btc Date: Fri, 7 Aug 2026 00:56:08 -0300 Subject: [PATCH 2/4] wallet, test: add unit test for variant txid validation in CWalletTx deserializer --- src/wallet/test/wallet_transaction_tests.cpp | 32 ++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/wallet/test/wallet_transaction_tests.cpp b/src/wallet/test/wallet_transaction_tests.cpp index 38be52f41f4..19129dad0e8 100644 --- a/src/wallet/test/wallet_transaction_tests.cpp +++ b/src/wallet/test/wallet_transaction_tests.cpp @@ -4,6 +4,9 @@ #include +#include +#include +#include #include #include @@ -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 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 From fa48b5d28eb5c326115af246b12ff644279172c4 Mon Sep 17 00:00:00 2001 From: pablomartin4btc Date: Fri, 7 Aug 2026 01:24:03 -0300 Subject: [PATCH 3/4] test: assert listsinceblock "removed" reports current canonical wtxid When a block is detached, listsinceblock "removed" entries reflect the wallet's current CWalletTx rather than a snapshot of the variant that was actually in the detached block. Add assertions to make this behaviour explicit. A future followup could improve listsinceblock to track and report the specific witness variant that was in the disconnected block (requires per-block tracking of which witness variant was included). Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com> --- test/functional/wallet_listtransactions.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/functional/wallet_listtransactions.py b/test/functional/wallet_listtransactions.py index 0391109cc5c..f48f0cfc52f 100755 --- a/test/functional/wallet_listtransactions.py +++ b/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() From 4ca182ca4028b9e681d65ec21f79fd7fed3ce215 Mon Sep 17 00:00:00 2001 From: pablomartin4btc Date: Fri, 7 Aug 2026 01:24:09 -0300 Subject: [PATCH 4/4] doc: clarify alternate_wtxids is empty when only one witness variant When there is only one known witness variant for a transaction, alternate_wtxids is an empty array, analogous to walletconflicts and mempoolconflicts. Suggested-by: polespinasa --- doc/release-notes-35501.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/release-notes-35501.md b/doc/release-notes-35501.md index 2294d63cd70..2399ca1edf8 100644 --- a/doc/release-notes-35501.md +++ b/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`.