fix: charge collateral when a CoinJoin session aborts - #7568
fix: charge collateral when a CoinJoin session aborts#7568PastaPastaPasta wants to merge 9 commits into
Conversation
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.
The line-based lint-logs.py flags any LogPrint( line that carries neither a newline terminator nor an explicit /* Continued */ marker.
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
PastaPastaPasta
changed the title
|
⛔ Blockers found — Opus deferred (commit 3d5cf6b) |
![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 👍 / 👎.
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (3)
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 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.
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.
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']
| 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']
2 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.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, and successful-session random charging.
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.