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]

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]

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.