Merge bitcoin/bitcoin#31794: wallet: abandon orphan coinbase txs, and their descendants, during startup

e4dd5a3 test: wallet, abandon coinbase txs and their descendants during startup (furszy)
474139a wallet: abandon inactive coinbase tx and their descendants during startup (furszy)

Pull request description:

  Since #26499, we mark coinbase transactions and their descendants as abandoned when a reorg arises through the "block disconnection" signal handler. However, this does not cover all scenarios; external wallets could contain coinbase transactions from blocks the node has not seen yet, or the user could have replaced the chain with an earlier or different version (one without the coinbase chain).

  This affects balance calculation as well as mempool rebroadcast (descendants shouldn't be relayed).
  Fix this by marking orphaned coinbase transactions and their descendants as abandoned during wallet startup.

ACKs for top commit:
  achow101:
    ACK e4dd5a3
  rkrux:
    tACK e4dd5a3
  mzumsande:
    Code Review ACK e4dd5a3

Tree-SHA512: 461a43de7a6f5a580f2e6e3b56ec9bc92239cd45e850a2ff594ab5488dcd4a507f68fbbf550a33d7173b2add0de80de1e1b3841e1dfab0c95b284212d8ced08a

Author: achow101

src/wallet/wallet.cpp

@@ -1315,12 +1315,15 @@ void CWallet::MarkInputsDirty(const CTransactionRef& tx)
 bool CWallet::AbandonTransaction(const uint256& hashTx)
 {
     LOCK(cs_wallet);
-
-    // Can't mark abandoned if confirmed or in mempool
     auto it = mapWallet.find(hashTx);
     assert(it != mapWallet.end());
-    const CWalletTx& origtx = it->second;
-    if (GetTxDepthInMainChain(origtx) != 0 || origtx.InMempool()) {
+    return AbandonTransaction(it->second);
+}
+
+bool CWallet::AbandonTransaction(CWalletTx& tx)
+{
+    // Can't mark abandoned if confirmed or in mempool
+    if (GetTxDepthInMainChain(tx) != 0 || tx.InMempool()) {
         return false;
     }
 
@@ -1342,7 +1345,7 @@ bool CWallet::AbandonTransaction(const uint256& hashTx)
     // 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);
+    RecursiveUpdateTxState(tx.GetHash(), try_updating_state);
 
     return true;
 }

src/wallet/wallet.h

@@ -871,6 +871,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
 
     /* Mark a transaction (and it in-wallet descendants) as abandoned so its inputs may be respent. */
     bool AbandonTransaction(const uint256& hashTx);
+    bool AbandonTransaction(CWalletTx& tx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     /** Mark a transaction as replaced by another transaction. */
     bool MarkReplaced(const uint256& originalHash, const uint256& newHash);

src/wallet/walletdb.cpp

@@ -1115,6 +1115,14 @@ static DBErrors LoadTxRecords(CWallet* pwallet, DatabaseBatch& batch, std::vecto
     });
     result = std::max(result, order_pos_res.m_result);
 
+    // After loading all tx records, abandon any coinbase that is no longer in the active chain.
+    // This could happen during an external wallet load, or if the user replaced the chain data.
+    for (auto& [id, wtx] : pwallet->mapWallet) {
+        if (wtx.IsCoinBase() && wtx.isInactive()) {
+            pwallet->AbandonTransaction(wtx);
+        }
+    }
+
     return result;
 }
 

test/functional/wallet_reorgsrestore.py

@@ -19,6 +19,7 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
         assert_equal,
+        assert_raises_rpc_error
 )
 
 class ReorgsRestoreTest(BitcoinTestFramework):
@@ -31,6 +32,65 @@ def set_test_params(self):
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
 
