fix: data races and check-then-act races in the CoinJoin server

[PastaPastaPasta]

Issue being fixed or feature implemented

CCoinJoinServer state is touched by two threads: msghand (single-threaded, via ProcessDS*) and the 1s scheduler tick (CheckForCompleteQueue / CheckPool / CheckTimeout), plus RPC threads in GetJsonInfo(). Note that CheckPool() runs on both — the scheduler tick and msghand via ProcessDSVIN / ProcessDSSIGNFINALTX.

vecSessionCollaterals was mutated only under cs_coinjoin but read without any lock from seven places across both threads, and several decisions sampled nState, 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:

1. Use-after-free in ChargeRandomFees(). It iterated vecSessionCollaterals by reference while calling ConsumeCollateral() — a cs_main mempool submission — for each element. A concurrent SetNull() destroys the CTransactionRefs the loop is walking.

2. A reset session could be finalized. CheckPool() sampled nState, then took and released cs_coinjoin for GetEntriesCount(), then read vecSessionCollaterals.size() unlocked. A SetNull() between the samples makes an already-reset session read as 0 entries == 0 collaterals → finalize. That builds an empty final transaction and puts a dead session back into POOL_STATE_SIGNING, so every new dsa is rejected with ERR_MODE until the 15s signing timeout clears it.

3. A session could be finalized twice. Because CheckPool() runs on both threads, two concurrent calls could both take the finalize path. Clients then receive DSFINALTX twice and sign twice; the duplicate signatures make AddScriptSig() fail, which relays STATUS_REJECTED and aborts the session for every participant.

4. AddEntry() could commit into a dead session, and the damage outlived the window. The bound was checked, then IsCollateralValid() and IsValidInOuts() ran (both take cs_main, both can block behind block validation), and only then was cs_coinjoin re-taken to push_back. A CheckTimeout() in that window resets the session, leaving an orphaned entry in vecEntries while vecSessionCollaterals is empty — so the next session starts one entry ahead of its own participant count, CheckPool()'s entries == collaterals test fires early, and it finalizes a transaction containing an input nobody present will sign. That session then stalls to its signing timeout and ChargeFees() charges its honest participants.

5. ChargeFees() sampled nState three times, so a transition in between could select the "didn't send" offenders and then charge and log them as "didn't sign".

6. nSessionDenom was a plain int written by both threads and read unlocked by both plus RPC threads — the one CCoinJoinBaseSession field that was neither atomic nor guarded. Benign on supported hardware, but UB and TSan-reportable.

What was done?

Eight commits, each standalone:

fix: make CoinJoin nSessionDenom atomic — std::atomic<int>, matching its siblings nState, nSessionID and nTimeLastSuccessfulStep. 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.
• A new cs_check_pool, acquired only via TRY_LOCK by CheckPool() and CheckTimeout(), makes finalization, commit and timeout reset single-shot. It is strictly outermost and never contended-blocking, so msghand is never made to wait on the scheduler thread; a contended caller just skips to the next tick.
• SetState() and IsSessionReady() now require cs_coinjoin. This is what makes the existing revalidation blocks in CreateNewSession() / AddUserToExistingSession() actually effective — previously nState could be flipped by a concurrent SetState() immediately after they revalidated it.
• CheckForCompleteQueue() performs its transition under the lock and moves BLS signing and the dsq relay outside it — a shorter hold than before, not a longer one.
• ChargeFees() samples nState once, under the lock, together with the data it describes.

fix: guard the CoinJoin session collaterals with cs_coinjoin — addresses 1.

• vecSessionCollaterals and setSessionCollateralPrevouts become a single SessionCollaterals member marked GUARDED_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.
• Every read site is fixed. ChargeRandomFees() now works from a copy taken under the lock, which fixes the use-after-free and also keeps cs_coinjoin from being held across cs_main.
• The copy goes through CopyTxs(), which returns by value on purpose: WITH_LOCK expands to a lambda returning decltype(auto), so returning a const& 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 the push_back now 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.

fix: serialize CoinJoin timeout transitions — makes CheckTimeout() use the same cs_check_pool guard as CheckPool(). Offender selection and session reset happen together under cs_coinjoin; collateral consumption happens after releasing it. A timeout can no longer invalidate a finalization or commit already in progress.

fix(coinjoin): validate entries against session snapshot — passes the denomination captured with the session identity into IsValidInOuts(). A concurrent reset can therefore reject the stale commit without first misclassifying the old entry as a punishable denomination mismatch.

fix(coinjoin): prioritize progress over timeout reset — rechecks queue readiness, finalizability and signature completeness under cs_coinjoin immediately before a timeout reset. If the last collateral, entry or signature arrived after the scheduler's earlier snapshot, actionable work wins and is handled on the next tick.

fix(coinjoin): bind completion tail to session snapshot — captures the final transaction, participants and collaterals under cs_coinjoin, makes completion notification side-effect-free, and resets only the matching signing session. Delayed relay or fee work can no longer charge or clear a replacement session.

The load-bearing part is the GUARDED_BY: reintroducing an unlocked read of the session collaterals is now a compile error under -Wthread-safety rather than something a reviewer has to catch.

Lock discipline

No new cs_coinjoin-across-cs_main, cs_coinjoin-across-network-send, or cs_coinjoin-across-BLS-signing edge is introduced; CheckForCompleteQueue() and ChargeRandomFees() end up with strictly shorter holds than on develop. cs_check_pool is TRY_LOCK-only, taken only by CheckPool() and CheckTimeout() while holding nothing else, so the order cs_check_pool → cs_coinjoin is unidirectional and no ABBA cycle is possible.

How Has This Been Tested?

macOS arm64, --enable-debug, depends build.

• Clean build.
• clang -Wthread-safety clean on every TU that includes coinjoin/server.h (server.cpp, rpc/coinjoin.cpp, init.cpp, test/coinjoin_inouts_tests.cpp) plus coinjoin.cpp and client.cpp. The baseline before these changes was also clean, so nothing is being suppressed.
• Each of the eight commits compiles on its own.
• Full unit suite passes. Targeted coinjoin_inouts_tests (12 cases) and wallet coinjoin_tests (12 cases) pass.
• test/functional/rpc_coinjoin.py and test/functional/p2p_dstx.py pass.
• CI's exact clang-format-diff invocation returns no output.

Four targeted regression tests were added:

• server_timeout_does_not_reset_during_pool_check drives cs_check_pool from two threads and proves a contended timeout tick cannot reset an in-flight pool check.
• server_timeout_does_not_reset_actionable_session covers queue-ready, finalizable and fully signed sessions at their timeout boundary.
• server_validation_uses_session_denom_snapshot proves validation uses the captured denomination after a concurrent reset.
• server_completion_does_not_reset_an_unreachable_or_replacement_session proves completion relay is side-effect-free and a delayed reset cannot clear a replacement queue.

These tests deterministically cover the newly exposed concurrency seams. Broader end-to-end scheduling coverage would still benefit from a dedicated multi-threaded CoinJoin server harness.

Breaking Changes

None. No protocol, serialization or RPC change. Two debug log lines have vecSessionCollaterals.size(): renamed to participants: since the member no longer exists under that name.

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

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

CoinJoin 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

Loading

Possibly related PRs

• dashpay/dash#7507: Updates existing-session admission to reject replayed collateral prevouts.
• dashpay/dash#7566: Modifies CoinJoin collateral handling and fee charging.
• dashpay/dash#7567: Modifies CoinJoin session timeout and collateral handling.

Suggested reviewers: thepastaclaw, udjinm6

🚥 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%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly summarizes the main changes: fixing data races and check-then-act races in the CoinJoin server.
Description check	✅ Passed	The description directly explains the CoinJoin race conditions, synchronization changes, affected code paths, and tests.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[thepastaclaw]

⛔ Blockers found — Opus deferred (commit e50b57d)
Canonical validated blockers: 1

[chatgpt-codex-connector]

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

[github-actions]

This pull request has conflicts, please rebase.

[coderabbitai]

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 win

Revalidate the session identity and the denomination under cs_coinjoin.

Line 930 compares dsa.nDenom to nSessionDenom without the lock. IsAcceptableDSA() at Line 919 runs a mempool test-accept before that. The revalidation at Line 942 only checks nSessionID == 0, not that the session is still the same session.

A scheduler-thread timeout can call SetNull(), and another DSACCEPT can 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 nSessionID before 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

📥 Commits

Reviewing files that changed from the base of the PR and between ffaa783 and 8c005c8.

📒 Files selected for processing (4)

• src/coinjoin/client.cpp
• src/coinjoin/coinjoin.h
• src/coinjoin/server.cpp
• src/coinjoin/server.h

[thepastaclaw]

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.

[github-actions]

This pull request has conflicts, please rebase.

[chatgpt-codex-connector]

💡 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]

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]

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.

[thepastaclaw]

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

[chatgpt-codex-connector]

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

[chatgpt-codex-connector]

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]

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]

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.

[thepastaclaw]

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)

[knst]

is it just copy-paste of original implementation? This regressions tests are basically useless then ; they tests not interface but the very specific implementation

[thepastaclaw]

Final validation — Codex/Sol only (Phase 2 disabled)

