p2p_core%fix!: make `ProtocolVersion` signed, bound commands and V2 contents as Core does, check commands as Core's transports do by PastaPastaPasta · Pull Request #59 · dashpay/base-sdk

PastaPastaPasta · 2026-10-09T10:10:45Z

Motivation

Five places where P2pMsg decoding and V2 framing differ from Dash Core's transports:

ProtocolVersion was a u32. Core's version is an int, so a version with the top bit set is negative and below every minimum (src/net_processing.cpp:3970 in v24.0.0-rc.3).
The 3 MiB limit applied only to some commands. Core's V1 transport refuses any message above MAX_PROTOCOL_MESSAGE_LENGTH before looking at the command (src/net.h:84, src/net.cpp:737-738).
Core's V2 transport instead bounds the whole contents by MAX_CONTENTS_LEN, 1 + 12 + 3 MiB, before it reads the type (src/net.cpp:1297-1306). A short-ID message may carry 12 more payload bytes than a V1 message, and an oversized message is refused before its type is checked.
Commands were not checked. Core's V1 IsCommandValid (src/protocol.cpp:232-249) and V2 GetMessageType (src/net.cpp:1496-1530) accept printable bytes up to the first NUL and then only NULs, and V2 also accepts 0x7f.
P2pDecodeError::Consensus held the decode error's text, so callers could not tell a short read from trailing bytes.

Additional Information

decode_payload (V1) checks the payload size first for every command, known or not. decode_v2 checks the contents size first, then the short ID or long-form type, and skips the V1 bound. For an oversized message with an unknown short ID, PayloadTooLarge.command is empty.
V1 callers check CommandString::is_valid_v1 themselves; decode_payload reports an invalid command as UnknownCommand, which Core also ignores.
CommandString::is_valid_v1 and is_valid_v2 follow Core. decode_v2 refuses a long-form type that GetMessageType refuses, as P2pDecodeError::InvalidCommand, which Core drops while keeping the peer.
Parsed messages decode with BaseCodec and fail on trailing bytes, as before. P2pDecodeError::Consensus now carries the DecodeError, which source() returns. Every P2P type was already bound by the same 3 MiB limit, which the transport check now enforces first.
Tests in tests/decode_payload.rs cover the V1 size bound per dispatch arm, the V2 contents bound and its order, command validity and the typed errors.

Breaking Changes

ProtocolVersion wraps an i32.
P2pDecodeError::PayloadTooLarge.command is a CommandString. For V2, size and max measure the whole contents.
P2pDecodeError::Consensus holds a DecodeError instead of a String.
P2pDecodeError::InvalidCommand is new.

How Has This Been Tested?

./contrib/git_filter.py --fast-fail develop upstream-pr/p2p_frame -- bash -c 'cargo clippy --workspace --exclude dash-pow --all-targets --no-default-features -- -D warnings && cargo clippy --workspace --exclude dash-pow --all-targets --features full -- -D warnings && cargo test --all-targets --features full && ./maint/lint_all.py --exclude lint_codeql'

Run on x86_64 Linux. There, -D warnings fails in dash-pow (dead code in src/util/aes/amd64.rs and src/groestl/consts.rs) on develop as well, and this PR does not touch dash-pow, so clippy excludes it; CI runs per package on aarch64. CodeQL (./maint/lint/lint_codeql.py run --lang=rust --with-suite=rust-security-and-quality) was run on the head commit. lint_nix was skipped (no nixfmt).

Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional tests
I have made corresponding changes to the documentation
I did not use agentic assistance (i.e. LLMs, AI) except as disclosed and can answer questions,
comments and concerns without said assistance. I assume authorship of the code submitted.
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

Agent disclosure

