#35444 wallet: make descriptor SPKM mutex non-recursive
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35444 wallet: make descriptor SPKM mutex non-recursive | Ready | 1 | Strong | 326 + 125 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Replaces DescriptorScriptPubKeyMan's public RecursiveMutex with a private non-recursive Mutex and renames it to m_desc_mutex. Public methods now encapsulate lock acquisition while shared internal logic is split into lock-held helper functions. Address-availability signals are deferred until after the mutex is released to prevent re-entrancy deadlocks during synchronous callbacks.
Problem: DescriptorScriptPubKeyMan used a public recursive mutex, allowing external callers to lock internal state directly and obscuring lock order hierarchies across the wallet. Recursive mutexes also prevent full Clang thread-safety annotations and create deadlock hazards when synchronous callbacks query the wallet while the lock is held.
Evidence of importance:
- Part of tracking issue #19303 to eliminate recursive mutexes in the codebase (PR description)
- Includes a unit test in scriptpubkeyman_tests.cpp covering synchronous callback re-entry during keypool top-ups and address generation (commit 1f29c2b31c)
Scope: Commit 2cde6f9534 is a scripted-diff renaming cs_desc_man to m_desc_mutex across the repository.
Risk of a mistake: medium (wallet). Mistakes in wallet mutex refactoring could introduce deadlocks or lock-order inversions that freeze wallet RPCs and UI callbacks.
Changed since the previous assessment: No structural or architectural changes; the commit progression and overall approach remain the same.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- nothing: no pending reviewer requests or objections
Agreement
Strong
substantive support, no open objection (pablomartin4btc, pablomartin4btc, pablomartin4btc)
Thread notes: Author replied to pablomartin4btc's question about the dropped test coverage and restored the test, after which pablomartin4btc re-ACKed.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| pablomartin4btc (member) | question, correctness | Whether atomicity across the two reads in listdescriptors was intended now that they are read under two separate locks | answered · author confirmed listdescriptors holds cs_wallet across both calls | answered | yes | 2026-06-10: '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 ' Status from: 2026-06-17: w0xlt: 'Yes, I also think it is safe because listdescriptors holds wallet->cs_wallet across both calls.' |
| pablomartin4btc (member) | suggestion, style | store the result of GetWalletDescriptor() in a local variable before calling GetPubKeys() to clarify that it operates on a copy | answered · author replied 'Done. Thanks.' | answered | yes | 2026-06-10: 'would it be clearer to first store the result of GetWalletDescriptor() in a local variable and then call `GetPubKeys()` on it?' Status from: 2026-06-17: w0xlt: 'Done. Thanks.' |
| polespinasa (member) | objection, correctness | Commit message claims no behavior change, but index is now only written on success instead of both success and failure | fixed · fix pushed · author rebased after acknowledging the issue | changed | yes | 2026-10-06: '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 o' Status from: 2026-10-08: w0xlt: 'Rebased' |
| polespinasa (member) | suggestion, maintenance | Remove protected wrapper TopUpWithDB as no one uses it anymore | answered · author replied 'Done. Thanks.' | answered | yes | 2026-10-06: 'After removing this line, no one uses TopUpWithDB anymore. Maybe could also remove the protected wrapper?' Status from: 2026-10-06: w0xlt: 'Done. Thanks.' |
| polespinasa (member) | suggestion, maintenance | Consider removing CanUpdateToWalletDescriptor entirely since it is only used by fuzz testing | answered · author replied 'Done. Thanks.' | answered | yes | 2026-10-06: 'After removing this, CanUpdateToWalletDescriptor is only used by a fuzz test, but not real code. Consider removing the function entirely.' Status from: 2026-10-06: w0xlt: 'Done. Thanks.' |
| polespinasa (member) | suggestion, maintenance | Remove unnecessary CanGetAddresses_() change detection and NotifyCanGetAddressesChanged calls since there are no signal subscribers yet during SPKM construction/setup | answered · author replied 'Done. Thanks.' | answered | yes | 2026-10-07: '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 y' Status from: 2026-10-08: w0xlt: 'Done. Thanks.' |
| pablomartin4btc (member) | question, maintenance | Whether dropping test coverage for UpdateWalletDescriptor throwing mid-top-up in desc_spkm_availability_notifications was intentional | accepted · fix pushed · author confirmed it was unintentional and restored test coverage; reviewer accepted with a re-ACK | answered | yes | 2026-10-09: 'was dropping this coverage intentional?' Status from: 2026-10-30: pablomartin4btc: 'plus the restored `UpdateWalletDescriptor()` failing-top-up sub-test in `desc_spkm_availability_notifications` (thanks for addressing https://github.com/bitcoin/bitcoin/pull/35444#discussion_r4225874270).' |
Support:
- pablomartin4btc (member) (Concept ACK, 2026-06-10) [read]: split between public self-locking methods and lock-held helpers makes sense, and external lock removals seem correct
- hebasto (member) (Concept ACK, 2026-06-10): Concept ACK without further rationale [not substantive]
- polespinasa (member) (Concept ACK, 2026-10-06): concept ACK 3d5e0777ac1a779191562d0fe7ad2b00f2a83f46 [not substantive]
- pablomartin4btc (member) (ACK, 2026-10-09) [read]: verified polespinasa's feedback is properly addressed and code rebased
- pablomartin4btc (member) (ACK, 2026-10-30) [read: desc_spkm_availability_notifications]: verified range-diff shows only trailing-newline log fixes after rebase, and verified the restored UpdateWalletDescriptor failing-top-up sub-test in desc_spkm_availability_notifications
Participants:
- hebasto (member, support, 1 statements 2026-06-10 to 2026-06-10): Concept ACK
- pablomartin4btc (member, support, 7 statements 2026-06-10 to 2026-10-30): ACK; asked about atomicity of listdescriptors reads, suggested a readability nit, and asked about dropped test coverage
- sedited (member, neutral, 1 statements 2026-08-27 to 2026-08-27): rebase ping
- polespinasa (member, objection, 5 statements 2026-10-06 to 2026-10-07): Concept ACK, but objected to index writing behavior change and suggested cleanups of unused methods and change detection
Files
133 lines under test/bench/ci.
- src/wallet/scriptpubkeyman.cpp +258/-146
- src/wallet/test/scriptpubkeyman_tests.cpp +121/-0
- src/wallet/scriptpubkeyman.h +64/-53
- src/wallet/external_signer_scriptpubkeyman.cpp +2/-9
- src/wallet/test/fuzz/scriptpubkeyman.cpp +2/-6
- src/wallet/wallet.cpp +2/-6
- src/wallet/test/wallet_tests.cpp +2/-2
- src/wallet/export.cpp +0/-1
- src/wallet/rpc/wallet.cpp +0/-1
Card
The short text the ranking pass compares across PRs.
Replaces DescriptorScriptPubKeyMan's public RecursiveMutex with a private non-recursive Mutex as part of #19303. External callers across the wallet and RPC code no longer take the lock directly, internal paths are split into lock-held helpers, and address-availability notifications are deferred until after unlocking to avoid re-entrant deadlocks with synchronous callbacks. A regression test is added to exercise callback re-entry and lock release during keypool operations. No changes to RPC output, wallet storage, or keypool semantics are intended.
Data
thread read through 2026-10-30 (24 statements), head 2cde6f9534
labels: Wallet
Categories
Wallet
P3 · cleanup
Replaces DescriptorScriptPubKeyMan's public recursive mutex with an encapsulated non-recursive mutex as part of tracking issue #19303. This clarifies lock hierarchy, enables Clang thread-safety annotations, and mitigates potential re-entrancy deadlocks during synchronous callbacks, but is primarily internal concurrency maintenance without an active user-facing failure.
Membership: Matches Wallet label and modifies wallet descriptor lock handling across src/wallet/
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1