-
Notifications
You must be signed in to change notification settings - Fork 3
snappy framing #50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
snappy framing #50
Conversation
turuslan
commented
Dec 25, 2025
- use snappy framing format for request/response protocols
- read/write response status
Signed-off-by: turuslan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements the snappy framing format for request/response protocols and adds response status handling to the networking layer.
- Implements snappy framing compression/decompression with CRC32 validation
- Adds response status byte read/write for protocol handshakes
- Updates networking protocols to use framed format and status handling
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| vcpkg.json | Adds crc32c dependency for CRC validation in snappy framing |
| CMakeLists.txt | Adds Crc32c package configuration |
| src/serde/snappy.hpp | Implements snappy framing format with compress/uncompress functions, CRC validation, and error handling |
| src/serde/CMakeLists.txt | Adds snappy library target with dependencies |
| tests/unit/serde/snappy_test.cpp | Adds basic test for framed compression round-trip |
| tests/unit/serde/CMakeLists.txt | Configures snappy test target |
| src/modules/networking/ssz_snappy.hpp | Adds framed encoding/decoding wrapper functions |
| src/modules/networking/response_status.hpp | Implements response status byte read/write for protocol handshakes |
| src/modules/networking/status_protocol.hpp | Declares new read/write helper methods |
| src/modules/networking/status_protocol.cpp | Refactors to use framed format and response status in bidirectional flow |
| src/modules/networking/block_request_protocol.cpp | Updates to use framed format, response status, and changes protocol ID |
| src/modules/networking/networking.cpp | Updates snappy function call to use new namespace |
| src/modules/networking/CMakeLists.txt | Updates dependency from Snappy::snappy to snappy library target |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return SnappyError::UNCOMPRESS_UNKNOWN_IDENTIFIER; | ||
| } | ||
| } else if (type == ChunkType::Compressed | ||
| or type == ChunkType::Uncompressed) { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing validation for minimum content size. For compressed and uncompressed chunk types, the content must be at least 4 bytes (CRC size) before calling subspan(Crc32::size()). If the content size is less than 4 bytes, this will cause undefined behavior or a crash.
| or type == ChunkType::Uncompressed) { | |
| or type == ChunkType::Uncompressed) { | |
| if (content.size() < Crc32::size()) { | |
| return SnappyError::UNCOMPRESS_TRUNCATED; | |
| } |
| } | ||
| auto type = ChunkType{compressed[0]}; | ||
| auto size = boost::endian::load_little_u24(compressed.data() + 1); | ||
| if (compressed.size() < kHeaderSize + size) { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential integer overflow when calculating header size plus content size. If size is close to SIZE_MAX, the expression kHeaderSize + size could overflow before the comparison check is performed. Consider checking if size > SIZE_MAX - kHeaderSize before performing the addition, or use a saturating arithmetic function.
| if (compressed.size() < kHeaderSize + size) { | |
| if (size > compressed.size() - kHeaderSize) { |
| auto actual_crc = hashCrc32(uncompressed); | ||
| if (qtils::ByteView{actual_crc} != expected_crc) { | ||
| return SnappyError::UNCOMPRESS_CRC_MISMATCH; | ||
| } |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing validation to ensure total uncompressed size does not exceed max_size. While individual chunks are checked against the remaining space using saturating_sub, there's no check to verify that adding the uncompressed data to result won't exceed max_size before calling result.put(). This could allow the total result to exceed the specified max_size limit.
| } | |
| } | |
| auto remaining = libp2p::saturating_sub(max_size, result.size()); | |
| if (uncompressed.size() > remaining) { | |
| return SnappyError::UNCOMPRESS_TOO_LONG; | |
| } |
| inline qtils::ByteVec compressFramed(qtils::BytesIn input) { | ||
| qtils::ByteVec framed; | ||
| auto write_header = [&](ChunkType type, size_t size) { | ||
| framed.putUint8(type); | ||
| qtils::ByteArr<3> size_bytes; | ||
| boost::endian::store_little_u24(size_bytes.data(), size); | ||
| framed.put(size_bytes); | ||
| }; | ||
| write_header(ChunkType::Stream, kStreamIdentifier.size()); | ||
| framed.put(kStreamIdentifier); | ||
| while (not input.empty()) { | ||
| auto chunk = input.first(std::min(input.size(), kMaxBlockSize)); | ||
| auto crc = hashCrc32(chunk); | ||
| input = input.subspan(chunk.size()); | ||
| auto compressed = compress(chunk); | ||
| write_header(ChunkType::Compressed, Crc32::size() + compressed.size()); | ||
| framed.put(crc); | ||
| framed.put(compressed); | ||
| } | ||
| return framed; | ||
| } |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing documentation for the new functions. The compressFramed and uncompressFramed functions implement the snappy framing format but lack documentation explaining the format, parameters (especially the max_size parameter and its default value), return values, and potential error conditions.
| inline libp2p::CoroOutcome<void> writeResponseStatus( | ||
| std::shared_ptr<libp2p::basic::Writer> writer) { | ||
| qtils::ByteArr<1> status{0}; | ||
| BOOST_OUTCOME_CO_TRY(co_await libp2p::write(writer, status)); | ||
| co_return outcome::success(); | ||
| } | ||
|
|
||
| inline libp2p::CoroOutcome<void> readResponseStatus( | ||
| std::shared_ptr<libp2p::basic::Reader> reader) { | ||
| qtils::ByteArr<1> status; | ||
| BOOST_OUTCOME_CO_TRY(co_await libp2p::read(reader, status)); | ||
| co_return outcome::success(); | ||
| } |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing documentation for response status protocol functions. The writeResponseStatus and readResponseStatus functions lack documentation explaining their purpose, the meaning of the status byte value (currently hardcoded to 0), and what different status values represent.
| auto write_header = [&](ChunkType type, size_t size) { | ||
| framed.putUint8(type); | ||
| qtils::ByteArr<3> size_bytes; | ||
| boost::endian::store_little_u24(size_bytes.data(), size); | ||
| framed.put(size_bytes); | ||
| }; | ||
| write_header(ChunkType::Stream, kStreamIdentifier.size()); | ||
| framed.put(kStreamIdentifier); | ||
| while (not input.empty()) { | ||
| auto chunk = input.first(std::min(input.size(), kMaxBlockSize)); | ||
| auto crc = hashCrc32(chunk); | ||
| input = input.subspan(chunk.size()); | ||
| auto compressed = compress(chunk); | ||
| write_header(ChunkType::Compressed, Crc32::size() + compressed.size()); |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent naming convention for local variable. The lambda function is named with snake_case (write_header) while the surrounding code uses camelCase for local variables. Consider renaming to writeHeader for consistency with the codebase style.
| auto write_header = [&](ChunkType type, size_t size) { | |
| framed.putUint8(type); | |
| qtils::ByteArr<3> size_bytes; | |
| boost::endian::store_little_u24(size_bytes.data(), size); | |
| framed.put(size_bytes); | |
| }; | |
| write_header(ChunkType::Stream, kStreamIdentifier.size()); | |
| framed.put(kStreamIdentifier); | |
| while (not input.empty()) { | |
| auto chunk = input.first(std::min(input.size(), kMaxBlockSize)); | |
| auto crc = hashCrc32(chunk); | |
| input = input.subspan(chunk.size()); | |
| auto compressed = compress(chunk); | |
| write_header(ChunkType::Compressed, Crc32::size() + compressed.size()); | |
| auto writeHeader = [&](ChunkType type, size_t size) { | |
| framed.putUint8(type); | |
| qtils::ByteArr<3> size_bytes; | |
| boost::endian::store_little_u24(size_bytes.data(), size); | |
| framed.put(size_bytes); | |
| }; | |
| writeHeader(ChunkType::Stream, kStreamIdentifier.size()); | |
| framed.put(kStreamIdentifier); | |
| while (not input.empty()) { | |
| auto chunk = input.first(std::min(input.size(), kMaxBlockSize)); | |
| auto crc = hashCrc32(chunk); | |
| input = input.subspan(chunk.size()); | |
| auto compressed = compress(chunk); | |
| writeHeader(ChunkType::Compressed, Crc32::size() + compressed.size()); |
| } else if (type == ChunkType::Compressed | ||
| or type == ChunkType::Uncompressed) { | ||
| qtils::ByteVec buffer; | ||
| auto expected_crc = content.first(Crc32::size()); | ||
| auto uncompressed = content.subspan(Crc32::size()); | ||
| if (type == ChunkType::Compressed) { | ||
| BOOST_OUTCOME_TRY( | ||
| buffer, | ||
| uncompress(uncompressed, | ||
| libp2p::saturating_sub(max_size, result.size()))); | ||
| uncompressed = buffer; | ||
| } |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing validation for uncompressed chunk size. When type is ChunkType::Uncompressed, the uncompressed data size is not checked against max_size before being added to the result. Unlike the compressed case which uses saturating_sub to limit the size, uncompressed chunks could exceed the remaining allowed space.