fix: charge one collateral when a CoinJoin session aborts - #7567
fix: charge one collateral when a CoinJoin session aborts#7567PastaPastaPasta wants to merge 2 commits into
Conversation
![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: 6b8231e819
ℹ️ 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(); | ||
| } | ||
|
|
||
| if (txCollateralToConsume) { | ||
| ConsumeCollateral(txCollateralToConsume); |
There was a problem hiding this comment.
When ProcessDSACCEPT runs concurrently after SetNull() releases cs_coinjoin, an offender can immediately open the newly idle server with the same collateral—or a re-signed transaction spending the same inputs—because the selected transaction has not entered the mempool yet. The subsequent ConsumeCollateral() then spends those inputs, leaving the new session with an invalid committed collateral and causing its entry or finalization to fail. Previously the reset happened only after ChargeFees() had consumed the collateral, so preserve the reservation/non-idle state until consumption completes or otherwise prevent those inputs from being admitted to the next session.
Useful? React with 👍 / 👎.
|
✅ Final review complete — no blockers (commit b682741) |
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughCoinJoin fee handling now separates collateral selection from collateral consumption. Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CCoinJoinServer
participant CoinJoinSession
participant Collateral
CCoinJoinServer->>CoinJoinSession: detect timeout under cs_coinjoin
CCoinJoinServer->>Collateral: select collateral with FeePolicy
CCoinJoinServer->>CoinJoinSession: reset session
CCoinJoinServer->>Collateral: consume selected collateral
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/test/coinjoin_inouts_tests.cpp (1)
236-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the server construction into a local factory.
The test builds
TestableCoinJoinServernine more times with the identical ten-argument list (s2 through s10). Any constructor change requires ten edits. Add one factory and call it per scenario.♻️ Suggested refactor
CActiveMasternodeManager mn_activeman(*Assert(m_node.connman), *Assert(m_node.dmnman), MakeSecretKey()); - TestableCoinJoinServer server(m_node.peerman.get(), *Assert(m_node.chainman), *Assert(m_node.connman), - *Assert(m_node.dmnman), *Assert(m_node.dstxman), *Assert(m_node.mn_metaman), - *Assert(m_node.mempool), mn_activeman, *Assert(m_node.mn_sync), - *Assert(m_node.llmq_ctx->isman)); + auto MakeServer = [&]() { + return std::make_unique<TestableCoinJoinServer>( + m_node.peerman.get(), *Assert(m_node.chainman), *Assert(m_node.connman), *Assert(m_node.dmnman), + *Assert(m_node.dstxman), *Assert(m_node.mn_metaman), *Assert(m_node.mempool), mn_activeman, + *Assert(m_node.mn_sync), *Assert(m_node.llmq_ctx->isman)); + }; + auto server = MakeServer();🤖 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 236 - 246, In coinjoin_offender_selection_and_abort_fee_scenarios, extract the repeated ten-argument TestableCoinJoinServer construction into a local factory that captures the shared dependencies and returns a server instance. Replace the direct constructions for the initial server and scenarios s2 through s10 with factory calls, preserving each scenario’s existing behavior.src/coinjoin/server.cpp (1)
451-460: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGuard the unsigned subtraction at Line 456.
vecSessionCollaterals.size() - 1wraps toSIZE_MAXwhen the vector is empty. The current code stays correct because Line 459 then returnsnullptr, so this is not an active defect. The expression is still fragile if the gate order changes later. Compare with addition instead.♻️ Suggested change
- if (vecOffendersCollaterals.size() >= vecSessionCollaterals.size() - 1 && GetRand<int>(/*nMax=*/100) > 33) return nullptr; + if (vecOffendersCollaterals.size() + 1 >= vecSessionCollaterals.size() && GetRand<int>(/*nMax=*/100) > 33) return nullptr;🤖 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 451 - 460, Replace the unsigned subtraction in the probabilistic policy check within the collateral-selection logic with an equivalent addition-based boundary comparison, avoiding vecSessionCollaterals.size() - 1 while preserving the existing behavior of the offender and session collateral thresholds.src/coinjoin/server.h (1)
70-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the default
FeePolicyargument.All
ChargeFeescallers pass the policy explicitly. Require an explicit policy so each fee decision remains visible at the call site.
NetHandleralready has a virtual destructor, so no destructor change is required.🤖 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.h` around lines 70 - 75, Remove the default FeePolicy::PROBABILISTIC argument from the ChargeFees declaration in the coinjoin server interface, requiring every caller to pass FeePolicy explicitly. Leave the existing const and lock annotations unchanged; no destructor changes are needed.
🤖 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/test/coinjoin_inouts_tests.cpp`:
- Around line 488-489: Replace the placeholder cases 11 and 12 in the coinjoin
tests with executable assertions or remove them from the stated case count. At
minimum, implement case 11 by configuring POOL_STATE_ACCEPTING_ENTRIES, adding
sufficient collaterals and entries to satisfy GetMinPoolParticipants(), expiring
the timeout, invoking CheckPool, and asserting that no more than one collateral
is consumed; do not claim implicit coverage without a CheckPool call.
- Around line 442-444: Update the comment above the count_c0/count_c1 assertions
to remove the inaccurate “deduplication” wording and describe only the intended
equal-weight selection distribution. Leave the assertions and
SelectCollateralToCharge behavior unchanged.
---
Nitpick comments:
In `@src/coinjoin/server.cpp`:
- Around line 451-460: Replace the unsigned subtraction in the probabilistic
policy check within the collateral-selection logic with an equivalent
addition-based boundary comparison, avoiding vecSessionCollaterals.size() - 1
while preserving the existing behavior of the offender and session collateral
thresholds.
In `@src/coinjoin/server.h`:
- Around line 70-75: Remove the default FeePolicy::PROBABILISTIC argument from
the ChargeFees declaration in the coinjoin server interface, requiring every
caller to pass FeePolicy explicitly. Leave the existing const and lock
annotations unchanged; no destructor changes are needed.
In `@src/test/coinjoin_inouts_tests.cpp`:
- Around line 236-246: In coinjoin_offender_selection_and_abort_fee_scenarios,
extract the repeated ten-argument TestableCoinJoinServer construction into a
local factory that captures the shared dependencies and returns a server
instance. Replace the direct constructions for the initial server and scenarios
s2 through s10 with factory calls, preserving each scenario’s existing behavior.
🪄 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: d05f5a2d-b61f-4e15-9d80-aebd436fc4cc
📒 Files selected for processing (3)
src/coinjoin/server.cppsrc/coinjoin/server.hsrc/test/coinjoin_inouts_tests.cpp
| // 11. Recoverable timeouts retain probabilistic policy (tested implicitly via CheckPool logic) | ||
| // 12. Successful-session random charging remains unchanged |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the placeholder comments with real cases or remove them.
Cases 11 and 12 contain no assertions. No test in this file calls CheckPool, so the claim at Line 488 that the probabilistic recoverable-timeout path is "tested implicitly" is not supported. These two lines count toward the 12 cases stated in the PR description but verify nothing.
Case 11 is testable: set POOL_STATE_ACCEPTING_ENTRIES, add enough collaterals and entries to pass GetMinPoolParticipants(), expire the timeout, call CheckPool, and assert that at most one collateral is consumed.
Do you want me to add the two cases?
🤖 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 488 - 489, Replace the
placeholder cases 11 and 12 in the coinjoin tests with executable assertions or
remove them from the stated case count. At minimum, implement case 11 by
configuring POOL_STATE_ACCEPTING_ENTRIES, adding sufficient collaterals and
entries to satisfy GetMinPoolParticipants(), expiring the timeout, invoking
CheckPool, and asserting that no more than one collateral is consumed; do not
claim implicit coverage without a CheckPool call.
There was a problem hiding this comment.
The placeholders indeed provide no executable coverage, and this file does not call CheckPool. The proposed “at most one collateral” assertion would still not prove that the probabilistic policy was retained because both policies select at most one collateral; a replacement test must distinguish the policy-specific outcome, or the numbered cases and advertised count should be removed.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/coinjoin/server.cpp --items all --type function --match 'CheckPool|CheckTimeout|ChargeFees|SelectCollateralToCharge'
ast-grep outline src/coinjoin/server.h --items all --type function --match 'CheckPool|CheckTimeout|ChargeFees|SelectCollateralToCharge'
rg -n -A70 -B15 'CCoinJoinServer::(CheckPool|CheckTimeout|ChargeFees|SelectCollateralToCharge)\s*\(' \
src/coinjoin/server.cppLength of output: 13337
@thepastaclaw, agreed. I verified the paths.
CheckPool uses FeePolicy::PROBABILISTIC only for an accepting-entry timeout with at least GetMinPoolParticipants() entries. Its random gates mean that the consumed-collateral count cannot prove the selected policy.
The deterministic all-offenders condition distinguishes the policies, but it cannot reach this CheckPool branch because that branch requires entries.
I will revise this finding: remove cases 11 and 12 and remove them from the advertised count. Add a CheckPool policy regression only if this PR introduces a narrow deterministic selection or RNG seam.
✏️ Learnings added
Learnt from: thepastaclaw
Repo: dashpay/dash PR: 7567
File: src/test/coinjoin_inouts_tests.cpp:488-489
Timestamp: 2026-08-09T22:50:46.598Z
Learning: In `src/coinjoin/server.cpp`, `CCoinJoinServer::CheckPool` calls `ChargeFees(FeePolicy::PROBABILISTIC)` only after an accepting-entry timeout with at least `CoinJoin::GetMinPoolParticipants()` entries. `CCoinJoinServer::SelectCollateralToCharge(FeePolicy::PROBABILISTIC)` uses random gates, so a consumed-collateral count does not prove that `CheckPool` retained the probabilistic policy. The deterministic all-offenders distinction cannot exercise this `CheckPool` branch because the branch requires the minimum entry count.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
6b8231e to
b682741
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The CoinJoin abort path correctly selects one offender, keeps the server non-idle during collateral consumption, and resets afterward; no functional defect was confirmed. The only in-scope issue is that two numbered “test cases” are comments rather than executable coverage, so the advertised test count is inaccurate.
Source: Reviewer backends: codex general — gpt-5.6-sol; codex dash-core-commit-history — gpt-5.6-sol. Final verifier backend: gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/coinjoin_inouts_tests.cpp`:
- [NITPICK] src/test/coinjoin_inouts_tests.cpp:488-489: Numbered cases 11 and 12 provide no test coverage
These numbered cases contain no setup, calls, or assertions, and this test file never invokes `CheckPool`, so case 11's claimed implicit coverage does not exist. This also makes the PR description's claim of 12 regression cases inaccurate. Either add executable tests that distinguish `PROBABILISTIC` from `GUARANTEED_ON_ABORT` and exercise the successful-session path, or remove these numbered comments and correct the stated test count. Merely asserting that at most one collateral is consumed would not distinguish the policies because both select at most one collateral.
| // 11. Recoverable timeouts retain probabilistic policy (tested implicitly via CheckPool logic) | ||
| // 12. Successful-session random charging remains unchanged |
There was a problem hiding this comment.
💬 Nitpick: Numbered cases 11 and 12 provide no test coverage
These numbered cases contain no setup, calls, or assertions, and this test file never invokes CheckPool, so case 11's claimed implicit coverage does not exist. This also makes the PR description's claim of 12 regression cases inaccurate. Either add executable tests that distinguish PROBABILISTIC from GUARANTEED_ON_ABORT and exercise the successful-session path, or remove these numbered comments and correct the stated test count. Merely asserting that at most one collateral is consumed would not distinguish the policies because both select at most one collateral.
source: ['coderabbit']
2 participants
Depends on #7566
Issue being fixed or feature implemented
When a CoinJoin mixing session aborts due to non-cooperation (missing entries or missing signatures),
ChargeFees()previously subjected collateral selection to probabilistic gates and an "everyone is an offender" exemption. When all participants failed to submit or sign, zero fees were charged, allowing attackers to abort sessions without penalty.What was done?
CheckTimeout()to perform offender selection viaFeePolicy::GUARANTEED_ON_ABORTand session state reset (SetNull()) atomically undercs_coinjoin.cs_coinjoinbefore callingConsumeCollateral().POOL_STATE_ACCEPTING_ENTRIESandPOOL_STATE_SIGNINGconsume exactly one collateral if non-cooperative participants exist, whilePOOL_STATE_QUEUEcharges nobody.src/test/coinjoin_inouts_tests.cpp.How Has This Been Tested?
src/test/test_dash.src/test/test_dash --run_test=coinjoin_inouts_tests(all 9 test cases passed)../src/test/test_dash(0 errors).test/lint/all-lint.py.Breaking Changes
None. Mixed-version operation is safe: only upgraded masternodes enforce the new failed-session fee.
Checklist: