removeprunedfunds removes all entries from mapTxSpends for the inputs of the pruned tx. However, this is incorrect, because there could be multiple entries from conflicting transactions (that shouldn’t be removed as well). This could lead to the wallet creating invalid transactions, trying to double spend utxos.
The bug persists when the conflicting tx was mined, because the wallet trusts its internal accounting instead of calling AddToSpends again.
The added test should fail on master.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34358.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Concept ACK
Confirmed that the new test fails on master and reviewed the test code. Need to spend some extra time with the wallet code.
138+ addr = node.getnewaddress()
139+ tx1_id = node.send(outputs=[{addr: 1}], inputs=[utxo])["txid"]
140+ tx1_fee = node.gettransaction(tx1_id)["fee"]
141+
142+ # Create a conflicting tx
143+ output_value = utxo["amount"] + tx1_fee - Decimal("0.00001")
I was puzzled by this but then figured out that tx1_fee is negative. Maybe clarify that:
0 # Create a conflicting tx with a larger fee (tx1_fee is negative)
1 output_value = utxo["amount"] + tx1_fee - Decimal("0.00001")
2403+ for (const auto& txin : it->second.tx->vin) {
2404+ auto range = mapTxSpends.equal_range(txin.prevout);
2405+ for (auto iter = range.first; iter != range.second; ++iter) {
2406+ if (iter->second == hash) {
2407+ mapTxSpends.erase(iter);
2408+ break;
mapTxSpends that it cannot contain duplicate entries - same key and same value, right? This constraint is not enforced by CWallet::AddToSpends().
CWallet::AddToSpends(): e.g. AddToWallet() only calls AddToSpend() if fInsertedNew is set. I think if duplicates would occur, it would be a bug elsewhere. If people think that we need extra defensiveness here I could change the fix to iterate over all entries, but I think I prefer the current version.
CWallet::AddToSpends(), which seems unrelated, or at least out of scope of this PR.
13@@ -14,6 +14,7 @@
14 from test_framework.test_framework import BitcoinTestFramework
15 from test_framework.util import (
16 assert_equal,
At the end of the commit message:
… instead of calling AddToSpends again
I am not sure I understand that. Do you imply that there is another solution, or that it would be better if AddToSpends() is called at some point “again”?
I meant that it would be conceivable that during block connection, the wallet would correct its mistake, and add the tx to AddToSpends() then, even if was wrong before when the txns were not confirmed. However, this is not the case, so the tx would be missing from mapTxSpends indefinitely. Which makes sense because calling AddToSpends() multiple times could create duplicates (see above), but wasn’t obvious to me from a high-level point of view.
Besides, it could maybe make sense to refactor the whole mapTxSpends logic in a follow-up to make it less mistake-prone, but I think that would better be a separate PR.
ACK 372843c0167438ebc67882663b991f2363a53c5a
I studied the code around this and the fix looks sound. mapTxSpends could contain more than one entry for the given previout and we want to delete not all of them but just the one that corresponds to the removed transaction.
I verified that the newly added test passes with the changes in this PR and fails without them.
Also, this change fixes the failure in https://github.com/bitcoin/bitcoin/pull/34359
removeprunedfunds removes all entries from mapTxSpends for the
inputs of the pruned tx. However, this is incorrect, because there could be
multiple entries from conflicting transactions (that shouldn't be
removed as well). This could lead to the wallet creating invalid
transactions, trying to double spend utxos.
The bug persists when the conflicting tx was mined, because
the wallet trusts its internal accounting instead of calling
AddToSpends again.
tACK 1f60ca360eb83fa7982b1aac402eaaf477294197
Reviewed the code and ensured that the new test fails on master. Also played around with the test code to confirm that it works with multiple inputs as well etc.
2397@@ -2398,8 +2398,15 @@ util::Result<void> CWallet::RemoveTxs(WalletBatch& batch, std::vector<Txid>& txs
2398 for (const auto& it : erased_txs) {
2399 const Txid hash{it->first};
2400 wtxOrdered.erase(it->second.m_it_wtxOrdered);
2401- for (const auto& txin : it->second.tx->vin)
2402- mapTxSpends.erase(txin.prevout);
2403+ for (const auto& txin : it->second.tx->vin) {
2404+ auto range = mapTxSpends.equal_range(txin.prevout);
2405+ for (auto iter = range.first; iter != range.second; ++iter) {
nano nit:
0 auto [first, last] = mapTxSpends.equal_range(txin.prevout);
1 for (auto iter = first; iter != last; ++iter) {
146+ tx2_id = node.sendrawtransaction(signed_tx2["hex"])
147+ assert_not_equal(tx2_id, tx1_id)
148+
149+ # Both txs should be in the wallet, tx2 replaced tx1 in mempool
150+ assert tx1_id in [tx["txid"] for tx in node.listtransactions()]
151+ assert tx2_id in [tx["txid"] for tx in node.listtransactions()]
nano nit: One RPC call instead of two.
0available_txs_ids = {tx["txid"] for tx in node.listtransactions()}
1assert tx1_id in available_txs_ids
2assert tx2_id in available_txs_ids
153+ # Remove the replaced tx from wallet
154+ node.removeprunedfunds(tx1_id)
155+
156+ # The UTXO should still be considered spent (by tx2)
157+ available_utxos = [u["txid"] for u in node.listunspent(minconf=0)]
158+ assert utxo["txid"] not in available_utxos, "UTXO should still be spent by conflicting tx"
