fix: data races and check-then-act races in the CoinJoin server - #7537
fix: data races and check-then-act races in the CoinJoin server#7537PastaPastaPasta wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCoinJoin session denomination storage is now atomic. Server collateral data uses synchronized session state. Pool checks and timeout handling use serialized execution and consistent session snapshots. Final transaction operations validate session IDs. Entry admission revalidates session state and rejects duplicate collateral prevouts. Lock contracts and concurrency tests were updated. Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CoinJoinServer
participant cs_check_pool
participant cs_coinjoin
participant Chainstate
participant FinalTransaction
CoinJoinServer->>cs_check_pool: serialize pool or timeout processing
CoinJoinServer->>cs_coinjoin: capture session state and session_id
CoinJoinServer->>Chainstate: validate entry or collateral
Chainstate-->>CoinJoinServer: return validation result
CoinJoinServer->>cs_coinjoin: revalidate session and capacity
CoinJoinServer->>FinalTransaction: create or commit with session_id
FinalTransaction->>cs_coinjoin: reject stale session
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit 597b549) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d449bb2c8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/coinjoin/server.cpp (1)
930-945: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRevalidate the session identity and the denomination under
cs_coinjoin.Line 930 compares
dsa.nDenomtonSessionDenomwithout the lock.IsAcceptableDSA()at Line 919 runs a mempool test-accept before that. The revalidation at Line 942 only checksnSessionID == 0, not that the session is still the same session.A scheduler-thread timeout can call
SetNull(), and anotherDSACCEPTcan open a new session with a different denomination, all inside that window. The observed values then belong to the old session, while the collateral at Line 960 is committed to the new one. The peer becomes a participant of a session whose denomination it never agreed to, and it is charged when it does not submit a matching entry.Capture
nSessionIDbefore the expensive validation, then compare both the session ID and the denomination under the lock.🐛 Proposed revalidation
-bool CCoinJoinServer::AddUserToExistingSession(const CCoinJoinAccept& dsa, PoolMessage& nMessageIDRet) -{ - // Cheap gates first: IsAcceptableDSA() below runs a mempool test-accept, which a full or - // absent session must not pay for. - if (nSessionID == 0 || WITH_LOCK(cs_coinjoin, return IsSessionReady())) return false; +bool CCoinJoinServer::AddUserToExistingSession(const CCoinJoinAccept& dsa, PoolMessage& nMessageIDRet) +{ + // Cheap gates first: IsAcceptableDSA() below runs a mempool test-accept, which a full or + // absent session must not pay for. + const int session_id{nSessionID}; + if (session_id == 0 || WITH_LOCK(cs_coinjoin, return IsSessionReady())) return false;- if (nSessionID == 0 || nState != POOL_STATE_QUEUE || IsSessionReady()) { + // The session that passed the checks above must still be the current one, with the same + // denomination: a reset plus a new session would otherwise admit this collateral to a + // session the peer never agreed to. + if (nSessionID != session_id || dsa.nDenom != nSessionDenom || nState != POOL_STATE_QUEUE || + IsSessionReady()) { nMessageIDRet = ERR_MODE; return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/coinjoin/server.cpp` around lines 930 - 945, Update CCoinJoinServer::AddUserToExistingSession to capture the current session ID before IsAcceptableDSA and the unlocked denomination validation, then under cs_coinjoin require both nSessionID and nSessionDenom to match those captured values. Reject with the existing error path if either changed, while preserving the current state and readiness checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/coinjoin/server.cpp`:
- Around line 392-407: Revalidate nSessionID before every tail operation in
CCoinJoinServer::CommitFinalTransaction after the unlocked ATMP/signing/relay
work. Add a reset_if_current helper that locks cs_coinjoin and calls SetNull
only when nSessionID equals session_id, replace both WITH_LOCK(cs_coinjoin,
SetNull()) calls with it, and guard ChargeRandomFees and both
RelayCompletedTransaction calls with the same session check so a newer session
is never reset, charged, or notified.
---
Outside diff comments:
In `@src/coinjoin/server.cpp`:
- Around line 930-945: Update CCoinJoinServer::AddUserToExistingSession to
capture the current session ID before IsAcceptableDSA and the unlocked
denomination validation, then under cs_coinjoin require both nSessionID and
nSessionDenom to match those captured values. Reject with the existing error
path if either changed, while preserving the current state and readiness checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bf00fe4b-c585-415c-a7cd-e1a42f94bc41
📒 Files selected for processing (4)
src/coinjoin/client.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/server.cppsrc/coinjoin/server.h
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Two in-scope CoinJoin lifecycle races remain at the exact head. A validated entry can be committed after its session has stopped accepting entries, and the scheduler can reset a session while its guarded finalization or commit is still running; the lint-only follow-up commit should also be folded into the commits that introduced those log calls.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/server.cpp`:
- [BLOCKING] src/coinjoin/server.cpp:766-775: Revalidate the session state before committing the entry
The commit-time check verifies only the session ID. While IsCollateralValid() or IsValidInOuts() is running, CheckPool() can take the timed-out ChargeAndFinalize path and transition the same session from POOL_STATE_ACCEPTING_ENTRIES to POOL_STATE_SIGNING without changing its ID. AddEntry() then appends an entry after finalMutableTransaction has already been built and relayed, so the accepted participant's input is absent from that transaction while its new unsigned entry prevents IsSignaturesComplete() from succeeding. The session eventually times out and may charge that participant. Require the captured session to still be accepting entries inside the locked commit block.
- [BLOCKING] src/coinjoin/server.cpp:297-301: Keep timeout resets inside the pool single-flight guard
cs_check_pool serializes CheckPool() only. If the message-handling thread holds it while finalizing or committing, the scheduler skips CheckPool() at line 1098 but immediately runs the unguarded CheckTimeout() at line 1099. During finalization, CheckTimeout() can observe the old timed-out accepting state and then reset the session immediately after CreateFinalTransaction() relays DSFINALTX, causing all returned signatures to be rejected. During commit, it can clear vecEntries and the final transaction before RelayCompletedTransaction(MSG_SUCCESS), leaving clients unnotified even though the DSTX is relayed. CheckTimeout's charging and reset must use the same single-flight guard and skip the scheduler tick when that guard is contended.
In `<commit:8c005c8>`:
- [SUGGESTION] <commit:8c005c8>:1: Squash the lint-only follow-up into its originating commits
Commit 8c005c841c6 only adds lint-logs.py continuation markers to calls introduced earlier in this PR. The CheckForCompleteQueue marker belongs in 4b8ba4e6b78, while the CreateNewSession and AddUserToExistingSession markers belong in abcaaeaee70. Fold those hunks into their originating commits and drop the standalone lint-fix commit so each commit remains independently lint-clean.
|
This pull request has conflicts, please rebase. |
nSessionDenom was the one CCoinJoinBaseSession field that was neither atomic nor guarded, while its siblings nState, nSessionID and nTimeLastSuccessfulStep are all std::atomic. On the server it is written by the message-handling thread in CreateNewSession() and by the scheduler thread in SetNull(), and read without any lock by CheckForCompleteQueue(), AddUserToExistingSession(), IsValidInOuts(), the relay logging, and by RPC threads via GetJsonInfo(). Concurrent unsynchronized access to a plain int is a data race: benign on the hardware we support, but formally UB and reportable by TSan.
8c005c8 to
aaa6d04
Compare
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aaa6d0464a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
All three carried-forward findings remain valid at head aaa6d04: two blocking CoinJoin lifecycle races and the suggestion to fold the lint-only follow-up into its originating commits. The latest rebase delta only adapts the ProcessGetData override to the updated base interface and introduces no new findings. The lifecycle races can admit an entry after finalization or reset a session during finalization/commit, so changes are still required.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:aaa6d04>`:
- [SUGGESTION] <commit:aaa6d04>:1: Squash the lint-only follow-up into its originating commits
Commit aaa6d0464a4 only adds four lint-logs.py continuation markers to LogPrint calls introduced earlier in this PR. The CheckForCompleteQueue marker belongs in 96cf3768cab, while the CreateNewSession and AddUserToExistingSession markers belong in 7443d022e09. Fold those hunks into their originating commits and drop the standalone lint-only commit so each substantive commit is independently lint-clean.
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At exact head bb056c6, the AddEntry state revalidation and CheckTimeout single-flight serialization fix both previously blocking CoinJoin lifecycle races. Two non-blocking commit-history cleanups remain: fold the lint-only follow-up into its originating commits and split or fold the final review-correction rollup into the commits it completes.
Source: reviewers gpt-5.6-sol (general) and gpt-5.6-sol (dash-core-commit-history); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:aaa6d04>`:
- [SUGGESTION] <commit:aaa6d04>:1: Squash the lint-only follow-up into its originating commits
Commit aaa6d0464a4 only adds four lint-logs.py continuation markers to LogPrint calls introduced earlier in this PR. The CheckForCompleteQueue marker belongs in 96cf3768cab, while the CreateNewSession and AddUserToExistingSession markers belong in 7443d022e09. Fold those hunks into their originating commits and drop the standalone lint-only commit so each substantive commit is independently lint-clean.
In `<commit:bb056c6>`:
- [SUGGESTION] <commit:bb056c6>:1: Fold the final race corrections into the commits they complete
Commit bb056c6d397 combines review corrections for two distinct earlier changes. The AddEntry state check completes the session revalidation introduced by cc270396780, while the CheckTimeout guard, atomic offender selection and timeout regression test complete the transition-serialization work introduced by 96cf3768cab. Fold the AddEntry hunk into cc270396780 and the timeout production changes into 96cf3768cab; the regression test can either accompany those changes or remain as a focused test commit.
bb056c6 to
76e366e
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The locking and session-revalidation changes address the targeted CoinJoin races, and both prior commit-organization findings were resolved by rewriting the stack. The exact head cannot compile because src/coinjoin/server.cpp contains an unmatched closing brace; the cs_check_pool documentation is also stale now that CheckTimeout() shares the guard.
Source: reviewers gpt-5.6-sol (general) and gpt-5.6-sol (dash-core-commit-history); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/server.cpp`:
- [BLOCKING] src/coinjoin/server.cpp:617: Remove the unmatched closing brace
`CheckTimeout()` is fully closed on line 616, and this additional `}` has no namespace, class, or other surrounding scope to close. It leaves an unmatched brace at global scope, so the following `CheckForCompleteQueue()` definition cannot be parsed and this translation unit will fail to compile. Remove line 617.
In `src/coinjoin/server.h`:
- [NITPICK] src/coinjoin/server.h:99-103: Update the cs_check_pool lock-discipline comment
The comment states that `cs_check_pool` is never taken by another code path, but `CheckTimeout()` now deliberately acquires it to serialize timeout resets with `CheckPool()` finalization and commit. Update this concurrency-sensitive documentation to describe both production users and preserve the explanation that contention causes a skipped round rather than blocking `msghand`.
76e366e to
bb056c6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 865d0bc33a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| TRY_LOCK(cs_check_pool, lock_check_pool); | ||
| if (!lock_check_pool) return; |
There was a problem hiding this comment.
Keep the pool guard across the timeout decision
When the last entry or signature arrives at the timeout boundary, the scheduler's CheckPool() can snapshot the session as incomplete, after which msghand records the final item but its own CheckPool() loses the TRY_LOCK race; the scheduler then releases this guard and immediately calls CheckTimeout(), which reacquires it and resets the now-complete session instead of finalizing or committing it. Fresh evidence in this revision is that the new timeout TRY_LOCK protects only the individual call, while Schedule() still leaves a lock-release gap between CheckPool() and CheckTimeout(); keep the guard across both decisions or recheck whether the session is now finalizable before SetNull().
AGENTS.md reference: AGENTS.md:L173-L175
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At exact head 68ad451, the targeted CoinJoin races are addressed and both prior findings are fixed; no in-scope correctness blocker remains. Four non-blocking commit-history improvements remain: fold three follow-up corrections into their originating commits and document the rationale for the late concurrency fixes.
Source: reviewers gpt-5.6-sol (general) and gpt-5.6-sol (dash-core-commit-history); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 4 suggestion(s)
4 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:aaa6d04>`:
- [SUGGESTION] <commit:aaa6d04>:1: Fold the lint-only commit into its originating changes
Commit aaa6d0464a4 only adds lint-logs continuation markers to LogPrint calls introduced earlier in this stack. Move the CheckForCompleteQueue marker into 96cf3768cab and the CreateNewSession and AddUserToExistingSession markers into 7443d022e09, then drop aaa6d0464a4. This keeps each substantive commit independently lint-clean and avoids retaining a CI-only follow-up.
In `<commit:bb056c6>`:
- [SUGGESTION] <commit:bb056c6>:1: Fold the entry-state correction into the entry revalidation commit
Commit cc270396780 says an entry can only be committed to the session for which it was validated, but its commit-time gate checks only the session ID. Commit bb056c6d397 later completes that invariant by also requiring POOL_STATE_ACCEPTING_ENTRIES, alongside unrelated timeout-serialization work. Move the AddEntry state-check hunk into cc270396780 so that commit is complete and accurate on its own.
- [SUGGESTION] <commit:bb056c6>:1: Add rationale to the late concurrency-fix commit messages
Commits bb056c6d397, 7da61132813, and 68ad4510c1f have empty bodies despite encoding non-obvious concurrency invariants. Add concise bodies explaining the timeout/finalization interleaving, why denomination validation must use a captured session snapshot, and why newly actionable work must take priority over timeout reset. This preserves the reasoning needed by future git-log and bisect readers without relying on the PR discussion.
In `<commit:865d0bc>`:
- [SUGGESTION] <commit:865d0bc>:1: Fold the pool-lock documentation correction into the code change
Commit bb056c6d397 makes CheckTimeout acquire cs_check_pool but leaves the preceding comment claiming no other path takes the guard. Commit 865d0bc33a8 only corrects that stale documentation. Fold the final comment into bb056c6d397 and drop the documentation-only follow-up so the concurrency change documents its lock discipline when introduced.
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact head addresses the targeted CoinJoin races, including timeout serialization, session-bound validation, progress-before-timeout handling, and completion-tail snapshots; no correctness blocker remains. Four non-blocking commit-history improvements remain valid: fold three corrective follow-ups into their originating commits and document the rationale for the late concurrency fixes.
Source: reviewers gpt-5.6-sol (general) and gpt-5.6-sol (dash-core-commit-history); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 4 suggestion(s)
4 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:aaa6d04>`:
- [SUGGESTION] <commit:aaa6d04>: Fold the lint-only commit into its originating changes
Commit aaa6d0464a4 only adds four lint-logs continuation markers to calls introduced earlier in this stack. Move the CheckForCompleteQueue marker into 96cf3768cab and the three CreateNewSession/AddUserToExistingSession markers into 7443d022e09, then drop aaa6d0464a4. This keeps each substantive commit independently lint-clean and avoids retaining a CI-only follow-up.
In `<commit:bb056c6>`:
- [SUGGESTION] <commit:bb056c6>: Fold the entry-state correction into the entry revalidation commit
Commit cc270396780 says an entry can only be committed to the session for which it was validated, but its commit-time gate checks only the session ID. Commit bb056c6d397 later completes that invariant by also requiring POOL_STATE_ACCEPTING_ENTRIES, alongside timeout-serialization work. Move the AddEntry state-check hunk into cc270396780 so that commit is complete and accurate on its own.
- [SUGGESTION] <commit:bb056c6>: Add rationale to the late concurrency-fix commit messages
Commits bb056c6d397, 7da61132813, 68ad4510c1f, and f708bb3e396 have empty bodies despite encoding non-obvious concurrency invariants. Add concise bodies explaining the timeout/finalization interleaving, why validation uses a captured denomination, why actionable work takes priority over timeout reset, and why completion notification, fee charging, and reset remain bound to the captured session. This preserves the reasoning needed by future git-log and bisect readers without relying on the PR discussion.
In `<commit:865d0bc>`:
- [SUGGESTION] <commit:865d0bc>: Fold the pool-lock documentation correction into the code change
Commit bb056c6d397 makes CheckTimeout acquire cs_check_pool but retains the preceding comment claiming no other path takes the guard. Commit 865d0bc33a8 only corrects that stale documentation. Fold the final comment into bb056c6d397 and drop the documentation-only follow-up so the concurrency change documents its lock discipline when introduced.
CheckPool() and CheckForCompleteQueue() read nState, the entry count and the collateral count under separate lock acquisitions (or none at all) and then acted on the result, so a scheduler-thread SetNull() could land between the samples. CheckPool() is the worst case: it sampled nState, then took and released cs_coinjoin for GetEntriesCount(), then read vecSessionCollaterals.size() unlocked. A SetNull() in between made an already-reset session read as '0 entries == 0 collaterals' and get finalized, putting a dead session back into POOL_STATE_SIGNING and rejecting every new dsa until the 15s signing timeout expired. It now decides from one snapshot and acts afterwards, and CreateFinalTransaction()/CommitFinalTransaction() revalidate nSessionID because the decision is made with the lock released. CheckPool() also runs on both the scheduler thread and the message-handling thread, so two concurrent calls could both finalize: clients would receive DSFINALTX twice, sign twice, and the duplicate signatures make AddScriptSig() fail and abort the session for everyone. A TRY_LOCK-only cs_check_pool makes it single-shot without ever blocking msghand. SetState() and IsSessionReady() now require cs_coinjoin, so a transition and the session data it describes can only be observed together; this is what makes the existing revalidation blocks in CreateNewSession()/AddUserToExistingSession() effective. CheckForCompleteQueue() performs its transition under the lock and moves BLS signing and dsq relay outside it. ChargeFees() samples nState once instead of three times, which previously let it select 'didn't send' offenders and then charge and log them as 'didn't sign'.
vecSessionCollaterals had no GUARDED_BY and was reached from both threads with no lock at all: the message-handling thread read it in ProcessDSACCEPT(), IsSessionReady() and AddEntry(), while the scheduler thread read it in CheckPool(), CheckForCompleteQueue(), ChargeFees() and ChargeRandomFees(). The only synchronized access was the clear() in SetNull(). Committing a collateral therefore raced every one of those reads. The worst of them was ChargeRandomFees(), which iterated the vector by reference while calling ConsumeCollateral() - a cs_main mempool submission - for each element. A concurrent SetNull() destroys the CTransactionRefs the loop is walking, so this was a use-after-free and not just a torn size read. It now works from a copy taken under the lock, which also keeps cs_coinjoin from being held across cs_main. The transactions and their prevout index are now a single SessionCollaterals member so they cannot drift apart, and GUARDED_BY on that member makes every access - including the calls on it - checked by -Wthread-safety. Reintroducing an unlocked read is now a compile error rather than a review finding.
AddEntry() checked its bound, then ran IsCollateralValid() and IsValidInOuts() - both of which take cs_main and can block behind block validation - and only then took cs_coinjoin again to push_back. A scheduler-thread CheckTimeout() in that window calls SetNull(), so the entry was committed to a session that no longer existed. The consequence outlives the window: vecEntries keeps the orphaned entry while vecSessionCollaterals is empty, so the next session starts one entry ahead of its own participant count. CheckPool()'s entries == collaterals test then fires early and finalizes a transaction containing an input from the dead session, which nobody present will sign, stalling the new session to its signing timeout and charging its honest participants in ChargeFees(). The bound check and the push_back now share one lock scope, and the session identity captured before validation is rechecked inside it, so an entry can only ever be committed to the session it was validated for.
CheckPool() holds cs_check_pool while finalizing or committing, but CheckTimeout() could previously reset the same session concurrently after the pool decision released cs_coinjoin. Make CheckTimeout() use the same TRY_LOCK guard, select any offender and close the session in one cs_coinjoin critical section, then consume the captured collateral after releasing it. This prevents timeout reset from invalidating finalization or commit without blocking the message-handling thread.
f708bb3 to
dfbde94
Compare
AddEntry() snapshots the active session denomination before validation. If a timeout resets the session while IsCollateralValid() or IsValidInOuts() is blocked on chainstate work, validation must still evaluate the denomination the participant agreed to, while the final locked gate rejects committing into a changed session. Pass the captured denomination explicitly through IsValidInOuts() so a concurrent reset cannot turn a valid old-session entry into a punishable denomination mismatch.
The scheduler calls CheckPool() and CheckTimeout() separately, leaving a gap in which the final collateral, entry, or signature can arrive after CheckPool() takes its snapshot. The message thread then skips its own CheckPool() while the scheduler holds cs_check_pool, and an unconditional timeout reset would discard a session that can now advance. Recheck readiness, finalizability, and signature completeness under cs_coinjoin before resetting. Actionable work takes priority and is picked up by the next scheduler tick.
Completion relay previously read and mutated live session state after CommitFinalTransaction() released cs_coinjoin. If all old participants disconnected, RelayCompletedTransaction() could reset the session, allowing a replacement session to open before random charging and the unconditional tail reset; those operations could then charge or clear the replacement. Capture the committed transaction, participants, and collaterals under cs_coinjoin. Relay and charge only those snapshots, keep completion notification side-effect-free, and reset only the matching signing session. The invalid-transaction path now also notifies captured participants before reset.
dfbde94 to
597b549
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At exact head 597b549, the CoinJoin session transitions, timeout handling, entry admission, collateral charging, and completion tail are consistently bound to locked session snapshots; no in-scope correctness issue remains. All four prior commit-history findings are fixed in the rewritten eight-commit stack, and the sole CodeRabbit concern is resolved by the shared pool guard plus snapshot-based completion handling.
Source: reviewers gpt-5.6-sol (general) and gpt-5.6-sol (dash-core-commit-history); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
Issue being fixed or feature implemented
CCoinJoinServerstate is touched by two threads:msghand(single-threaded, viaProcessDS*) and the 1s scheduler tick (CheckForCompleteQueue/CheckPool/CheckTimeout), plus RPC threads inGetJsonInfo(). Note thatCheckPool()runs on both — the scheduler tick andmsghandviaProcessDSVIN/ProcessDSSIGNFINALTX.vecSessionCollateralswas mutated only undercs_coinjoinbut read without any lock from seven places across both threads, and several decisions samplednState, the entry count and the collateral count under separate lock acquisitions before acting on the result. #7507 fixed the write side of the collateral race; this PR is the follow-up that closes the read side and the check-then-act paths around it.These are the concrete, reachable failures, not just TSan-visible UB:
Use-after-free in
ChargeRandomFees(). It iteratedvecSessionCollateralsby reference while callingConsumeCollateral()— acs_mainmempool submission — for each element. A concurrentSetNull()destroys theCTransactionRefs the loop is walking.A reset session could be finalized.
CheckPool()samplednState, then took and releasedcs_coinjoinforGetEntriesCount(), then readvecSessionCollaterals.size()unlocked. ASetNull()between the samples makes an already-reset session read as0 entries == 0 collaterals→ finalize. That builds an empty final transaction and puts a dead session back intoPOOL_STATE_SIGNING, so every newdsais rejected withERR_MODEuntil the 15s signing timeout clears it.A session could be finalized twice. Because
CheckPool()runs on both threads, two concurrent calls could both take the finalize path. Clients then receiveDSFINALTXtwice and sign twice; the duplicate signatures makeAddScriptSig()fail, which relaysSTATUS_REJECTEDand aborts the session for every participant.AddEntry()could commit into a dead session, and the damage outlived the window. The bound was checked, thenIsCollateralValid()andIsValidInOuts()ran (both takecs_main, both can block behind block validation), and only then wascs_coinjoinre-taken topush_back. ACheckTimeout()in that window resets the session, leaving an orphaned entry invecEntrieswhilevecSessionCollateralsis empty — so the next session starts one entry ahead of its own participant count,CheckPool()'sentries == collateralstest fires early, and it finalizes a transaction containing an input nobody present will sign. That session then stalls to its signing timeout andChargeFees()charges its honest participants.ChargeFees()samplednStatethree times, so a transition in between could select the "didn't send" offenders and then charge and log them as "didn't sign".nSessionDenomwas a plainintwritten by both threads and read unlocked by both plus RPC threads — the oneCCoinJoinBaseSessionfield that was neither atomic nor guarded. Benign on supported hardware, but UB and TSan-reportable.What was done?
Four commits, each standalone:
fix: make CoinJoin nSessionDenom atomic—std::atomic<int>, matching its siblingsnState,nSessionIDandnTimeLastSuccessfulStep.WalletCJLogPrint()takes its arguments by value, so the three client-side log sites need an explicit.load();LogPrint()takes by const reference and does not.fix: decide CoinJoin server state transitions under cs_coinjoin— addresses 2, 3 and 5.CheckPool()decides from a single locked snapshot, then acts with the lock released.CreateFinalTransaction()/CommitFinalTransaction()take the session id the decision was made for and revalidate it, since the lock is dropped in between.cs_check_pool, acquired only viaTRY_LOCKand only at the top ofCheckPool(), makes finalize and commit single-shot. It is strictly outermost and never contended-blocking, somsghandis never made to wait on the scheduler thread; a contended caller just skips to the next tick.SetState()andIsSessionReady()now requirecs_coinjoin. This is what makes the existing revalidation blocks inCreateNewSession()/AddUserToExistingSession()actually effective — previouslynStatecould be flipped by a concurrentSetState()immediately after they revalidated it.CheckForCompleteQueue()performs its transition under the lock and moves BLS signing and thedsqrelay outside it — a shorter hold than before, not a longer one.ChargeFees()samplesnStateonce, under the lock, together with the data it describes.fix: guard the CoinJoin session collaterals with cs_coinjoin— addresses 1.vecSessionCollateralsandsetSessionCollateralPrevoutsbecome a singleSessionCollateralsmember markedGUARDED_BY(cs_coinjoin). Folding them together means one annotation covers both and they cannot drift or be annotated inconsistently again — which is exactly what went wrong before, where one was guarded and the other was not.ChargeRandomFees()now works from a copy taken under the lock, which fixes the use-after-free and also keepscs_coinjoinfrom being held acrosscs_main.CopyTxs(), which returns by value on purpose:WITH_LOCKexpands to a lambda returningdecltype(auto), so returning aconst&accessor through it would hand back a reference and perform the copy after the lock was released.fix: revalidate the CoinJoin session before committing an entry— addresses 4. The bound check and thepush_backnow share one lock scope, and the session identity captured before validation is rechecked inside it. The duplicate-input scan is also hoisted out of the per-input loop so it is atomic across the whole entry instead of re-locking up to nine times.The load-bearing part is the
GUARDED_BY: reintroducing an unlocked read of the session collaterals is now a compile error under-Wthread-safetyrather than something a reviewer has to catch.Lock discipline
No new
cs_coinjoin-across-cs_main,cs_coinjoin-across-network-send, orcs_coinjoin-across-BLS-signing edge is introduced;CheckForCompleteQueue()andChargeRandomFees()end up with strictly shorter holds than on develop.cs_check_poolisTRY_LOCK-only, taken by exactly one function while holding nothing else, so the ordercs_check_pool→cs_coinjoinis unidirectional and no ABBA cycle is possible.How Has This Been Tested?
macOS arm64,
--enable-debug, depends build.clang -Wthread-safetyclean on every TU that includescoinjoin/server.h(server.cpp,rpc/coinjoin.cpp,init.cpp,test/coinjoin_inouts_tests.cpp) pluscoinjoin.cppandclient.cpp. The baseline before these changes was also clean, so nothing is being suppressed.coinjoin*(35 cases) and walletcoinjoin_tests(12 cases) pass.test/functional/rpc_coinjoin.pyandtest/functional/p2p_dstx.pypass.clang-format-diffinvocation returns no output.No new tests. These are multi-thread interleavings on the masternode side of mixing; reproducing them deterministically needs a harness that can drive
CCoinJoinServerfrom two threads with controlled scheduling, which does not exist today. Worth building separately — flagging it explicitly rather than leaving it as a silent gap.Breaking Changes
None. No protocol, serialization or RPC change. Two debug log lines have
vecSessionCollaterals.size():renamed toparticipants:since the member no longer exists under that name.Checklist: