implement forkid changes for time base forks #180
Conversation
WalkthroughThis pull request updates the method for generating fork IDs across multiple packages by adding an extra parameter representing the current block’s timestamp. The changes modify function signatures and internal logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ForkIDGenerator
Caller->>ForkIDGenerator: NewID(config, genesis, blockNumber, blockTime)
Note over ForkIDGenerator: Gather forks based on block and time
ForkIDGenerator-->>Caller: Return computed fork ID
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (4)
🪧 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: 0
🧹 Nitpick comments (9)
les/test_helper.go (1)
553-553: Maintain test coverage for time-based forks.Ensure that new or existing tests validate the fork ID calculation with
head.Time. This helps detect edge cases (e.g., zero or out-of-range timestamps).Would you like help writing a unit test for time-based forks?
core/vm/contracts.go (5)
38-39: Refine error messages for clarity.The error messages lump multiple issues under one umbrella. Consider refining them to precisely describe each failing condition to aid debugging and maintenance.
138-138: Add tests for Morph203 pairing logic.
&bn256PairingIstanbul{morph203: true}introduces a new code path for pairing checks. Ensure the existing test suite covers large input sets and morphological transitions.
328-329: Consider a single versioning mechanism.Adding fields like
morph203can grow unwieldy if multiple forks follow. A more scalable approach might store fork rules in a dedicated struct or strategy pattern.
604-609: Limit the number of Bn256 pairing inputs.Imposing a cap of 4 at line 606 is correct for non-Morph203 cases. Consider logging an error detail or returning an extended message to help users identify the cause of the revert.
640-640: Keep naming consistent.The
morph203field name is consistent withbigModExp. For future forks or EIPs, consider standardizing naming conventions (e.g.,IsMorph203).core/forkid/forkid_test.go (2)
139-139: Consider adding a time-based test case.Here, the test still invokes
NewIDwith a zero time parameter. This may skip coverage of time-based forks. Consider adding a separate test case that supplies a non-zero time to verify the function's response to future and past time-based forks.
250-293: Well-structured test for time-based forks; consider additional edge cases.The new
TestTimeBasedForkInGenesisfunction effectively validates both block-based and time-based forks. To increase coverage, you might test scenarios where a time-based fork and an upcoming block-based fork have overlapping or reverse order triggers. This ensures correct handling when a time-based fork precedes a still-outstanding block-based fork.core/forkid/forkid.go (1)
48-52: Parameterize or document the mainnet timestamp threshold.Relying on a single global
timestampThresholdfor differentiating block- vs. time-based forks is acceptable but might be confusing for private or test networks with different genesis timestamps. Consider making it configurable or clearly documenting these assumptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cmd/devp2p/internal/ethtest/chain.go(1 hunks)core/forkid/forkid.go(8 hunks)core/forkid/forkid_test.go(3 hunks)core/vm/contracts.go(7 hunks)eth/discovery.go(1 hunks)eth/handler.go(1 hunks)eth/protocols/eth/discovery.go(1 hunks)eth/protocols/eth/handshake_test.go(1 hunks)les/client_handler.go(1 hunks)les/peer_test.go(1 hunks)les/server_handler.go(1 hunks)les/test_helper.go(2 hunks)params/config.go(1 hunks)
🔇 Additional comments (19)
params/config.go (1)
30-30: Added utility function for creating uint64 pointersThis new function
NewUint64provides a convenient way to create pointers to uint64 values, which is useful for optional configuration fields or when dealing with fork time values.eth/protocols/eth/handshake_test.go (1)
43-43: Updated forkid.NewID call to include timestamp parameterThe fork ID calculation now includes the block timestamp as a fourth parameter. This change aligns with the implementation of time-based forks mentioned in the PR objectives.
cmd/devp2p/internal/ethtest/chain.go (1)
80-80:✅ Verification successful
Updated ForkID method to include block timestamp
The ForkID method now includes the genesis block's timestamp (
c.blocks[0].Time()) as the fourth parameter to forkid.NewID. This is part of the changes to support time-based forks.
🏁 Script executed:
#!/bin/bash # Check consistency of forkid.NewID usage across codebase rg -A 1 "forkid\.NewID\(" --glob "*.go" | grep -v "Time"Length of output: 945
Approval: ForkID timestamp integration verified
The change in
cmd/devp2p/internal/ethtest/chain.gocorrectly includesc.blocks[0].Time()as the fourth parameter toforkid.NewIDfor supporting time-based forks. A cross-code search confirms that no other instances require this change.
- Location:
cmd/devp2p/internal/ethtest/chain.go, line 80- Verification: The grep output shows the usage of
forkid.NewIDfiltered by the inclusion ofTime, confirming that only the intended change was applied.les/peer_test.go (1)
128-129: Added timestamp parameter to forkid.NewID callsBoth forkID1 and forkID2 calculations now include the current header's timestamp as the fourth parameter. This maintains consistency with the other changes implemented for time-based fork identification.
eth/protocols/eth/discovery.go (1)
63-63: Updated fork ID generation to include block timestampThe modification adds the current header's timestamp as a parameter to
forkid.NewID(), which enables time-based fork identification. This change is part of the broader implementation for time-based forks mentioned in the PR objectives.les/client_handler.go (1)
114-114: Updated fork ID generation to include block timestampThe function call to
forkid.NewIDhas been updated to include the current header's timestamp as the fourth parameter. This change aligns with the PR objective to implement time-based fork identification.les/server_handler.go (1)
119-119:❓ Verification inconclusive
Updated fork ID generation to include block timestamp
The function call to
forkid.NewIDnow includesh.blockchain.CurrentBlock().Time()as the fourth parameter. This change is consistent with the time-based fork implementation across the codebase.Note that this file uses
CurrentBlock().Time()while other implementations useCurrentHeader().Time. Ensure this difference is intentional and doesn't cause inconsistencies in fork ID generation.
🏁 Script executed:
#!/bin/bash # Check if there's any difference between CurrentBlock().Time() and CurrentHeader().Time in practice # This is important to ensure consistent fork ID generation # Look for all instances of CurrentBlock().Time() echo "Checking CurrentBlock().Time() usage:" rg "CurrentBlock\(\)\.Time\(\)" --type go # Look for all instances of CurrentHeader().Time echo -e "\nChecking CurrentHeader().Time usage:" rg "CurrentHeader\(\)\.Time" --type go # Check if these are used interchangeably with forkid.NewID echo -e "\nChecking usage with forkid.NewID:" rg "forkid\.NewID.*Time" --type goLength of output: 3297
Attention: Verify Fork Timestamp Method Consistency
After reviewing the codebase output, it’s clear that while the
les/server_handler.gofile usesh.blockchain.CurrentBlock().Time()when generating the fork ID, other parts of the code (e.g., inles/client_handler.go,les/peer_test.go, and various files undereth/) consistently useCurrentHeader().Time. This discrepancy may be intentional, but it raises the following points for review:
- Verification Needed: Confirm that
CurrentBlock().Time()andCurrentHeader().Timereturn equivalent timestamps or that any differences are acceptable for fork ID generation.- Potential Inconsistency: If these methods yield different values, this inconsistency might affect the behavior of the time-based fork implementation across the codebase.
- Action Item: Please double-check whether using
CurrentBlock().Time()in this context is deliberate. If so, consider adding a brief inline comment or documentation note to explain why the block’s timestamp is used here instead of the header’s timestamp, thereby preventing future confusion.eth/handler.go (1)
277-277: Updated fork ID generation to include block timestampThe call to
forkid.NewIDhas been modified to include the current header's timestamp, which is necessary for time-based fork identification. This change is consistent with modifications in other files and aligns with the PR objective.eth/discovery.go (1)
62-62: Validate header before usage.Adding
eth.blockchain.CurrentHeader().Timealigns with time-based fork requirements. Ensure the returned header is neverniland that theTimefield is correctly set to avoid potential panics or inconsistent fork ID values.les/test_helper.go (1)
489-489: Confirm consistent usage ofhead.Time.Including the timestamp in
forkid.NewIDsupports time-based forks. Verify all call sites now pass the correct timestamp to maintain consistent fork ID generation.core/vm/contracts.go (3)
135-135: ConfirmbigModExpcorrectness under Morph203.Initializing
bigModExpwith botheip2565: trueandmorph203: truecould affect exponentiation constraints. Validate large input edge cases to prevent unexpected reverts.
462-468: Double-check the length limit logic.When
morph203is false, inputs exceeding 32 bytes causeerrModexpUnsupportedInput. Confirm that this logic remains consistent with the intended precompile restrictions for each fork stage.
648-648: Retain consistent coverage.
runBn256Pairing(input, c.morph203)ensures correct behavior across forks. Confirm test coverage extends to Morph203 to avoid regressions.core/forkid/forkid_test.go (1)
221-221: Verify time-based coverage for mainnet test.Similar to line 139, passing
0for the time parameter omits time-based checks. If you wish to test time-based forks on mainnet, consider adding a meaningful timestamp instead of0.core/forkid/forkid.go (5)
27-27: Confirm the Go version for slices usage.Using the standard library
slicespackage requires Go 1.21 or newer. Ensure that your build environment meets this requirement.
75-99: Validate block-based vs. time-based ordering logic.In
NewID, the code processes all block-based forks before checking time-based forks. If a later block-based fork is not yet passed, the function returns early and won't incorporate a time-based fork that might have already occurred. Verify that this ordering is intentional for your chain rules. If not, consider merging the fork lists or checking them in chronological order to handle time-based forks that precede unpassed block-based forks.
103-111: Straightforward extension to include header time.Using the header’s timestamp in
NewIDWithChainensures time-based forks are accounted for automatically. Nice addition!
119-124: Time-based fork filtering logic looks good; verify threshold assumption.The updated filter now handles both block-based and time-based forks, defaulting to the threshold check to distinguish them. If you deploy on networks with a genesis timestamp above/below the threshold, confirm this logic remains valid.
Also applies to: 128-130, 135-224
241-296: Reflection-based fork gathering is effective; confirm coverage of custom fields.Using reflection to detect forks ending with "Block" or "Time" matches the existing pattern. Just ensure new custom chain fork fields (if any) are named accordingly, so they aren’t overlooked.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
core/forkid/forkid_test.go (6)
139-139: Update existing test cases to validate time-based forks.The NewID function signature has been updated to include a timestamp parameter, but all existing test cases set this to 0. Consider adding test cases that verify behavior with different timestamp values to thoroughly test the time-based functionality.
221-221: Update existing validation tests to include time-based scenarios.Similar to the creation tests, the validation tests now include a timestamp parameter but only pass 0. Consider expanding the test matrix to include scenarios where timestamps affect fork selection.
251-302: Good addition of time-based fork tests.The new test function thoroughly examines the behavior of time-based forks with various scenarios including passed, unpassed, and subset forks. Nice work on covering these edge cases.
A few suggestions for improvement:
- Add a comment explaining what morph203Time represents (2025-09-09 00:00:00 UTC)
- Consider using constants from a central location rather than hardcoding the timestamp
- morph203Time := uint64(1741579200) - morphMainnetConfig.Morph203Time = params.NewUint64(morph203Time) // 2025-09-09 00:00:00 UTC + // 2025-09-09 00:00:00 UTC + morph203Time := uint64(1741579200) + morphMainnetConfig.Morph203Time = params.NewUint64(morph203Time)
274-275: This test case needs a clearer comment.This test case appears to be checking that a legacy config rejects a client that reports a time-based fork ID after it has passed. Consider adding a comment to clarify the expected behavior.
- {&legacyConfig, 0, morph203Time + 1, ID{Hash: checksumToBytes(0xb0709522), Next: morph203Time}, ErrLocalIncompatibleOrStale}, + // Legacy config (without time-based forks) should reject a client that claims time-based fork knowledge + {&legacyConfig, 0, morph203Time + 1, ID{Hash: checksumToBytes(0xb0709522), Next: morph203Time}, ErrLocalIncompatibleOrStale},
280-291: Group similar test cases with more descriptive comments.The test cases for timestamp-based forks would be clearer if grouped by scenario with comments explaining what each is testing (e.g., remote client version vs local, before/after fork activation).
- // unpassed fork + // Local is before the fork, remote has no future fork knowledge {&morphMainnetConfig, 6656942, morph203Time - 1, ID{Hash: checksumToBytes(0xb0709522), Next: 0}, nil}, - // passed fork + // Local is after the fork, remote has no knowledge of the fork (stale) {&morphMainnetConfig, 6656942, morph203Time + 1, ID{Hash: checksumToBytes(0xb0709522), Next: 0}, ErrRemoteStale}, - // unpassed fork + // Local is before the fork, remote knows about the upcoming fork {&morphMainnetConfig, 6656942, morph203Time - 1, ID{Hash: checksumToBytes(0xb0709522), Next: morph203Time}, nil}, - // passed fork + // Local is after the fork, remote correctly indicates fork knowledge {&morphMainnetConfig, 6656942, morph203Time + 1, ID{Hash: checksumToBytes(0xb0709522), Next: morph203Time}, nil},
292-294: Clarify the "subset fork" test case.The meaning of "subset fork" is not immediately clear. Add a more descriptive comment explaining that this test is checking the case where the remote client indicates an incorrect/earlier timestamp for a known fork.
- // subset fork + // Local is after the fork, remote indicates wrong (earlier) timestamp for the fork {&morphMainnetConfig, 6656942, morph203Time + 1, ID{Hash: checksumToBytes(0xb0709522), Next: morph203Time - 1}, ErrRemoteStale},
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/forkid/forkid.go (2)
48-52: Document the selection criteria for timestamp threshold.While the comment explains the purpose of the
timestampThresholdconstant, it doesn't explain why this specific value (1729490400) was chosen. Consider adding more documentation about the selection criteria to help future maintainers understand this value's significance.
241-296: Refactored fork gathering to support dual fork types.The
gatherForksfunction now:
- Returns separate lists for block-based and time-based forks
- Identifies time-based fields by checking for the "Time" suffix
- Properly sorts and deduplicates both lists
- Handles edge cases like forks at block/time 0
However, the heavy use of reflection makes this code somewhat brittle. Consider adding a comment warning that changes to ChainConfig struct field naming conventions could break this logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
core/forkid/forkid.go(8 hunks)core/forkid/forkid_test.go(3 hunks)
🔇 Additional comments (9)
core/forkid/forkid_test.go (4)
139-139: Function call updated to accommodate new parameter.The
NewIDfunction call has been updated to include a new timestamp parameter (set to 0 for these tests), which supports the time-based fork identification feature being implemented.
221-221: Function signature updated for time-based forks.The callback function passed to
newFilternow returns both block number and timestamp (0), matching the implementation changes in the main code.
251-302: New test function for time-based forks.This comprehensive test function covers critical scenarios for the new time-based fork feature:
- Legacy configurations with block-based forks only
- Morph mainnet with time-based forks
- Testing behavior before and after fork timestamps
- Validation of fork ID generation with different timestamp values
The test cases look well-designed and thoroughly cover the functionality.
257-258:✅ Verification successful
Verify the Morph203 timestamp value.
The timestamp 1741579200 corresponds to March 10, 2025, which matches the comment. This resolves the issue identified in a previous review.
🏁 Script executed:
#!/bin/bash # Convert the timestamp to human-readable format to verify it's correct date -u -d @1741579200Length of output: 53
MORPH203 TIMESTAMP VERIFIED
The script output confirms that the timestamp1741579200converts toMon Mar 10 04:00:00 UTC 2025, which exactly matches the comment in the code (filecore/forkid/forkid_test.goat lines 257-258). There are no further issues to address.core/forkid/forkid.go (5)
75-76: Comprehensive update to support time-based forks.The
NewIDfunction now properly handles both block-based and time-based forks by:
- Collecting both types of forks from the chain configuration
- Processing block-based forks first, then time-based forks
- Updating the checksum calculation for both fork types
- Setting the Next field based on the first upcoming fork (block or time)
The implementation is logically sound.
Also applies to: 80-99
103-110: Updated to pass current header time to NewID.The function now correctly extracts both the block number and timestamp from the current header to support time-based forks.
119-123: Updated filter creation to support time-based forks.The callback function now correctly returns both block number and timestamp from the current header.
151-154: Added safeguard for purely block-based fork scenarios.This conditional adds a safety mechanism to ensure that in purely block-based forks, the sentry value doesn't incorrectly enter timestamp territory, which could cause validation issues.
176-194: Enhanced validation to support time-based forks.The validation logic now correctly:
- Determines whether to use block number or timestamp based on the fork type
- Checks for incompatibilities based on both block number and time
- Uses the timestamp threshold to identify time-based fork values
The implementation properly extends the validation rules to support time-based forks.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
core/forkid/forkid.go (3)
48-52: Consider a more robust approach for fork type differentiationThe use of a timestamp threshold to differentiate between block-based and timestamp-based forks, while functional, introduces a "hacky" solution as acknowledged in the comment. This approach might cause issues if timestamps need to be used below this threshold in the future.
Consider implementing a more explicit type system for forks that doesn't rely on a numerical threshold to differentiate between block numbers and timestamps. A more robust solution could involve using dedicated fork type identifiers or separate structures for each fork type.
152-152: Fix typo in commentThere's a small typo in the comment.
- // In purely block based forks, avoid the sentry spilling into timestapt territory + // In purely block based forks, avoid the sentry spilling into timestamp territory
193-193: Add comment explaining the time-based fork validationThe condition has been updated to check both block number and timestamp against the remote fork ID, but the complexity of this check deserves explanation.
Add a comment explaining the extended condition that now checks for both block-based and time-based remote forks:
- if id.Next > 0 && (head >= id.Next || (id.Next > timestampThreshold && time >= id.Next)) { + // Check if we've passed a remote fork: + // 1. For block-based forks (id.Next <= timestampThreshold), compare against head block number + // 2. For time-based forks (id.Next > timestampThreshold), compare against current timestamp + if id.Next > 0 && (head >= id.Next || (id.Next > timestampThreshold && time >= id.Next)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
core/forkid/forkid.go(8 hunks)
🔇 Additional comments (3)
core/forkid/forkid.go (3)
262-271:✅ Verification successful
Verify correct fork type classification
The code appears to classify forks based on their field types rather than their intended purpose:
- Fields of type
*uint64are classified as time-based forks- Fields of type
*big.Intare classified as block-based forksThis classification strategy might lead to incorrect fork ID calculations if there are block-based forks defined as pointers to uint64 or timestamp-based forks defined as big.Int.
Run the following script to check if the classification is consistent with the actual parameter definitions in params package:
🏁 Script executed:
#!/bin/bash # List all ChainConfig fields with their types echo "Checking ChainConfig field types and names:" grep -A 50 "type ChainConfig struct" ./params/config.go | grep -E "Time|Block" | grep -v funcLength of output: 2559
Fork type classification verified as correct.
After checking theChainConfiginparams/config.go, it's clear that all fork fields intended to specify block numbers (e.g., HomesteadBlock, EIP150Block, etc.) are defined as*big.Int, while the only time-based fork (Morph203Time) is defined as*uint64. The logic incore/forkid/forkid.gocorrectly differentiates between these fork types based solely on their data types. No changes are necessary.
75-99:❓ Verification inconclusive
Verify handling of concurrent block and time-based forks
The
NewIDfunction now handles both block-based and timestamp-based forks separately, but doesn't address potential edge cases where both types of forks might be activated at the same logical point.Ensure that your testing covers scenarios where both a block-based fork and a timestamp-based fork could be triggered simultaneously or in close proximity. The current implementation processes block-based forks first, but it's important to verify this ordering is intentional and works correctly in all scenarios.
🏁 Script executed:
#!/bin/bash # Check if there are any test cases that specifically test concurrent block and time-based forks grep -r "TestConcurrentForks\|test.*concurrent.*fork" ./core/forkid/Length of output: 70
Review: Address Missing Test Coverage for Concurrent Forks
The implementation in
core/forkid/forkid.gocorrectly processes block-based forks before timestamp-based forks. However, our automated search didn’t uncover any tests specifically targeting scenarios where a block-based fork and a timestamp-based fork might trigger concurrently or almost simultaneously.Please verify manually or add new tests (e.g., in
core/forkid/forkid_test.go) to cover cases where both fork types could be activated in close proximity. This will ensure that the ordering of processing (blocks first, then timestamps) works as intended under all conditions.
273-288:❓ Verification inconclusive
Handle potential duplicates across fork types
The code deduplicates within block-based forks and within time-based forks separately, but doesn't handle the case where a block number and a timestamp might have the same value.
While this is unlikely in practice (as timestamps are generally much larger than block numbers), it's worth verifying that your testing covers this edge case, especially given the "hacky" threshold approach mentioned in the comments.
🏁 Script executed:
#!/bin/bash # Check if there are any test cases that test for duplicate values across fork types grep -r "TestDuplicateForks\|test.*duplicate.*fork" ./core/forkid/Length of output: 68
Attention: Verify Handling of Cross-Type Duplicate Fork Identifiers
The deduplication logic in
core/forkid/forkid.go(lines 273–288) only removes duplicates within each fork type (i.e., withinforksByBlockandforksByTimeseparately). This means that if a block number and a timestamp happen to be numerically equal—a potential edge case—even though it’s unlikely given their generally distinct ranges, the current logic won’t catch such a duplicate.Recommendations:
- Test Coverage: Please verify and consider adding a test case specifically targeting the scenario where a block number might equal a timestamp. This will help ensure that any unintended deduplication across fork types is caught.
- Code Review: Confirm whether the current approach (separate deduplication) was intentional. If a unified deduplication is desired, the logic might need revision.
| time := strings.HasSuffix(field.Name, "Time") | ||
| if !time && !strings.HasSuffix(field.Name, "Block") { | ||
| continue | ||
| } | ||
| if field.Type != reflect.TypeOf(new(big.Int)) { | ||
| continue | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve fork type detection logic
The current approach to detect timestamp-based forks using string suffix matching is brittle and could lead to incorrect classification if field names follow unexpected patterns.
Consider implementing a more robust method for determining fork types. Instead of relying on naming conventions, you could:
- Use explicit type annotations in the
ChainConfigstruct - Define dedicated lists of which fields are timestamp-based vs block-based
- Implement a more sophisticated reflection-based approach that checks field documentation or tags
This would make the code more maintainable and less prone to errors if new fork types are added in the future.
1. Purpose or design rationale of this PR
implement forkid changes for time base forks, pre-compile contracts add fork check.
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Tests