fix: accept header at most 100ms earlier than timestamp #1129
Conversation
WalkthroughThis pull request refines consensus and miner functionality. In the consensus module, the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant verifyHeader
participant Timer
Caller->>verifyHeader: Pass block header with timestamp
verifyHeader->>Timer: Capture current time (now)
alt header.Time > now
verifyHeader->>verifyHeader: Check for 100ms leeway conditions
alt Conditions not met
verifyHeader-->>Caller: Return future block error
else Conditions met
verifyHeader-->>Caller: Accept header
end
else
verifyHeader-->>Caller: Accept header
end
sequenceDiagram
participant ScrollWorker
participant Logger
Note over ScrollWorker: In collectPendingL1Messages
ScrollWorker->>Logger: Log warning if block timestamp is backdated
Note over ScrollWorker: In newWork
ScrollWorker->>ScrollWorker: Check if system contract relaxed period is active
alt Relaxed period active
ScrollWorker->>ScrollWorker: Set deadline to header time + period
else
ScrollWorker->>ScrollWorker: Use standard deadline calculation
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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: 0
🧹 Nitpick comments (1)
consensus/system_contract/consensus.go (1)
119-124: Excellent enhancement to allow a small timing leeway for future blocks.This change adds a sophisticated tolerance mechanism for blocks that appear to be slightly in the future:
- It captures the current time once at the beginning of the check
- For blocks with timestamps in the future, it adds a ~100ms leeway by:
- Allowing blocks with timestamps exactly 1 second ahead, but only when we're already 900ms into the current second
- This effectively creates a small buffer to handle timer inaccuracies
This directly addresses the "block in the future" error mentioned in the PR objectives by allowing for small timing discrepancies between different components of the system.
For improved readability, you could extract the leeway logic into a named helper function:
-// Don't waste time checking blocks from the future -now := time.Now() -if header.Time > uint64(now.Unix()) { - // Add 100ms leeway since the scroll_worker internal timers might trigger early. - if uint64(now.Unix())+1 != header.Time || now.Nanosecond() < 900000000 { - return consensus.ErrFutureBlock - } -} +// Don't waste time checking blocks from the future +now := time.Now() +if header.Time > uint64(now.Unix()) { + // Add 100ms leeway since the scroll_worker internal timers might trigger early. + if !isWithinFutureBlockLeeway(now, header.Time) { + return consensus.ErrFutureBlock + } +} + +// isWithinFutureBlockLeeway checks if the header time is within +// the allowed leeway (100ms) for future blocks +func isWithinFutureBlockLeeway(now time.Time, headerTime uint64) bool { + return uint64(now.Unix())+1 == headerTime && now.Nanosecond() >= 900000000 +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
consensus/system_contract/consensus.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 (2)
miner/scroll_worker.go (2)
467-467: Enhanced logging for timestamp adjustment during EuclidV2 transition.This log message provides valuable visibility into when block timestamps are being backdated to ensure they precede the EuclidV2 transition. This will help with debugging and monitoring the transition process.
545-548: Good addition for handling system contracts with relaxed periods.This change appropriately extends the deadline calculation logic to support system contracts with relaxed periods, similar to how it's handled for Clique consensus. When a system contract is configured with relaxed period, the deadline is now properly calculated as the current header time plus the system contract's period.
This change directly addresses the PR's objective of fixing timing issues, as it ensures that blocks have appropriate deadlines when using system contracts with relaxed periods.
Thegaram
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
consensus/clique/clique.go (1)
257-261: Good fix for timing issue by adding leeway to future block validationThis change adds a 100ms leeway when checking for blocks from the future, addressing the intermittent "block in the future" errors mentioned in the PR objective. The implementation correctly:
- Captures the current time once in a variable to ensure consistency
- Applies the leeway only when needed by checking both conditions
- Clearly documents the rationale in the comments
For slightly improved readability, you could consider extracting the time difference into a named variable:
now := time.Now() -if header.Time > uint64(now.Unix()) && time.Unix(int64(header.Time), 0).Sub(now) > 100*time.Millisecond { +// Calculate the time difference between block timestamp and current time +timeDiff := time.Unix(int64(header.Time), 0).Sub(now) +if header.Time > uint64(now.Unix()) && timeDiff > 100*time.Millisecond { return consensus.ErrFutureBlock }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
consensus/clique/clique.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: check
1. Purpose or design rationale of this PR
Attempt to fix the following ocassional issue:
Context.
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