#35294 wallet: Update tx chain state during loading during AttachChain instead of before
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35294 wallet: Update tx chain state during loading during AttachChain instead of before | Waiting on author | 0 (+2) | Blocked | 13 + 81 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Defers wallet transaction state updates and inactive coinbase abandonment from the initial database loading phase to CWallet::AttachChain. In AttachChain, states are refreshed across all transactions in reverse order after chain notification handlers are registered. This ensures that any blocks missing between the wallet's stored best block and the chain tip are rescanned before transaction states are evaluated.
Problem: Updating transaction states against the chain prior to AttachChain can miss blocks connected between the wallet's stored tip and the active chain tip. This leaves wallet transaction states inconsistent with the active chain, causing assertion failures such as `!wtx.isConfirmed()` and allowing confirmed transactions to be incorrectly eligible for abandonment.
Evidence of importance:
- Fixes linked issue #34599 ('wallet: Assertion `!wtx.isConfirmed()` failed')
- Commit fcfec4b7a9 adds a dedicated functional test (wallet_chain_reprocess.py) demonstrating that wallet transaction states and confirmation statuses remain consistent after an unclean shutdown
Reviewability
Paused · Waiting on author
Open objections from l0rinc with no author reply for 40 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: respond to l0rinc's objection about reverse iteration order and suggestion for unit test coverage 2026-09-22
- author: response to l0rinc's change request on reverse iteration ordering and suggestion for unit test coverage 2026-09-22
Agreement
Blocked
objection that needs a change, nobody has replied (l0rinc)
Thread notes: Re-evaluated rc:4067859704 as a suggestion rather than an objection under schema 2 rules, and recorded questions and suggestions raised by rkrux, l0rinc, and pablomartin4btc. / Re-read processed suggestions and questions from rkrux, l0rinc, and pablomartin4btc that were previously omitted. Converted rc:4067859704 from objection to suggestion as it requests characterization test coverage without alleging a defect in code.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| rkrux (member) | suggestion, style | remove commented-out extra_args line in functional test | fixed · fix pushed · author removed the commented-out line | answered | yes | 2026-05-19: '- #self.extra_args = [["-checkblockindex=0"]]' Status from: 2026-05-19: achow101: 'Removed' |
| rkrux (member) | suggestion, maintenance | remove load_on_startup=False argument from createwallet call in test | fixed · fix pushed · author removed the argument | answered | yes | 2026-05-19: 'This argument doesn't seem necessary.' Status from: 2026-05-19: achow101: 'Removed' |
| rkrux (member) | question, approach | is there a particular reason to mine 1000 blocks in the test | answered · fix pushed · author explained the rationale and reduced the block count to 100 | answered | yes | 2026-05-19: 'Is there a particular reason to mine 1000 blocks?' Status from: 2026-05-19: achow101: '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.' |
| rkrux (member) | objection, correctness | transaction states may not be fully updated when abandonment happens in the same forward loop | fixed · fix pushed · author updated to reverse traversal | changed | 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.' Status from: 2026-05-19: achow101: 'Updated' |
| rkrux (member) | suggestion, maintenance | mention the reason for the block count as a comment in the test | fixed · fix pushed · author added a comment and reduced the count to 100 | answered | yes | 2026-05-19: 'Either way, it would be very helpful to mention the reason as a comment in the test here for review and future reference.' Status from: 2026-05-19: achow101: 'Added a comment and reduced to 100.' |
| rkrux (member) | suggestion, maintenance | remove wallet.unloadwallet() from test as best block is written to disk on every block connected notification | answered · author explained unloading is needed to prevent data loss with -unsafesqlitesync | answered | yes | 2026-05-25: '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.' Status from: 2026-05-26: achow101: 'Since the tests run with `-unsafesqlitesync`, not all data will be flushed at any given time.' |
| rkrux (member) | suggestion, correctness | add assertions to the test regarding node and wallet height states | fixed · fix pushed · author added the suggested assertions | answered | no | 2026-05-25: 'Considering the subtleness of the issue, can we add few assertions here to make it explicit for the reader regarding node/wallet height states?' Status from: 2026-05-26: achow101: 'Done' |
| maflcko (member) | objection, correctness | intermittent CI assertion failure in wallet_chain_reprocess.py | fixed · fix pushed · author pushed a fix for the CI test failure | changed | yes | 2026-05-28: 'wallet_chain_reprocess.py failed ... AssertionError: 302 <= 302' Status from: 2026-05-28: achow101: 'Hopefully fixed' |
| l0rinc (member) | suggestion, approach | perform state updates and abandonments in separate passes instead of reverse traversal | answered · author explained reverse order allows doing both in one pass to avoid iterating large tx sets twice | answered | yes | 2026-06-05: 'Do we need the reverse traversal here, or could we do the state updates and abandonments in separate passes instead?' Status from: 2026-06-05: achow101: 'We want to minimize iterating the entire set of transactions since there can be a lot of them, and be very slow.' |
| l0rinc (member) | suggestion, correctness | check that the wallet's processed block caught up to the node tip after restart | fixed · fix pushed · author added the assertion | answered | yes | 2026-06-05: 'Can we also check that the wallet's processed block has caught back up to the node tip after the restart catch-up path?' Status from: 2026-06-05: achow101: 'Done' |
| pablomartin4btc (member) | suggestion, safety | add AssertLockHeld to AbandonTransaction(CWalletTx&) | answered · author declined, pointing to the existing EXCLUSIVE_LOCKS_HELD annotation | answered | yes | 2026-08-06: 'just noticed that there's no `AssertLockHeld` in `AbandonTransaction(CWalletTx&)`: this new call site in `AttachChain` relies on `cs_wallet` being held (which it is), but the overload doesn't assert it' Status from: 2026-08-26: achow101: 'Leaving this as is. There is also an `EXCLUSIVE_LOCKS_HELD` annotation.' |
| pablomartin4btc (member) | suggestion, maintenance | add a comment explaining the ordering dependency where updateState runs before rescan | open · author deferred addressing this | answered | yes | 2026-08-06: 'I think a comment explaining this ordering dependency perhaps would help future readers (?).' |
| l0rinc (member) | objection, correctness | reverse iteration may not visit child transactions before parents if wallet insertion order does not reflect dependency order | open | changed | no | 2026-09-22: 'I'm not yet fully convinced that reverse iteration always visits children before their parents, since wallet insertion order doesn't necessarily reflect dependency order as far as I can tell.' |
| l0rinc (member) | suggestion, correctness | add a unit test covering the ordering bug since the functional test passes with the fix reverted | open | answered | no | 2026-09-22: 'the test still passes with the fix reverted. It's a useful test that covers wallet catch-up, but doesn't distinguish the broken behavior from the fixed behavior' |
Support:
- shuv-amp (contributor) (ACK, 2026-05-19) [both: AttachChain(), LoadToWallet(), src/wallet/wallet.cpp, test/functional/wallet_chain_reprocess.py]: Moving the tx state update out of LoadToWallet() prevents updating against a moving chain tip before notifications are attached, verified that test reproduces issue on parent and passes on PR
- rkrux (member) (Concept ACK, 2026-05-19): This is indeed quite a subtle and peculiar issue! [not substantive]
- dergoegge (member) (tACK, 2026-05-22) [tested]: This fixes the issue.
- l0rinc (member) (2026-06-05) [read]: the change makes high-level sense [not substantive]
- dergoegge (member) (ACK, 2026-07-24): (no reason given) [not substantive]
- pablomartin4btc (member) (Concept ACK, 2026-08-06) [read: AttachChain, LoadExisting, PopulateWalletFromDB, src/wallet/wallet.cpp]: closes a real race where the chainstate could advance during PopulateWalletFromDB and cause AbandonTransaction to hit an assertion failure
Participants:
- rkrux (member, objection, 10 statements 2026-05-19 to 2026-05-25): Concept ACKed, pointed out issue with tx state updates and abandon in the same loop, and offered test suggestions
- shuv-amp (contributor, support, 1 statements 2026-05-19 to 2026-05-19): tested and ACKed the change, confirming it fixes the issue and test passes
- dergoegge (member, support, 3 statements 2026-05-21 to 2026-07-24): ran Antithesis and gave tACK and utACK confirming the fix
- fanquake (member, neutral, 1 statements 2026-05-21 to 2026-05-21): asked whether CI was ready for an Antithesis run
- maflcko (member, objection, 1 statements 2026-05-28 to 2026-05-28): reported an intermittent CI test assertion failure
- l0rinc (member, objection, 5 statements 2026-06-05 to 2026-09-22): requested changes, questioning whether reverse iteration visits children before parents and noting the functional test passes without the fix
- pablomartin4btc (member, support, 6 statements 2026-08-06 to 2026-08-06): Concept ACK with detailed walkthrough of how the fix prevents the race condition
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
The short text the ranking pass compares across PRs.
This pull request defers wallet transaction state updates and inactive coinbase abandonment from database loading to CWallet::AttachChain after chain notifications are attached. Previously, updating states before attaching chain handlers could miss blocks between the stored wallet state and the current tip, leading to inconsistent transaction states. It fixes issue #34599, where an assertion failed on an inconsistent confirmation status during wallet load. A new functional test verifies that transaction states remain correct after an unclean shutdown.
Data
thread read through 2026-09-22 (40 statements), head fcfec4b7a9
labels: Wallet
Categories
Wallet
P1 · bug fix
Fixes an assertion failure (`!wtx.isConfirmed()`) and wallet transaction state inconsistency where confirmed transactions could be erroneously eligible for abandonment after missing connected blocks during load (fixing issue #34599). This prevents wallet crashes and incorrect transaction states following unclean shutdowns or concurrent chain advancement.
Membership: Touches src/wallet/wallet.cpp and src/wallet/walletdb.cpp and carries the maintainer Wallet label.
Factors: security/stability 2, bug 2, performance 0, user value 1, leverage 0