Merge bitcoin/bitcoin#27145: wallet: when a block is disconnected, update transactions that are no longer conflicted

89df798 Add wallets_conflicts (Antoine Riard)
dced203 wallet, tests: mark unconflicted txs as inactive (ishaanam)
096487c wallet: introduce generic recursive tx state updating function (ishaanam)

Pull request description:

  This implements a fix for #7315. Previously when a block was disconnected any transactions that were conflicting with transactions mined in that block were not updated to be marked as inactive. The fix implemented here is described on the [Bitcoin DevWiki](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking#idea-refresh-conflicted). A test which tested the previous behavior has also been updated.

  Second attempt at #17543

ACKs for top commit:
  achow101:
    ACK 89df798
  rajarshimaitra:
    tACK 89df798.
  glozow:
    ACK 89df798
  furszy:
    Tested ACK 89df798

Tree-SHA512: 3133b151477e8818302fac29e96de30cd64c09a8fe3a7612074a34ba1a332e69148162e6cb3f1074d9d9c9bab5ef9996967d325d8c4c99ba42b5fe3b66a60546

Author: achow101

src/wallet/wallet.cpp

@@ -1266,11 +1266,6 @@ bool CWallet::AbandonTransaction(const uint256& hashTx)
 {
     LOCK(cs_wallet);
 
-    WalletBatch batch(GetDatabase());
-
-    std::set<uint256> todo;
-    std::set<uint256> 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<TxSpends::const_iterator, TxSpends::const_iterator> 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<uint256> todo;
     std::set<uint256> 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<TxSpends::const_iterator, TxSpends::const_iterator> 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<TxSpends::const_iterator, TxSpends::const_iterator> 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<TxStateConflicted>()->conflicting_block_height >= disconnect_height) {
+                        tx.m_state = TxStateInactive{};
+                        return TxUpdate::CHANGED;
+                    }
+                    return TxUpdate::UNCHANGED;
+                };
+
+                RecursiveUpdateTxState(wtx.tx->GetHash(), try_updating_state);
+            }
+        }
     }
 }
 

src/wallet/wallet.h

@@ -334,6 +334,13 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     /** 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<TxUpdate(CWalletTx& wtx)>;
+
+    /** 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);
 

test/functional/test_runner.py

@@ -196,6 +196,8 @@
     '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',

test/functional/wallet_abandonconflict.py

@@ -226,20 +226,16 @@ def run_test(self):
         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()

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()