fix: Enforce correct endianness in Segment retransmission request of the Session protocol #7235
Conversation
…the Session protocol Invalid unit test led to hiding an incorrect encoding of the missing segment bitmap.
📝 Walkthrough""" WalkthroughThe changes refactor the handling of missing segment bitmaps in the session frame reassembly logic, reducing the bitmap size to match the number of bits in Changes
Sequence Diagram(s)sequenceDiagram
participant Sender
participant FrameBuilder
participant FrameReassembler
Sender->>FrameBuilder: Build frame
FrameBuilder->>FrameBuilder: Initialize missing_segments bitmap (8 bits, Msb0)
FrameBuilder->>Sender: Return FrameInfo with missing_segments
Sender->>FrameReassembler: Send FrameInfo
FrameReassembler->>FrameReassembler: Use missing_segments bitmap for reassembly
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
common/network-types/src/session/protocol.rs (1)
295-300:⚠️ Potential issueProtocol constant now caps a frame at 8 segments but segmentation still allows 256
SessionMessage::MAX_SEGMENTS_PER_FRAMEwas reduced to 8, yet
segment()inframe.rscontinues to accept up toSeqNum::MAX(256) chunks.
Frames larger than 8 segments will therefore:
- Be produced and sent successfully, but
- Lose information about missing segments ≥ 8 (only 8-bit bitmap), breaking retransmission.
Either enforce the 8-segment limit at segmentation time or restore a wider bitmap.
🧹 Nitpick comments (2)
common/network-types/src/session/protocol.rs (2)
59-60: Remove unusedbitvec::field::BitFieldimportThe trait isn’t referenced anywhere in this module after the refactor, so it will raise an “unused import” warning (and could fail the build if
#![deny(warnings)]is enabled).-use bitvec::field::BitField;
104-116: Over-allocates vector capacity by 8×
seq_sizealready equals the number of bits (8).
Vec::with_capacity(seq_size * 8 * self.0.len())reserves64 * leneven though at most8 * lenitems can be pushed.- let mut ret = Vec::with_capacity(seq_size * 8 * self.0.len()); + let mut ret = Vec::with_capacity(seq_size * self.0.len());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
common/network-types/Cargo.toml(1 hunks)common/network-types/src/session/frame.rs(5 hunks)common/network-types/src/session/protocol.rs(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: hoprd / docker
- GitHub Check: tests-unit-nightly
- GitHub Check: tests-unit
- GitHub Check: zizmor
- GitHub Check: Linter
🔇 Additional comments (2)
common/network-types/Cargo.toml (1)
3-3: Version bump is straightforward and harmless
0.7.1accurately reflects the API changes introduced in this PR.
No further action required.common/network-types/src/session/protocol.rs (1)
106-113: Bit ⇄ segment-index mapping is inverted – please verifyThe loop scans bits from MSB→LSB (
mask = 1 << (seq_size - i - 1)) but then emitsSegmentId(frame_id, i).
With the newMsb0bitmap, segment0is stored in bit 7, yet here it is returned as index0. That reverses the sequence order and could retransmit the wrong segments.Please double-check the expected mapping and, if the intention is to return the real sequence index (LSB 0), adjust the calculation to:
let mask = 1 << i;or keep the mask and emit
SegmentId(frame_id, (seq_size - i - 1) as SeqNum).
Invalid unit test led to hiding an incorrect encoding of the missing segment bitmap.