fix: change BlocksByRoot container de/serialization

[MegaRedHand]

The request type was changed in leanEthereum/leanSpec@054b2e9. This PR updates it to the latest format, while keeping backwards compatibility with the other one. This is because Zeam uses the new format, while the rest of clients use the old one. Once everyone moves to the new format we can remove this patch.

[github-actions]

Kimi AI Code Review

### Review

Code Correctness and Potential Bugs

1. Lines 99-100: The change from request.len() to request.roots.len() is correct, as the request now contains a roots field which is a RequestedBlockRoots type. This ensures the correct number of roots in the request is retrieved.

2. Lines 166-167: The change from BlocksByRootRequest to RequestedBlockRoots is correct, as the new structure of BlocksByRootRequest now includes a roots field. This change is consistent with the updated structure.

3. Lines 46-49: The addition of RequestedBlockRoots and the BlocksByRootRequest struct is correct. It encapsulates the roots in a struct, which is a good practice for clarity and maintainability.

Security Vulnerabilities

The changes do not introduce any new security vulnerabilities. However, it's important to ensure that the de/serialization process is secure and does not introduce any vulnerabilities, such as deserialization of untrusted data leading to unexpected behavior.

Performance Implications

The changes are structural and do not directly impact performance. However, it's important to monitor the performance after these changes, especially if the new structure leads to additional overhead in de/serialization.

Rust Best Practices and Idiomatic Patterns

1. Lines 166-167: The use of RequestedBlockRoots::empty() is idiomatic Rust, as it leverages the empty method provided by the ssz_types::VariableList to create an empty list.

2. Lines 46-49: The struct BlocksByRootRequest follows Rust's best practices by encapsulating the roots field, making the API more explicit and easier to understand.

Memory Safety and Proper Error Handling

1. Lines 166-167: The error handling for pushing a root into the roots list is appropriate. It logs an error and returns false if the operation fails, which is a reasonable approach to handle such errors.

Code Readability and Maintainability

1. Lines 46-49: The new BlocksByRootRequest struct improves readability by clearly separating the roots field, making the code easier to understand and maintain.

2. Lines 99-100 and 166-167: The changes make the code more readable by using more descriptive field names and structuring the data in a more intuitive way.

Consensus-Layer Considerations

The changes do not directly affect consensus-layer logic such as fork choice, attestation processing, or state transition functions. However, it's important to ensure that these changes do not inadvertently affect the serialization and deserialization of consensus-related data.

Conclusion

The changes in this PR are well-structured and follow Rust best practices. They improve code readability and maintainability without introducing new security vulnerabilities or performance issues. The changes are correct and should be merged. However, it's crucial to thoroughly test the updated serialization and deserialization logic to ensure there are no regressions or unintended consequences.

---

Automated review by Kimi (Moonshot AI)

[github-actions]

Kimi AI Code Review

### Review

Code Correctness and Potential Bugs

1. Makefile Changes:

   • The addition of leanSpec/fixtures in the test target seems unrelated to the changes in the codebase. This might be a leftover from another change or a mistake. It should be removed unless there's a specific reason for it being there.
   • Line: 6

2. Handlers.rs Changes:

   • The change from request.len() to request.roots.len() is correct as it now correctly accesses the length of the roots field in the BlocksByRootRequest struct.
   • Line: 99
   • The creation of BlocksByRootRequest has been changed to use RequestedBlockRoots. This is correct and aligns with the new struct definition in messages.rs.
   • Line: 166

3. Messages.rs Changes:

   • The introduction of RequestedBlockRoots and the BlocksByRootRequest struct is a good refactoring step. It makes the code more explicit and easier to understand.
   • Line: 46
   • The Encode and Decode traits are derived for BlocksByRootRequest, which is necessary for serialization and deserialization. Ensure that these derive macros are correctly implemented for the fields.
   • Line: 49

4. Mod.rs Changes:

   • The update to include RequestedBlockRoots in the public re-exports is correct and necessary for the changes to be visible in other modules.
   • Line: 7

Security Vulnerabilities

• The changes do not introduce any obvious security vulnerabilities. However, it's important to ensure that the serialization and deserialization of BlocksByRootRequest are secure and do not lead to any potential attacks, such as deserialization of malicious data.

Performance Implications

• The changes are structural and do not directly impact performance. However, the introduction of a new struct for BlocksByRootRequest might have minor implications on memory usage and serialization performance. It's important to monitor and profile the application to ensure there are no regressions.

Rust Best Practices and Idiomatic Patterns

• The use of ssz_types::VariableList for RequestedBlockRoots is idiomatic and aligns with common practices in Rust for handling variable-length lists.
• The refactoring to introduce a separate struct for BlocksByRootRequest is a good practice as it encapsulates the data and makes the code more maintainable.

Memory Safety and Proper Error Handling

• The error handling in fetch_block_from_peer is appropriate, logging the error and returning false when the request creation fails.
• Line: 166

Code Readability and Maintainability

• The changes improve readability by making the code more explicit and structured.
• The introduction of RequestedBlockRoots and BlocksByRootRequest structs makes the code easier to maintain and understand.

Conclusion

The changes in this PR are well-structured and improve the codebase by making it more explicit and maintainable. The refactoring to introduce RequestedBlockRoots and BlocksByRootRequest is a good practice. However, ensure that the leanSpec/fixtures addition in the Makefile is intentional or remove it if it's a mistake. Overall, the changes look good and adhere to Rust best practices.

---

Automated review by Kimi (Moonshot AI)

[github-actions]

Code review by ChatGPT

[github-actions]

Code Review Comments

1. Code Correctness & Potential Bugs:

   • The restructuring of imports in the diff snippet seems fine (req_resp::RequestedBlockRoots now bundled with RetryMessage). Ensure that all referenced modules are correctly imported and relevant to the operations used in handle_req_resp_message.

2. Security Vulnerabilities:

   • Ensure any network or P2P message handling follows strict validation and sanitization checks in handle_req_resp_message. There is no code shown for handle_req_resp_message itself. Please verify the implementation inside for security best practices.

3. Performance Implications:

   • No performance concerns are evident in this snippet. The performance impact usually occurs in how requests are handled asynchronously. Cross-check that asynchronous processing within handle_req_resp_message is optimized.

4. Rust Best Practices and Idiomatic Patterns:

   • Organizing imports should conform to project standards, e.g., standard, external crates, and internal module imports are usually separated by categories.

5. Memory Safety and Proper Error Handling:

   • Ensure that proper error handling mechanisms are implemented within handle_req_resp_message. Use Result and Option efficiently to handle potential errors gracefully.

6. Code Readability and Maintainability:

   • The current structure of imports suggests good readability. If these are the only changes in this PR, ensure adequate comments and documentation within handle_req_resp_message for continued maintainability.

7. Consensus-layer Considerations:

   • Unfortunately, the PR snippet provided does not directly touch on consensus mechanisms like LMD GHOST, attestation processing, justification, finalization logic, or XMSS verification. Reviewers must ensure any implementation of these consensus protocols and signature operations in the code have been handled correctly against the relevant Ethereum consensus specifications.

8. SSZ Encoding/Decoding:

   • There is insufficient context here to comment on SSZ encoding/decoding, but if related handling is present in handle_req_resp_message, ensure that encoding and decoding adhere to SSZ standards without data corruption or incorrect parsing.

Conclusion

To ensure these points are adequately addressed, additional context or information about the specific changes within handle_req_resp_message and possibly its implementation should be provided. More comprehensive scrutiny of how these modifications impact the overall consensus logic and protocol integrity is recommended before proceeding with the merge.

[github-actions]

Kimi AI Code Review

### Code Review

Makefile

Change:

-test: ## 🧪 Run all tests, then forkchoice tests with skip-signature-verification
+test: leanSpec/fixtures ## 🧪 Run all tests, then forkchoice tests with skip-signature-verification

Review:

• The addition of leanSpec/fixtures as a dependency for the test target is unclear. It's not specified what this does or why it's necessary. Please provide more context or documentation for this change.

handlers.rs

Change:

-    let num_roots = request.len();
+    let num_roots = request.roots.len();

Review:

• This change correctly updates the code to use the new roots field in the BlocksByRootRequest struct. This is a necessary change to maintain functionality after the refactor.

Change:

-    let mut request = BlocksByRootRequest::empty();
+    let mut roots = RequestedBlockRoots::empty();

Review:

• This change correctly updates the code to use the new RequestedBlockRoots type. However, the variable name roots is more descriptive and aligns with the new type, which is good.

Change:

+    let request = BlocksByRootRequest { roots };

Review:

• This change correctly constructs a BlocksByRootRequest from the roots variable. This is necessary to maintain the structure of the request.

messages.rs

Change:

-pub type BlocksByRootRequest = ssz_types::VariableList<H256, MaxRequestBlocks>;
+pub type RequestedBlockRoots = ssz_types::VariableList<H256, MaxRequestBlocks>;
+
+#[derive(Debug, Clone, Encode, Decode)]
+// TODO: remove this for devnet 3
+#[ssz(struct_behaviour = "transparent")]
+pub struct BlocksByRootRequest {
+    pub roots: RequestedBlockRoots,
+}

Review:

• The introduction of RequestedBlockRoots and the BlocksByRootRequest struct is a good refactor to encapsulate the roots in a struct. This improves the readability and maintainability of the code.
• The // TODO: remove this for devnet 3 comment suggests that this is a temporary change. It would be helpful to include more details on why this is temporary and what the final implementation will look like.

mod.rs

Change:

-    BLOCKS_BY_ROOT_PROTOCOL_V1, BlocksByRootRequest, Request, Response, ResponsePayload,
-    ResponseResult, STATUS_PROTOCOL_V1, Status,
+    BLOCKS_BY_ROOT_PROTOCOL_V1, BlocksByRootRequest, Request, RequestedBlockRoots, Response,
+    ResponsePayload, ResponseResult, STATUS_PROTOCOL_V1, Status,

Review:

• This change correctly updates the public re-exports to include the new RequestedBlockRoots type. This is necessary to make the new type available to other modules.

Conclusion

The changes in this PR are mostly good, improving the structure and readability of the code. The introduction of RequestedBlockRoots and BlocksByRootRequest is a positive refactor. However, the // TODO: remove this for devnet 3 comment requires more context to understand the long-term plan for this code. Additionally, the Makefile change is unclear and needs more documentation.

Overall Rating: 👍 (with reservations due to TODO comment and Makefile change)

---

Automated review by Kimi (Moonshot AI)

[github-actions]

Code review by ChatGPT

[github-actions]

1. Performance & Correctness: The #[ssz(struct_behaviour = "transparent")] attribute suggests that the BlocksByRootRequest struct should be treated as a transparent wrapper over its only field, which can have implications on how this struct is serialized and deserialized. Make sure you really want this behavior, as it might impact how data is encoded/decoded and lead to potential issues with SSZ consistency. Verify that the transparency is suitable for the data being handled.

2. Comments & Maintainability: The // TODO: remove this for devnet 3 comment suggests that this change is temporary. Ensure to track this change and verify the potential impact when it is removed. It might be beneficial to include a more detailed explanation for future maintainers about why this is temporary and under what conditions it should be removed.

3. General Code Quality: Considering the critical nature of consensus layers in blockchain, ensure that any modifications are rigorously tested, especially around serialization/deserialization and the fork choice rule implementation.

4. Additional Checks: Double-check that the RequestedBlockRoots type usage aligns with the expected maximum and minimum sizes, as incorrect usage here can lead to memory issues or overflow vulnerabilities in extreme cases.

5. Rust Best Practices: Make sure to handle any potential panic situations appropriately, especially given the critical domain of blockchain technology, where consensus safety is paramount.

Due to the above concerns, particularly around the temporary SSZ attribute, the PR is not ready for merging.

[github-actions]

Kimi AI Code Review

This PR introduces changes to the `BlocksByRootRequest` container's serialization and deserialization. Here are my detailed comments and suggestions:

1. Codec Changes:

   • The change in codec.rs to use from_ssz_bytes_compat instead of from_ssz_bytes is good for backward compatibility. However, it's important to ensure that the new from_ssz_bytes_compat function correctly handles both old and new formats. The provided test in messages.rs seems sufficient for this purpose. codec.rs:41-42

2. Handlers Changes:

   • The change in handlers.rs to use request.roots.len() instead of request.len() is correct, as the new BlocksByRootRequest struct contains a roots field. handlers.rs:99-100
   • The change to create a RequestedBlockRoots instance and then wrap it in a BlocksByRootRequest is correct. However, it's worth considering if there's a more concise way to create the request directly. handlers.rs:166-167

3. Messages Changes:

   • The new BlocksByRootRequest struct and its from_ssz_bytes_compat function seem well-implemented. The backward compatibility logic is crucial for ensuring smooth transitions between protocol versions.
   • The test in tests is a good addition to verify the backward compatibility. It covers both old and new formats, which is essential. messages.rs:46-47 and messages.rs:47-61

4. Mod Changes:

   • The update to the pub use statements to include RequestedBlockRoots is correct. mod.rs:7-8

Overall, the changes look good and should improve the protocol's robustness and compatibility. The tests and documentation are also helpful. One minor suggestion is to consider adding more comments explaining the backward compatibility logic, especially in the from_ssz_bytes_compat function, to make the code easier to understand for future maintainers.

One potential area for improvement is error handling. Currently, the map_err in codec.rs simply formats the error as a string. It might be more informative to include the specific error kind or code, especially if there are different types of errors that can occur during deserialization.

Overall, this PR seems well-tested and documented, and the changes should improve the protocol's robustness and compatibility. The tests and documentation are also helpful.

---

Automated review by Kimi (Moonshot AI)

[github-actions]

Code review by ChatGPT

[github-actions]

Review Comments

Code Correctness and Potential Bugs

1. Function Call: The change from from_ssz_bytes to from_ssz_bytes_compat (lines 43-44) implies a modified function for deserialization. Ensure that this new function is validated against the SSZ standards. Verify that from_ssz_bytes_compat is fully compatible with the expected data structure, especially if this data is involved in consensus-critical operations.

Security Vulnerabilities

• Deserialization Handling: While there's handling of the SSZ payload with error mapping to io::Error, it's crucial to ensure that from_ssz_bytes_compat itself does not introduce any vulnerabilities or unchecked assumptions about the input. Consider running fuzz tests to ensure robustness against malformed inputs.

Rust Best Practices and Idiomatic Patterns

• No obvious violations of Rust best practices seen in the provided snippet. The use of map_err for error handling is commendable.

Memory Safety and Proper Error Handling

• Error Handling: The current error mapping is appropriate, but consider using a more descriptive error context where possible. Details can help when debugging.

Code Readability and Maintainability

• Code Comments: Adding a brief comment explaining the switch to from_ssz_bytes_compat would be beneficial for future maintainability.

Consensus-Layer Considerations

• SSZ Decoding: Ensure extensive testing of from_ssz_bytes_compat if it affects block processing. Differences in serialization interpretation can potentially lead to consensus failures.

Overall, while the change seems minimal, the implications on consensus and security require careful validation. Double-check the compatibility and testing of the from_ssz_bytes_compat function before merging.

[github-actions]

Code Correctness and Potential Bugs

• The implementation correctly attempts to decode using a new format first, then falls back to an older format if decoding fails. However, there is a lack of detailed logging for failure causes, which can help in debugging if the decoding process fails.

Security Vulnerabilities

• The code doesn’t present any immediate security concerns, but handling untrusted input during the decoding process is critical. Make sure that ssz::Decode handles cases like oversized input or malformed data securely without panicking or exposing vulnerabilities.

Performance Implications

• The use of or_else for fallback decoding doesn't incur significant overhead beyond the required decoding operations. However, please ensure that the ssz decoding performance is sufficient for production loads.

Rust Best Practices and Idiomatic Patterns

• Consider adding an expect_err or similar debug logging when the fallback decoding succeeds after a primary decoding failure. This will make your tests more robust and easier to debug.

Memory Safety and Proper Error Handling

• Error handling seems adequate with the structured Result return type. However, ensure that all errors are adequately documented, especially since this function interfaces with serialized data.

Code Readability and Maintainability

• The code is generally readable. Consider adding comments to the from_ssz_bytes_compat function to clarify the fallback behavior for maintenance purposes.

Consensus Layer Considerations

• Ensure that the SSZ encoding and decoding is aligned with Ethereum's consensus layer specification to prevent deserialization issues.
• There are no indications of consensus-layer functionality (like fork choice or state transitions) being impacted directly by this change.

Additional Recommendations

• Consider using feature flags or versioning comments to facilitate transitions between decoding structures as Ethereum evolves.
• Expand test coverage to include malformed and edge case inputs to ensure robustness against unexpected input.

Overall, the code introduces a necessary compatibility feature but could be improved with better logging and more comprehensive test cases for malformed input.