feat: store block signatures and respond to BlocksByRoot requests

[MegaRedHand]

Add BlockSignatures table to store proposer attestation and signatures separately from blocks, allowing reconstruction of SignedBlockWithAttestation for P2P responses.

Changes:

• Add BlockSignaturesWithAttestation type with from_signed_block/to_signed_block
• Add BlockSignatures table to storage
• Add insert_signed_block and get_signed_block methods to Store
• Implement handle_blocks_by_root_request to serve signed blocks to peers

[github-actions]

Kimi AI Code Review

### Review

General Observations

The PR introduces changes to store block signatures and respond to BlocksByRoot requests. This is a significant change as it affects the storage layer and the networking layer. Here are the key points I've reviewed:

1. Storage of Signed Blocks: The PR introduces a new struct BlockSignaturesWithAttestation to store block signatures and proposer attestation separately from the block itself. This is a good design choice as it allows for more granular control and potential optimizations.

2. Response to BlocksByRoot Requests: The PR modifies the handle_blocks_by_root_request function to respond to BlocksByRoot requests by sending the requested signed block.

3. Error Handling and Logging: The PR includes appropriate logging for various cases, which is crucial for debugging and monitoring the health of the network.

4. Memory Safety and Error Handling: The PR uses expect for error handling, which is generally acceptable in this context as it simplifies the code. However, in a production environment, more robust error handling might be necessary.

Specific Comments

1. Line 366:

   • The comment // Store signed block and state is misleading as it only stores the signatures and not the actual block. Consider renaming it to // Store block signatures and state for clarity.

2. Line 129-168:

   • The new struct BlockSignaturesWithAttestation is well-defined and includes methods to convert from SignedBlockWithAttestation and back. This is a good design choice as it encapsulates the logic for handling block signatures.

3. Line 94-132:

   • The handle_blocks_by_root_request function now responds to BlocksByRoot requests. However, it currently only handles the first root in the request. Consider implementing support for multiple roots per request as per the protocol specification.

4. Line 305-358:

   • The insert_signed_block and get_signed_block methods are well-implemented. They correctly store and retrieve the signed blocks by combining the block and signatures. Consider adding error handling for cases where the block or signatures are not found.

5. Line 13-14:

   • The new constant Table::BlockSignatures is correctly added to the cf_name function. This ensures that the new table is correctly identified in the RocksDB backend.

6. Line 18-26:

   • The ALL_TABLES array is updated to include the new Table::BlockSignatures. This is important to ensure that all tables are correctly initialized.

Conclusion

The PR is well-implemented and addresses the requirements for storing block signatures and responding to BlocksByRoot requests. The code is clear, follows Rust best practices, and includes appropriate error handling and logging. However, consider implementing support for multiple roots per request in the handle_blocks_by_root_request function.

Overall, the PR is a significant improvement to the project and moves it closer to a full implementation of the Ethereum consensus client. Good work!

---

Automated review by Kimi (Moonshot AI)

[github-actions]

Code review by ChatGPT

[github-actions]

Code Correctness and Potential Bugs

1. Block and State Insertion Logic: The change from store.insert_block(block_root, block.clone()); to store.insert_signed_block(block_root, &signed_block); needs to be validated to ensure that all downstream logic that expects an unsigned block can properly handle a signed one. Ensure that store.insert_signed_block is functionally equivalent or that the system is designed for this change.

Security Vulnerabilities

1. Data Integrity and Consensus Safety: Ensure that signing the blocks does not introduce any potential weaknesses in verification or introduce unintended consensus consequences. Signed blocks should have their signatures verified promptly to prevent unauthorized state transitions.

Rust Best Practices and Idiomatic Patterns

1. Borrowing Practices: Changing the ownership passing style from a cloned block to a borrowed &signed_block is generally a good practice to reduce unnecessary allocations, assuming lifetime issues are managed correctly.

Memory Safety and Proper Error Handling

