Skip to content

fix: coinjoin wallet manager lock order#7412

Closed
PastaPastaPasta wants to merge 1 commit into
dashpay:developfrom
PastaPastaPasta:codex/fix-coinjoin-lock-order
Closed

fix: coinjoin wallet manager lock order#7412
PastaPastaPasta wants to merge 1 commit into
dashpay:developfrom
PastaPastaPasta:codex/fix-coinjoin-lock-order

Conversation

@PastaPastaPasta

@PastaPastaPasta PastaPastaPasta commented Jul 7, 2026

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

Fixes a debug lock-order assertion seen in CI when CoinJoin wallet-manager maintenance and wallet keypool/client lookup paths observe opposite lock orders.

The problematic cycle was:

  • wallet/keypool path: CWallet::cs_wallet -> cs_wallet_manager_map
  • CoinJoin maintenance/teardown path: cs_wallet_manager_map -> CoinJoin manager/session code -> CWallet::cs_wallet

This can abort debug builds with a potential deadlock / inconsistent lock order assertion.

What was done?

  • Store CoinJoin client managers with std::shared_ptr ownership instead of std::unique_ptr.
  • Limit cs_wallet_manager_map to short-lived map/index operations: snapshot, lookup, insert, extract/erase, and destructor swap.
  • Run manager callbacks, maintenance, reset/stop, and teardown after releasing cs_wallet_manager_map.
  • Make CoinJoinClientImpl hold a shared_ptr so returned interface clients cannot dangle after wallet removal.
  • Add regression coverage for the newkeypool callback path that stops CoinJoin mixing while holding the wallet lock.

How Has This Been Tested?

Ran the checks available in this checkout:

  • git diff --check upstream/develop..HEAD
  • PYTHONPYCACHEPREFIX=/private/tmp/dash-pycache python3 -m py_compile test/functional/rpc_coinjoin.py
  • Static audit for LOCK(cs_wallet_manager_map) and WITH_LOCK(cs_wallet_manager_map) to confirm remaining map locks only protect map swap/snapshot/lookup/insert/extract.

Full unit/functional execution was not run locally because this checkout has no configured build tree or binaries (Makefile, src/test/test_dash, and src/dashd are absent).

Breaking Changes

None. This changes internal CoinJoin wallet-manager ownership/lifetime semantics only; there are no RPC, wallet-file, network, or user-facing API changes.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@PastaPastaPasta
PastaPastaPasta force-pushed the codex/fix-coinjoin-lock-order branch from 5d5128b to d1c1ddd Compare July 7, 2026 16:53
@PastaPastaPasta PastaPastaPasta changed the title fix coinjoin wallet manager lock order fix: coinjoin wallet manager lock order Jul 7, 2026
@knst

knst commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

I refactored this part significantly: https://github.com/dashpay/dash/pull/7259/files
Let me revive my PR instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants