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`. 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 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; 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()