Skip to content

Streamable size limits#1423

Open
arvidn wants to merge 3 commits intomainfrom
streamable-size-limits
Open

Streamable size limits#1423
arvidn wants to merge 3 commits intomainfrom
streamable-size-limits

Conversation

@arvidn
Copy link
Copy Markdown
Contributor

@arvidn arvidn commented Apr 28, 2026

This adds an option to the streamable macro, to specify a limit on vectors in streamable types. It can short cut some validation by failing earlier for messages that are invalid.

The second commit employs the new option to set appropriate limits for some message types.


Note

Medium Risk
Changes deserialization behavior for Streamable structs by enforcing new per-field vector length limits, which can cause previously-accepted oversized network messages to be rejected. Risk is moderate because it touches core protocol parsing but is additive and guarded by explicit #[chia(max_length = ...)] annotations.

Overview
Adds a new #[chia(max_length = N)] field attribute to the Streamable derive macro to enforce bounded parsing for Vec<T> and Option<Vec<T>>, failing early with Error::SequenceTooLarge.

Refactors vector parsing in chia-traits to route through a shared parse_vec_with_max_length helper and adds tests covering bounded Vec/Option<Vec> behavior.

Applies explicit maximum lengths to several full node and wallet protocol message fields (e.g., block/header responses, additions/removals queries, subscription update payloads, mempool notifications) to cap decoded list sizes.

Reviewed by Cursor Bugbot for commit 8c79e84. Bugbot is set up for automated code reviews on this repo. Configure here.

Comment thread crates/chia-traits/src/streamable.rs
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds bounded vector parsing to the Streamable derive to fail early on oversized protocol payloads, and applies explicit max-length limits to selected full node and wallet message types.

Changes:

  • Introduces #[chia(max_length = N)] on Streamable-derived fields to bound Vec<T> / Option<Vec<T>> parsing.
  • Adds parse_vec_with_max_length() to chia-traits plus unit tests for bounded parsing behavior.
  • Annotates various protocol message vector fields with concrete max_length limits to prevent oversized allocations/validation work.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
crates/chia_streamable_macro/src/lib.rs Extends the derive macro to parse #[chia(max_length = ...)] and generate bounded parsing for supported vector fields.
crates/chia-traits/src/streamable.rs Adds parse_vec_with_max_length() and tests validating bounded vector/optional-vector parsing failures.
crates/chia-protocol/src/wallet_protocol.rs Applies max_length bounds to multiple wallet protocol message vectors.
crates/chia-protocol/src/full_node_protocol.rs Applies max_length bound to RespondBlocks.blocks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/chia_streamable_macro/src/lib.rs
Comment on lines 318 to 322
Ok(())
}
fn parse<const TRUSTED: bool>(input: &mut std::io::Cursor<&[u8]>) -> #crate_name::chia_error::Result<Self> {
Ok(Self { #( #fnames: <#ftypes as #crate_name::Streamable>::parse::<TRUSTED>(input)?, )* })
Ok(Self { #( #fnames: #parse_exprs, )* })
}
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new max_length support is enforced only during parse(). The derived stream() implementation still allows serializing vectors longer than max_length, which can let the code construct and emit protocol messages that peers (or future local parsing) will reject. Consider also generating a length check in stream() for bounded fields (returning Error::SequenceTooLarge) to keep the invariant consistent in both directions.

Copilot uses AI. Check for mistakes.
Comment thread crates/chia-traits/src/streamable.rs
Comment thread crates/chia-protocol/src/wallet_protocol.rs
Comment thread crates/chia-protocol/src/full_node_protocol.rs
@coveralls-official
Copy link
Copy Markdown

coveralls-official Bot commented Apr 28, 2026

Coverage Report for CI Build 25061556133

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.03%) to 80.732%

Details

  • Coverage increased (+0.03%) from the base build.
  • Patch coverage: 5 uncovered changes across 1 file (95 of 100 lines covered, 95.0%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
crates/chia_streamable_macro/src/lib.rs 54 49 90.74%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 18461
Covered Lines: 14904
Line Coverage: 80.73%
Coverage Strength: 12217114.08 hits per line

💛 - Coveralls

@arvidn arvidn force-pushed the streamable-size-limits branch from 2f5d61a to 652502a Compare April 28, 2026 08:47
Comment thread crates/chia-protocol/src/wallet_protocol.rs Outdated
@arvidn arvidn requested a review from emlowe April 28, 2026 11:28

#[streamable(message)]
pub struct RequestPuzzleState {
#[chia(max_length = 32700)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If register_for_coin_updates and register_for_ph_updates can allow 1.6 million, so should these. This limit is insufficient for wallets like Sage to function. Many wallets have over this many puzzle hashes to subscribe to (each one is a wallet address).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the 1.6 million is derived from the 50 MiB message limit, which will not fit more 32 byte hashes.

Does Sage subscribe to all coins in a single request? This number was picked based on CoinStore.MAX_PUZZLE_HASH_BATCH_SIZE, see: https://github.com/Chia-Network/chia-blockchain/blob/main/chia/full_node/full_node_api.py#L1949

The limit is defined here: https://github.com/Chia-Network/chia-blockchain/blob/main/chia/full_node/coin_store.py#L444

It seems like we silently drop any item past this limit.


#[streamable(message)]
pub struct RespondPuzzleState {
#[chia(max_length = 32700)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

height: u32,
header_hash: Bytes32,
is_finished: bool,
#[chia(max_length = 32700)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above


#[streamable(message)]
pub struct RequestCoinState {
#[chia(max_length = 32700)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above


#[streamable(message)]
pub struct RespondCoinState {
#[chia(max_length = 32700)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

pub struct RespondCoinState {
#[chia(max_length = 32700)]
coin_ids: Vec<Bytes32>,
#[chia(max_length = 32700)]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Comment thread crates/chia-traits/src/streamable.rs
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 8c79e84. Configure here.

height: u32,
header_hash: Bytes32,
is_finished: bool,
#[chia(max_length = 500000)]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent puzzle hash limits between request and response

Medium Severity

RequestPuzzleState accepts up to 35000 puzzle hashes while RespondPuzzleState only allows 33000. Since the response echoes back the requested puzzle hashes, a peer accepting a request near the upper limit cannot generate a response that successfully parses on the other side, producing an unrecoverable protocol failure for otherwise valid requests.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 8c79e84. Configure here.

header_hash: Bytes32,
#[chia(max_length = 30000)]
coins: Vec<(Bytes32, Vec<Coin>)>,
#[chia(max_length = 30000)]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vec size limits may break existing wallet clients

Medium Severity

The 30000/35000 caps on puzzle_hashes/coin_names in RequestRemovals, RequestAdditions, RequestPuzzleState, RespondPuzzleState, and CoinStateUpdate are far below the 1_600_000 used for RegisterForPhUpdates/RegisterForCoinUpdates. Wallets that currently operate with more puzzle hashes per request (each address counts as a hash) will see legitimate messages rejected with SequenceTooLarge after this change.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 8c79e84. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants