wallet: move fAbortRescan reset into WalletRescanReserver reserve function #35512

polespinasa commented at 11:00 AM on June 11, 2026: member

Follow-up of #35179 For extra context refer to the conversations #35179 (review) 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.

wallet: move fAbortRescan reset into WalletRescanReserver reserve()

Reserving the wallet rescan is the first thing done when starting a rescan.
As part of the reservation, clear any leftover state from previous
rescans (reset fAbortRescan). This prevents a race condition
where an abort request arrives before the rescan loop starts; without
this reset, the abort could be ignored and the rescan would proceed.

Co-authored-by: w0xlt <woltx@protonmail.com>

bc30e95163

test: add abortscan unit test

Co-authored-by: w0xlt <woltx@protonmail.com>

2818a171c0

DrahtBot added the label Wallet on Jun 11, 2026

DrahtBot commented at 11:00 AM on June 11, 2026: 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/35512.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	w0xlt, nebula-21, pablomartin4btc, pinheadmz, achow101

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34681 (wallet: move rescan logic into ChainScanner and wallet/scan by Eunovo)
#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)

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.

in src/wallet/wallet.cpp:2009 in bc30e95163

2004 | @@ -2006,10 +2005,10 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
2005 |          WITH_LOCK(cs_wallet, chain().requestMempoolTransactions(*this));
2006 |      }
2007 |      ShowProgress(strprintf("[%s] %s", DisplayName(), _("Rescanning…")), 100); // hide progress dialog in GUI
2008 | -    if (block_height && fAbortRescan) {

polespinasa commented at 11:02 AM on June 11, 2026:

Note for reviewers: removing the block_height check here and below is necessary because if the scan is aborted before started the block_height might be 0, making the function return the wrong value.

w0xlt commented at 6:25 PM on June 11, 2026: contributor

ACK 2818a171c0057d1775d4148b7d76b1fbbbdbb88a

nebula-21 commented at 11:32 PM on June 12, 2026: none

ACK 2818a171c0057d1775d4148b7d76b1fbbbdbb88a

I wrote a small Python functional test that forces the race condition. The test fails on master and succeeds with this PR.

diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py
--- a/test/functional/wallet_importdescriptors.py
+++ b/test/functional/wallet_importdescriptors.py
@@ -789,6 +789,31 @@ class ImportDescriptorsTest(BitcoinTestFramework):
 
         assert_equal(temp_wallet.getbalance(), encrypted_wallet.getbalance())
 
+
+        self.log.info("abort test")
+        self.nodes[0].createwallet(wallet_name="abort", blank=True)
+        w0_import = self.nodes[0].create_new_rpc_connection(mode="AUTHPROXY") / "wallet/abort"
+        w0_abort = self.nodes[0].create_new_rpc_connection(mode="AUTHPROXY") / "wallet/abort"
+
+        xprv = "tprv8ZgxMBicQKsPeuVhWwi6wuMQGfPKi9Li5GtX35jVNknACgqe3CY4g5xgkfDDJcmtF7o1QnxWDRYw4H5P26PXq7sbcUkEqeR4fg3Kxp2tigg"
+        descriptor = [{
+            "desc": descsum_create(f"pkh({xprv}/0h/*h)"),
+            "timestamp": 0,
+            "range": [0, 4000],
+        }]
+
+        with concurrent.futures.ThreadPoolExecutor(max_workers=1) as executor:
+            future = executor.submit(w0_import.importdescriptors, descriptor)
+            time.sleep(0.1)
+            assert_equal(w0_abort.abortrescan(), True)
+
+            try:
+                future.result(timeout=30)
+                raise AssertionError("importdescriptors should have been aborted")
+            except JSONRPCException as exc:
+                assert_equal(exc.error["message"], "Rescan aborted by user.")
+
+
         self.log.info("Multipath descriptors")
         self.nodes[1].createwallet(wallet_name="multipath", blank=True)
         w_multipath = self.nodes[1].get_wallet_rpc("multipath")

</details>

pablomartin4btc commented at 11:08 PM on June 13, 2026: member

ACK 2818a17

Tested it using the functional test from @nebula-21 above, it fails on master without the fix, please consider to add it here.

polespinasa commented at 5:46 AM on June 14, 2026: member

please consider to add it here

The test already exist, is the first approach of #35179 (last commit).

Now it is just slightly different to not fail (or fail less). As it is not testing the race condition, but is effected by it. I guess I could simplify it again in this PR.

hebasto commented at 6:59 AM on June 14, 2026: member

2818a171c0057d1775d4148b7d76b1fbbbdbb88a

Tested in https://github.com/hebasto/bitcoin-core-nightly for the FreeBSD and OpenBSD builds that started failing after #35179. wallet_importdescriptors.py passes every run now.

in src/wallet/wallet.h:1108 in bc30e95163

1102 | @@ -1103,6 +1103,10 @@ class WalletRescanReserver
1103 |          if (m_wallet.fScanningWallet.exchange(true)) {
1104 |              return false;
1105 |          }
1106 | +        // Discard any abort request left over from previous reservation, so
1107 | +        // that an abort requested while the reservation is held always applies
1108 | +        // to abort this rescan, even if it arrives before the scan loop starts.

pinheadmz commented at 2:58 PM on June 18, 2026:

bc30e9516324d82bd8540a82544085553a58aabc

nit, only if you retouch. I was confused by this comment at first, maybe

  // Discard any stale abort request by resetting to false.
  // Stale abort could be either left over from a previous scan
  // that was aborted, or set spuriously when no scan was running.
  // Ensure an abort arriving while this reservation is held can cancel this scan.

pinheadmz approved

pinheadmz commented at 6:21 PM on June 18, 2026: member

ACK 2818a171c0057d1775d4148b7d76b1fbbbdbb88a

This patch should close the intermittent test issue on master, although I don't think the branch itself is based on master so to fully test, I had to rebase locally ;-)

The issue is that importdescriptors has two phases: key generation, and then the rescan. The first phase can not be interrupted but the second phase can be. On master, the abort flag was reset to false at the beginning of the second phase. So if there were a lot of keys to generate, there is plenty of time for an abortrescan to be sent by the user (or the test) just to have it wiped out when the second phase begins.

I was able to guarantee the failure of wallet_importdescriptors.py locally by bumping the descriptor range by 10x and lowering the "keep trying to abort" deadline by 10x. With the patch from this PR applied, even those extreme conditions don't fail the test.

It's worth noting that if the "keep trying to abort" deadline is long enough to send an abort during the second phase -- the test WILL NOT FAIL on master. I dunno if it's worth trying to make that test more strict so regressions are obvious instead of intermittent ...?

The solution in the PR is to simply move the "abort flag reset to false" as early as possible at the beginning of the first phase.

<details><summary>Show Signature</summary>

pinheadmz's public key is on openpgp.org

</details>

fanquake requested review from achow101 on Jun 19, 2026

in src/wallet/test/wallet_tests.cpp:223 in 2818a171c0

 218 | +    CWallet wallet(m_node.chain.get(), "", CreateMockableWalletDatabase());
 219 | +    uint256 genesis_hash;
 220 | +    {
 221 | +        LOCK(wallet.cs_wallet);
 222 | +        LOCK(Assert(m_node.chainman)->GetMutex());
 223 | +        wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);

rkrux commented at 8:44 AM on June 19, 2026:

In 2818a171c0057d1775d4148b7d76b1fbbbdbb88a "test: add abortscan unit test"

Setting the descriptors flag doesn't seem necessary, the test works fine without it.

polespinasa commented at 2:39 PM on June 19, 2026:

It is set in all tests, will keep it for consistency. Also even if it is not strictly necessary this specifies that this is a descriptor wallet. Some functions may Assert the flag is set and we don't want the test to fail in the future if the test is modified. Or if some of the used functions assert that the wallet is not a legacy wallet.

in src/wallet/wallet.cpp:2010 in 2818a171c0

2004 | @@ -2006,10 +2005,10 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
2005 |          WITH_LOCK(cs_wallet, chain().requestMempoolTransactions(*this));
2006 |      }
2007 |      ShowProgress(strprintf("[%s] %s", DisplayName(), _("Rescanning…")), 100); // hide progress dialog in GUI
2008 | -    if (block_height && fAbortRescan) {
2009 | +    if (fAbortRescan) {
2010 |          WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", block_height, progress_current);
2011 |          result.status = ScanResult::USER_ABORT;

rkrux commented at 8:53 AM on June 19, 2026:

Shouldn't fAbortRescan be set to false here now after the result.status has been decided? It will avoid having stale aborts lying around.

polespinasa commented at 3:20 PM on June 19, 2026:

I think it should work "for now" because we should not be able to call abortrescan rpc if there's no rescan running. But if some function calls wallet::abortrescan() we would have the problem of never starting the rescan in a future rescan call.

You can easily test this with the test added in this PR, which will fail, and the change:

$ git diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 8d50204928..cd16d0b5b6 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2008,6 +2008,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
     if (fAbortRescan) {
         WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", block_height, progress_current);
         result.status = ScanResult::USER_ABORT;
+        fAbortRescan = false;
     } 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;
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 62fba7b4b7..db83269d44 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1106,7 +1106,6 @@ public:
         // 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;

I think the current approach of cleaning the value at the very beginning of a rescan is a cleaner approach and safer.

rkrux commented at 3:31 PM on June 19, 2026:

You can easily test this with the test added in this PR, which will fail, and the change: I think the current approach of cleaning the value at the very beginning of a rescan is a cleaner approach and safer.

This suggestion #35512 (review) doesn't mention to remove the current approach, the intent of the suggestion is to instead complement the current approach :)

polespinasa commented at 3:33 PM on June 19, 2026:

Oh I see, then it is redundant as it will be overwritten in the next reserve() call anyway

achow101 commented at 7:29 PM on June 19, 2026: member

ACK 2818a171c0057d1775d4148b7d76b1fbbbdbb88a

achow101 merged this on Jun 19, 2026

achow101 closed this on Jun 19, 2026