1. Handle Potential Errors from insert_signed_block: Ensure that insert_signed_block returns a Result and errors are handled, rather than ignored, to prevent unnoticed failures in block storage.

Code Readability and Maintainability

1. Comments: The comment adjustment from Store block and state to Store signed block and state is minor but helpful for context. Ensure that comments are updated to reflect the logical changes accurately and across the entire codebase where impacted.

Performance Implications

1. Clone vs. Reference: The change to use a reference for signed_block should be more efficient than cloning it, assuming no mutation occurs that would require a clone later, which indicates good practice barring any unforeseen side effects.

Consensus-layer Considerations

• Ensure that modifying the insertion logic does not inadvertently affect the fork choice, attestation validation, or state transition accuracy.
• No explicit comments on the impact on justification and finalization logic based on presented code; verify surrounding code logic handles signed_block correctly.

Overall, ensure thorough testing around this code to guarantee that expectations around signed_block are met without introducing hidden bugs or security issues.

[github-actions]

Code Correctness and Potential Bugs:

1. Error Handling for Not Found Block: There should be explicit error feedback for not found block rather than allowing a silent timeout. It is better to send a specific error response to help peer identify the issue.

Line 32:

warn!(%peer, %root, "Block not found for BlocksByRoot request");

Suggestion: Send an explicit error response indicating the block is not found.

2. Multiple Roots Handling: The code currently only processes the first root. To be protocol compliant, it's suggested to implement handling for all roots, or at least acknowledge this limitation clearly in documentation until fully supported.

Security Considerations:

1. Denial-of-Service (DoS) Vulnerability: Handling requests one at a time could be exploited for DoS. Consider implementing rate limiting, request prioritization, or sequential processing to mitigate.

Performance Implications:

1. Batch Processing and Response Efficiency: Not fully utilizing the allowed batch processing may lead to inefficiencies, especially as requests might contain up to 1024 roots. If performance issues occur, opt for more parallel processing strategies to enhance throughput.

Rust Best Practices and Idiomatic Patterns:

1. Using Option Pattern Effectively: It's good that the Option pattern is used, however, a more idiomatic approach could involve pattern matching directly on Options to allow for both presence and absence cases in a single expression.

Memory Safety and Proper Error Handling:

• Memory Safety: The current approach looks memory-safe as Option and Result are used appropriately. However, ensure any added storage implementations follow Rust's safety guidelines.

Code Readability and Maintainability:

1. Comments and Documentation: It would be beneficial to have more descriptive comments, especially around known limitations like single-block handling and potential future improvements.

Consensus-layer Considerations:

• Fork Choice and Attestation Processing: These aspects aren't directly affected by the code in question but ensure the fork choice logic and attestation processing are accurately reflected elsewhere.
• SSZ Encoding/Decoding: Ensure that when responses are sent, they comply with SSZ validity standards.

Overall, this PR has groundwork in place but requires additional handling and clear informative responses for better protocol adherence and robustness.

[github-actions]

Code Correctness and Potential Bugs

• Ensure that the new BlockSignatures enum variant is correctly integrated into all relevant areas of the code, especially where tables are manipulated or accessed. Without the full context of how Table is used, it's difficult to give a detailed assessment, but typically adding a new variant may necessitate updating match statements or other logic where Table is pattern matched or otherwise used.

Security Vulnerabilities

• Adding a new table for block signatures warrants a review of access patterns and data manipulation operations involving this table. Ensure that any external input, especially from nodes or users, is properly validated before interacting with the BlockSignatures table to prevent vulnerabilities, such as unauthorized access or injection attacks.

Performance Implications

• Adding a new table can have performance implications, especially if it involves frequent reads/writes in the consensus process. Benchmarking may be necessary to ensure the change does not introduce bottlenecks.

Rust Best Practices and Idiomatic Patterns

• The code snippet provided adheres to fundamental Rust best practices. However, ensure that wherever the Table enum is used, pattern matching is exhaustive (using _ => if needed) to accommodate future changes more easily.

Memory Safety and Error Handling

• As this is a merely structural addition, no immediate concerns about memory safety are visible from this change alone. Nevertheless, ensure error handling appropriately covers the new BlockSignatures table to handle cases where data might not exist or retrieval fails.

Code Readability and Maintainability

• The change is simple and follows the current pattern, maintaining readability. Ensure that documentation or comments are updated elsewhere in the code base to reflect the introduction of the BlockSignatures table and what its intended use is.

Consensus-Layer Considerations

• Given that this change involves storage enumeration related to block signatures, evaluate the effect on fork choice rule implementation, specifically related to attestation processing and validation if this is relevant.
• Review justification and finalization logic to ensure that any block signature interactions are handled correctly.
• Ensure proper SSZ encoding/decoding integration with the BlockSignatures.

In conclusion, while this change appears structurally sound, additional review of related parts of the code base outside of this snippet is necessary to address potential security, performance, and consensus issues regarding the introduction of the BlockSignatures table.

[github-actions]

Code Correctness and Potential Bugs

• In the provided code snippet, there is a clear addition of Table::BlockSignatures to the cf_name function, which appears to be correct and necessary for processing a new table type. Ensure all uses of cf_name are updated to handle this new table.

Security Vulnerabilities

• No specific vulnerabilities are evident from this snippet. Ensure that any interaction with block_signatures is properly validated, especially considering potential implications for consensus logic.

Performance Implications

• Introducing a new table does not inherently affect performance from the code snippet provided, but keep in mind potential database access latency when interacting with new tables.

Rust Best Practices and Idiomatic Patterns

• Using a match on Table enums is idiomatic in Rust and ensures compile-time checking of all possible Table variants. Double-check that all operations involving the newly added table variant properly match the new entries.

Memory Safety and Proper Error Handling

• Ensure that interactions with tables handle errors gracefully. It is crucial, particularly in consensus-related operations, as failures can lead to forks or loss of consensus.

Code Readability and Maintainability

• This code is readable and straightforward. Make sure to maintain consistency across the codebase when adding new table types.

Consensus-layer Considerations

• Ensure that the logic involving BlockSignatures does not interfere with critical consensus functions like fork choice, attestation processing, or state transitions. Consider comprehensive tests covering these parts if they interact with new table entries.

Based on the information provided, the code change looks appropriately applied; however, comprehensive testing and verification are required in the context of its impact throughout the codebase.

[github-actions]

Code Correctness and Potential Bugs

• Panic on Errors: The use of expect throughout the code (lines 318, 322, 327, 333) can lead to panics if there are errors during execution. Instead, consider using proper error handling techniques such as Result and propagate errors using the ? operator.
• Error Handling: On line 350, expect is used with Block::from_ssz_bytes. If the block or its signatures are malformed, it can cause a panic, risking node failure. Use proper error handling to manage such cases gracefully.

Security Vulnerabilities

• Unchecked Assumptions: The use of expect assumes that every operation will succeed. This is a risky practice in blockchain applications where data integrity is crucial.

Performance Implications

• Batch Processing: On lines 314-324, using batch operations improves performance when writing multiple entries. Ensure that this practice is applied consistently across other similar operations in the codebase.

Rust Best Practices and Idiomatic Patterns

• Error Propagation: Replace expect and panic with error propagation where possible. This improves both stability and comprehensibility of the code.

Memory Safety and Proper Error Handling

• Error Propagation: As mentioned above, utilize Result types for better error management.

Code Readability and Maintainability

• Documentation: The code is adequately commented, but some additional detail regarding the assumptions can provide clarity, especially for future maintainers.

Consensus-layer Considerations

• SSZ Decoding: Ensure that proper validation is performed on SSZ-encoded data to prevent invalid data being processed, which can lead to consensus issues.
• Data Integrity: It might be beneficial to verify the integrity of the blocks and signatures before processing, although not strictly required in this snippet.

Conclusion

The code needs improvements primarily in terms of error handling and assumption management. Ensuring graceful handling of errors will significantly increase robustness and security.