fix: reject duplicate session collateral in CoinJoin dsa acceptance - #7507
Conversation
|
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:
Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32715ddaa0
ℹ️ 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".
| // don't let the same collateral join the session twice: a replayed dsa would | ||
| // otherwise be counted as an extra participant | ||
| if (std::ranges::any_of(vecSessionCollaterals, [&dsa](const CTransactionRef& txCollateral) { | ||
| return txCollateral->GetHash() == dsa.txCollateral.GetHash(); |
There was a problem hiding this comment.
When a malicious client creates multiple collateral transactions spending the same UTXO but varies the change output or signature, each transaction has a different hash and passes IsCollateralValid independently because these transactions are only test-accepted, not inserted into the mempool. All variants can therefore still be appended as separate participants even though at most one can be consumed, preserving the participant-inflation and session-stalling attack this change is intended to prevent. Reject a DSA when any of its collateral prevouts are already used by a session collateral rather than only when the complete transaction hash matches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/coinjoin/server.cpp (1)
807-816: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a targeted regression test for duplicate admission.
For a non-ready session, submit the same collateral twice. Verify that the second call returns
false, setsnMessageIDRettoERR_ALREADY_HAVE, and leavesvecSessionCollaterals.size()unchanged. Verify that the replay does not makeIsSessionReady()returntrue.The PR reports that no tests were run. Run the targeted CoinJoin test for
src/coinjoin/server.cpp. As per coding guidelines, choose tests based on the files touched and do not claim broad validation when only a targeted test was run.🤖 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 807 - 816, Add a targeted CoinJoin regression test for duplicate admission in the non-ready session path: submit the same collateral twice through AddUserToExistingSession, then assert the second call returns false, sets nMessageIDRet to ERR_ALREADY_HAVE, preserves vecSessionCollaterals.size(), and leaves IsSessionReady() false. Run the targeted test covering src/coinjoin/server.cpp and report only that validation.Source: Coding guidelines
🤖 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/server.cpp`:
- Around line 807-816: Add a targeted CoinJoin regression test for duplicate
admission in the non-ready session path: submit the same collateral twice
through AddUserToExistingSession, then assert the second call returns false,
sets nMessageIDRet to ERR_ALREADY_HAVE, preserves vecSessionCollaterals.size(),
and leaves IsSessionReady() false. Run the targeted test covering
src/coinjoin/server.cpp and report only that validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 75e60afe-887b-44f4-b32f-48e37adad066
📒 Files selected for processing (1)
src/coinjoin/server.cpp
|
🕓 Ready for review — 3 ahead in queue (commit 7af25a9) |
9171828 to
4b58e0f
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The prevout-based comparison addresses the previously reported collateral-txid malleability issue, but the newly added set is accessed concurrently with session resets without synchronization. This introduces undefined behavior and can leave stale prevouts after a timeout, so the synchronization issue must be fixed before merge; targeted regression coverage should also be added for the security-relevant admission behavior.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Validated blockers were found in the Codex precheck. Sonnet 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)
🔴 1 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:814-828: Synchronize the collateral prevout set with session resets
`ProcessDSACCEPT()` executes on the message-processing thread, while `Schedule()` invokes `CheckTimeout()` on the scheduler thread. A timeout clears `setSessionCollateralPrevouts` under `cs_coinjoin` in `SetNull()`, but the lookup and insertion here, as well as the insertion in `CreateNewSession()`, do not hold that lock. Concurrent `clear()` with `count()` or `insert()` on an `unordered_set` is undefined behavior. The mirrored state can also become inconsistent if a timeout clears the vector and set after `vecSessionCollaterals.push_back()` but before these insertions: this thread then repopulates the set after the session has been reset, causing a later session to reject valid collateral as `ERR_ALREADY_HAVE`. Protect the vector and set update as one invariant under `cs_coinjoin`, and revalidate the session ID and state while holding the lock so a reset cannot occur between the admission checks and the atomic update.
- [SUGGESTION] src/coinjoin/server.cpp:814-828: Add regression coverage for prevout-based duplicate admission
No unit or functional test exercises the security-relevant admission behavior added by this commit. Add a targeted test that establishes a session and verifies that both an identical collateral and a distinct transaction reusing any accepted prevout are rejected with `ERR_ALREADY_HAVE`, while collateral using disjoint inputs is accepted. The test should include multi-input overlap and verify that resetting the session allows previously used prevouts again, guarding against regressions to txid comparison and stale mirrored state.
| if (const auto it = std::ranges::find_if(dsa.txCollateral.vin, | ||
| [this](const CTxIn& txin) { return setSessionCollateralPrevouts.count(txin.prevout) > 0; }); | ||
| it != dsa.txCollateral.vin.end()) { | ||
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddUserToExistingSession -- collateral %s spends prevout %s already used by a session collateral, rejecting\n", | ||
| dsa.txCollateral.GetHash().ToString(), it->prevout.ToStringShort()); | ||
| nMessageIDRet = ERR_ALREADY_HAVE; | ||
| return false; | ||
| } | ||
|
|
||
| // count new user as accepted to an existing session | ||
|
|
||
| nMessageIDRet = MSG_NOERR; | ||
| vecSessionCollaterals.push_back(MakeTransactionRef(dsa.txCollateral)); | ||
| for (const auto& txin : dsa.txCollateral.vin) { | ||
| setSessionCollateralPrevouts.insert(txin.prevout); |
There was a problem hiding this comment.
🔴 Blocking: Synchronize the collateral prevout set with session resets
ProcessDSACCEPT() executes on the message-processing thread, while Schedule() invokes CheckTimeout() on the scheduler thread. A timeout clears setSessionCollateralPrevouts under cs_coinjoin in SetNull(), but the lookup and insertion here, as well as the insertion in CreateNewSession(), do not hold that lock. Concurrent clear() with count() or insert() on an unordered_set is undefined behavior. The mirrored state can also become inconsistent if a timeout clears the vector and set after vecSessionCollaterals.push_back() but before these insertions: this thread then repopulates the set after the session has been reset, causing a later session to reject valid collateral as ERR_ALREADY_HAVE. Protect the vector and set update as one invariant under cs_coinjoin, and revalidate the session ID and state while holding the lock so a reset cannot occur between the admission checks and the atomic update.
source: ['codex']
There was a problem hiding this comment.
Fixed in 9187f19. AddUserToExistingSession now takes cs_coinjoin before the duplicate-prevout check and holds it through the vecSessionCollaterals/setSessionCollateralPrevouts update, revalidating nSessionID and nState under the lock so a scheduler-thread SetNull() cannot interleave between the admission checks and the commit. CreateNewSession takes the lock around its mirrored update as well, and setSessionCollateralPrevouts is now GUARDED_BY(cs_coinjoin) so the invariant is enforced by thread-safety analysis (verified with -Wthread-safety, clean).
| if (const auto it = std::ranges::find_if(dsa.txCollateral.vin, | ||
| [this](const CTxIn& txin) { return setSessionCollateralPrevouts.count(txin.prevout) > 0; }); | ||
| it != dsa.txCollateral.vin.end()) { | ||
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddUserToExistingSession -- collateral %s spends prevout %s already used by a session collateral, rejecting\n", | ||
| dsa.txCollateral.GetHash().ToString(), it->prevout.ToStringShort()); | ||
| nMessageIDRet = ERR_ALREADY_HAVE; | ||
| return false; | ||
| } | ||
|
|
||
| // count new user as accepted to an existing session | ||
|
|
||
| nMessageIDRet = MSG_NOERR; | ||
| vecSessionCollaterals.push_back(MakeTransactionRef(dsa.txCollateral)); | ||
| for (const auto& txin : dsa.txCollateral.vin) { | ||
| setSessionCollateralPrevouts.insert(txin.prevout); |
There was a problem hiding this comment.
🟡 Suggestion: Add regression coverage for prevout-based duplicate admission
No unit or functional test exercises the security-relevant admission behavior added by this commit. Add a targeted test that establishes a session and verifies that both an identical collateral and a distinct transaction reusing any accepted prevout are rejected with ERR_ALREADY_HAVE, while collateral using disjoint inputs is accepted. The test should include multi-input overlap and verify that resetting the session allows previously used prevouts again, guarding against regressions to txid comparison and stale mirrored state.
source: ['codex']
There was a problem hiding this comment.
There is currently no test harness that can reach this path: no unit tests construct CCoinJoinServer (it needs live PeerManagerInternal/CConnman/CDeterministicMNManager/active-masternode wiring, and CreateNewSession/AddUserToExistingSession are private), and the functional framework has no dsa/DSACCEPT message support (test/functional/test_framework/messages.py has no CoinJoin P2P messages; the only coinjoin functional test is RPC-only). Building that harness is worthwhile but is a substantially larger change than this fix; I'd prefer to keep this security fix minimal and take regression coverage as a follow-up. The GUARDED_BY annotation added in 9187f19 does give compile-time enforcement of the new invariant in the meantime.
There was a problem hiding this comment.
Correction: this test-coverage finding is intentionally deferred, not resolved. No regression test was added in 9187f19; the current verifier accepted the maintainer's explanation that reaching the private DSACCEPT admission path requires substantial new unit-test or functional-P2P scaffolding. The thread remains available as follow-up context.
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta eliminates concurrent access to the mirrored prevout set and correctly revalidates admission to an existing session, but the carried-forward synchronization blocker remains in the first-collateral path: CreateNewSession() can still append collateral after a scheduler reset. No genuinely new latest-delta defect was confirmed; the regression-test request was explicitly deferred, and the commit-squash suggestion is a maintainer preference rather than an actionable defect.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Validated blockers were found in the Codex precheck. Sonnet 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)
🔴 1 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The final commit (4a68bfa) closes the last synchronization gap in CoinJoin session admission: CreateNewSession() now revalidates the idle-session precondition and commits nSessionID/nSessionDenom/POOL_STATE_QUEUE together with the first collateral's vector/set insertion inside a single cs_coinjoin critical section, mirroring the pattern already applied to AddUserToExistingSession() in the prior commit, and defers dsq signing/relay until after that atomic commit. I independently confirmed this at exact head via direct code inspection (lines 747-800 of src/coinjoin/server.cpp) and found no remaining race: SetNull() clears both containers under the same lock, and all mutation sites are consistently GUARDED_BY/EXCLUSIVE_LOCKS_REQUIRED annotated. No new correctness defects were found; the only substantive finding across both agents is a commit-history squash style suggestion, which I verified is contradicted by established project precedent (e.g. 7956e24, a standalone review-feedback fix commit kept before merge) and is not actionable.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); claude/general=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/general=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(completed); claude/general=claude-sonnet-5(completed); verifier=claude/final-verifier=claude-sonnet-5(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— general (completed)
4a68bfa to
1b72d2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 807-823: Update AddUserToExistingSession to acquire cs_coinjoin
immediately after IsAcceptableDSA(), then revalidate nSessionID, nState, and
nSessionDenom while holding the lock. Keep that lock through the
vecSessionCollaterals duplicate scan and push_back insertion, and apply the same
lock-and-revalidate invariant to the session-creation path.
🪄 Autofix (Beta)
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: df2b849c-a7b2-4973-9129-285563a463f5
📒 Files selected for processing (1)
src/coinjoin/server.cpp
CreateNewSession and AddUserToExistingSession run on the message-handler thread and mutate session state with no lock held, while the scheduler thread resets that same state every second via CheckTimeout -> SetNull() under cs_coinjoin. The check-then-mutate sequence in both functions is therefore not atomic, and the unlocked push_back into vecSessionCollaterals races SetNull()'s clear() outright. The window is not theoretical: IsAcceptableDSA sits between the state checks and the mutation, and it performs a mempool test-accept under cs_main. Take cs_coinjoin for the commit itself and revalidate the session state under it. The dsq signing and relay in CreateNewSession stay outside the lock scope, so cs_coinjoin is never held across BLS signing, network relay, or cs_main.
AddUserToExistingSession never checked whether the incoming dsa.txCollateral was already accepted into the current session; IsAcceptableDSA only validates the denomination and the collateral itself. A client that resends its dsa, or an attacker replaying an observed one, was pushed into vecSessionCollaterals again and counted as an extra participant, inflating the count that IsSessionReady compares against the min/max pool participant thresholds. Track the input prevouts of every committed collateral in setSessionCollateralPrevouts and refuse a dsa whose collateral reuses one of them, reporting the existing ERR_ALREADY_HAVE pool message. No new enum value is introduced, so there is no protocol change. Comparing collateral txids would not work. Session collaterals are only test-accepted against the mempool, never added to it, so nothing pins the transaction's identity: the same UTXO can be re-signed with a varied change output into arbitrarily many distinct txids. Prevout overlap closes that hole and strictly subsumes a txid comparison. Honest participants cannot collide, since distinct wallets cannot spend the same UTXO and a wallet locks its collateral inputs per session.
1b72d2c to
7af25a9
Compare
|
This pull request has conflicts, please rebase. |
d1fca89 fix(coinjoin): decide the pool state from one consistent snapshot (pasta) adb2765 fix(coinjoin): bound the sides of a post-V24 DSTX against each other (UdjinM6) 26d0a98 fix(coinjoin): tell participants when an uncovered session is reset (UdjinM6) 123858d fix(coinjoin): don't burn queue announcements a rebalance pass skips (UdjinM6) f232f02 fix(coinjoin): keep the V24 tip-skew tolerance at one block (pasta) e6687d9 fix(coinjoin): tolerate arbitrary tip skew when validating a final transaction (pasta) 3646c95 fix(coinjoin): tolerate DSTX tip skew from V24 lock-in, and address review feedback (pasta) 0dc132f docs: describe rebalance session gating and DSTX downgrade accurately (pasta) 5748316 refactor(coinjoin): use static_cast in new promotion/demotion code (pasta) e78b965 test(coinjoin): cover session finalization race and gap-threshold boundary (pasta) 6af461c fix(coinjoin): scale the rebalance gap threshold with the denoms goal (pasta) 428eb97 fix(coinjoin): recognize unbalanced mixing transactions as denominated (pasta) 032bc99 fix(coinjoin): widen the V24 DSTX skew tolerance and downgrade withheld DSTXes (pasta) 1ff74e9 fix(coinjoin): latch rebalance capability on admission, not on creator version (pasta) f7d2bbb fix(coinjoin): bind entry admission and charging to the validated session (pasta) 50c62c9 fix(coinjoin): annotate m_fRebalanceSession as GUARDED_BY(cs_coinjoin) (pasta) 9727337 fix(coinjoin): keep relaying islocks when withholding a DSTX from legacy peers (pasta) 88d4e6b fix(coinjoin): guard against null prevtx in GetRealOutpointCoinJoinRounds (pasta) 9e83d6a fix(coinjoin): don't announce oversized balanced DSTXes to legacy peers (pasta) 9cae6f4 feat(coinjoin): reset mixing rounds on promotion/demotion outputs (pasta) efff25c fix(coinjoin): only announce unbalanced DSTXes to peers that support them (pasta) 363d305 fix(coinjoin): tolerate one-block tip skew only at the V24 boundary (pasta) 9c29052 feat(coinjoin): gate rebalance sessions and require session-denom cover (pasta) 33702b4 docs: add release notes for pr 7052 (Pasta) 3d6d49a test: cover CoinJoin promotion/demotion validation and decision logic (Pasta) 49d7d5f feat: coinjoin promotion / demotion (Pasta) Pull request description: ## Summary This PR adds CoinJoin denomination promotion and demotion so participants can convert between adjacent standard denominations inside a mixing session instead of being limited to strict 1:1 denomination mixes. The new behavior is gated by V24 activation. Pre-V24 behavior remains unchanged. ## What was done? - added promotion support for 10 smaller-denomination inputs to 1 next-larger-denomination output - added demotion support for 1 larger-denomination input to 10 next-smaller-denomination outputs - updated CoinJoin client, server, wallet, and validation flow to build, accept, and verify promotion/demotion entries - updated DSTX structural validation so post-V24 sessions can accept valid unbalanced promo/demo transactions while preserving pre-V24 1:1 rules; input and output counts are capped independently post-V24 - enforced a side-coverage invariant before a session may complete: each side of the session denomination must be occupied by nobody or by at least two participants, since coins are only concealed by other coins of the same size on the same side. A lone promoter or demoter on a side would have the only coins of that size there and be trivially identifiable on-chain. Note that this permits rebalancers to cover each other (e.g. two promoters and no standard mixers); there is deliberately no separate standard-mixer minimum. Sessions whose received entries can no longer cover both sides reset immediately instead of stalling until timeout - rebalance inputs are locked when selected and released on every failure path (queue-join failure, connect failure, entry-preparation failure, session reset) - pre-V24, unbalanced DSVIN entries keep flowing to `AddEntry` → `IsValidInOuts` so their collateral is consumed as before (anti-spam behavior preserved) - conversions only spend fully-mixed coins (both directions; demotion's ready-to-mix fallback removed) and their outputs start mixing over at 0 rounds: the conversion's public 10:1 shape clusters one participant's coins even inside a mixing transaction, so a converted coin is not treated as mixed — it re-enters mixing at its new denomination and disperses normally, while the histories of the fully-mixed coins that fed it remain protected. Implemented in `GetRealOutpointCoinJoinRounds` (0 rounds for an output whose own inputs in the same tx are at a different denomination); the rule is inert pre-V24 and needs no activation gating - extracted the final-transaction aggregate composition check into `CoinJoin::ValidateFinalTxComposition()` and covered it directly in unit tests - expanded unit coverage around structure validation, expiry logic, promotion/demotion entry validation, final-tx composition, and standard-entry privacy checks ### Protocol-version gating of rebalance sessions - bumped `PROTOCOL_VERSION` to 70241 and added `COINJOIN_REBALANCE_VERSION`; the `dsa` message gained a flags field declaring promotion/demotion intent, serialized only between peers that both negotiated ≥ 70241, so the wire format toward older peers is byte-identical and DSQ messages are untouched - each mixing session's rebalance capability is fixed at creation from the creator's negotiated protocol version; older clients are rejected from rebalance-capable sessions at `dsa` time with `ERR_VERSION` (a message ID old releases already understand, delivered before any collateral is committed) — this prevents pre-70241 wallets from ever facing an unbalanced final transaction they cannot validate, which they would refuse to sign at the risk of losing their collateral to `ChargeFees`. Old clients keep mixing in sessions created by old peers, which new clients still join for standard mixing - the masternode records the direction each participant declares in its `dsa`, and `IsSessionReady` holds the session in queue until the declared shapes cover both sides of the session denomination; a session that never attracts the missing counterparty times out fee-free in queue state. Because admission relies on the declarations, every entry must match its participant's declared direction exactly — a deviating entry (e.g. declaring a promotion, then submitting a standard entry, which could strip a side of its declared cover and force a fee-free reset for everyone) has its collateral consumed - clients refuse to sign a post-V24 final transaction without sufficient foreign cover at the session denomination on whichever side they occupy, preventing a malicious masternode from finalizing a pool that would publicly link a promotion participant's 10 fully-mixed inputs to a single output - final-tx validation on the client tolerates a masternode whose tip is one block ahead at the V24 activation boundary (it also accepts V24 activating in the block following the local tip), so tip skew at the boundary cannot cost an honest client its collateral; per-entry validation on the masternode keeps using the strict tip state - unbalanced (promotion/demotion) DSTXes are only announced to peers at protocol ≥ 70241: pre-70241 software treats them as structurally invalid, drops them and penalizes the relayer by 10 per DSTX, which would gradually get honest relayers discouraged by old peers — and the zero-fee transaction can't enter old mempools anyway. Older peers see the transaction on block inclusion instead. Balanced DSTXes keep relaying to everyone, so standard mixes retain their zero-fee propagation ## History The branch is rebased onto current `develop` (#7507, which it previously depended on, has since merged, so its commit is no longer carried here). Commits: feature, tests, release notes, and protocol-version gating from earlier review rounds, plus follow-ups from review: declared-direction enforcement with collateral consumption, activation-boundary tip-skew tolerance, release-note clarifications, version-gated announcement of unbalanced DSTXes, promotion input selection aligned with the standard selector (spendable-only, shuffled, at most one coin per parent transaction — so a demotion's 10 sibling outputs are never promoted together as an identifiable group), `nFlags` in `CCoinJoinAccept` equality, and the rounds reset for conversion outputs described above. ## How Has This Been Tested? - `./src/test/test_dash --run_test=coinjoin_inouts_tests` (including coverage for version-gated `dsa` serialization, mix-shape classification, and the side-coverage invariant), `--run_test=coinjoin_tests`, and `--run_test=net_tests` pass - new `coinjoin_rebalance_rounds_reset_tests` in the wallet suite exercises `GetRealOutpointCoinJoinRounds` directly: standard 1:1 mixing advances rounds, promotion/demotion-shaped transactions reset their outputs to 0, and a promoted coin advances normally when re-mixed - `dashd` and `test_dash` build cleanly - lint: whitespace, logs, format strings, circular dependencies, python pass - `P2P_VERSION` in the functional-test framework is bumped in lockstep with `PROTOCOL_VERSION`, so functional tests can exercise the new gating; an old client is now cleanly emulatable with a 70240-advertising `P2PInterface` Post-V24 activation behavior (including the server-side admission/entry-enforcement paths) still needs functional coverage because EHF activation paths cannot be fully exercised in these unit tests alone. ## Breaking Changes None. The feature is activation-gated and preserves existing pre-V24 behavior. The protocol version bump to 70241 is backward compatible: older peers keep the previous `dsa` wire format and are only excluded from sessions that could contain entries they cannot validate. Top commit has no ACKs. Tree-SHA512: bf0cedcd19e9ea7ffd3e4e8e371d5124c6f5d3f5565ddeaf515aa57caf8237bee74cef408b8e1b48659f9980418ea86cd1c271cd77836c3d6a35b17bf32724a2
Issue being fixed or feature implemented
CCoinJoinServer::AddUserToExistingSessionnever checks whether the incomingdsa.txCollateralwas already accepted into the current session —IsAcceptableDSAonly validates the denomination and the collateral itself. A client that resends itsdsa(or an attacker replaying an observed one) ispush_back'd intovecSessionCollateralsagain and counted as an extra participant. This inflates the count thatIsSessionReadycompares against the min/max pool participant thresholds, so a session can look "ready" with fewer real participants than required. Each phantom slot expects a DSVIN entry that never arrives, so such sessions stall until timeout, andChargeFeesmay then consume the replayed collateral — but the participant-count inflation itself is wrong regardless.Committing the rejection requires a lock, and taking one exposes a second, pre-existing defect.
CreateNewSessionandAddUserToExistingSessionrun on the message-handler thread and mutate session state with no lock held, while the scheduler thread resets that same state every second viaCheckTimeout→SetNull()undercs_coinjoin(Schedule, 1s interval). The check-then-mutate sequence in both functions is therefore not atomic, and the unlockedpush_backintovecSessionCollateralsracesSetNull()'sclear()outright. The window is not theoretical:IsAcceptableDSAsits between the state checks and the mutation and performs a mempool test-accept undercs_main.What was done?
Two commits.
fix: commit CoinJoin session state and its collateral atomicallytakescs_coinjoinfor the commit itself in both functions and revalidates the session state under it, so the state and the collateral that goes with it land as one unit. Thedsqsigning and relay inCreateNewSessionstay outside the lock scope, socs_coinjoinis never held across BLS signing, network relay, orcs_main. Adsathat loses this race is now answered withERR_MODEinstead of being committed into a session that no longer exists.fix: reject duplicate session collateral in CoinJoin dsa acceptancetracks the input prevouts of every committed collateral insetSessionCollateralPrevoutsand refuses adsawhose collateral reuses one of them, reporting the existingERR_ALREADY_HAVEpool message ("Already have that input."). No new enum value is introduced, so there is no protocol change; old clients handle it as an ordinarydssurejection.CommitSessionCollateralis the single place that appends tovecSessionCollateralsand indexes its prevouts, so the two cannot drift apart.The obvious check — comparing collateral txids — is insufficient and deliberately not used. Session collaterals are only ever test-accepted against the mempool, never actually added to it, so nothing pins the transaction's identity: a malicious client can re-sign the same collateral UTXO with a varied change output (or just a different signature, since collaterals are malleable pre-broadcast) and produce arbitrarily many distinct txids for what is economically the same collateral. Every one of those variants would sail past a
GetHash()comparison and the participant inflation would survive. Prevout overlap closes that hole: no matter how the transaction is re-signed, it must spend the same UTXO, and identical transactions necessarily share prevouts, so the prevout check strictly subsumes the txid check.Honest participants can never collide on a prevout: distinct wallets cannot spend the same UTXO, and a wallet mixing in multiple sessions locks its collateral inputs per session, so each concurrent session gets a collateral built from disjoint inputs.
Scope note:
vecSessionCollateralsitself still carries noGUARDED_BYand is read withoutcs_coinjoininProcessDSACCEPT,IsSessionReady,CheckPool,CheckForCompleteQueue,ChargeFees,ChargeRandomFeesandAddEntry. Every write to it now holds the lock, so the new prevout index cannot drift from it, but those unlocked reads remain a pre-existing race. Annotating the member and repairing all seven call sites — along withSetState, which writesnStateunlocked, andnSessionDenom, which is neither atomic nor guarded — is left to a follow-up PR so this one stays reviewable.How Has This Been Tested?
src/coinjoin/server.cppwithclang++ -fsyntax-only -Wthread-safetyusing the project'scompile_commands.jsonflags, clean.AddUserToExistingSessionrequires drivingProcessDSACCEPT, which needs the server's active masternode registered in the deterministic MN list at chain tip plus a mempool-valid collateral transaction (thefUnitTestbypass is private and never enabled anywhere). None of that scaffolding exists in the current CoinJoin unit tests (src/test/coinjoin_inouts_tests.cpponly reaches the DSSIGNFINALTX path), and building it out is beyond the scope of this fix.Breaking Changes
None. The rejection reuses an existing pool message code, so wire compatibility is unchanged; only
dsamessages whose collateral reuses an input prevout already committed to the session, or which race a session reset, are now refused.Checklist: