From 9ef429b6ae65f6ad3e9ac11c2d9c0a6c52beb865 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 29 Jan 2025 13:33:22 -0500 Subject: [PATCH 1/2] wallet: fix crash on double block disconnection 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 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. --- src/wallet/wallet.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7abd17d31e9..09eda0c28e4 100644 --- a/src/wallet/wallet.cpp +++ b/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 From 11f8ab140fe63857f6a93b81021efda8f90ceeda Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Tue, 18 Feb 2025 19:07:04 -0300 Subject: [PATCH 2/2] test: wallet, coverage for crash on dup block disconnection during unclean shutdown Co-authored-by: furszy --- test/functional/wallet_reorgsrestore.py | 52 +++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/test/functional/wallet_reorgsrestore.py b/test/functional/wallet_reorgsrestore.py index a600930acd8..d9be3bd2e9a 100755 --- a/test/functional/wallet_reorgsrestore.py +++ b/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_greater_than, assert_raises_rpc_error ) @@ -90,6 +91,54 @@ class ReorgsRestoreTest(BitcoinTestFramework): # 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 @@ class ReorgsRestoreTest(BitcoinTestFramework): # 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()