fix: saturate Fin sequence arithmetic in proposal-part streaming

[memosr]

Summary

StreamState::insert in the consensus proposal-part streaming layer
computed a stream's expected message count from the Fin message's
sequence field as msg.sequence as usize + 1. sequence is an
unvalidated u64 decoded straight off the gossip wire
(crates/types/src/codec/proto.rs — sequence: proto.sequence, no
bound-check), and PartStreamsMap::insert applies no upper bound on it
before reaching this arithmetic. A peer on the consensus gossip topic can
therefore send a Fin part for the current height with sequence = u64::MAX and drive the + 1 to overflow:

• debug / test builds (overflow-checks on): panic → node crash.
• release builds (overflow-checks off): wraps to 0.

Release builds are incidentally safe today: the later
buffer.len() == expected_messages completion check never matches a
wrapped-to-zero target, so the stream stays Incomplete and is evicted
by the age/limit sweep. No crash, no unbounded memory (already capped by
MAX_MESSAGES_PER_STREAM). The safety rested on wrapping behaviour
rather than intent, and the old comment asserted a false invariant — it
claimed the + 1 "cannot overflow because MAX_MESSAGES_PER_STREAM <<
u64::MAX", but expected_messages is assigned from the unbounded
msg.sequence, not from the bounded message_count.

Change

• Use saturating_add(1): worst case is usize::MAX, an unreachable
  completion target the stream is evicted for — never a panic or a silent
  wrap-to-zero.
• Rewrite the comment to describe the actual (peer-controlled, unbounded)
  source of sequence.
• Drop the now-unnecessary clippy::arithmetic_side_effects allow, since
  the arithmetic is checked.

Impact

Eliminates a remotely reachable panic in debug/test builds and removes
release builds' latent reliance on wrap-to-zero. No behavioural change on
valid streams (sequence values are < MAX_MESSAGES_PER_STREAM in
practice). No public API or wire-format change; the touched method is
module-private and PartStreamsMap::insert's signature is unchanged.

Test plan

• cargo build -p arc-node-consensus — clean
• cargo test -p arc-node-consensus streaming — 46 passed, 0 failed
  (includes property tests for per-stream / total stream limits),
  run under debug-assertions

🤖 Generated with Claude Code

[osr21]

Nice fix — the saturating_add(1) change is the right call here. Worth noting for reviewers: the old comment's invariant ("+1 cannot overflow because MAX_MESSAGES_PER_STREAM << u64::MAX") conflated the bounded local constant with the unbounded wire value assigned into expected_messages — easy mistake to make since the two get used together right after. Good catch separating those in the new comment.

On the README "Networks" section: can independently corroborate both corrections from our own testnet integration —

• eth_chainId against https://rpc.testnet.arc.network returns 0x4cef52 (5042002), consistent with the PR
• https://testnet.arcscan.app is the explorer we've had working reliably for contract verification/tx links; we never got explorer.testnet.arc.network to resolve

Good to see these codified in the README directly rather than left to circulate informally — the 1516 chain ID mixup in particular seems like the kind of thing that causes real wallet-connection support tickets.

[memosr]

Update: the three README commits that were originally in this branch have been
split out into a separate docs PR. This PR is now a single commit touching one
file.

[osr21]

Disclosure: I'm not affiliated with Circle — an external community contributor, not a maintainer. I have no write access to this repository, so any review state I set (approval or change request) carries no merge authority and is advisory only. Please treat this as one contributor's technical assessment, and defer to Circle maintainers for the binding review.

---

Confirmed the split: this branch is now a single commit touching only crates/malachite-app/src/streaming.rs, and #263 carries the README material — reviewing each on its own merits is now possible, which is what the security fix deserved.

Re-verified the premise against a fresh clone of main (static verification on my side — no Rust toolchain here, so your 46-test run and CI are the executable check):

