Merge bitcoin/bitcoin#31757: wallet: fix crash on double block disconnection

11f8ab1 test: wallet, coverage for crash on dup block disconnection during unclean shutdown (Martin Zumsande)
9ef429b wallet: fix crash on double block disconnection (furszy)

Pull request description:

  The wallet crashes if it processes the same block disconnection event twice in a row due
  to an incompatible coinbase transaction state.
  This happens because `disconnectBlock` provides `TxStateInactive` without the "abandoned"
  flag for coinbase transactions to `SyncTransaction`, while `AddToWallet()` internally modifies
  it to retain the abandoned state.

  The crash flow is as follows:
  1) On the first disconnection, the transaction state transitions from "confirmed" to
  "inactive," bypassing the state equality check since the provided state differs. Then,
  `AddToWallet` internally updates the state to "inactive + abandoned"

  2) On the second disconnection, as we provide only the "inactive" state
  to `SyncTransaction()`, the state equality assertion fails and crashes the wallet.

  Reviewers Note:
  The crash can easily be replicated by cherry-picking the test commit in master.

ACKs for top commit:
  mzumsande:
    Code Review ACK 11f8ab1
  rkrux:
    ACK 11f8ab1
  pinheadmz:
    ACK 11f8ab1

Tree-SHA512: 971069bca562f0afb06c34a2516842d01b5cbc2b18ed851392aa3caa3bb7488f4a84a5d017ea334e6361261d3c44aa597cc67a1d4fa16781f85e081f3d1f8771

Author: ryanofsky

src/wallet/wallet.cpp

@@ -1548,8 +1548,11 @@ void CWallet::blockDisconnected(const interfaces::BlockInfo& block)
 
     int disconnect_height = block.height;
 
-    for (const CTransactionRef& ptx : Assert(block.data)->vtx) {
-        SyncTransaction(ptx, TxStateInactive{});
+    for (size_t index = 0; index < block.data->vtx.size(); index++) {
+        const CTransactionRef& ptx = Assert(block.data)->vtx[index];
+        // Coinbase transactions are not only inactive but also abandoned,
+        // meaning they should never be relayed standalone via the p2p protocol.
+        SyncTransaction(ptx, TxStateInactive{/*abandoned=*/index == 0});
 
         for (const CTxIn& tx_in : ptx->vin) {
             // No other wallet transactions conflicted with this transaction

test/functional/wallet_reorgsrestore.py

@@ -19,6 +19,7 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
         assert_equal,
+        assert_greater_than,
         assert_raises_rpc_error
 )
 
@@ -90,6 +91,54 @@ def test_coinbase_automatic_abandon_during_startup(self):
         # Verify the coinbase descendant was also marked as abandoned
         assert_equal(wallet0.gettransaction(descendant_tx_id)['details'][0]['abandoned'], True)
 
+    def test_reorg_handling_during_unclean_shutdown(self):
+        self.log.info("Test that wallet doesn't crash due to a duplicate block disconnection event after an unclean shutdown")
+        node = self.nodes[0]
+        # Receive coinbase reward on a new wallet
+        node.createwallet(wallet_name="reorg_crash", load_on_startup=True)
+        wallet = node.get_wallet_rpc("reorg_crash")
+        self.generatetoaddress(node, 1, wallet.getnewaddress(), sync_fun=self.no_op)
+
+        # Restart to ensure node and wallet are flushed
+        self.restart_node(0)
+        wallet = node.get_wallet_rpc("reorg_crash")
+        assert_greater_than(wallet.getwalletinfo()['immature_balance'], 0)
+
+        # Disconnect tip and sync wallet state
+        tip = wallet.getbestblockhash()
+        wallet.invalidateblock(tip)
+        wallet.syncwithvalidationinterfacequeue()
+
+        # Tip was disconnected, ensure coinbase has been abandoned
+        assert_equal(wallet.getwalletinfo()['immature_balance'], 0)
+        coinbase_tx_id = wallet.getblock(tip, verbose=1)["tx"][0]
+        assert_equal(wallet.gettransaction(coinbase_tx_id)['details'][0]['abandoned'], True)
+
+        # Abort process abruptly to mimic an unclean shutdown (no chain state flush to disk)
+        node.process.kill()
+
+        # Restart the node and confirm that it has not persisted the last chain state changes to disk
+        self.start_node(0)
+        assert_equal(node.getbestblockhash(), tip)
+
+        # Due to an existing bug, the wallet incorrectly keeps the transaction in an abandoned state, even though that's
+        # no longer the case (after the unclean shutdown, the node's chain returned to the pre-invalidation tip).
+        # This issue blocks any future spending and results in an incorrect balance display.
+        wallet = node.get_wallet_rpc("reorg_crash")
+        assert_equal(wallet.getwalletinfo()['immature_balance'], 0) # FIXME: #31824.
+
+        # Previously, a bug caused the node to crash if two block disconnection events occurred consecutively.
+        # Ensure this is no longer the case by simulating a new reorg.
+        node.invalidateblock(tip)
+        assert(node.getbestblockhash() != tip)
+        # Ensure wallet state is consistent now
+        assert_equal(wallet.gettransaction(coinbase_tx_id)['details'][0]['abandoned'], True)
+        assert_equal(wallet.getwalletinfo()['immature_balance'], 0)
+
+        # And finally, verify the state if the block ends up being into the best chain again
+        node.reconsiderblock(tip)
+        assert_equal(wallet.gettransaction(coinbase_tx_id)['details'][0]['abandoned'], False)
+        assert_greater_than(wallet.getwalletinfo()['immature_balance'], 0)
 
     def run_test(self):
         # Send a tx from which to conflict outputs later
@@ -163,6 +212,9 @@ def run_test(self):
         # Verify we mark coinbase txs, and their descendants, as abandoned during startup
         self.test_coinbase_automatic_abandon_during_startup()
 
+        # Verify reorg behavior during an unclean shutdown
+        self.test_reorg_handling_during_unclean_shutdown()
+
 
 if __name__ == '__main__':
     ReorgsRestoreTest(__file__).main()