Repository navigation
primitives%fix: check the serialized transaction size in Transaction::check - #63
Closed
PastaPastaPasta wants to merge 1 commit into
Closed
PastaPastaPasta wants to merge 1 commit into
PastaPastaPasta wants to merge 1 commit into
Conversation
`TxInvalid::Oversize` was never returned. The size is now checked after the empty checks and before the payload size.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the main change: adding a serialized transaction-size check in Transaction::check. |
| Description check | |
The description directly explains the motivation, validation order, implementation approach, testing, and breaking behavior for the transaction-size check. |
| 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.
|
Note This pull request has no conflicts! 🎊 🎉 🎊 |
6 tasks done
github-actions
Bot
added
the
topic:Primitives
Member
Author
Superseded by #67, which reworks this series per review.🤖 Posted autonomously by Claude on behalf of pasta. |
kwvg
removed
the
topic:Primitives
5 of 6 tasks
kwvg
added a commit
that referenced
this pull request
Oct 21, 2026
…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) `````
kwvg
added a commit
that referenced
this pull request
Oct 22, 2026
…e implementation (#73) * [7dc7661] primitives%test: add a generated corpus for `Transaction::check` (pasta) * [3242b6c] types%feat(codec): count the encoded size of a value without allocating (pasta) * [71faa37] primitives%fix: check the serialized transaction size (pasta) * [5a8a5d0] primitives%test: add a generated corpus for `Block::check` (pasta) * [4df8528] primitives%fix: treat only a lone null-prevout input as a coinbase (pasta) * [c318c6a] primitives%fix!: reject blocks whose merkle tree is mutated (pasta) * [37bad97] primitives%fix: check the serialized block size (pasta) Description: ````` ## Motivation `Transaction::check` and `Block::check` in `dash-primitives` diverge from the context-free checks of the reference implementation (`dashpay/dash` [`v24.0.0-rc.3`](https://github.com/dashpay/dash/tree/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18)). In particular: - `TxInvalid::Oversize` is never returned; - a transaction with several inputs whose first prevout is null counts as a coinbase; - a block whose merkle tree has equal sibling hashes passes, although its root matches a block without the repeat; - only an empty block fails the length check, whatever its serialized size. This pull request aligns them. Both checks are pinned by a corpus generated by running a program against the reference implementation at the pin above. Each program is embedded in the section it generated, and each fix extends its program with the shapes it changes. The transaction check commits were part of #67, which superseded #63. They move here so that #67 covers `dash-p2p-core` only. ## Additional Information | Description | Notes | Reference implementation | Corpus | | ----------- | ----- | ------------------------ | ------ | | Pin the transaction check order | The corpus covers every context-free reason a valid `Amount` can reach. An amount only holds values within the money range, so the two per-output range reasons are not covered. The total-too-large reason is. Special payloads are checked elsewhere in the reference implementation, so the test treats payload errors as passing. | [check](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/consensus/tx_check.cpp#L14-L75) | `tx_check.json5` `check` | | Count the encoded size without allocating | `dash_types::codec::encoded_size` runs a value's encoder into a byte counter instead of a buffer. The reference implementation's serialization framework sizes an object the same way, with a stream that only records the number of bytes written to it. The transaction and block size checks use it. | [size stream](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/serialize.h#L1417-L1460), [size](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/serialize.h#L1519-L1523) | `dash_types` codec unit test | | Check the serialized transaction size | Above 1,000,000 bytes is rejected after the empty checks and before the payload size. The size is counted with `encoded_size`. | [checks](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/consensus/tx_check.cpp#L32-L35), [limit](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/consensus/consensus.h#L10) | `tx_check.json5` `check` | | Pin the block check order | The proof of work is left out of the program. The test grinds the nonce at the regtest target instead. Each vector carries the block's merkle root, so the test rebuilds the reference implementation's transactions byte for byte. | [check](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/validation.cpp#L4366-L4432), [sigops limit](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/consensus/consensus.h#L17-L20) | `block_check.json5` `check` | | Treat only a lone null-prevout input as a coinbase | This also applies to the block's missing-coinbase and multiple-coinbase checks. | [definition](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/primitives/transaction.h#L308-L311), [transaction check](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/consensus/tx_check.cpp#L60-L72), [block check](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/validation.cpp#L4396-L4401) | `tx_check.json5` `check`, `block_check.json5` `check` | | Reject a mutated merkle tree | `Block::merkle` already reported the repeat. It is now rejected right after the root check, before the size and coinbase checks. Equal sibling hashes are rejected at any position in a level. A block that repeats a transaction without a repeated pair of hashes passes, as it does in the reference implementation. | [repeat](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/consensus/merkle.cpp#L45-L59), [check](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/validation.cpp#L4324-L4346) | `block_check.json5` `check` | | Check the serialized block size | Above 2,000,000 bytes is rejected after the merkle checks and before the coinbase checks. The reference implementation also bounds the transaction count by the same limit, which the size bound already implies. | [check](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/validation.cpp#L4392-L4394), [limit](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/consensus/consensus.h#L11-L15) | `block_check.json5` `check` | The reference implementation's merkle root check also compares the asset unlock merkle root of a coinbase payload of version 4 or later ([check](https://github.com/dashpay/dash/blob/0364ed626ec2eaa518bf1ae4a0566ce638c7fe18/src/validation.cpp#L4351-L4360)). `CbTx` does not carry that field yet, so that step is out of scope here. ## Breaking Changes Changes below for `dash-primitives`. - `Transaction::check` returns `TxInvalid::Oversize` for a serialized size above 1,000,000 bytes. - A transaction counts as a coinbase only with exactly one input and a null prevout. This applies to `Transaction::check` and `Block::check`. - `Block::check` returns the new `BlockInvalid::DuplicateTransactions` for a mutated merkle tree. - `Block::check` returns `BlockInvalid::BadBlockLength` for a serialized size above 2,000,000 bytes. Its `size` is the serialized size, including for an empty block, which reported 0 before. ## How Has This Been Tested? ``` just comb ``` `just comb` (`just test lint fuzz::test`, CodeQL 2.27.2 included) passed on every commit, on `develop` (62d91ea), 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 fail without its fix. ## 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. The branch was reviewed by a Claude Code agent using Opus 5.5. 🤖 Generated with [Claude Code](https://claude.com/claude-code) `````
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Transaction::checknever returnedTxInvalid::Oversize, so a transaction of any size passed. Dash Core rejects a serialized size aboveMAX_LEGACY_BLOCK_SIZEafter the empty input and output checks and before the payload size check (source, limit).Additional Information
EncodeBufthat keeps only the byte count.Breaking Changes
Changes below for
dash-primitives.Transaction::checkreturnsTxInvalid::Oversizefor a transaction whose serialized size exceedsMAX_LEGACY_BLOCK_SIZE, afterTxInvalid::EmptyInputsandTxInvalid::EmptyOutputsand before the payload checks.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