wallet: ignore chainStateFlushed notifications while attaching chain #24984

Fixes #24487

When a rescan is performed during CWallet::AttachChain() (e.g. when loading an old wallet) but this is interrupted by a shutdown signal, the wallet will currently stop the rescan, receive a chainStateFlushed signal, set the saved best block to the tip and shut down. At next startup, the rescan is not continued or repeated because of this. But some blocks have never been scanned by the wallet, which could lead to an incorrect balance.

Fix this by ignoring chainStateFlushed notifications until the chain is attached. Since CWallet::chainStateFlushed is being manually called by AttachChain() anyway after finishing with the rescan, it is not a problem if intermediate notifications are ignored.

Manual rescans started / aborted by the rescanblockchain / abortrescan RPCs are not affected by this.

I didn't choose alternative ways of fixing this issue that would delay the validationinterface registration or change anything else about the handling of blockConnected signals for the reasons mentioned in this existing comment.

This breaks feature_pruning.py, putting into draft until I have analyzed it.

Fixed.

With current implementation we don't save the rescan progress and on the next load we will have to start from scratch. Is it possible to save the last processed blocked at shutdown?

With current implementation we don't save the rescan progress and on the next load we will have to start from scratch. Is it possible to save the last processed blocked at shutdown?

Yes, that's true. I think it should be possible to call chainStateFlushed with a locator to the block that was last processed - I'll look into that. Of course it would save some time only in the rather rare situation of a clean shutdown during rescan (the bug must have existed for years but wasn't reported until very recently), so I'm not sure how important a feature like this would actually be for users.

Concept ACK

so I'm not sure how important a feature like this would actually be for users.

Right, I'm concerned about unexpected behavior. I don't think optimality is important in this case.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #23997 (wallet: avoid rescans under assumed-valid blocks by jamesob)

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.

Yes, that's true. I think it should be possible to call chainStateFlushed with a locator to the block that was last processed - I'll look into that. Of course it would save some time only in the rather rare situation of a clean shutdown during rescan (the bug must have existed for years but wasn't reported until very recently), so I'm not sure how important a feature like this would actually be for users.

The wallet already tracks last processed block, why can't we just write that form chainStateFlushed handler regardless of what locator was passed in. That way we don't need any bool flags I believe.

The wallet already tracks last processed block, why can't we just write that form chainStateFlushed handler regardless of what locator was passed in. That way we don't need any bool flags I believe.

Not sure I understand this right - do you mean why locators are used in general instead of single block hashes? I think that's because we want to be flexible and not dependent on a specific block: There may be a reorg so a given block is no longer part of the chain when the wallet is loaded, or maybe the the wallet called the chainStateFlushed handler manually without the actual chainstate being flushed, and then an unclean shutdown happens so that the wallet would be ahead of the chain at next startup.

In any case, any block must be converted to a locator anyway to be stored in BESTBLOCK_NOMERKLE in order not to break the db format.

Also note that AttachChain() updates m_last_block_processed optimistically to the tip (before actually processing any missing blocks) and not incrementally. So if it gets interrupted early, m_last_block_processed is not correct.

Code review ACK 2052e3aa9aa666bdc86dac370f1dd8fb978d3497. This is a straightforward fix for the bug described in #24487 where a wallet could skip scanning blocks if is shut down in the middle of a sync and a chainStateFlushed notification was received during the sync. It would be nice to write a test for this but probably would be tricky to write.

S3RK raises a great point in #24984#pullrequestreview-952900468 that the wallet doesn't save the scan position during syncs, so if it is interrupted, parts of the scan could be repeated unnecessarily next time the wallet is loaded. This issue would be good to fix, and I created #25010 to track it, but I think it's basically a separate issue with a separate fix. This PR makes that issue more likely to happen, but in the cases it does this, current behavior of skipping unscanned blocks is worse than new behavior of rescanning already-scanned blocks, so in every case this PR should be a strict improvement over the status quo.

ACK 2052e3aa9aa666bdc86dac370f1dd8fb978d3497

Agree that "this PR should be a strict improvement over the status quo".

The only thing that still slightly worries me is two return false statements between setting and unsetting the m_attaching_chain flag. In both cases rescan fails, so I guess the behaviour is still correct, but it looks fragile.

It would be nice to write a test for this but probably would be tricky to write.

Yes, I'd like that too. the problem is how to deal with the racing condition, timing the shutdown to be during the sync and not after. I played around a bit with bpftrace / uprobes after an idea by @jnewbery (e.g. hook into a function such as SyncTransaction() and send a SIGINT when it is invoked the 20th time) and that is really cool and seems to work, but obviously still very experimental and not ready for the CI.

The only thing that still slightly worries me is two return false statements between setting and unsetting the m_attaching_chain flag. In both cases rescan fails, so I guess the behaviour is still correct, but it looks fragile.

Yes, we currently rely on this to be the case. We aren't only concerned about accidental signals during the sync but also after it, caused by the interrupting signal: In case of a shutdown by SIGINT, Shutdown() in Init will create a chainStateFlushed signal which is processed by the wallet. At this point in time, m_attaching_chain must still be true or the bug is not fixed.

Code Review ACK https://github.com/bitcoin/bitcoin/pull/24984/commits/2052e3aa9aa666bdc86dac370f1dd8fb978d3497

If I understand correctly, the problem is that at the end of CWallet::AttachChain(), CWallet::chainStateFlushed(...) is called passing the chain tip as parameter, even if abort or shutdown is requested during the wallet scan (CWallet::ScanForWalletTransactions).

I think this solution achieves the goal of preventing the wallet from being in an inconsistent state by forcing it to write the tip only after the scan completes.

Ideally there would be a functional test for this, but I don't see how to simulate this on regtest.