wallet: reserve walletrescan before checking wallet is at the tip #35579

polespinasa commented at 10:45 AM on June 22, 2026: member

ImportDescriptors rpc has a race condition where two imports running in parallel can both succeed or fail one of them.

The race happens when there are two threads A and B trying to importdescriptors at the same time.

Thread A calls BlockUntilSyncedToCurrentChain() (holding cs_wallet fast, no contention) and then reserve(), acquiring the WalletRescanReserver. It proceeds to ProcessDescriptorImport(), which holds cs_wallet for an extended time (specially on slow machines) while importing descriptors.
B reaches BlockUntilSyncedToCurrentChain(), which internally does WITH_LOCK(cs_wallet, ...). Since A holds cs_wallet, B blocks here for the entire duration of Thread A's descriptors import.
Then A finishes importing, releases cs_wallet, rescans (fast in regtest), and sets fScanningWallet = false.
B can now continue in BlockUntilSyncedToCurrentChain() acquiring cs_wallet, and then calls reserve() which succeeds because fScanningWallet is already false. Both imports succeed.

I don't think the behavior is problematic at all from a usability PoV, but it can be a bad UX if some imports fails and other's no. It also makes testing difficult as race conditions are not easy to test.

This PR fixes it by calling reserver.reserve() before cs_wallet is locked, so multiple threads will be aware of currently imports before being stuck at any point. So only one importdescriptor call can be done at the same time.

I think this should fix #35544 (comment)

wallet: reserve walletrescan before checking wallet is at the tip 336f5a738b

DrahtBot added the label Wallet on Jun 22, 2026

DrahtBot commented at 10:45 AM on June 22, 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/35579.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	nebula-21
Stale ACK	w0xlt

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:

#35377 (wallet: Allow importing of descriptors without private keys when the wallet has the private keys by achow101)
#34861 (wallet: Add importdescriptors interface by polespinasa)
#33392 (wallet, rpc: add UTXO set check and incremental rescan to importdescriptors by musaHaruna)

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.

polespinasa commented at 10:49 AM on June 22, 2026: member

Friendly ping @w0xlt, would like to know what you think :) @pinheadmz I was unable to actually discover the cause of #35544 (comment), it's really difficult to detect race-conditions like this :/ . But I think it could be fixed by this PR. Were you able to trigger the failing test multiple times? If so, can you try to trigger it with this path to see if it works?? :)

achow101 commented at 8:59 PM on June 22, 2026: member

While this change seems fine and correct, looking at the failed test, I think the test itself is also incorrect. The test is supposed to have the 2 calls conflict during the rescan phase, but the rescan is not actually that slow because its only scanning a few hundred blocks with few relevant things in those blocks, so the rescan goes pretty fast. The way to slow down a rescan is to mine a ton of blocks, each of which have something relevant to the wallet.

w0xlt commented at 6:15 AM on June 23, 2026: contributor

ACK 336f5a738b3d89abb8dd7ebdc4b31ac12a6de85f

Regarding the test, it really could be improved.

The patch below mines 5000 blocks paying to an address derived from the descriptor being imported, so the rescan has relevant wallet work.

It also removes the two-worker race between competing imports. The test still uses one background thread to keep importdescriptors in flight, but it waits until getwalletinfo()["scanning"] confirms an active rescan before sending the conflicting RPC, making it less timing-dependent.

Measurements on my machine:

Before:

Full test: 10.96s
rescan_wallet rescan: 2ms

After:

Full test: 17.37s
rescan_wallet primary rescan: 457ms

diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py
index f9429079bb..22f35b1f36 100755
--- a/test/functional/wallet_importdescriptors.py
+++ b/test/functional/wallet_importdescriptors.py
@@ -16,7 +16,6 @@ variants.
   and test the values returned."""
 
 import concurrent.futures
-import threading
 import time
 
 from test_framework.blocktools import COINBASE_MATURITY
@@ -128,41 +127,31 @@ class ImportDescriptorsTest(BitcoinTestFramework):
 
         w_import = self.nodes[0].create_new_rpc_connection(mode="AUTHPROXY") / f"wallet/{wallet_name}"
 
-        with concurrent.futures.ThreadPoolExecutor(max_workers=2) as thread:
+        with concurrent.futures.ThreadPoolExecutor(max_workers=1) as thread:
             w_rescan = self.nodes[0].create_new_rpc_connection(mode="AUTHPROXY") / f"wallet/{wallet_name}"
-            w_conflict = self.nodes[0].create_new_rpc_connection(mode="AUTHPROXY") / f"wallet/{wallet_name}"
-            # Use an xprv with timestamp=0 and a large key-range to trigger a slow full rescan that stays in-flight
-            slow_desc = [{"desc": descsum_create("pkh(" + xpriv + "/0h/*h)"),
-            "timestamp": 0, "range": [0, 10000]}]
-            conflicting_desc = [{"desc": descsum_create("pkh(" + xpriv + "/1h/*h)"),
-            "timestamp": 0, "range": [0, 10000]}]
-
-            start = threading.Barrier(3)
-
-            def import_after_barrier(wallet, descriptors):
-                start.wait(timeout=10)
-                return wallet.importdescriptors(descriptors)
-
-            imports = [
-                thread.submit(import_after_barrier, w_rescan, slow_desc),
-                thread.submit(import_after_barrier, w_conflict, conflicting_desc),
-            ]
-            start.wait(timeout=10)
-
-            # One importdescriptor call must hold WalletRescanReserver while the other fails immediately.
-            num_errors = 0
-            num_success = 0
-            for future in concurrent.futures.as_completed(imports, timeout=30 * self.options.timeout_factor):
-                try:
-                    assert_equal(future.result(), [{'success': True}])
-                    num_success += 1
-                except JSONRPCException as e:
-                    assert_equal(e.error["code"], -4)
-                    assert_equal(e.error["message"], "Wallet is currently rescanning. Abort existing rescan or wait.")
-                    num_errors += 1
-
-            assert_equal(num_success, 1)
-            assert_equal(num_errors, 1)
+            slow_desc_str = descsum_create("pkh(" + xpriv + "/0h/*h)")
+            relevant_address = self.nodes[0].deriveaddresses(slow_desc_str, [0, 0])[0]
+
+            num_relevant_blocks = 5000
+            # Mine blocks that are relevant to the descriptor being imported, so
+            # the rescan stays in-flight long enough to test the RPC conflict.
+            self.generatetoaddress(self.nodes[0], num_relevant_blocks, relevant_address)
+
+            slow_desc = [{"desc": slow_desc_str, "timestamp": 0, "range": [0, 10000]}]
+            rescanning = thread.submit(w_rescan.importdescriptors, slow_desc)
+
+            def rescan_in_progress():
+                if rescanning.done():
+                    raise AssertionError("importdescriptors rescan finished before the conflicting RPC could be tested")
+                scanning = w_import.getwalletinfo()["scanning"]
+                return isinstance(scanning, dict) and 0 < scanning["progress"] < 0.95
+
+            self.wait_until(rescan_in_progress, timeout=30 * self.options.timeout_factor)
+
+            assert_raises_rpc_error(-4, "Wallet is currently rescanning. Abort existing rescan or wait.",
+                w_import.importdescriptors,
+                [{"desc": other_desc, "timestamp": "now"}])
+            assert_equal(rescanning.result(timeout=30 * self.options.timeout_factor), [{'success': True}])
 
         # After the rescan finishes, any importdescriptors should succeed.
         result = w_import.importdescriptors([{"desc": other_desc, "timestamp": "now"}])
@@ -180,8 +169,8 @@ class ImportDescriptorsTest(BitcoinTestFramework):
 
             importing = thread.submit(w_import.importdescriptors, descriptor)
 
-            # Keep trying because an abort before ScanForWalletTransactions starts
-            # is reset when the scan loop begins.
+            # Keep trying until the abort lands while importdescriptors holds the
+            # wallet rescan reservation.
             abort_succeeded = False
             abort_deadline = time.time() + 30 * self.options.timeout_factor
             while not importing.done() and time.time() < abort_deadline:

</details>

polespinasa commented at 8:02 AM on June 23, 2026: member

It also removes the two-worker race between competing imports. The test still uses one background thread to keep importdescriptors in flight, but it waits until getwalletinfo()["scanning"] confirms an active rescan before sending the conflicting RPC, making it less timing-dependent.

Are we sure about this? IIRC we were already doing this for a previous version of the test and it caused some failures. I think keeping the race is ok, I tried to slow down both imports, so it should be fine?¿

in test/functional/wallet_importdescriptors.py:139 in b27a0bc6ca

 135 | @@ -136,6 +136,9 @@ def test_rescan_fails_import(self):
 136 |              "timestamp": 0, "range": [0, 10000]}]
 137 |              conflicting_desc = [{"desc": descsum_create("pkh(" + xpriv + "/1h/*h)"),
 138 |              "timestamp": 0, "range": [0, 10000]}]
 139 | +            num_relevant_blocks = 5000

achow101 commented at 9:16 PM on June 23, 2026:

In b27a0bc6ca7fb0f78f3b67b3cb07bf4668166080 "test: slow down rescaning process"

5000 is a bit much, generatetoaddress times out on my machine. I think 1000 is probably sufficient.

polespinasa commented at 6:18 AM on June 25, 2026:

done

test: slow down rescaning process 9e62e4b1f3

polespinasa force-pushed on Jun 25, 2026

polespinasa requested review from w0xlt on Jun 29, 2026

polespinasa requested review from achow101 on Jun 29, 2026

in src/wallet/rpc/backup.cpp:386 in 9e62e4b1f3

 379 | @@ -380,15 +380,15 @@ RPCMethod importdescriptors()
 380 |      if (!pwallet) return UniValue::VNULL;
 381 |      CWallet& wallet{*pwallet};
 382 |  
 383 | -    // Make sure the results are valid at least up to the most recent block
 384 | -    // the user could have gotten from another RPC command prior to now
 385 | -    wallet.BlockUntilSyncedToCurrentChain();
 386 | -

nebula-21 commented at 7:59 PM on June 29, 2026:

Maybe worth adding some comment explaining the ordering to avoid introducing this race condition again in the future.

Something like:

// Reserve the rescan slot before any operation that can block on cs_wallet,
// so concurrent imports are rejected immediately rather than causing a race condition.
WalletRescanReserver reserver(*pwallet);

nebula-21 commented at 10:31 PM on June 29, 2026: none

ACK 9e62e4b1f346f70eaa23e83eff769b6e6cc04630

Left a suggestion on the code.