fix: correctly select consensus engine during EuclidV2 transition #1130
Conversation
WalkthroughThe pull request introduces enhancements to timestamp handling across multiple consensus modules. A new Changes
Sequence Diagram(s)sequenceDiagram
participant BC as BlockChain
participant CE as Consensus Engine
participant CLK as Clock/Time Source
BC->>CE: Prepare(chain, header, timeOverride)
alt timeOverride provided
CE-->>BC: Use provided timeOverride for header timestamp
else
CE->>CE: CalcTimestamp(parent header)
alt Calculated timestamp is in past or relaxed period enabled
CE->>CLK: Fetch current time
CE-->>CE: Adjust timestamp to current time
end
CE-->>BC: Return computed timestamp in header
end
BC->>BC: Write block with finalized header
sequenceDiagram
participant UE as UpgradableEngine
participant E as SelectedConsensusEngine
UE->>UE: Extract timestamp from header or use timeOverride
UE->>UE: chooseEngine(timestamp)
UE-->>E: Return selected consensus engine
UE->>E: Call Prepare(chain, header, timeOverride)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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: 1
🧹 Nitpick comments (3)
core/blockchain.go (1)
1835-1838: Preserve the original timestamp when signing blocks
Passing&originalTimeintobc.engine.Prepareis a clean way to ensure the timestamp isn't overwritten by the consensus engine. Consider adding unit tests around this logic to verify that the timestamp remains intact when signing is enabled.consensus/consensus.go (1)
82-82: Clarify behavior of the new 'timeOverride' parameter in Prepare
Document more clearly that a non-niltimeOverrideinstructs the engine to use that time value and skip its usual timestamp logic. This helps avoid confusion for future maintainers.consensus/wrapper/consensus.go (1)
154-157: UnimplementedCalcTimestampmethod triggers a panic
Currently, calling this method will cause a runtime error. Consider either implementing it or removing it if not used.Below is an example implementation delegating to the chosen engine, if that matches your intent:
-func (ue *UpgradableEngine) CalcTimestamp(parent *types.Header) uint64 { - panic("Called CalcTimestamp on UpgradableEngine, not implemented") -} +func (ue *UpgradableEngine) CalcTimestamp(parent *types.Header) uint64 { + // Delegate to the underlying engine based on the parent's timestamp + return ue.chooseEngine(parent.Time).CalcTimestamp(parent) +}Would you like me to open a new issue or provide a full implementation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
consensus/clique/clique.go(2 hunks)consensus/consensus.go(2 hunks)consensus/ethash/consensus.go(1 hunks)consensus/system_contract/consensus.go(2 hunks)consensus/wrapper/consensus.go(6 hunks)core/blockchain.go(1 hunks)eth/catalyst/api.go(1 hunks)miner/scroll_worker.go(2 hunks)params/version.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- params/version.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (27)
eth/catalyst/api.go (1)
153-153: Updated method call to match the new Engine interface.The
Preparemethod call has been updated to include an additionalnilparameter, which aligns with the new Engine interface that accepts an optionaltimeOverrideparameter.consensus/ethash/consensus.go (2)
582-584: Added unimplemented CalcTimestamp method that throws a panic.The method is required to fulfill the Engine interface, but it's not actually implemented for Ethash. This approach clearly communicates that this functionality isn't supported in Ethash.
However, consider the implications of using a panic in a production system. If this method somehow gets called, it will crash the application rather than returning an error that could be handled gracefully.
Would it be safer to return a predictable fallback value or error rather than panicking? Or is the panic intentional to ensure this path is never accidentally taken?
588-588: Updated Prepare method signature to include timeOverride parameter.The method signature has been updated to match the changes in the Engine interface, which now accepts an optional timestamp override parameter.
consensus/clique/clique.go (3)
510-520: Added CalcTimestamp method for flexible timestamp calculation.This new method encapsulates the logic for calculating block timestamps, considering both the configured period and real-time constraints. It properly handles the
RelaxedPeriodconfiguration, ensuring timestamps are always current when needed.
524-524: Updated Prepare method signature to include timeOverride parameter.The method signature has been updated to match the changes in the Engine interface, adding support for an optional timestamp override.
583-587: Refactored timestamp calculation logic to use the new CalcTimestamp method.The timestamp setting logic has been improved to either use an explicitly provided timestamp override or call the new CalcTimestamp method. This refactoring improves code organization by centralizing timestamp calculation logic.
consensus/system_contract/consensus.go (3)
228-238: Added CalcTimestamp method with logic consistent with Clique implementation.This implementation calculates a block timestamp based on the parent's timestamp and configured period, with appropriate handling for
RelaxedPeriod. The consistency with the Clique implementation is good for maintaining uniform behavior across consensus engines.
242-242: Updated Prepare method signature to include timeOverride parameter.The method signature has been updated to match the changes in the Engine interface, adding support for an optional timestamp override.
258-262: Refactored timestamp calculation logic to use the new CalcTimestamp method.The modification provides a clear choice between using an explicitly provided timestamp or calculating one based on established rules. This change properly integrates with the EuclidV2 transition mentioned in the PR objective.
consensus/consensus.go (1)
114-117: Ensure comprehensive tests for CalcTimestamp
The newly introducedCalcTimestampmethod should be tested under various scenarios (e.g., parent’s timestamp close to or equal to the next block’s). Include boundary cases to ensure correct behavior across different consensus transitions.miner/scroll_worker.go (2)
497-501: Handle default L1 message index with care
Reading the parent’s queue index and defaulting to zero if absent works fine, but ensure that edge cases (e.g., genesis or missed writes) don’t cause silent off-by-one issues. A quick test verifying the fallback to zero explicitly can help prevent regressions.
511-511: Nil timeOverride usage
Here,Prepareis called with aniltime override, signaling that the engine should manage the header time as usual. Confirm that this is intended behavior in all normal mining scenarios without special overrides.consensus/wrapper/consensus.go (15)
42-43: Refactored to a timestamp-based signature forchooseEngine
This new approach cleanly separates engine selection from header details, ensuring future flexibility.
54-54: Consistent use of timestamp-based engine selection inAuthor
Switching toheader.Timefor engine detection aligns well with the updatedchooseEnginefunction.
59-59: Unified engine selection inVerifyHeader
This preserves the same logic while simplifying the parameter flow. The approach looks good.
75-76: Batch verification starts and ends with timestamp checks
Relying on the first and last header timestamps to determine engine transitions is valid under single-transition assumptions.
92-92: Conditional transition to system engine
The conditional logic for identifying system headers mid-batch is well-structured and aligns with the single-switchover assumption.
159-163: Time override logic inPrepare
This conditional override is helpful, but ensure inputs can't cause unexpected transitional edge cases (e.g., an override less than the parent's time).
165-169: Graceful handling of unknown ancestors
Properly returningErrUnknownAncestorif the parent is missing is a robust error-check.
170-185: Clique-based transition block detection
Calculating clique timestamp for the parent, then deciding if the child block transitions to EuclidV2 is logical. Double-check boundary conditions where the parent's time is near the switchover threshold.
186-199: Post-EuclidV2 timestamp handling
Transition logic here correctly accounts for the possibility of reverting to clique by accident. Confirm that scenario never legitimately occurs.
204-204: Engine selection forSeal
Aligns with the new pattern of usingblock.Time().
209-209: Time-based difficulty calculation
The approach of re-selecting the engine based onparent.Timeremains consistent with prior logic.
214-214: Finalize via chosen engine
Delegating finalization is a straightforward and coherent extension.
219-219: Finalize-and-assemble via chosen engine
Methodically follows the same pattern, promoting consistency in final assembly logic.
224-224: Timestamp-based uncle verification
CallingVerifyUncleson the appropriate engine is properly aligned with the overarching design.
249-249: Utilize chosen engine'sSealHash
Sealing logic now uniformly routes throughchooseEngine(header.Time), ensuring consistent calculations.
1. Purpose or design rationale of this PR
#1102 added
UpgradableEngine, a consensus engine that chooses one ofCliqueandSystemContractbased on the block timestamp.However, as noted in #1126, this will not work well in multiple scenarios:
Preparewith the correct engine (in this case Clique).This PR fixes this by adding a timestamp override mechanism to
consensus.Engine.UpgradableEngineinspects the resulting timestamps and chooses the correct engine accordingly.ScrollWorkerre-runsPreparewith a timestamp override if needed.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
Refactor
Chores