feat(node-api): use timestamp ID instead of execution ID for proof namespace #2066
Conversation
WalkthroughThe changes in this pull request primarily involve renaming methods and fields related to execution numbers to their corresponding timestamp counterparts across various files. The Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🔇 Additional comments (3)
🪧 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: 21
🧹 Outside diff range comments (6)
mod/node-api/handlers/types/request.go (1)
Line range hint
1-33: Summary: Successful implementation of timestamp-based identificationThe changes in this file successfully implement the transition from execution ID to timestamp ID for proof namespace, as outlined in the PR objectives. This modification aligns with EIP 4788 and will facilitate offchain clients in querying beacon block proofs based on timestamps.
Key points:
- The
ExecutionIDRequesthas been removed and replaced withTimestampIDRequest.- The new
TimestampIDRequestfollows the same structure and validation pattern as other request types in the file.- This change is likely to affect API consumers, so thorough testing and clear communication of the API changes will be crucial.
To ensure a smooth transition:
- Update all related API documentation to reflect this change.
- Implement proper error handling for cases where clients might still try to use the old ExecutionID-based endpoints.
- Consider providing a migration guide for API consumers to update their implementations.
- Ensure that all parts of the system that previously used ExecutionID are updated to use TimestampID consistently.
mod/node-api/handlers/proof/types/request.go (1)
Line range hint
1-41: Summary of changes and their impactThe modifications in this file consistently replace
execution_idwithtimestamp_idacross all struct definitions and comments. This change aligns well with the PR objectives of using timestamp IDs instead of execution IDs for indexing proofs.These changes will affect how clients interact with the proof endpoints, now requiring them to provide timestamp IDs instead of execution IDs. This could potentially improve the ability of offchain clients to query beacon block proofs based on timestamps, as mentioned in the PR objectives.
To ensure a smooth transition:
- Update any client code that interacts with these endpoints to use timestamp IDs.
- Update any documentation or API references to reflect these changes.
- Consider adding a deprecation notice or backwards compatibility for any existing code that might still use execution IDs.
mod/node-api/handlers/proof/execution_number.go (1)
Line range hint
28-40: Update method name and return type for consistencyThe method name
GetExecutionNumberand the return typeExecutionNumberResponseare now inconsistent with the internal logic, which uses a timestamp ID instead of an execution ID. Consider updating these to reflect the new functionality.Suggested changes:
- Rename the method to
GetExecutionNumberByTimestampor similar.- Update the return type and its fields to use "Timestamp" terminology where appropriate.
Also, update the function comment to reflect these changes and the use of timestamp ID.
mod/node-api/handlers/proof/execution_fee_recipient.go (1)
Line range hint
27-30: Consider updating the function documentation.While the function logic remains consistent after the change to use timestamp IDs, it would be beneficial to update the function's documentation to reflect this change. This will help maintain clarity for future developers working with this code.
Consider updating the comment to something like:
// GetExecutionFeeRecipient returns the fee recipient from the latest execution // payload header for the given timestamp ID, along with the proof that can be // verified against the beacon block root. The timestamp ID is used to identify // the relevant block in accordance with EIP 4788.This change would more accurately reflect the function's current behavior and its alignment with the PR objectives.
mod/node-api/handlers/proof/handler.go (1)
Action Required: Update Remaining References to
execution_idSeveral instances of
execution_idremain in comments and route paths:
- Files:
mod/node-api/handlers/proof/types/response.go
- Endpoints:
/proof/block_proposer/{execution_id},/proof/execution_number/{execution_id},/proof/execution_fee_recipient/{execution_id}mod/node-api/handlers/proof/routes.go
- Paths:
"bkit/v1/proof/block_proposer/:execution_id","bkit/v1/proof/execution_number/:execution_id","bkit/v1/proof/execution_fee_recipient/:execution_id"mod/node-api/engines/echo/validator.go
- Validation:
"execution_id": ValidateTimestampIDAdditionally, there's an outstanding TODO:
examples/berad/pkg/state-transition/state_processor_payload.go:// TODO: Verify timestamp data once Clock is done.Ensure all
execution_idreferences are updated totimestamp_idand address the pending TODO to complete the timestamp data verification.🔗 Analysis chain
Line range hint
74-102: LGTM! The changes align well with the PR objectives.The renaming from
resolveExecutionIDtoresolveTimestampIDand the corresponding updates in the function signature, comment, and internal logic are consistent with the goal of using timestamp ID instead of execution ID. This change aligns with the EIP 4788 requirement mentioned in the PR objectives.Consider updating the comment to be more specific:
// resolveTimestampID gets the slot, beacon state, and beacon block header for the given timestamp ID.Let's verify the impact of these changes on the rest of the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to ExecutionID or execution_id # that might need to be updated. # Test 1: Search for ExecutionID echo "Searching for ExecutionID:" rg --type go 'ExecutionID' # Test 2: Search for execution_id echo "Searching for execution_id:" rg --type go 'execution_id' # Test 3: Search for resolveExecutionID echo "Searching for resolveExecutionID:" rg --type go 'resolveExecutionID' # Test 4: Check for any TODO comments related to this change echo "Checking for related TODO comments:" rg --type go 'TODO.*timestamp'Length of output: 1204
mod/consensus-types/pkg/types/block_test.go (1)
Line range hint
1-214: Consider adding more test coverage for the Timestamp field.While the changes made are correct and focused, it might be beneficial to add more test coverage for the new Timestamp field. This could include:
- Testing edge cases for the Timestamp value.
- Verifying the behavior when Timestamp and Number fields have different values.
- Adding assertions in other relevant test functions to ensure the Timestamp is correctly handled throughout the BeaconBlock functionality.
Would you like assistance in generating additional test cases for the Timestamp field?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (18)
- mod/consensus-types/pkg/types/block.go (1 hunks)
- mod/consensus-types/pkg/types/block_test.go (2 hunks)
- mod/node-api/backend/backend.go (1 hunks)
- mod/node-api/backend/types.go (1 hunks)
- mod/node-api/engines/echo/vaildator.go (3 hunks)
- mod/node-api/handlers/proof/backend.go (1 hunks)
- mod/node-api/handlers/proof/block_proposer.go (1 hunks)
- mod/node-api/handlers/proof/execution_fee_recipient.go (1 hunks)
- mod/node-api/handlers/proof/execution_number.go (1 hunks)
- mod/node-api/handlers/proof/handler.go (1 hunks)
- mod/node-api/handlers/proof/types/request.go (1 hunks)
- mod/node-api/handlers/types/request.go (1 hunks)
- mod/node-api/handlers/utils/constants.go (1 hunks)
- mod/node-api/handlers/utils/id.go (1 hunks)
- mod/node-core/pkg/components/interfaces.go (4 hunks)
- mod/storage/pkg/block/store.go (4 hunks)
- mod/storage/pkg/block/store_test.go (3 hunks)
- mod/storage/pkg/block/types.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (16)
mod/node-api/handlers/utils/constants.go (1)
30-30: LGTM! Constant renamed as per PR objectives.The change from
ExecutionIDPrefixtoTimestampIDPrefixaligns with the PR's goal of using timestamp ID instead of execution ID for proof namespace. The new prefix "t" is concise and clear.To ensure consistency across the codebase, let's verify that there are no remaining references to the old constant name:
✅ Verification successful
Verified: No remaining references to
ExecutionIDPrefixfound.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to ExecutionIDPrefix # Test: Search for ExecutionIDPrefix. Expect: No results. rg --type go "ExecutionIDPrefix" # Test: Search for TimestampIDPrefix to confirm its usage. Expect: At least one result (this file). rg --type go "TimestampIDPrefix"Length of output: 571
mod/storage/pkg/block/types.go (1)
33-33: Verify the impact of replacing GetExecutionNumber with GetTimestamp.The change from
GetExecutionNumber()toGetTimestamp()is significant and may have far-reaching effects in the codebase. To ensure a smooth transition:
- Update all implementations of the
BeaconBlockinterface.- Modify any code that calls
GetExecutionNumber()to useGetTimestamp().- Review the logic in dependent code to account for the semantic difference between execution number and timestamp.
Run the following script to identify potential areas that need updating:
Please review the output of this script and update the identified areas accordingly.
✅ Verification successful
Verified that replacing
GetExecutionNumberwithGetTimestamphas been consistently applied across the codebase and no usages ofGetExecutionNumberremain. Ensure that dependent logic accounts for the semantic change from execution number to timestamp.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Identify usages of GetExecutionNumber and BeaconBlock interface implementations # Search for GetExecutionNumber usages echo "Searching for GetExecutionNumber usages:" rg --type go "GetExecutionNumber\(\)" # Search for BeaconBlock interface implementations echo "Searching for BeaconBlock interface implementations:" rg --type go "type \w+ struct" -A 10 | rg "BeaconBlock" # Search for timestamp-related functions that might need updating echo "Searching for timestamp-related functions that might need updating:" rg --type go "func.*Timestamp"Length of output: 4195
mod/node-api/handlers/proof/types/request.go (1)
38-40: LGTM!The change from
execution_idtotimestamp_idis consistent with the PR objectives and correctly implemented.mod/node-api/handlers/proof/block_proposer.go (1)
40-41: LGTM! Verify consistency across the codebase.The change from
resolveExecutionIDtoresolveTimestampIDand the corresponding parameter update align with the PR objectives. This modification correctly implements the shift from execution ID to timestamp ID for proof namespace.To ensure consistency across the codebase, please run the following script:
This script will help identify any inconsistencies or missed updates related to this change.
✅ Verification successful
Consistency Verified Across Codebase.
All instances of
ExecutionIDandresolveExecutionIDhave been successfully updated toTimestampIDandresolveTimestampIDrespectively. No remaining references to the old identifiers were found, ensuring consistency throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all occurrences of ExecutionID have been updated to TimestampID # Test 1: Search for any remaining instances of ExecutionID echo "Searching for remaining instances of ExecutionID:" rg --type go 'ExecutionID' # Test 2: Verify that TimestampID is now used consistently echo "Verifying usage of TimestampID:" rg --type go 'TimestampID' # Test 3: Check for any remaining instances of resolveExecutionID echo "Checking for any remaining instances of resolveExecutionID:" rg --type go 'resolveExecutionID'Length of output: 2402
mod/storage/pkg/block/store_test.go (2)
74-76: LGTM! Change is consistent with the new timestamp-based approach.The update from
GetSlotByExecutionNumbertoGetSlotByTimestampis consistent with the PR objective and the changes made to theMockBeaconBlockstruct.
Line range hint
1-87: Overall, the changes successfully implement the timestamp-based approach.The modifications in this file consistently replace execution number references with timestamp references, aligning well with the PR objectives. The test structure and coverage remain intact, ensuring that the new timestamp-based functionality is properly tested.
Key points:
GetExecutionNumbermethod renamed toGetTimestamp.GetSlotByExecutionNumbercalls updated toGetSlotByTimestamp.- Error checking adjusted to reflect the new timestamp-based approach.
These changes contribute to the goal of indexing proofs by timestamp in accordance with EIP 4788, facilitating offchain clients in querying beacon block proofs based on timestamps.
mod/storage/pkg/block/store.go (3)
51-53: LGTM: Proper initialization of timestamps cacheThe initialization of the
timestampscache in theNewStorefunction is consistent with the struct changes. The error handling and use of theavailabilityWindowparameter for cache size are appropriate.Also applies to: 60-63
Line range hint
1-108: Summary: Successful implementation of timestamp-based indexingThe changes in this file successfully implement the transition from execution number-based to timestamp-based indexing for beacon block proofs. This aligns well with the PR objectives and EIP 4788 requirements. The modifications are consistent across the
KVStorestruct, its initialization, and associated methods.Key points:
executionNumbersfield replaced withtimestampsinKVStore.NewStorefunction updated to initialize thetimestampscache.Setmethod now stores timestamps instead of execution numbers.- New
GetSlotByTimestampmethod added to replaceGetSlotByExecutionNumber.The implementation is clean and maintains the existing error handling patterns. Minor suggestions for improvement have been made in the individual comments.
Line range hint
1-108: Verify complete removal of GetSlotByExecutionNumberThe
GetSlotByExecutionNumbermethod has been correctly removed as part of the transition from execution numbers to timestamps. To ensure a clean transition, we should verify that all references to this method have been updated or removed throughout the codebase.Let's run a check to ensure no references to
GetSlotByExecutionNumberremain:#!/bin/bash # Check for any remaining references to GetSlotByExecutionNumber rg --type go "GetSlotByExecutionNumber" ./If this search returns any results, those locations will need to be updated to use
GetSlotByTimestampinstead.mod/node-api/engines/echo/vaildator.go (3)
Line range hint
1-213: Summary: Changes align well with PR objectivesThe modifications in this file successfully implement the transition from execution IDs to timestamp IDs for the proof namespace. The changes are focused, maintain backwards compatibility, and align well with the PR objectives. The implementation is solid, with only a minor suggestion for improvement in error handling.
Key points:
- The
ConstructValidatorfunction has been updated to useValidateTimestampID.- The
ValidateTimestampIDfunction has been implemented to handle both timestamp IDs and existing state IDs.- The changes maintain backwards compatibility while introducing the new functionality.
Overall, the implementation looks good and achieves the intended goals of the PR.
65-65: LGTM: Validator function updated to use timestamp IDThe change from
ValidateExecutionIDtoValidateTimestampIDis consistent with the PR objective of using timestamp IDs instead of execution IDs for proof namespace.To ensure consistency across the codebase, let's verify that all references to
ValidateExecutionIDhave been updated:
Line range hint
99-113: LGTM: ValidateTimestampID function implemented correctlyThe
ValidateTimestampIDfunction has been successfully updated to handle timestamp IDs while maintaining backwards compatibility with state IDs. The implementation aligns well with the PR objectives.A minor suggestion for improvement:
Consider adding explicit error handling for the case where
IsTimestampIDPrefixreturns true, butValidateUint64Decfails. This could help in providing more specific error messages.func ValidateTimestampID(fl validator.FieldLevel) bool { // ... (existing code) if utils.IsTimestampIDPrefix(value) { - return ValidateUint64Dec(value[1:]) + if !ValidateUint64Dec(value[1:]) { + // Log an error or set a more specific error message + return false + } + return true } // ... (existing code) }To ensure the
utils.IsTimestampIDPrefixfunction is properly implemented, let's check its definition:✅ Verification successful
LGTM: ValidateTimestampID function implemented correctly
The
ValidateTimestampIDfunction is correctly implemented, effectively handling timestamp IDs while maintaining backwards compatibility with state IDs. The implementation aligns with the PR objectives.A minor suggestion for improvement:
Consider adding explicit error handling for cases where
IsTimestampIDPrefixreturns true butValidateUint64Decfails. This can provide more specific error messages or logging.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find the implementation of IsTimestampIDPrefix # Expect: The function definition in a utils file rg --type go "func IsTimestampIDPrefix"Length of output: 127
Script:
#!/bin/bash # Description: Extract the implementation of IsTimestampIDPrefix function from id.go # Expect: The full function definition including its logic ast-grep --lang go --pattern 'func IsTimestampIDPrefix($_) bool { $$$ }' mod/node-api/handlers/utils/id.goLength of output: 334
mod/consensus-types/pkg/types/block_test.go (1)
105-105: LGTM! Assertion updated correctly.The change from
GetExecutionNumber()toGetTimestamp()is consistent with the PR objective and the addition of the Timestamp field. The expected value of 10 matches the Timestamp set in thegenerateValidBeaconBlockfunction.mod/node-core/pkg/components/interfaces.go (2)
116-118: LGTM: Method renamed from GetExecutionNumber to GetTimestampThis change aligns with the PR objective of using timestamp ID instead of execution ID for proof namespace. The return type (math.U64) remains appropriate for the timestamp.
Line range hint
1-1173: Overall assessment: Changes align with PR objectivesThe modifications in this file consistently rename methods related to execution numbers to use timestamps instead. This aligns well with the PR objective of indexing proofs by timestamp in accordance with EIP 4788. The changes are applied consistently across multiple interfaces (BeaconBlock, BlockStore, NodeAPIBackend, and NodeAPIProofBackend).
A minor suggestion has been made to update the parameter names in the renamed methods for better clarity and consistency.
These changes should facilitate offchain clients in querying beacon block proofs based on timestamps, as intended.
mod/node-api/handlers/utils/id.go (1)
98-101: EnsureTimestampIDPrefixis defined and consistent.The function
IsTimestampIDPrefixrelies onTimestampIDPrefixto check the prefix oftimestampID. Please verify thatTimestampIDPrefixis defined appropriately (e.g., asconst TimestampIDPrefix = "t") and that this prefix is used consistently throughout the codebase.Run the following script to check the definition and usage of
TimestampIDPrefix:✅ Verification successful
TimestampIDPrefixis correctly defined and consistently used across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'TimestampIDPrefix' is defined and used consistently. # Search for the definition of 'TimestampIDPrefix'. Expect: A single definition. rg --type go --word-regex --pcre2 'const\s+TimestampIDPrefix\s*=\s*.*' # Search for all usages of 'TimestampIDPrefix'. Expect: Consistent usage across files. rg --type go 'TimestampIDPrefix'Length of output: 1055
Script:
#!/bin/bash # Description: Verify that 'TimestampIDPrefix' is defined and used consistently. # Search for the definition of 'TimestampIDPrefix'. Expect: A single definition. rg --type go --word-regexp --pcre2 'const\s+TimestampIDPrefix\s*=\s*.*' # Search for all usages of 'TimestampIDPrefix'. Expect: Consistent usage across files. rg --type go 'TimestampIDPrefix'Length of output: 612
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (8)
- mod/node-api/backend/backend.go (1 hunks)
- mod/node-api/backend/types.go (1 hunks)
- mod/node-api/handlers/proof/backend.go (1 hunks)
- mod/node-api/handlers/proof/handler.go (1 hunks)
- mod/node-api/handlers/utils/id.go (1 hunks)
- mod/node-core/pkg/components/interfaces.go (4 hunks)
- mod/storage/pkg/block/store.go (4 hunks)
- mod/storage/pkg/block/store_test.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (14)
mod/storage/pkg/block/store_test.go (4)
45-47: LGTM! Consider adding a clarifying comment.The renaming from
GetExecutionNumbertoGetTimestampaligns well with the PR objective of using timestamp IDs instead of execution IDs. However, the implementation assumes that the slot number is equivalent to the timestamp, which may not be immediately clear to readers.As suggested in a previous review, consider adding a comment to clarify this assumption:
func (m MockBeaconBlock) GetTimestamp() math.U64 { + // For testing purposes, we assume the slot number is equivalent to the timestamp return m.slot }
74-76: LGTM! The change is consistent with the PR objective.The update from
GetSlotByExecutionNumbertoGetParentSlotByTimestampaligns well with the PR's goal of using timestamp IDs instead of execution IDs. The test logic remains intact, which maintains the existing test coverage.
86-87: LGTM! Consider updating the error message for clarity.The update to check
GetParentSlotByTimestampinstead ofGetSlotByExecutionNumberis consistent with the previous changes and aligns with the PR objective.As suggested in a previous review, consider updating the error message to explicitly mention "timestamp" for improved clarity and consistency:
_, err = blockStore.GetParentSlotByTimestamp(2) -require.ErrorContains(t, err, "not found") +require.ErrorContains(t, err, "timestamp not found")
Line range hint
1-89: Overall, the changes successfully implement the PR objective.The modifications in this file consistently reflect the shift from execution numbers to timestamps, aligning well with the PR's goal. The test coverage appears to be maintained, which is crucial for ensuring the reliability of the changes.
A few minor suggestions for improved clarity have been made:
- Adding a comment to clarify the assumption in the
GetTimestampmethod.- Updating an error message to be more specific about timestamps.
These small improvements will enhance the readability and maintainability of the test code.
mod/node-api/handlers/proof/handler.go (1)
86-86: Verify the implications of using ParentSlotFromTimestampIDThe change from
utils.SlotFromExecutionIDtoutils.ParentSlotFromTimestampIDmight have implications on the slot calculation logic. Please ensure that:
- The
ParentSlotFromTimestampIDfunction correctly calculates the slot based on the timestamp.- Using the parent slot instead of the direct slot is intentional and doesn't affect the proof generation process.
- The rest of the codebase is updated to handle this change in slot calculation.
To verify the usage and implementation of
ParentSlotFromTimestampID, please run the following script:This script will help us understand the implementation of
ParentSlotFromTimestampIDand verify that it's being used consistently across the codebase.✅ Verification successful
Verification Successful: ParentSlotFromTimestampID Usage Confirmed
The
ParentSlotFromTimestampIDfunction is correctly implemented and consistently used inhandler.go. There are no remaining references toSlotFromExecutionID, ensuring that the slot calculation logic has been updated as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and usage of ParentSlotFromTimestampID # Check the implementation of ParentSlotFromTimestampID echo "Checking ParentSlotFromTimestampID implementation:" ast-grep --lang go --pattern $'func ParentSlotFromTimestampID($_, $_) ($_) { $$$ }' # Check for other usages of ParentSlotFromTimestampID in the codebase echo "\nChecking other usages of ParentSlotFromTimestampID:" rg --type go "ParentSlotFromTimestampID" # Check for any remaining usages of SlotFromExecutionID echo "\nChecking for any remaining usages of SlotFromExecutionID:" rg --type go "SlotFromExecutionID"Length of output: 830
mod/node-api/backend/backend.go (2)
Line range hint
1-215: Reminder: Update mock referencesAs mentioned in a previous review, please ensure that any mock implementations in
mod/node-api/backend/mocks/block_store.mock.goare updated to useGetParentSlotByTimestampinstead ofGetSlotByExecutionNumber. This will maintain consistency across the codebase and ensure accurate test behavior.
153-158: LGTM! Verify usage across the codebase.The introduction of
GetParentSlotByTimestampaligns well with the PR objectives of using timestamp ID instead of execution ID. The implementation correctly delegates to the block store, maintaining a good separation of concerns.To ensure consistency across the codebase, please run the following script to check for any remaining references to
GetSlotByExecutionNumberthat might need updating:If any results are found, they may need to be updated to use
GetParentSlotByTimestampandtimestamprespectively.Additionally, please verify that this change doesn't introduce any unexpected behavior in the system, especially in areas where the execution number was previously used for other purposes beyond retrieving the parent slot.
mod/node-api/handlers/proof/backend.go (1)
31-31: Ensure all references to the renamed method are updatedThe method
GetSlotByExecutionNumberhas been renamed toGetParentSlotByTimestamp. Please verify that all implementations of theBackendinterface and any code calling this method have been updated to reflect this change to prevent potential inconsistencies or runtime errors.Run the following script to identify any remaining references to the old method name:
mod/node-api/handlers/utils/id.go (2)
73-96: Well-documented implementation ofParentSlotFromTimestampID.The new function
ParentSlotFromTimestampIDeffectively replacesSlotFromExecutionID, aligning with the shift from execution numbers to timestamps as per the PR objectives. The documentation is clear, providing detailed explanations and examples for the usage oftimestampID. The function logic is correct and maintains consistency with the existing codebase.
99-102: Correct implementation ofIsTimestampIDPrefix.The
IsTimestampIDPrefixfunction appropriately checks for the 't' prefix intimestampID, which is essential for the new timestamp-based querying mechanism. The implementation is straightforward and efficient.mod/node-core/pkg/components/interfaces.go (4)
116-118: MethodGetTimestampcorrectly updatedThe
GetTimestamp()method has been correctly introduced to replaceGetExecutionNumber(), aligning with the shift from execution numbers to timestamps as per the PR objectives.
298-300: Parameter name inconsistency remainsThe parameter name
executionNumbershould be updated totimestampfor consistency with the methodGetParentSlotByTimestamp.This issue has been previously noted.
1094-1094: Parameter name inconsistency remainsThe parameter name
executionNumbershould be updated totimestampto maintain consistency with the methodGetParentSlotByTimestamp.This issue has been previously noted.
1126-1126: Parameter name inconsistency remainsThe parameter name
executionNumbershould be updated totimestampfor consistency with the updated method nameGetParentSlotByTimestamp.This issue has been previously noted.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (6)
- mod/node-api/handlers/proof/block_proposer.go (2 hunks)
- mod/node-api/handlers/proof/execution_fee_recipient.go (2 hunks)
- mod/node-api/handlers/proof/execution_number.go (2 hunks)
- mod/node-api/handlers/utils/id.go (1 hunks)
- mod/storage/pkg/block/store.go (4 hunks)
- mod/storage/pkg/block/store_test.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (10)
mod/node-api/handlers/proof/block_proposer.go (2)
29-31: LGTM: Documentation updated to reflect the use of timestamp IDThe method documentation has been correctly updated to mention "timestamp id" instead of "execution id". This change aligns with the PR objective of using timestamp ID for indexing proofs.
41-42: LGTM: Implementation updated to use timestamp IDThe method implementation has been correctly updated to use
resolveTimestampIDinstead ofresolveExecutionID, and the parameter has been changed toparams.TimestampID. These changes are consistent with the PR objective and the updated documentation.To ensure the changes are applied consistently throughout the codebase, please run the following verification script:
mod/node-api/handlers/proof/execution_number.go (3)
30-30: LGTM: Comment updated to reflect timestamp usageThe comment has been correctly updated to mention "timestamp id" instead of "execution id", which aligns with the PR objectives of using timestamp ID for proof namespace.
41-42: LGTM: Method and parameter updated to use timestampThe changes from
resolveExecutionIDtoresolveTimestampIDandparams.ExecutionIDtoparams.TimestampIDalign well with the PR objectives of using timestamp ID for proof namespace.
Line range hint
34-34: Consider updating the function name for consistencyWhile the internal logic has been updated to use timestamp ID, the function name
GetExecutionNumberstill refers to "Execution". Consider renaming this function to better reflect its new purpose, such asGetTimestampNumberorGetBlockNumberByTimestamp.Here's a suggested change:
-func (h *Handler[ - BeaconBlockHeaderT, _, _, ContextT, _, _, -]) GetExecutionNumber(c ContextT) (any, error) { +func (h *Handler[ + BeaconBlockHeaderT, _, _, ContextT, _, _, +]) GetBlockNumberByTimestamp(c ContextT) (any, error) {Don't forget to update any references to this function in other parts of the codebase.
✅ Verification successful
Function name updated for consistency
The function
GetExecutionNumberhas been successfully renamed toGetBlockNumberByTimestamp. All references to this function in the codebase have been updated to reflect the new name.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for references to GetExecutionNumber in the codebase echo "Checking for references to GetExecutionNumber:" rg --type go 'GetExecutionNumber'Length of output: 426
mod/storage/pkg/block/store_test.go (4)
45-47: LGTM! Consider adding a comment for clarity.The implementation of
GetTimestampaligns with the PR objective of using timestamps instead of execution numbers. However, it assumes that the slot number is equivalent to the timestamp, which might not be immediately clear to all readers.As suggested in a previous review, consider adding a clarifying comment:
func (m MockBeaconBlock) GetTimestamp() math.U64 { + // For testing purposes, we assume the slot number is equivalent to the timestamp return m.slot }
68-76: LGTM! Changes align with PR objectives.The modifications in this test loop accurately reflect the shift from execution numbers to timestamps:
- The use of
GetParentSlotByTimestampinstead ofGetSlotByExecutionNumberis consistent with the PR's goal.- The adjustment of the expected value from
itoi-1is correct, as we're now retrieving the parent slot.These changes ensure that the test cases properly validate the updated functionality of the block store.
86-87: Consider updating the error message for clarity.The update to check
GetParentSlotByTimestampinstead ofGetSlotByExecutionNumberis consistent with the previous changes. However, the error message could be more specific.As suggested in a previous review, consider updating the error message to explicitly mention "timestamp" for improved clarity and consistency:
_, err = blockStore.GetParentSlotByTimestamp(2) -require.ErrorContains(t, err, "not found") +require.ErrorContains(t, err, "timestamp not found")
Line range hint
1-87: Overall implementation aligns well with PR objectives.The changes in this file successfully transition from using execution numbers to timestamps, which is consistent with the PR's goal of facilitating offchain clients in querying beacon block proofs based on timestamps. The test cases have been appropriately updated to reflect this change.
A few minor suggestions for improved clarity have been made:
- Adding a comment to clarify the assumption in the
GetTimestampmethod.- Updating the error message in the test case for better specificity.
These small improvements will enhance the readability and maintainability of the test code.
mod/node-api/handlers/utils/id.go (1)
73-97: Implementation ofParentSlotFromTimestampIDaligns with the timestamp ID usage.The function correctly handles both prefixed and non-prefixed
timestampIDvalues, parsing the timestamp and retrieving the parent slot as intended. The documentation is comprehensive and clarifies the semantics.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (1)
mod/node-api/engines/echo/vaildator.go (1)
Line range hint
1-1: Typo in filename 'vaildator.go'The filename
vaildator.goappears to be misspelled. It should likely bevalidator.go.Apply the following change:
-Rename file: vaildator.go +Rename file: validator.go
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
- mod/node-api/engines/echo/vaildator.go (3 hunks)
- mod/node-api/handlers/utils/id.go (2 hunks)
- mod/node-core/pkg/components/interfaces.go (4 hunks)
- mod/storage/pkg/block/store.go (5 hunks)
🧰 Additional context used
🔇 Additional comments (9)
mod/node-api/handlers/utils/id.go (3)
27-27: Import statement added for enhanced error handlingIncluding
github.com/berachain/beacon-kit/mod/errorsallows for consistent and detailed error wrapping throughout the package, improving error tracing and debugging.
86-98: Ensure handling of emptytimestampIDafter prefixThe function
ParentSlotFromTimestampIDdoes not handle the case wheretimestampIDis only the prefix't'without any timestamp value. This can lead to an error when slicing or parsing an empty string.This issue was previously raised and is still applicable. Consider adding a check to handle this edge case explicitly.
86-98: Consider refactoring to reduce code duplicationThe functions
SlotFromStateID,SlotFromBlockID, andParentSlotFromTimestampIDshare similar patterns in parsing IDs and retrieving slots. Extracting common logic into shared helper functions can improve code maintainability and reduce redundancy.As noted in previous reviews, this suggestion remains valid and can enhance the codebase's readability and maintainability.
mod/node-api/engines/echo/vaildator.go (2)
65-65: Registering 'timestamp_id' validatorThe addition of
"timestamp_id": ValidateTimestampIDcorrectly registers the new validation function for timestamp IDs.
Line range hint
99-112: Ensure safe slicing of 'value' inValidateTimestampIDWhen slicing
value[1:]after checkingutils.IsTimestampIDPrefix(value), ensure thatvaluehas sufficient length to prevent potential index out of range errors.Run the following script to confirm that
utils.IsTimestampIDPrefix(value)guaranteesvalueis at least two characters long:mod/node-core/pkg/components/interfaces.go (4)
116-118: Addition ofGetTimestamp()method is appropriateThe inclusion of
GetTimestamp()in theBeaconBlockinterface aligns with the PR objective of shifting from execution numbers to timestamps. This provides necessary access to the block's timestamp derived from the execution payload.
298-300: Introduction ofGetParentSlotByTimestamp()inBlockStoreAdding
GetParentSlotByTimestamp(timestamp math.U64)to theBlockStoreinterface enables retrieval of the parent slot based on a given timestamp. This supports timestamp-based queries as per the PR objectives.
1094-1094: Implementation ofGetParentSlotByTimestamp()inNodeAPIBackendThe addition of
GetParentSlotByTimestamp(timestamp math.U64)in theNodeAPIBackendinterface is appropriate and consistent with the transition to timestamp-based methods, facilitating improved access patterns.
1126-1126: Addition ofGetParentSlotByTimestamp()inNodeAPIProofBackendIncluding
GetParentSlotByTimestamp(timestamp math.U64)in theNodeAPIProofBackendinterface aligns with the goal of enabling timestamp-based access within the proof backend, supporting the changes outlined in the PR.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- mod/consensus-types/pkg/types/block_test.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
mod/consensus-types/pkg/types/block_test.go (1)
104-104: LGTM! Test case updated correctly.The change from
GetExecutionNumber()toGetTimestamp()in the assertion is consistent with the modifications made to theExecutionPayloadstruct. The test case now correctly validates theTimestampfield while maintaining the same expected value.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range comments (2)
mod/node-api/engines/echo/vaildator.go (2)
Line range hint
99-111: Handle potential index out of range error inValidateTimestampIDThere's a potential edge case in the
ValidateTimestampIDfunction. Ifvalueis an empty string andutils.IsTimestampIDPrefix(value)returnstrue, slicingvalue[1:]could result in an index out of range error.Apply this diff to handle the edge case:
func ValidateTimestampID(fl validator.FieldLevel) bool { allowedValues := map[string]bool{ utils.StateIDHead: true, utils.StateIDGenesis: true, utils.StateIDFinalized: true, utils.StateIDJustified: true, } value := fl.Field().String() + if len(value) < 1 { + return false + } if utils.IsTimestampIDPrefix(value) { return ValidateUint64Dec(value[1:]) } return validateStateBlockIDs(value, allowedValues) }
Line range hint
1-1: Correct the filename from 'vaildator.go' to 'validator.go'The filename
vaildator.goappears to be misspelled. It should bevalidator.goto reflect the correct spelling of "validator" and maintain consistency.Apply this change to rename the file:
-// File: vaildator.go +// File: validator.go
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
- mod/consensus-types/pkg/types/block_test.go (2 hunks)
- mod/node-api/engines/echo/vaildator.go (3 hunks)
- mod/node-api/handlers/utils/id.go (2 hunks)
- mod/node-core/pkg/components/interfaces.go (4 hunks)
- mod/storage/pkg/block/store.go (5 hunks)
🧰 Additional context used
🔇 Additional comments (7)
mod/consensus-types/pkg/types/block_test.go (1)
104-104: LGTM: Test assertion updated correctly.The modification of the test assertion from
GetExecutionNumber()toGetTimestamp()is consistent with the PR objective of using timestamp ID instead of execution ID. The test now correctly validates the new Timestamp field, and the expected value of 10 matches the Timestamp value set in thegenerateValidBeaconBlockfunction.mod/storage/pkg/block/store.go (2)
39-42: Well-DocumentedtimestampsFieldGreat job adding detailed comments for the
timestampscache. The explanation clarifies that the timestamp-to-slot mapping is injective for finalized blocks due to CometBFT consensus, ensuring that each slot corresponds to a unique timestamp. This enhances the code's readability and maintainability.
107-109: Appropriate Handling of Genesis Slot Edge CaseThe explicit check for
slot == 0correctly handles the edge case where the genesis slot has no parent. Returning a specific error message improves the robustness of the method.mod/node-api/handlers/utils/id.go (2)
86-101: FunctionParentSlotFromTimestampIDimplementation looks good.The
ParentSlotFromTimestampIDfunction correctly parses the timestamp ID and retrieves the corresponding parent slot. The error handling and use of generics are appropriate.
86-101:⚠️ Potential issueUndefined constant
TimestampIDPrefixinIsTimestampIDPrefix.The function
IsTimestampIDPrefixusesTimestampIDPrefix, but this constant is not defined within the provided code. This will lead to a compilation error. Ensure thatTimestampIDPrefixis defined appropriately.Apply this diff to define
TimestampIDPrefix:+const TimestampIDPrefix = "t"Place this definition at the top of the file, after the package declaration and before any functions, to ensure it is accessible throughout the module.
Likely invalid or redundant comment.
mod/node-api/engines/echo/vaildator.go (1)
65-65: Approve addition oftimestamp_idvalidatorThe registration of
ValidateTimestampIDunder the"timestamp_id"tag is appropriate and follows the existing pattern.mod/node-core/pkg/components/interfaces.go (1)
116-118: Approved: Addition of 'GetTimestamp' MethodThe addition of the
GetTimestampmethod to theBeaconBlockinterface is appropriate and aligns with the shift to using timestamps. The implementation is correct.
Proofs are indexed by timestamp in EIP 4788, and not execution block numbers. Consequently this allows offchain clients to easily query for beacon block proofs by timestamps, which is directly applicable for use in EL.
Summary by CodeRabbit
New Features
GetTimestampmethod in theBeaconBlockstructure to retrieve the timestamp from the execution payload.Timestampfield in theExecutionPayloadstructure.Bug Fixes
Refactor
KVStorestructure to replace execution numbers with timestamps.