Raptorcast: Canonicalize depth and reserved fields

[xinyuan-dev]

This PR enforces v1 packet rules the receiver was not previously checking.

• Reject nonzero reserved bits in mode field
  • Previously we do not validate on the reserved bits.
• Reject nonzero reserved u16 in chunk header
  • Previously such nonzero bytes on chunk header gets rejected during merkle proof check.
  • This PR rejects the values earlier to be consistent with the other reserved bits check.
• Reject non-canonical merkle tree depth/symbol length
  • An invalid merkle depth is already rejected early in packet parsing. However, valid but non-canonical depth were previously accepted in chunk ingestion path.
  • Messages of non-canonical depth were previously only detected and rejected during re-encoding.
  • This PR rejects non-canonical merkle tree depth early in decoder initialization indirectly through canonical symbol length check.

These changes are made to align with the MIP specification.

[Copilot]

🟡 Changes recommended

A newly added test asserts the wrong expected InvalidMode byte value (it doesn’t match the actual mutated mode/depth byte), which will cause test failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR tightens Raptorcast v1 receiver-side validation by enforcing reserved-field rules and rejecting chunks whose Merkle tree depth is valid-but-non-canonical for the receiver’s expected parameters, aligning parsing/ingestion behavior with v1 packet invariants.

Changes:

• Enforce v1 reserved-field rules in packet parsing (reserved bits in mode/depth byte; reserved u16 in chunk header).
• Plumb merkle_tree_depth through ValidatedChunk and reject deterministic primary/secondary chunks whose depth is non-canonical for the receiver.
• Add/extend deterministic tests for reserved-field rejection and non-canonical depth handling.

File summaries

File	Description
monad-raptorcast/src/udp.rs	Reject deterministic chunks early when Merkle tree depth is non-canonical; add tests for reserved fields and depth rejection.
monad-raptorcast/src/parser/packet_parser.rs	Add v1 reserved-field validation and carry merkle_tree_depth into validated chunk metadata/errors.
monad-raptorcast/src/round_info.rs	Update test helper ValidatedChunk construction to include merkle_tree_depth.
monad-raptorcast/src/decoding.rs	Update decoding tests’ ValidatedChunk construction to include merkle_tree_depth.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[xinyuan-dev]

@claude review

[github-actions]

Claude finished @xinyuan-dev's task in 5m 27s —— View job

---

Review of PR #3247 — Canonicalize depth and reserved fields

• Read PR diff and surrounding code
• Analyze correctness, security, and performance
• Post review feedback

Note: the code on the branch has been substantially reworked since the earlier automated reviews — the per-packet depth check in udp.rs was replaced by a canonical-symbol-length check at decoder initialization, validate_reserved() is now encapsulated in the header structs, and the InvalidMode test assertion was fixed. Those earlier findings (hot-path cost, code duplication, mode_byte() leaking layout) are all resolved in the current revision. This review is of the code as it stands now.

Overall

The design is sound. Enforcing depth canonicality via symbol length works because PacketLayout::symbol_len() is strictly decreasing in depth (proof length is 20 * (depth - 1)), so within the valid depth range symbol length uniquely determines depth. Coverage is complete across the decoder-cache paths: the opening symbol is checked in DecoderState::from_initial_message (decoding.rs:1613), subsequent symbols are checked against the opener's length (decoding.rs:1655, 1724), and the recently-decoded path checks length too (decoding.rs:1686). Sender/receiver group-size consistency holds: both sides count the group including the author (stake_partition_num_chunks_hint at assigner.rs:314 vs. StakePartition::num_chunks_hint at assigner.rs:415), and PrimaryBroadcastGroup::of_epoch guarantees the author is a member. The non-canonical drop correctly suppresses rebroadcast (try_decode returns None on InvalidSymbol, udp.rs:427).

Findings

1. Three panics on a network-input path in ensure_canonical_symbol_len (decoding.rs:1482–1496) — medium

The function has two .expect()s and one unreachable!, all guarded by invariants enforced far away:

