wallet: fix rescanning inconsistency #31629

1. 
   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

2. wallet: fix rescanning inconsistency

   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
3. 
   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
   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

   No conflicts as of last run.

4. DrahtBot added the label Wallet on Jan 9, 2025
5. maflcko added this to the milestone 29.0 on Jan 10, 2025
6. 
   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

7. 
   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.

8. 
   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).

Contributors
mzumsande DrahtBot psgreco luke-jr

Labels
Wallet

Milestone
29.0