-
Notifications
You must be signed in to change notification settings - Fork 282
fix(worker): respect deadline in commit condition #1243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMain loop in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant W as ScrollWorker
participant T as TxPool
participant C as Chain
rect rgba(220,235,255,0.30)
note over W: Main loop iteration (new)
W->>T: Fetch pending transactions (ev.Txs)
W->>W: processTxnSlice(ev.Txs) -- always run
W->>W: Check block deadline and tx count
alt deadline reached AND tx_count > 0
W->>C: Commit block (use current time ≤ deadline)
C-->>W: Commit result
else otherwise
W->>W: Wait / continue accumulating txs
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug where the signer node commits blocks without waiting for the deadline, leading to ErrFutureBlock
consensus failures under high load. The fix ensures that blocks are only committed when both the processing condition is met AND the deadline has been reached.
- Modified the commit condition to require both
shouldCommit
anddeadlineReached
to be true - Added clarifying comments explaining the logic
- Updated the patch version from 5 to 6
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
miner/scroll_worker.go | Updated commit condition to respect deadline and added explanatory comments |
params/version.go | Incremented patch version from 5 to 6 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
miner/scroll_worker.go
(1 hunks)
⏰ 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). (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
miner/scroll_worker.go (1)
417-422
: Verify impact of removing early commits on block production latency.The fix prevents
ErrFutureBlock
by only committing after the deadline, but also defers all full-block commits (the variousshouldCommit
cases inprocessTxnSlice
) until that deadline. Confirm this won’t harm throughput under load by reviewing those early-commit triggers (e.g. overflow, fork, L1 message logic) and monitoring block production latency metrics.
1. Purpose or design rationale of this PR
It was reported that under high load, the signer node runs into
ErrFutureBlock
consensus failures. The root cause seems to be that onceprocessTxnSlice
returnstrue
, we commit regardless of the deadline (block timestamp). The timestamp without relaxed mode is set toparent.Time + Period
which might be in the future.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.go
been updated?4. Breaking change label
Does this PR have the
breaking-change
label?Summary by CodeRabbit
Bug Fixes
Chores