Merge bitcoin/bitcoin#35512: wallet: move fAbortRescan reset into WalletRescanReserver reserve function

2818a171c0 test: add abortscan unit test (Pol Espinasa)
bc30e95163 wallet: move fAbortRescan reset into WalletRescanReserver reserve() (Pol Espinasa)

Pull request description:

  Follow-up of https://github.com/bitcoin/bitcoin/pull/35179
  For extra context refer to the conversations https://github.com/bitcoin/bitcoin/pull/35179#discussion_r3378136210 and comments bellow it.

  Long story short: currently `ScanForWalletTransactions()` resets the value of `fAbortRescan` before starting the rescan loop. This can cause a race condition where some function (e.g. `importdescriptors`) starts a rescan and at the same time the user aborts it manually. Could happen that the `abortrescan` call returns True (success) but the rescan continues running as the value is overwritten.

  This PR fixes this by resetting the value of `fAbortRescan` at the very beginning, when the wallet rescan is reserved, removing the race condition. Also adds a test for it.

ACKs for top commit:
  achow101:
    ACK 2818a171c0
  w0xlt:
    ACK 2818a171c0
  nebula-21:
    ACK 2818a171c0
  pablomartin4btc:
    ACK 2818a17
  pinheadmz:
    ACK 2818a171c0

Tree-SHA512: 5b64b9a16a209dd145ccf7b2d7c3a7205038b3b214b02c3d9f1cbda75a6a1c9a41ad5c42cbeda43af6141793965613c47e44300d758149be9bfdf458e85a5520

src/wallet/test/wallet_tests.cpp

@@ -213,6 +213,36 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
     }
 }
 
+BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions_abort, TestChain100Setup)
+{
+    CWallet wallet(m_node.chain.get(), "", CreateMockableWalletDatabase());
+    uint256 genesis_hash;
+    {
+        LOCK(wallet.cs_wallet);
+        LOCK(Assert(m_node.chainman)->GetMutex());
+        wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
+        wallet.SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
+        genesis_hash = m_node.chainman->ActiveChain().Genesis()->GetBlockHash();
+    }
+
+    // An abort requested while no rescan is held is stale and must
+    // not cancel a later scan.
+    wallet.AbortRescan();
+    WalletRescanReserver reserver(wallet);
+    BOOST_CHECK(reserver.reserve());
+    BOOST_CHECK(!wallet.IsAbortingRescan());
+
+    // An abort requested after the reservation but before the scan starts
+    // (e.g. while importdescriptors is still deriving keys) must cancel the
+    // scan.
+    wallet.AbortRescan();
+    CWallet::ScanResult result = wallet.ScanForWalletTransactions(genesis_hash, /*start_height=*/0, /*max_height=*/{}, reserver, /*save_progress=*/false);
+    BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::USER_ABORT);
+    BOOST_CHECK(result.last_scanned_block.IsNull());
+    BOOST_CHECK(!result.last_scanned_height);
+    BOOST_CHECK(result.last_failed_block.IsNull());
+}
+
 // This test verifies that wallet settings can be added and removed
 // concurrently, ensuring no race conditions occur during either process.
 BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurrently, TestingSetup)

src/wallet/wallet.cpp

@@ -1884,7 +1884,6 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
     WalletLogPrintf("Rescan started from block %s... (%s)\n", start_block.ToString(),
                     fast_rescan_filter ? "fast variant using block filters" : "slow variant inspecting all blocks");
 
-    fAbortRescan = false;
     ShowProgress(strprintf("[%s] %s", DisplayName(), _("Rescanning…")), 0); // show rescan progress in GUI as dialog or on splashscreen, if rescan required on startup (e.g. due to corruption)
     uint256 tip_hash = WITH_LOCK(cs_wallet, return GetLastBlockHash());
     uint256 end_hash = tip_hash;
@@ -2006,10 +2005,10 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
         WITH_LOCK(cs_wallet, chain().requestMempoolTransactions(*this));
     }
     ShowProgress(strprintf("[%s] %s", DisplayName(), _("Rescanning…")), 100); // hide progress dialog in GUI
-    if (block_height && fAbortRescan) {
+    if (fAbortRescan) {
         WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", block_height, progress_current);
         result.status = ScanResult::USER_ABORT;
-    } else if (block_height && chain().shutdownRequested()) {
+    } else if (chain().shutdownRequested()) {
         WalletLogPrintf("Rescan interrupted by shutdown request at block %d. Progress=%f\n", block_height, progress_current);
         result.status = ScanResult::USER_ABORT;
     } else {

src/wallet/wallet.h

@@ -1103,6 +1103,10 @@ public:
         if (m_wallet.fScanningWallet.exchange(true)) {
             return false;
         }
+        // Discard any abort request left over from previous reservation, so
+        // that an abort requested while the reservation is held always applies
+        // to abort this rescan, even if it arrives before the scan loop starts.
+        m_wallet.fAbortRescan = false;
         m_wallet.m_scanning_with_passphrase.exchange(with_passphrase);
         m_wallet.m_scanning_start = SteadyClock::now();
         m_wallet.m_scanning_progress = 0;