Repository navigation
p2p_core%feat!: type senddsq, qsendrecsigs, sendcmpct, sendtxrcncl, ssc and getblocks - #74
Conversation
PastaPastaPasta
added
the
topic:P2P
|
Note This pull request has no conflicts! 🎊 🎉 🎊 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the six P2P message types that the pull request adds typed payloads for. The p2p_core%feat! prefix is noisy but does not prevent understanding the main change. |
| 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. |
| Description check | |
The pull request description directly explains the typed payload changes for six P2P messages, the reference implementation behavior, tests, breaking changes, and validation performed. |
- 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.
fa797da to
f813760
Compare
|
🕓 Review not started yet because the new head is waiting for the 30-minute push debounce.
Commit b713938. Normal review starts when eligible; priority review starts as soon as a slot is available. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against head f813760. The six payload layouts match the pinned Dash Core readers, and no blocking defect was found; one non-blocking suggestion remains for public-dispatch regression coverage. This was a static review with no local builds or tests; the supplied CI snapshot and current PR checks show successful build, test, lint, and cross-platform checks.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The diff changes peer-facing deserialization in msg/control.rs, msg/blocks.rs, and msg/gov.rs, but its six straightforward payload types reuse existing codecs and dispatch machinery rather than introducing large or intricate parsing logic. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane 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— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (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 `pkgs/p2p_core/src/msg/control.rs`:
- [SUGGESTION] pkgs/p2p_core/src/msg/control.rs:103-114: Cover typed dispatch and malformed-payload rejection for the four promoted stubs
These tests call `check_wire`, which decodes the concrete struct directly without exercising `P2pMsg::decode_payload`. The same applies to `SyncStatusCount` and `GetBlocks`, so all four corpus suites would still pass if their registrations reverted to raw stubs, losing the payload validation this PR promises. Extend the dispatch-test pattern already used for `SendDsq` and `QSendRecSigs`: assert the expected typed variants for valid corpus payloads and errors for empty/truncated payloads and an appended byte. Include a non-minimal CompactSize locator count for `getblocks`. These can be ordinary parameterized tests without introducing an error-corpus format. The current registrations and shared decoder appear correct; this is regression coverage, not an observed decoding defect.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Reviewed the complete base-to-head diff at 4a27668 and found no actionable defects. The six typed payloads match the pinned Dash Core layouts, retain the shared bounded and strict decoder path, and now have typed-dispatch regression coverage. Validation was static only; current GitHub checks report passing lint, MSRV build/test, stable-platform, and cross-build jobs.
🔴 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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The diff changes peer-facing deserialization in pkgs/p2p_core/src/msg/mod.rs, but the six payload types use straightforward existing codecs and corpus tests rather than introducing large or intricate parsing logic. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane 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— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (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.
kwvg
left a comment
kwvg
left a comment
There was a problem hiding this comment.
Implementation is sound, suggestions are mostly convention-related
The reference implementation sends and reads `senddsq` with a flag, so requiring an empty payload refused every honest message. The variant now carries a `SendDsq`.
The reference implementation sends and reads `qsendrecsigs` with a flag, so requiring an empty payload refused every honest message. The variant now carries a `QSendRecSigs`.
The payload is a flag and a version, as the reference implementation reads it. The variant now carries a `SendCmpct`, and a payload that does not decode to one is refused.
4a27668 to
371b2b4
Compare
PastaPastaPasta
changed the title
senddsq, qsendrecsigs, sendcmpct, sendtxrcncl, ssc and getblocks
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 diff from 62d91ea to 371b2b4 and found no actionable in-scope issues. The six typed payloads match the pinned Dash Core wire layouts, preserve shared decoding protections and short IDs, and include typed-dispatch regression coverage that resolves the prior finding. Validation was static only; the exact-head CI snapshot and current PR checks show successful lint, MSRV build/tests, stable-platform builds, and cross-builds.
🔴 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: rust-quality); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The diff changes peer-facing network deserialization in Rust for multiple P2P message types, including blocks and transaction reconciliation payloads, making it an intricate critical-surface change. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (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) - 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— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (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.
|
@PastaPastaPasta, just do a rename and force-push based on the review comment and then I can approve this PR. |
The payload is a version and a salt, as the reference implementation reads it. The variant now carries a `SendTxRcncl`, and a payload that does not decode to one is refused.
The payload is an item kind and a count, as the reference implementation reads it. The variant now carries a `GovSyncStatusCount`, and a payload that does not decode to one is refused.
The payload is a version, a locator and a stop hash, as the reference implementation reads it. The variant now carries a `GetBlocks`, and a payload that does not decode to one is refused.
371b2b4 to
b713938
Compare
|
@kwvg Done. The module and its corpus are renamed to 🤖 Posted autonomously by Claude on behalf of pasta. |
Motivation
Several messages that peers of the reference implementation (
dashpay/dashv24.0.0-rc.3) send with a payload are not typed indash-p2p-core:senddsqandqsendrecsigsare declared as empty, soP2pMsg::decode_payloadrefuses every honest message withPayloadNotEmpty. The reference implementation sends both with a flag, the first one at the end of the handshake unless it rejects the peer's transactions.sendcmpct,sendtxrcncl,sscandgetblocksare stubs, so their payload is carried as bytes and never checked.This pull request gives each of them a type that reads its payload as the reference implementation does. Each type sits in the module of the subsystem it belongs to, and comes with a corpus section generated by running a program against the reference implementation at the pin above. Each section is embedded with its program. Every corpus test also checks that
P2pMsg::decode_payloadreads the payloads as the typed variant.coinjoin.rs(new)senddsqquorum_sigs.rs(new)qsendrecsigsblocks.rs(new)sendcmpct,getblockstx_reconcile.rs(new)sendtxrcnclgov.rssscAdditional Information
senddsqas oneboolcoinjoin.json5senddsqqsendrecsigsas oneboolquorum_sigs.json5qsendrecsigssendcmpctblocks.json5sendcmpctsendtxrcncltx_reconcile.json5sendtxrcnclsscGovSyncasGovSyncStatusCount.gov.json5sscgetblocksgetheaders.blocks.json5getblocksThe reference implementation disconnects a peer whose
getblockslocator holds more than 101 hashes. That is a decision taken after decoding, andgetheadersdoes not enforce it here either, so it is left out.Breaking Changes
Changes below for
dash-p2p-core.P2pMsg::SendDsqandP2pMsg::QSendRecSigscarry the newSendDsqandQSendRecSigs. Decoding them takes one byte instead of an empty payload.P2pMsg::SendCmpct,P2pMsg::SendTxRcncl,P2pMsg::SscandP2pMsg::GetBlockscarry the newSendCmpct,SendTxRcncl,GovSyncStatusCountandGetBlocksinstead of the payload bytes. A payload that does not decode to one of them is refused.How Has This Been Tested?
just comb(just test lint fuzz::test, CodeQL 2.27.2 included) passed on every commit, ondevelop(62d91ea), on x86_64 Linux. After the review's rename oftx_reconciliationtotx_reconcile, the three rewritten commits (sendtxrcncl,ssc,getblocks) were checked again withcargo fmt --check,cargo clippy -p dash-p2p-core --all-targets --all-features -- -D warnings,cargo test -p dash-p2p-core --all-featuresand the commit lint; the other three commits are unchanged.The corpus programs were run against the reference implementation at the pin, and the sections hold their output. Each section says how to run its program.
Checklist
comments and concerns without said assistance. I assume authorship of the code submitted.
Agent disclosure
Code, tests, corpus programs, commit messages and this description were generated with Claude Code using Claude Opus 5.5, running autonomously as an agent on behalf of the author. The agent also ran the corpus programs against the reference implementation. The branch was reviewed by a Claude Code agent using Opus 5.5.
🤖 Generated with Claude Code