diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d5268afbf6..0b241c4ef4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1266,11 +1266,6 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) { LOCK(cs_wallet); - WalletBatch batch(GetDatabase()); - - std::set todo; - std::set done; - // Can't mark abandoned if confirmed or in mempool auto it = mapWallet.find(hashTx); assert(it != mapWallet.end()); @@ -1279,44 +1274,25 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) return false; } - todo.insert(hashTx); - - while (!todo.empty()) { - uint256 now = *todo.begin(); - todo.erase(now); - done.insert(now); - auto it = mapWallet.find(now); - assert(it != mapWallet.end()); - CWalletTx& wtx = it->second; - int currentconfirm = GetTxDepthInMainChain(wtx); - // If the orig tx was not in block, none of its spends can be - assert(currentconfirm <= 0); - // if (currentconfirm < 0) {Tx and spends are already conflicted, no need to abandon} - if (currentconfirm == 0 && !wtx.isAbandoned()) { - // If the orig tx was not in block/mempool, none of its spends can be in mempool - assert(!wtx.InMempool()); + auto try_updating_state = [](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { + // If the orig tx was not in block/mempool, none of its spends can be. + assert(!wtx.isConfirmed()); + assert(!wtx.InMempool()); + // If already conflicted or abandoned, no need to set abandoned + if (!wtx.isConflicted() && !wtx.isAbandoned()) { wtx.m_state = TxStateInactive{/*abandoned=*/true}; - wtx.MarkDirty(); - batch.WriteTx(wtx); - NotifyTransactionChanged(wtx.GetHash(), CT_UPDATED); - // Iterate over all its outputs, and mark transactions in the wallet that spend them abandoned too. - // States are not permanent, so these transactions can become unabandoned if they are re-added to the - // mempool, or confirmed in a block, or conflicted. - // Note: If the reorged coinbase is re-added to the main chain, the descendants that have not had their - // states change will remain abandoned and will require manual broadcast if the user wants them. - for (unsigned int i = 0; i < wtx.tx->vout.size(); ++i) { - std::pair range = mapTxSpends.equal_range(COutPoint(now, i)); - for (TxSpends::const_iterator iter = range.first; iter != range.second; ++iter) { - if (!done.count(iter->second)) { - todo.insert(iter->second); - } - } - } - // If a transaction changes 'conflicted' state, that changes the balance - // available of the outputs it spends. So force those to be recomputed - MarkInputsDirty(wtx.tx); + return TxUpdate::NOTIFY_CHANGED; } - } + return TxUpdate::UNCHANGED; + }; + + // Iterate over all its outputs, and mark transactions in the wallet that spend them abandoned too. + // States are not permanent, so these transactions can become unabandoned if they are re-added to the + // mempool, or confirmed in a block, or conflicted. + // Note: If the reorged coinbase is re-added to the main chain, the descendants that have not had their + // states change will remain abandoned and will require manual broadcast if the user wants them. + + RecursiveUpdateTxState(hashTx, try_updating_state); return true; } @@ -1333,13 +1309,29 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c if (conflictconfirms >= 0) return; + auto try_updating_state = [&](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { + if (conflictconfirms < GetTxDepthInMainChain(wtx)) { + // Block is 'more conflicted' than current confirm; update. + // Mark transaction as conflicted with this block. + wtx.m_state = TxStateConflicted{hashBlock, conflicting_height}; + return TxUpdate::CHANGED; + } + return TxUpdate::UNCHANGED; + }; + + // Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too. + RecursiveUpdateTxState(hashTx, try_updating_state); + +} + +void CWallet::RecursiveUpdateTxState(const uint256& tx_hash, const TryUpdatingStateFn& try_updating_state) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { // Do not flush the wallet here for performance reasons WalletBatch batch(GetDatabase(), false); std::set todo; std::set done; - todo.insert(hashTx); + todo.insert(tx_hash); while (!todo.empty()) { uint256 now = *todo.begin(); @@ -1348,14 +1340,12 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c auto it = mapWallet.find(now); assert(it != mapWallet.end()); CWalletTx& wtx = it->second; - int currentconfirm = GetTxDepthInMainChain(wtx); - if (conflictconfirms < currentconfirm) { - // Block is 'more conflicted' than current confirm; update. - // Mark transaction as conflicted with this block. - wtx.m_state = TxStateConflicted{hashBlock, conflicting_height}; + + TxUpdate update_state = try_updating_state(wtx); + if (update_state != TxUpdate::UNCHANGED) { wtx.MarkDirty(); batch.WriteTx(wtx); - // Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too + // Iterate over all its outputs, and update those tx states as well (if applicable) for (unsigned int i = 0; i < wtx.tx->vout.size(); ++i) { std::pair range = mapTxSpends.equal_range(COutPoint(now, i)); for (TxSpends::const_iterator iter = range.first; iter != range.second; ++iter) { @@ -1364,7 +1354,12 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c } } } - // If a transaction changes 'conflicted' state, that changes the balance + + if (update_state == TxUpdate::NOTIFY_CHANGED) { + NotifyTransactionChanged(wtx.GetHash(), CT_UPDATED); + } + + // If a transaction changes its tx state, that usually changes the balance // available of the outputs it spends. So force those to be recomputed MarkInputsDirty(wtx.tx); } @@ -1459,8 +1454,36 @@ void CWallet::blockDisconnected(const interfaces::BlockInfo& block) // future with a stickier abandoned state or even removing abandontransaction call. m_last_block_processed_height = block.height - 1; m_last_block_processed = *Assert(block.prev_hash); + + int disconnect_height = block.height; + for (const CTransactionRef& ptx : Assert(block.data)->vtx) { SyncTransaction(ptx, TxStateInactive{}); + + for (const CTxIn& tx_in : ptx->vin) { + // No other wallet transactions conflicted with this transaction + if (mapTxSpends.count(tx_in.prevout) < 1) continue; + + std::pair range = mapTxSpends.equal_range(tx_in.prevout); + + // For all of the spends that conflict with this transaction + for (TxSpends::const_iterator _it = range.first; _it != range.second; ++_it) { + CWalletTx& wtx = mapWallet.find(_it->second)->second; + + if (!wtx.isConflicted()) continue; + + auto try_updating_state = [&](CWalletTx& tx) { + if (!tx.isConflicted()) return TxUpdate::UNCHANGED; + if (tx.state()->conflicting_block_height >= disconnect_height) { + tx.m_state = TxStateInactive{}; + return TxUpdate::CHANGED; + } + return TxUpdate::UNCHANGED; + }; + + RecursiveUpdateTxState(wtx.tx->GetHash(), try_updating_state); + } + } } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index e74eeb0a99..7bf7264568 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -334,6 +334,13 @@ private: /** Mark a transaction (and its in-wallet descendants) as conflicting with a particular block. */ void MarkConflicted(const uint256& hashBlock, int conflicting_height, const uint256& hashTx); + enum class TxUpdate { UNCHANGED, CHANGED, NOTIFY_CHANGED }; + + using TryUpdatingStateFn = std::function; + + /** Mark a transaction (and its in-wallet descendants) as a particular tx state. */ + void RecursiveUpdateTxState(const uint256& tx_hash, const TryUpdatingStateFn& try_updating_state) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** Mark a transaction's inputs dirty, thus forcing the outputs to be recomputed */ void MarkInputsDirty(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index a6f1bbae7d..5895e1de84 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -196,6 +196,8 @@ BASE_SCRIPTS = [ 'wallet_watchonly.py --legacy-wallet', 'wallet_watchonly.py --usecli --legacy-wallet', 'wallet_reorgsrestore.py', + 'wallet_conflicts.py --legacy-wallet', + 'wallet_conflicts.py --descriptors', 'interface_http.py', 'interface_rpc.py', 'interface_usdt_coinselection.py', diff --git a/test/functional/wallet_abandonconflict.py b/test/functional/wallet_abandonconflict.py index 934f44588d..2691507773 100755 --- a/test/functional/wallet_abandonconflict.py +++ b/test/functional/wallet_abandonconflict.py @@ -226,20 +226,16 @@ class AbandonConflictTest(BitcoinTestFramework): assert_equal(double_spend["walletconflicts"], [txAB1]) # Verify that B and C's 10 BTC outputs are available for spending again because AB1 is now conflicted + assert_equal(alice.gettransaction(txAB1)["confirmations"], -1) newbalance = alice.getbalance() assert_equal(newbalance, balance + Decimal("20")) balance = newbalance - # There is currently a minor bug around this and so this test doesn't work. See Issue #7315 - # Invalidate the block with the double spend and B's 10 BTC output should no longer be available - # Don't think C's should either + # Invalidate the block with the double spend. B & C's 10 BTC outputs should no longer be available self.nodes[0].invalidateblock(self.nodes[0].getbestblockhash()) + assert_equal(alice.gettransaction(txAB1)["confirmations"], 0) newbalance = alice.getbalance() - #assert_equal(newbalance, balance - Decimal("10")) - self.log.info("If balance has not declined after invalidateblock then out of mempool wallet tx which is no longer") - self.log.info("conflicted has not resumed causing its inputs to be seen as spent. See Issue #7315") - assert_equal(balance, newbalance) - + assert_equal(newbalance, balance - Decimal("20")) if __name__ == '__main__': AbandonConflictTest().main() diff --git a/test/functional/wallet_conflicts.py b/test/functional/wallet_conflicts.py new file mode 100755 index 0000000000..802b718cd5 --- /dev/null +++ b/test/functional/wallet_conflicts.py @@ -0,0 +1,127 @@ +#!/usr/bin/env python3 +# Copyright (c) 2023 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +""" +Test that wallet correctly tracks transactions that have been conflicted by blocks, particularly during reorgs. +""" + +from decimal import Decimal + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, +) + +class TxConflicts(BitcoinTestFramework): + def add_options(self, parser): + self.add_wallet_options(parser) + + def set_test_params(self): + self.num_nodes = 3 + + def skip_test_if_missing_module(self): + self.skip_if_no_wallet() + + def get_utxo_of_value(self, from_tx_id, search_value): + return next(tx_out["vout"] for tx_out in self.nodes[0].gettransaction(from_tx_id)["details"] if tx_out["amount"] == Decimal(f"{search_value}")) + + def run_test(self): + self.log.info("Send tx from which to conflict outputs later") + txid_conflict_from_1 = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), Decimal("10")) + txid_conflict_from_2 = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), Decimal("10")) + self.generate(self.nodes[0], 1) + self.sync_blocks() + + self.log.info("Disconnect nodes to broadcast conflicts on their respective chains") + self.disconnect_nodes(0, 1) + self.disconnect_nodes(2, 1) + + self.log.info("Create transactions that conflict with each other") + output_A = self.get_utxo_of_value(from_tx_id=txid_conflict_from_1, search_value=10) + output_B = self.get_utxo_of_value(from_tx_id=txid_conflict_from_2, search_value=10) + + # First create a transaction that consumes both A and B outputs. + # + # | tx1 | -----> | | | | + # | AB_parent_tx | ----> | Child_Tx | + # | tx2 | -----> | | | | + # + inputs_tx_AB_parent = [{"txid": txid_conflict_from_1, "vout": output_A}, {"txid": txid_conflict_from_2, "vout": output_B}] + tx_AB_parent = self.nodes[0].signrawtransactionwithwallet(self.nodes[0].createrawtransaction(inputs_tx_AB_parent, {self.nodes[0].getnewaddress(): Decimal("19.99998")})) + + # Secondly, create two transactions: One consuming output_A, and another one consuming output_B + # + # | tx1 | -----> | Tx_A_1 | + # ---------------- + # | tx2 | -----> | Tx_B_1 | + # + inputs_tx_A_1 = [{"txid": txid_conflict_from_1, "vout": output_A}] + inputs_tx_B_1 = [{"txid": txid_conflict_from_2, "vout": output_B}] + tx_A_1 = self.nodes[0].signrawtransactionwithwallet(self.nodes[0].createrawtransaction(inputs_tx_A_1, {self.nodes[0].getnewaddress(): Decimal("9.99998")})) + tx_B_1 = self.nodes[0].signrawtransactionwithwallet(self.nodes[0].createrawtransaction(inputs_tx_B_1, {self.nodes[0].getnewaddress(): Decimal("9.99998")})) + + self.log.info("Broadcast conflicted transaction") + txid_AB_parent = self.nodes[0].sendrawtransaction(tx_AB_parent["hex"]) + self.generate(self.nodes[0], 1, sync_fun=self.no_op) + + # Now that 'AB_parent_tx' was broadcast, build 'Child_Tx' + output_c = self.get_utxo_of_value(from_tx_id=txid_AB_parent, search_value=19.99998) + inputs_tx_C_child = [({"txid": txid_AB_parent, "vout": output_c})] + + tx_C_child = self.nodes[0].signrawtransactionwithwallet(self.nodes[0].createrawtransaction(inputs_tx_C_child, {self.nodes[0].getnewaddress() : Decimal("19.99996")})) + tx_C_child_txid = self.nodes[0].sendrawtransaction(tx_C_child["hex"]) + self.generate(self.nodes[0], 1, sync_fun=self.no_op) + + self.log.info("Broadcast conflicting tx to node 1 and generate a longer chain") + conflicting_txid_A = self.nodes[1].sendrawtransaction(tx_A_1["hex"]) + self.generate(self.nodes[1], 4, sync_fun=self.no_op) + conflicting_txid_B = self.nodes[1].sendrawtransaction(tx_B_1["hex"]) + self.generate(self.nodes[1], 4, sync_fun=self.no_op) + + self.log.info("Connect nodes 0 and 1, trigger reorg and ensure that the tx is effectively conflicted") + self.connect_nodes(0, 1) + self.sync_blocks([self.nodes[0], self.nodes[1]]) + conflicted_AB_tx = self.nodes[0].gettransaction(txid_AB_parent) + tx_C_child = self.nodes[0].gettransaction(tx_C_child_txid) + conflicted_A_tx = self.nodes[0].gettransaction(conflicting_txid_A) + + self.log.info("Verify, after the reorg, that Tx_A was accepted, and tx_AB and its Child_Tx are conflicting now") + # Tx A was accepted, Tx AB was not. + assert conflicted_AB_tx["confirmations"] < 0 + assert conflicted_A_tx["confirmations"] > 0 + + # Conflicted tx should have confirmations set to the confirmations of the most conflicting tx + assert_equal(-conflicted_AB_tx["confirmations"], conflicted_A_tx["confirmations"]) + # Child should inherit conflicted state from parent + assert_equal(-tx_C_child["confirmations"], conflicted_A_tx["confirmations"]) + # Check the confirmations of the conflicting transactions + assert_equal(conflicted_A_tx["confirmations"], 8) + assert_equal(self.nodes[0].gettransaction(conflicting_txid_B)["confirmations"], 4) + + self.log.info("Now generate a longer chain that does not contain any tx") + # Node2 chain without conflicts + self.generate(self.nodes[2], 15, sync_fun=self.no_op) + + # Connect node0 and node2 and wait reorg + self.connect_nodes(0, 2) + self.sync_blocks() + conflicted = self.nodes[0].gettransaction(txid_AB_parent) + tx_C_child = self.nodes[0].gettransaction(tx_C_child_txid) + + self.log.info("Test that formerly conflicted transaction are inactive after reorg") + # Former conflicted tx should be unconfirmed as it hasn't been yet rebroadcast + assert_equal(conflicted["confirmations"], 0) + # Former conflicted child tx should be unconfirmed as it hasn't been rebroadcast + assert_equal(tx_C_child["confirmations"], 0) + # Rebroadcast former conflicted tx and check it confirms smoothly + self.nodes[2].sendrawtransaction(conflicted["hex"]) + self.generate(self.nodes[2], 1) + self.sync_blocks() + former_conflicted = self.nodes[0].gettransaction(txid_AB_parent) + assert_equal(former_conflicted["confirmations"], 1) + assert_equal(former_conflicted["blockheight"], 217) + +if __name__ == '__main__': + TxConflicts().main()