wallet: fix rescanning inconsistency

mzumsande commented at 6:01 pm on January 9, 2025: contributor

If the chain advances during a rescan, ScanForWalletTransactions would previously process the new blocks without adjusting m_last_processed_block, which would leave the wallet in an inconsistent state temporarily, and could lead to crashes in the GUI reported in #31474. Fix this by not rescanning blocks beyond m_last_processed_block - for all blocks beyond that height, there will be pending BlockConnected notifications that will process them after the rescan is finished.

This means that if rescanning was triggered with cs_wallet permanently held (AttachChain), additional blocks that were connected during the rescan will only be processed with the pending blockConnected notifications after the lock is released. If rescanning without a permanent cs_wallet lock (RescanFromTime), additional blocks that were connected during the rescan can be re-processed here because m_last_processed_block was already updated by blockConnected.

Fixes #31474

If the chain advances during a rescan, ScanForWalletTransactions
would previously process the new blocks without adjusting m_last_processed_block,
which would leave the wallet in an inconsistent state temporarily, and could lead
to crashes in the GUI.
Fix this by not rescanning blocks beyond the last_processed_block -
for all blocks beyond that height, there will be pending BlockConnected
notifications that will process them after the rescan is finished.

Co-authored-by: Pablo Greco <psgreco@gmail.com>

4818da809f

DrahtBot commented at 6:01 pm on January 9, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31629.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	psgreco, furszy, achow101
Approach NACK	luke-jr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30221 (wallet: Ensure best block matches wallet scan state by achow101)

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.

DrahtBot added the label Wallet on Jan 9, 2025

maflcko added this to the milestone 29.0 on Jan 10, 2025

psgreco commented at 6:14 pm on January 10, 2025: contributor

Not that it matters much, but UTACK 4818da809f0e300016390dd41e02c6067ffa008f

This is what fixed the issue in Elements

luke-jr commented at 9:36 pm on January 14, 2025: member

Approach NACK.

Seems like we already have logic for this in max_height, and if it’s not passed, we intend to not stop early… I think adjusting m_last_processed_block instead would be the right fix.

mzumsande commented at 10:26 pm on January 14, 2025: contributor

I think adjusting m_last_processed_block instead would be the right fix.

I wanted to do that initially, but I think it’s incorrect: If we set m_last_processed_block for block 1 until block N within the rescan, and after the rescan has finished we process the outstanding blockConnected notification for block 1, m_last_processed_block will be set backwards to the the wrong block, and the wallet will be in a inconsistent state until all other outstanding notifications until block N are processed as well.

Seems like we already have logic for this in max_height

Setting max_height in AttachChain() is an independent fix from adjusting m_last_processed_block, and it would solve the problem there. But I think that other existing callers of ScanForWalletTransactions that don’t want to set max_height can have the same problem too: These callers don’t lock the wallet permamently, and for them, reprocessing additional blocks during the rescan will be correct if the notification was already processed, incorrect if it was pending, so there could be race for them on current master. I like that the current solution fixes the issue for these other callers as well. Besides, if we set max_height we’d need to do more changes, because mempool scanning is currently coupled to max_height not being set (but that might be good to decouple anyways).

glozow requested review from achow101 on Feb 13, 2025

glozow added the label Bug on Feb 13, 2025

glozow removed this from the milestone 29.0 on Feb 13, 2025

furszy commented at 9:45 pm on February 13, 2025: member

After a few conversations with mzumzande, it seems to me that we could split the “scan blocks beyond the last processed block” issue into two scenarios:

(1) when the wallet mutex is locked for the entire time the rescanning process is executed. And (2) when the wallet mutex is not locked for the entire rescanning time.

(1) Is #31474 case: Given that we set the “last processed block” to the chain tip just before calling the initial rescan. I do think this process expects to stop at the “last processed block” as any new coming block will be processed after this process finishes. The only thing preventing us from fixing this just by providing the correct max_height is that the mempool scanning process is tied to it. But I don’t see why this should always be the case. I’d advocate for decoupling it, as scanning the mempool could be useful independently of rescanning the blockchain.

(2) is therescanblockchain/importdescriptors/importmulti cases: Because in this case we do not keep the wallet mutex locked the entire time, new blocks are processed during rescan rather than after it, which makes this trickier to solve because while rescanning is in progress, the wallet may receive a new block event and ignore it if it doesn’t contain any relevant transactions. But.. what if rescanning later finds a relevant script? That would mean the wallet’s look-ahead script pool will be expanded, potentially leading to new scripts being detected in previously ignored blocks. So the user might need to rescan those blocks too.

I think we could solve the first case here, so it can get inside the upcoming release, and leave the second case for a follow-up. Also, I’m not really against the fix proposed in this PR, it works. It just feels a bit hacky.

mzumsande commented at 5:04 pm on February 14, 2025: contributor

I think we could solve the first case here, so it can get inside the upcoming release, and leave the second case for a follow-up. Also, I’m not really against the fix proposed in this PR, it works. It just feels a bit hacky.

See https://github.com/mzumsande/bitcoin/tree/202502_rescan_initfix for this alternative. Happy to change it to that if people like it better. This fixes the issue during AttachChain() and is admittedly nicer, but it wouldn’t fix the following race:

User runs rescanblockchain rpc with no stop height -> ScanForWalletTransactions runs without permanent lock of the wallet mutex
Node connects a block -> race between ScanForWalletTransactions and blockConnected signal
ScanForWalletTransactions processes this blocks first (so that m_last_processed_block is behind the actual last processed block).
wallet is temporarily in a inconsistent state / GUI can crash (until the queued blockConnected signal is executed).

Not requiring a stop height during rescanblockchain (or internally setting it to the tip at its start) could be seen a feature that makes the need for repeated scans less likely (as furszy described above), so the same approach as in AttachChain probably wouldn’t be great there?!

furszy commented at 10:33 pm on February 14, 2025: member

utACK 4818da809f0e300016390dd41e02c6067ffa008f

DrahtBot requested review from luke-jr on Feb 14, 2025

achow101 commented at 10:35 pm on February 14, 2025: member

ACK 4818da809f0e300016390dd41e02c6067ffa008f

achow101 merged this on Feb 14, 2025

achow101 closed this on Feb 14, 2025

mzumsande deleted the branch on Feb 19, 2025

wallet: fix rescanning inconsistency #31629

Code Coverage & Benchmarks

Reviews

Conflicts