Skip to content

wallet: Avoid potentially writing incorrect best block locator

Placeholder Ryan Ofsky requested to merge github/fork/ryanofsky/pr/noloc into master

This PR updates the CWallet::chainStateFlushed method to write the m_last_block_processed locator to the database as the wallet's best block instead of the chain tip locator.

Saving the last block processed locator as the best block is less fragile than saving chainStateFlushed locator as best block.

Right now wallet code relies on blockConnected notifications being sent before chainStateFlushed notifications, and on the chainStateFlushed locator pointing to the last connected block. But this is a fragile assumption that can be easily broken by ActivateBestChain/ConnectTrace changes, and in fact is currently broken (since d96c59cc from #25740) when the assumeutxo snapshot block is validated. In that case the background validation chainStateFlushed notification is sent before the blockConnected notification so writing the locator as the best block would record a best block that was never processed. This specific change does not cause a problem for the wallet, because the wallet ignores events from the background validation chainstate. But nothing prevents similar out-of-order chainStateFlushed notifications from being sent in the future, so it is safer for the wallet to not rely on the chain tip sent in chainStateFlushed notifications.

A good followup to this change would probably be to drop the ChainStateFlushed locator argument entirely so it is not misused in the future. Indexing code already stopped using this locator argument a long time ago for identifying the best block (in 4368384f from #14121), though it is currently using the locator argument to log diagnostic warnings.

Merge request reports

Loading