refactor: separate CoinJoin offender selection policy - #7566
refactor: separate CoinJoin offender selection policy#7566PastaPastaPasta wants to merge 1 commit into
Conversation
|
✅ Final review complete — no blockers (commit 2dda8c3) |
WalkthroughCoinJoin fee charging now uses explicit Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TimeoutHandling
participant CCoinJoinServer
participant ConsumeCollateral
TimeoutHandling->>CCoinJoinServer: ChargeFees(PROBABILISTIC)
CCoinJoinServer->>CCoinJoinServer: SelectCollateralToCharge(policy)
CCoinJoinServer->>ConsumeCollateral: 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
🤖 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 467-470: Update the log message in
CCoinJoinServer::SelectCollateralToCharge for the probabilistic penalty so it
describes the selected participant as an “offending participant” rather than
“non-submitting.” If state-specific wording is retained, derive it from nState
and ensure POOL_STATE_SIGNING reports unsigned-input offenders correctly.
- Around line 419-483: Add targeted C++ unit tests for
CCoinJoinServer::SelectCollateralToCharge covering POOL_STATE_ACCEPTING_ENTRIES
and POOL_STATE_SIGNING. Verify only offender collaterals are returned, each
signing entry contributes at most one collateral, GUARANTEED_ON_ABORT can select
when all participants offend, and PROBABILISTIC returns no collateral under its
deterministic no-charge conditions.
🪄 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: 1fa69102-29ef-474b-a50f-79c2789e5f99
📒 Files selected for processing (2)
src/coinjoin/server.cppsrc/coinjoin/server.h
| CTransactionRef CCoinJoinServer::SelectCollateralToCharge(FeePolicy policy) const | ||
| { | ||
| AssertLockNotHeld(cs_coinjoin); | ||
|
|
||
| //we don't need to charge collateral for every offence. | ||
| if (GetRand<int>(/*nMax=*/100) > 33) return; | ||
| AssertLockHeld(cs_coinjoin); | ||
|
|
||
| std::vector<CTransactionRef> vecOffendersCollaterals; | ||
|
|
||
| if (nState == POOL_STATE_ACCEPTING_ENTRIES) { | ||
| LOCK(cs_coinjoin); | ||
| for (const auto& txCollateral : vecSessionCollaterals) { | ||
| bool fFound = std::ranges::any_of(vecEntries, [&txCollateral](const auto& entry) { | ||
| return *entry.txCollateral == *txCollateral; | ||
| }); | ||
|
|
||
| // This queue entry didn't send us the promised transaction | ||
| if (!fFound) { | ||
| LogPrint(BCLog::COINJOIN, /* Continued */ | ||
| "CCoinJoinServer::ChargeFees -- found uncooperative node (didn't send transaction), found " | ||
| "offence\n"); | ||
| vecOffendersCollaterals.push_back(txCollateral); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (nState == POOL_STATE_SIGNING) { | ||
| } else if (nState == POOL_STATE_SIGNING) { | ||
| // who didn't sign? | ||
| LOCK(cs_coinjoin); | ||
| 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); | ||
| } | ||
| bool fHasUnsignedInput = std::ranges::any_of(entry.vecTxDSIn, [](const auto& txdsin) { | ||
| return !txdsin.fHasSig; | ||
| }); | ||
| if (fHasUnsignedInput) { | ||
| vecOffendersCollaterals.push_back(entry.txCollateral); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // no offences found | ||
| if (vecOffendersCollaterals.empty()) return; | ||
| if (vecOffendersCollaterals.empty()) return nullptr; | ||
|
|
||
| if (policy == FeePolicy::PROBABILISTIC) { | ||
| // we don't need to charge collateral for every offence. | ||
| if (GetRand<int>(/*nMax=*/100) > 33) return nullptr; | ||
|
|
||
| //mostly offending? Charge sometimes | ||
| if (vecOffendersCollaterals.size() >= vecSessionCollaterals.size() - 1 && GetRand<int>(/*nMax=*/100) > 33) return; | ||
| // mostly offending? Charge sometimes | ||
| if (vecOffendersCollaterals.size() >= vecSessionCollaterals.size() - 1 && GetRand<int>(/*nMax=*/100) > 33) return nullptr; | ||
|
|
||
| //everyone is an offender? That's not right | ||
| if (vecOffendersCollaterals.size() >= vecSessionCollaterals.size()) return; | ||
| // everyone is an offender? That's not right | ||
| if (vecOffendersCollaterals.size() >= vecSessionCollaterals.size()) return nullptr; | ||
| } | ||
|
|
||
| //charge one of the offenders randomly | ||
| // charge one of the offenders randomly | ||
| Shuffle(vecOffendersCollaterals.begin(), vecOffendersCollaterals.end(), FastRandomContext()); | ||
|
|
||
| if (nState == POOL_STATE_ACCEPTING_ENTRIES || nState == POOL_STATE_SIGNING) { | ||
| LogPrint(BCLog::COINJOIN, /* Continued */ | ||
| "CCoinJoinServer::ChargeFees -- found uncooperative node (didn't %s transaction), charging fees: %s", | ||
| (nState == POOL_STATE_SIGNING) ? "sign" : "send", vecOffendersCollaterals[0]->ToString()); | ||
| ConsumeCollateral(vecOffendersCollaterals[0]); | ||
| CTransactionRef selectedCollateral = vecOffendersCollaterals[0]; | ||
|
|
||
| if (policy == FeePolicy::PROBABILISTIC) { | ||
| LogPrint(BCLog::COINJOIN, | ||
| "CCoinJoinServer::SelectCollateralToCharge -- selected non-submitting participant for probabilistic penalty. state=%s, participants=%d, offenders=%d, txid=%s\n", | ||
| GetStateString(), vecSessionCollaterals.size(), vecOffendersCollaterals.size(), selectedCollateral->GetHash().ToString()); | ||
| } else if (policy == FeePolicy::GUARANTEED_ON_ABORT) { | ||
| if (vecOffendersCollaterals.size() >= vecSessionCollaterals.size()) { | ||
| LogPrint(BCLog::COINJOIN, | ||
| "CCoinJoinServer::SelectCollateralToCharge -- all participants missing or uncooperative, selected participant for failed-session fee. state=%s, participants=%d, offenders=%d, txid=%s\n", | ||
| GetStateString(), vecSessionCollaterals.size(), vecOffendersCollaterals.size(), selectedCollateral->GetHash().ToString()); | ||
| } else { | ||
| LogPrint(BCLog::COINJOIN, | ||
| "CCoinJoinServer::SelectCollateralToCharge -- selected participant for failed-session fee. state=%s, participants=%d, offenders=%d, txid=%s\n", | ||
| GetStateString(), vecSessionCollaterals.size(), vecOffendersCollaterals.size(), selectedCollateral->GetHash().ToString()); | ||
| } | ||
| } | ||
|
|
||
| return selectedCollateral; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add targeted tests for fee-policy selection.
This PR adds state-dependent offender selection and two fee policies. It contains no C++ test change.
Add tests for accepting and signing states. Verify that selection only returns offender collateral, signing entries contribute at most one collateral, and GUARANTEED_ON_ABORT can select when all participants offend. Verify the deterministic no-charge conditions for PROBABILISTIC.
As per coding guidelines, “Choose and add targeted C++ unit tests for changed behavior.”
🤖 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 419 - 483, Add targeted C++ unit tests
for CCoinJoinServer::SelectCollateralToCharge covering
POOL_STATE_ACCEPTING_ENTRIES and POOL_STATE_SIGNING. Verify only offender
collaterals are returned, each signing entry contributes at most one collateral,
GUARANTEED_ON_ABORT can select when all participants offend, and PROBABILISTIC
returns no collateral under its deterministic no-charge conditions.
Source: Coding guidelines
| if (policy == FeePolicy::PROBABILISTIC) { | ||
| LogPrint(BCLog::COINJOIN, | ||
| "CCoinJoinServer::SelectCollateralToCharge -- selected non-submitting participant for probabilistic penalty. state=%s, participants=%d, offenders=%d, txid=%s\n", | ||
| GetStateString(), vecSessionCollaterals.size(), vecOffendersCollaterals.size(), selectedCollateral->GetHash().ToString()); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Log the correct offender type in the signing state.
POOL_STATE_SIGNING selects participants with unsigned inputs. The log still calls the selected participant “non-submitting.”
Use generic “offending participant” text, or derive the text from nState.
🤖 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 467 - 470, Update the log message in
CCoinJoinServer::SelectCollateralToCharge for the probabilistic penalty so it
describes the selected participant as an “offending participant” rather than
“non-submitting.” If state-specific wording is retained, derive it from nState
and ensure POOL_STATE_SIGNING reports unsigned-input offenders correctly.
![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: 2dda8c33f9
ℹ️ 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".
|
|
||
| /// Charge fees to bad actors (Charge clients a fee if they're abusive) | ||
| void ChargeFees() const EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin); | ||
| void ChargeFees(FeePolicy policy = FeePolicy::PROBABILISTIC) const EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin); |
There was a problem hiding this comment.
CheckTimeout() still invokes ChargeFees() without an argument, so this default selects PROBABILISTIC for the signing-timeout and insufficient-entry paths that immediately reset the session. Consequently, GUARANTEED_ON_ABORT and its failed-session behavior are unreachable in production, and an offender can abort these sessions without collateral consumption whenever the probabilistic gates decline the charge. Pass FeePolicy::GUARANTEED_ON_ABORT from the aborting timeout path while retaining the explicit probabilistic policy in CheckPool() for sessions that can continue.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The refactor correctly changes signing-stage offender selection from per-unsigned-input weighting to one candidate per entry, and no functional correctness defect was confirmed. The behavior-changing selection logic lacks direct regression coverage, and one newly added log message inaccurately describes signing-stage offenders.
Source: reviewer backends codex-general=gpt-5.6-sol and codex-dash-core-commit-history=gpt-5.6-sol; final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/server.cpp`:
- [SUGGESTION] src/coinjoin/server.cpp:419-459: Add regression coverage for offender selection policies
This is the PR's principal behavior change: a signing entry containing several unsigned inputs now contributes only one collateral candidate instead of receiving proportionally greater selection weight. The PR changes no tests, and the existing `coinjoin_inouts_tests` do not call `SelectCollateralToCharge()`, so restoring the old per-input loop would pass the current suite. Add focused C++ tests for accepting and signing states, including entries with different unsigned-input counts, selection exclusively from offenders, the no-offender case, guaranteed selection when all participants offend, and the deterministic probabilistic-policy exemptions.
- [NITPICK] src/coinjoin/server.cpp:467-470: Describe signing-stage offenders accurately in the log
The probabilistic policy is used in both accepting-entries and signing states. In the signing state, the selected participant submitted an entry but left at least one input unsigned, so calling it a `non-submitting participant` is inaccurate and can mislead operational diagnosis. Use state-neutral offender wording.
| CTransactionRef CCoinJoinServer::SelectCollateralToCharge(FeePolicy policy) const | ||
| { | ||
| AssertLockNotHeld(cs_coinjoin); | ||
|
|
||
| //we don't need to charge collateral for every offence. | ||
| if (GetRand<int>(/*nMax=*/100) > 33) return; | ||
| AssertLockHeld(cs_coinjoin); | ||
|
|
||
| std::vector<CTransactionRef> vecOffendersCollaterals; | ||
|
|
||
| if (nState == POOL_STATE_ACCEPTING_ENTRIES) { | ||
| LOCK(cs_coinjoin); | ||
| for (const auto& txCollateral : vecSessionCollaterals) { | ||
| bool fFound = std::ranges::any_of(vecEntries, [&txCollateral](const auto& entry) { | ||
| return *entry.txCollateral == *txCollateral; | ||
| }); | ||
|
|
||
| // This queue entry didn't send us the promised transaction | ||
| if (!fFound) { | ||
| LogPrint(BCLog::COINJOIN, /* Continued */ | ||
| "CCoinJoinServer::ChargeFees -- found uncooperative node (didn't send transaction), found " | ||
| "offence\n"); | ||
| vecOffendersCollaterals.push_back(txCollateral); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (nState == POOL_STATE_SIGNING) { | ||
| } else if (nState == POOL_STATE_SIGNING) { | ||
| // who didn't sign? | ||
| LOCK(cs_coinjoin); | ||
| 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); | ||
| } | ||
| bool fHasUnsignedInput = std::ranges::any_of(entry.vecTxDSIn, [](const auto& txdsin) { | ||
| return !txdsin.fHasSig; | ||
| }); | ||
| if (fHasUnsignedInput) { | ||
| vecOffendersCollaterals.push_back(entry.txCollateral); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // no offences found | ||
| if (vecOffendersCollaterals.empty()) return; | ||
| if (vecOffendersCollaterals.empty()) return nullptr; | ||
|
|
||
| if (policy == FeePolicy::PROBABILISTIC) { | ||
| // we don't need to charge collateral for every offence. | ||
| if (GetRand<int>(/*nMax=*/100) > 33) return nullptr; | ||
|
|
||
| //mostly offending? Charge sometimes | ||
| if (vecOffendersCollaterals.size() >= vecSessionCollaterals.size() - 1 && GetRand<int>(/*nMax=*/100) > 33) return; | ||
| // mostly offending? Charge sometimes | ||
| if (vecOffendersCollaterals.size() >= vecSessionCollaterals.size() - 1 && GetRand<int>(/*nMax=*/100) > 33) return nullptr; | ||
|
|
||
| //everyone is an offender? That's not right | ||
| if (vecOffendersCollaterals.size() >= vecSessionCollaterals.size()) return; | ||
| // everyone is an offender? That's not right | ||
| if (vecOffendersCollaterals.size() >= vecSessionCollaterals.size()) return nullptr; |
There was a problem hiding this comment.
🟡 Suggestion: Add regression coverage for offender selection policies
This is the PR's principal behavior change: a signing entry containing several unsigned inputs now contributes only one collateral candidate instead of receiving proportionally greater selection weight. The PR changes no tests, and the existing coinjoin_inouts_tests do not call SelectCollateralToCharge(), so restoring the old per-input loop would pass the current suite. Add focused C++ tests for accepting and signing states, including entries with different unsigned-input counts, selection exclusively from offenders, the no-offender case, guaranteed selection when all participants offend, and the deterministic probabilistic-policy exemptions.
source: ['codex', 'coderabbit']
| if (policy == FeePolicy::PROBABILISTIC) { | ||
| LogPrint(BCLog::COINJOIN, | ||
| "CCoinJoinServer::SelectCollateralToCharge -- selected non-submitting participant for probabilistic penalty. state=%s, participants=%d, offenders=%d, txid=%s\n", | ||
| GetStateString(), vecSessionCollaterals.size(), vecOffendersCollaterals.size(), selectedCollateral->GetHash().ToString()); |
There was a problem hiding this comment.
💬 Nitpick: Describe signing-stage offenders accurately in the log
The probabilistic policy is used in both accepting-entries and signing states. In the signing state, the selected participant submitted an entry but left at least one input unsigned, so calling it a non-submitting participant is inaccurate and can mislead operational diagnosis. Use state-neutral offender wording.
| if (policy == FeePolicy::PROBABILISTIC) { | |
| LogPrint(BCLog::COINJOIN, | |
| "CCoinJoinServer::SelectCollateralToCharge -- selected non-submitting participant for probabilistic penalty. state=%s, participants=%d, offenders=%d, txid=%s\n", | |
| GetStateString(), vecSessionCollaterals.size(), vecOffendersCollaterals.size(), selectedCollateral->GetHash().ToString()); | |
| LogPrint(BCLog::COINJOIN, | |
| "CCoinJoinServer::SelectCollateralToCharge -- selected offending participant for probabilistic penalty. state=%s, participants=%d, offenders=%d, txid=%s\n", | |
| GetStateString(), vecSessionCollaterals.size(), vecOffendersCollaterals.size(), selectedCollateral->GetHash().ToString()); |
source: ['coderabbit']
2 participants
Issue being fixed or feature implemented
In
CCoinJoinServer, offender discovery and collateral fee charging were coupled insideChargeFees().ChargeFees()also pushedentry.txCollateralonce per unsigned input duringPOOL_STATE_SIGNING, causing participants with multiple inputs to receive disproportionate offender selection weight.What was done?
enum class FeePolicy { PROBABILISTIC, GUARANTEED_ON_ABORT }insrc/coinjoin/server.h.SelectCollateralToCharge(FeePolicy policy)insrc/coinjoin/server.cpp:POOL_STATE_ACCEPTING_ENTRIES(reservations missing matchingDSVINentries) andPOOL_STATE_SIGNING(entries with unsigned inputs).CheckPool().SelectCollateralToChargeto distinguish probabilistic penalties vs failed-session fees.How Has This Been Tested?
src/test/test_dash.src/test/test_dash --run_test=coinjoin_inouts_tests.test/lint/lint-logs.pyandtest/lint/lint-whitespace.py.Breaking Changes
None.
Checklist: