fix: charge collateral when a CoinJoin session aborts - #7568
fix: charge collateral when a CoinJoin session aborts#7568PastaPastaPasta wants to merge 12 commits into
Conversation
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
PastaPastaPasta
changed the title
|
⛔ Blockers found — Opus deferred (commit 3fc743f) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bc5dbd248
ℹ️ 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".
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckTimeout -- %s timed out -- resetting\n", | ||
| (nState == POOL_STATE_SIGNING) ? "Signing" : "Session"); | ||
| if (nState == POOL_STATE_ACCEPTING_ENTRIES || nState == POOL_STATE_SIGNING) { | ||
| collateral_to_charge = SelectCollateralToCharge(FeePolicy::GUARANTEED_ON_ABORT); |
There was a problem hiding this comment.
When a valid DSVIN arrives before the deadline but AddEntry() is still blocked in IsCollateralValid() or IsValidInOuts() outside cs_coinjoin, the scheduler can observe fewer than the minimum committed entries here and classify that participant as a missing submitter. With three reservations, two committed entries, and the third validation in flight, this path deterministically selects and consumes the third participant's collateral, resets the session, and then makes the already-received entry fail its session revalidation. Track in-flight submissions or serialize this cutoff with entry processing so an honest pre-deadline submission cannot receive the guaranteed abort penalty.
AGENTS.md reference: AGENTS.md:L173-L175
Useful? React with 👍 / 👎.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughCoinJoin session denomination is now atomic, and validation uses captured denomination values. Server collateral state is consolidated in a lock-protected container. Pool checks, message handling, entry admission, finalization, and timeout processing use session snapshots and lock coordination. Fee selection supports probabilistic and guaranteed-abort policies. Tests cover collateral selection, timeout ordering, in-flight messages, pool locking, session recovery, and concurrent denomination validation. Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to The PR adds collateral charging for non-cooperative aborted sessions while preserving free queue timeouts and expanding test coverage. No actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant CoinJoinClient
participant CCoinJoinServer
participant cs_coinjoin
participant SessionCollaterals
participant Mempool
CoinJoinClient->>CCoinJoinServer: submit entry or signing message
CCoinJoinServer->>cs_coinjoin: capture and validate session state
CCoinJoinServer->>SessionCollaterals: check or store collateral
CCoinJoinServer->>cs_coinjoin: finalize session or update state
CCoinJoinServer->>Mempool: process final transaction
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/coinjoin/coinjoin.h (1)
345-350: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the logging explanation.
tinyformatformatsstd::atomic<int>through its implicit conversion toint.LogPrint()accepts arguments byconstreference, which avoids copying the non-copyable atomic.WalletCJLogPrint()forwards toCWallet::WalletLogPrintf, whose parameters are passed by value, so.load()is required to pass anint.🤖 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/coinjoin.h` around lines 345 - 350, Update the comments above nSessionDenom to accurately explain that tinyformat uses the atomic’s implicit int conversion, LogPrint() accepts it by const reference without copying, and WalletCJLogPrint() forwards to CWallet::WalletLogPrintf with by-value parameters, requiring an explicit .load().
🤖 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.
Nitpick comments:
In `@src/coinjoin/coinjoin.h`:
- Around line 345-350: Update the comments above nSessionDenom to accurately
explain that tinyformat uses the atomic’s implicit int conversion, LogPrint()
accepts it by const reference without copying, and WalletCJLogPrint() forwards
to CWallet::WalletLogPrintf with by-value parameters, requiring an explicit
.load().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c98ea00-f5ed-46f1-b28d-035fcce2d4d4
📒 Files selected for processing (5)
src/coinjoin/client.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/server.cppsrc/coinjoin/server.hsrc/test/coinjoin_inouts_tests.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The guaranteed abort-fee policy can still consume collateral from honest submissions already being processed, and a separate TRY_LOCK interleaving can reset sessions that remain recoverable. The commit stack also contains four syntactically unbuildable intermediate commits, while the offender-deduplication behavior change is obscured by a refactor-only commit subject.
Source: reviewer backends: gpt-5.6-sol (general), gpt-5.6-sol (dash-core-commit-history); final verifier backend: gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is 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)
🔴 3 blocking | 🟡 1 suggestion(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:625-628: Exclude in-flight submissions from guaranteed timeout penalties
`SelectCollateralToCharge()` only recognizes entries already committed to `vecEntries`, but `AddEntry()` releases `cs_coinjoin` while running `IsCollateralValid()` and `IsValidInOuts()` at lines 759-794. A valid `DSVIN` whose processing began before the deadline can therefore still be validating when the scheduler observes the timeout. With three reservations, two committed entries, and the third entry in flight, the third participant is classified as the only missing submitter, selected with certainty, and charged after `SetNull()` makes its final session revalidation fail. Signing has the same gap while `DSSIGNFINALTX` is being decoded or between its per-input `AddScriptSig()` calls. Track in-flight messages for the current session or serialize the timeout cutoff with their complete processing before applying a guaranteed collateral penalty.
- [BLOCKING] src/coinjoin/server.cpp:610-628: Preserve recoverable finalization when the preceding pool check is skipped
`CheckTimeout()` assumes the immediately preceding `CheckPool()` handled every recoverable accepting-entry timeout, but `CheckPool()` uses a non-blocking `TRY_LOCK`. A message-handling thread can hold `cs_check_pool`, sample `HasTimedOut()` as false immediately before the deadline, and then release the mutex after the scheduler's `CheckPool()` has skipped it but before the scheduler calls `CheckTimeout()`. `CheckTimeout()` then acquires the mutex after the deadline and unconditionally resets the session. If the session has at least `GetMinPoolParticipants()` committed entries but fewer entries than reservations, it should enter `ChargeAndFinalize` and retain the probabilistic policy; this interleaving instead aborts it and applies `GUARANTEED_ON_ABORT`. Re-evaluate the full accepting-entry action after acquiring `cs_check_pool` rather than relying on a preceding check that may have been skipped or sampled an earlier time.
- [BLOCKING] src/coinjoin/server.cpp:634: Fold the stray-brace correction into its introducing commit
Commit `67c9647ed03` introduces an extra closing brace immediately after `CCoinJoinServer::CheckTimeout()`. The unmatched brace remains in `264ba3fdfa7`, `76e366e1ceb`, and `9e199087637`, and is only removed by the final commit `2bc5dbd248b`. Those four intermediate commits are syntactically unbuildable and unusable as `git bisect` points. Amend `67c9647ed03` to omit the extra brace, remove the corrective deletion from the final commit, and rebase the intervening commits so every permanent-history state builds independently.
- [SUGGESTION] src/coinjoin/server.cpp:503-509: Make offender deduplication explicit in the commit history
Commit `9e199087637` is titled `refactor: separate CoinJoin offender selection policy`, but it also changes observable behavior: signing offenders are added once per entry instead of once per unsigned input. This removes the extra random-selection weight previously assigned to participants with multiple unsigned inputs. Move the deduplication hunk into the behavioral fix commit or reword `9e199087637` and add a commit body explaining the weighting change so `git log` and `git blame` expose the functional change.
| if (nState == POOL_STATE_ACCEPTING_ENTRIES || nState == POOL_STATE_SIGNING) { | ||
| collateral_to_charge = SelectCollateralToCharge(FeePolicy::GUARANTEED_ON_ABORT); | ||
| } | ||
| SetNull(); |
There was a problem hiding this comment.
🔴 Blocking: Exclude in-flight submissions from guaranteed timeout penalties
SelectCollateralToCharge() only recognizes entries already committed to vecEntries, but AddEntry() releases cs_coinjoin while running IsCollateralValid() and IsValidInOuts() at lines 759-794. A valid DSVIN whose processing began before the deadline can therefore still be validating when the scheduler observes the timeout. With three reservations, two committed entries, and the third entry in flight, the third participant is classified as the only missing submitter, selected with certainty, and charged after SetNull() makes its final session revalidation fail. Signing has the same gap while DSSIGNFINALTX is being decoded or between its per-input AddScriptSig() calls. Track in-flight messages for the current session or serialize the timeout cutoff with their complete processing before applying a guaranteed collateral penalty.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Exclude in-flight submissions from guaranteed timeout penalties no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // CheckPool can be finalizing or committing on the message-handling thread. Skipping this tick | ||
| // keeps timeout reset and finalization/commit single-flight without blocking the scheduler. | ||
| TRY_LOCK(cs_check_pool, lock_check_pool); | ||
| if (!lock_check_pool) return; | ||
|
|
||
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckTimeout -- %s timed out -- resetting\n", | ||
| (nState == POOL_STATE_SIGNING) ? "Signing" : "Session"); | ||
| ChargeFees(); | ||
| WITH_LOCK(cs_coinjoin, SetNull()); | ||
| CTransactionRef collateral_to_charge; | ||
| { | ||
| LOCK(cs_coinjoin); | ||
|
|
||
| // Too early to do anything. Recheck while holding the lock so selecting an offender and | ||
| // closing the session form one atomic cutoff for late entries and signatures. | ||
| if (!CCoinJoinServer::HasTimedOut()) return; | ||
|
|
||
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckTimeout -- %s timed out -- resetting\n", | ||
| (nState == POOL_STATE_SIGNING) ? "Signing" : "Session"); | ||
| if (nState == POOL_STATE_ACCEPTING_ENTRIES || nState == POOL_STATE_SIGNING) { | ||
| collateral_to_charge = SelectCollateralToCharge(FeePolicy::GUARANTEED_ON_ABORT); | ||
| } | ||
| SetNull(); |
There was a problem hiding this comment.
🔴 Blocking: Preserve recoverable finalization when the preceding pool check is skipped
CheckTimeout() assumes the immediately preceding CheckPool() handled every recoverable accepting-entry timeout, but CheckPool() uses a non-blocking TRY_LOCK. A message-handling thread can hold cs_check_pool, sample HasTimedOut() as false immediately before the deadline, and then release the mutex after the scheduler's CheckPool() has skipped it but before the scheduler calls CheckTimeout(). CheckTimeout() then acquires the mutex after the deadline and unconditionally resets the session. If the session has at least GetMinPoolParticipants() committed entries but fewer entries than reservations, it should enter ChargeAndFinalize and retain the probabilistic policy; this interleaving instead aborts it and applies GUARANTEED_ON_ABORT. Re-evaluate the full accepting-entry action after acquiring cs_check_pool rather than relying on a preceding check that may have been skipped or sampled an earlier time.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Preserve recoverable finalization when the preceding pool check is skipped no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| if (collateral_to_charge) { | ||
| ConsumeCollateral(collateral_to_charge); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Fold the stray-brace correction into its introducing commit
Commit 67c9647ed03 introduces an extra closing brace immediately after CCoinJoinServer::CheckTimeout(). The unmatched brace remains in 264ba3fdfa7, 76e366e1ceb, and 9e199087637, and is only removed by the final commit 2bc5dbd248b. Those four intermediate commits are syntactically unbuildable and unusable as git bisect points. Amend 67c9647ed03 to omit the extra brace, remove the corrective deletion from the final commit, and rebase the intervening commits so every permanent-history state builds independently.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Fold the stray-brace correction into its introducing commit no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // who didn't sign? Include each participant once even if multiple inputs are unsigned. | ||
| for (const auto& entry : vecEntries) { | ||
| for (const auto& txdsin : entry.vecTxDSIn) { | ||
| if (!txdsin.fHasSig) { | ||
| LogPrint(BCLog::COINJOIN, /* Continued */ | ||
| "CCoinJoinServer::ChargeFees -- found uncooperative node (didn't sign), found offence\n"); | ||
| vecOffendersCollaterals.push_back(entry.txCollateral); | ||
| } | ||
| if (std::ranges::any_of(entry.vecTxDSIn, [](const auto& txdsin) { return !txdsin.fHasSig; })) { | ||
| LogPrint(BCLog::COINJOIN, /* Continued */ | ||
| "CCoinJoinServer::SelectCollateralToCharge -- found uncooperative node (didn't sign), " | ||
| "found offence\n"); | ||
| vecOffendersCollaterals.push_back(entry.txCollateral); |
There was a problem hiding this comment.
🟡 Suggestion: Make offender deduplication explicit in the commit history
Commit 9e199087637 is titled refactor: separate CoinJoin offender selection policy, but it also changes observable behavior: signing offenders are added once per entry instead of once per unsigned input. This removes the extra random-selection weight previously assigned to participants with multiple unsigned inputs. Move the deduplication hunk into the behavioral fix commit or reword 9e199087637 and add a commit body explaining the weighting change so git log and git blame expose the functional change.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Make offender deduplication explicit in the commit history no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
2bc5dbd to
3d5cf6b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d5cf6b6e0
ℹ️ 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".
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckTimeout -- %s timed out -- resetting\n", | ||
| (nState == POOL_STATE_SIGNING) ? "Signing" : "Session"); | ||
| if (nState == POOL_STATE_ACCEPTING_ENTRIES || nState == POOL_STATE_SIGNING) { | ||
| collateral_to_charge = SelectCollateralToCharge(FeePolicy::GUARANTEED_ON_ABORT); |
There was a problem hiding this comment.
When RelayFinalTransaction() encounters one disconnected entry, it stops relaying and calls RelayStatus(STATUS_REJECTED), which tells every connected client to abort, but the server remains in POOL_STATE_SIGNING unless all entries are disconnected. At the subsequent timeout, this guaranteed policy classifies unsigned entries—including connected peers after the failed relay that never received DSFINALTX—as offenders and consumes one at random. Thus a single disconnect can cost an honest participant its collateral; this coordinator-side relay-failure path should reset without charging, or only peers that were successfully sent the final transaction should be eligible.
AGENTS.md reference: AGENTS.md:L168-L168
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
All four prior verified findings are resolved at the current head, but two signing-timeout paths still block approval: guaranteed abort charging can penalize honest participants after the server tells them to stop, and a stale pool-check interleaving can discard a fully signed transaction. The stack also has two non-blocking history issues: the lint-only follow-up should be folded into its introducing commits, and the subtle timeout fixes should retain their rationale in commit bodies.
Source: reviewer backends: gpt-5.6-sol (general), gpt-5.6-sol (dash-core-commit-history); 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),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 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 `src/coinjoin/server.cpp`:
- [BLOCKING] src/coinjoin/server.cpp:681-684: Do not guarantee a fee after the coordinator aborts signing
The guaranteed timeout policy applies even after the server has instructed connected clients to stop signing. `RelayFinalTransaction()` calls `RelayStatus(STATUS_REJECTED)` when any entry is disconnected, and `ProcessDSSIGNFINALTX()` does the same after any `AddScriptSig()` failure. Connected clients process that rejection by entering `POOL_STATE_ERROR` and releasing their session resources, but the server remains in `POOL_STATE_SIGNING` unless every entry is disconnected. A malicious participant can exploit this by submitting its valid signature and then resubmitting it: the duplicate fails, every honest peer is told to abort, and the malicious participant is excluded from the unsigned-offender set. At timeout, this block then guarantees that one honest participant is charged. A failed `DSFINALTX` relay can similarly charge a connected entry that never received the transaction. Coordinator-originated signing aborts must reset without the guaranteed fee, or eligibility must be limited to participants that received the final transaction and were not subsequently instructed to abort.
- [BLOCKING] src/coinjoin/server.cpp:669-684: Commit complete signatures when rechecking a timeout
`CheckTimeout()` re-evaluates recoverable accepting-entry sessions but does not re-evaluate `IsSignaturesComplete()` for signing sessions. A scheduler `CheckPool()` can acquire `cs_check_pool`, observe the signatures as incomplete, and release `cs_coinjoin`. The final on-time `DSSIGNFINALTX` can then add its signature, skip its own `CheckPool()` because the scheduler still owns `cs_check_pool`, and clear its in-flight guard. When the scheduler subsequently enters `CheckTimeout()` after the deadline, there are no unsigned offenders, but this block still calls `SetNull()` and discards the fully signed transaction. Re-evaluate the signing action under `cs_coinjoin`, then call `CommitFinalTransaction()` after releasing that lock, just as the accepting-entry action is re-evaluated and finalized.
In `<commit:aaa6d04>`:
- [SUGGESTION] <commit:aaa6d04>:1: Fold the lint-only follow-up into its introducing commits
Commit `aaa6d0464a4` only adds `/* Continued */` markers to four `LogPrint` calls introduced earlier in this stack: one in `96cf3768cab` and three in `7443d022e09`. The linter rejects those unmarked calls, so retaining the separate correction leaves the introducing revisions as lint-failing bisect points and adds review-fix noise to permanent history. Fold each marker into the commit that introduced its call and drop `aaa6d0464a4`.
In `<commit:bb056c6>`:
- [SUGGESTION] <commit:bb056c6>:1: Preserve the rationale for the non-obvious CoinJoin fixes
Commits `bb056c6d397`, `7da61132813`, and `3d5cf6b6e0f` have empty bodies despite changing subtle concurrency and fee-policy invariants. In particular, they serialize timeout reset with finalization, bind entry validation to a session denomination snapshot, and combine guaranteed abort charging with in-flight-message deferral and recoverable-timeout re-evaluation. Add concise bodies explaining the race or policy invariant each commit preserves so ordinary `git log`, blame, and bisect retain the reasoning currently available only from source comments and PR discussion.
| if (nState == POOL_STATE_ACCEPTING_ENTRIES || nState == POOL_STATE_SIGNING) { | ||
| collateral_to_charge = SelectCollateralToCharge(FeePolicy::GUARANTEED_ON_ABORT); | ||
| } | ||
| SetNull(); |
There was a problem hiding this comment.
🔴 Blocking: Do not guarantee a fee after the coordinator aborts signing
The guaranteed timeout policy applies even after the server has instructed connected clients to stop signing. RelayFinalTransaction() calls RelayStatus(STATUS_REJECTED) when any entry is disconnected, and ProcessDSSIGNFINALTX() does the same after any AddScriptSig() failure. Connected clients process that rejection by entering POOL_STATE_ERROR and releasing their session resources, but the server remains in POOL_STATE_SIGNING unless every entry is disconnected. A malicious participant can exploit this by submitting its valid signature and then resubmitting it: the duplicate fails, every honest peer is told to abort, and the malicious participant is excluded from the unsigned-offender set. At timeout, this block then guarantees that one honest participant is charged. A failed DSFINALTX relay can similarly charge a connected entry that never received the transaction. Coordinator-originated signing aborts must reset without the guaranteed fee, or eligibility must be limited to participants that received the final transaction and were not subsequently instructed to abort.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Do not guarantee a fee after the coordinator aborts signing no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| if (nState == POOL_STATE_ACCEPTING_ENTRIES) { | ||
| const int entries{GetEntriesCountLocked()}; | ||
| if ((!m_session_collaterals.empty() && size_t(entries) == m_session_collaterals.size()) || | ||
| entries >= CoinJoin::GetMinPoolParticipants()) { | ||
| session_to_finalize = nSessionID; | ||
| charge_fees = size_t(entries) != m_session_collaterals.size(); | ||
| } | ||
| } | ||
|
|
||
| if (session_to_finalize == 0) { | ||
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckTimeout -- %s timed out -- resetting\n", | ||
| (nState == POOL_STATE_SIGNING) ? "Signing" : "Session"); | ||
| if (nState == POOL_STATE_ACCEPTING_ENTRIES || nState == POOL_STATE_SIGNING) { | ||
| collateral_to_charge = SelectCollateralToCharge(FeePolicy::GUARANTEED_ON_ABORT); | ||
| } | ||
| SetNull(); |
There was a problem hiding this comment.
🔴 Blocking: Commit complete signatures when rechecking a timeout
CheckTimeout() re-evaluates recoverable accepting-entry sessions but does not re-evaluate IsSignaturesComplete() for signing sessions. A scheduler CheckPool() can acquire cs_check_pool, observe the signatures as incomplete, and release cs_coinjoin. The final on-time DSSIGNFINALTX can then add its signature, skip its own CheckPool() because the scheduler still owns cs_check_pool, and clear its in-flight guard. When the scheduler subsequently enters CheckTimeout() after the deadline, there are no unsigned offenders, but this block still calls SetNull() and discards the fully signed transaction. Re-evaluate the signing action under cs_coinjoin, then call CommitFinalTransaction() after releasing that lock, just as the accepting-entry action is re-evaluated and finalized.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Commit complete signatures when rechecking a timeout no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
3d5cf6b to
9777136
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/test/coinjoin_inouts_tests.cpp (4)
750-781: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the
size_tvalues passed toMakeCollateralandMakePeer.
MakeCollateral(uint32_t)receives thesize_tloop variableiat Line 762, andMakePeer(NodeId, uint32_t)receives0x0a000001 + iat Line 766. Both conversions are implicit narrowing. Add explicit casts so the intent is clear and no compiler in CI reports a conversion warning.♻️ Proposed change
- const auto collateral = MakeCollateral(i); + const auto collateral = MakeCollateral(static_cast<uint32_t>(i)); @@ - auto peer = MakePeer(i, 0x0a000001 + i); + auto peer = MakePeer(static_cast<NodeId>(i), static_cast<uint32_t>(0x0a000001 + i));🤖 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/test/coinjoin_inouts_tests.cpp` around lines 750 - 781, In server_timeout_rechecks_recoverable_session, explicitly narrow the size_t loop variable when passing it to MakeCollateral(uint32_t), and explicitly cast the computed address value passed to MakePeer(NodeId, uint32_t). Preserve the existing loop behavior while eliminating implicit conversion warnings.
188-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative lock annotations to the new helpers that take
cs_coinjoin.
SeedParticipantat Line 246 declaresEXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin), andRelayAbortForTestat Line 240 follows that pattern. The other new helpers (ResetForTest,SetTimedOutForTest,AddCollateralForTest,AddEntryForTest,SelectForTest,EnterSigningState,SetFinalTransactionForTest,SeedTimedOutSession,MarkMessageInFlightForTest) also acquirecs_coinjoinbut declare no negative capability. Clang thread-safety analysis then cannot detect a caller that already holdscs_coinjoin.♻️ Example annotation for the affected helpers
- void ResetForTest(PoolState state) + void ResetForTest(PoolState state) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin) { LOCK(cs_coinjoin);Also applies to: 253-275
🤖 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/test/coinjoin_inouts_tests.cpp` around lines 188 - 244, Add EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin) annotations to ResetForTest, SetTimedOutForTest, AddCollateralForTest, AddEntryForTest, SelectForTest, EnterSigningState, SetFinalTransactionForTest, SeedTimedOutSession, and MarkMessageInFlightForTest, matching the existing annotations on SeedParticipant and RelayAbortForTest. Keep each helper’s internal cs_coinjoin locking unchanged.
321-424: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting this test case into separate scenarios.
server_abort_fee_selects_unique_offendersverifies nine distinct behaviours in one test case: queue timeout, all-missing submitters, one submitter, partial submitters, all submitters, a lone non-signer, weighting across multiple non-signers, the probabilistic gate, andChargeRandomFees. A failure in an early scenario stops the later ones, and the failure output does not name the scenario. SeparateBOOST_AUTO_TEST_CASEblocks per policy scenario would isolate failures. The server construction can move into a small helper to avoid repetition.🤖 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/test/coinjoin_inouts_tests.cpp` around lines 321 - 424, Split server_abort_fee_selects_unique_offenders into separate BOOST_AUTO_TEST_CASE blocks covering each policy scenario, including timeout handling, submission coverage, non-signer selection and weighting, the probabilistic gate, and ChargeRandomFees. Move shared server setup into a small helper to avoid repetition, and retain each scenario’s existing assertions and setup behavior.
609-624: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer an exact integer constant for the expected mempool delta.
static_cast<CAmount>(0.1 * COIN)uses floating point for a consensus-adjacent amount comparison. UseCOIN / 10to keep the expectation exact and independent of double rounding.♻️ Proposed change
- BOOST_CHECK_EQUAL(delta, static_cast<CAmount>(0.1 * COIN)); + BOOST_CHECK_EQUAL(delta, COIN / 10);🤖 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/test/coinjoin_inouts_tests.cpp` around lines 609 - 624, Replace the floating-point expected amount in the delta assertion after ApplyDelta with the exact integer expression COIN / 10, while preserving the existing CAmount comparison and test behavior.
🤖 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.
Nitpick comments:
In `@src/test/coinjoin_inouts_tests.cpp`:
- Around line 750-781: In server_timeout_rechecks_recoverable_session,
explicitly narrow the size_t loop variable when passing it to
MakeCollateral(uint32_t), and explicitly cast the computed address value passed
to MakePeer(NodeId, uint32_t). Preserve the existing loop behavior while
eliminating implicit conversion warnings.
- Around line 188-244: Add EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin) annotations to
ResetForTest, SetTimedOutForTest, AddCollateralForTest, AddEntryForTest,
SelectForTest, EnterSigningState, SetFinalTransactionForTest,
SeedTimedOutSession, and MarkMessageInFlightForTest, matching the existing
annotations on SeedParticipant and RelayAbortForTest. Keep each helper’s
internal cs_coinjoin locking unchanged.
- Around line 321-424: Split server_abort_fee_selects_unique_offenders into
separate BOOST_AUTO_TEST_CASE blocks covering each policy scenario, including
timeout handling, submission coverage, non-signer selection and weighting, the
probabilistic gate, and ChargeRandomFees. Move shared server setup into a small
helper to avoid repetition, and retain each scenario’s existing assertions and
setup behavior.
- Around line 609-624: Replace the floating-point expected amount in the delta
assertion after ApplyDelta with the exact integer expression COIN / 10, while
preserving the existing CAmount comparison and test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ec59951d-1248-4c0c-a6ba-d5f91c7b66eb
📒 Files selected for processing (3)
src/coinjoin/server.cppsrc/coinjoin/server.hsrc/test/coinjoin_inouts_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/coinjoin/server.h
- src/coinjoin/server.cpp
|
@PastaPastaPasta this could result in CoinJoin participants being penalized for masternode connectivity issues, right? |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact-head Codex revalidation found no actionable defects: the two signing-timeout blockers are fixed, and the rewritten commit stack resolves both history suggestions. All four prior verified findings are resolved at 9777136, so this preliminary gate approves the PR for the next review phase.
Source: reviewer backends: gpt-5.6-sol (general), gpt-5.6-sol (dash-core-commit-history); final verifier backend: 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)
9777136 to
4618c53
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4618c53e4b
ℹ️ 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".
| SetNull(); | ||
| } | ||
|
|
||
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckTimeout -- %s timed out -- resetting\n", | ||
| (nState == POOL_STATE_SIGNING) ? "Signing" : "Session"); | ||
| ChargeFees(); | ||
| WITH_LOCK(cs_coinjoin, SetNull()); | ||
| if (collateral_to_charge) { | ||
| ConsumeCollateral(collateral_to_charge); |
There was a problem hiding this comment.
In the scheduler's CheckTimeout() path, SetNull() makes the server accept a replacement session before ConsumeCollateral() has acquired cs_main. A peer can therefore resubmit the selected collateral during this window: CreateNewSession() may test-accept and record it first, after which the old timeout broadcasts the collateral and spends its inputs. The replacement session is then built around an invalid collateral, so that participant cannot submit its entry and the new mix may time out or exclude it. Keep admission closed until consumption finishes, or track pending penalty collateral so it cannot be admitted.
AGENTS.md reference: AGENTS.md:L178-L180
Useful? React with 👍 / 👎.
4618c53 to
a67fc15
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/coinjoin/server.cpp (1)
536-612: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a unit test for the guaranteed-abort path after a relayed abort.
CheckTimeout()skips collateral selection whenm_relayed_abortis set.RelayStatus(STATUS_REJECTED)and the disconnect path both set that flag. The test helpers shown insrc/test/coinjoin_inouts_tests.cppseed timed-out sessions and relay rejections, so a case that relays a rejection and then asserts that no collateral is consumed is cheap to add.As per coding guidelines: "Add small tests proving invariants when changing consensus, serialization, masternode, LLMQ, InstantSend, ChainLocks, governance, EvoDB, credit-pool, Platform, BLS, quorum prediction, or timing/shutdown behavior."
Also applies to: 666-708
🤖 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 536 - 612, Add a unit test in the existing coinjoin input/output test helpers covering a timed-out session that receives RelayStatus(STATUS_REJECTED), then verify CheckTimeout() takes the guaranteed-abort path without consuming or selecting collateral. Reuse the established session-seeding and relay-rejection setup, and assert the collateral state remains unchanged after the timeout check.Source: Coding guidelines
src/test/coinjoin_inouts_tests.cpp (4)
180-199: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConfirm that
consumed_collateralsis never written from two threads.
consumed_collateralsis a plainmutable std::vectorwith no synchronization. Inserver_timeout_does_not_reset_during_pool_check, a second thread runsHoldPoolCheck, which only takescs_check_pooland does not consume collateral, so the current tests appear safe. If a future test lets a helper thread reachConsumeCollateral, the vector will race. Guarding it withcs_coinjoinor a dedicated mutex would make the invariant explicit.Also applies to: 220-251, 260-345, 819-841
🤖 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/test/coinjoin_inouts_tests.cpp` around lines 180 - 199, The test-only TestableCoinJoinServer::consumed_collaterals vector is unsynchronized and could race if ConsumeCollateral runs on another thread. Protect accesses in ConsumeCollateral, ResetForTest, and all relevant test assertions with cs_coinjoin or a dedicated mutex, preserving the existing lifecycle-test behavior.
355-364: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the
unsigned_inputssemantics inMakeEntry.
unsigned_inputs == 0produces one signed input, andunsigned_inputs == NproducesNunsigned inputs. The parameter therefore controls both the input count and the signature flag. A short comment or a distinct parameter name would make the call sites at Lines 429, 438, and 539 easier to read.🤖 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/test/coinjoin_inouts_tests.cpp` around lines 355 - 364, Clarify the dual semantics of unsigned_inputs in MakeEntry: zero creates one signed input, while a positive value creates that many unsigned inputs. Add a concise comment or rename the parameter to explicitly communicate this behavior, keeping the existing input-count and fHasSig logic unchanged.
366-486: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated server construction into a fixture helper.
Eight test cases repeat the same
CActiveMasternodeManagerplusTestableCoinJoinServerconstruction with ten identical arguments. A helper that returns the configured server would remove the duplication and keep future constructor changes to one site.Also applies to: 488-524, 526-555, 654-677, 679-718, 720-775, 777-817, 819-841, 843-863, 865-885
🤖 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/test/coinjoin_inouts_tests.cpp` around lines 366 - 486, Extract the repeated CActiveMasternodeManager and TestableCoinJoinServer setup from the affected test cases into a shared fixture helper that returns a configured TestableCoinJoinServer. Update each listed test, including server_abort_fee_selects_unique_offenders, to use the helper while preserving the existing node dependencies and test behavior; keep constructor arguments centralized in that helper.
709-716: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the duplicated prioritisation amount with the production constant.
The test hardcodes
static_cast<CAmount>(0.1 * COIN). The production code inCommitFinalTransactionapplies the same value. If the production value changes, this assertion silently encodes the old policy. Reference the shared constant instead.🔍 Verification script
#!/bin/bash # Locate the prioritisation delta used by CommitFinalTransaction. rg -nP -C4 'PrioritiseTransaction|0\.1\s*\*\s*COIN' src/coinjoin🤖 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/test/coinjoin_inouts_tests.cpp` around lines 709 - 716, Update the delta assertion in the coinjoin test to use the shared production prioritisation constant applied by CommitFinalTransaction instead of hardcoding static_cast<CAmount>(0.1 * COIN). Keep the existing delta calculation and validation unchanged.
🤖 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 348-404: Update CCoinJoinServer::CheckPool’s locked snapshot so
the timeout-driven ChargeAndFinalize action is selected only when
m_inflight_session does not equal the current nSessionID. Preserve normal
finalization when all entries are present, but defer charging/finalizing while
the current session has an in-flight submission, matching the guard used by
CheckTimeout().
- Around line 578-587: Update the probabilistic collateral check using
nSessionCollaterals and vecOffendersCollaterals so it cannot evaluate
nSessionCollaterals - 1 when the size is zero. Add an explicit nonzero guard or
use a signed comparison, while preserving the intended “mostly offending”
behavior when entries exist without collaterals.
---
Nitpick comments:
In `@src/coinjoin/server.cpp`:
- Around line 536-612: Add a unit test in the existing coinjoin input/output
test helpers covering a timed-out session that receives
RelayStatus(STATUS_REJECTED), then verify CheckTimeout() takes the
guaranteed-abort path without consuming or selecting collateral. Reuse the
established session-seeding and relay-rejection setup, and assert the collateral
state remains unchanged after the timeout check.
In `@src/test/coinjoin_inouts_tests.cpp`:
- Around line 180-199: The test-only
TestableCoinJoinServer::consumed_collaterals vector is unsynchronized and could
race if ConsumeCollateral runs on another thread. Protect accesses in
ConsumeCollateral, ResetForTest, and all relevant test assertions with
cs_coinjoin or a dedicated mutex, preserving the existing lifecycle-test
behavior.
- Around line 355-364: Clarify the dual semantics of unsigned_inputs in
MakeEntry: zero creates one signed input, while a positive value creates that
many unsigned inputs. Add a concise comment or rename the parameter to
explicitly communicate this behavior, keeping the existing input-count and
fHasSig logic unchanged.
- Around line 366-486: Extract the repeated CActiveMasternodeManager and
TestableCoinJoinServer setup from the affected test cases into a shared fixture
helper that returns a configured TestableCoinJoinServer. Update each listed
test, including server_abort_fee_selects_unique_offenders, to use the helper
while preserving the existing node dependencies and test behavior; keep
constructor arguments centralized in that helper.
- Around line 709-716: Update the delta assertion in the coinjoin test to use
the shared production prioritisation constant applied by CommitFinalTransaction
instead of hardcoding static_cast<CAmount>(0.1 * COIN). Keep the existing delta
calculation and validation unchanged.
🪄 Autofix
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: c9e11abb-3443-427e-ac0d-4a4598969547
📒 Files selected for processing (5)
src/coinjoin/coinjoin.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/server.cppsrc/coinjoin/server.hsrc/test/coinjoin_inouts_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/coinjoin/coinjoin.h
- src/coinjoin/server.h
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The timeout handling still has two in-scope concurrency defects: an on-time in-flight entry can be excluded and probabilistically charged during recoverable finalization, and a selected timeout collateral can be admitted into a replacement session before the penalty transaction consumes it. The unsigned-underflow report is not actionable because the locked production invariants ensure a nonempty offender set always belongs to a session with at least one collateral.
Source: reviewer backend model gpt-5.6-sol (general); 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)
🔴 2 blocking
🤖 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:382-385: Defer timeout finalization while an entry is in flight
`ProcessDSVIN()` marks an on-time entry in flight before deserialization and validation, but this timeout-driven `ChargeAndFinalize` branch does not inspect that marker. If the existing committed entries already meet the minimum, the scheduler can select the still-validating participant as a missing submitter, potentially charge its collateral, and move the session to signing. The pending `AddEntry()` then fails its session-state revalidation, excluding an entry that arrived before the deadline. Require `m_inflight_session != nSessionID` for this timeout branch, matching `CheckTimeout()`; normal finalization when every reservation already has an entry should remain unchanged.
- [BLOCKING] src/coinjoin/server.cpp:703-707: Keep new-session admission closed until the penalty is consumed
`SetNull()` clears the timed-out session and makes the server idle before `ConsumeCollateral()` acquires `cs_main`. During that interval, the message-handling thread can test-accept the selected collateral in `IsAcceptableDSA()` and record it in a replacement session through `CreateNewSession()`. The timeout thread can then consume and relay that collateral, leaving the replacement session with a reservation whose inputs are spent; its participant cannot submit a valid entry, and the new mix can stall or abort. Keep admission closed until consumption finishes, or maintain a pending-penalty prevout set that both new- and existing-session admission reject until the penalty attempt completes.
| } else if (CCoinJoinServer::HasTimedOut() && entries >= CoinJoin::GetMinPoolParticipants()) { | ||
| // We timed out while accepting entries but still have more than the minimum, so | ||
| // punish the misbehaving participants and complete the session without them | ||
| action = Action::ChargeAndFinalize; |
There was a problem hiding this comment.
🔴 Blocking: Defer timeout finalization while an entry is in flight
ProcessDSVIN() marks an on-time entry in flight before deserialization and validation, but this timeout-driven ChargeAndFinalize branch does not inspect that marker. If the existing committed entries already meet the minimum, the scheduler can select the still-validating participant as a missing submitter, potentially charge its collateral, and move the session to signing. The pending AddEntry() then fails its session-state revalidation, excluding an entry that arrived before the deadline. Require m_inflight_session != nSessionID for this timeout branch, matching CheckTimeout(); normal finalization when every reservation already has an entry should remain unchanged.
| } else if (CCoinJoinServer::HasTimedOut() && entries >= CoinJoin::GetMinPoolParticipants()) { | |
| // We timed out while accepting entries but still have more than the minimum, so | |
| // punish the misbehaving participants and complete the session without them | |
| action = Action::ChargeAndFinalize; | |
| } else if (CCoinJoinServer::HasTimedOut() && entries >= CoinJoin::GetMinPoolParticipants() && | |
| m_inflight_session != nSessionID) { |
source: ['coderabbit']
There was a problem hiding this comment.
Resolved in this update — Defer timeout finalization while an entry is in flight no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| SetNull(); | ||
| } | ||
|
|
||
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckTimeout -- %s timed out -- resetting\n", | ||
| (nState == POOL_STATE_SIGNING) ? "Signing" : "Session"); | ||
| ChargeFees(); | ||
| WITH_LOCK(cs_coinjoin, SetNull()); | ||
| if (collateral_to_charge) { | ||
| ConsumeCollateral(collateral_to_charge); |
There was a problem hiding this comment.
🔴 Blocking: Keep new-session admission closed until the penalty is consumed
SetNull() clears the timed-out session and makes the server idle before ConsumeCollateral() acquires cs_main. During that interval, the message-handling thread can test-accept the selected collateral in IsAcceptableDSA() and record it in a replacement session through CreateNewSession(). The timeout thread can then consume and relay that collateral, leaving the replacement session with a reservation whose inputs are spent; its participant cannot submit a valid entry, and the new mix can stall or abort. Keep admission closed until consumption finishes, or maintain a pending-penalty prevout set that both new- and existing-session admission reject until the penalty attempt completes.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Keep new-session admission closed until the penalty is consumed no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
a67fc15 to
4556612
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The in-flight timeout-finalization defect is fixed at the current head: CheckPool() now defers ChargeAndFinalize while the current session has a message in flight. One blocking race remains because CheckTimeout() exposes the idle state before consuming the selected penalty collateral, allowing that collateral to be reserved by a replacement session before its inputs are spent.
Source: reviewer backend model gpt-5.6-sol (general); 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 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 `src/coinjoin/server.cpp`:
- [BLOCKING] src/coinjoin/server.cpp:706-710: Keep new-session admission closed until the penalty is consumed
(existing thread: https://github.com/dashpay/dash/pull/7568#discussion_r3770096427)
`SetNull()` clears the timed-out session and exposes `POOL_STATE_IDLE` before `ConsumeCollateral()` acquires `cs_main`. A concurrent `DSACCEPT` can therefore pass `IsAcceptableDSA()` while the selected collateral is still unspent and commit it to a replacement session through `CreateNewSession()`; the timeout thread can then consume that collateral, leaving the replacement session with a reservation whose inputs are already spent. The participant cannot subsequently submit a valid entry, so the replacement mix can stall or abort. Keep admission closed until the consumption attempt completes without introducing a `cs_coinjoin` to `cs_main` lock inversion, or retain the selected collateral's prevouts in a pending-penalty set that both new- and existing-session admission reject until consumption finishes.
4556612 to
118d995
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 118d995b42
ℹ️ 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".
| // the collateral cannot be re-committed while its penalty spend is in flight. | ||
| MarkPendingCharge(collateral_to_charge); | ||
| } | ||
| RelayStatus(STATUS_REJECTED); |
There was a problem hiding this comment.
When an extra or duplicate DSSIGNFINALTX passes the initial state check after all required signatures are already stored, the scheduler's CheckPool() can concurrently commit and reset that session because its commit path does not defer for m_inflight_session. AddScriptSig() then fails against the cleared entries, and this unconditional call sets m_relayed_abort while the server is idle; CreateNewSession() does not clear that flag, so the next aborted session skips its guaranteed collateral charge. Recheck that nSessionID == *session_id and the state is still POOL_STATE_SIGNING before charging or relaying the abort.
AGENTS.md reference: AGENTS.md:L178-L180
Useful? React with 👍 / 👎.
nSessionDenom was the one CCoinJoinBaseSession field that was neither atomic nor guarded, while its siblings nState, nSessionID and nTimeLastSuccessfulStep are all std::atomic. On the server it is written by the message-handling thread in CreateNewSession() and by the scheduler thread in SetNull(), and read without any lock by CheckForCompleteQueue(), AddUserToExistingSession(), IsValidInOuts(), the relay logging, and by RPC threads via GetJsonInfo(). Concurrent unsynchronized access to a plain int is a data race: benign on the hardware we support, but formally UB and reportable by TSan.
CheckPool() and CheckForCompleteQueue() read nState, the entry count and the collateral count under separate lock acquisitions (or none at all) and then acted on the result, so a scheduler-thread SetNull() could land between the samples. CheckPool() is the worst case: it sampled nState, then took and released cs_coinjoin for GetEntriesCount(), then read vecSessionCollaterals.size() unlocked. A SetNull() in between made an already-reset session read as '0 entries == 0 collaterals' and get finalized, putting a dead session back into POOL_STATE_SIGNING and rejecting every new dsa until the 15s signing timeout expired. It now decides from one snapshot and acts afterwards, and CreateFinalTransaction()/CommitFinalTransaction() revalidate nSessionID because the decision is made with the lock released. CheckPool() also runs on both the scheduler thread and the message-handling thread, so two concurrent calls could both finalize: clients would receive DSFINALTX twice, sign twice, and the duplicate signatures make AddScriptSig() fail and abort the session for everyone. A TRY_LOCK-only cs_check_pool makes it single-shot without ever blocking msghand. SetState() and IsSessionReady() now require cs_coinjoin, so a transition and the session data it describes can only be observed together; this is what makes the existing revalidation blocks in CreateNewSession()/AddUserToExistingSession() effective. CheckForCompleteQueue() performs its transition under the lock and moves BLS signing and dsq relay outside it. ChargeFees() samples nState once instead of three times, which previously let it select 'didn't send' offenders and then charge and log them as 'didn't sign'.
vecSessionCollaterals had no GUARDED_BY and was reached from both threads with no lock at all: the message-handling thread read it in ProcessDSACCEPT(), IsSessionReady() and AddEntry(), while the scheduler thread read it in CheckPool(), CheckForCompleteQueue(), ChargeFees() and ChargeRandomFees(). The only synchronized access was the clear() in SetNull(). Committing a collateral therefore raced every one of those reads. The worst of them was ChargeRandomFees(), which iterated the vector by reference while calling ConsumeCollateral() - a cs_main mempool submission - for each element. A concurrent SetNull() destroys the CTransactionRefs the loop is walking, so this was a use-after-free and not just a torn size read. It now works from a copy taken under the lock, which also keeps cs_coinjoin from being held across cs_main. The transactions and their prevout index are now a single SessionCollaterals member so they cannot drift apart, and GUARDED_BY on that member makes every access - including the calls on it - checked by -Wthread-safety. Reintroducing an unlocked read is now a compile error rather than a review finding.
AddEntry() checked its bound, then ran IsCollateralValid() and IsValidInOuts() - both of which take cs_main and can block behind block validation - and only then took cs_coinjoin again to push_back. A scheduler-thread CheckTimeout() in that window calls SetNull(), so the entry was committed to a session that no longer existed. The consequence outlives the window: vecEntries keeps the orphaned entry while vecSessionCollaterals is empty, so the next session starts one entry ahead of its own participant count. CheckPool()'s entries == collaterals test then fires early and finalizes a transaction containing an input from the dead session, which nobody present will sign, stalling the new session to its signing timeout and charging its honest participants in ChargeFees(). The bound check and the push_back now share one lock scope, and the session identity captured before validation is rechecked inside it, so an entry can only ever be committed to the session it was validated for.
CheckTimeout() reset the pool while CheckPool() could be finalizing or committing the very same session on the message-handling thread: HasTimedOut() was tested without any lock and the reset could land between CheckPool()'s decision and its execution, clearing a live session mid-step. CheckTimeout() now takes the same cs_check_pool guard as CheckPool() - with TRY_LOCK, so a contended scheduler tick is skipped instead of blocking the message-handling thread - which makes timeout resets and finalize/commit single-flight. Offender selection also moves under cs_coinjoin, into SelectCollateralToCharge(), and into the same lock scope that closes the corresponding admission path: CheckTimeout() selects and resets atomically, and CreateFinalTransaction() selects and transitions to SIGNING atomically, so an entry that crossed the cutoff on time can no longer be charged as missing. The collateral is consumed only after the lock is released, because ConsumeCollateral() takes cs_main and mempool submission must not run under cs_coinjoin.
AddEntry() validated a submission against the live nSessionDenom while holding no session lock: IsValidInOuts() runs long cs_main work, and a scheduler-thread SetNull() in that window zeroes the denomination, so every output of an honest, on-time entry compared unequal to denom 0 and the ERR_DENOM path consumed that participant's collateral for a reset it could not have known about. IsValidInOuts() now takes the denomination as a parameter and AddEntry() passes the snapshot captured under cs_coinjoin alongside the session id it already revalidates before committing, so validation, punishment and commit are all bound to the same session. AddEntry() also rejects submissions up front when the pool is no longer accepting entries, instead of relying on the entries-full bound alone.
The scheduler calls CheckPool() and CheckTimeout() separately, leaving a gap in which the final collateral, entry, or signature can arrive after CheckPool() takes its snapshot. The message thread then skips its own CheckPool() while the scheduler holds cs_check_pool, and an unconditional timeout reset would discard a session that can now advance. Recheck readiness, finalizability, and signature completeness under cs_coinjoin before resetting. Actionable work takes priority and is picked up by the next scheduler tick.
Completion relay previously read and mutated live session state after CommitFinalTransaction() released cs_coinjoin. If all old participants disconnected, RelayCompletedTransaction() could reset the session, allowing a replacement session to open before random charging and the unconditional tail reset; those operations could then charge or clear the replacement. Capture the committed transaction, participants, and collaterals under cs_coinjoin. Relay and charge only those snapshots, keep completion notification side-effect-free, and reset only the matching signing session. The invalid-transaction path now also notifies captured participants before reset.
Split offender discovery from fee selection so callers can preserve the existing probabilistic policy or request guaranteed charging when a session aborts. Count each signing participant once even if multiple inputs remain unsigned. The previous per-input list gave participants with multiple inputs extra random-selection weight.
A timed-out session was abandoned with only the probabilistic charge, which also skips charging entirely when every participant offended. An attacker who reserved all the slots of a session - or was its sole non-cooperator - could abort session after session at little to no expected cost. Timeout aborts now use FeePolicy::GUARANTEED_ON_ABORT, which always charges exactly one collateral from the offender set; the finalize-with-stragglers path keeps the historical probabilistic policy. Guaranteeing the charge makes an existing race punitive, so it is closed: a DSVIN or DSSIGNFINALTX that passed its state check before the deadline can still be validating - including long cs_main work - when the scheduler crosses the timeout, and its sender would be charged as a no-show for a submission the server was actively processing. Such messages now mark themselves in flight under cs_coinjoin at the timeout cutoff, and CheckTimeout() defers while one is pending; InFlightMessageGuard clears the mark on every exit path. The can-advance recheck already keeps a finalizable or committable session out of this reset path, so by the time the charge runs the session has conclusively failed, and the charge is restricted to the states with an identifiable offender set: accepting entries or signing. Selecting the offender and resetting happen under cs_coinjoin, but the mempool submission that actually spends the collateral cannot run under it: in that window admission is already open again, and the still-unspent collateral would be accepted into a replacement session that the charge then breaks - its reservation can never be backed by a valid entry, and re-charging it later fails because the funds are gone. A selected collateral therefore reserves its prevouts in m_pending_charges before the reset, admission rejects any collateral spending a reserved prevout, and the reservation is released once the submission has settled and the mempool's own view takes over.
… session The scheduler's CheckPool() can sample the signatures as incomplete while holding cs_check_pool, the DSSIGNFINALTX that completes them in that window skips its own CheckPool() round because the guard is busy, and the same scheduler pass then enters CheckTimeout() past the deadline with the in-flight mark already cleared. Before the can-advance recheck this reset threw the fully signed transaction away, and with every input signed there was not even an offender to charge for the abort. Pin the required behavior down: CheckTimeout() defers - charging nobody and destroying nothing - and the next CheckPool() round commits the transaction. The commit attempt is made observable through the mempool prioritisation CommitFinalTransaction() applies before submitting.
…hans The guaranteed timeout charge assumed a session that dies in SIGNING dies because someone withheld a signature. It can also die because this coordinator told every participant to stand down: a session-wide STATUS_REJECTED - relayed when a signature submission fails validation, or when the final transaction cannot be delivered to a participant - puts honest clients into POOL_STATE_ERROR, where they release their inputs and never sign. At the timeout that followed, exactly those obedient clients formed the didn't-sign offender set and one of them was guaranteed to be charged. A participant who resubmitted its own valid signature could weaponize that: the duplicate fails AddScriptSig(), aborts the session for everyone, and an honest peer pays for it, every session. RelayStatus() now records the session-wide rejection in m_relayed_abort (cleared by SetNull()), and CheckTimeout() skips the guaranteed charge for a session this coordinator already told its participants to abandon. The saboteur does not go free: a signature that fails AddScriptSig() comes from a verified participant of the live signing session and has no honest cause, so ProcessDSSIGNFINALTX() charges that sender's collateral at the moment it forces the abort. The immediate charge reserves the sender's prevouts in m_pending_charges like the timeout charge does, so a reset that follows the abort cannot re-admit the collateral while its penalty spend is still in flight.
118d995 to
3fc743f
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
One blocking concurrency defect remains in the signature failure path: a stale DSSIGNFINALTX can relay an abort after the completed session has reset, causing the following session to inherit m_relayed_abort and skip its guaranteed abort charge. The prior admission race is fixed by reserving selected collateral prevouts in m_pending_charges until collateral consumption settles.
Source: reviewer backend model gpt-5.6-sol (general); 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
🤖 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:294-310: Revalidate the session before relaying a signature abort
The failure path does not verify that the session captured in `session_id` is still the current signing session before charging collateral or calling `RelayStatus(STATUS_REJECTED)`. After an extra or duplicate signature passes the initial state check, `CheckPool()` can observe that all required signatures are complete and commit the session without consulting `m_inflight_session`. If the commit resets the session before this message reaches the failure block, `AddScriptSig()` fails against the cleared entries, the collateral lookup finds nothing, and the unconditional `RelayStatus()` sets `m_relayed_abort` while the server is idle. `CreateNewSession()` does not clear that flag, so the next session inherits it and `CheckTimeout()` suppresses its guaranteed abort charge. Revalidate `nSessionID == *session_id` and `nState == POOL_STATE_SIGNING` under `cs_coinjoin` before mutating abort state.
| LOCK(cs_coinjoin); | ||
| // The sender is a verified participant of this signing session, so a signature | ||
| // that fails validation - a duplicate, an invalid script or an input that is not | ||
| // in the pool - is the sender's own doing, and the abort it forces on everyone | ||
| // else identifies the sender as the offender to charge. The participants this | ||
| // abort orphans must not pay for it at the timeout that follows. | ||
| const auto it = std::ranges::find_if(vecEntries, | ||
| [&peer](const auto& entry) { return entry.addr == peer.addr; }); | ||
| if (it != vecEntries.end()) { | ||
| collateral_to_charge = it->txCollateral; | ||
| // The submission below runs outside cs_coinjoin, and the reset that can follow | ||
| // this abort would reopen admission before it settles: reserve the charge so | ||
| // the collateral cannot be re-committed while its penalty spend is in flight. | ||
| MarkPendingCharge(collateral_to_charge); | ||
| } | ||
| RelayStatus(STATUS_REJECTED); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Revalidate the session before relaying a signature abort
The failure path does not verify that the session captured in session_id is still the current signing session before charging collateral or calling RelayStatus(STATUS_REJECTED). After an extra or duplicate signature passes the initial state check, CheckPool() can observe that all required signatures are complete and commit the session without consulting m_inflight_session. If the commit resets the session before this message reaches the failure block, AddScriptSig() fails against the cleared entries, the collateral lookup finds nothing, and the unconditional RelayStatus() sets m_relayed_abort while the server is idle. CreateNewSession() does not clear that flag, so the next session inherits it and CheckTimeout() suppresses its guaranteed abort charge. Revalidate nSessionID == *session_id and nState == POOL_STATE_SIGNING under cs_coinjoin before mutating abort state.
| LOCK(cs_coinjoin); | |
| // The sender is a verified participant of this signing session, so a signature | |
| // that fails validation - a duplicate, an invalid script or an input that is not | |
| // in the pool - is the sender's own doing, and the abort it forces on everyone | |
| // else identifies the sender as the offender to charge. The participants this | |
| // abort orphans must not pay for it at the timeout that follows. | |
| const auto it = std::ranges::find_if(vecEntries, | |
| [&peer](const auto& entry) { return entry.addr == peer.addr; }); | |
| if (it != vecEntries.end()) { | |
| collateral_to_charge = it->txCollateral; | |
| // The submission below runs outside cs_coinjoin, and the reset that can follow | |
| // this abort would reopen admission before it settles: reserve the charge so | |
| // the collateral cannot be re-committed while its penalty spend is in flight. | |
| MarkPendingCharge(collateral_to_charge); | |
| } | |
| RelayStatus(STATUS_REJECTED); | |
| } | |
| { | |
| LOCK(cs_coinjoin); | |
| if (nSessionID != *session_id || nState != POOL_STATE_SIGNING) { | |
| LogPrint(BCLog::COINJOIN, | |
| "DSSIGNFINALTX -- session %d is gone or no longer signing\n", *session_id); | |
| return; | |
| } | |
| // The sender is a verified participant of this signing session, so a signature | |
| // that fails validation - a duplicate, an invalid script or an input that is not | |
| // in the pool - is the sender's own doing, and the abort it forces on everyone | |
| // else identifies the sender as the offender to charge. The participants this | |
| // abort orphans must not pay for it at the timeout that follows. | |
| const auto it = std::ranges::find_if(vecEntries, | |
| [&peer](const auto& entry) { return entry.addr == peer.addr; }); | |
| if (it != vecEntries.end()) { | |
| collateral_to_charge = it->txCollateral; | |
| // The submission below runs outside cs_coinjoin, and the reset that can follow | |
| // this abort would reopen admission before it settles: reserve the charge so | |
| // the collateral cannot be re-committed while its penalty spend is in flight. | |
| MarkPendingCharge(collateral_to_charge); | |
| } | |
| RelayStatus(STATUS_REJECTED); | |
| } |
source: ['codex']
PastaPastaPasta
added
the
backport-candidate-23.1.x
label
ChatGPT Codex Connector
3 participants
Issue being fixed or feature implemented
CoinJoin participants can currently reserve a coordinator slot and then abort during entry submission or signing without necessarily losing collateral. In particular, the existing probabilistic policy exempts sessions where every participant is an offender, allowing coordinated non-cooperation to repeatedly kill sessions without cost.
This PR is intentionally built on #7537, which makes fee selection and session reset atomic. It should be reviewed and merged after that prerequisite.
What was done?
PROBABILISTICandGUARANTEED_ON_ABORTmodes.SetNull()undercs_coinjoin, then consume the selected collateral after releasing the lock.STATUS_REJECTED): honest clients obey the abort, release their inputs, and stop signing, so they must not form the offender set. The participant whose invalid signature forced the abort is charged directly inProcessDSSIGNFINALTX()instead.IsSignaturesComplete()whenCheckTimeout()inspects a signing session, and commit a fully signed transaction instead of discarding it — the finalDSSIGNFINALTXcan land in a scheduler round whoseCheckPool()already sampled the signatures as incomplete.server.cpphead so the stacked branch compiles.How Has This Been Tested?
Built
src/test/test_dashlocally on macOS arm64 using the prebuilt depends prefix, then ran:The unit coverage exercises queue timeouts, all/many/few/no missing entries, lone and multiple non-signers, deduplication of participants with several unsigned inputs, all-participant signing failure, timeout/reset atomicity, the recoverable probabilistic policy, successful-session random charging, committing a fully signed session at timeout, direct saboteur charging, and forgoing the guaranteed charge after a relayed abort.
Breaking Changes
No wire-format, wallet, database, persistent-format, or consensus change. Mixed-version operation remains safe; only upgraded masternodes apply the guaranteed failed-session fee.
Checklist:
This pull request was created by Codex.