Skip to content

fix: hold chainstate lock during CoinJoin input validation - #7820

Merged
PastaPastaPasta merged 1 commit into
dashpay:developfrom
PastaPastaPasta:codex/security-coinjoin-coins-lock
Oct 9, 2026
Merged

PastaPastaPasta merged 1 commit into
dashpay:developfrom
PastaPastaPasta:codex/security-coinjoin-coins-lock

Conversation

@PastaPastaPasta

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

CoinJoin input validation obtained a pointer to the shared chainstate coins cache under cs_main, then released that lock before reading the coins. Confirmed-coin lookups can populate the cache, so those reads could race block connection, disconnection or flushing. CoinJoin is opt-in for clients and requires an active mixing session on coordinators. The source lock-contract violation is established; no race, crash or state corruption has been reproduced.

What was done?

Hold cs_main for the complete IsValidInOuts() input-validation path, including every CCoinsViewMemPool::GetCoin() fallback to the shared cache. The RAII guard also covers early returns and releases before callers perform wallet or session cleanup. This follows the existing locking used by collateral validation.

How Has This Been Tested?

macOS arm64 with the prebuilt depends prefix, configured with --without-gui --disable-bench --disable-fuzz-binary:

  • make -j1 passed.
  • test_dash --run_test=coinjoin_inouts_tests,coinjoin_tests,coinjoin_dstxmanager_tests,coinjoin_queue_tests --log_level=message passed (82 cases).
  • test_runner.py -j1 --portseed=1 rpc_coinjoin.py passed with the assigned TEST_RUNNER_PORT_MIN=20000, along with the runner's framework unit tests.
  • Include, whitespace and circular-dependency linters and git diff --check passed.

Independent four-pass review checked the shared-cache access, caller lock ordering, mempool and InstantSend nesting, and error paths. No blocking issue was found. No timing-sensitive regression test was added; no TSAN, scheduled race or crash consequence is claimed. GUI and other build configurations were not validated locally.

Breaking Changes

None. No wire-format, wallet-format, fee-policy or consensus-rule change.

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

This pull request was created by Codex.

@thepastaclaw

thepastaclaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

✅ Final review complete — no blockers (commit a819870) · triage: low · Phase 2 only (queue backlog)

@coderabbitai

coderabbitai Bot commented Oct 7, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e7c9cf31-3fa5-4ddb-8ca5-55e42b250638
📥 Commits

Reviewing files that changed from the base of the PR and between f35a24c and a819870.

📒 Files selected for processing (1)
  • src/coinjoin/coinjoin.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.


Walkthrough

IsValidInOuts now acquires cs_main with LOCK before obtaining CoinsTip() and constructing CCoinsViewMemPool. The subsequent input and output validation is unchanged.

Priority: ➖ Normal

Estimated code review effort: 1 (Trivial) | ~3 minutes

Merge Risk: ⚪ Minimal · up to a8198

CoinJoin validation now keeps the chainstate lock while reading shared coin data, preventing the identified race. No concrete new lock-cycle or material availability risk was established; merge risk is minimal.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the change: holding the chainstate lock during CoinJoin input validation.
Description check ✅ Passed The description explains the locking issue, the change, and the reported tests. It is directly related to the pull request.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Phase 2 only (queue backlog)

Verified the complete one-commit diff at a819870 and the surrounding CoinJoin callers, coins-cache access, and nested locking; no in-scope defects were found. The RAII guard protects every shared-cache lookup through completion of input validation, releases on all return paths before caller cleanup, and matches collateral validation's existing pattern. This was static verification only: the supplied CI snapshot shows successful container builds and formatting checks, with two multi-architecture manifest jobs queued; the author-reported local build and tests were not independently rerun.

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: low by gpt-6.1-sol (effort low) — The small, contained change extends an existing lock across CoinJoin input validation without changing validation rules, making correctness straightforward to confirm by checking lock scope and ordering.
  • Phase 1 reviewers: not run (skipped for throughput: 18 PRs queued, above the 10 limit)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer

@thepastaclaw thepastaclaw added the pastaclaw:approved thepastaclaw's latest review approved this PR label Oct 7, 2026
@PastaPastaPasta PastaPastaPasta added this to the 24 milestone Oct 9, 2026

@PastaPastaPasta PastaPastaPasta left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved

@PastaPastaPasta
PastaPastaPasta merged commit 3d18d42 into dashpay:develop Oct 9, 2026
42 of 49 checks passed
@PastaPastaPasta
PastaPastaPasta deleted the codex/security-coinjoin-coins-lock branch October 9, 2026 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-candidate-24.0.x pastaclaw:approved thepastaclaw's latest review approved this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants