highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-07-16 01:28:39 +02:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
9e62e4b1f3 test: slow down rescaning process (Pol Espinasa)
336f5a738b wallet: reserve walletrescan before checking wallet is at the tip (Pol Espinasa)

Pull request description:

  `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.

  1. 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.

  2. 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.

  3. Then A finishes importing, releases `cs_wallet`, rescans (fast in regtest), and sets `fScanningWallet = false`.

  4. 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 https://github.com/bitcoin/bitcoin/issues/35544#issuecomment-4763488259

ACKs for top commit:
  achow101:
    ACK 9e62e4b1f3
  nebula-21:
    ACK 9e62e4b1f3
  w0xlt:
    lgtm reACK 9e62e4b1f3

Tree-SHA512: be0027e1a7b77252ed9fb514c3b3311d6905903d4b0bfc1021a1e1c2bb06872ef599647ff8a4536929240717ae9db62fb75374f629cd3046c7192e2b8b4d7344
This commit is contained in:
merge-script
2026-07-14 09:45:16 +02:00
parent 11ae426552 9e62e4b1f3
commit e3554bf361
2 changed files with 7 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
src/wallet/rpc/backup.cpp
View File

@@ -381,15 +381,15 @@ RPCMethod importdescriptors()
if (!pwallet) return UniValue::VNULL;
CWallet& wallet{*pwallet};
// Make sure the results are valid at least up to the most recent block
// the user could have gotten from another RPC command prior to now
wallet.BlockUntilSyncedToCurrentChain();
WalletRescanReserver reserver(*pwallet);
if (!reserver.reserve(/*with_passphrase=*/true)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
}
// Make sure the results are valid at least up to the most recent block
// the user could have gotten from another RPC command prior to now
wallet.BlockUntilSyncedToCurrentChain();
// Ensure that the wallet is not locked for the remainder of this RPC, as
// the passphrase is used to top up the keypool.
LOCK(pwallet->m_relock_mutex);

3
test/functional/wallet_importdescriptors.py
View File

@@ -140,6 +140,9 @@ class ImportDescriptorsTest(BitcoinTestFramework):
"timestamp": 0, "range": [0, 10000]}]
conflicting_desc = [{"desc": descsum_create("pkh(" + xpriv + "/1h/*h)"),
"timestamp": 0, "range": [0, 10000]}]
num_relevant_blocks = 1000
self.generatetoaddress(self.nodes[0], num_relevant_blocks, self.nodes[0].deriveaddresses(slow_desc[0]['desc'], [0, 0])[0])
self.generatetoaddress(self.nodes[0], num_relevant_blocks, self.nodes[0].deriveaddresses(conflicting_desc[0]['desc'], [0, 0])[0])
start = threading.Barrier(3)