fix(ssz): ignore BitVector padding bits#329
fix(ssz): ignore BitVector padding bits#329lodekeeper-z wants to merge 2 commits intoChainSafe:feat/beacon-nodefrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where padding bits in BitVectors were incorrectly interpreted as valid data, leading to crashes during attestation processing. By implementing a masking mechanism for non-byte-aligned bits, the changes ensure that only relevant bits are processed, improving the robustness of SSZ type handling. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a maskedByte helper function to BitVector in src/ssz/type/bit_vector.zig to ensure that padding bits are ignored during bitwise operations. This helper is applied across getTrueBitIndexes, getSingleTrueBit, and intersectValues, and a corresponding test case has been added. The reviewer suggested refactoring the maskedByte implementation to comply with the repository's style guide, specifically regarding the decomposition of compound conditions and the inclusion of pre- and post-condition assertions.
| fn maskedByte(self: *const @This(), i_byte: usize) u8 { | ||
| const remainder_bits = length % 8; | ||
| if (remainder_bits == 0 or i_byte + 1 < byte_len) { | ||
| return self.data[i_byte]; | ||
| } | ||
| const tail_mask: u8 = (@as(u8, 1) << @intCast(remainder_bits)) - 1; | ||
| return self.data[i_byte] & tail_mask; | ||
| } |
There was a problem hiding this comment.
The maskedByte function should be refactored to adhere to the repository style guide:
- Control Flow (Rule 133): Split the compound condition into nested
if/elsebranches. This improves readability and ensures that the logic for handling the last byte is explicitly separated from the common case. - Assertions (Rule 51): Add a precondition assertion to verify that
i_byteis within bounds. This helps catch programmer errors early and documents the function's contract. - Pair Assertions (Rule 58): Add an assertion to verify that the resulting byte has no bits set beyond the logical length. This provides defense-in-depth and documents the expected state of the data.
- Assertion Style (Rule 68): Use single-line
iffor assertions of implications where appropriate.
fn maskedByte(self: *const @This(), i_byte: usize) u8 {
std.debug.assert(i_byte < byte_len);
const remainder_bits = length % 8;
if (remainder_bits == 0) {
return self.data[i_byte];
} else {
if (i_byte + 1 < byte_len) {
return self.data[i_byte];
}
}
const tail_mask: u8 = (@as(u8, 1) << @intCast(remainder_bits)) - 1;
const byte = self.data[i_byte] & tail_mask;
// Pair assertion: ensure no bits are set beyond the logical length.
if (remainder_bits > 0) std.debug.assert(@clz(byte) >= 8 - remainder_bits);
return byte;
}
|
I think maskedByte should only be needed for the final byte |
|
Updated in fe4768a: Verification:
|
Summary
Production context
Test plan