#35294 wallet: Update tx chain state during loading during AttachChain instead of before
https://github.com/bitcoin/bitcoin/pull/35294 · · +94/-13 in 4 files, 2 commits · labels: Wallet
Goal
- Prevent node crashes caused by assertion failures when loading a wallet while blocks are actively syncing
This PR moves wallet transaction chain state updates and inactive coinbase abandonment out of the database loading phase and into `AttachChain`, occurring strictly after chain notification handlers are registered. It also processes transactions in reverse order to ensure descendant transactions update before parent coinbases are evaluated for abandonment, and adds a regression test simulating an unclean shutdown during active block production.
Problem: When loading a wallet, querying chainstate before chain notifications are attached allows the chain tip to advance between individual transaction loads. This can leave child transactions marked confirmed while parent coinbases appear inactive, triggering an assertion failure (`assert(!wtx.isConfirmed())`) and crashing the node (#34599).
Category: Wallet (#11 of 84)
P2 · bug fix
- P2 because it fixes an assertion failure crash when loading a wallet during active block connection
- Ensures transaction state is evaluated against a coherent chain tip rather than a moving target
P2 because it fixes a wallet assertion crash triggered during wallet loading if the chain tip moves before notifications are attached (#34599). While the condition requires an unclean shutdown or rapid block generation while loading, the resulting crash halts the node.
Membership: Changes CWallet::AttachChain, CWallet::LoadToWallet, and wallet database loading logic in src/wallet/.
Factors: security/stability 2, bug 2, performance 0, user value 1, leverage 0
Reviewability: Ready
- Ready to review; the patch is small, cleanly rebased, and all feedback has been addressed
CI is clean, test failures from earlier revisions were fixed, and all reviewer feedback has been addressed.
Author status: active
Resolved concerns:
- rkrux noted that abandonment inside the state update loop could cause issues; author adjusted the order and added reverse traversal.
- rkrux suggested reducing the test block count from 1000 to 100 and adding explicit height assertions; author updated the test.
- maflcko reported a CI failure in the functional test; author pushed a fix.
- l0rinc asked about separating state update and abandonment into two passes; author clarified that a single reverse pass minimizes expensive iterations over large wallets.
Agreement: Strong
- Strong consensus that deferring state updates until chain attachment fixes the crash bug
- Verified the reproduction test fails before the patch and passes after (shuv-amp)
- Approved following Antithesis testing (dergoegge)
- Verified race window specifics and confirmed safety for offline wallet tooling (pablomartin4btc)
Strong consensus with multiple ACKs confirming the race condition and bug fix.
Multiple maintainers and contributors examined the race condition in depth, verified that the test caught the bug, and tested the fix with positive reviews.
- shuv-amp (2026-05-19): 'The new test fails on the pre-fix parent with a wallet transaction still reporting 0 confirmations after restart, and passes on this PR head'
- dergoegge (2026-05-22): 'tACK 4de5c3b... This fixes the issue.'
- pablomartin4btc (2026-08-06): 'Concept ACK... closes a real race: previously, as each tx was loaded, updateState queried the live chainstate — which could advance between loading tx A and tx B.'
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| rkrux | correctness | wallet transaction states were not completely updated because abandonment was called within the same loop | resolved | no | yes | 2026-05-19: "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." Settled: 2026-05-19: "Updated" |
| maflcko | correctness | intermittent test failure in CI due to height assertion failure | resolved | no | yes | 2026-05-28: "wallet_chain_reprocess.py failed ... AssertionError: 302 <= 302" Settled: 2026-05-28: "Hopefully fixed" |
Support:
- shuv-amp: Moving tx state update to AttachChain ensures wallet no longer updates tx states against a moving tip without receiving intervening block connections
- dergoegge: Verified fix locally and via Antithesis runs
- pablomartin4btc: Moving updateState to AttachChain closes a real race where the chainstate advances during PopulateWalletFromDB
- rkrux: Concept ACK, noted the issue is subtle and the fix makes sense
- l0rinc: notes the change makes high-level sense [not substantive]
Participants: shuv-amp (support), rkrux (objection), fanquake (neutral), dergoegge (support), maflcko (objection), l0rinc (support), pablomartin4btc (support)
State derived from the lists: substantive support, no open objection (shuv-amp, dergoegge, pablomartin4btc, rkrux)
Review verdicts (DrahtBot): 0 (+2)
- Stale ACK: shuv-amp, dergoegge
- Concept ACK: rkrux, pablomartin4btc
Files
81 lines under test/bench/ci.
- test/functional/wallet_chain_reprocess.py +80/-0
- src/wallet/wallet.cpp +13/-5
- src/wallet/walletdb.cpp +0/-8
- test/functional/test_runner.py +1/-0
Card
This PR moves wallet transaction state updating and inactive coinbase abandonment into AttachChain after notification listeners are registered, resolving an assertion crash (#34599) where the active chain tip advances during database loading. It also adds a functional test reproducing the failure after an unclean shutdown. Review is mature with strong support and verified testing from multiple reviewers, and the code is ready for final review.