Skip to content

refactor: separate CoinJoin offender selection policy #7566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
PastaPastaPasta wants to merge 1 commit into from
+65 −33
Closed

refactor: separate CoinJoin offender selection policy #7566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 53 additions & 31 deletions src/coinjoin/server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ void CCoinJoinServer::CheckPool()
if (nState == POOL_STATE_ACCEPTING_ENTRIES && CCoinJoinServer::HasTimedOut() &&
GetEntriesCount() >= CoinJoin::GetMinPoolParticipants()) {
// Punish misbehaving participants
ChargeFees();
ChargeFees(FeePolicy::PROBABILISTIC);
// Try to complete this session ignoring the misbehaving ones
CreateFinalTransaction();
return;
Expand Down Expand Up @@ -416,63 +416,85 @@ void CCoinJoinServer::CommitFinalTransaction()
// transaction for the client to be able to enter the pool. This transaction is kept by the Masternode
// until the transaction is either complete or fails.
//
void CCoinJoinServer::ChargeFees() const
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;
Comment on lines +419 to +459

@thepastaclaw thepastaclaw Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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']

}

//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());
Comment on lines +467 to +470

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Comment on lines +467 to +470

@thepastaclaw thepastaclaw Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 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.

Suggested change
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']

} 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;
Comment on lines +419 to +483

@coderabbitai coderabbitai Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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

}

void CCoinJoinServer::ChargeFees(FeePolicy policy) const
{
AssertLockNotHeld(cs_coinjoin);

CTransactionRef txCollateralToConsume;
{
LOCK(cs_coinjoin);
txCollateralToConsume = SelectCollateralToCharge(policy);
}

if (txCollateralToConsume) {
ConsumeCollateral(txCollateralToConsume);
}
}

Expand Down
14 changes: 12 additions & 2 deletions src/coinjoin/server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,17 +52,27 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public NetHandler

bool fUnitTest;

public:
enum class FeePolicy {
PROBABILISTIC,
GUARANTEED_ON_ABORT,
};

protected:
/// Select a collateral to charge based on offender discovery and fee policy
CTransactionRef SelectCollateralToCharge(FeePolicy policy) const EXCLUSIVE_LOCKS_REQUIRED(cs_coinjoin);

/// Add a clients entry to the pool
bool AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessageIDRet) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin);
/// Add signature to a txin
bool AddScriptSig(const CTxIn& txin) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin);

/// 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);

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use the guaranteed policy when aborting a session

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 👍 / 👎.

/// Rarely charge fees to pay miners
void ChargeRandomFees() const;
/// Consume collateral in cases when peer misbehaved
void ConsumeCollateral(const CTransactionRef& txref) const;
virtual void ConsumeCollateral(const CTransactionRef& txref) const;

/// Check for process
void CheckPool();
Expand Down
Loading