wallet: make descriptor SPKM mutex non-recursive #35444

w0xlt commented at 4:33 PM on June 2, 2026: contributor

Part of #19303

This PR makes DescriptorScriptPubKeyMan use a non-recursive private mutex by moving locking responsibility into its public methods and using lock-held internal helpers for shared logic.

The change removes external locking of DescriptorScriptPubKeyMan::cs_desc_man, splits recursive internal call paths such as keypool top-up and descriptor updates, and then renames the mutex to m_desc_mutex.

No wallet database format, RPC behavior, keypool semantics, or external signer behavior is intended to change.

wallet: split descriptor SPKM locked internals

Prepare DescriptorScriptPubKeyMan for making cs_desc_man non-recursive by
separating public lock-taking entry points from helpers that require
cs_desc_man to already be held.

This removes recursive calls from the core descriptor paths without changing
caller behavior yet. The public mutex remains recursive and externally
accessible in this commit, so existing call sites continue to build while
locked helpers centralize the shared logic used by keypool top-up, descriptor
updates, address generation, and descriptor matching.

2ee00d83bb

wallet: stop locking descriptor SPKM externally

Move the remaining descriptor SPKM callers to self-locking public methods
instead of taking cs_desc_man directly. This removes direct locks from wallet,
RPC, and fuzz code and lets DescriptorScriptPubKeyMan own synchronization for
its descriptor state.

External signer wallet creation also stops mutating m_wallet_descriptor under
a direct external lock. It now calls SetupDescriptor, which performs the same
descriptor write and top-up sequence behind the SPKM API.

The mutex remains recursive and public in this commit, so this is a call-site
cleanup that sets up the final non-recursive conversion without changing
wallet behavior.

aebc7a8861

wallet: make descriptor SPKM mutex non-recursive

With recursive internal paths removed and external callers no longer taking
cs_desc_man, replace DescriptorScriptPubKeyMan::cs_desc_man with a
non-recursive Mutex and make it private.

Annotate self-locking methods so clang rejects calls made while cs_desc_man is
already held. Factory-only setup methods keep LOCKS_EXCLUDED declarations
because they are called on newly-created SPKMs, where clang cannot prove the
negative lock state at the call site.

Lock-held helpers remain annotated as requiring cs_desc_man.

This completes the DescriptorScriptPubKeyMan::cs_desc_man change to Mutex
without changing wallet database format, RPC behavior, external signer
behavior, or lock/unlock semantics.

015201b176

scripted-diff: wallet: rename cs_desc_man to m_desc_mutex

Now that DescriptorScriptPubKeyMan::cs_desc_man is a non-recursive, private
Mutex, rename it to m_desc_mutex to match the member-mutex naming convention
adopted by the other RecursiveMutex->Mutex conversions. No behavior change.

-BEGIN VERIFY SCRIPT-
sed -i 's/cs_desc_man/m_desc_mutex/g' $(git grep -l cs_desc_man -- '*.cpp' '*.h')
-END VERIFY SCRIPT-

6f3f81c063

DrahtBot added the label Wallet on Jun 2, 2026

DrahtBot commented at 4:33 PM on June 2, 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/35444.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	pablomartin4btc, hebasto

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:

#bitcoin-core/gui/872 (Menu action to export a watchonly wallet by achow101)
#35445 (wallet, descriptor: Revert StringType::COMPAT for Miniscript expressions and drop the concept of a Descriptor ID that can be validated by achow101)
#35436 (wallet: Add addHDkey interface by pseudoramdom)
#35302 (Silent Payments: Sending (take 2) by Eunovo)
#35201 (Refactor: Updated signMessage to use util::Expected and removed SigningResult::OK by kevkevinpal)
#34993 (wallet: NotifyCanGetAddressesChanged when advancing next_index by davidgumberg)
#34969 (fuzz: several improvements to scriptpubkeyman harness by brunoerg)
#34520 (refactor: Add [[nodiscard]] to functions returning bool+mutable ref by maflcko)
#32489 (wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)
#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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

WalletDescriptor(std::move(desc), creation_time, 0, 0, 0) in src/wallet/external_signer_scriptpubkeyman.cpp

in src/wallet/scriptpubkeyman.cpp:972 in 6f3f81c063

 972 | -    auto op_dest = GetNewDestination(type);
 973 | -    index = m_wallet_descriptor.next_index - 1;
 974 | -    return op_dest;
 975 | +    LOCK(m_desc_mutex);
 976 | +    auto dest{GetNewDestination_(type)};
 977 | +    if (dest) index = m_wallet_descriptor.next_index - 1;

pablomartin4btc commented at 3:42 AM on June 10, 2026:

Nice cleanup: this avoids updating index after a failed destination request.

in src/wallet/rpc/backup.cpp:547 in 6f3f81c063

 543 | @@ -544,7 +544,6 @@ RPCMethod listdescriptors()
 544 |          if (!desc_spk_man) {
 545 |              throw JSONRPCError(RPC_WALLET_ERROR, "Unexpected ScriptPubKey manager type.");
 546 |          }
 547 | -        LOCK(desc_spk_man->cs_desc_man);

pablomartin4btc commented at 3:52 AM on June 10, 2026:

I think removing the external lock here is fine now that GetWalletDescriptor() and GetDescriptorString() are self-locking. One subtle difference is that the descriptor metadata and descriptor string are now read under two separate locks instead of one caller-held lock. I don’t see an issue because descriptor state should not normally change concurrently here, but I wanted to double-check that no atomicity across these two reads was intended.

in src/wallet/rpc/wallet.cpp:909 in 6f3f81c063

 903 | @@ -905,7 +904,6 @@ RPCMethod addhdkey()
 904 |  
 905 |              UniValue response(UniValue::VOBJ);
 906 |              const DescriptorScriptPubKeyMan& desc_spkm = spkm->get();
 907 | -            LOCK(desc_spkm.cs_desc_man);
 908 |              std::set<CPubKey> pubkeys;
 909 |              std::set<CExtPubKey> extpubs;
 910 |              desc_spkm.GetWalletDescriptor().descriptor->GetPubKeys(pubkeys, extpubs);

pablomartin4btc commented at 4:09 AM on June 10, 2026:

Very minor readability nit: would it be clearer to first store the result of GetWalletDescriptor() in a local variable and then call GetPubKeys() on it? That would make it more obvious that the operation is performed on a copied descriptor returned by the accessor, rather than directly on internal SPKM state.

            auto wallet_descriptor = desc_spkm.GetWalletDescriptor(); // locks internally, returns copy
            wallet_descriptor.descriptor->GetPubKeys(pubkeys, extpubs); // operates on the copy

pablomartin4btc commented at 4:16 AM on June 10, 2026: member

Concept ACK.

I reviewed the commits individually. The split between public self-locking methods and lock-held helpers makes sense to me, and the external lock removals seem correct now that descriptor accessors return/ use state under their own lock. I left one question around listdescriptors, where two descriptor reads are now done through separate self-locking calls instead of under one caller-held lock, and a minor readability suggestion regarding the cs_desc_man lock removal in addhdkey().

hebasto commented at 7:13 AM on June 10, 2026: member

Concept ACK.