Repository navigation
fix: reject invalid-denomination DSQUEUEs on mixing clients by sharing the server validation - #7955
PastaPastaPasta wants to merge 3 commits into
Conversation
CCoinJoinServer::ProcessDSQUEUE and CJWalletManagerImpl::ProcessDSQueue each carried their own copy of the DSQUEUE checks, and the copies have drifted apart. Move the checks that do not depend on the receiving role into CoinJoin::ValidateDSQueue: denomination, masternode lookup, time bounds and the operator signature. The function returns the signing masternode and the score to penalize the sending peer with. The duplicate check is a callback, so each role keeps its own queue storage and locking, and duplicates are still dropped before the BLS signature is verified. The mixing-rate threshold stays in the callers, because the server and the client apply it to different queues. No behavior change: the server runs the same checks in the same order with the same penalties. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The server started rejecting DSQUEUE messages with an invalid denomination (misbehaving 10) in dashpay#6865, but the wallet-side handler never got that check. A client accepted a signed DSQUEUE with a bogus denomination into its queue, where it matched no session and only took up a slot. Validate client DSQUEUEs with CoinJoin::ValidateDSQueue, the checks the server already uses, so the client rejects such queues with the same penalty. The client keeps its own duplicate tracking under cs_ProcessDSQueue, the ready-queue submission and the mixing-rate threshold. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
PastaPastaPasta
changed the title
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title names the main change: invalid-denomination DSQUEUEs are now rejected on mixing clients by sharing the server's validation. The raw summary confirms the client wallet handler now calls the s… Description check The description explains the problem (mixing clients accept DSQUEUEs with invalid denominations), the refactor into CoinJoin::ValidateDSQueue, the server and client call sites, intentional remaining d… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the main change: invalid-denomination DSQUEUEs are now rejected on mixing clients by sharing the server's validation. The raw summary confirms the client wallet handler now calls the shared CoinJoin::ValidateDSQueue, and the objectives describe the same penalty-10 behavior. The title is specific and matches the changeset.
Full details: Description check
-
Explanation
The description explains the problem (mixing clients accept DSQUEUEs with invalid denominations), the refactor into CoinJoin::ValidateDSQueue, the server and client call sites, intentional remaining differences, and the new test coinjoin_queue_tests/client_rejects_invalid_queues. This matches the changeset described in the summaries.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 7 functions across 5 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
|
✅ Final review complete — no blockers (commit 0039926) · triage: normal |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The DSQUEUE validation refactor preserves the server’s validation order, penalties, duplicate handling, and client rejection behavior. Two non-blocking issues remain: the server loses peer attribution in the invalid-denomination log, and the regression test does not cover the documented ready-queue case. AMD64 builds and ClangFormat passed; ARM64 builds are still in progress, while the failed PR-title check is unrelated to the code changes.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a contained peer-to-peer validation behavior change with shared logic refactoring and focused tests, but it does not alter consensus, funds, cryptographic algorithms, or network deserialization. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 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/coinjoin.cpp`:
- [NITPICK] src/coinjoin/coinjoin.cpp:775-777: Shared log drops peer id for invalid-denomination DSQUEUEs
Before extraction, the server logged the invalid denomination together with the relaying peer id. The shared helper now logs only the denomination, so server diagnostics no longer identify which peer sent the malformed queue. This does not change the misbehavior score, but it removes useful attribution when investigating or correlating misbehavior. Preserve the peer id for the server call, either by passing it into the helper or by adding server-side logging at the call site.
In `src/test/coinjoin_queue_tests.cpp`:
- [SUGGESTION] src/test/coinjoin_queue_tests.cpp:370: No ready-queue regression case for invalid denominations
The PR documents that an invalid-denomination ready queue must be rejected before the client attempts to submit it to a session, but the regression test only constructs non-ready queues because MakeQueue hardcodes fReady=false. Add a ready invalid-denomination case to verify the same penalty and storage rejection on the path whose behavior is explicitly called out by the PR.
thepastaclaw
added
the
pastaclaw:commented
Co-Authored-By: Claude Opus 5.5 <[email protected]>
thepastaclaw
removed
the
pastaclaw:commented
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The shared DSQUEUE validator preserves the server's validation order, duplicate handling, timeout behavior, masternode lookup, signature checks, and penalties while extending denomination validation to wallet clients. The regression test now covers both non-ready and ready invalid-denomination queues, along with other invalid and valid cases. No in-scope correctness issues were found; build and lint checks for this exact head were still in progress in the supplied CI snapshot.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a contained but cross-cutting peer-facing CoinJoin message-validation change with shared validation, penalty handling, and queue-state behavior, but it does not alter consensus, funds movement, cryptographic key handling, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Potential PR merge conflictsThis is advisory only. It does not block CI. This PR and each open PR below change the same files and cannot be merged together cleanly, so whichever of the two merges second will likely need a rebase:
|
Issue being fixed or feature implemented
Mixing clients accept a signed DSQUEUE with an invalid denomination into their queue. Masternodes have rejected such queues (misbehaving 10) since #6865, but that check was added only to the server's DSQUEUE handler. The wallet-side handler keeps its own copy of the same validation, and the two copies have drifted apart. The bogus queue matches no session, but it takes up a queue slot, and it counts toward the masternode's mixing-rate threshold.
What was done?
refactor: moved the DSQUEUE checks that both roles share into
CoinJoin::ValidateDSQueue(src/coinjoin/coinjoin.{h,cpp}):It returns the signing masternode and the score to penalize the peer with.
CCoinJoinServer::ProcessDSQUEUEnow uses it. The server's checks, order and penalties are unchanged.fix:
CJWalletManagerImpl::ProcessDSQueueuses the same function, so clients now reject invalid denominations with the same penalty as the server (10).The duplicate check is passed in as a callback. Each role keeps its own queue storage and locking, and duplicates are still dropped before the BLS signature is verified. Each caller applies the returned penalty its own way (
PeerMisbehavingon the server,MisbehavingErroron the client).Remaining differences between the two handlers, kept on purpose and not changed here:
AllowMixing, only to non-ready queues, and never stores or relays ready queues. The client first tries to submit to its session for a ready queue. If no session is waiting on that masternode, it applies the threshold and stores the ready queue like any other. So on clients, ready queues count toward the threshold. This is long-standing behavior. It may deserve its own look, but this PR doesn't change it.TryCheckDuplicateunderTRY_LOCKand drops the message silently on lock contention. The client takes a blocking lock, checksHasQueue(hash)thenHasQueueFromMasternode, and holdscs_ProcessDSQueueuntil it has stored the queue.Side effects of the client change:
masternodeOutpointis not serialized, so in practice every DSQUEUE is resolved throughm_protxHash. The collateral lookup and protx fill-in after it can't fail for network input. I kept them so the server code moves over line for line.How Has This Been Tested?
coinjoin_queue_tests/client_rejects_invalid_queues:CJWalletManager::processMessage.coinjoin_queue_testspasses on both.--enable-debug:coinjoin_queue_tests,coinjoin_inouts_tests,coinjoin_dstxmanager_tests,coinjoin_testsrpc_coinjoin.py,p2p_dstx.pylint-circular-dependencies.py,lint-includes.py,lint-whitespace.pyBreaking Changes
None.
Checklist:
🤖 Generated with Claude Code