consensus/l2: allow the block has the same time with the parent block #224
consensus/l2: allow the block has the same time with the parent block #224FletcherMan merged 3 commits intomainfrom
Conversation
WalkthroughThe L2 consensus code exports Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Catalyst as eth/catalyst
participant L2 as consensus/l2
note over Client,Catalyst: Previous flow (delegated Prepare)
Client->>Catalyst: create block data
Catalyst->>L2: Engine().Prepare(header, ...)
L2->>L2: set Difficulty, Nonce, timestamp guards
L2-->>Catalyst: prepared header
Catalyst-->>Client: block
note over Client,Catalyst: New flow (direct init)
Client->>Catalyst: create block data
Catalyst->>Catalyst: header.Difficulty = l2.Difficulty
Catalyst->>Catalyst: header.Nonce = l2.Nonce
Catalyst->>Catalyst: header.UncleHash = EmptyUncleHash
Catalyst->>Catalyst: header.Extra = []byte{}
Catalyst->>Catalyst: if FeeVault: header.Coinbase = empty
Catalyst-->>Client: block
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)consensus/l2/consensus.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
consensus/l2/consensus.go(6 hunks)eth/catalyst/l2_api.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
eth/catalyst/l2_api.go (2)
consensus/l2/consensus.go (2)
Difficulty(20-20)Nonce(21-21)core/types/block.go (2)
EmptyUncleHash(36-36)EmptyAddress(37-37)
consensus/l2/consensus.go (1)
core/types/block.go (1)
EncodeNonce(46-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
| if l2.config.IsEmerald(header.Number, header.Time) && header.Time > uint64(time.Now().Unix()) { | ||
| header.Time = uint64(time.Now().Unix()) | ||
| } |
There was a problem hiding this comment.
Avoid clamping header time below the parent’s timestamp
Clamping to time.Now() here breaks block production as soon as the parent block is even slightly ahead of the local wall clock (which is common, since verifyHeader still accepts future timestamps). worker prepares headers with header.Time ≥ parent.Time; this clamp drags it back below the parent, the subsequent verification trips header.Time < parent.Time, and the sequencer stalls until real time catches up. Please drop this clamp or only adjust after comparing with the parent header so we never regress below parent.Time.
- if l2.config.IsEmerald(header.Number, header.Time) && header.Time > uint64(time.Now().Unix()) {
- header.Time = uint64(time.Now().Unix())
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if l2.config.IsEmerald(header.Number, header.Time) && header.Time > uint64(time.Now().Unix()) { | |
| header.Time = uint64(time.Now().Unix()) | |
| } |
🤖 Prompt for AI Agents
In consensus/l2/consensus.go around lines 198-200, the current clamp that forces
header.Time down to time.Now() can regress the header below its parent’s
timestamp and break block production; remove the unconditional clamp or modify
the logic so you only adjust header.Time after checking the parent timestamp —
ensure header.Time is never set lower than parent.Time (e.g., take the maximum
of parent.Time and the desired time/now) so the header never regresses below its
parent.
2 participants
1. Purpose or design rationale of this PR
Considering that the block time will be less than 1 second in the future, and the timestamp has a precision of seconds. Therefore, this PR supports the possibility that adjacent blocks can have the same time.
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