Repository: bitcoin/bitcoin (language: cpp)

Issue:
Automatic wallet rescan skipped after abort
I ran bitcoin-qt with an old wallet, causing it to rescan, then stopped it with a SIGINT signal, resulting in the following log. Nothing unexpected, so far:
```
2022-03-05T16:01:03Z init message: Loading wallet…
2022-03-05T16:01:03Z BerkeleyEnvironment::Open: LogDir=…/.bitcoin/database ErrorFile=…/.bitcoin/db.log
2022-03-05T15:57:18Z [walletname.dat] Wallet File Version = 159900
2022-03-05T15:57:18Z [walletname.dat] Keys: 2397 plaintext, 0 encrypted, 2397 w/ metadata, 2397 total. Unknown wallet records: 0
2022-03-05T15:57:18Z [walletname.dat] Wallet completed loading in              65ms
2022-03-05T15:57:18Z init message: Rescanning…
2022-03-05T15:57:18Z [walletname.dat] Rescanning last 134072 blocks (from block 591711)...
2022-03-05T15:57:18Z [walletname.dat] Rescan started from block 000000000000000000075fcc1872da61f45bcfd0e0bb36b7c8cb45e7a328ad41...
2022-03-05T15:58:18Z [walletname.dat] Still rescanning. At block 593552. Progress=0.632716
2022-03-05T15:59:18Z [walletname.dat] Still rescanning. At block 595458. Progress=0.638084
2022-03-05T15:59:37Z [walletname.dat] Rescan interrupted by shutdown request at block 596080. Progress=0.639932
2022-03-05T15:59:37Z Error: Failed to rescan the wallet during initialization
2022-03-05T15:59:41Z GUI: initializeResult : Initialization result:  false
2022-03-05T15:59:41Z GUI: requestShutdown : Requesting shutdown
2022-03-05T15:59:41Z GUI: Running Shutdown in thread
2022-03-05T15:59:41Z Shutdown: In progress...
2022-03-05T15:59:41Z scheduler thread exit
2022-03-05T15:59:42Z Shutdown: done
2022-03-05T15:59:42Z GUI: Shutdown finished
2022-03-05T15:59:42Z GUI: ~InitExecutor : Stopping thread
2022-03-05T15:59:42Z GUI: ~InitExecutor : Stopped thread
```

However, the next time it was launched, the rescan didn't continue, nor did it start over. It seems to have been skipped?
```
2022-03-05T16:01:03Z init message: Loading wallet…
2022-03-05T16:01:03Z BerkeleyEnvironment::Open: LogDir=…/.bitcoin/database ErrorFile=…/.bitcoin/db.log
2022-03-05T16:01:03Z [walletname.dat] Wallet File Version = 159900
2022-03-05T16:01:03Z [walletname.dat] Keys: 2397 plaintext, 0 encrypted, 2397 w/ metadata, 2397 total. Unknown wallet records: 0
2022-03-05T16:01:03Z [walletname.dat] Wallet completed loading in              91ms
2022-03-05T16:01:03Z [walletname.dat] setKeyPool.size() = 2000
2022-03-05T16:01:03Z [walletname.dat] mapWallet.size() = 378
2022-03-05T16:01:03Z [walletname.dat] m_address_book.size() = 183
2022-03-05T16:01:03Z Loaded 2 addresses from "anchors.dat"
```

I didn't investigate deeply, but could it be that despite the failed rescan, the new tip block was stored in the wallet? if so, this would be confusing.


Discussion / hints:
I would guess that because the signals are subscribed before the rescan (in `CWallet::AttachChain`), when the chain tip changes during rescan,
```
CMainSignals::ChainStateFlushed → CWallet::chainStateFlushed
```
which invokes `WriteBestBlock`, even though the rescan doesn't complete.

> (...) when the chain tip changes during rescan (...)

I think that it wouldn't even be necessary that the chain tip changes during the rescan (which would only occasionally cause `chainStateFlushed`), but SIGINT causes the `chainStateFlushed` signal itself during Shutdown.


> I think that it wouldn't even be necessary that the chain tip changes during the rescan 

Good point! 

I guess, strictly, the signal should only cause a best block update if the last best block matches the last chain tip. But it's more straightforward to implement in terms of a boolean `in_sync` that gets set when already in sync with the chain, or on *successful* rescan. But maybe there's other options as well.

I haven't looked how the indexes handle this situation.

Edit: added 23.0 milestone, I think this is a sufficiently serious issue because it might result in perceived (fixable by requesting an explicit rescan) missing funds.
> I haven't looked how the indexes handle this situation.

I don't know much about the wallet but I recently spent some time looking at index stability in this and similar situations: 
The indexes do pretty much what you suggested, they have a bool `m_synced` and don't process [chainStateFlushed](https://github.com/bitcoin/bitcoin/blob/5e49b2a2529adc737175ebdcbf312635c52b05f6/src/index/base.cpp#L287) or [blockConnected](https://github.com/bitcoin/bitcoin/blob/5e49b2a2529adc737175ebdcbf312635c52b05f6/src/index/base.cpp#L245) callbacks unless synced successfully.

I could locally reproduce that blocks are skipped if a rescan is aborted and agree that it would be good to fix this.

Fix patch:
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 0d7075810db71..34247d14e3a3d 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -523,6 +523,11 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase,
 
 void CWallet::chainStateFlushed(const CBlockLocator& loc)
 {
+    // Don't update the best block until the chain is attached so that in case of a shutdown,
+    // the rescan will be restarted at next startup.
+    if (m_attaching_chain) {
+        return;
+    }
     WalletBatch batch(GetDatabase());
     batch.WriteBestBlock(loc);
 }
@@ -2940,6 +2945,8 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf
     // be pending on the validation-side until lock release. It's likely to have
     // block processing duplicata (if rescan block range overlaps with notification one)
     // but we guarantee at least than wallet state is correct after notifications delivery.
+    // However, chainStateFlushed notifications are ignored until the rescan is finished
+    // so that in case of a shutdown event, the rescan will be repeated at the next start.
     // This is temporary until rescan and notifications delivery are unified under same
     // interface.
     walletInstance->m_chain_notifications_handler = walletInstance->chain().handleNotifications(walletInstance);
@@ -2968,6 +2975,7 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf
 
     if (tip_height && *tip_height != rescan_height)
     {
+        walletInstance->m_attaching_chain = true; //ignores chainStateFlushed notifications
         if (chain.havePruned()) {
             int block_height = *tip_height;
             while (block_height > 0 && chain.haveBlockOnDisk(block_height - 1) && rescan_height != block_height) {
@@ -3007,6 +3015,7 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf
                 return false;
             }
         }
+        walletInstance->m_attaching_chain = false;
         walletInstance->chainStateFlushed(chain.getTipLocator());
         walletInstance->GetDatabase().IncrementUpdateCounter();
     }
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 26b7f97b5f5f1..80b6845ae6c3c 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -237,6 +237,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
 
     std::atomic<bool> fAbortRescan{false};
     std::atomic<bool> fScanningWallet{false}; // controlled by WalletRescanReserver
+    std::atomic<bool> m_attaching_chain{false};
     std::atomic<int64_t> m_scanning_start{0};
     std::atomic<double> m_scanning_progress{0};
     friend class WalletRescanReserver;


Test patch: