-
Notifications
You must be signed in to change notification settings - Fork 628
feat: support Fusaka blob type #1746
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
WalkthroughPin go-ethereum and bump several KZG/crypto and indirect dependencies; add Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Sender as Sender
participant L1 as L1 RPC
participant Config as SenderConfig
participant Sidecar as makeSidecar
Sender->>L1: request header (number, time, baseFee, blobBaseFee)
L1-->>Sender: header.number, header.time, baseFee, blobBaseFee
Sender->>Config: read FusakaTimestamp
alt header.time < FusakaTimestamp
note right of Sender #f9f0c1: Pre‑upgrade — pause/skip blob txs
Sender-->>Sender: skip sending blobs / return early
else header.time >= FusakaTimestamp
note right of Sender #e6f7e6: Post‑upgrade — use new sidecar proofs
Sender->>Sidecar: makeSidecar(version, blobs)
Sidecar-->>Sender: BlobTxSidecar (versioned proofs)
Sender->>Sender: build & send (or replace) tx
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1746 +/- ##
===========================================
- Coverage 36.96% 36.94% -0.03%
===========================================
Files 245 245
Lines 20897 20920 +23
===========================================
+ Hits 7725 7728 +3
- Misses 12353 12370 +17
- Partials 819 822 +3
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:
|
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
🧹 Nitpick comments (1)
rollup/internal/controller/sender/sender.go (1)
314-316
: Clarify the 180-second pause window.The hardcoded 180-second (3-minute) pause window before the Fusaka upgrade prevents blob transactions near the transition. While this provides a safety buffer, consider:
- Document the rationale for choosing 180 seconds (e.g., expected block time × safety margin)
- Consider making this duration configurable if different networks have different requirements
Example refactor to make it configurable:
+// The number of seconds before Fusaka upgrade to pause blob transactions +FusakaUpgradePauseWindow uint64 `json:"fusaka_upgrade_pause_window"`Then in sender.go:
-if blockTimestamp < s.config.FusakaTimestamp && (s.config.FusakaTimestamp-blockTimestamp) < 180 { - return common.Hash{}, 0, fmt.Errorf("pausing blob txs before Fusaka upgrade, eta %d seconds", s.config.FusakaTimestamp-blockTimestamp) +if blockTimestamp < s.config.FusakaTimestamp && (s.config.FusakaTimestamp-blockTimestamp) < s.config.FusakaUpgradePauseWindow { + return common.Hash{}, 0, fmt.Errorf("pausing blob txs before Fusaka upgrade, eta %d seconds", s.config.FusakaTimestamp-blockTimestamp) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
bridge-history-api/go.sum
is excluded by!**/*.sum
common/go.sum
is excluded by!**/*.sum
coordinator/go.sum
is excluded by!**/*.sum
database/go.sum
is excluded by!**/*.sum
go.work.sum
is excluded by!**/*.sum
rollup/go.sum
is excluded by!**/*.sum
tests/integration-test/go.sum
is excluded by!**/*.sum
📒 Files selected for processing (10)
Makefile
(1 hunks)bridge-history-api/go.mod
(4 hunks)common/go.mod
(4 hunks)coordinator/go.mod
(3 hunks)database/go.mod
(2 hunks)rollup/conf/config.json
(1 hunks)rollup/go.mod
(3 hunks)rollup/internal/config/relayer.go
(1 hunks)rollup/internal/controller/sender/sender.go
(6 hunks)tests/integration-test/go.mod
(3 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). (9)
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: test
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (4)
rollup/internal/controller/sender/sender.go (4)
290-294
: LGTM!The function call properly retrieves the block timestamp needed for Fusaka upgrade detection, and error handling is appropriate.
318-321
: LGTM!The version selection logic correctly chooses
BlobSidecarVersion0
before the Fusaka upgrade andBlobSidecarVersion1
at or after the upgrade timestamp. The boundary condition (blockTimestamp == FusakaTimestamp) appropriately uses the post-upgrade version.
827-844
: LGTM!The function correctly extracts
header.Time
as the block timestamp and maintains the existing logic for determining the latest block number (pending - 1). The expanded return signature properly supports the Fusaka upgrade detection.
846-889
: Verify version 1 cell proof handling in external packages
- Confirm
kzg4844.ComputeCellProofs
returns all expected cell proofs per blob- Ensure
gethTypes.NewBlobTxSidecar
accepts the larger proof slice for version 1 without error- Add or update tests for version 1 sidecar creation to assert the correct proof count
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.
should we get scroll-tech/go-ethereum#1245 merged first?
a5ed0c9
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/cmd/rollup_relayer/app/app.go (1)
99-101
: Consider adding timestamp range validation.While the zero check is sufficient to catch missing configuration, you could optionally add validation to ensure FusakaTimestamp represents a reasonable Unix timestamp (e.g., after a known lower bound, not too far in the future). This would catch configuration errors where an invalid timestamp value is provided.
Example validation:
if cfg.L2Config.RelayerConfig.SenderConfig.FusakaTimestamp == 0 { log.Crit("cfg.L2Config.RelayerConfig.SenderConfig.FusakaTimestamp must be set") } +// Sanity check: Fusaka timestamp should be after 2024 (1704067200 = 2024-01-01) +if cfg.L2Config.RelayerConfig.SenderConfig.FusakaTimestamp < 1704067200 { + log.Crit("cfg.L2Config.RelayerConfig.SenderConfig.FusakaTimestamp appears invalid", "timestamp", cfg.L2Config.RelayerConfig.SenderConfig.FusakaTimestamp) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rollup/cmd/rollup_relayer/app/app.go
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
rollup/cmd/rollup_relayer/app/app.go (2)
rollup/internal/config/l2.go (1)
L2Config
(10-29)rollup/internal/config/relayer.go (2)
RelayerConfig
(63-88)SenderConfig
(9-40)
⏰ 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). (19)
- GitHub Check: coordinator-cron
- GitHub Check: bridgehistoryapi-db-cli
- GitHub Check: coordinator-api
- GitHub Check: bridgehistoryapi-fetcher
- GitHub Check: bridgehistoryapi-api
- GitHub Check: rollup-db-cli
- GitHub Check: blob_uploader
- GitHub Check: gas_oracle
- GitHub Check: rollup_relayer
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: test
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (1)
rollup/cmd/rollup_relayer/app/app.go (1)
99-101
: LGTM! Essential config validation.The sanity check ensures FusakaTimestamp is explicitly set before initializing the L2 relayer, which is critical since the sender logic depends on this value for version selection and timing-based behavior. The validation pattern is consistent with other config checks and appropriately uses
log.Crit
to fail fast on misconfiguration.
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)
rollup/internal/controller/sender/sender.go
(7 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). (6)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (6)
rollup/internal/controller/sender/sender.go (6)
290-294
: LGTM! Timestamp retrieval integrated correctly.The function call has been properly updated to retrieve and use the block timestamp for version-aware blob transaction handling.
314-316
: Verify the 180-second pause threshold is appropriate.The code pauses blob transactions 3 minutes before the Fusaka upgrade. Ensure this duration provides sufficient safety margin given L1 block times and potential clock skew, while not being overly conservative.
Consider documenting why 180 seconds was chosen (e.g., in a comment or constant).
318-327
: LGTM! Version selection logic is correct.The code properly selects BlobSidecarVersion0 before the upgrade and BlobSidecarVersion1 after, then creates the sidecar with the appropriate version.
653-653
: Acceptable: timestamp not needed in this context.The timestamp is retrieved but unused in
checkPendingTransaction
, which is appropriate since this function only needs block number and fees for gas price adjustment logic.
831-848
: LGTM! Function correctly returns block timestamp.The renamed function properly extracts and returns the block timestamp (
header.Time
) along with other required values.
850-896
: Code correctly implements V1 cell proofs flattening per EIP-7594 — no changes needed.The
append(proofs, ps...)
pattern at line 888 correctly flattens all cell proofs fromComputeCellProofs
into a single array. Per EIP-7594, cell_proofs must be a flattened array containing CELLS_PER_EXT_BLOB proofs for each blob, indexed ascell_proofs[i * CELLS_PER_EXT_BLOB + j]
for cell j of blobs[i]. The sequential processing of each blob's cell proofs and accumulation into a single slice achieves this structure correctly, matching the expected input forNewBlobTxSidecar
.
Purpose or design rationale of this PR
BlobSidecarVersion1
after Fusaka upgrade happens on L1.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
tag
incommon/version.go
been updated or have you addedbump-version
label to this PR?Breaking change label
Does this PR have the
breaking-change
label?Summary by CodeRabbit
New Features
Chores