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.

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 and AI policy 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:

#36230 (wallet: Improve HasWalletDescriptor performance and other canonical descriptor string followups by achow101)
#34969 (fuzz: several improvements to scriptpubkeyman harness by brunoerg)
#34520 (refactor: Add [[nodiscard]] to functions returning bool+mutable ref by maflcko)

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 comparison-specific test macros should replace generic comparisons:

[src/wallet/test/scriptpubkeyman_tests.cpp] BOOST_CHECK_THROW((void)spkm->GetNewDestination(OutputType::LEGACY), std::runtime_error); -> use BOOST_CHECK_EXCEPTION(..., std::runtime_error, HasReason("...")) so the test checks the specific failure message, not just the exception type.

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

 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 outdated

 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.

w0xlt commented at 10:39 PM on June 17, 2026:

Yes, I also think it is safe because listdescriptors holds wallet->cs_wallet across both calls. Any update that could change either the descriptor metadata or descriptor string also needs cs_wallet, so it cannot run between those two reads.

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

w0xlt commented at 11:28 PM on June 17, 2026:

Done. Thanks.

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.

DrahtBot added the label Needs rebase on Jun 14, 2026

w0xlt force-pushed on Jun 17, 2026

DrahtBot removed the label Needs rebase on Jun 18, 2026

w0xlt commented at 4:26 AM on June 18, 2026: contributor

Rebased.

w0xlt force-pushed on Jun 20, 2026

DrahtBot added the label Needs rebase on Jul 3, 2026

sedited commented at 11:37 AM on August 27, 2026: contributor

Ping for rebase @w0xlt

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.

e12579abc7

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.

1c32c431c0

w0xlt force-pushed on Sep 15, 2026

DrahtBot removed the label Needs rebase on Sep 15, 2026

w0xlt force-pushed on Sep 15, 2026

DrahtBot added the label CI failed on Sep 15, 2026

wallet: make descriptor SPKM mutex non-recursive

Make cs_desc_man private and replace RecursiveMutex with Mutex. Annotate
self-locking entry points with negative lock requirements and retain
LOCKS_EXCLUDED on factory-only setup methods.

Move address-availability notifications to the outer operations. Compare
availability before and after each operation under the mutex, then notify
after unlocking so synchronous callbacks can query the wallet safely.
Notify only for a net change, including partial updates that throw, and
rethrow captured exceptions after notification.

Add a watch-only hardened-descriptor regression test covering synchronous
callback re-entry, address consumption and return, range extension,
unchanged availability, and partial failures.

eebdf62e41

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-

b6d586638a

w0xlt force-pushed on Sep 15, 2026

w0xlt commented at 10:55 PM on September 15, 2026: contributor

Rebased.

DrahtBot removed the label CI failed on Sep 15, 2026

DrahtBot added the label Needs rebase on Sep 19, 2026

DrahtBot commented at 5:08 PM on September 19, 2026: contributor

🐙 This pull request conflicts with the target branch and needs rebase.