• validator_set_size().expect(...): holds because primary v1 chunks only reach try_decode through handle_deterministic_primary, which always builds the context with Some(g.validator_set()). But handle_unicast (udp.rs:149–154) can build a context with validator_set = None for GroupId::Primary with an unknown epoch — it's only safe because unicast chunks are v0 and hit the EncodingScheme early-return first.
• .expect("every admitted message length and group size fits a permitted depth"): holds because the parser bounds app_message_len ≤ MAX_MESSAGE_SIZE (packet_parser.rs:702) and test_no_panic_on_valid_ranges covers the range (with MAX_VALIDATOR_SET_SIZE being the worst case since the chunk hint is monotonic in group size).
• unreachable!("v1 chunks carry a broadcast mode"): holds because v0 chunks always get EncodingScheme::Unspecified (packet_parser.rs:535) and v1 rejects unspecified mode bits.

Each is currently correct, but the guards live in the parser and udp routing, several layers away, and app_message_len is attacker-influenced. A future caller of DecoderCache::try_decode, a new encoding-scheme variant, or a relaxed parser bound turns any of these into a remotely triggerable panic. Since InvalidSymbol already exists as the error channel here, returning an error variant instead would cost nothing and remove the risk class. Fix this →

2. The early rejection is best-effort, not airtight — worth a comment — low

The check compares the observed symbol length to the canonical one, but the symbol length on the wire is attacker-chosen independently of the Merkle depth: a sender can build a packet with non-canonical depth d' whose payload is sized to the canonical symbol length for depth d. That packet passes this check, opens a decoder, and is only caught later by the existing re-encode check (check_deterministic_encoding), same as before this PR. That's fine — no regression, and the re-encode check remains the authoritative enforcement — but the PR description's "rejects non-canonical merkle tree depth early" holds only for consistently-constructed packets. A one-line comment on ensure_canonical_symbol_len noting that the re-encode check is still the backstop would prevent someone later treating this as a complete depth guarantee.

3. Validator-set skew now warns per chunk with no metric — low (carries over from the earlier bot review)

If the receiver's epoch validator set differs from the sender's enough to shift the canonical depth (the removed udp test showed depth 9 vs 7 for 16 KiB at 100 vs 10 validators), every chunk of the proposal fails at decoder open. Each failure emits a tracing::warn! (decoding.rs:1580) — for a multi-MB proposal that's thousands of warn lines per round — and there is still no UdpStateMetrics/decoder-cache counter for it, so systematic skew is visible only as log spam. Consider a counter plus rate-limiting the warn (or demoting repeats to debug).

4. Minor nits

• MalformedPacket::InvalidChunkHeader carries no payload, unlike its siblings (InvalidMode(u8), InvalidTreeDepth(u8)). Carrying the nonzero reserved u16 would help debugging interop issues.
• In the decoding test helper, merkle_root: HexBytes([depth; 20]) is doing real work (distinct cache keys per depth) but reads like filler; a short comment would help.
• The udp.rs test that previously exercised the receive path for non-canonical depth (test_deterministic_primary_rejects_non_canonical_depth) survives and now validates end-to-end that no rebroadcast happens — good; the secondary path is covered in test_non_canonical_symbol_len_opens_no_decoder.

Not verified

I could not run cargo test -p monad-raptorcast — the Bash permission set for this job doesn't allow cargo (or git fetch). If you'd like me to run tests in future runs, add Bash(cargo test:*) to the action's --allowedTools. Also note there is no CLAUDE.md in the repo root.

---

[xinyuan-dev]

1. Three panics on a network-input path in ensure_canonical_symbol_len

i removed the panics.

2. The early rejection is best-effort, not airtight — worth a comment — low

this claim is not true for v1. in v1, the segment length is fixed. so the symbol length is canonical iff depth is canonical.

3. Validator-set skew now warns per chunk with no metric

live validators are expected to have their validator sets in sync, so no two honest validators have mismatching validator set for a given round, even at epoch boundaries. for lagging validators, blocks are sync'ed through blocksync/statesync.