The exact head consistently binds CoinJoin finalization, timeout handling, entry admission, completion notification, fee charging, and reset operations to synchronized session state; no in-scope correctness issue remains. One non-blocking commit-history cleanup remains because the final review-feedback rollup changes lines introduced by several earlier commits and should be folded into those commits.
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)

🟡 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 `<commit:2957e73>`:
- [SUGGESTION] <commit:2957e73>:1: Fold the review-feedback rollup into its originating commits
  Commit 2957e738a77 (`refactor(coinjoin): address review feedback`) revises material introduced by several earlier commits: the atomic-field explanation from 5d22ec38dc7, casts introduced by a926d583b0e, 83f07112087, and dd838402ab5, and the test-fixture documentation associated with the regression-test changes. Fold each hunk into the commit that introduced the affected material and drop this generic review-feedback rollup so the substantive commits remain independently complete and blame retains the correct rationale.

[thepastaclaw]

Preliminary review — Codex only

One blocking signing-timeout race remains: a multi-input DSSIGNFINALTX message is applied through separately locked AddScriptSig() calls, allowing CheckTimeout() to reset and potentially charge the session between inputs from the same already-received message. The rewritten eight-commit stack fixes the prior commit-history finding, and the CodeRabbit completion-tail concern is resolved by session snapshots, matching-session reset, and the shared pool guard.
Source: reviewer backend gpt-5.6-sol; final verifier backend 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)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 1 blocking

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:263-277: Apply each signature message atomically against timeout reset
  A single DSSIGNFINALTX message can contain several inputs, but each input is committed by a separate AddScriptSig() call that independently acquires and releases cs_coinjoin. At the signing-timeout boundary, the scheduler can run after the first input: CheckPool() snapshots the session as incompletely signed, then CheckTimeout() acquires cs_check_pool and cs_coinjoin, still sees an incomplete session, selects an unsigned participant as an offender, and calls SetNull(). Processing the next input from the already-received message then fails against the cleared session, and the selected participant's collateral can be consumed. Validate the state and participant and apply the complete vecTxIn batch in one cs_coinjoin critical section; after that lock is released, CheckPool() can perform the completion work because CheckTimeout() will observe IsSignaturesComplete().

[thepastaclaw]

Preliminary review — Codex only

One blocking signing-timeout race remains: a multi-input DSSIGNFINALTX message is still applied through separately locked AddScriptSig() calls, allowing the scheduler to reset the session between inputs from the same message. The DSVIN cutoff claim is not actionable because entry validity is established only when AddEntry() commits under the session lock, while the CodeRabbit completion-tail concern is resolved by captured participants and collaterals, matching-session reset, and the shared pool guard.
Source: Reviewer backend model: gpt-5.6-sol; final verifier backend model: 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)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 1 blocking

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:266-268: Apply each signature message atomically against timeout reset
  A DSSIGNFINALTX message can contain several signed inputs, but this loop commits each input through a separate AddScriptSig() call, releasing cs_coinjoin after every input. At the signing-timeout boundary, the scheduler can run between two inputs: CheckPool() observes an incomplete session, then CheckTimeout() acquires cs_check_pool and cs_coinjoin, still finds an unsigned input, can select that participant as an offender, and resets the session. The next input from the already-deserialized message then fails against the cleared state, and the honest participant's collateral can be consumed. Revalidate the signing state and participant after deserialization and apply the entire vecTxIn batch while holding one cs_coinjoin critical section; release the lock before calling CheckPool().

[thepastaclaw]

Preliminary review — Codex only

One blocking CoinJoin signing-timeout race remains at the exact head: a DSSIGNFINALTX batch is still applied through separately locked AddScriptSig() calls, so CheckTimeout() can reset and potentially charge the session between inputs from the same received message. The CodeRabbit completion-tail concern no longer applies because completion uses captured participants and collaterals, ResetSigningSessionIfCurrent() is session-bound, and CheckPool()/CheckTimeout() share cs_check_pool.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model 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)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 1 blocking

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:266-268: Apply each signature message atomically against timeout reset
  A DSSIGNFINALTX message can contain multiple signed inputs, but this loop applies each input through a separate AddScriptSig() call, releasing cs_coinjoin after every input. At the signing-timeout boundary, the scheduler can acquire cs_check_pool and cs_coinjoin between two calls, observe that signatures are still incomplete, select the participant as an offender, and reset the session. The next input from the already-deserialized message then fails against the cleared state, and the selected participant's collateral can be consumed. The state and participant checks are also released before deserialization, so the same reset can occur before the first input is applied. After deserialization, revalidate that the same signing session and participant are current, then apply the complete vecTxIn batch under one cs_coinjoin critical section; release it before calling CheckPool().

[github-actions]

This pull request has conflicts, please rebase.