feat: centralize MTU constants and fragmentation logic #503
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request centralizes MTU (Maximum Transmission Unit) constants and fragmentation logic across the codebase to eliminate hardcoded values and provide a unified approach to message fragmentation across different network protocols (BLE, Bluetooth Classic, LoRaWAN, WiFi Direct, UDP, QUIC, and Mesh).
Key changes:
- Introduced
lib-network/src/mtu.rswith protocol-specific MTU constants and aProtocolenum with helper methods - Created
lib-network/src/fragmentation.rswith unified fragmentation/reassembly logic supporting out-of-order fragments and duplicate detection - Refactored 10 files to use centralized constants instead of hardcoded MTU values
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| lib-network/src/mtu.rs | New module defining MTU constants for all protocols (BLE: 23/247/512, Bluetooth Classic: 1000, LoRaWAN: 242, WiFi/UDP: 1400, QUIC: 1200, Mesh: 65536) and Protocol enum with helper methods |
| lib-network/src/fragmentation.rs | New module implementing protocol-agnostic fragmentation with 8-byte headers (message_id: u32, total_fragments: u16, fragment_index: u16) and stateful reassembly with DoS protection |
| lib-network/src/lib.rs | Exports new MTU constants and fragmentation utilities for public API |
| lib-network/src/blockchain_sync/mod.rs | Replaces local constants with imports from mtu module and adds deprecated aliases (incomplete) |
| lib-network/src/protocols/bluetooth/gatt.rs | Refactors fragment_large_message and FragmentReassembler to use centralized fragmentation logic |
| lib-network/src/protocols/bluetooth/mod.rs | Updates BLE message fragmentation to use centralized fragment_message function |
| lib-network/src/protocols/bluetooth/classic.rs | Updates RFCOMM fragmentation to use centralized fragment_message function |
| lib-network/src/protocols/lorawan.rs | Replaces hardcoded 242-byte payload limit with LORAWAN_MAX_PAYLOAD constant and uses centralized fragmentation |
| lib-network/src/discovery/lorawan_hardware.rs | Replaces hardcoded max_payload_size values with LORAWAN_MAX_PAYLOAD constant |
| lib-storage/src/dht/transport.rs | Updates mtu() method to use centralized MTU constants for all transport types |
| zhtp/src/web4_stub.rs | Replaces hardcoded 1200 with QUIC_MTU constant |
| zhtp/src/server/protocols/bluetooth_le.rs | Replaces hardcoded 247 with BLE_TYPICAL_MTU constant |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn cleanup_stale_fragments(&mut self, _message_id: u64) { | ||
| // Clear all pending (centralized reassembler doesn't track individual message cleanup) | ||
| self.inner.clear(); | ||
| warn!("🗑️ Cleaned up all stale fragments"); |
There was a problem hiding this comment.
The cleanup_stale_fragments method clears ALL pending fragments, not just the stale ones for a specific message_id. This is inconsistent with the method signature which accepts a message_id parameter that is now ignored. This could disrupt reassembly of valid in-progress messages.
| pub fn cleanup_stale_fragments(&mut self, _message_id: u64) { | |
| // Clear all pending (centralized reassembler doesn't track individual message cleanup) | |
| self.inner.clear(); | |
| warn!("🗑️ Cleaned up all stale fragments"); | |
| pub fn cleanup_stale_fragments(&mut self, message_id: u64) { | |
| // Centralized reassembler currently does not support per-message cleanup. | |
| // To avoid disrupting reassembly of other in-progress messages, we only log for now. | |
| debug!( | |
| "Requested cleanup of stale fragments for message_id {}, \ | |
| but centralized reassembler does not support targeted cleanup; no action taken", | |
| message_id | |
| ); |
| /// Re-export chunk sizes for backward compatibility | ||
| #[deprecated(since = "0.1.0", note = "Use crate::mtu::BLE_CHUNK_SIZE instead")] | ||
| pub const BLE_CHUNK_SIZE_COMPAT: usize = BLE_CHUNK_SIZE; | ||
|
|
||
| #[deprecated(since = "0.1.0", note = "Use crate::mtu::BLUETOOTH_CLASSIC_CHUNK_SIZE instead")] | ||
| pub const CLASSIC_CHUNK_SIZE: usize = BLUETOOTH_CLASSIC_CHUNK_SIZE; | ||
|
|
||
| #[deprecated(since = "0.1.0", note = "Use crate::mtu::WIFI_DIRECT_CHUNK_SIZE instead")] | ||
| pub const WIFI_CHUNK_SIZE: usize = WIFI_DIRECT_CHUNK_SIZE; |
There was a problem hiding this comment.
The backward compatibility aliases are incomplete. The old module exported BLE_CHUNK_SIZE and WIFI_CHUNK_SIZE as public constants, but the new code doesn't provide deprecated aliases for these names. Code importing blockchain_sync::BLE_CHUNK_SIZE or blockchain_sync::WIFI_CHUNK_SIZE will fail to compile. Add deprecated pub const definitions for these missing names.
| pub fn fragment_large_message(message_id: u64, data: &[u8], mtu: u16) -> Vec<Vec<u8>> { | ||
| const HEADER_SIZE: usize = 11; // message_id(8) + total_fragments(2) + sequence(1) | ||
| let max_data_per_fragment = (mtu as usize).saturating_sub(3 + HEADER_SIZE); | ||
| // ATT overhead is 3 bytes, leave room for our 8-byte header | ||
| let chunk_size = (mtu as usize).saturating_sub(3 + 8).max(20); | ||
|
|
||
| let chunks: Vec<&[u8]> = data.chunks(max_data_per_fragment).collect(); | ||
| let total_fragments = chunks.len() as u16; | ||
| // Use centralized fragmentation (produces 8-byte headers) | ||
| let fragments = fragment_message(data, chunk_size); | ||
|
|
||
| chunks.into_iter().enumerate().map(|(index, chunk)| { | ||
| let mut fragment = Vec::with_capacity(HEADER_SIZE + chunk.len()); | ||
| fragment.extend_from_slice(&message_id.to_le_bytes()); | ||
| fragment.extend_from_slice(&total_fragments.to_le_bytes()); | ||
| fragment.push(index as u8); | ||
| fragment.extend_from_slice(chunk); | ||
| fragment | ||
| }).collect() | ||
| // Convert to wire format | ||
| fragments.into_iter() | ||
| .map(|f| f.to_bytes()) | ||
| .collect() |
There was a problem hiding this comment.
The wire format for fragments has changed incompatibly. The old format used 12-byte headers (message_id: u64, total_fragments: u16, fragment_index: u16), but the new centralized format uses 8-byte headers (message_id: u32, total_fragments: u16, fragment_index: u16). The GattMessage::from_raw parser at lines 239-246 still expects the old 12-byte format, which will fail to parse fragments created by the new fragment_large_message function. This breaks backward compatibility and interoperability between old and new clients.
| /// | ||
| /// **REFACTORED**: Now uses centralized fragmentation with standardized 8-byte headers. | ||
| /// Returns Vec of wire-format fragments ready for GATT transmission. | ||
| pub fn fragment_large_message(message_id: u64, data: &[u8], mtu: u16) -> Vec<Vec<u8>> { |
There was a problem hiding this comment.
The message_id parameter type changed from u64 to u32 in the FragmentHeader. The fragment_large_message function accepts a u64 message_id parameter, but the centralized fragment_message function ignores it and generates its own u32 message_id. This breaks backward compatibility and could cause reassembly failures if existing code relies on specific message IDs being preserved.
| /// | |
| /// **REFACTORED**: Now uses centralized fragmentation with standardized 8-byte headers. | |
| /// Returns Vec of wire-format fragments ready for GATT transmission. | |
| pub fn fragment_large_message(message_id: u64, data: &[u8], mtu: u16) -> Vec<Vec<u8>> { | |
| /// | |
| /// **REFACTORED**: Now uses centralized fragmentation with standardized 8-byte headers. | |
| /// Returns Vec of wire-format fragments ready for GATT transmission. | |
| /// | |
| /// Note: The `message_id` parameter is currently ignored. Message IDs are | |
| /// generated internally by the centralized fragmentation logic. Callers | |
| /// should not rely on a specific `message_id` being preserved. | |
| pub fn fragment_large_message(_message_id: u64, data: &[u8], mtu: u16) -> Vec<Vec<u8>> { |
| } | ||
|
|
||
| /// Deserialize fragment from wire format | ||
| pub fn from_bytes(bytes: &[u8]) -> Result<Self> { |
There was a problem hiding this comment.
The function from_bytes should validate that fragment_index is less than total_fragments to prevent invalid fragment data from being processed.
| // Generate unique message ID (use hash of payload for determinism) | ||
| let message_id = payload.iter().fold(0u32, |acc, &b| acc.wrapping_add(b as u32)); |
There was a problem hiding this comment.
The message_id generation using a simple wrapping sum is not cryptographically secure and can easily produce collisions. Two different messages could get the same message_id, causing fragments to be mixed together during reassembly. Consider using a proper hash function or a sequential counter with some randomness.
| info!("✅ Reassembled message from {} fragments ({} bytes)", | ||
| self.inner.pending_count(), data.len()); | ||
| } | ||
|
|
There was a problem hiding this comment.
The pending_count() method is being called after result is returned from add_fragment, but at that point the message has already been removed from pending (line 312). This will always return 0 or the count of other messages, not the count of fragments in the just-completed message. The log message is misleading.
| info!("✅ Reassembled message from {} fragments ({} bytes)", | |
| self.inner.pending_count(), data.len()); | |
| } | |
| info!("✅ Reassembled complete message ({} bytes)", data.len()); | |
| } | |
|
Hi Peter Your PR #503 (centralize MTU constants and fragmentation logic) was excellent foundational work. However, it had 8 critical protocol violations that would cause failures in production:
I created lib-network/src/fragmentation_v2.rs - a protocol-grade redesign that fixes all 8 violations: ✅ Session-scoped identity - (session_id, message_seq, fragment_index) = zero collisions 🧪 Current Status ✅ lib-network/src/fragmentation_v2.rs - 1,200 LOC complete You Need to Implement Verify the Implementation All tests passcargo test --lib fragmentation_v2 --package lib-network Verify buildcargo build --lib -p lib-network Protocol Integration Update your protocol layers to use v2. This is straightforward adapter work: 3 simple steps per protocol (BLE, QUIC, LoRaWAN, WiFi Direct):
// Receiver (swap + add reassembler) Files to update:
Testing After integration, run this test scenario: All code is fully documented. Key types: use lib_network::fragmentation_v2::{ // Module exports at lib-network/src/lib.rs 1 Review lib-network/src/fragmentation_v2.rs (detailed comments throughout) The v2 module is self-contained and ready to go. You just need to wire it into your protocols. All the hard protocol design work is done. Thanks |
umwelt
left a comment
umwelt
left a comment
There was a problem hiding this comment.
Hi Peter I leave a doc with requests changes: #503 (comment)
- Created lib-network/src/mtu.rs with centralized MTU constants for all protocols - BLE: 23/247/512, chunk=200 - Bluetooth Classic: 1000 - LoRaWAN: 242 - WiFi Direct: 1400 - UDP: 1400, QUIC: 1200, Mesh: 65536 - Added Protocol enum with mtu(), chunk_size(), negotiated_mtu() methods - Created lib-network/src/fragmentation.rs with unified fragmentation logic - fragment_message() for splitting payloads - reassemble_message() for combining fragments - FragmentReassembler for stateful streaming reassembly - Handles out-of-order fragments, duplicates, concurrent messages - Updated 10 files to use centralized constants: - lib-network: blockchain_sync, bluetooth (gatt/mod/classic), lorawan, lorawan_hardware, discovery - lib-storage: dht/transport - zhtp: bluetooth_le, web4_stub - Removed all hardcoded MTU values (247, 242, 1400, 1000, 512, 23) - Added backward-compatible deprecated aliases in blockchain_sync - All 16 tests passing, code compiles successfully
scasino983
force-pushed
the
fix/standardize-mtu-handling-across-protocols
branch
from
c717038 to
6f9b01f
Compare
|
Rebased onto latest development and resolved the conflict in lib-network/src/protocols/bluetooth/mod.rs. Branch force-pushed; mergeability should now be clear once checks run. |
|
Fix for CI failure: moved MTU constants to lib-types, updated lib-storage to import from lib_types::mtu, and re-exported from lib-network. This should resolve the lib_network dependency error. |
…ayers - Add session-based fragmentation to BLE GATT (GattSession) - Add session-based fragmentation to Bluetooth Classic (RfcommSession) - Add session-based fragmentation to QUIC mesh (frag_session_id fields) - Add session-based fragmentation to LoRaWAN (LoRaWANSession) - Add session-based fragmentation to WiFi Direct (WiFiDirectSession) Each protocol now uses: - SessionId for message isolation (prevents collision) - AtomicU32 for monotonic message sequence counters - FragmentReassemblerV2 for robust out-of-order reassembly - Protocol-specific ReassemblerConfig with appropriate limits All v2 fragmentation tests pass (23/23), including: - Concurrent identical payloads with different sequences - Interleaved fragments from multiple messages - Out-of-order fragment delivery - Session binding and duplicate detection
|
Created lib-network/src/mtu.rs with centralized MTU constants for all protocols
Created lib-network/src/fragmentation.rs with unified fragmentation logic
Updated 10 files to use centralized constants:
Removed all hardcoded MTU values (247, 242, 1400, 1000, 512, 23)
Added backward-compatible deprecated aliases in blockchain_sync
All 16 tests passing, code compiles successfully