Remove sequencer batch write paths and unused NewL2Block batch hash#319
Remove sequencer batch write paths and unused NewL2Block batch hash#319FletcherMan merged 3 commits intomainfrom
Conversation
Drop geth's sequencer batch write handlers and storage helpers so batch creation is fully removed from the execution client while historical read surfaces stay intact. Made-with: Cursor
Drop the unused batchHash parameter from engine_newL2Block so the sequencer execution path only carries executable block data while BatchHash remains confined to the safe block compatibility flow. Made-with: Cursor
📝 WalkthroughWalkthroughThis PR removes batch processing subsystem integration from the Ethereum protocol backend by eliminating the batchHandler infrastructure, simplifying RPC types, removing batchHash parameters from L2 block creation, and deleting unused batch utility functions across multiple components. ChangesBatch Processing Removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.12.1)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Code reviewFound 1 issue:
go-ethereum/eth/catalyst/l2_api.go Lines 292 to 321 in 4019516 (Compare with 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Code review (additional finding)[Important] With Cross-repo search (
Preserving "historical-only backfill / on-demand recompute" is only valuable when there is a consumer that needs the data. There isn't one. Keeping an API that always returns Recommend deleting, as a follow-up to this PR (or within it):
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
|
Addressed panos' review feedback in
Validation:
New reference tag: |
Drop the L1 fee RPC/read helpers and leftover block context decoder now that sequencer batch fee collection is no longer written by geth. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
eth/catalyst/l2_api.go (1)
293-321:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftClarify the design intent for
Header.BatchHashacross live and safe block paths.
ExecutableL2Datadeliberately omitsBatchHash(per its documented design: "fields which need to be signed...and affect state calculation"). As a result, blocks constructed via the live sequencer path (NewL2Block/AssembleL2Block/ValidateL2Block) will haveHeader.BatchHash == common.Hash{}, while the safe path (NewSafeL2BlockwithSafeL2Data) populates it. SinceBatchHashremains exposed in RPC output (eth/api.go:545), the same logical block built through different paths will have different hashes and different RPC representations.If this asymmetry is intentional (i.e.,
BatchHashis only relevant for finalized, L1-approved blocks), document it explicitly in the PR description and ensure indexers, RPC clients, and derivation pipelines are aware that live-sequenced blocks will report zerobatchHash. If not intentional, addBatchHashtoExecutableL2Dataand populate it inexecutableDataToBlock, or remove it from theHeadertype entirely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eth/catalyst/l2_api.go` around lines 293 - 321, The current asymmetry is that executableDataToBlock (which consumes ExecutableL2Data) leaves Header.BatchHash zero while the safe-path (NewSafeL2Block using SafeL2Data) sets it, causing differing block hashes/RPC output; decide and implement one of three fixes: 1) If zero BatchHash for live blocks is intended, add a clear note in the PR and repository docs and update any consumers (RPC/indexers/derivation) to expect zero batchHash for sequenced blocks (mention NewL2Block/AssembleL2Block/ValidateL2Block and eth API consumers); 2) If it is not intended, extend ExecutableL2Data to include BatchHash and set header.BatchHash inside executableDataToBlock so live-built blocks match safe-built blocks; or 3) remove BatchHash from the Header type entirely if it must not affect block identity; implement the chosen change and update related tests and RPC serialization code that references Header.BatchHash.
🧹 Nitpick comments (1)
eth/catalyst/l2_api_test.go (1)
191-213: ⚡ Quick winConsider locking in the new
BatchHashsemantics with an assertion.The signature update is correct. Since this PR changes a header field's behavior on the live path (
Header.BatchHashis now always zero for blocks built viaNewL2Block), a small assertion here would prevent silent regressions:♻️ Suggested addition
err = api.NewL2Block(l2Data) require.NoError(t, err) + + // Live-sequenced blocks no longer carry a BatchHash. + currentHeader := ethService.BlockChain().CurrentHeader() + require.Equal(t, common.Hash{}, currentHeader.BatchHash)Skip if you plan to remove
Header.BatchHashoutright in a follow-up.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eth/catalyst/l2_api_test.go` around lines 191 - 213, Add an assertion verifying the new BatchHash semantics: after calling api.NewL2Block(l2Data) and again after creating a block via api.AssembleL2Block/ api.NewL2Block (using resp from AssembleL2Block and validResp from api.ValidateL2Block), retrieve the block header and assert that Header.BatchHash is zero (empty/zero value) to lock in the behavior change and prevent regressions; place the checks near the existing balance assertions that follow NewL2Block and the Assemble/Validate/NewL2Block sequence so failures clearly tie to the new header behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@eth/catalyst/l2_api.go`:
- Around line 293-321: The current asymmetry is that executableDataToBlock
(which consumes ExecutableL2Data) leaves Header.BatchHash zero while the
safe-path (NewSafeL2Block using SafeL2Data) sets it, causing differing block
hashes/RPC output; decide and implement one of three fixes: 1) If zero BatchHash
for live blocks is intended, add a clear note in the PR and repository docs and
update any consumers (RPC/indexers/derivation) to expect zero batchHash for
sequenced blocks (mention NewL2Block/AssembleL2Block/ValidateL2Block and eth API
consumers); 2) If it is not intended, extend ExecutableL2Data to include
BatchHash and set header.BatchHash inside executableDataToBlock so live-built
blocks match safe-built blocks; or 3) remove BatchHash from the Header type
entirely if it must not affect block identity; implement the chosen change and
update related tests and RPC serialization code that references
Header.BatchHash.
---
Nitpick comments:
In `@eth/catalyst/l2_api_test.go`:
- Around line 191-213: Add an assertion verifying the new BatchHash semantics:
after calling api.NewL2Block(l2Data) and again after creating a block via
api.AssembleL2Block/ api.NewL2Block (using resp from AssembleL2Block and
validResp from api.ValidateL2Block), retrieve the block header and assert that
Header.BatchHash is zero (empty/zero value) to lock in the behavior change and
prevent regressions; place the checks near the existing balance assertions that
follow NewL2Block and the Assemble/Validate/NewL2Block sequence so failures
clearly tie to the new header behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 510a90fc-4af5-4512-a686-1a273b75bb34
📒 Files selected for processing (9)
core/rawdb/accessors_rollup_batch.goeth/api.goeth/backend.goeth/catalyst/l2_api.goeth/catalyst/l2_api_test.goethclient/authclient/engine.goethclient/ethclient.gorollup/batch/block_context.gorollup/batch/handler.go
💤 Files with no reviewable changes (6)
- ethclient/ethclient.go
- rollup/batch/block_context.go
- rollup/batch/handler.go
- eth/api.go
- eth/backend.go
- core/rawdb/accessors_rollup_batch.go
Summary
RollupBatchL1DataFeeKeyNewL2BlockbatchHash argument from the live executable block path; newly live-sequenced blocks intentionally keepHeader.BatchHashzero because batch boundaries are no longer produced by the sequencerHeader.BatchHashitself for RLP/wire compatibility and keep the safe-block compatibility path (NewSafeL2Block/SafeL2Data.BatchHash) unchangedTest plan
go build ./...go test ./eth/catalyst ./ethclient/authclient -short -count=1go test ./eth ./ethclient ./core/rawdb -run '^$' -count=1go test ./... -short -count=1(the repo currently has unrelated baseline failures outside this diff)Summary by CodeRabbit
Refactor
CollectedL1Feefield from rollup batch RPC responsesChores
GetRollupBatchL1FeeByIndexRPC methodBatchHandler()public methodTests