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.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/35444
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/35444.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/35444#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	pablomartin4btc
Concept ACK	hebasto, polespinasa

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:

#36397 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36397.svg"> (scripted-diff: [wallet] Remove trailing newlines from one-line log messages by maflcko)
#36122 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36122.svg"> (BIP460: CISA for Taproot key path spends by fjahr)
#35989 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35989.svg"> (wallet: fix crash on importdescriptors with a range ending at 2^31-1 by shuv-amp)
#35302 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35302.svg"> (Silent Payments: Sending (take 2) by Eunovo)
#34969 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/34969.svg"> (fuzz: several improvements to scriptpubkeyman harness by brunoerg)
#30343 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/30343.svg"> (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/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

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

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

w0xlt force-pushed on Sep 21, 2026

DrahtBot removed the label Needs rebase on Sep 21, 2026

in src/wallet/scriptpubkeyman.cpp:1040 in 2f2cc56578

1035 | @@ -1033,9 +1036,9 @@ bool DescriptorScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, Walle
1036 |  util::Result<CTxDestination> DescriptorScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, int64_t& index)
1037 |  {
1038 |      LOCK(cs_desc_man);
1039 | -    auto op_dest = GetNewDestination(type);
1040 | -    index = m_wallet_descriptor.GetNext() - 1;
1041 | -    return op_dest;
1042 | +    auto dest{GetNewDestination_(type)};
1043 | +    if (dest) index = m_wallet_descriptor.GetNext() - 1;

polespinasa commented at 8:38 AM on October 6, 2026:

in 2f2cc56578d867384475eaa52bd1272b5c3e84af wallet: split descriptor SPKM locked internals

The commit message claims there is no behavior change in this commit, but this is indeed a behavior change as now it only writes the index on success, before it was writing it in success and in case of failure.

w0xlt commented at 7:51 PM on October 6, 2026:

Done. Thanks.

in src/wallet/external_signer_scriptpubkeyman.cpp:48 in dadc205c80 outdated

  44 | -    if (!batch.WriteDescriptor(spkm->GetID(), spkm->m_wallet_descriptor)) {
  45 | -        throw std::runtime_error(std::string(__func__) + ": writing descriptor failed");
  46 | -    }
  47 | -
  48 | -    // TopUp
  49 | -    spkm->TopUpWithDB(batch);

polespinasa commented at 8:42 AM on October 6, 2026:

in dadc205c806f8bd6e95334035f9be119270f00e6 wallet: stop locking descriptor SPKM externally

After removing this line, no one uses TopUpWithDB anymore. Maybe could also remove the protected wrapper?

w0xlt commented at 7:51 PM on October 6, 2026:

Done. Thanks.

in src/wallet/scriptpubkeyman.cpp:1621 in 2f2cc56578 outdated

1663 | @@ -1618,7 +1664,7 @@ util::Result<void> DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescr
1664 |  {
1665 |      LOCK(cs_desc_man);
1666 |      std::string error;
1667 | -    if (!CanUpdateToWalletDescriptor(descriptor, error)) {

polespinasa commented at 8:44 AM on October 6, 2026:

in 2f2cc56578d867384475eaa52bd1272b5c3e84af wallet: split descriptor SPKM locked internals

After removing this, CanUpdateToWalletDescriptor is only used by a fuzz test, but not real code. Consider removing the function entirely.

w0xlt commented at 7:51 PM on October 6, 2026:

Done. Thanks.

polespinasa commented at 8:46 AM on October 6, 2026: member

concept ACK 3d5e0777ac1a779191562d0fe7ad2b00f2a83f46

w0xlt force-pushed on Oct 6, 2026

DrahtBot added the label CI failed on Oct 6, 2026

DrahtBot commented at 8:26 PM on October 6, 2026: contributor

🚧 At least one of the CI tasks failed. Task OpenBSD Cross: https://github.com/bitcoin/bitcoin/actions/runs/37521939038/job/112469290765 LLM reason (✨ experimental): CI failed during compilation: scriptpubkeyman.cpp references the undeclared identifier cs_desc_man.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

w0xlt force-pushed on Oct 6, 2026

DrahtBot removed the label CI failed on Oct 6, 2026

in src/wallet/scriptpubkeyman.cpp:1319 in 04b7fc10b4

1318 | +        } catch (...) {
1319 | +            exception = std::current_exception();
1320 | +        }
1321 | +        changed = before != CanGetAddresses_();
1322 | +    }
1323 | +    if (changed) NotifyCanGetAddressesChanged();

polespinasa commented at 10:11 AM on October 7, 2026:

in 04b7fc10b4d0b17abf6c169c1b478ece7568c081 wallet: make descriptor SPKM mutex non-recursive

Correct me if I am wrong, but SetupDescriptorGeneration, SetupDescriptor and UpdateWithSigningProvider run before the new constructed SPKM is returned so they don't have any signal subscribers yet. This makes calling NotifyCanGetAddressesChanged useless. Because of that all the change detection logic calling CanGetAddresses_() is also unecessary.

w0xlt commented at 12:33 AM on October 8, 2026:

Done. Thanks.

w0xlt force-pushed on Oct 7, 2026

DrahtBot added the label Needs rebase on Oct 8, 2026

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

Remove the public CanUpdateToWalletDescriptor wrapper, which is only used
by the fuzz harness. Have the harness call UpdateWalletDescriptor under
cs_wallet so updates exercise the same validation as production callers.

GetReservedDestination() now sets the output index only when destination
creation succeeds, leaving it unchanged on failure. Existing wallet callers
already ignore this output on failure, so their behavior is unchanged.

f509b2dae4

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.

Remove the now-unused protected TopUpWithDB wrapper. Internal callers
continue to use the lock-held TopUpWithDB_ helper.

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.

2aaa761789

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 outer operations on existing
SPKMs. 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 and rethrow captured exceptions
after notification.

Factory initialization takes the mutex without change tracking or
notifications, since newly constructed SPKMs have no signal subscribers.

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

b3f25031e5

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-

b38237fa22

w0xlt force-pushed on Oct 8, 2026

w0xlt commented at 11:25 PM on October 8, 2026: contributor

Rebased

DrahtBot removed the label Needs rebase on Oct 9, 2026

in src/wallet/test/scriptpubkeyman_tests.cpp:161 in b38237fa22

 156 | +
 157 | +    index = -1;
 158 | +    BOOST_CHECK(!spkm->GetReservedDestination(OutputType::BECH32, /*internal=*/false, index));
 159 | +    BOOST_CHECK_EQUAL(index, -1);
 160 | +    check_notifications({});
 161 | +}

pablomartin4btc commented at 1:49 AM on October 9, 2026:

One thing, non-blocking: in b3f25031e5 ("wallet: make descriptor SPKM mutex non-recursive"), in desc_spkm_availability_notifications, the sub-test exercising UpdateWalletDescriptor() throwing mid-top-up and still firing a notification before rethrowing (failed_descriptor.SetEnd(5) / HasReason("Could not top up scriptPubKeys") / check_notifications({true})) is gone — was dropping this coverage intentional?

pablomartin4btc commented at 1:49 AM on October 9, 2026: member

ACK b38237fa22fbcae8c8a763d7af37e85fe1996b4b

Thanks for taking the suggestions.

Verified @polespinasa's feedback is properly addressed and code rebased.

Left a non-blocking comment.

DrahtBot requested review from hebasto on Oct 9, 2026

DrahtBot requested review from polespinasa on Oct 9, 2026