Repository navigation
p2p_core%fix: accept up to 8000 headers in headers2, bound header counts by the input, export the header count limits - #65
PastaPastaPasta wants to merge 2 commits into
Conversation
`headers2` replies carry up to 8000 headers. A count the remaining input cannot hold is reported as `Eof` before anything is allocated.
`headers2` carries up to 8000 headers only from protocol version 70235, `ProtocolVersion::INCREASE_MAX_HEADERS2`; below it, 2000.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
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 |
|
Warning This pull request may have conflicts, please coordinate with the authors of these pull requests. Potential conflicts |
kwvg
left a comment
There was a problem hiding this comment.
This guidance is aimed at the agent for all of its unmerged PRs, note as follows:
-
Stay compliant with
AGENTS.md, avoid expository comments or over-explaining your work, do not restate the code if it can be trivially understood, references to Dash Core should be substituted with reference implementation or identical terms, do not reference C++ function names or variables as they can go stale. -
If your changes claim alignment with Dash Core, updating the test corpus of the crate is ideal. This isn't always necessary but is encouraged when fixing bugs or introducing new consensus types in
dash-primitivesordash-p2p-coreas they are assertions of consensus conformance. See these as examples: -
Avoid many small pull requests (like #58) and instead pick a theme or a cluster of changes, each pull request should attempt to have 6-14 commits.
- Within those commits, they must be structured to avoid churn within each other while still being individually readable. Move-only commits and changes should not be done in the same commit unless the change is trivial enough that the move-only is registered by Git.
-
Do not open more than 5 continuous PRs regardless of draft state.
-
Rebase if your branches are no longer in sync with
developand avoid conflicts between your branches (though this can be ignored if it is unavoidable). -
When claiming fidelity with Dash Core or another codebase, citations are required, see #20 as an example.
-
Do not write tautological tests. Write code in the style of this codebase. Commits are expected to be bisectable,
just combmust succeed. This is not optional.
PR marked as requiring changes because PR subject and contents are too sparse. Commit above feedback as agent policy for this codebase.
|
Understood, and recorded as standing policy for all of the agent's base-sdk PRs. #59, #63, #64 and #65 will be reworked to it: themed PRs of 6–14 bisectable commits that each pass 🤖 Posted autonomously by Claude on behalf of pasta. |
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) `````
Motivation
Headers2refused more than 2000 headers on decode, but aheaders2reply carries up to 8000 once both peers are at protocol version 70235 (limits, version, selection). Dash Core reads the count and refuses one above that limit before reading any header (source).Additional Information
Decoding caps
headersatHeaders::MAX_COUNT(2000) andheaders2atHeaders2::MAX_COUNT(8000), the most Dash Core reads in either message.A count the remaining input cannot hold at the smallest entry size, 81 bytes for
headersand 39 forheaders2, is reported asDecodeError::Eofbefore anything is allocated for it.Headers2::max_countgives the limit for the protocol version both peers share, withProtocolVersion::INCREASE_MAX_HEADERS2.Breaking Changes
Changes below for
dash-p2p-core.Headers2accepts up to 8000 headers.HeadersandHeaders2reports a count the remaining input cannot hold asDecodeError::Eof.Headers::MAX_COUNT,Headers2::MAX_COUNT,Headers2::max_countandProtocolVersion::INCREASE_MAX_HEADERS2are new.How Has This Been Tested?
Run on x86_64 Linux on every commit. There, the
-D warningsclippy steps fail indash-powondevelopas well, so they ran with--workspace --exclude dash-pow.Checklist
comments and concerns without said assistance. I assume authorship of the code submitted.
Agent disclosure
Code, tests, 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, and self-reviewed with a Claude Code review agent (Opus 5.5).
🤖 Generated with Claude Code