wallet: fix crash on double block disconnection

furszy commented at 6:46 pm on January 29, 2025: member

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:

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”
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.

DrahtBot commented at 6:46 pm on January 29, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31757.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	mzumsande, rkrux, pinheadmz
Concept ACK	achow101, naiyoma

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30221 (wallet: Ensure best block matches wallet scan state by achow101)
#29500 (test: create assert_not_equal util by kevkevinpal)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Wallet on Jan 29, 2025

furszy force-pushed on Jan 29, 2025

DrahtBot added the label CI failed on Jan 29, 2025

DrahtBot commented at 7:01 pm on January 29, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36373842434

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot removed the label CI failed on Jan 29, 2025

mzumsande commented at 7:17 pm on February 7, 2025: contributor

Concept ACK - I opened #31824, which shows that this can actually happen in production.

The suggested fix seems good from a defensive programming point of view, although it doesn’t address the root problem why double disconnections can happen.

in src/wallet/wallet.cpp:1552 in 4229839b45 outdated

1549-        SyncTransaction(ptx, TxStateInactive{});
1550+    for (size_t index = 0; index < block.data->vtx.size(); index++) {
1551+        const CTransactionRef& ptx = Assert(block.data)->vtx[index];
1552+        // Coinbase transactions are not only inactive but also abandoned,
1553+        // meaning they should never be relayed standalone via the p2p protocol.
1554+        SyncTransaction(ptx, TxStateInactive{ /*abandoned=*/index == 0});

rkrux commented at 2:28 pm on February 14, 2025:

0- for (const CTransactionRef& ptx : Assert(block.data)->vtx) {
1-        SyncTransaction(ptx, TxStateInactive{});
2+ for (const CTransactionRef& ptx : Assert(block.data)->vtx) {
3+        // Coinbase transactions are not only inactive but also abandoned,
4+        // meaning they should never be relayed standalone via the p2p protocol.
5+        SyncTransaction(ptx, TxStateInactive{ /*abandoned=*/ ptx->IsCoinBase()});

Build and tests all good with this.

I considered this with readability in mind, upon checking the IsCoinBase() implementation I noticed a basic sanity if condition on the input, which would be helpful for robustness.

rkrux commented at 8:40 am on February 18, 2025:

The same function seems to be used in the first block disconnection as well: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L1129

rkrux commented at 2:28 pm on February 14, 2025: contributor

Concept ACK

Initial pass, will review in detail soon.

achow101 commented at 10:52 pm on February 14, 2025: member

Concept ACK

rkrux approved

rkrux commented at 8:40 am on February 18, 2025: contributor

I have checked that a coinbase transaction is set to “inactive + abandoned” here on first block disconnect inside the AddToWallet function: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L1132

And on an unwanted second block disconnection, the same state needs to be passed so that the assertion on line 1114 is satisfied if it’s not a new transaction and with the same state variant: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L1114

I cherry-picked the unit test on master and as expected it failed with the below error due to wallet crash:

0wallet/test/wallet_tests.cpp:1027: Entering test case "wallet_crash_during_disconnectBlock"
1wallet/test/wallet_tests.cpp:1037: info: check GetBalance(*wallet, 0, false).m_mine_immature == 50 * COIN has passed
2Assertion failed: (TxStateSerializedIndex(wtx.m_state) == TxStateSerializedIndex(state)), function AddToWallet, file wallet.cpp, line 1114.
3unknown location:0: fatal error: in "wallet_tests/wallet_crash_during_disconnectBlock": signal: SIGABRT (application abort requested)
4wallet/test/wallet_tests.cpp:1037: last checkpoint
5wallet/test/wallet_tests.cpp:1027: Leaving test case "wallet_crash_during_disconnectBlock"; testing time: 576342us

ACK 4229839b454befea2e18764eee20186b6c18f1c5

DrahtBot requested review from achow101 on Feb 18, 2025

DrahtBot requested review from mzumsande on Feb 18, 2025

in src/wallet/test/wallet_tests.cpp:1047 in 4229839b45 outdated

1042+    m_node.chain->findBlock(tip->GetBlockHash(), interfaces::FoundBlock().data(block_data));
1043+
1044+    // Construct block info for disconnection
1045+    interfaces::BlockInfo info(*tip->phashBlock);
1046+    info.data = &block_data;
1047+    info.prev_hash = tip->phashBlock;

rkrux commented at 9:11 am on February 18, 2025:

0- info.prev_hash = tip->phashBlock;
1+ info.prev_hash = tip->pprev->phashBlock;

The hash of the previous block needs to be added here instead of the hash of the same block. I ran the test with this and it works.

Reference: MakeBlockInfo() here https://github.com/bitcoin/bitcoin/blob/9da0820ec55e136d5213bf5bb90c36499debc034/src/kernel/chain.cpp#L18

The pprev presence check can be avoided because the test is using TestChain100Setup that has 100 blocks mined already leading to pprev being present after CreateAndProcessBlock() is called on line 1035.

furszy commented at 10:11 pm on February 18, 2025:

Ended up changing the testing approach, preferring functional over unit tests. But thanks.

in src/wallet/test/wallet_tests.cpp:1031 in 4229839b45 outdated

1026+ */
1027+BOOST_FIXTURE_TEST_CASE(wallet_crash_during_disconnectBlock, TestChain100Setup)
1028+{
1029+    const auto& chainman = Assert(m_node.chainman);
1030+    const int64_t BLOCK_TIME = WITH_LOCK(chainman->GetMutex(), return chainman->ActiveChain().Tip()->GetBlockTimeMax() + 5);
1031+    SetMockTime(BLOCK_TIME);  // to by-pass the wallet birth time.

rkrux commented at 9:19 am on February 18, 2025:

// to by-pass the wallet birth time.

Can you explain this a bit more? I ran the test w/o it and it works.

furszy commented at 10:10 pm on February 18, 2025:

It was a leftover from a previous implementation. But see #27469.

furszy force-pushed on Feb 18, 2025

furszy commented at 10:16 pm on February 18, 2025: member

Ended up changing the testing approach, opting for a functional test over a unit test due to its reduced maintenance costs across code refactorings. Made a few modifications to mzumzande’s #31824 functional tests.

DrahtBot added the label Needs rebase on Feb 19, 2025

in test/functional/wallet_reorgsrestore.py:110 in df87cf2a52 outdated

45+        assert_equal(wallet.getwalletinfo()['immature_balance'], 25)
46+
47+        # Disconnect tip
48+        tip = wallet.getbestblockhash()
49+        wallet.invalidateblock(tip)
50+        wallet.syncwithvalidationinterfacequeue()

rkrux commented at 12:22 pm on February 19, 2025:

Can add a comment here highlighting that this RPC call is added to ensure the block disconnected notification is handled by the wallet.

furszy commented at 5:25 pm on February 19, 2025:

Done as suggested

in test/functional/wallet_reorgsrestore.py:45 in df87cf2a52 outdated

40+        self.generatetoaddress(node, 1, wallet.getnewaddress(), sync_fun=self.no_op)
41+
42+        # Restart to ensure node and wallet are flushed
43+        self.restart_node(0)
44+        wallet = node.get_wallet_rpc("reorg_crash")
45+        assert_equal(wallet.getwalletinfo()['immature_balance'], 25)

rkrux commented at 12:49 pm on February 19, 2025:

Might be in the nit-land but I find reading 25 here distracting. I had to figure out why it’s 25 that is because in regtest the subsidy halving interval is 150 and initially a 199 blocks long chain is created in the regtest. The block count at this stage is 215 which is because few blocks were created in the preceding test in this file, thereby making this test dependent on the changes of that test as well. IMO, a simple balance greater than 0 check is sufficient.

Also generally, I am inclined to creating and using a constant, for such usages of 25, called INITIAL_ACTIVE_BLOCK_REWARD in the test framework.

furszy commented at 5:25 pm on February 19, 2025:

Done as suggested

in test/functional/wallet_reorgsrestore.py:118 in df87cf2a52 outdated

53+        assert_equal(wallet.getwalletinfo()['immature_balance'], 0)
54+        coinbase_tx_id = wallet.getblock(tip, verbose=1)["tx"][0]
55+        assert_equal(wallet.gettransaction(coinbase_tx_id)['details'][0]['abandoned'], True)
56+
57+        # Abort process abruptly to force an unclean shutdown (no flush)
58+        node.process.kill()

rkrux commented at 12:59 pm on February 19, 2025:

This test passes in my system but one thing that’s on my mind is that can this test ever become flaky? Can there be a scenario that the best block locator changes are written to the disk before the node is killed here? Do you think a time delay should be added before killing the node here?

furszy commented at 4:24 pm on February 19, 2025:

This test passes in my system but one thing that’s on my mind is that can this test ever become flaky? Can there be a scenario that the best block locator changes are written to the disk before the node is killed here?

That wouldn’t be a problem, it would be a fix. See #30221 and the last paragraph of #31824’s issue description.

Do you think a time delay should be added before killing the node here?

The test will still pass if the best locator is flushed to disk before the abrupt shutdown. And that’s fine, that’s the expected behavior.

in test/functional/wallet_reorgsrestore.py:57 in df87cf2a52 outdated

52+        # Tip was disconnected, ensure coinbase has been abandoned
53+        assert_equal(wallet.getwalletinfo()['immature_balance'], 0)
54+        coinbase_tx_id = wallet.getblock(tip, verbose=1)["tx"][0]
55+        assert_equal(wallet.gettransaction(coinbase_tx_id)['details'][0]['abandoned'], True)
56+
57+        # Abort process abruptly to force an unclean shutdown (no flush)

rkrux commented at 1:01 pm on February 19, 2025:

0+  # Abort process abruptly to force an unclean shutdown (no flush) so that the best block locator changes are not persisted

furszy commented at 5:25 pm on February 19, 2025:

Done as suggested

in test/functional/wallet_reorgsrestore.py:129 in df87cf2a52 outdated

60+        # Start wallet again. Ensure the tx is still abandoned and the wallet has no balance.
61+        self.start_node(0)
62+        wallet = node.get_wallet_rpc("reorg_crash")
63+        assert_equal(wallet.getbestblockhash(), tip)
64+        assert_equal(wallet.getwalletinfo()['immature_balance'], 0)
65+        wallet.invalidateblock(tip)

rkrux commented at 1:03 pm on February 19, 2025:

Can add a comment here: Disconnect tip again

furszy commented at 5:25 pm on February 19, 2025:

Done as suggested

in src/wallet/wallet.cpp:1555 in f99b2f136c outdated

1549-        SyncTransaction(ptx, TxStateInactive{});
1550+    for (size_t index = 0; index < block.data->vtx.size(); index++) {
1551+        const CTransactionRef& ptx = Assert(block.data)->vtx[index];
1552+        // Coinbase transactions are not only inactive but also abandoned,
1553+        // meaning they should never be relayed standalone via the p2p protocol.
1554+        SyncTransaction(ptx, TxStateInactive{/*abandoned=*/index == 0});

rkrux commented at 1:06 pm on February 19, 2025:

Re: #31757 (review)

Bringing it back again because I feel besides the robustness IsCoinBase() brings in here, we can also get rid of introducing index that is not used elsewhere in the function.

LMK your thoughts.

furszy commented at 4:17 pm on February 19, 2025:

Bringing it back again because I feel besides the robustness IsCoinBase() brings in here

By consensus, coinbase transactions must be the first in a block. Using IsCoinBase() is slightly more readable but not more robust. It is also a slower check, as it verifies that all bytes in the input hash are zero for every tx in the block. I’m a ~0 here.

rkrux commented at 9:49 am on February 20, 2025:

Thanks for entertaining this request, the readability was indeed pulling me towards it but with the consensus rule in place, I agree with leaving this as-is.

in test/functional/wallet_reorgsrestore.py:135 in df87cf2a52 outdated

62+        wallet = node.get_wallet_rpc("reorg_crash")
63+        assert_equal(wallet.getbestblockhash(), tip)
64+        assert_equal(wallet.getwalletinfo()['immature_balance'], 0)
65+        wallet.invalidateblock(tip)
66+        # Ensure abandoned state persists after restart
67+        assert_equal(wallet.gettransaction(coinbase_tx_id)['details'][0]['abandoned'], True)

rkrux commented at 1:20 pm on February 19, 2025:

Might not hurt to check for wallet balance again. But more importantly it would be nice to check that the best block hash is not tip now like below. Should we stop the node again (cleanly this time) and check this on start?

0+ assert_equal(node.getbestblockhash() == tip, False)

furszy commented at 5:20 pm on February 19, 2025:

Should we stop the node again (cleanly this time) and check this on start?

There should be another test case already ensuring locator persistence during a clean shutdown.

in test/functional/wallet_reorgsrestore.py:116 in df87cf2a52 outdated

51+
52+        # Tip was disconnected, ensure coinbase has been abandoned
53+        assert_equal(wallet.getwalletinfo()['immature_balance'], 0)
54+        coinbase_tx_id = wallet.getblock(tip, verbose=1)["tx"][0]
55+        assert_equal(wallet.gettransaction(coinbase_tx_id)['details'][0]['abandoned'], True)
56+

rkrux commented at 1:27 pm on February 19, 2025:

Interesting that the best block hash at the stage is not the tip because of invalidation but not so after unclean-stop-start.

0assert_equal(node.getbestblockhash() == tip, False)

in test/functional/wallet_reorgsrestore.py:35 in df87cf2a52 outdated

30@@ -31,6 +31,41 @@ def set_test_params(self):
31     def skip_test_if_missing_module(self):
32         self.skip_if_no_wallet()
33 
34+    def test_reorg_handling_during_unclean_shutdown(self):
35+        self.log.info("Test crash due to a duplicate block disconnection event during an unclean shutdown")

rkrux commented at 1:31 pm on February 19, 2025:

Test that wallet doesn't crash due to a duplicate block disconnection event after an unclean shutdown

furszy commented at 5:25 pm on February 19, 2025:

Done as suggested

rkrux commented at 1:36 pm on February 19, 2025: contributor

I reviewed till df87cf2

I had thought about adding the functional test earlier but had ruled it out because I thought technically it’s a bug fix and not a functionality per se. But after reading this test, I’m in favour of this test, makes me think from the POV of what’s happening to the node overall.

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.

9ef429b6ae

furszy force-pushed on Feb 19, 2025

DrahtBot removed the label Needs rebase on Feb 19, 2025

in test/functional/wallet_reorgsrestore.py:121 in c80b105f77 outdated

116+
117+        # Abort process abruptly to mimic an unclean shutdown (no flush)
118+        # The best block locator changes might not have been persisted
119+        node.process.kill()
120+
121+        # Start wallet again. Ensure the tx is still abandoned and the wallet has no balance

mzumsande commented at 7:12 pm on February 19, 2025:

“Ensure the tx is still abandoned and the wallet has no balance” sounds too positive to me - the test comment should say clearly that this is a bug in master and the wallet is showing the wrong balance because it incorrectly treats a tx which is in the current chain as abandoned (even if it doesn’t crash anymore).

rkrux commented at 11:55 am on February 20, 2025:

Agree, it’s a subtle point but important to convey in the comment. The 2 assertions on line 124 and 125 below are correct individually but incongruent with regards to the overall node state.

pinheadmz commented at 8:11 pm on March 4, 2025:

@mzumsande

tx which is in the current chain

Isn’t the tx in a disconnected block?

mzumsande commented at 8:19 pm on March 4, 2025:

it was in a disconnected block before we killed the node - but the chainstate was not saved to disk with the unclean shutdown, so after the restart that block is part of the chain again - while the wallet (which synced with disk immediately) thinks the tx is not in the chain. Of course we might do the reorg again if peers give us the better chain again, fixing the problem, but this is not guaranteed: while the node was down, the original chain may have been extended, in which case the balance would be wrong indefinitely.

furszy commented at 3:45 pm on March 5, 2025:

Updated per feedback, thanks!. Hopefully the wording is better now.

in test/functional/wallet_reorgsrestore.py:124 in c80b105f77 outdated

119+        node.process.kill()
120+
121+        # Start wallet again. Ensure the tx is still abandoned and the wallet has no balance
122+        self.start_node(0)
123+        wallet = node.get_wallet_rpc("reorg_crash")
124+        assert_equal(wallet.getbestblockhash(), tip)

rkrux commented at 11:50 am on February 20, 2025:

Re: #31757 (review) Continuing this thread here because there doesn’t seem to be a way to continue that thread while making it part of the review.

As per this comment and related code #31824 (comment), I understand that FlushStateMode::IF_NEEDED mode is used and therefore I assume that makes it quite unlikely to flush chainstate in a functional test, so the test will not fail.

The test will still pass if the best locator is flushed to disk before the abrupt shutdown. And that’s fine, that’s the expected behavior.

However, I don’t understand this comment. Hypothetically, if the state were to be flushed before abrupt node kill, wouldn’t this assertion here fail?

furszy commented at 3:50 pm on March 5, 2025:

However, I don’t understand this comment. Hypothetically, if the state were to be flushed before abrupt node kill, wouldn’t this assertion here fail?

I was talking about the wallet best locator, which is different to the node’s best block (getwalletinfo()['lastprocessedblock'] vs getbestblockhash()). Just updated the test to reflect this in a more understandable manner.

rkrux commented at 11:58 am on February 20, 2025: contributor

Asked a question for clarity.

naiyoma commented at 12:37 pm on March 4, 2025: contributor

Concept ACK , cherry-picked test commit https://github.com/bitcoin/bitcoin/pull/31757/commits/c80b105f77d8ee397c7dc07b013c37b7df63abd5

on master branch and verified that the test case fails on the second invalidation:

02025-03-04T12:18:39.077000Z TestFramework (ERROR): Unexpected exception caught during testing
1 raise RemoteDisconnected("Remote end closed connection without"
2http.client.RemoteDisconnected: Remote end closed connection without response

DrahtBot requested review from mzumsande on Mar 4, 2025

DrahtBot requested review from rkrux on Mar 4, 2025

test: wallet, coverage for crash on dup block disconnection during unclean shutdown

Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

11f8ab140f

in test/functional/wallet_reorgsrestore.py:128 in c80b105f77 outdated

123+        wallet = node.get_wallet_rpc("reorg_crash")
124+        assert_equal(wallet.getbestblockhash(), tip)
125+        assert_equal(wallet.getwalletinfo()['immature_balance'], 0)
126+
127+        # If the unclean shutdown succeeded, the chainstate wasn't persisted after the tip invalidation.
128+        # So we need to invalidate it again (this is a no-op if the invalidation was persisted).

pinheadmz commented at 8:13 pm on March 4, 2025:

Aren’t you garunteeing the shutdown was not clean on L124 by asserting the “invalid” block is still the tip?

furszy commented at 3:44 pm on March 5, 2025:

Aren’t you garunteeing the shutdown was not clean on L124 by asserting the “invalid” block is still the tip?

I was talking about the wallet best block locator inside the parentheses. My wording was really not the best. Updated, thanks. Hopefully it is more understandable now.

furszy force-pushed on Mar 5, 2025

mzumsande commented at 9:57 pm on March 5, 2025: contributor

Code Review ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda

DrahtBot requested review from naiyoma on Mar 5, 2025

in test/functional/wallet_reorgsrestore.py:126 in 11f8ab140f

121+        self.start_node(0)
122+        assert_equal(node.getbestblockhash(), tip)
123+
124+        # Due to an existing bug, the wallet incorrectly keeps the transaction in an abandoned state, even though that's
125+        # no longer the case (after the unclean shutdown, the node's chain returned to the pre-invalidation tip).
126+        # This issue blocks any future spending and results in an incorrect balance display.

rkrux commented at 9:00 am on March 6, 2025:

This issue blocks any future spending and results in an incorrect balance display.

Technically correct but since the block was supposed to be invalidated and with the assumption that the likelihood for it to be invalidated again (successfully this time) is high, this immature unspent will not be blocked for long.

Not suggesting any changes, just noteworthy. Please do LMK if my assumption is incorrect.

furszy commented at 2:55 pm on March 6, 2025:

Technically correct but since the block was supposed to be invalidated and with the assumption that the likelihood for it to be invalidated again (successfully this time) is high, this immature unspent will not be blocked for long.

Usually, yes. Yet, there is another edge case. “Invalidated” means a reorg, which could also be reversed. That is, a block reorged out of the active chain before the unclean shutdown could become part of the active chain again when the node restarts (due to an extension of that chain - new block). In this scenario, the node starts at the reorged block, so no block connection event is signaled, and continues extending such chain. Leaving the tx in an invalid state forever (users can fix this state by calling rescanblockchain).

in test/functional/wallet_reorgsrestore.py:134 in 11f8ab140f

129+
130+        # Previously, a bug caused the node to crash if two block disconnection events occurred consecutively.
131+        # Ensure this is no longer the case by simulating a new reorg.
132+        node.invalidateblock(tip)
133+        assert(node.getbestblockhash() != tip)
134+        # Ensure wallet state is consistent now

rkrux commented at 9:13 am on March 6, 2025:

This comment is too positive. If the node undergoes through an unclean shutdown and restart here, the wallet crashes again. The underlying issue of the discrepancy between wallet-view and chainstate-view is still there that causes the failure. Can update the comment to the following but I don’t intend to block the PR just for this.

0Ensure wallet state is consistent for now if the node doesn't undergo an unclean shutdown

furszy commented at 2:59 pm on March 6, 2025:

This comment is too positive. If the node undergoes through an unclean shutdown and restart here, the wallet crashes again.

If there is any shutdown here (clean or unclean), the node will be off anyway, and all RPC calls will fail.

in test/functional/wallet_reorgsrestore.py:141 in 11f8ab140f

136+        assert_equal(wallet.getwalletinfo()['immature_balance'], 0)
137+
138+        # And finally, verify the state if the block ends up being into the best chain again
139+        node.reconsiderblock(tip)
140+        assert_equal(wallet.gettransaction(coinbase_tx_id)['details'][0]['abandoned'], False)
141+        assert_greater_than(wallet.getwalletinfo()['immature_balance'], 0)

rkrux commented at 9:14 am on March 6, 2025:

+1 for adding this to wrap the test up.

in test/functional/wallet_reorgsrestore.py:110 in 11f8ab140f

105+        assert_greater_than(wallet.getwalletinfo()['immature_balance'], 0)
106+
107+        # Disconnect tip and sync wallet state
108+        tip = wallet.getbestblockhash()
109+        wallet.invalidateblock(tip)
110+        wallet.syncwithvalidationinterfacequeue()

rkrux commented at 9:23 am on March 6, 2025:

I would have used node RPCs here because these don’t seem to be wallet specific requests and moreover, there’s just one loaded wallet and using wallet RPCs here made me think there was some specific reason for it, but fine. I don’t want the ACKs to be invalidated for this.

furszy commented at 2:41 pm on March 6, 2025:

will do if I have to retouch it.

rkrux approved

rkrux commented at 9:25 am on March 6, 2025: contributor

ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda

Thank for you adding verbose comments in the test, specially because of the peculiar flow involved in reproducing the bug.

in test/functional/wallet_reorgsrestore.py:133 in 11f8ab140f

128+        assert_equal(wallet.getwalletinfo()['immature_balance'], 0) # FIXME: #31824.
129+
130+        # Previously, a bug caused the node to crash if two block disconnection events occurred consecutively.
131+        # Ensure this is no longer the case by simulating a new reorg.
132+        node.invalidateblock(tip)
133+        assert(node.getbestblockhash() != tip)

pinheadmz commented at 1:55 pm on March 11, 2025:

nanonit: funny, the only occurrences of assert(... (with parenthesis instead of just a space) are in this test ;-)

pinheadmz approved

pinheadmz commented at 2:04 pm on March 11, 2025: member

ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda

Built on arm/macos. Confirmed the functional tests passes on the branch, on master bitcoind fails the test with a crash:

stderr Assertion failed: (TxStateSerializedIndex(wtx.m_state) == TxStateSerializedIndex(state)), function AddToWallet, file wallet.cpp, line 1114.

Also confirmed that if the assertion is commented out, the test still passes on both master and branch, indicating that the new state (abandoned) is already correct.

The code change is minimal, the wallet treats coinbase transactions as a special case when disconnecting blocks and adds a functional test.

Since it is a bug fix (and a node-crashing bug) it seems reasonable to merge for 29 if possible.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmfQQasACgkQ5+KYS2KJ
 7yTreIA/+Ok9YoZVQCmkKAL4aNbLBj9kaBeG3s5XfYR55jUbGi3GeE0HRTfp5tJkf
 83qnmZ7CRQuHXBcuxJTrLbdO3qEvM/5eVrNsjMoVa0GVc/9RoaFR+Q2I085MdqAra
 9KIJlpqebslFwmApnMHj5kWriiv/W/l2hqVLCTdoL2qol30BzkFYHOhBFBaVYihrI
10pqjk2yBX5dLhrYwv/uFRjriXKN7/N41ZhSxczT17bDvblxYX9XlaWcX3D9pFIJcV
116hLSbIHw2Q9t386bgV2Qmgd7YKA/zB4udMVX6+d21wNJAmT1KNHZnyetMQTAUIwN
12nLw2KICj+80XZy1qcn/1iRhFBMkPUHeb9vYDMux24pwFKP7i11xeYgu9rex5ilgE
1335unppYSuUgcFpNTRUs+Da9kEJy1lP3zEpDKtnDAm6XJfglXOQTS6yBwPa/2coMX
14SYxuBvObUvdFDqHZC6aXkJKdo8JSKKcZHZmdJVRavN+m8IaZ0KIZmQK9DimEtjt2
15k5oWpEdy+a+kIltHGNnOPYkpn5NbtIPnnZe7mVNMh4aL1Z/9tPLNUV/D8lY9M7S+
16/liZfjoU9Y9MnWjqv0X1iAHKZRmOhOHhWwZkVZ8XSnndP39LwKtK078JF4fLLPRF
17jv8AxUpKKacmlmKi44e1FjmcYHRNABV1fdDwrgRKAt1eO6LXwUE=
18=13xG
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on openpgp.org

ryanofsky assigned ryanofsky on Mar 13, 2025

ryanofsky merged this on Mar 13, 2025

ryanofsky closed this on Mar 13, 2025

furszy deleted the branch on Mar 13, 2025

in src/wallet/wallet.cpp:1552 in 11f8ab140f

1547@@ -1548,8 +1548,11 @@ void CWallet::blockDisconnected(const interfaces::BlockInfo& block)
1548 
1549     int disconnect_height = block.height;
1550 
1551-    for (const CTransactionRef& ptx : Assert(block.data)->vtx) {
1552-        SyncTransaction(ptx, TxStateInactive{});
1553+    for (size_t index = 0; index < block.data->vtx.size(); index++) {
1554+        const CTransactionRef& ptx = Assert(block.data)->vtx[index];

maflcko commented at 8:25 am on March 14, 2025:

This change makes the assert a no-op and turns it undefined behavior.

Previously, the block.data presence was asserted before the for-loop. Now, the for loop will run into UB when block.data is null. (It is true that the for-loop body still checks for the presence, but once UB has happened, it is not possible to recover from it)

rkrux commented at 11:51 am on March 14, 2025:

Thank you for highlighting this. I started looking into it and found a language level assert at the start of this function. Doesn’t that make the Assert inside the for-loop redundant like it was before this change as well? https://github.com/bitcoin/bitcoin/blob/698f86964c68041d938aaf54fdd39466266c371c/src/wallet/wallet.cpp#L1537-L1552

furszy commented at 2:19 pm on March 14, 2025:

yikes, good catch. Not a bug, but it wouldn’t hurt to remove it (or place it in the correct position) in a small follow-up.

wallet: fix crash on double block disconnection #31757

Code Coverage & Benchmarks

Reviews

Conflicts