fix: reject duplicate session collateral in CoinJoin dsa acceptance - #7507
fix: reject duplicate session collateral in CoinJoin dsa acceptance#7507PastaPastaPasta wants to merge 2 commits into
Conversation
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:
|
|
Warning Review limit reached
Next review available in: 1 minute 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)
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 |
![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: 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 👍 / 👎.
![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/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
|
⛔ Blockers found — Sonnet deferred (commit 9187f19) |
32715dd to
9171828
Compare
AddUserToExistingSession never checked whether the incoming dsa.txCollateral was already accepted into the current session. A client (or an attacker replaying a client's dsa) could therefore be added to vecSessionCollaterals multiple times and be counted as multiple participants, inflating the count IsSessionReady compares against the min/max pool participant thresholds. A session could then look ready with fewer real participants than required; each phantom slot expects a DSVIN entry that never arrives, so such sessions stall until timeout and ChargeFees may consume the replayed collateral. Comparing collateral txids is not sufficient: collaterals are only ever test-accepted, never added to the mempool, so a malicious client can re-sign the same collateral UTXO with a varied change output or signature and produce arbitrarily many distinct txids for what is economically the same collateral, bypassing any hash comparison. Instead, maintain a set mirroring the input prevouts of every accepted session collateral - populated when a collateral enters the session in CreateNewSession or AddUserToExistingSession, cleared alongside vecSessionCollaterals in SetNull - and reject a dsa whose collateral spends any prevout in it; identical transactions necessarily share prevouts, so this 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. Rejected with ERR_ALREADY_HAVE, an existing pool message, so no protocol change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9171828 to
4b58e0f
Compare
thepastaclaw
left a comment
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.
…esets CheckTimeout runs on the scheduler thread and clears vecSessionCollaterals and setSessionCollateralPrevouts under cs_coinjoin in SetNull, but ProcessDSACCEPT mutated both from the message-processing thread without the lock: concurrent clear() and count()/insert() on the unordered_set is undefined behavior, and a timeout landing between the admission checks and the insertions could repopulate the set after the session was reset, poisoning a later session into rejecting valid collateral with ERR_ALREADY_HAVE. Take cs_coinjoin around the duplicate-prevout check and the vector/set update as one atomic block in AddUserToExistingSession, revalidating nSessionID and nState under the lock so a reset cannot interleave, and around the mirrored update in CreateNewSession. Annotate setSessionCollateralPrevouts GUARDED_BY(cs_coinjoin) so thread-safety analysis enforces this. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
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.
2 participants
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.What was done?
Added an early rejection in
AddUserToExistingSession: adsais refused when any input prevout of its collateral already appears among the input prevouts of a transaction invecSessionCollaterals, using 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.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.
How Has This Been Tested?
src/coinjoin/server.cppwithclang++ -fsyntax-onlyusing the project'scompile_commands.jsonflags.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 are now refused.Checklist: