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

e4dd5a351bde88a94326824945f4c8b1e4c15df2 test: wallet, abandon coinbase txs and their descendants during startup (furszy)
474139aa9bf7109df78e46936e5a649c70703386 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 e4dd5a351bde88a94326824945f4c8b1e4c15df2
  rkrux:
    tACK e4dd5a351bde88a94326824945f4c8b1e4c15df2
  mzumsande:
    Code Review ACK e4dd5a351bde88a94326824945f4c8b1e4c15df2

Tree-SHA512: 461a43de7a6f5a580f2e6e3b56ec9bc92239cd45e850a2ff594ab5488dcd4a507f68fbbf550a33d7173b2add0de80de1e1b3841e1dfab0c95b284212d8ced08a

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 @@ public:
 
     /* 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 @@ import shutil
 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 @@ class ReorgsRestoreTest(BitcoinTestFramework):
     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 @@ class ReorgsRestoreTest(BitcoinTestFramework):
         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()