+    def test_coinbase_automatic_abandon_during_startup(self):
+        ##########################################################################################################
+        # Verify the wallet marks coinbase transactions, and their descendants, as abandoned during startup when #
+        # the block is no longer part of the best chain.                                                         #
+        ##########################################################################################################
+        self.log.info("Test automatic coinbase abandonment during startup")
+        # Test setup: Sync nodes for the coming test, ensuring both are at the same block, then disconnect them to
+        # generate two competing chains. After disconnection, verify no other peer connection exists.
+        self.connect_nodes(1, 0)
+        self.sync_blocks(self.nodes[:2])
+        self.disconnect_nodes(1, 0)
+        assert all(len(node.getpeerinfo()) == 0 for node in self.nodes[:2])
+
+        # Create a new block in node0, coinbase going to wallet0
+        self.nodes[0].createwallet(wallet_name="w0", load_on_startup=True)
+        wallet0 = self.nodes[0].get_wallet_rpc("w0")
+        self.generatetoaddress(self.nodes[0], 1, wallet0.getnewaddress(), sync_fun=self.no_op)
+        node0_coinbase_tx_hash = wallet0.getblock(wallet0.getbestblockhash(), verbose=1)['tx'][0]
+
+        # Mine 100 blocks on top to mature the coinbase and create a descendant
+        self.generate(self.nodes[0], 101, sync_fun=self.no_op)
+        # Make descendant, send-to-self
+        descendant_tx_id = wallet0.sendtoaddress(wallet0.getnewaddress(), 1)
+
+        # Verify balance
+        wallet0.syncwithvalidationinterfacequeue()
+        assert(wallet0.getbalances()['mine']['trusted'] > 0)
+
+        # Now create a fork in node1. This will be used to replace node0's chain later.
+        self.nodes[1].createwallet(wallet_name="w1", load_on_startup=True)
+        wallet1 = self.nodes[1].get_wallet_rpc("w1")
+        self.generatetoaddress(self.nodes[1], 1, wallet1.getnewaddress(), sync_fun=self.no_op)
+        wallet1.syncwithvalidationinterfacequeue()
+
+        # Verify both nodes are on a different chain
+        block0_best_hash, block1_best_hash = wallet0.getbestblockhash(), wallet1.getbestblockhash()
+        assert(block0_best_hash != block1_best_hash)
+
+        # Stop both nodes and replace node0 chain entirely for the node1 chain
+        self.stop_nodes()
+        for path in ["chainstate", "blocks"]:
+            shutil.rmtree(self.nodes[0].chain_path / path)
+            shutil.copytree(self.nodes[1].chain_path / path, self.nodes[0].chain_path / path)
+
+        # Start node0 and verify that now it has node1 chain and no info about its previous best block
+        self.start_node(0)
+        wallet0 = self.nodes[0].get_wallet_rpc("w0")
+        assert_equal(wallet0.getbestblockhash(), block1_best_hash)
+        assert_raises_rpc_error(-5, "Block not found", wallet0.getblock, block0_best_hash)
+
+        # Verify the coinbase tx was marked as abandoned and balance correctly computed
+        tx_info = wallet0.gettransaction(node0_coinbase_tx_hash)['details'][0]
+        assert_equal(tx_info['abandoned'], True)
+        assert_equal(tx_info['category'], 'orphan')
+        assert(wallet0.getbalances()['mine']['trusted'] == 0)
+        # Verify the coinbase descendant was also marked as abandoned
+        assert_equal(wallet0.gettransaction(descendant_tx_id)['details'][0]['abandoned'], True)
+
+
     def run_test(self):
         # Send a tx from which to conflict outputs later
         txid_conflict_from = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), Decimal("10"))
@@ -100,5 +160,9 @@ def run_test(self):
         assert_equal(conflicted_after_reorg["confirmations"], 1)
         assert conflicting["blockhash"] != conflicted_after_reorg["blockhash"]
 
+        # Verify we mark coinbase txs, and their descendants, as abandoned during startup
+        self.test_coinbase_automatic_abandon_during_startup()
+
+
 if __name__ == '__main__':
     ReorgsRestoreTest(__file__).main()