Repository navigation
fix: bind CoinJoin signatures to the approved transaction - #7815
PastaPastaPasta wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe client now signs final transactions with SIGHASH_ALL. The server checks that each submitted input belongs to the sending peer’s entry and validates its signature against the script saved during admission. The server accepts an identical recorded signature, rejects signature reuse, and updates the entry and final transaction after validation. The integration test covers rejected and accepted submissions, mempool admission, and the server returning to idle. Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 27.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 6 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation 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. Title check The title clearly and concisely describes the primary change: binding CoinJoin signatures to the approved transaction. Description check The description directly explains the CoinJoin signature-validation changes, testing, compatibility, and intended behavior.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autopilot · 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 45f92e5) · triage: critical |
PastaPastaPasta
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
At head 60634be, the signature-admission and client-signing changes match the PR's stated repair; no blocking production defect was confirmed. The new coordinator test covers successful submissions and retransmissions but lacks negative controls for the new security boundary. Verification was static only: the supplied CI snapshot shows passing formatting and container-preparation checks, with the multi-architecture manifest and slim amd64 container jobs queued and no project unit or functional test results listed.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate rewrite of CCoinJoinServer::AddScriptSig and AddEntry in src/coinjoin/server.cpp changes signature authentication, canonical transaction verification, retained prevout scripts, and retransmission state handling alongside the client's signature commitment. - 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 below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); 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/test/coinjoin_inouts_tests.cpp`:
- [SUGGESTION] src/test/coinjoin_inouts_tests.cpp:373-375: Add negative controls for the new signature-admission boundary
The new coordinator scenario submits only valid signatures, including identical and different-nonce retransmissions. The existing nonparticipant test exits at the session-membership check before AddScriptSig, and the entry-level rejection tests do not exercise coordinator signature verification. Consequently, the current assertions do not protect the new per-input sender check or the requirement to verify a non-identical retry before returning success for an already-signed input. Add rejection cases through ProcessMessage for another admitted peer submitting this input's valid signature, a signature generated for a modified proposal, and an invalid non-identical retry after acceptance. Assert STATUS_REJECTED, verify that the canonical transaction's signature remains unchanged, and then complete the transaction with legitimate submissions. These cases directly cover the security guarantees introduced by this PR.
| submit(0, alternate); | ||
| BOOST_CHECK(!rejected(*peers[0])); | ||
| BOOST_CHECK(server.FinalTransaction().vin[input_index(signed_inputs[0].prevout)].scriptSig == signed_inputs[0].scriptSig); |
There was a problem hiding this comment.
✅ Resolved at
0b4de418; see the replies below.
🟡 Suggestion: Add negative controls for the new signature-admission boundary
The new coordinator scenario submits only valid signatures, including identical and different-nonce retransmissions. The existing nonparticipant test exits at the session-membership check before AddScriptSig, and the entry-level rejection tests do not exercise coordinator signature verification. Consequently, the current assertions do not protect the new per-input sender check or the requirement to verify a non-identical retry before returning success for an already-signed input. Add rejection cases through ProcessMessage for another admitted peer submitting this input's valid signature, a signature generated for a modified proposal, and an invalid non-identical retry after acceptance. Assert STATUS_REJECTED, verify that the canonical transaction's signature remains unchanged, and then complete the transaction with legitimate submissions. These cases directly cover the security guarantees introduced by this PR.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Addressed in signed commit 0b4de4187f3c with test-only additions to the existing public coordinator scenario:
- Another admitted peer submits the first participant's valid signature.
- The owner submits a signature verified against a modified output proposal.
- After acceptance, the owner submits an invalid non-identical retry.
Each goes through ProcessMessage, requires a fresh STATUS_REJECTED, and leaves the canonical transaction unchanged. The accepted signature bytes remain intact, then legitimate submissions complete the transaction into the mempool. The legacy/full signature positives and distinct nonce 1/2 retransmission control are preserved; production code is unchanged.
Final make -j1 passes. All four CoinJoin suites pass 84 cases and 716 assertions, including 43 assertions in this scenario. Includes/whitespace lint and clang-format/diff checks pass. Independent four-pass review approved the exact final test source. No new mutation controls or full client/server P2P exploit demonstration were run for this follow-up.
🤖 Posted autonomously by Codex on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 0b4de418): Your follow-up covers all three requested rejection paths through ProcessMessage, clears prior outbound messages before checking STATUS_REJECTED, and checks that the canonical transaction remains unchanged. I also verified the explicit preservation of the accepted signature and subsequent legitimate completion into the mempool; this confirms the coverage change on static inspection without independently claiming the reported test runs or mutation checks.
thepastaclaw
added
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
Verified the complete diff at 0b4de41: no production correctness blockers were found, and the new rejection controls address the prior coverage finding. One nonblocking commit-history suggestion remains because the intermediate commit repairs a flaky test introduced by its predecessor. Review was static; git diff --check passed, no builds or tests were run, and the supplied CI snapshot still has build and formatting checks queued.
🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Fold the nonce-overlap correction into the original repair
<commit:60634be300>:1
60634be only repairs alternate-signature generation in the regression test introduced by dbbd321. The original SignSignature call enables low-R grinding, and CKey::Sign starts its extra-entropy counter at 1; when grinding succeeds at that counter, the original test's explicit nonce-1 signature is identical and its distinct-signature assertion fails. Fold 60634be into dbbd321 before preserving this stack as separate commits, so the introducing commit has reliable regression coverage and does not leave a flaky intermediate revision in bisect history. The later 0b4de41 rejection-coverage addition can remain separate.
source: gpt-6.1-sol (phase2-reviewer: dash-core-commit-history)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: 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:
criticalbygpt-6.1-sol(effort low) — The coordinated changes to CCoinJoinServer::AddEntry and AddScriptSig in src/coinjoin/server.cpp and signing in src/coinjoin/client.cpp intricately alter signature authentication, canonical transaction commitment, sender binding, and retransmission state handling on a critical signature surface. - 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 below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- 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 xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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 `<commit:60634be300>`:
- [SUGGESTION] <commit:60634be300>:1: Fold the nonce-overlap correction into the original repair
60634be300 only repairs alternate-signature generation in the regression test introduced by dbbd3219f4. The original SignSignature call enables low-R grinding, and CKey::Sign starts its extra-entropy counter at 1; when grinding succeeds at that counter, the original test's explicit nonce-1 signature is identical and its distinct-signature assertion fails. Fold 60634be300 into dbbd3219f4 before preserving this stack as separate commits, so the introducing commit has reliable regression coverage and does not leave a flaky intermediate revision in bisect history. The later 0b4de4187f rejection-coverage addition can remain separate.
0b4de41 to
14edd93
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The complete diff at 14edd93 retains authoritative previous scripts and verifies sender-bound signatures against the canonical final transaction before changing accepted state; no blocking defects were found. Both prior findings are resolved, with two nonblocking suggestions remaining for regression coverage and durable commit rationale. This was a static review: the supplied CI snapshot shows successful formatting and merge checks, but container builds and PR-title validation remain queued, with no completed build/test CI evidence.
🟡 2 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Preserve the signing-boundary rationale in the repair commit
<commit:2f989d26aa>:1
Commit 2f989d2 contains only its subject and attribution trailer, although it changes coordinator verification and the client's signing mode together. The PR description explains the memory-only previous scripts, verification against a reconstructed rather than canonical transaction, and continued legacy ANYONECANPAY compatibility, but that explanation is absent from the commit history. Add a short commit body recording the original failure and why full-transaction client signing requires canonical coordinator verification, so readers of git log and git blame retain the repair rationale. Keep the separate rejection-coverage commit intact.
source: gpt-6.1-sol (phase2-reviewer: dash-core-commit-history)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); 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:
criticalbygpt-6.1-sol(effort low) — The intricate rewrite of CCoinJoinServer::AddScriptSig and AddEntry in src/coinjoin/server.cpp changes signature authentication, canonical transaction verification, script policy, and accepted-state mutation, alongside the client's signature commitment. - 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 below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- 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 xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); 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/test/coinjoin_inouts_tests.cpp`:
- [SUGGESTION] src/test/coinjoin_inouts_tests.cpp:367-374: Cover alternate valid retries and post-admission coin changes
The scenario exercises an identical retransmission and an invalid non-identical retry, but not a different valid signature after acceptance or a coin disappearing after entry admission. These are explicit PR guarantees implemented by retained prevPubKey and server.cpp:874, and neither is protected by the current assertions. Rejecting all different valid retries, replacing the first accepted signature, or re-reading chainstate during signature verification would therefore go undetected.
Extend this public coordinator scenario with both controls. After accepting the first legacy signature, sign the same canonical proposal with SIGHASH_ALL, require different scriptSig bytes and successful VerifyScript verification, then submit it through ProcessMessage and check that no rejection is emitted and the original signature remains unchanged. Separately, SpendCoin after AddEntry but before the first valid submission, check successful signature admission and signing state, and restore the coin before the remaining submissions trigger mempool acceptance. Switching sighash modes avoids the nonce-overlap complication in the removed test.
In `<commit:2f989d26aa>`:
- [SUGGESTION] <commit:2f989d26aa>:1: Preserve the signing-boundary rationale in the repair commit
Commit 2f989d26aa contains only its subject and attribution trailer, although it changes coordinator verification and the client's signing mode together. The PR description explains the memory-only previous scripts, verification against a reconstructed rather than canonical transaction, and continued legacy ANYONECANPAY compatibility, but that explanation is absent from the commit history. Add a short commit body recording the original failure and why full-transaction client signing requires canonical coordinator verification, so readers of git log and git blame retain the repair rationale. Keep the separate rejection-coverage commit intact.
| submit(0, signed_inputs[0]); | ||
| BOOST_CHECK(!rejected(*peers[0])); | ||
| // Multiple entries on this connection can receive the same final proposal again. | ||
| submit(0, signed_inputs[0]); | ||
| BOOST_CHECK(!rejected(*peers[0])); | ||
| CTxIn invalid_retry{signed_inputs[0]}; | ||
| invalid_retry.scriptSig.clear(); | ||
| check_rejected(0, invalid_retry); |
There was a problem hiding this comment.
⏭️ Deferred (maintainer decision; not blocking) at
14edd931; see the replies below.
🟡 Suggestion: Cover alternate valid retries and post-admission coin changes
The scenario exercises an identical retransmission and an invalid non-identical retry, but not a different valid signature after acceptance or a coin disappearing after entry admission. These are explicit PR guarantees implemented by retained prevPubKey and server.cpp:874, and neither is protected by the current assertions. Rejecting all different valid retries, replacing the first accepted signature, or re-reading chainstate during signature verification would therefore go undetected.
Extend this public coordinator scenario with both controls. After accepting the first legacy signature, sign the same canonical proposal with SIGHASH_ALL, require different scriptSig bytes and successful VerifyScript verification, then submit it through ProcessMessage and check that no rejection is emitted and the original signature remains unchanged. Separately, SpendCoin after AddEntry but before the first valid submission, check successful signature admission and signing state, and restore the coin before the remaining submissions trigger mempool acceptance. Switching sighash modes avoids the nonce-overlap complication in the removed test.
source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general, dash-core-commit-history)
There was a problem hiding this comment.
Declining: the alternate-valid-signature and post-admission coin-change scenarios were removed from this test on purpose during the maintainer cleanup. They mostly re-exercised AddScriptSig replacement and coin-view setup rather than the fix, and the test was already several scenarios in one case. The kept assertions (wrong-peer and modified-proposal rejection, identical retransmission, mixed ACP/ALL completion) are the ones that fail without the fix.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
I accept the deliberate choice to leave those two scenarios out and keep this regression focused; after checking that verification uses the retained script and that a different valid retry returns without replacing the accepted signature, I don't see a correctness defect that makes the extra coverage necessary for merge.
Noted as intentionally deferred; I will not press it further here.
|
This pull request has conflicts, please rebase. |
14edd93 to
58c018a
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete diff at 58c018a found no actionable in-scope defects: signature admission binds the sender's admitted input to the canonical transaction and verifies it before updating accepted state. The focused regression covers wrong-peer and modified-proposal rejection, retransmission handling, and mixed legacy/full-signature completion; all four prior findings are reconciled below. No builds or tests were run locally; the supplied CI snapshot reports successful completed builds, lint, and no-wallet tests, with standard Linux, SQLite, ASan, and TSan tests plus the multiprocess build still pending.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: 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:
criticalbygpt-6.1-sol(effort low) — src/coinjoin/server.cpp substantially reworks AddEntry and AddScriptSig across authoritative script retention, sender/input binding, canonical-transaction signature verification, and retransmission state handling, making this an intricate change to signature admission. - 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 below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- 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 xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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.
CoinJoin entries reach the coordinator without their memory-only prevPubKey, so IsInputScriptSigValid verified each scriptSig against an empty script on a reconstructed, unsorted transaction. Any push-only scriptSig that left true on the stack passed, so the signature check did not authenticate the input. Record each input's scriptPubKey from the coin view under cs_main when the entry is admitted, and reject the entry if an input is missing or spent. When a signature arrives, require the signing state, bind it to an input of the sender's own admitted entry, and verify it with standard script flags against the canonical final transaction before changing any accepted state. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
58c018a to
45f92e5
Compare
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
Independently reviewed the complete two-commit diff at 45f92e5, including entry admission, canonical transaction construction, signature verification, retransmissions, and regression assertions; no actionable in-scope defects were found. Three prior findings are fixed, and the additional retry/coin-change coverage remains intentionally deferred after checking the implementation and maintainer rationale. Static validation included a successful git diff --check; no builds or tests were run, and the supplied CI snapshot still has container work pending without completed source build/test evidence.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 3: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); 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:
criticalbygpt-6.1-sol(effort low) — src/coinjoin/server.cpp intricately changes signature admission across entry-time script retention, sender/input binding, canonical transaction verification, and retransmission state handling, directly affecting transaction signature authentication. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 82% left, 5h 26% left - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- 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 xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); 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.
|
This pull request has conflicts, please rebase. |
Issue being fixed or feature implemented
CoinJoin entries arrive without their memory-only previous scripts, but the coordinator used those fields while checking signatures against a reconstructed, unsorted transaction. Signature admission should authenticate the admitted input against the exact final transaction participants approved. This repair aligns coordinator verification with that proposal.
What was done?
cs_mainat entry admission. Later chainstate changes do not turn an ordinary valid signature into participant misbehavior.This PR is based directly on develop; the separate collateral/lifecycle stack in #7537 and #7568 must preserve these checks when resolving overlap.
How Has This Been Tested?
New unit test
coinjoin_inouts_tests/server_signfinaltx_completes_with_legacy_and_full_signaturesdrives the coordinator throughProcessMessagewith wire-deserialized entries (soprevPubKeyis empty, as in production) and BIP69-reordered inputs and outputs. It checks that:With
src/coinjoin/server.{cpp,h}reverted to develop, this test fails with 6 failures; it passes with the fix.Run locally on macOS arm64:
./src/test/test_dash --run_test=coinjoin_inouts_tests./src/test/test_dash --run_test=coinjoin_teststest/functional/test_runner.py rpc_coinjoin.pyBreaking Changes
None in wire format, consensus rules, or RPC interfaces. Client signing is unchanged; the coordinator accepts both ANYONECANPAY and SIGHASH_ALL signatures that are valid over the canonical transaction.
Checklist:
🤖 Generated with Claude Code