[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(). CheckPool() runs on both: the scheduler tick and msghand via ProcessDSVIN / ProcessDSSIGNFINALTX.

Several pieces of the original version of this PR have since landed on develop separately (#7597 nSessionDenom atomic, #7599 entry admission bound to the session snapshot, plus the single-snapshot CheckPool() and the session-bound CreateFinalTransaction() from the promotion/demotion work). This PR is now rebased on top of those and contains what is still missing:

1. CheckPool() was not single-flight. Two concurrent calls (scheduler + msghand) could both act on the same snapshot. If both finalize, clients receive DSFINALTX twice and sign twice, and the duplicate signatures make AddScriptSig() fail, which relays STATUS_REJECTED and aborts the session for everyone. Both calls could also commit the same final transaction.
2. CommitFinalTransaction() was not bound to a session. It committed whatever finalMutableTransaction held, even when the session had timed out and been reset after the decision. That pushes a cleared transaction through ATMP and notifies participants of a failure that never happened.
3. CheckTimeout() raced finalization and commit. It tested HasTimedOut() without a lock and could reset a session halfway through CheckPool() finalizing or committing it. It also reset sessions that could still advance, because the final collateral, entry or signature can arrive after the scheduler's CheckPool() snapshot. In that case msghand skips its own CheckPool() while the scheduler holds the guard.
4. Offender selection and charging were not atomic with closing admission. ChargeFees() sampled nState three times, so it could select "didn't send" offenders and then charge and log them as "didn't sign". On the timeout-finalize path it ran before eligibility was rechecked, so an ineligible session was charged once there and again by the CheckTimeout() reset that followed.
5. The session collaterals were not GUARDED_BY. vecSessionCollaterals was unannotated while its companion prevout index was guarded. CreateNewSession() still read it after releasing the lock, and nothing prevented the next unlocked read.
6. The completion tail mutated live state. If every participant had disconnected, RelayCompletedTransaction() could reset the session. A replacement session could then open before random charging and the unconditional tail reset ran, and those would charge or clear the replacement. The invalid-transaction path also reset the pool before notifying anyone, so ERR_INVALID_TX reached nobody.
7. A DSSIGNFINALTX message was not applied atomically. Each input went through its own locked AddScriptSig(). A signing timeout landing between two inputs of one already-received message would select the sender as a non-signer, charge them, and reset the session.

What was done?

Six commits, each standalone:

fix(coinjoin): decide server state transitions under cs_coinjoin: addresses 1, 2 and the triple sample in 4. A new cs_check_pool, acquired only via TRY_LOCK, makes CheckPool() single-shot without ever blocking msghand. Signature completeness joins the existing pool snapshot. CommitFinalTransaction() takes the session id the decision was made for and revalidates it. SetState() and IsSignaturesComplete() now require cs_coinjoin, and ChargeFees() samples nState once.

fix(coinjoin): guard the session collaterals with cs_coinjoin: addresses 5. The collateral transactions and their prevout index become a single SessionCollaterals member marked GUARDED_BY(cs_coinjoin), so they cannot drift apart or be annotated inconsistently. Reintroducing an unlocked read is now a -Wthread-safety compile error.

fix(coinjoin): serialize timeout transitions: addresses 3 (first half) and the rest of 4. CheckTimeout() uses the same cs_check_pool guard. Offender selection moves into SelectCollateralToCharge() under cs_coinjoin, in the same lock scope that resets the session (CheckTimeout()) or moves it to SIGNING (CreateFinalTransaction(session_id, charge_fees)). The timeout-finalize path only charges once the live session is confirmed eligible. Collateral is consumed after cs_coinjoin is released, because ConsumeCollateral() takes cs_main. The session may be reset before that consumption runs, so the selected collateral stays reserved in m_collaterals_being_charged (which survives SetNull()) until ConsumeCollateral() has finished. During that time CreateNewSession() and AddUserToExistingSession() refuse a dsa whose collateral spends one of its inputs. The charged participant therefore cannot be readmitted into the next session with a collateral that is about to be spent, and the fee is still taken. AddEntry()'s ConsumeCollateralIfCurrentSession() takes the same reservation.

fix(coinjoin): prioritize progress over timeout reset: addresses 3 (second half). Before resetting, CheckTimeout() rechecks under cs_coinjoin whether the session can still advance: queue ready, an entry for every collateral, enough entries covering both sides of the session denomination, or every signature in. If so it leaves the session to the next tick. The rules mirror the decisions CheckPool() and CreateFinalTransaction() make, so a deferred session is always acted on. The decision is the pure function ShouldResetTimedOutSession(), which CheckTimeout() calls with the session's progress read under cs_coinjoin.

fix(coinjoin): bind completion tail to session snapshot: addresses 6. The committed transaction, participants and collaterals are captured under cs_coinjoin. Completion notification is side-effect-free, and only the matching signing session is reset. The invalid-transaction path notifies the captured participants. CheckPool()'s uncovered-session reset captures its participants and resets in one step, then tells them.

fix(coinjoin): apply a DSSIGNFINALTX message atomically: addresses 7. The signing session the sender was checked against is revalidated after decoding, and the whole batch is applied in one cs_coinjoin critical section. The rejection relay for a failed input happens inside that same section.

Lock discipline

No new cs_coinjoin-across-cs_main edge is introduced: offender collateral is consumed only after cs_coinjoin is released, and signature verification already ran under cs_coinjoin. cs_check_pool is TRY_LOCK-only, taken only by CheckPool() and CheckTimeout() while holding nothing else, so cs_check_pool → cs_coinjoin / cs_main is unidirectional and no ABBA cycle is possible.

How Has This Been Tested?

macOS arm64, depends build, clang with -Wthread-safety -Werror, rebased on develop c8bfd7a.

• Every one of the six commits builds test_dash and passes coinjoin_inouts_tests on its own.
• Unit suites coinjoin_inouts_tests, coinjoin_tests, coinjoin_queue_tests, coinjoin_dstxmanager_tests pass.
• Functional rpc_coinjoin.py and p2p_dstx.py pass.
• lint-whitespace.py, lint-includes.py, lint-circular-dependencies.py pass.

New/updated unit tests in coinjoin_inouts_tests:

• server_timeout_resets_only_sessions_that_cannot_advance is one table over ShouldResetTimedOutSession(). Rows that must be left alone: a session that has not timed out; a ready queue; an entry for every collateral (even with a side uncovered, which CheckPool() resets itself and tells the participants); enough covered entries with one straggler; every input signed. Rows that must be reset: a queue that is not ready; too few entries; enough entries with a side uncovered and one straggler; a missing signature. Mutations checked: removing the deferral, the signing-advance clause, either accepting-advance clause, or the side-coverage condition each fails at least one row.
• server_finalization_rechecks_live_side_coverage now exercises the charging finalize path.

The cs_check_pool single-flight guard, the completion-tail snapshot and the DSSIGNFINALTX atomicity change close timing windows that cannot be reproduced deterministically in a unit test without driving private synchronization state, so they have no dedicated test. The completion tail is bound to the session id and participant list captured under cs_coinjoin, and the reset checks both the session id and nState, which is visible in the diff. They were verified by inspection and by the existing server_signfinaltx_* tests, which still pass. The charge reservation is in the same position: its window lies between two lock scopes inside one call. It was verified by checking that every reservation is released exactly once, after ConsumeCollateral() returns, on every path; that admission checks it under the same cs_coinjoin scope that admits; by -Wthread-safety -Werror; and by an independent code review.

Breaking Changes

None. No protocol, serialization or RPC change. Two debug log lines have vecSessionCollaterals.size(): renamed to participants:.

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 (for repository code-owners and collaborators only)

[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		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		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title clearly and concisely summarizes the main changes: fixing data races and check-then-act races in the CoinJoin server.
Description check		The description directly explains the CoinJoin race conditions addressed, the synchronization changes, testing, and scope of the pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[thepastaclaw]

✅ Final review complete — no blockers (commit a231f05) · triage: normal

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

[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)

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-13T21:01:21.257708Z	a231f05	New commits

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

The series correctly serializes pool decisions and binds completion work to session snapshots, but offender collateral consumption remains outside the session ownership boundary. After a timeout reset or a completion-triggered reset, a replacement session can admit the same collateral before the deferred consumption occurs, causing the wrong participant to be charged. The timeout regression test also still lacks an independent negative case for the side-coverage condition.

🔴 1 blocking | 🟡 1 suggestion(s)

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

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); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: 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: critical by gpt-6.1-sol (effort low) — The intricate concurrency changes in src/coinjoin/server.cpp directly alter signature application in ProcessDSSIGNFINALTX, collateral charging, and final transaction commitment, meeting the bar for changes to signatures and funds movement.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort xhigh); agent phase2-reviewer

🤖 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:448-449: Prevent deferred offender collateral consumption after session ownership changes
  The collateral is selected while holding `cs_coinjoin`, but it is consumed only after that lock is released. In `CreateFinalTransaction()`, `RelayFinalTransaction()` runs before this call and can invoke `RelayStatus()`; if all entry participants have disconnected, `RelayStatus()` calls `SetNull()`. `CheckTimeout()` has the same ordering more directly: it calls `SetNull()` at lines 707-708 and consumes the previously selected collateral at lines 711-712. Neither path prevents `ProcessDSACCEPT()` from creating a replacement session while `cs_check_pool` remains held, because admission does not acquire that guard and session collaterals are not pinned in the mempool. The replacement can therefore admit the same collateral before `ConsumeCollateral()` runs, violating the no-charge reset behavior and potentially spending the replacement participant's collateral. Preserve explicit reservation/ownership of the selected collateral until consumption completes, or use an equivalent atomic protocol; a simple post-reset session-ID check would avoid the wrong charge but silently lose the intended fee.

In `src/test/coinjoin_inouts_tests.cpp`:
- [SUGGESTION] src/test/coinjoin_inouts_tests.cpp:516-530: Cover non-actionable sessions in the timeout regression test
  (existing thread: https://github.com/dashpay/dash/pull/7537#discussion_r4210151998)
  `SeedTimedOutSession()` hardcodes `MixShape::STANDARD` for every seeded collateral and entry at lines 227-235. Consequently, the accepting rows either have `entries == collaterals`, which intentionally defers to `CheckPool()`, or have fewer than the minimum entries; none independently exercises the `GetMixSideCountsLocked().IsCovered()` conjunct at `CheckTimeout()` lines 699-701. Removing only that conjunct would leave all current rows passing. Add a negative accepting-session case with enough entries but an uncovered side and a missing participant—for example, three demotions and one promotion received with a second promoter still missing—and assert `POOL_STATE_IDLE`. This specifically protects the side-coverage reset decision while retaining the intentional full-entry deferral behavior.

[thepastaclaw]

✅ Resolved at cdb4951b; see the replies below.

🔴 Blocking: Prevent deferred offender collateral consumption after session ownership changes

The collateral is selected while holding cs_coinjoin, but it is consumed only after that lock is released. In CreateFinalTransaction(), RelayFinalTransaction() runs before this call and can invoke RelayStatus(); if all entry participants have disconnected, RelayStatus() calls SetNull(). CheckTimeout() has the same ordering more directly: it calls SetNull() at lines 707-708 and consumes the previously selected collateral at lines 711-712. Neither path prevents ProcessDSACCEPT() from creating a replacement session while cs_check_pool remains held, because admission does not acquire that guard and session collaterals are not pinned in the mempool. The replacement can therefore admit the same collateral before ConsumeCollateral() runs, violating the no-charge reset behavior and potentially spending the replacement participant's collateral. Preserve explicit reservation/ownership of the selected collateral until consumption completes, or use an equivalent atomic protocol; a simple post-reset session-ID check would avoid the wrong charge but silently lose the intended fee.

_{source: gpt-6.1-sol (phase2-reviewer: general, dash-core-commit-history)}

[PastaPastaPasta]

Valid, and this PR introduced it. On develop, ChargeFees() consumed the collateral before CheckTimeout()'s SetNull() and before finalization. This PR moved the selection under cs_coinjoin and the ATMP to after the lock is released, which opened the gap.

Impact: a collateral is only ever sent to the masternode, and only its owner can sign another tx that spends the same input. So a readmitted copy always belongs to the offender who was being charged, and no other participant could be charged through this path. What actually happened was that the charged participant could re-enter the next session with a collateral about to be spent.

Fixed in 0ba94a6 ("serialize timeout transitions") with an explicit reservation:

• SelectCollateralToCharge() records the chosen collateral in m_collaterals_being_charged. It is guarded by cs_coinjoin and survives SetNull().
• ConsumeReservedCollateral() releases the reservation only after ConsumeCollateral() returns.
• Under cs_coinjoin, CreateNewSession() and AddUserToExistingSession() refuse a dsa whose collateral spends any input of a reserved one (ERR_INVALID_COLLATERAL).

The fee is kept, and once it is consumed the mempool keeps the collateral out. ConsumeCollateralIfCurrentSession() (AddEntry's charge) has the same gap against a concurrent timeout, so it takes the same reservation.

There is no unit test. The window sits between two lock scopes inside one call, and the server's interface can't observe it without a test hook. I verified it by reviewing the lock scopes, with clang -Wthread-safety -Werror, and with an independent review.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[thepastaclaw]

Resolved (re-reviewed at cdb4951b): The selected collateral is now reserved under cs_coinjoin, admission rejects collaterals overlapping that reservation, and ConsumeReservedCollateral() releases it only after consumption completes. Because the reservation survives SetNull(), a replacement session cannot reuse the collateral during the deferred charging window.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b7893f5815

ℹ️ 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]

Test timeout transitions through the public surface

This helper constructs the whole timed-out session by assigning the protected session ID, state, timer, collateral map, and entry vector directly, so the regression can pass even if normal DSA admission or state transitions can never produce the seeded invariant. The repository requires unit tests to use standard fixtures and the public surface rather than injected internal state, and directs unobservable cases to an extracted testable function or functional test; restructure this setup instead of adding a state-injection seam.

AGENTS.md reference: AGENTS.md:L134-L134

Useful? React with 👍 / 👎.

[PastaPastaPasta]

Fixed in 562150b (folded into "prioritize progress over timeout reset"). The timeout decision is now a pure function, ShouldResetTimedOutSession(const CoinJoinSessionProgress&). CheckTimeout() fills the progress under cs_coinjoin and calls it. The test calls that function directly with one row per case, plus the negative rows: not timed out, queue not ready, too few entries, a side uncovered with a straggler, a signature missing. SeedTimedOutSession() is gone, and SetNull() is private again. Mutation-checked: removing the deferral, the signing-advance clause, either accepting-advance clause, or the side-coverage check each makes a row fail.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[chatgpt-codex-connector]

Preserve the reservation through in-flight admission

After the scheduler's CheckTimeout() resets a session, a new DSACCEPT can validate the same prevout in IsAcceptableDSA() before ConsumeCollateral() wins cs_main, then be delayed while the consume succeeds and this line erases the reservation. Its final admission check consequently sees neither a reservation nor the now-stale validation result and accepts a collateral whose input is already spent, stalling the replacement session; retain a consumed-prevout epoch/tombstone through in-flight admission or revalidate after observing reservation removal.

AGENTS.md reference: AGENTS.md:L247-L247

Useful? React with 👍 / 👎.

[PastaPastaPasta]

The race is real, but it gives nobody a new capability, so I'm not adding machinery for it. Only two parties can resubmit that prevout: its owner, or someone replaying a collateral transaction the owner already signed. Either way the input is spent by the time the admission lands. AddEntry()'s own IsCollateralValid() then rejects that participant's entry with ERR_INVALID_COLLATERAL and charges nothing, and charging it at the replacement session's timeout is a no-op because ATMP fails. The worst case is one dead slot that holds the session until timeout. Anyone can already cause that: the owner by double-spending its collateral right after admission, and a replayer by taking a slot with any unspent collateral it has observed. That existing replay is worse for the victim, since that collateral can actually be charged. Closing this window would take a consume-generation counter plus a revalidation under cs_coinjoin, for no change in what an attacker can do.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

The current head addresses the two previously reported issues: deferred collateral remains reserved through consumption, and timeout coverage now exercises both actionable and reset paths. One remaining admission race can still accept a collateral after its reservation is cleared but before an in-flight DSACCEPT commits, leaving a spent collateral occupying a replacement session slot.

🟡 1 suggestion(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: 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: normal by gpt-6.1-sol (effort low) — This is a large and intricate concurrency fix in CoinJoin server state management, but it does not itself change consensus, funds movement, cryptography, signatures or key handling, peer-facing deserialization, or storage migrations.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-reviewer

🤖 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`:
- [SUGGESTION] src/coinjoin/server.cpp:638: Preserve the reservation through in-flight admission
  `ConsumeReservedCollateral()` erases `m_collaterals_being_charged` immediately after consuming the collateral. However, `CreateNewSession()` and `AddUserToExistingSession()` perform `IsAcceptableDSA()` before taking their final `cs_coinjoin` admission lock, and only then check `SpendsCollateralBeingCharged()`. An in-flight DSACCEPT can therefore validate the collateral while it is still unspent, wait for `ConsumeReservedCollateral()` to consume and erase the reservation, and then pass the final reservation check. The replacement session accepts a collateral whose input is already spent; its later entry submission fails validation and leaves a dead participant slot until timeout. Preserve a consumed-prevout tombstone or generation through in-flight admission, and reject an admission whose validation predates the corresponding consumption.

[thepastaclaw]

⏭️ Deferred (maintainer decision; not blocking) at cdb4951b; see the replies below.

🟡 Suggestion: Preserve the reservation through in-flight admission

ConsumeReservedCollateral() erases m_collaterals_being_charged immediately after consuming the collateral. However, CreateNewSession() and AddUserToExistingSession() perform IsAcceptableDSA() before taking their final cs_coinjoin admission lock, and only then check SpendsCollateralBeingCharged(). An in-flight DSACCEPT can therefore validate the collateral while it is still unspent, wait for ConsumeReservedCollateral() to consume and erase the reservation, and then pass the final reservation check. The replacement session accepts a collateral whose input is already spent; its later entry submission fails validation and leaves a dead participant slot until timeout. Preserve a consumed-prevout tombstone or generation through in-flight admission, and reject an admission whose validation predates the corresponding consumption.

_{source: muse-spark-1.3-contributor (phase1-reviewer: general)}

[PastaPastaPasta]

Same race as the Codex thread on this line (#7537 (comment)). I'm deliberately not adding a tombstone or generation for it. The only way to win it is to resubmit that exact prevout, which takes either its owner or someone replaying the owner's already-signed collateral transaction. Either party can already leave a dead slot in a session today by spending or replaying a collateral right after admission. Once the input is spent, AddEntry() rejects that participant with ERR_INVALID_COLLATERAL, charges no one, and the slot clears at the session timeout. The reservation exists so a reset can't hand a collateral that's being charged to a new session as a valid one. That still holds; this window adds no new capability.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[thepastaclaw]

You're right that this does not introduce a new capability: the same stale-validation window already exists when a collateral is spent or replayed immediately after admission, and AddEntry() rejects the now-invalid collateral without charging it. The reservation still protects the reset-to-new-session interval until consumption completes; I accept the deliberate decision not to add a tombstone or generation here.

Noted as intentionally deferred; I will not press it further here.

[thepastaclaw]

Re-review after discussion — commit cdb4951

Standing review was COMMENTED; this re-review is APPROVE: no blocking findings remain.

Approved: the discussion withdrew or deferred every finding the full review of cdb4951b (run 4891) raised, and the approval rests on that review.

Same commit as the standing review; the inline threads above carry the per-finding outcome. This follow-up exists only to correct the verdict.

Review provenance

Source: conversation lane gpt-6.1-sol (agent: conversation); no reviewer or verifier lanes ran for this follow-up

• Verdict moved because the inline discussion withdrew or deferred findings of the standing review of this commit; the code was not re-reviewed

[PastaPastaPasta]

Fixed the linux64_tsan-build failure: clang's -Wmissing-designated-field-initializers rejected two timeout-table rows that leave out sides. CoinJoinSessionProgress::sides is now value-initialized (sides{}), like the struct's other members. The fix is folded into "prioritize progress over timeout reset" (now d1e23c7, head a231f05), and no other commit changed. Compiling the test with that warning as an error reproduces both CI errors on the previous header and is clean on the new one. The four coinjoin unit suites pass. #7568 is restacked on top with its commits unchanged.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

The complete six-commit CoinJoin server change was independently reviewed at the exact head. The session-state locking, timeout deferral, collateral reservation, session snapshots, and atomic signature handling are consistent with the stated race fixes; no new in-scope correctness or security findings remain. CI evidence shows formatting, merge, and main checks passing, while several build and lint jobs were still queued at the snapshot.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: 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: normal by gpt-6.1-sol (effort low) — This is a substantial concurrency and state-machine change in CoinJoin server logic, but the diff does not itself alter consensus rules, funds movement, cryptography, signatures or key handling, peer-facing deserialization, or storage migrations.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

[PastaPastaPasta]

The linux64_tsan-test failure at a231f05 looks unrelated to this PR.

• One test failed: p2p_blocksonly.py, on all 3 attempts. It waits for a peer with relay permission to relay a transaction to a second peer while the node is in -blocksonly mode, and the wait_until timed out after 240 s. The test exercises no CoinJoin code, and the log has no ThreadSanitizer reports.
• The same commits pass this job elsewhere: fix: don't charge CoinJoin participants for failures the masternode can't attribute to them #7568 contains this PR's six commits unchanged, and its linux64_tsan-test passed on head e328cd7.
• Other first-attempt failures passed on retry: feature_llmq_chainlocks, feature_dip4_coinbasemerkleroots, feature_llmq_singlenode and feature_timestampindex.

A re-run of the job should clear it. I don't have permission to trigger one.

---

🤖 Posted autonomously by Claude on behalf of pasta.