When loading a wallet, updating the state based on the chain data prior to AttachChain results in some blocks being missed, which leads to inconsistent state information that can cause assertion failures. Moving the state updating inside of AttachChain after the chain notifications handler has been attached ensures that the state will always be up to date with the tip that the wallet is tracking.
Fixes #34599
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35294.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
ACK 18bc686c6e97fb5522a93412240ddca99057f1b7
Reviewed the change and tested locally.
Moving the tx state update out of LoadToWallet() makes sense to me. During DB
load, the wallet may already have a chain pointer, but it is not yet registered
for chain notifications. In AttachChain(), notifications are registered before
the state update and any needed rescan, so the wallet no longer updates tx
states against a chain tip that can move without the wallet seeing the
intervening block connections.
The new test fails on the pre-fix parent (ddb94fd3e1) with a wallet transaction
still reporting 0 confirmations after restart, and passes on this PR head:
test/functional/wallet_chain_reprocess.py --configfile=$PWD/build/test/config.ini --timeout-factor=4
13 | + 14 | +class WalletChainReprocess(BitcoinTestFramework): 15 | + def set_test_params(self): 16 | + self.setup_clean_chain = True 17 | + self.num_nodes = 2 18 | + #self.extra_args = [["-checkblockindex=0"]]
- #self.extra_args = [["-checkblockindex=0"]]
Removed
20 | + def skip_test_if_missing_module(self): 21 | + self.skip_if_no_wallet() 22 | + 23 | + def test_process_all_blocks_after_unclean(self): 24 | + self.log.info("Testing that all blocks are reprocessed by the wallet after an unclean shutdown") 25 | + self.nodes[0].createwallet("target", load_on_startup=False)
- self.nodes[0].createwallet("target", load_on_startup=False)
+ self.nodes[0].createwallet("target")
This argument doesn't seem necessary.
Removed
31 | + self.restart_node(0) 32 | + self.connect_nodes(0, 1) 33 | + self.nodes[0].loadwallet("target") 34 | + wallet = self.nodes[0].get_wallet_rpc("target") 35 | + 36 | + # mine 1000 blocks, include a transaction in each one that is a child
Is there a particular reason to mine 1000 blocks?
updating the state based on the chain data prior to AttachChain results in some blocks being missed
I believe the broad reason is to make the processing of few blocks by the wallet to be missed (on master without the fix here). Ultimately, this can happen as long as there are enough blocks for the node to process (post an unclean shutdown) so that block processing keeps on happening even after the loading of wallet (for the sync_all to come into effect).
For which I think a lower number can also be sufficient - 100 works on my system. I checked it by comparing the node's chain height right before the sync_all call on line 56 and the node's chain height before the unclean shutdown - it showed a more than 50 block difference.
Either way, it would be very helpful to mention the reason as a comment in the test here for review and future reference.
Yes, it's so that there's enough blocks that on the restart, the node is still processing blocks while and after the wallet is loading. I chose 1000 because that was a number very likely to trigger the problem.
Added a comment and reduced to 100.
Concept ACK 18bc686c6e97fb5522a93412240ddca99057f1b7
This is indeed quite a subtle and peculiar issue!
Updating transactions against the current chain state can only be done
in AttachChain after chain notifications have been attached. AttachChain
will guarantee that any blocks missing between the stored best block and
the current tip are rescanned, and any blocks connected during
AttachChain will be processed after loading completes.
This fixes an issue where tx states were updated with no guarantee that
any blocks connected prior to AttachChain are scanned, resulting in
incorrect tx states.
3253 | @@ -3258,6 +3254,18 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf 3254 | walletInstance->SetLastBlockProcessedInMem(-1, uint256()); 3255 | } 3256 | 3257 | + // Update the state of every transaction now that we are connected to the chain 3258 | + // Do this in reverse order to ensure that any abandoning will work recursively 3259 | + for (auto& [_, wtx] : std::ranges::reverse_view(walletInstance->wtxOrdered)) { 3260 | + wtx->updateState(chain); 3261 | + // After loading updating the state of all txs, abandon any coinbase that is no longer in the active chain.
After loading updating the state of all txs
This doesn't seem entirely correct now because the state of all transactions are not updated due to the following abandon call being present in the same loop that also updates the transaction state.
Updated
@dergoegge CI is passing now, so can run through Antithesis?
so can run through Antithesis?
Running
tACK 4de5c3b692b6958b36451a0bd6ac368f8f605b4f
This fixes the issue.
31 | + self.nodes[0].loadwallet("target") 32 | + wallet = self.nodes[0].get_wallet_rpc("target") 33 | + 34 | + # In order to ensure that wallet transaction states match the chain, we want to mine 35 | + # enough blocks such that the chainstate will still be moving forward while the 36 | + # wallet is loading. 1000 blocks should be sufficiednt.
In 4de5c3b692b6958b36451a0bd6ac368f8f605b4f "test: Check all wallet tx states are correct after unclean shutdown"
s/1000/100 s/sufficiednt/sufficient
Done
43 | + txids.append(res["txid"]) 44 | + self.generate(self.nodes[0], 1, sync_fun=self.no_op) 45 | + 46 | + # Sync nodes, kill node 0, and restart 47 | + self.sync_all() 48 | + wallet.unloadwallet()
In 4de5c3b "test: Check all wallet tx states are correct after unclean shutdown"
Nit: I don't believe unloading the wallet is necessary because all the blocks contain wallet transaction so the best block will be written to disk on every block connected notification just like unloading the wallet would do.
- wallet.unloadwallet()
Since the tests run with -unsafesqlitesync, not all data will be flushed at any given time. While we don't currently fsync everything on a wallet close (we probably should though), unloading the wallet at least calls sqlite3_close which should help to guarantee that all of the wallet data is persisted. My main concern is that killing the process while the wallet is loaded might result in data loss which could cause the test to fail intermittently or behave unexepctedly. Unloading the wallet should help mitigate such issues.
46 | + # Sync nodes, kill node 0, and restart 47 | + self.sync_all() 48 | + wallet.unloadwallet() 49 | + self.nodes[0].kill_process() 50 | + self.start_node(0) 51 | + self.connect_nodes(0, 1)
Considering the subtleness of the issue, can we add few assertions here to make it explicit for the reader regarding node/wallet height states? Maybe the following diff:
diff --git a/test/functional/wallet_chain_reprocess.py b/test/functional/wallet_chain_reprocess.py
index be4c748fca..1158fa964c 100755
--- a/test/functional/wallet_chain_reprocess.py
+++ b/test/functional/wallet_chain_reprocess.py
@@ -45,13 +45,20 @@ class WalletChainReprocess(BitcoinTestFramework):
# Sync nodes, kill node 0, and restart
self.sync_all()
+ node_chain_height_before_kill = self.nodes[0].getblockchaininfo()["blocks"]
+ wallet_scan_height_before_kill = wallet.getwalletinfo()["lastprocessedblock"]["height"]
+ assert_equal(wallet_scan_height_before_kill, node_chain_height_before_kill)
self.nodes[0].kill_process()
self.start_node(0)
self.connect_nodes(0, 1)
+ node_chain_height_after_kill = self.nodes[0].getblockchaininfo()["blocks"]
+ assert_greater_than(node_chain_height_before_kill, node_chain_height_after_kill)
+ assert_greater_than(wallet_scan_height_before_kill, node_chain_height_after_kill)
self.nodes[0].loadwallet("target")
wallet = self.nodes[0].get_wallet_rpc("target")
+ wallet_scan_height_after_kill = wallet.getwalletinfo()["lastprocessedblock"]["height"]
+ assert_greater_than(wallet_scan_height_before_kill, wallet_scan_height_after_kill)
# Wait for node0 to be synced
self.sync_all()
Done
LGTM broadly 4de5c3b692b6958b36451a0bd6ac368f8f605b4f.
Few minor suggestions.
51 | + wallet.unloadwallet() 52 | + self.nodes[0].kill_process() 53 | + self.start_node(0) 54 | + self.connect_nodes(0, 1) 55 | + restart_tip_height = self.nodes[0].getblockchaininfo()["blocks"] 56 | + assert_greater_than(tip_height, restart_tip_height)
https://github.com/bitcoin/bitcoin/actions/runs/26480361555/job/77976026440?pr=35294#step:9:3403:
216/469 - wallet_chain_reprocess.py failed, Duration: 3 s
stdout:
2026-05-26T23:34:49.429029Z TestFramework (INFO): PRNG seed is: 2150906247185331291
2026-05-26T23:34:49.429675Z TestFramework (INFO): Initializing test directory /Users/runner/work/bitcoin/bitcoin/repo_archive/ci/scratch_ ₿🧪_/test_runner/test_runner_₿_🏃_20260526_232554/wallet_chain_reprocess_251
2026-05-26T23:34:49.962081Z TestFramework (INFO): Testing that all blocks are reprocessed by the wallet after an unclean shutdown
2026-05-26T23:34:52.293951Z TestFramework (ERROR): Unexpected exception:
Traceback (most recent call last):
File "/Users/runner/work/bitcoin/bitcoin/repo_archive/test/functional/test_framework/test_framework.py", line 143, in main
self.run_test()
~~~~~~~~~~~~~^^
File "/Users/runner/work/bitcoin/bitcoin/repo_archive/ci/scratch_ ₿🧪_/build-aarch64-apple-darwin24.6.0/test/functional/wallet_chain_reprocess.py", line 76, in run_test
self.test_process_all_blocks_after_unclean()
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "/Users/runner/work/bitcoin/bitcoin/repo_archive/ci/scratch_ ₿🧪_/build-aarch64-apple-darwin24.6.0/test/functional/wallet_chain_reprocess.py", line 56, in test_process_all_blocks_after_unclean
assert_greater_than(tip_height, restart_tip_height)
~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/runner/work/bitcoin/bitcoin/repo_archive/test/functional/test_framework/util.py", line 96, in assert_greater_than
raise AssertionError("%s <= %s" % (str(thing1), str(thing2)))
AssertionError: 302 <= 302
Hopefully fixed
3253 | @@ -3263,6 +3254,18 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf 3254 | walletInstance->SetLastBlockProcessedInMem(-1, uint256()); 3255 | } 3256 | 3257 | + // Update the state of every transaction now that we are connected to the chain 3258 | + // Do this in reverse order to ensure that any abandoning will work recursively 3259 | + for (auto& [_, wtx] : std::ranges::reverse_view(walletInstance->wtxOrdered)) {
1a35042 wallet: Update tx chain state after chain notifications are attached:
Do we need the reverse traversal here, or could we do the state updates and abandonments in separate passes instead? It seems simpler to first update every transaction state, then run the inactive coinbase abandonment pass afterward.
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
--- a/src/wallet/wallet.cpp (revision d33afaf84e54c0262f3b6f1e93ac2edf877747de)
+++ b/src/wallet/wallet.cpp (revision 866e54d2e3f0851256950ad6c737c6032f67527d)
@@ -76,7 +76,6 @@
#include <condition_variable>
#include <exception>
#include <optional>
-#include <ranges>
#include <stdexcept>
#include <thread>
#include <tuple>
@@ -3254,17 +3253,18 @@
walletInstance->SetLastBlockProcessedInMem(-1, uint256());
}
- // Update the state of every transaction now that we are connected to the chain
- // Do this in reverse order to ensure that any abandoning will work recursively
- for (auto& [_, wtx] : std::ranges::reverse_view(walletInstance->wtxOrdered)) {
- wtx->updateState(chain);
- // After updating the state of the tx, abandon if it is a 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.
- if (wtx->IsCoinBase() && wtx->isInactive()) {
- walletInstance->AbandonTransaction(*wtx);
+ // Update the state of every transaction now that we are connected to the chain.
+ for (auto& [_, wtx] : walletInstance->mapWallet) {
+ wtx.updateState(chain);
+ }
+
+ // After updating 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& [_, wtx] : walletInstance->mapWallet) {
+ if (wtx.IsCoinBase() && wtx.isInactive()) {
+ walletInstance->AbandonTransaction(wtx);
}
}
-
if (tip_height && *tip_height != rescan_height)
{
We want to minimize iterating the entire set of transactions since there can be a lot of them, and be very slow. Iterating in reverse order allows us to do both at the same time.
59 | + self.nodes[0].loadwallet("target") 60 | + wallet = self.nodes[0].get_wallet_rpc("target") 61 | + assert_greater_than(wallet_height, wallet.getwalletinfo()["lastprocessedblock"]["height"]) 62 | + 63 | + # Wait for node0 to be synced 64 | + self.sync_all()
Can we also check that the wallet's processed block has caught back up to the node tip after the restart catch-up path?
diff --git a/test/functional/wallet_chain_reprocess.py b/test/functional/wallet_chain_reprocess.py
--- a/test/functional/wallet_chain_reprocess.py (revision 866e54d2e3f0851256950ad6c737c6032f67527d)
+++ b/test/functional/wallet_chain_reprocess.py (revision 6c6e9e004a4c698a4a1a691d1fb09863271d173f)
@@ -62,6 +62,7 @@
# Wait for node0 to be synced
self.sync_all()
+ assert_equal(wallet.getwalletinfo()["lastprocessedblock"]["height"], self.nodes[0].getblockchaininfo()["blocks"])
# Check every transaction is confirmed
for txid in txids:
Done
I just jumped here for a different reason, the change makes high-level sense, but I'm not well-versed enough with the details to provide a more meaningful review, left two nits.