Code, tests and commit messages generated with Claude Code using Claude Opus 5.5, high effort, running autonomously on behalf of the author; the rebase onto develop (after #60) was done the same way. Self-reviewed with Claude Code review agents (Opus 5.5). The AI co-author trailers were removed from the commits to satisfy lint_commit; this section replaces them.

🤖 Generated with Claude Code

coderabbitai · 2026-10-09T10:10:55Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: dashpay/base-sdk/.coderabbit.yml
Review profile: CHILL
Plan: Advanced
Run ID: dd753c01-0b04-4b5d-b99c-17bdcedf0896

📥 Commits

Reviewing files that changed from the base of the PR and between d9e0d24 and 6b32b34.

📒 Files selected for processing (6)

pkgs/p2p_core/src/bip324.rs
pkgs/p2p_core/src/command.rs
pkgs/p2p_core/src/error.rs
pkgs/p2p_core/src/macros.rs
pkgs/p2p_core/src/version.rs
pkgs/p2p_core/tests/decode_payload.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Summary

Summary by CodeRabbit

Bug Fixes
- Improved handling of malformed or incomplete peer messages, including invalid commands and truncated input.
- Oversized messages are consistently rejected, including messages with unrecognized commands.
- Decoding errors now provide more precise information, and extra trailing data is rejected.
- Peer protocol versions with negative values are now interpreted correctly.

📝 Summary

Walkthrough

Command strings now have separate V1 and V2 validity checks. Payload decoding applies size limits before command dispatch and returns structured decode errors. V2 decoding checks total contents size, validates long-format commands, and uses the unbounded payload decoder after applying its own size check. ProtocolVersion now uses a signed i32.

Sequence Diagram(s)

sequenceDiagram
  participant Transport
  participant decode_v2
  participant CommandString
  participant P2pMsg
  Transport->>decode_v2: V2 contents
  decode_v2->>decode_v2: Parse type and command
  decode_v2->>CommandString: Validate long-format command
  decode_v2->>P2pMsg: Decode bounded payload with decode_unbounded
  P2pMsg-->>decode_v2: Decoded message or decode error
  decode_v2-->>Transport: Message or V2 decode error

Suggested reviewers: kwvg

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 6b32b

No actionable issue was established in the changed decoding paths. The PR is mergeable after normal checks.

Pre-merge checks | 4

✅ Passed checks (4 passed)

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 summarizes the main changes: signed ProtocolVersion values, message and V2 content bounds, and command validation. It is somewhat long but remains specific and relevant.
Description check		The description directly explains the decoding, framing, validation, error, testing, and compatibility changes in the 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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-10-09T10:12:54Z

Warning

This pull request may have conflicts, please coordinate with the authors of these pull requests.

Potential conflicts

p2p_core%fix: accept up to 8000 headers in headers2, bound header counts by the input, export the header count limits #65

Callers can now tell a short read from trailing bytes.

PastaPastaPasta · 2026-10-15T23:06:37Z

Superseded by #67, which reworks this series per review.

🤖 Posted autonomously by Claude on behalf of pasta.

…ith the reference implementation (#67) * [59981d4] p2p_core%fix!: make `ProtocolVersion` signed (pasta) * [e12661a] p2p_core%refac!: carry the decode error in `P2pDecodeError` (pasta) * [8ca2966] p2p_core%fix!: bound every command by the 3 MiB message limit (pasta) * [3413681] p2p_core%fix!: bound V2 contents before reading the type (pasta) * [49ef62a] p2p_core%fix!: take V2 message types as the reference transport does (pasta) * [8cfb0a3] p2p_core%fix!: refuse commands the reference V1 transport refuses (pasta) * [8626fe1] p2p_core%feat!: name every inventory type and map it to its command (pasta) * [a222647] p2p_core%fix: accept up to 8000 headers in `headers2` (pasta) * [b04e311] p2p_core%fix: bound header counts by the remaining input (pasta) * [cfbc119] p2p_core%feat: add `Headers2::max_count` for the negotiated limit (pasta) * [606d44d] p2p_core%refac: decode the V2 message type through `BaseCodec` (pasta) * [06c6803] p2p_core%fix!: check commands against the V2 rule only (pasta) * [07a739e] p2p_core%test: check the V2 transport vectors from `tests` (pasta) * [ebc5c06] p2p_core%feat: export `MAX_P2P_PAYLOAD_SIZE` and link limits in docs (pasta) * [46e4704] p2p_core%refac: suffix size constants with `_SIZE` (pasta) * [8b43b25] p2p_core%refac: derive the V2 short ID table counts from the messages (pasta) * [ab11ad1] p2p_core%test: decode a full batch of minimally compressed headers (pasta) Description: ````` ## Motivation `dash-p2p-core` diverges from the reference implementation (`dashpay/dash` [`v24.0.0-rc.3`](https://github.com/dashpay/dash/tree/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18)) in how it frames and bounds P2P messages, which inventory types it knows, and how many headers it accepts. In particular: - an oversized message is not refused whatever its command; - invalid commands are not told apart from unknown ones; - some V2 short IDs the reference implementation takes are refused; - `headers2` is capped at 2000 headers instead of 8000. This pull request aligns them. Where the behaviour has a library entry point in the reference implementation, the claim comes with a corpus section generated by running a program against it at the pin above. Each program is embedded in the section it generated. Where the behaviour lives only in the reference implementation's message handler, the claim is cited. It supersedes #59, #64 and #65, which are reworked here into one series. The `dash-primitives` transaction check changes that this pull request carried before, which superseded #63, now have their own pull request. ## Additional Information | Description | Notes | Reference implementation | Corpus | | ----------- | ----- | ------------------------ | ------ | | Read the protocol version as signed | A version with the top bit set reads below every minimum. | [read as `int`](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/net_processing.cpp#L3945-L3950), [minimum check](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/net_processing.cpp#L3970), [minimum](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/version.h#L20) | - | | Bound every command by the message limit | `P2pMsg::decode_payload` applies `MAX_P2P_PAYLOAD_SIZE` to unknown commands too, as the reference implementation bounds a message before it looks at its type. | [limit](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/net.h#L84) | - | | Bound V2 contents before reading the type | Contents above 1 + 12 bytes + `MAX_P2P_PAYLOAD_SIZE` disconnect whatever the type. A long-form type is printable ASCII (`0x20` to `0x7f`) up to the first NUL and then only NULs. Otherwise the message is rejected without disconnecting. | [bound](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/net.cpp#L1297-L1307), [type](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/net.cpp#L1496-L1534), [reject](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/net.cpp#L1549-L1559) | `transport.json5` `v2` | | Take V2 message types as the reference transport does | Contents that end before the type is complete are rejected. Short IDs 1 to 32 and 128 to 168 are taken whether a message is assigned to them or not. An unassigned one gives an empty type, which is ignored. Every other short ID is rejected. The table counts are derived from the messages and the new `unassigned` slots of `define_p2p!`. | [empty contents](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/net.cpp#L1498), [short long form](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/net.cpp#L1512-L1514), [short ID ranges](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/net.cpp#L1028-L1035), [tables](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/net.cpp#L892-L977) | `transport.json5` `v2`, `tests/decode_v2.rs` | | Refuse invalid commands | `P2pMsg::decode_payload` refuses a command that a V2 transport refuses as a long-form type. `p2p_core` only speaks V2, so V1 rules are not evaluated. | [type](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/net.cpp#L1496-L1534) | `tests/decode_payload.rs` | | Name every inventory type | - | [types](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/protocol.h#L495-L528) | - | | Map inventory types to commands, and tell transaction and block types apart | Asset unlocks are announced by their own type but carried in `tx` messages. | [commands](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/protocol.cpp#L281-L309), [predicates](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/protocol.h#L555-L562) | `inv.json5` `inv_type` | | Accept up to 8000 headers in `headers2` | `headers` stays at 2000. | [limits](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/validation.h#L82-L83), [count check](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/net_processing.cpp#L5323-L5327) | - | | Pick the `headers2` limit from the negotiated version | `Headers2::MAX_COUNT` only from protocol version 70235, otherwise `Headers::MAX_COUNT`. | [selection](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/net_processing.cpp#L1210-L1216), [version](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/version.h#L50) | - | Header counts are also bounded by the remaining input before anything is allocated. A count the input cannot hold now reports `DecodeError::Eof`. The reference implementation fails the same read. The command in a `PayloadTooLarge` or `InvalidCommand` message is escaped, as it is for `UnknownCommand`, since it can come from any peer. The last six commits answer the review: the V2 type is read through `BaseCodec`, commands are checked against the V2 rule only, the V2 transport vectors are checked from `tests`, `MAX_P2P_PAYLOAD_SIZE` is exported so docs can link it instead of quoting its value, size constants carry a `_SIZE` suffix, and the short ID table counts are derived from the messages. The branch is rebased onto `develop` after #70 and #71. It needed no conflict resolution: #71 does not touch the code this pull request changes, and the header count reads keep their own bound, since each entry takes more than the one byte `CompactSize::decode_len` assumes. ## Breaking Changes Changes below for `dash-p2p-core`. - `ProtocolVersion` wraps an `i32` instead of a `u32`. - `P2pDecodeError::Consensus` carries the `DecodeError` instead of its message. - `P2pDecodeError::PayloadTooLarge::command` is a `CommandString`, and its message escapes the command. Every command, known or not, is bounded by `MAX_P2P_PAYLOAD_SIZE`. - `decode_v2` refuses contents above 1 + 12 + `MAX_P2P_PAYLOAD_SIZE` bytes with `PayloadTooLarge`. It reports contents that end before the type as the new `P2pDecodeError::MissingCommand`, and an invalid long-form type as the new `P2pDecodeError::InvalidCommand`. An unassigned short ID within the reference implementation's tables decodes as an empty command, giving `UnknownCommand`. - `P2pMsg::decode_payload` reports a command that fails `CommandString::is_valid_v2` as `InvalidCommand` instead of `UnknownCommand`. - `InvType` has a variant for every inventory type the reference implementation defines. - Decoding `Headers2` accepts up to 8000 headers. Decoding `Headers` and `Headers2` reports a count the remaining input cannot hold as `DecodeError::Eof`. - `MAX_P2P_PAYLOAD_SIZE` (now exported at the crate root), `CommandString::is_valid_v2`, `ShortId::is_in_table`, `InvType::command`, `InvType::is_tx`, `InvType::is_block`, `Headers::MAX_COUNT`, `Headers2::MAX_COUNT`, `Headers2::max_count` and `ProtocolVersion::INCREASE_MAX_HEADERS2` are new. ## How Has This Been Tested? ``` just comb ``` `just comb` (`just test lint fuzz::test`, CodeQL 2.27.2 included) passed on every commit, rebased on `develop` (f2fdb28), on x86_64 Linux. Since #66, `dash-pow` builds without warnings there, so no local patch was needed. 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 - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional tests - [x] I have made corresponding changes to the documentation - [ ] I did not use agentic assistance (i.e. LLMs, AI) except as [disclosed](#agent-disclosure) and can answer questions, comments and concerns without said assistance. I assume authorship of the code submitted. - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ### 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, answered the review in follow-up commits, and split and rebased the branch. The branch was reviewed by Claude Code agents, using Opus 5.5 for two review rounds and Sonnet 5.5 for a simplification pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) `````

github-actions Bot mentioned this pull request Oct 12, 2026

sdk%feat!: bump MSRV to 1.89, add portable feature, define system requirements, decouple serde from codec, use thiserror, tune build flags, add mutant config, git merge script #60

Merged

6 tasks done

kwvg added this to base-sdk v0.1 Oct 12, 2026

kwvg added this to the 0.1 milestone Oct 12, 2026

github-actions Bot added topic:P2P Pull requests that concern primarily concern the dash-p2p-core crate. tag:needs-rebase This pull request conflicts with the base branch and needs to be rebased. labels Oct 12, 2026

PastaPastaPasta force-pushed the p2p_frame branch from 24ae7ad to 6b32b34 Compare October 12, 2026 17:56

github-actions Bot removed the tag:needs-rebase This pull request conflicts with the base branch and needs to be rebased. label Oct 12, 2026

PastaPastaPasta added 5 commits October 12, 2026 18:04

p2p_core%fix!: make ProtocolVersion signed, as Core's int is

c0fbd19

p2p_core%fix!: bound every command by the 3 MiB message limit

22e5f21

p2p_core%feat: check commands as Core's transports do

672c0aa

p2p_core%refac!: carry the decode error in P2pDecodeError

ac937e2

Callers can now tell a short read from trailing bytes.

p2p_core%fix: bound V2 contents before the type, as Core does

19d34e7

PastaPastaPasta force-pushed the p2p_frame branch from 6b32b34 to 19d34e7 Compare October 12, 2026 18:04

PastaPastaPasta mentioned this pull request Oct 13, 2026

sdk%fix: range-check decoded lengths, apply Dash Core's decode-time caps #57

Merged

5 of 6 tasks

github-actions Bot mentioned this pull request Oct 14, 2026

p2p_core%fix: accept up to 8000 headers in headers2, bound header counts by the input, export the header count limits #65

Closed

4 of 6 tasks

PastaPastaPasta mentioned this pull request Oct 15, 2026

p2p_core%fix!: align P2P framing, inventory types and header limits with the reference implementation #67

Merged

5 of 6 tasks

PastaPastaPasta closed this Oct 15, 2026

kwvg removed the topic:P2P Pull requests that concern primarily concern the dash-p2p-core crate. label Oct 19, 2026

kwvg removed this from base-sdk v0.1 Oct 19, 2026

kwvg removed this from the 0.1 milestone Oct 19, 2026

Navigation Menu

Repository navigation

p2p_core%fix!: make `ProtocolVersion` signed, bound commands and V2 contents as Core does, check commands as Core's transports do - #59

p2p_core%fix!: make `ProtocolVersion` signed, bound commands and V2 contents as Core does, check commands as Core's transports do#59
PastaPastaPasta wants to merge 5 commits into
dashpay:developfrom
PastaPastaPasta:p2p_frame

PastaPastaPasta commented Oct 9, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 9, 2026 •

edited

Loading

Summary by CodeRabbit

Walkthrough

Sequence Diagram(s)

Uh oh!

github-actions Bot commented Oct 9, 2026 •

edited

Loading

Uh oh!

PastaPastaPasta commented Oct 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

PastaPastaPasta commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Additional Information

Breaking Changes

How Has This Been Tested?

Checklist

Agent disclosure

Uh oh!

coderabbitai Bot commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Sequence Diagram(s)

Uh oh!

github-actions Bot commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Potential conflicts

Uh oh!

PastaPastaPasta commented Oct 15, 2026

Superseded by #67, which reworks this series per review.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

PastaPastaPasta commented Oct 9, 2026 •

edited

Loading

coderabbitai Bot commented Oct 9, 2026 •

edited

Loading

github-actions Bot commented Oct 9, 2026 •

edited

Loading