fix: timestamp include genesis delay #18
Conversation
WalkthroughThe pull request introduces significant modifications to the Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- crates/api/src/builder/api.rs (1 hunks)
- crates/api/src/proposer/api.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
crates/api/src/builder/api.rs (1)
Line range hint
1649-1661: Timestamp calculation correctly includesgenesis_delay.The updated
expected_timestampcalculation now addschain_info.context.genesis_delay, ensuring that the genesis delay is accounted for in timestamp validation during block submissions. This change aligns with the protocol specifications and accurately addresses the issue outlined in the PR objectives.
zsluedem
force-pushed
the
fix-genesis-timestamp-delay
branch
from
eee8389
to
68e59a0
Compare
zsluedem
force-pushed
the
fix-genesis-timestamp-delay
branch
from
68e59a0
to
64df57a
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
crates/api/src/lib.rs (1)
22-27: LGTM: Function implementation is correct and aligns with PR objectives.The
get_genesis_timefunction correctly incorporates the genesis delay in timestamp calculations, addressing the issues mentioned in the PR objectives. The error handling is appropriate, providing a fallback calculation ifgenesis_time()fails.Consider adding a brief documentation comment explaining the function's purpose and its fallback behavior. For example:
/// Returns the genesis time, considering the genesis delay if necessary. /// Falls back to calculating genesis time from genesis_time_in_secs and genesis_delay /// if the context's genesis_time() method fails. pub fn get_genesis_time(chain_info: &ChainInfo) -> u64 { // ... (existing implementation) }
🛑 Comments failed to post (1)
crates/api/src/builder/api.rs (1)
1650-1651: 💡 Codebase verification
get_genesis_timedoes not conditionally handle genesis delay for different network typesThe
get_genesis_timefunction appliesgenesis_delayuniformly without distinguishing between testnet and mainnet. This may lead to incorrect timestamp calculations for different network environments.
- File: crates/api/src/lib.rs
🔗 Analysis chain
Ensure
get_genesis_timecorrectly handles genesis delay for both testnet and mainnetThe function
get_genesis_timeis introduced to account for the genesis delay in timestamp calculations. Please verify that this function accurately computes the genesis time for both testnets (wheregenesis_time = min_genesis_time + genesis_delay) and mainnet (where genesis delay may differ or be zero), to prevent potential discrepancies in timestamp validation.Run the following script to verify the implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `get_genesis_time` handles genesis delay correctly for both testnet and mainnet. # Search for the definition and implementation of `get_genesis_time`. rg --type rust -A 10 'fn get_genesis_time' # Expected result: The function should conditionally include `genesis_delay` based on the network type.Length of output: 415
Script:
#!/bin/bash # Description: Check if `get_genesis_time` conditionally applies `genesis_delay` based on network type. # Search for conditional statements involving network types within `get_genesis_time`. rg --type rust 'fn get_genesis_time' -A 20 | rg -i 'testnet|mainnet|network_type|if|match'Length of output: 160
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/api/src/proposer/api.rs (1)
Line range hint
1-1325: Consider refactoring repeated timestamp calculationsWhile the current changes correctly address the genesis delay issue, there are still instances of repeated timestamp calculations throughout the file. For example, similar calculations are found in the
calculate_slot_time_infofunction (around line 1325) and potentially in other methods.To improve code maintainability and reduce the risk of inconsistencies, consider refactoring these repeated calculations into a single helper function. This would ensure that all timestamp calculations consistently use the
get_genesis_timefunction and follow the same logic.Would you like assistance in identifying all occurrences of timestamp calculations and proposing a refactored solution?
crates/api/src/builder/api.rs (1)
Line range hint
1012-1180: Well-implementedsubmit_block_v2method with enhanced featuresThe new
submit_block_v2method is a comprehensive implementation that includes various checks, validations, and optimizations for block submission. It properly handles optimistic processing, builder collateral checks, and integrates well with the auctioneer and database services.However, given the method's complexity and length, consider refactoring it to improve maintainability:
- Extract some of the validation logic into separate private methods.
- Use early returns for error cases to reduce nesting.
- Consider creating a separate struct to hold the state of the submission process, which could be passed between the extracted methods.
Here's a high-level example of how you might start refactoring:
impl<A, DB, S, G> BuilderApi<A, DB, S, G> where // ... existing where clauses ... { pub async fn submit_block_v2(...) -> Result<StatusCode, BuilderApiError> { let mut submission = BlockSubmissionV2::new(request_id, ...); submission.decode_payload(req).await?; submission.validate_slot(self.curr_slot_info.read().await)?; submission.fetch_and_validate_payload_attributes(&self.payload_attributes).await?; submission.validate_builder_info(self.auctioneer.as_ref()).await?; // ... more validation steps ... self.process_valid_submission(submission).await } async fn process_valid_submission(&self, submission: BlockSubmissionV2) -> Result<StatusCode, BuilderApiError> { // ... handle the actual submission process ... } } struct BlockSubmissionV2 { request_id: Uuid, trace: SubmissionTrace, payload: Option<SignedBidSubmission>, // ... other fields as needed ... } impl BlockSubmissionV2 { fn new(request_id: Uuid, ...) -> Self { ... } async fn decode_payload(&mut self, req: Request<Body>) -> Result<(), BuilderApiError> { ... } async fn validate_slot(&self, curr_slot_info: &(u64, Option<BuilderGetValidatorsResponseEntry>)) -> Result<(), BuilderApiError> { ... } // ... other validation methods ... }This refactoring would make the main
submit_block_v2method more readable and easier to maintain, while also improving testability of individual components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- crates/api/src/builder/api.rs (2 hunks)
- crates/api/src/lib.rs (2 hunks)
- crates/api/src/proposer/api.rs (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/api/src/lib.rs
🧰 Additional context used
🔇 Additional comments (5)
crates/api/src/proposer/api.rs (2)
57-57: LGTM: New import for genesis time calculationThe addition of the
get_genesis_timeimport is in line with the PR objective to include the genesis delay in timestamp calculations. This change appears to be correct and necessary for the implementation of the fix.
853-854: LGTM: Improved genesis time calculation for constraint validationThis change correctly implements the fix for including the genesis delay in timestamp calculations, addressing the issue described in #17. By using the
get_genesis_timefunction, the code now properly accounts for the genesis delay specified in the network configuration.This modification is crucial for ensuring accurate timestamp validation during the sanity check process, resolving the 60-second discrepancy mentioned in the issue.
crates/api/src/builder/api.rs (3)
1650-1651: Excellent addition to address the genesis delay issue!This change correctly incorporates the genesis time when calculating the expected timestamp for block submissions. It addresses the issue mentioned in #17, ensuring that the sanity check considers the genesis delay specified in the network configuration.
Line range hint
1-1651: Summary of changes incrates/api/src/builder/api.rs
- The
sanity_check_block_submissionfunction has been updated to consider the genesis delay when calculating the expected timestamp, addressing the issue mentioned in Helix does't consider genesis delay when sanity check #17.- A new
submit_block_v2method has been added, providing enhanced functionality for block submissions with optimistic processing and additional checks.- The
get_genesis_timefunction has been introduced to support the new timestamp calculation.These changes appear to effectively address the PR objectives and improve the overall functionality of the builder API. The main areas for potential improvement are:
- Refactoring the
submit_block_v2method to enhance maintainability.- Verifying the correct implementation of the
get_genesis_timefunction.
52-52: Verify the implementation ofget_genesis_timeThe new
get_genesis_timefunction is crucial for correctly calculating the expected timestamp in thesanity_check_block_submissionfunction. However, its implementation is not visible in this file.Please ensure that the
get_genesis_timefunction is correctly implemented and returns the genesis time including the genesis delay. You can verify this by running the following command:This will help locate the implementation of the function and allow for a review of its correctness.
fix #17 #16
Summary by CodeRabbit
New Features
submit_block_v2) with enhanced optimistic processing capabilities.Bug Fixes
Improvements