Repository navigation
p2p_core%fix!: align handshake and header sync messages with the reference implementation - #86
PastaPastaPasta wants to merge 9 commits into
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly summarizes the main change: aligning p2p_core handshake and header-sync messages with the reference implementation. |
| Description check | |
The description directly explains the handshake, header-sync, serialization, benchmark, testing, and breaking API changes in the pull request. |
| 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. |
- 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.
|
Warning This pull request may have conflicts, please coordinate with the authors of these pull requests. Potential conflicts |
|
✅ Final review complete — no blockers (commit e5bcfef) · triage: critical |
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Updated after discussion: 2 finding(s) of this review were settled in the inline threads; no blocking finding remains.
- ✅ Withdrawn: 🔴 Qualify the unresolved
size_ofcall- ✅ Withdrawn: 🟡 Represent
addr_sendandnonceas one optional pair
The change aligns the handshake and compressed-header behavior with the stated reference implementation, and the available CI checks are successful, with several macOS Bazel jobs still running. However, the changed Pong decoder contains an unresolved size_of reference that prevents dash-p2p-core from compiling, and the public Version API permits inconsistent optional-field states that encoding silently truncates.
🔴 1 0 blocking | 🟡 1 0 suggestion(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: rust-quality); 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); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change to peer-facing network deserialization and protocol encoding in files such aspkgs/p2p_core/src/msg/headers2.rs,version.rs, andping.rs, which directly changes how untrusted network messages are parsed. - 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 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
🤖 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/ping.rs`:
- [BLOCKING → WITHDRAWN in discussion; do not fix] pkgs/p2p_core/src/msg/ping.rs:43: Qualify the unresolved `size_of` call
`size_of` is not imported in this module and is not provided by the Rust prelude. The production imports only `BaseCodec`, `DecodeError`, and `EncodeBuf`; the test-only `crate::prelude::*` import does not affect the implementation. This leaves the changed Pong decoder with an unresolved name and prevents `dash-p2p-core` from compiling, including no_std builds.
In `pkgs/p2p_core/src/msg/version.rs`:
- [SUGGESTION → WITHDRAWN in discussion; do not fix] pkgs/p2p_core/src/msg/version.rs:194-198: Represent `addr_send` and `nonce` as one optional pair
`addr_send` and `nonce` are required to appear together on the wire, and decoding always produces both values or neither. As separate public `Option` fields, the API nevertheless permits states such as `addr_send: Some(...)` with `nonce: None`. `encode_optional` then returns at the first missing field and silently discards the present value and all subsequent optional fields, so encoding is lossy and `decode(encode(value))` does not preserve the value. Group these fields into a single optional pair or dedicated struct, or otherwise make inconsistent states rejectable rather than silently truncating them.
|
|
||
| impl BaseCodec for Pong { | ||
| fn decode(data: &mut &[u8]) -> Result<Self, DecodeError> { | ||
| if data.len() < size_of::<u64>() { |
There was a problem hiding this comment.
✅ Withdrawn at
e5bcfefc; see the replies below.
🔴 Blocking: Qualify the unresolved size_of call
size_of is not imported in this module and is not provided by the Rust prelude. The production imports only BaseCodec, DecodeError, and EncodeBuf; the test-only crate::prelude::* import does not affect the implementation. This leaves the changed Pong decoder with an unresolved name and prevents dash-p2p-core from compiling, including no_std builds.
| if data.len() < size_of::<u64>() { | |
| if data.len() < core::mem::size_of::<u64>() { |
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
size_of is in the Rust prelude since 1.80 (core::mem::size_of, size_of_val, align_of and align_of_val were added to every edition's prelude), and the workspace's rust-version is 1.89.0. The module compiles as written: the build, clippy and test checks on d42b7d5 all passed. So no import is needed.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
You're right. size_of was added to the Rust prelude in 1.80, and this workspace requires Rust 1.89, so the unqualified call is valid here. I withdraw this finding.
Withdrawing this finding.
There was a problem hiding this comment.
Withdrawn (re-reviewed at e5bcfefc): The unqualified size_of call resolves on the workspace's Rust 1.89 MSRV because it is in the prelude since Rust 1.80. The changed Pong decoder therefore has no unresolved-name compilation defect.
| /// Sender's own address, present with `nonce`. | ||
| pub addr_send: Option<VersionAddr>, | ||
| /// Random nonce for connection deduplication, taken as 1 if absent. | ||
| #[cfg_attr(feature = "serde", serde(default, with = "dash_types::serialize::opt_str_u64"))] | ||
| pub nonce: Option<u64>, |
There was a problem hiding this comment.
🟡 Suggestion: Represent addr_send and nonce as one optional pair
addr_send and nonce are required to appear together on the wire, and decoding always produces both values or neither. As separate public Option fields, the API nevertheless permits states such as addr_send: Some(...) with nonce: None. encode_optional then returns at the first missing field and silently discards the present value and all subsequent optional fields, so encoding is lossy and decode(encode(value)) does not preserve the value. Group these fields into a single optional pair or dedicated struct, or otherwise make inconsistent states rejectable rather than silently truncating them.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
There was a problem hiding this comment.
Thanks. I'd keep the current shape. Every field from addr_send on is read only while bytes remain, so the fields present always form a prefix. user_agent absent with start_height present is just as unrepresentable on the wire as addr_send without nonce. The struct models each of them as an Option and encode_optional writes up to the first absent one, which the type doc (lines 179-182) describes. Pairing only addr_send and nonce would make those two follow a different rule from the four fields after them, without removing the general case. If you'd prefer the whole tail as nested options or a length-indexed enum, I can do that in a follow-up.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
That rationale is fair. The wire representation is a prefix across the entire optional tail, so pairing only addr_send and nonce would not address the same representational issue for the later fields. Since the truncation behavior is documented and intentional, I withdraw this suggestion.
Withdrawing this finding.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review after discussion — commit d42b7d5
Standing review was CHANGES_REQUESTED; this re-review is APPROVE: no blocking findings remain.
Approved: the discussion withdrew or deferred every finding the full review of d42b7d57 (run 6688) raised, and the approval rests on that review.
No longer blocking:
- Qualify the unresolved
size_ofcall
Same commit as the standing review; the inline threads above carry the per-finding outcome. This follow-up exists only to correct the verdict.
Review provenance
Source: conversation lane gpt-6.1-sol (agent: conversation); no reviewer or verifier lanes ran for this follow-up
- Verdict moved because the inline discussion withdrew or deferred findings of the standing review of this commit; the code was not re-reviewed
github-actions
Bot
added
the
tag:needs-rebase
`serialize::str_u64` for an `Option<u64>` field, writing `None` as null, so an optional nonce keeps its decimal string in JSON.
The reference transport sends a short ID once the version both peers share reaches the one that assigned it, 70240 for `platformban` and 70235 otherwise. `encode_v2` and `P2pMsg::short_id` take that version.
The reference reads the fields from `addr_send` on only while payload bytes remain, so it takes a 46-byte `version`. They become `Option`, and encoding stops at the first absent one.
The reference reads the nonce of a `pong` only when 8 bytes remain, so an empty `pong` is taken without one. `Pong::nonce` becomes an `Option`, and 1 to 7 bytes are trailing bytes.
The reference reads the count after each header as a size and ignores it, so a `headers` entry with a count other than 0 is accepted.
`Headers2` keeps each entry as a `CompressedHeader`, so a decoded message encodes back to the same bytes. `CompressionState` goes, and `Headers2::announce` builds a message from block headers.
The reference leaves out every previous hash but the first, steps the time modulo 2^32, and keeps the first version only when announcing. `Headers2::reply` and `Headers2::announce` now do the same.
The reference restores nothing into a first header and ignores its time offset, and a version offset past the recent versions restores nothing. Those fields stay zero instead of failing or taking the offset as time.
Decodes `headers2` replies of 2000 and 8000 chained headers, the limits before and after the raised count.
d42b7d5 to
e5bcfef
Compare
github-actions
Bot
removed
the
tag:needs-rebase
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The reviewed wire-format changes are consistent with the stated Dash Core behavior, and the completed build, test, lint, and cross-platform checks show no related failures. One non-blocking API design issue remains: Version exposes independent optional tail fields while its infallible encoder silently truncates at the first gap; several platform checks remain queued or in progress.
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
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:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change to peer-facing network deserialization and protocol encoding in files such as pkgs/p2p_core/src/msg/headers2.rs and pkgs/p2p_core/src/msg/version.rs. - 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 each finding against the current code and only fix it if needed.
In `pkgs/p2p_core/src/msg/version.rs`:
- [SUGGESTION] pkgs/p2p_core/src/msg/version.rs:194-198: Represent `addr_send` and `nonce` as one optional pair
(existing thread: https://github.com/dashpay/base-sdk/pull/86#discussion_r4369719389)
The underlying invariant concern remains, although it applies to the whole optional suffix rather than only this pair. Because these fields are public and independently optional, callers can construct values such as `addr_send: Some(...)` with `nonce: None`, or populate a later field after an earlier one is absent. `encode_optional` then silently drops the present field and every suffix field after the gap, and `hash_impl!(Version)` hashes that truncated representation. Model the optional suffix as nested options or a length-indexed type, or otherwise prevent invalid prefix states from being constructed. The current documentation explains the truncation behavior but does not prevent this lossy API state.
Motivation
Several messages that
dash-p2p-coresends or reads during the handshake and the header sync differ from the reference implementation (dashpay/dashv24.0.0-rc.3):encode_v2sends a message by its short ID whenever it has one, whatever version the peers share. The reference implementation sends the long format until the handshake agrees on a version both peers share, and then sends a short ID only if that version is 70235 or later, 70240 or later forplatformban. A peer at 70235 to 70239 therefore receives a message type it does not know.Versionrequires every field up to the masternode connection flag. The reference implementation reads the fields fromaddr_sendon only while payload bytes remain, so it takes a 46-byteversion.Pongrequires a nonce, where the reference implementation takes an emptypong.Headersrequires each header's transaction count to be zero, where the reference implementation reads the count and ignores it.Headers2decodes into plain headers, so a decoded message does not encode back to the same bytes. Its encoder takes a signed time step and keeps the first version, so its output differs from both of the reference implementation's encoders. It also hashes each previous header to decide whether to leave the previous hash out, where the reference implementation leaves it out after the first header without comparing. Its decoder fails on a version offset past the recent versions and takes a first header's time offset as its time, where the reference implementation leaves those fields zero.This pull request aligns them. The V2 send gate and the
headers2encoders and decoder are pinned by corpus sections generated by running a program against the reference implementation at the pin above. Each section is embedded with its program, and each fix extends or adds a section with the shapes it changes. Theversion,pongandheadersreads happen in message handlers, so they are cited rather than pinned by a program. They are tested with the existingversionandpongvectors cut at every length, and with the main network genesis header forheaders.Additional Information
serialize::opt_str_u64todash-typesserialize::str_u64for anOption<u64>field, writingNoneas null. The optionalversionandpongnonces keep their decimal string form in JSON.dash-typesunit testencode_v2andP2pMsg::short_idtake the version both peers share,ProtocolVersion::INIT(209) until the handshake agrees on one.ShortId::for_sendgives the short ID only at or past the version that assigned it:ProtocolVersion::PLATFORMBAN_V2_SHORT_ID(70240) forplatformban,ProtocolVersion::BIP324_BASELINE(70235) for the others. The reference implementation sets the peer's version to the common version once the handshake agrees on one. The vectors cover 209, each side of both thresholds, and the latest version.transport.json5v2_sendversionfields as the reference implementationaddr_sendon becomeOptions. Each is read only while payload bytes remain,addr_sendand the nonce together, so a payload ending at any field boundary decodes. They form a prefix: encoding stops at the first absent field, so a field present after an absent one is not sent. The field docs give the values the reference implementation takes for an absent nonce, start height and relay.version.json5version, cut at every lengthpongas the reference implementationpongdecodes without one. 1 to 7 bytes are left unread, so the strict codec reports them as trailing bytes.ping.json5pong, cut at every lengthHeaders2holdsCompressedHeaderentries, each with its flags, its header and its time offset. Decoding restores the fields an entry leaves out from the entry before it, as before, and keeps the rest, so a decoded message encodes back to the same bytes, unused flag bits included.Headers2::block_headersgives the headers.Headers2::announcebuilds a message from headers with the encoderCompressionStatehad.headers2.json5uncompressheaders2as the reference implementationHeaders2::replyandHeaders2::announcematch the reference implementation's two encoders. Every header after the first leaves its previous hash out, without comparing it. The time step is taken modulo 2^32, so a step back goes as the full time, and a step past the largest time wraps around and goes as an offset. A reply does not keep the first header's version for the headers after it, and an announcement does.headers2.json5reply,announceheaders2fields that cannot be restored at zeroheaders2.json5uncompressheaders2decodedivanbench, as indash-num,dash-pkcanddash-pow, decoding replies of 2000 and 8000 chained headers, the most a peer sends before and after the raised limit.The
version,pongandheadersreads have no program of their own: the reference implementation reads them inside its message handlers, which a test program cannot drive without a connected peer. Their tests derive every payload from the existing vectors and the genesis header, and the table cites the handler code they follow.Breaking Changes
Changes below for
dash-p2p-core.encode_v2andP2pMsg::short_idtake theProtocolVersionboth peers share. PassProtocolVersion::INITuntil the handshake agrees on one.Version::addr_send,nonce,user_agent,start_height,relay,mnauth_challengeandmn_connectionareOptions. Encoding stops at the firstNone.Pong::nonceis anOption<u64>.Headers2::headersis private.Headers2::headers()gives theCompressedHeaderentries, each with the restored header fromCompressedHeader::header(), andHeaders2::block_headers()gives the headers. A message is built withHeaders2::replyorHeaders2::announce. Its serde form lists the entries.CompressionStateis removed.Headers2built from headers encodes as the reference implementation's encoders do, so the bytes differ from before for most chains.How Has This Been Tested?
just comb(just test lint fuzz::test, CodeQL 2.27.2 included) passed on every commit, ondevelop(3153d6f), on x86_64 Linux.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. Each fix commit's new vectors and tests fail without its fix.
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, and simplified by one using Sonnet 5.5.
🤖 Generated with Claude Code