-
Notifications
You must be signed in to change notification settings - Fork 626
feat: force commit batches at hardfork boundary #1768
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
Conversation
WalkthroughBumps package version tag to v4.7.8 and updates relayer batch processing so ProcessPendingBatches sets Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
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)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rollup/internal/controller/relayer/l2_relayer.go (1)
450-479: Codec-version boundary now (correctly) forces submission; update comment/log for clarityThe new
forceSubmit = truewhen encountering a codec-version change after at least one queued batch makes sense: it ensures the leading contiguous run of older-version batches is committed even iflen(batchesToSubmit) < minBatches, preventing them from being stranded at a hardfork boundary while newer-version batches pile up.Given this, two minor clarity points:
- The function-level comment (Lines 342–347) and the condition comment (“we only submit batches if we have a timeout or if we have enough batches”, Line 471) no longer fully describe the behavior; there is now an additional trigger: “codec-version boundary after at least one batch queued”.
- The log at Line 477 still says
"Forcing submission of batches due to timeout"even whenforceSubmitis only set because of the codec-version boundary or backlog size. That can be misleading when debugging fork transitions.Consider updating comments and the log message to reflect all
forceSubmitcauses, e.g. include whether timeout/backlog/version-boundary triggered it or make the message reason-agnostic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/version/version.go(1 hunks)rollup/internal/controller/relayer/l2_relayer.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: colinlyguo
Repo: scroll-tech/scroll PR: 1714
File: rollup/internal/controller/relayer/l2_relayer.go:1548-1555
Timestamp: 2025-07-29T16:38:24.647Z
Learning: In rollup/internal/controller/relayer/l2_relayer.go, the validateBatchFields function should error out when GetBatchByHash fails to find a parent batch. This is intentional behavior - missing parent batches represent genuine error conditions that should halt batch submission processing. Genesis batch handling occurs separately from normal batch validation flow.
Learnt from: Thegaram
Repo: scroll-tech/scroll PR: 1746
File: rollup/internal/controller/sender/sender.go:637-640
Timestamp: 2025-10-18T06:49:24.796Z
Learning: In the file `rollup/internal/controller/sender/sender.go`, the resubmission logic in `createReplacingTransaction` does not convert V0 blob sidecars to V1 when resubmitting transactions after the Fusaka upgrade. This is an accepted edge case because it's unlikely to occur, geth is expected to handle it gracefully, and manual recovery is available if needed.
📚 Learning: 2025-07-29T16:38:24.647Z
Learnt from: colinlyguo
Repo: scroll-tech/scroll PR: 1714
File: rollup/internal/controller/relayer/l2_relayer.go:1548-1555
Timestamp: 2025-07-29T16:38:24.647Z
Learning: In rollup/internal/controller/relayer/l2_relayer.go, the validateBatchFields function should error out when GetBatchByHash fails to find a parent batch. This is intentional behavior - missing parent batches represent genuine error conditions that should halt batch submission processing. Genesis batch handling occurs separately from normal batch validation flow.
Applied to files:
rollup/internal/controller/relayer/l2_relayer.go
📚 Learning: 2025-10-18T06:49:24.796Z
Learnt from: Thegaram
Repo: scroll-tech/scroll PR: 1746
File: rollup/internal/controller/sender/sender.go:637-640
Timestamp: 2025-10-18T06:49:24.796Z
Learning: In the file `rollup/internal/controller/sender/sender.go`, the resubmission logic in `createReplacingTransaction` does not convert V0 blob sidecars to V1 when resubmitting transactions after the Fusaka upgrade. This is an accepted edge case because it's unlikely to occur, geth is expected to handle it gracefully, and manual recovery is available if needed.
Applied to files:
rollup/internal/controller/relayer/l2_relayer.go
⏰ 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). (3)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (1)
common/version/version.go (1)
8-8: Version bump looks consistent with release metadataTag update to
v4.7.6keeps Version composition unchanged and is consistent with the PR’s intent. No issues here.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1768 +/- ##
===========================================
- Coverage 36.59% 36.59% -0.01%
===========================================
Files 247 247
Lines 21238 21237 -1
===========================================
- Hits 7773 7771 -2
+ Misses 12636 12635 -1
- Partials 829 831 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Purpose or design rationale of this PR
During today's Galileo testnet upgrade, we had the 2 last Feynman batches pending, with a number of Galileo batches after them. We’d expect the Feynman batches to be committed immediately, but this was not the case.
Root cause:
scroll/rollup/internal/controller/relayer/l2_relayer.go
Line 454 in 2ecc42e
scroll/rollup/internal/controller/relayer/l2_relayer.go
Line 471 in 2ecc42e
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:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.