Merge bitcoin/bitcoin#31629: wallet: fix rescanning inconsistency

4818da8 wallet: fix rescanning inconsistency (Martin Zumsande)

Pull request description:

  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

ACKs for top commit:
  psgreco:
    Not that it matters much, but UTACK 4818da8
  achow101:
    ACK 4818da8
  furszy:
    utACK 4818da8

Tree-SHA512: 8e7dbc9e00019aef4f80a11776f3089cd671e0eadd3c548cc6267b5c722433f80339a9b2b338ff9b611863de75ed0a817a845e1668e729b71af70c9038b075af

Author: achow101

src/wallet/wallet.cpp

@@ -1990,6 +1990,14 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
         if (max_height && block_height >= *max_height) {
             break;
         }
+        // If rescanning was triggered with cs_wallet permanently locked (AttachChain), additional blocks that were connected during the rescan
+        // aren't processed here but will be processed with the pending blockConnected notifications after the lock is released.
+        // If rescanning without a permanent cs_wallet lock, additional blocks that were added during the rescan will be re-processed if
+        // the notification was processed and the last block height was updated.
+        if (block_height >= WITH_LOCK(cs_wallet, return GetLastBlockHeight())) {
+            break;
+        }
+
         {
             if (!next_block) {
                 // break successfully when rescan has reached the tip, or
@@ -3272,12 +3280,12 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf
     }
 
     // Register wallet with validationinterface. It's done before rescan to avoid
-    // missing block connections between end of rescan and validation subscribing.
-    // Because of wallet lock being hold, block connection notifications are going to
-    // 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
+    // missing block connections during the rescan.
+    // Because of the wallet lock being held, block connection notifications are going to
+    // be pending on the validation-side until lock release. Blocks that are connected while the
+    // rescan is ongoing will not be processed in the rescan but with the block connected notifications,
+    // so the wallet will only be completeley synced after the notifications delivery.
+    // 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.