• streaming.rs:287 still reads self.expected_messages = msg.sequence as usize + 1; under the comment asserting "+1 cannot overflow because MAX_MESSAGES_PER_STREAM << u64::MAX" — and your diagnosis of that comment is exactly right: the bound applies to message_count (checked at line 260), while expected_messages is assigned from the unbounded wire value. Two different quantities, one invariant claimed for both.
• The "release builds are incidentally safe" reasoning also holds up: after a Fin insert the buffer holds at least that one message, so buffer.len() == 0 can never match and the wrapped stream parks as Incomplete until eviction. Correctly scoping this as debug/test-panic + latent-reliance rather than a live release crash is the kind of severity honesty that makes a security PR easy to trust.
• saturating_add(1) is the right primitive: usize::MAX is an unreachable completion target given the MAX_MESSAGES_PER_STREAM cap on inserts, so the malicious stream dies by eviction on both build profiles. And dropping the arithmetic_side_effects allow while keeping cast_possible_truncation is the correct split — the former is now genuinely handled, the latter is still real.

One residual nuance worth a line in the comment, non-blocking: the retained truncation allow means on a 32-bit target msg.sequence as usize truncates before the saturating add — e.g. 2^32 + 5 becomes 5, so a malicious Fin could set a small-but-wrong completion target rather than an unreachable one. On 64-bit targets (every realistic node deployment, and what the old comment already assumed) this is moot, but since the new comment's theme is "this field is peer-controlled and unbounded," it might as well state the 64-bit assumption explicitly instead of inheriting it silently from the allow.

One suggestion that would strengthen the PR at near-zero cost: the test plan runs the existing suite, but nothing pins the new behavior. A regression test that inserts a Fin with sequence = u64::MAX under debug assertions — asserting no panic and that the stream remains incomplete — would (a) fail on today's main, proving the fix, and (b) stop a future refactor from quietly reintroducing the + 1. Given the property tests already in the module, it should slot in as a few lines.

Fix is correct as-is; both notes are hardening, not blockers.

[memosr]

Both notes applied, thanks.

64-bit assumption: the comment now states it explicitly instead of leaving it
implicit in the retained cast_possible_truncation allow, including what would
go wrong on a 32-bit target (truncation before the saturating add gives a
small-but-wrong completion target, still bounded and still evicted, but wrong).

Regression test: added test_fin_with_max_sequence_does_not_overflow. It inserts
an Init at sequence 0 then a Fin at u64::MAX, and asserts the insert does not
panic, the stream does not complete, and it stays in the map pending eviction.

Verified it behaves as a regression test should. With the fix, 47/47 streaming
tests pass. Reverting just the saturating_add back to + 1 on the same tree:

thread 'streaming::tests::test_fin_with_max_sequence_does_not_overflow' panicked at
crates/malachite-app/src/streaming.rs:299:42:
attempt to add with overflow

So it fails on pre-fix code and passes after, which is what pins the behavior.

[osr21]

Disclosure: I'm not affiliated with Circle — an external community contributor, not a maintainer. I have no write access to this repository, so any review state I set (approval or change request) carries no merge authority and is advisory only. Please treat this as one contributor's technical assessment, and defer to Circle maintainers for the binding review.

---

Reviewed 62df0de — both notes are closed properly, and the way they were closed is worth remarking on:

The 64-bit comment now does more than state the assumption: it describes the exact 32-bit failure mode (truncation before the saturating add → small-but-wrong completion target) and its bounded consequence (still evicted, just wrong). That's the right level of detail — a future reader porting this to a 32-bit target gets the full threat analysis at the site of the cast, not just a warning flag. The old comment asserted a false invariant; the new one documents a true, explicitly-scoped one.

The regression test is textbook. Three properties asserted — no panic, no completion, stream stays pending eviction — which pin both build-profile failure modes at once: the debug panic (the insert itself) and the release wrap-to-zero (completion would have required matching a wrapped target). And your revert-and-rerun proof is the part I'd highlight for the maintainers: reverting only the saturating_add back to + 1 on the same tree makes the new test fail with attempt to add with overflow at exactly the fixed line, then 47/47 pass with the fix. A regression test that has been demonstrated to fail on pre-fix code is categorically stronger evidence than one that merely passes post-fix — it can't be vacuous.

Static verification on my side as before (the diff structure, assertion logic, and test-helper usage all check out against the module's existing patterns; your executed runs and CI are the ground truth). Nothing further from me — the fix was already correct, and now it's also self-defending: the false invariant is replaced by a documented true one, and the arithmetic is pinned by a test that fails loudly if anyone walks it back. Ready for a maintainer, and between this and #263 the split has served both halves well.

[memosr]

Re-verified this against current main (90d71dc).

crates/malachite-app/src/streaming.rs:287 still reads:

self.expected_messages = msg.sequence as usize + 1;

msg.sequence is attacker-controlled u64 off the wire, and as usize is a no-op widening on 64-bit targets, so + 1 overflows at u64::MAX: panic in debug, wraparound to 0 in release. The fix in this PR still applies cleanly, and the branch is based on 90d71dc so no rebase is needed.

For precedent, v0.7.0 shipped two fixes of exactly this shape: Use checked arithmetic in total_voting_power() (CL) and Use checked arithmetic for cumulative gas accounting in payload builder (EL). This is the same class of change in the same crate family.

Happy to adjust the approach if you would rather this saturate elsewhere or return an error instead.

[osr21]

Independently re-verified everything in your last comment against main (90d71dc) — all four points hold:

• crates/malachite-app/src/streaming.rs:287 still reads self.expected_messages = msg.sequence as usize + 1;, still under both cast_possible_truncation and arithmetic_side_effects allows (line 285), and the false-invariant comment ("+1 cannot overflow because MAX_MESSAGES_PER_STREAM << u64::MAX") is still at line 284.
• crates/types/src/codec/proto.rs:306 decodes sequence: proto.sequence with no bound check, so it's genuinely attacker-controlled off the wire.
• Tracing the guards ahead of line 287: seen_sequences dedup (254) and the message_count >= MAX_MESSAGES_PER_STREAM cap (260) both operate on count, never on the value of msg.sequence — so a single Fin at u64::MAX sails past both and reaches the + 1. Your attack path is exact.
• Both v0.7.0 precedents are real and in the same crate family — CHANGELOG lines for [CL] Use checked arithmetic in total_voting_power() and [EL] Use checked arithmetic for cumulative gas accounting in payload builder. Good precedent to cite; this is the same class of fix.

On your open question ("saturate elsewhere or return an error instead") — I'd keep saturating_add(1) as the merge candidate. It's the minimal, behavior-preserving fix: valid streams are unaffected, and the malicious case degrades to an unreachable completion target that the existing age/limit sweep already evicts. Returning an error from this arm would be a larger, higher-risk change to the insert contract for no additional safety, since the eviction path already bounds memory.

That said, there's a strictly-stronger optional hardening worth considering as a follow-up (not a blocker for this PR): an explicit upper-bound reject on the Fin sequence, mirroring the existing message_count guard at line 260 — e.g.

if msg.sequence >= MAX_MESSAGES_PER_STREAM as u64 {
    return StreamInsertResult::ExceededMaxMessages; // or a dedicated variant
}

A legitimate Fin.sequence can never reach MAX_MESSAGES_PER_STREAM (a valid stream has at most that many parts), so this rejects the malformed part immediately instead of buffering stream state and waiting for the sweep to reclaim it. It converts "silently evicted later" into "denied up front," and it composes with peer-scoring/DoS accounting since the reject is attributable to the sending peer. But that's an enhancement to the bounding model, whereas this PR is correctly scoped to removing the panic/wrap — I wouldn't hold the merge for it. saturating_add(1) + the corrected comment + the test_fin_with_max_sequence_does_not_overflow regression test is the right, reviewable unit.

Approving as-is.