refactor(rollup-relayer): simplify logic post-Euclid #1658
Conversation
## Walkthrough
This set of changes removes support for legacy codec versions (V4, V5, V6) throughout the rollup system, updating all relevant code, configuration, and tests to exclusively use CodecV7. Numerous resource limit parameters and related metrics (e.g., gas, calldata size, row consumption) are eliminated from configs, structs, and logic. Corresponding tests, configuration files, and metrics are refactored or simplified accordingly.
## Changes
| File(s) | Change Summary |
|---------|---------------|
| `common/version/version.go` | Updated version tag from "v4.5.12" to "v4.5.13". |
| `rollup/abi/bridge_abi_test.go` | Removed six test functions related to batch and bundle packing for legacy codecs. |
| `rollup/cmd/proposer_tool/app/app.go`, `rollup/cmd/rollup_relayer/app/app.go` | Added validation to enforce minimum codec version of CodecV7; program exits on lower versions. |
| `rollup/conf/config.json`, `rollup/proposer-tool-config.json` | Removed resource limit parameters (e.g., max_tx_num_per_chunk, max_l1_commit_gas_per_chunk) from chunk and batch proposer configs; increased max_chunks_per_batch. |
| `rollup/docker-compose-proposer-tool.yml` | Updated min codec version argument from "4" to "7" in proposer-tool service. |
| `rollup/internal/config/l2.go` | Removed legacy resource limit fields from `ChunkProposerConfig` and `BatchProposerConfig` structs. |
| `rollup/internal/config/relayer.go` | Removed `TestEnvBypassOnlyUntilForkBoundary` field from `RelayerConfig`. |
| `rollup/internal/controller/relayer/common.go` | Removed deprecated exported error variables `ErrExecutionRevertedMessageExpired` and `ErrExecutionRevertedAlreadySuccessExecuted`. |
| `rollup/internal/controller/relayer/l2_relayer.go` | Removed all legacy codec versions (V4-V6) support and related batch processing logic; now supports only CodecV7. Deleted legacy methods and simplified batch and bundle processing. |
| `rollup/internal/controller/relayer/l2_relayer_test.go` | Refactored tests to use CodecV7 exclusively; updated proofs to OpenVMBundleProof with patched sanity checks; added blocks field to batches; extended chain config with Euclid fork times. |
| `rollup/internal/controller/watcher/batch_proposer.go`, `rollup/internal/controller/watcher/batch_proposer_test.go` | Consolidated batch proposer config into single struct; removed gas and calldata size limits and related metrics; updated tests for CodecV7 and new config. |
| `rollup/internal/controller/watcher/bundle_proposer.go`, `rollup/internal/controller/watcher/bundle_proposer_test.go` | Replaced separate config fields with config struct; removed CodecV5-specific batch per bundle limitation; tests updated for CodecV7 and simplified configs. |
| `rollup/internal/controller/watcher/chunk_proposer.go`, `rollup/internal/controller/watcher/chunk_proposer_test.go` | Consolidated chunk proposer config; removed multiple resource limits and metrics; removed special-case Euclid transition chunk proposal; tests updated for CodecV7 and added genesis chunk insertion. |
| `rollup/internal/controller/watcher/l2_watcher.go`, `rollup/internal/controller/watcher/l2_watcher_metrics.go` | Removed validation and metrics related to RowConsumption and L1 commit calldata size. |
| `rollup/internal/controller/watcher/watcher_test.go` | Updated test runner to invoke CodecV7 test variants instead of CodecV4; removed some legacy tests without replacement. |
| `rollup/internal/orm/batch.go` | Reformatted struct literal initialization in `InsertBatch` for compactness; no logic changes. |
| `rollup/internal/orm/chunk.go` | Marked `CrcMax`, `TotalL1CommitCalldataSize`, and `TotalL1CommitGas` fields as deprecated; stopped setting these fields in `InsertChunk`. |
| `rollup/internal/orm/l2_block.go` | Removed all handling of the `RowConsumption` field in block retrieval and insertion methods. |
| `rollup/internal/orm/orm_test.go` | Cleared `RowConsumption` field in blocks during test setup to nil. |
| `rollup/internal/utils/utils.go` | Removed `CrcMax`, L1 commit gas, calldata size, and uncompressed batch size fields from chunk and batch metrics structs; removed related timing and error handling; retained only blob size metric. |
| `rollup/tests/bridge_test.go` | Removed invocation of legacy codec batch/bundle test `testCommitBatchAndFinalizeCodecV4V5V6`. |
| `rollup/tests/gas_oracle_test.go` | Removed initialization of `RowConsumption` in block structs in test functions. |
| `rollup/tests/process_start_test.go` | Changed min codec version argument from "4" to "7" in rollup relayer app tests. |
| `rollup/tests/rollup_test.go` | Deleted legacy codec batch/bundle test `testCommitBatchAndFinalizeCodecV4V5V6`; updated configs and formatting in remaining CodecV7 test. |
| `rollup/go.mod` | Updated dependency `github.com/scroll-tech/da-codec` version to a newer commit. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant ProposerTool
participant RollupRelayer
participant Config
participant DB
User->>ProposerTool: Start with --min-codec-version=7
ProposerTool->>Config: Load config (CodecV7 only)
ProposerTool->>ProposerTool: Validate minCodecVersion >= CodecV7
ProposerTool->>DB: Propose chunks/batches (CodecV7 logic only)
User->>RollupRelayer: Start with --min-codec-version=7
RollupRelayer->>Config: Load config (CodecV7 only)
RollupRelayer->>RollupRelayer: Validate minCodecVersion >= CodecV7
RollupRelayer->>DB: Process batches/bundles (CodecV7 logic only)Poem
|
colinlyguo
force-pushed
the
refactor-simplify-rollup-relayer
branch
from
79902c4 to
d961f40
Compare
colinlyguo
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1658 +/- ##
===========================================
+ Coverage 40.91% 41.13% +0.22%
===========================================
Files 225 225
Lines 18303 17823 -480
===========================================
- Hits 7488 7331 -157
+ Misses 10088 9780 -308
+ Partials 727 712 -15
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (3)
rollup/internal/orm/batch.go (1)
315-321:⚠️ Potential issue
WithContextresult is ignored – DB operations lose request context
db.WithContext(ctx)returns a new *gorm.DB instance, but the result isn’t reassigned. All subsequent calls therefore run without the supplied context, defeating cancellation / timeout propagation and tracing.- db.WithContext(ctx) - db = db.Model(&Batch{}) + db = db.WithContext(ctx).Model(&Batch{})Apply the same pattern anywhere else this anti-pattern appears.
rollup/internal/controller/relayer/l2_relayer.go (1)
205-208: 🛠️ Refactor suggestionLong-lived DB transaction can starve the connection pool
The transaction opened in
initializeGenesisremains open while you:
- Send the L1 transaction, and
- Wait (potentially up to 5 minutes) for confirmation.
Holding the write TX that long can block all other writers and even hit
LOCK_WAIT_TIMEOUT/connection-max-lifetime limits.A safer pattern is:
- Insert chunk/batch with status pending, commit immediately.
- Send L1 tx outside any transaction.
- On confirmation, open a new TX to flip the status to verified/finalized or roll back if the tx fails.
This still allows full recovery because commitSender already tracks the tx-hash context.
rollup/internal/controller/watcher/batch_proposer_test.go (1)
71-73:⚠️ Potential issueUndefined identifiers
block1/block2break compilation
block1andblock2are referenced but never declared, causing a compilation error.- l2BlockOrm := orm.NewL2Block(db) - err = l2BlockOrm.InsertL2Blocks(context.Background(), []*encoding.Block{block1, block2}) + l2BlockOrm := orm.NewL2Block(db) + + // prepare two test blocks (numbers 1 & 2) re-using the JSON fixture + block1 := readBlockFromJSON(t, "../../../testdata/blockTrace_03.json") + block1.Header.Number = big.NewInt(1) + block1.Header.Time = 1 + + block2 := readBlockFromJSON(t, "../../../testdata/blockTrace_03.json") + block2.Header.Number = big.NewInt(2) + block2.Header.Time = 2 + + err = l2BlockOrm.InsertL2Blocks(context.Background(), []*encoding.Block{block1, block2})Without these declarations the whole package fails to build.
🧹 Nitpick comments (10)
rollup/internal/controller/watcher/bundle_proposer_test.go (1)
123-131: Genesis batch is still inserted with CodecV0 – double-check intentAll new logic/tests target
encoding.CodecV7, yet the genesis chunk and batch are inserted withCodecV0(lines 77–87). If the reader modules no longer accept legacy codecs this may fail at runtime when those rows are re-loaded.If genesis really has to stay V0, consider adding an explanatory comment so that future maintainers do not “fix” it back to V7.
rollup/internal/controller/relayer/l2_relayer_test.go (1)
60-63: AlwaysdeferpatchGuard.Reset() to avoid leakageIf the test fails or panics before reaching line 92, the monkey-patch on
SendTransactionwill bleed into subsequent tests. Place the reset in adeferright after patch creation:patchGuard := gomonkey.ApplyMethodFunc(l2Cli, "SendTransaction", func(_ context.Context, _ *gethTypes.Transaction) error { return nil }) -defer patchGuard.Reset() +defer patchGuard.Reset()(This is merely a move; behaviour stays the same but becomes panic-safe.)
rollup/internal/controller/watcher/chunk_proposer_test.go (3)
95-99: Run sub-tests in parallel to shorten overall suite execution time
t.Runblocks are already isolated (new DB per sub-test).
Addingt.Parallel()as the first line inside each sub-test allows them to execute concurrently and typically cuts several seconds off CI time without side–effects.t.Run(tt.name, func(t *testing.T) { + t.Parallel()
104-108: Avoid inserting the genesis chunk repeatedly in every testThe helper inserts a genesis chunk for every sub-test, recreating identical data and increasing I/O.
Consider moving the genesis-insertion intosetupDBonce, or gate it with a quick existence check.This will speed up the test set and reduce the chance of “duplicate primary-key” flakiness when tests are executed in parallel.
140-147: Instantiate the ORM once outside the loop
l2BlockOrm := orm.NewL2Block(db)is executed 510 times, yet the value is identical.
Creating it once before the loop removes 509 heap allocations and DB handle look-ups:- for i := uint64(0); i < 510; i++ { - l2BlockOrm := orm.NewL2Block(db) + l2BlockOrm := orm.NewL2Block(db) + for i := uint64(0); i < 510; i++ {rollup/internal/controller/relayer/l2_relayer.go (1)
869-880: Remove leftover debugfmt.Println
fmt.Printlnin production code spams stdout (and can break JSON-structured logging).
Use the project logger or delete the statement:- fmt.Println("packing finalizeBundlePostEuclidV2NoProof", len(dbBatch.BatchHeader), dbBatch.CodecVersion, ...) + log.Debug("packing finalizeBundlePostEuclidV2NoProof", + "batchHeaderLen", len(dbBatch.BatchHeader), + "codecVersion", dbBatch.CodecVersion, + "poppedMsgs", + endChunk.TotalL1MessagesPoppedBefore+endChunk.TotalL1MessagesPoppedInChunk)rollup/internal/controller/watcher/batch_proposer_test.go (3)
155-156: Test helper duplication – consider extracting a sharedchainConfig()The same verbose
¶ms.ChainConfig{LondonBlock: … EuclidV2Time: new(uint64)}literal is repeated in every test case.
Creating a tiny helper (or re-using a fixture) will reduce duplication and the risk of future drift.
165-178: Stress-style loop inserts ~2 700 blocks – slows the unit test suiteThe nested
for total < 90 { for i < 30 { … } cp.TryProposeChunk() }loop inserts thousands of rows into an in-memory DB.
Unless the high volume is strictly required to hit a code path, trimming the numbers (e.g. 10×10) will speed test execution noticeably.
229-238: Repeated setup code – factor into a helper to keep tests conciseGenesis batch/chunk insertion logic is duplicated in every test.
Afunc insertGenesis(t *testing.T, db *gorm.DB)helper would keep each scenario focused on its unique behaviour.rollup/internal/controller/watcher/batch_proposer.go (1)
64-75: NilRegistereraccepted – add a short comment to document the intent
NewBatchProposerallowsregto benil, relying onpromauto.With(nil)falling back to the default registerer.
Future readers might interpret this as a bug. A one-line comment clarifying thatnilis intentionally supported would avert confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
common/version/version.go(1 hunks)rollup/abi/bridge_abi_test.go(0 hunks)rollup/cmd/proposer_tool/app/app.go(1 hunks)rollup/cmd/rollup_relayer/app/app.go(1 hunks)rollup/conf/config.json(0 hunks)rollup/docker-compose-proposer-tool.yml(1 hunks)rollup/internal/config/l2.go(1 hunks)rollup/internal/config/relayer.go(0 hunks)rollup/internal/controller/relayer/common.go(0 hunks)rollup/internal/controller/relayer/l2_relayer.go(1 hunks)rollup/internal/controller/relayer/l2_relayer_test.go(4 hunks)rollup/internal/controller/watcher/batch_proposer.go(5 hunks)rollup/internal/controller/watcher/batch_proposer_test.go(4 hunks)rollup/internal/controller/watcher/bundle_proposer.go(0 hunks)rollup/internal/controller/watcher/bundle_proposer_test.go(3 hunks)rollup/internal/controller/watcher/chunk_proposer.go(4 hunks)rollup/internal/controller/watcher/chunk_proposer_test.go(3 hunks)rollup/internal/controller/watcher/l2_watcher.go(1 hunks)rollup/internal/controller/watcher/l2_watcher_metrics.go(1 hunks)rollup/internal/controller/watcher/watcher_test.go(1 hunks)rollup/internal/orm/batch.go(1 hunks)rollup/internal/orm/chunk.go(1 hunks)rollup/internal/orm/l2_block.go(0 hunks)rollup/internal/orm/orm_test.go(1 hunks)rollup/internal/utils/utils.go(2 hunks)rollup/proposer-tool-config.json(0 hunks)rollup/tests/bridge_test.go(0 hunks)rollup/tests/gas_oracle_test.go(2 hunks)rollup/tests/process_start_test.go(2 hunks)rollup/tests/rollup_test.go(1 hunks)
💤 Files with no reviewable changes (8)
- rollup/proposer-tool-config.json
- rollup/internal/config/relayer.go
- rollup/tests/bridge_test.go
- rollup/internal/controller/relayer/common.go
- rollup/internal/controller/watcher/bundle_proposer.go
- rollup/internal/orm/l2_block.go
- rollup/conf/config.json
- rollup/abi/bridge_abi_test.go
🧰 Additional context used
🧠 Learnings (1)
rollup/internal/controller/watcher/batch_proposer.go (1)
Learnt from: colinlyguo
PR: scroll-tech/scroll#1530
File: rollup/internal/controller/watcher/batch_proposer.go:291-294
Timestamp: 2024-10-20T16:13:20.397Z
Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.
🧬 Code Graph Analysis (3)
rollup/internal/orm/batch.go (2)
coordinator/internal/types/auth.go (1)
Hash(134-141)common/types/db.go (6)
ChunkProofsStatus(175-175)ChunkProofsStatusPending(182-182)ProvingStatus(140-140)ProvingTaskUnassigned(146-146)RollupStatus(225-225)RollupPending(231-231)
rollup/internal/controller/watcher/bundle_proposer_test.go (3)
rollup/internal/controller/watcher/chunk_proposer.go (1)
NewChunkProposer(59-141)rollup/internal/config/l2.go (2)
ChunkProposerConfig(30-37)BatchProposerConfig(40-45)rollup/internal/controller/watcher/batch_proposer.go (1)
NewBatchProposer(58-128)
rollup/internal/controller/watcher/batch_proposer_test.go (6)
rollup/internal/controller/watcher/batch_proposer.go (1)
NewBatchProposer(58-128)rollup/internal/config/l2.go (2)
BatchProposerConfig(40-45)ChunkProposerConfig(30-37)rollup/internal/orm/l2_block.go (1)
NewL2Block(43-45)common/database/db.go (1)
CloseDB(83-92)rollup/internal/orm/chunk.go (3)
Chunk(20-64)Chunk(72-74)NewChunk(67-69)rollup/internal/orm/batch.go (3)
Batch(22-72)Batch(80-82)NewBatch(75-77)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (25)
common/version/version.go (1)
8-8: Version bump applied correctly
Thetagvariable has been updated from"v4.5.8"to"v4.5.9"to reflect the removal of legacy codec support and align with the new release baseline.rollup/cmd/proposer_tool/app/app.go (1)
57-59: Codec version validation added successfully.This check ensures that only CodecV7 and later versions are used, which aligns with the PR objective to simplify the rollup relayer by removing support for legacy codec versions.
rollup/internal/orm/orm_test.go (1)
74-74: Removed RowConsumption tracking from test blocks.These changes align with the broader removal of row consumption tracking in the codebase, ensuring test data is consistent with the simplified data structures used in production code.
Also applies to: 81-81
rollup/tests/process_start_test.go (2)
22-22: Updated minimum codec version from 4 to 7.This change ensures test consistency with the PR's objective to use only CodecV7+ throughout the codebase.
39-39: Updated minimum codec version from 4 to 7 in metrics test.This change ensures codec version consistency when testing with metrics enabled, aligning with the broader changes to use only CodecV7+.
rollup/cmd/rollup_relayer/app/app.go (1)
105-107: Added validation for minimum codec version.This critical check ensures only CodecV7 and later versions are used in the rollup relayer, which is consistent with the PR objective of removing support for legacy codec versions.
rollup/internal/orm/batch.go (1)
291-313: Struct field alignment refactor is safeThe literal now follows a cleaner, one-field-per-line style while preserving all assignments. No functional impact detected.
rollup/docker-compose-proposer-tool.yml (1)
30-32: Codec minimum bumped to 7 – double-check cluster consistencyThe flag aligns the container with the codebase’s removal of legacy codecs.
Please ensure every component (relayer, proposer-tool configs, DB migrations) is already running ≥ CodecV7, otherwise start-up will fail.No changes requested.
rollup/internal/controller/watcher/l2_watcher.go (1)
108-112: Removal ofRowConsumptionfield reflected correctlyBlock construction now omits the obsolete
RowConsumption. This matches the cleaned-up schema and compiles without issues.rollup/internal/controller/watcher/l2_watcher_metrics.go (2)
10-16: Metrics simplification is appropriateThe removal of calldata size and row consumption metrics aligns with the broader simplification of the rollup-relayer component by eliminating tracking of legacy resource constraints.
23-45: Clean removal of initialization codeThe initialization function has been updated to match the simplified metrics structure, removing initialization of the deprecated metrics.
rollup/internal/controller/watcher/watcher_test.go (3)
104-105: Well-structured update to CodecV7 for chunk proposer testsThe test cases have been appropriately updated to test only CodecV7 functionality, aligning with the removal of legacy codec support.
108-110: Complete coverage of batch proposer tests for CodecV7All batch proposer test cases have been consistently updated to use CodecV7, ensuring comprehensive test coverage of the current implementation.
113-113: Bundle proposer test updated for CodecV7The bundle proposer test has been updated to match the CodecV7 standardization across the codebase.
rollup/internal/orm/chunk.go (2)
53-54: Appropriate field deprecation for CrcMaxThe field has been correctly marked as deprecated rather than removed, maintaining database schema compatibility while indicating it's no longer used.
59-60: Appropriate field deprecation for TotalL1CommitCalldataSizeThe field has been correctly marked as deprecated rather than removed, maintaining database schema compatibility while indicating it's no longer used.
rollup/internal/utils/utils.go (4)
12-23: Streamlined ChunkMetrics structThe ChunkMetrics struct has been simplified by removing fields related to L1 commit gas, CRC max, and other resource limits that are no longer needed with the exclusive use of CodecV7.
55-65: Streamlined BatchMetrics structThe BatchMetrics struct has been similarly simplified by removing fields that are no longer relevant, making the code more focused and easier to maintain.
26-53: Simplified metric calculation logicThe CalculateChunkMetrics function has been updated to align with the simplified metrics structure, removing calculations for deprecated metrics while maintaining core functionality.
68-88: Consistent simplification of batch metrics calculationThe CalculateBatchMetrics function has been updated in a manner consistent with the changes to the CalculateChunkMetrics function, ensuring a coherent approach across the codebase.
rollup/internal/config/l2.go (1)
41-45: Use a consistent unsigned type for all “max-*” limits
MaxChunksPerBatchis declared as anint, whereas every other upper-bound field in the surrounding structs usesuint64.
Mixing signed/unsigned types invites hidden overflows and makes JSON → Go marshalling a little harder (large positive JSON numbers silently become negative when decoded intointon 32-bit platforms).-type BatchProposerConfig struct { - ProposeIntervalMilliseconds uint64 `json:"propose_interval_milliseconds"` - BatchTimeoutSec uint64 `json:"batch_timeout_sec"` - MaxUncompressedBatchBytesSize uint64 `json:"max_uncompressed_batch_bytes_size"` - MaxChunksPerBatch int `json:"max_chunks_per_batch"` +type BatchProposerConfig struct { + ProposeIntervalMilliseconds uint64 `json:"propose_interval_milliseconds"` + BatchTimeoutSec uint64 `json:"batch_timeout_sec"` + MaxUncompressedBatchBytesSize uint64 `json:"max_uncompressed_batch_bytes_size"` + MaxChunksPerBatch uint64 `json:"max_chunks_per_batch"` }Unless there is a strong reason to keep a signed type, aligning it with the other limit fields improves API ergonomics and future-proofs the config structure.
rollup/tests/rollup_test.go (1)
120-126: UnboundedMaxL2GasPerChunkdefaults to 0 – is that intentional?
ChunkProposerConfigomitsMaxL2GasPerChunk. Because the struct field’s zero value is 0, the proposer may treat this as no gas can ever fit unless the implementation interprets 0 as “unlimited”.
For clarity and future safety set it explicitly, e.g.:- MaxBlockNumPerChunk: 100, + MaxBlockNumPerChunk: 100, + MaxL2GasPerChunk: math.MaxUint64,Please confirm the proposer behaves as expected with a zero value.
rollup/internal/controller/relayer/l2_relayer.go (1)
161-174: Populate mandatory Chunk header fields when creating the genesis chunk
encoding.Chunkis created with onlyBlockspopulated.
If later code or DB constraints rely onStartBlockNumber,EndBlockNumber,StartBlockTime, … they will be zero, which can break range queries and metrics.Make sure
utils.CalculateChunkMetrics(called when inserting chunks elsewhere) is not required for genesis; otherwise initialise the fields explicitly:chunk := &encoding.Chunk{ Blocks: []*encoding.Block{{Header: genesis}}, + StartBlockNumber: genesis.Number.Uint64(), + EndBlockNumber: genesis.Number.Uint64(), + StartBlockTime: genesis.Time, + EndBlockTime: genesis.Time, }rollup/internal/controller/watcher/chunk_proposer.go (1)
302-312: Undefined or package-levelmaxBlobSizeconstant may shadow imports
maxBlobSizeis referenced but not defined in this file segment.
If it is a package-level var elsewhere, consider:
- Adding an explicit
const maxBlobSize = …in this file for clarity, or- Qualifying it (
encoding.MaxBlobSize) if defined in another package.This prevents accidental shadowing and makes the origin obvious.
rollup/internal/controller/watcher/batch_proposer.go (1)
301-328: Metric calculation inside the chunk loop is potentially expensive
utils.CalculateBatchMetricsis invoked for every appended chunk, which could be O(n²) as the batch grows.
If profiling ever shows this to be hot, caching the previous metrics and updating incrementally would cut work in half.
No change required now, but worth keeping in mind.
colinlyguo
force-pushed
the
refactor-simplify-rollup-relayer
branch
2 times, most recently
from
ad11390 to
937a4b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
rollup/internal/controller/watcher/batch_proposer_test.go (3)
23-125:⚠️ Potential issueRename test functions to be automatically discovered by Go testing framework
All test helper functions start with lowercase "t" (testBatchProposerLimitsCodecV7), which means they won't be automatically executed by Go's testing framework. Test functions must start with "Test" to be discovered.
-func testBatchProposerLimitsCodecV7(t *testing.T) { +func TestBatchProposerLimitsCodecV7(t *testing.T) {This is a duplicate of an issue identified in a previous review, but it still needs to be addressed for these test functions.
127-196:⚠️ Potential issueRename test function and update blob size limit test
The test has been significantly changed from testing gas and calldata size estimation to testing blob size limits, which better reflects the post-Euclid architecture. However, it still needs to be renamed with a "Test" prefix to be discovered by Go's testing framework.
-func testBatchProposerBlobSizeLimitCodecV7(t *testing.T) { +func TestBatchProposerBlobSizeLimitCodecV7(t *testing.T) {
198-255:⚠️ Potential issueRename the max chunk per batch limit test
This test function also needs to be renamed with a "Test" prefix to be automatically executed by Go's testing framework.
-func testBatchProposerMaxChunkNumPerBatchLimitCodecV7(t *testing.T) { +func TestBatchProposerMaxChunkNumPerBatchLimitCodecV7(t *testing.T) {
🧹 Nitpick comments (5)
rollup/tests/rollup_test.go (1)
112-115: Clean up struct formatting for consistencyThe struct formatting in this updated block uses a different style from the rest of the codebase. Consider maintaining consistent formatting across the struct initialization.
-blocks = append(blocks, &encoding.Block{ - Header: &header, - Transactions: transactions, - WithdrawRoot: common.HexToHash("0x2"), - }) +blocks = append(blocks, &encoding.Block{ + Header: &header, + Transactions: transactions, + WithdrawRoot: common.HexToHash("0x2"), +})rollup/internal/controller/relayer/l2_relayer.go (1)
869-869: Remove debug print statementThere's a debug
fmt.Printlnstatement that was likely used during development and should be removed or replaced with a proper log statement.-fmt.Println("packing finalizeBundlePostEuclidV2NoProof", len(dbBatch.BatchHeader), dbBatch.CodecVersion, dbBatch.BatchHeader, new(big.Int).SetUint64(endChunk.TotalL1MessagesPoppedBefore+endChunk.TotalL1MessagesPoppedInChunk), common.HexToHash(dbBatch.StateRoot), common.HexToHash(dbBatch.WithdrawRoot)) +log.Debug("Packing finalizeBundlePostEuclidV2NoProof", + "headerLen", len(dbBatch.BatchHeader), + "codecVersion", dbBatch.CodecVersion, + "totalL1MessagesPopped", endChunk.TotalL1MessagesPoppedBefore+endChunk.TotalL1MessagesPoppedInChunk, + "stateRoot", common.HexToHash(dbBatch.StateRoot), + "withdrawRoot", common.HexToHash(dbBatch.WithdrawRoot))rollup/internal/controller/watcher/batch_proposer_test.go (3)
164-174: Consider refactoring test block creation loopThe nested loops create 90 x 30 = 2700 blocks, which might be excessive for a unit test. Consider extracting the block creation logic to a helper function and reducing the number of iterations to improve test execution time.
-for total := int64(0); total < 90; total++ { - for i := int64(0); i < 30; i++ { - blockHeight++ - l2BlockOrm := orm.NewL2Block(db) - block.Header.Number = new(big.Int).SetUint64(blockHeight) - block.Header.Time = blockHeight - err = l2BlockOrm.InsertL2Blocks(context.Background(), []*encoding.Block{block}) - assert.NoError(t, err) - } - cp.TryProposeChunk() -} +// Helper function to create multiple blocks and chunks +func createBlocksAndChunks(t *testing.T, blockCount, chunksCount int, db *gorm.DB, cp *ChunkProposer) uint64 { + block := readBlockFromJSON(t, "../../../testdata/blockTrace_03.json") + var blockHeight uint64 = 0 + l2BlockOrm := orm.NewL2Block(db) + + for c := 0; c < chunksCount; c++ { + var blocks []*encoding.Block + for i := 0; i < blockCount; i++ { + blockHeight++ + blockCopy := *block + blockCopy.Header = &gethTypes.Header{} + *blockCopy.Header = *block.Header + blockCopy.Header.Number = new(big.Int).SetUint64(blockHeight) + blockCopy.Header.Time = blockHeight + blocks = append(blocks, &blockCopy) + } + err := l2BlockOrm.InsertL2Blocks(context.Background(), blocks) + assert.NoError(t, err) + cp.TryProposeChunk() + } + return blockHeight +} + +// Use the helper function +blockHeight = createBlocksAndChunks(t, 30, 90, db, cp)
224-225: Document the significance of the expected chunk numberThe expected chunk number (45) is set without any explanation. Add a comment explaining why this specific value is chosen to make the test more maintainable.
-var expectedChunkNum uint64 = 45 +// expectedChunkNum is set to match MaxChunksPerBatch in the batch proposer config below +var expectedChunkNum uint64 = 45
243-246: Document test configuration parametersThe batch proposer is configured with specific values that are critical to the test but lack explanation. Add comments to explain why these values were chosen.
bp := NewBatchProposer(context.Background(), &config.BatchProposerConfig{ - MaxChunksPerBatch: 45, - BatchTimeoutSec: math.MaxUint32, + // Set to equal the expectedChunkNum to test that batches are limited by this parameter + MaxChunksPerBatch: 45, + // Use a high timeout to ensure batches are created by chunk limit, not by timeout + BatchTimeoutSec: math.MaxUint32, }, encoding.CodecV7, chainConfig, db, nil)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rollup/go.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
common/version/version.go(1 hunks)rollup/abi/bridge_abi_test.go(0 hunks)rollup/cmd/proposer_tool/app/app.go(1 hunks)rollup/cmd/rollup_relayer/app/app.go(1 hunks)rollup/conf/config.json(1 hunks)rollup/docker-compose-proposer-tool.yml(1 hunks)rollup/go.mod(1 hunks)rollup/internal/config/l2.go(1 hunks)rollup/internal/config/relayer.go(0 hunks)rollup/internal/controller/relayer/common.go(0 hunks)rollup/internal/controller/relayer/l2_relayer.go(1 hunks)rollup/internal/controller/relayer/l2_relayer_test.go(4 hunks)rollup/internal/controller/watcher/batch_proposer.go(4 hunks)rollup/internal/controller/watcher/batch_proposer_test.go(4 hunks)rollup/internal/controller/watcher/bundle_proposer.go(0 hunks)rollup/internal/controller/watcher/bundle_proposer_test.go(3 hunks)rollup/internal/controller/watcher/chunk_proposer.go(5 hunks)rollup/internal/controller/watcher/chunk_proposer_test.go(3 hunks)rollup/internal/controller/watcher/l2_watcher.go(1 hunks)rollup/internal/controller/watcher/l2_watcher_metrics.go(1 hunks)rollup/internal/controller/watcher/watcher_test.go(1 hunks)rollup/internal/orm/batch.go(1 hunks)rollup/internal/orm/chunk.go(1 hunks)rollup/internal/orm/l2_block.go(0 hunks)rollup/internal/orm/orm_test.go(1 hunks)rollup/internal/utils/utils.go(4 hunks)rollup/proposer-tool-config.json(1 hunks)rollup/tests/bridge_test.go(0 hunks)rollup/tests/gas_oracle_test.go(2 hunks)rollup/tests/process_start_test.go(2 hunks)rollup/tests/rollup_test.go(1 hunks)
💤 Files with no reviewable changes (6)
- rollup/internal/config/relayer.go
- rollup/internal/controller/relayer/common.go
- rollup/internal/orm/l2_block.go
- rollup/tests/bridge_test.go
- rollup/internal/controller/watcher/bundle_proposer.go
- rollup/abi/bridge_abi_test.go
✅ Files skipped from review due to trivial changes (3)
- rollup/go.mod
- rollup/docker-compose-proposer-tool.yml
- rollup/proposer-tool-config.json
🚧 Files skipped from review as they are similar to previous changes (19)
- rollup/cmd/proposer_tool/app/app.go
- rollup/internal/orm/batch.go
- rollup/internal/controller/watcher/l2_watcher.go
- common/version/version.go
- rollup/internal/orm/orm_test.go
- rollup/tests/process_start_test.go
- rollup/conf/config.json
- rollup/cmd/rollup_relayer/app/app.go
- rollup/internal/orm/chunk.go
- rollup/tests/gas_oracle_test.go
- rollup/internal/controller/watcher/l2_watcher_metrics.go
- rollup/internal/controller/watcher/watcher_test.go
- rollup/internal/utils/utils.go
- rollup/internal/controller/watcher/bundle_proposer_test.go
- rollup/internal/controller/watcher/chunk_proposer_test.go
- rollup/internal/controller/relayer/l2_relayer_test.go
- rollup/internal/config/l2.go
- rollup/internal/controller/watcher/batch_proposer.go
- rollup/internal/controller/watcher/chunk_proposer.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
rollup/tests/rollup_test.go (3)
rollup/internal/controller/watcher/chunk_proposer.go (1)
NewChunkProposer(55-131)rollup/internal/config/l2.go (2)
ChunkProposerConfig(30-36)BatchProposerConfig(39-43)rollup/internal/controller/watcher/batch_proposer.go (1)
NewBatchProposer(57-125)
rollup/internal/controller/relayer/l2_relayer.go (1)
rollup/internal/orm/chunk.go (2)
Chunk(20-64)Chunk(72-74)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (7)
rollup/tests/rollup_test.go (2)
120-125: Configuration updated to match simplified post-Euclid requirementsThe chunk proposer configuration has been correctly updated to remove the legacy parameters (
MaxL1CommitGasPerChunk,MaxL1CommitCalldataSizePerChunk,MaxRowConsumptionPerChunk) and now usesMaxL2GasPerChunkwithmath.MaxUint64. This aligns with the broader refactoring to support only CodecV7.
127-130: Simplified batch proposer configurationThe batch proposer configuration has been streamlined to remove the legacy parameters (
MaxL1CommitGasPerBatch,MaxL1CommitCalldataSizePerBatch), keeping only the essentialMaxChunksPerBatchandBatchTimeoutSec. This change reflects the removal of resource limit parameters now unnecessary with CodecV7.rollup/internal/controller/relayer/l2_relayer.go (5)
161-162: Simplified genesis chunk initializationThe initialization of the genesis chunk has been simplified to directly use the encoding.Chunk struct with a blocks array containing only the genesis header. This change properly removes unused fields and simplifies the code.
362-373: Removed support for legacy codec versionsThe switch statement has been simplified to only support CodecV7, removing all legacy codec handling for V4, V5, and V6. This aligns with the PR objective of simplifying logic post-Euclid.
419-427: Simplified context ID generationThe contextID generation now assumes only CodecV7 batches are present, adding a "v7" prefix to all context IDs. This is consistent with the removal of legacy codec support.
429-435: Updated context ID parsing for CodecV7The batch hash extraction from contextID has been updated to handle the new "v7-" prefix format. Consider adding a test to verify this updated parsing logic works correctly with both old and new format context IDs.
Can you verify this function works correctly for both the new "v7-" prefixed context IDs and any potential legacy context IDs that might still exist in the database?
590-598: Codec version handling in finalize bundleThe finalize bundle payload construction now exclusively supports CodecV7. This is aligned with the PR objective but ensure all existing batches in production environments support this version before deploying.
Before deployment, confirm that no batches with legacy codec versions exist in the production database that would need to be finalized.
colinlyguo
force-pushed
the
refactor-simplify-rollup-relayer
branch
from
937a4b0 to
f6f91e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
rollup/internal/controller/watcher/batch_proposer_test.go (1)
23-23:⚠️ Potential issueThese test helpers won't run with Go's testing framework.
Go's testing framework only runs functions whose names start with
TestXxx. The helper functions (testBatchProposerLimitsCodecV7,testBatchProposerBlobSizeLimitCodecV7,testBatchProposerMaxChunkNumPerBatchLimitCodecV7) will compile but will never be executed bygo test.Rename these functions to follow Go's testing convention:
-func testBatchProposerLimitsCodecV7(t *testing.T) { +func TestBatchProposerLimitsCodecV7(t *testing.T) {Apply the same rename to all three helpers in this file.
rollup/internal/controller/watcher/chunk_proposer.go (2)
222-224:⚠️ Potential issuePotential issue with genesis handling.
The error check
if err != nil || parentChunk == nilwill fail when initializing a fresh database before genesis is added. This behavior was flagged in a previous review but not addressed.Instead of failing when no parent chunk exists, consider falling back to an empty hash for genesis initialization:
- if err != nil || parentChunk == nil { - return fmt.Errorf("failed to get parent chunk: %w", err) - } + if err != nil { + return fmt.Errorf("failed to get parent chunk: %w", err) + } + if parentChunk == nil { + // no previous chunk – treat as genesis + chunk.PrevL1MessageQueueHash = common.Hash{} + } else { + chunk.PrevL1MessageQueueHash = common.HexToHash(parentChunk.PostL1MessageQueueHash) + }
241-244: 🛠️ Refactor suggestionInefficient message queue hash calculation.
The message queue hash is calculated once per block, which is inefficient for large block numbers. This inefficiency was noted in a previous review but not addressed.
The current implementation has O(n²) complexity when processing thousands of blocks. Calculate the hash once after the loop instead:
- for i, block := range blocks { - ... - previousPostL1MessageQueueHash = chunk.PostL1MessageQueueHash - chunk.PostL1MessageQueueHash, err = encoding.MessageQueueV2ApplyL1MessagesFromBlocks(previousPostL1MessageQueueHash, []*encoding.Block{block}) - if err != nil { - return fmt.Errorf("failed to calculate last L1 message queue hash for block %d: %w", block.Header.Number.Uint64(), err) - } + for i, block := range blocks { + ... + // Collect blocks but don't calculate hash yet + } + // Calculate hash once with all blocks + chunk.PostL1MessageQueueHash, err = encoding.MessageQueueV2ApplyL1MessagesFromBlocks(chunk.PrevL1MessageQueueHash, chunk.Blocks) + if err != nil { + return fmt.Errorf("failed to calculate last L1 message queue hash: %w", err) + }
🧹 Nitpick comments (3)
rollup/internal/controller/watcher/batch_proposer_test.go (2)
153-153: Chain configuration declaration formatting could be improved.The chain configuration declaration is very long and could be formatted with line breaks for better readability.
- chainConfig := ¶ms.ChainConfig{LondonBlock: big.NewInt(0), BernoulliBlock: big.NewInt(0), CurieBlock: big.NewInt(0), DarwinTime: new(uint64), DarwinV2Time: new(uint64), EuclidTime: new(uint64), EuclidV2Time: new(uint64)} + chainConfig := ¶ms.ChainConfig{ + LondonBlock: big.NewInt(0), + BernoulliBlock: big.NewInt(0), + CurieBlock: big.NewInt(0), + DarwinTime: new(uint64), + DarwinV2Time: new(uint64), + EuclidTime: new(uint64), + EuclidV2Time: new(uint64), + }
189-191: Hardcoded batch count expectations could be more descriptive.The code uses magic numbers for expected batch counts and multipliers without explaining their significance.
- var expectedNumBatches int = 1 - var numChunksMultiplier uint64 = 64 + // Only expect one batch since we're testing blob size limits with CodecV7 + const expectedNumBatches = 1 + // With CodecV7, a batch can contain up to 64 chunks before hitting blob size limits + const numChunksMultiplier uint64 = 64rollup/internal/controller/relayer/l2_relayer.go (1)
869-869: Remove debug println statement.There's a leftover
fmt.Printlndebug statement that should be removed or replaced with a proper log statement.- fmt.Println("packing finalizeBundlePostEuclidV2NoProof", len(dbBatch.BatchHeader), dbBatch.CodecVersion, dbBatch.BatchHeader, new(big.Int).SetUint64(endChunk.TotalL1MessagesPoppedBefore+endChunk.TotalL1MessagesPoppedInChunk), common.HexToHash(dbBatch.StateRoot), common.HexToHash(dbBatch.WithdrawRoot)) + log.Debug("packing finalizeBundlePostEuclidV2NoProof", + "headerLength", len(dbBatch.BatchHeader), + "codecVersion", dbBatch.CodecVersion, + "totalL1MessagesPoppedBeforeAndInChunk", endChunk.TotalL1MessagesPoppedBefore+endChunk.TotalL1MessagesPoppedInChunk, + "stateRoot", common.HexToHash(dbBatch.StateRoot), + "withdrawRoot", common.HexToHash(dbBatch.WithdrawRoot))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rollup/go.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
common/version/version.go(1 hunks)rollup/abi/bridge_abi_test.go(0 hunks)rollup/cmd/proposer_tool/app/app.go(1 hunks)rollup/cmd/rollup_relayer/app/app.go(1 hunks)rollup/conf/config.json(1 hunks)rollup/docker-compose-proposer-tool.yml(1 hunks)rollup/go.mod(1 hunks)rollup/internal/config/l2.go(1 hunks)rollup/internal/config/relayer.go(0 hunks)rollup/internal/controller/relayer/common.go(0 hunks)rollup/internal/controller/relayer/l2_relayer.go(1 hunks)rollup/internal/controller/relayer/l2_relayer_test.go(4 hunks)rollup/internal/controller/watcher/batch_proposer.go(4 hunks)rollup/internal/controller/watcher/batch_proposer_test.go(4 hunks)rollup/internal/controller/watcher/bundle_proposer.go(0 hunks)rollup/internal/controller/watcher/bundle_proposer_test.go(3 hunks)rollup/internal/controller/watcher/chunk_proposer.go(5 hunks)rollup/internal/controller/watcher/chunk_proposer_test.go(3 hunks)rollup/internal/controller/watcher/l2_watcher.go(1 hunks)rollup/internal/controller/watcher/l2_watcher_metrics.go(1 hunks)rollup/internal/controller/watcher/watcher_test.go(1 hunks)rollup/internal/orm/batch.go(1 hunks)rollup/internal/orm/chunk.go(1 hunks)rollup/internal/orm/l2_block.go(0 hunks)rollup/internal/orm/orm_test.go(1 hunks)rollup/internal/utils/utils.go(4 hunks)rollup/proposer-tool-config.json(1 hunks)rollup/tests/bridge_test.go(0 hunks)rollup/tests/gas_oracle_test.go(2 hunks)rollup/tests/process_start_test.go(2 hunks)rollup/tests/rollup_test.go(1 hunks)
💤 Files with no reviewable changes (6)
- rollup/internal/controller/relayer/common.go
- rollup/internal/config/relayer.go
- rollup/internal/controller/watcher/bundle_proposer.go
- rollup/tests/bridge_test.go
- rollup/internal/orm/l2_block.go
- rollup/abi/bridge_abi_test.go
✅ Files skipped from review due to trivial changes (3)
- rollup/docker-compose-proposer-tool.yml
- rollup/tests/process_start_test.go
- rollup/conf/config.json
🚧 Files skipped from review as they are similar to previous changes (19)
- common/version/version.go
- rollup/cmd/proposer_tool/app/app.go
- rollup/internal/orm/batch.go
- rollup/internal/orm/orm_test.go
- rollup/tests/gas_oracle_test.go
- rollup/go.mod
- rollup/cmd/rollup_relayer/app/app.go
- rollup/internal/controller/watcher/l2_watcher.go
- rollup/proposer-tool-config.json
- rollup/internal/controller/watcher/l2_watcher_metrics.go
- rollup/internal/orm/chunk.go
- rollup/internal/controller/watcher/bundle_proposer_test.go
- rollup/tests/rollup_test.go
- rollup/internal/controller/watcher/chunk_proposer_test.go
- rollup/internal/controller/relayer/l2_relayer_test.go
- rollup/internal/controller/watcher/watcher_test.go
- rollup/internal/config/l2.go
- rollup/internal/utils/utils.go
- rollup/internal/controller/watcher/batch_proposer.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
rollup/internal/controller/relayer/l2_relayer.go (1)
rollup/internal/orm/chunk.go (2)
Chunk(20-64)Chunk(72-74)
rollup/internal/controller/watcher/batch_proposer_test.go (7)
rollup/internal/controller/watcher/batch_proposer.go (1)
NewBatchProposer(57-125)rollup/internal/config/l2.go (2)
BatchProposerConfig(39-43)ChunkProposerConfig(30-36)rollup/internal/controller/watcher/chunk_proposer.go (1)
NewChunkProposer(55-131)rollup/internal/orm/l2_block.go (1)
NewL2Block(43-45)common/database/db.go (1)
CloseDB(83-92)rollup/internal/orm/chunk.go (3)
Chunk(20-64)Chunk(72-74)NewChunk(67-69)rollup/internal/orm/batch.go (3)
Batch(22-72)Batch(80-82)NewBatch(75-77)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (11)
rollup/internal/controller/watcher/batch_proposer_test.go (3)
91-94: LGTM: BatchProposer configuration simplified.Configuration correctly focuses on the essential batch limits (max chunks per batch and timeout), removing deprecated L1 gas and calldata size parameters.
85-86: New Euclid parameters correctly added to chain config.The test properly includes the new parameters for Euclid in the chain configuration.
74-79:❌ Incorrect review comment
ChunkProposer config uses deprecated parameters.
The ChunkProposer config includes
MaxL2GasPerChunk(line 77) which is still used in line 254 of chunk_proposer.go for validation, but other gas/size limits have been removed from the implementation.
🏁 Script executed:
#!/bin/bash # Check if MaxL2GasPerChunk is used anywhere else rg "MaxL2GasPerChunk" --type goLength of output: 1635
MaxL2GasPerChunk Remains Required
TheMaxL2GasPerChunkfield is still actively used throughout the codebase, so it isn’t deprecated yet:
- Validation in
rollup/internal/controller/watcher/chunk_proposer.go(around line 254)- CLI checks in
rollup/cmd/rollup_relayer/app/app.gorollup/cmd/proposer_tool/app/app.go- Tests in
rollup/internal/controller/watcher/chunk_proposer_test.gorollup/internal/controller/watcher/bundle_proposer_test.gorollup/internal/controller/watcher/batch_proposer_test.goIf you do intend to deprecate and remove this parameter, please update the implementation and remove all references (including tests). Otherwise, you can disregard the deprecation warning—the current tests and code are correct.
Likely an incorrect or invalid review comment.
rollup/internal/controller/relayer/l2_relayer.go (3)
161-161: Chunk initialization simplified correctly.The chunk initialization in
initializeGenesishas been properly simplified for clarity as part of the codec version consolidation.
353-373: CodecV7-only batch processing implemented correctly.The relayer's batch processing now exclusively supports CodecV7, removing all legacy codec version support (V4-V6). This makes the code more maintainable and aligns with the PR's objective to simplify rollup relayer logic post-Euclid.
590-598: CodecV7-only bundle finalization implemented correctly.The bundle finalization switch case now exclusively supports CodecV7, which aligns with the removal of legacy codec version support across the codebase.
rollup/internal/controller/watcher/chunk_proposer.go (5)
63-75: ChunkProposer structure simplified correctly.Resource limit parameters and related fields have been properly removed from the ChunkProposer initialization, keeping only the essential parameters: maxBlockNumPerChunk, maxTxNumPerChunk, maxL2GasPerChunk, and chunkTimeoutSec.
151-158:updateDBChunkInfomethod simplified correctly.The method has been correctly simplified to remove handling for codec versions and related metric recalculations.
253-269: Validation logic simplified correctly.The chunk validation logic has been appropriately simplified to remove checks against legacy parameters like L1CommitGas and CalldataSize, focusing only on transaction count, L2 gas, and blob size limits.
307-313: Metrics recording updated correctly.The metrics recording has been correctly updated to reflect the removed gas and calldata size metrics, focusing on the remaining relevant metrics for CodecV7.
61-61:❌ Incorrect review comment
Mention of
maxBlobSizewithout declaration.The
maxBlobSizevariable is used here and in several places throughout the file, but its declaration is not visible. This appears to be a global constant that should be properly declared.
🏁 Script executed:
#!/bin/bash # Find the declaration of maxBlobSize rg "maxBlobSize\s*=[^=]" --type goLength of output: 116
No action needed:
maxBlobSizeis properly declared
The constantmaxBlobSizeis defined inrollup/internal/controller/watcher/common.go:
const maxBlobSize = uint64(131072)You can safely ignore the original comment.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
rollup/internal/controller/watcher/batch_proposer_test.go (3)
23-23:⚠️ Potential issueFunction naming convention: should be capitalized to make it an exported test
This test function uses a lowercase prefix "test" which means it won't be discovered and run by Go's testing package. Test functions must start with "Test" to be executed.
-func testBatchProposerLimitsCodecV7(t *testing.T) { +func TestBatchProposerLimitsCodecV7(t *testing.T) {
126-126:⚠️ Potential issueFunction naming convention: should be capitalized to make it an exported test
This test function also uses a lowercase prefix "test" which means it won't be discovered and run by Go's testing package.
-func testBatchProposerBlobSizeLimitCodecV7(t *testing.T) { +func TestBatchProposerBlobSizeLimitCodecV7(t *testing.T) {
196-196:⚠️ Potential issueFunction naming convention: should be capitalized to make it an exported test
This test function also uses a lowercase prefix "test" which means it won't be discovered and run by Go's testing package.
-func testBatchProposerMaxChunkNumPerBatchLimitCodecV7(t *testing.T) { +func TestBatchProposerMaxChunkNumPerBatchLimitCodecV7(t *testing.T) {
🧹 Nitpick comments (2)
rollup/internal/controller/watcher/batch_proposer_test.go (1)
187-189: Clarify the expected batch count logicThe logic for determining the expected number of batches is now hardcoded to 1. Consider making this more dynamic or adding a comment to explain why exactly 1 batch is expected in this test case.
- var expectedNumBatches int = 1 + // After removing blob size limits and increasing max chunks per batch, + // we expect only a single batch to be created for all chunks + const expectedNumBatches int = 1rollup/internal/controller/watcher/chunk_proposer.go (1)
232-236: Optimize message queue hash calculation for better performanceThe message queue hash is calculated once per block in a loop, which is inefficient for large chunks with many blocks (O(n²) complexity).
- for i, block := range blocks { - chunk.Blocks = append(chunk.Blocks, block) - - previousPostL1MessageQueueHash = chunk.PostL1MessageQueueHash - chunk.PostL1MessageQueueHash, err = encoding.MessageQueueV2ApplyL1MessagesFromBlocks(previousPostL1MessageQueueHash, []*encoding.Block{block}) - if err != nil { - return fmt.Errorf("failed to calculate last L1 message queue hash for block %d: %w", block.Header.Number.Uint64(), err) - } + for i, block := range blocks { + chunk.Blocks = append(chunk.Blocks, block) + + // Store current state for potential rollback + previousPostL1MessageQueueHash = chunk.PostL1MessageQueueHash + + // Check limits before proceeding with potentially expensive calculations + metrics, calcErr := utils.CalculateChunkMetrics(&chunk, codecVersion) + if calcErr != nil { + return fmt.Errorf("failed to calculate chunk metrics: %w", calcErr) + } + + if metrics.L2Gas > p.maxL2GasPerChunk || metrics.L1CommitBlobSize > maxBlobSize { + if i == 0 { + // The first block exceeds hard limits + return fmt.Errorf("the first block exceeds limits; block number: %v, limits: %+v, maxBlobSize: %v", block.Header.Number, metrics, maxBlobSize) + } + + // Roll back the last block + chunk.Blocks = chunk.Blocks[:len(chunk.Blocks)-1] + // Calculate queue hash once for all included blocks + chunk.PostL1MessageQueueHash, err = encoding.MessageQueueV2ApplyL1MessagesFromBlocks(chunk.PrevL1MessageQueueHash, chunk.Blocks) + if err != nil { + return fmt.Errorf("failed to calculate L1 message queue hash: %w", err) + } + + metrics, calcErr := utils.CalculateChunkMetrics(&chunk, codecVersion) + if calcErr != nil { + return fmt.Errorf("failed to calculate chunk metrics: %w", calcErr) + } + + p.recordAllChunkMetrics(metrics) + return p.updateDBChunkInfo(&chunk, codecVersion, metrics) + } + } + + // Calculate final queue hash once for all blocks + chunk.PostL1MessageQueueHash, err = encoding.MessageQueueV2ApplyL1MessagesFromBlocks(chunk.PrevL1MessageQueueHash, chunk.Blocks) + if err != nil { + return fmt.Errorf("failed to calculate L1 message queue hash: %w", err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
rollup/conf/config.json(1 hunks)rollup/internal/config/l2.go(1 hunks)rollup/internal/controller/watcher/batch_proposer_test.go(4 hunks)rollup/internal/controller/watcher/bundle_proposer_test.go(3 hunks)rollup/internal/controller/watcher/chunk_proposer.go(5 hunks)rollup/internal/controller/watcher/chunk_proposer_test.go(3 hunks)rollup/internal/orm/chunk.go(1 hunks)rollup/internal/utils/utils.go(4 hunks)rollup/proposer-tool-config.json(1 hunks)rollup/tests/rollup_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- rollup/proposer-tool-config.json
- rollup/conf/config.json
- rollup/internal/controller/watcher/chunk_proposer_test.go
- rollup/internal/orm/chunk.go
- rollup/internal/config/l2.go
- rollup/tests/rollup_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
rollup/internal/controller/watcher/batch_proposer_test.go (7)
rollup/internal/controller/watcher/batch_proposer.go (1)
NewBatchProposer(57-125)rollup/internal/config/l2.go (2)
BatchProposerConfig(38-42)ChunkProposerConfig(30-35)rollup/internal/controller/watcher/chunk_proposer.go (1)
NewChunkProposer(53-123)rollup/internal/orm/l2_block.go (1)
NewL2Block(43-45)common/database/db.go (1)
CloseDB(83-92)rollup/internal/orm/chunk.go (3)
Chunk(20-64)Chunk(72-74)NewChunk(67-69)rollup/internal/orm/batch.go (3)
Batch(22-72)Batch(80-82)NewBatch(75-77)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (16)
rollup/internal/controller/watcher/bundle_proposer_test.go (4)
93-93: LGTM: Chain configuration updated with new fork time fieldsThe chain configuration has been correctly updated to include the new
EuclidTimeandEuclidV2Timefields, which is necessary for proper hardfork detection.
95-99: LGTM: Simplified ChunkProposerConfig with only essential parametersThe configuration has been appropriately simplified to include only the necessary parameters for the CodecV7 implementation. This is aligned with the PR objective of simplifying the logic post-Euclid.
101-104: LGTM: Simplified BatchProposerConfig with appropriate settingsThe batch proposer configuration has been streamlined to only include essential parameters for CodecV7. Setting
MaxChunksPerBatchtomath.MaxInt32effectively removes the chunk count limitation, which aligns with the simplification goals.
114-114: LGTM: Updated codec version to V7Consistently using codec V7 throughout the test is in line with the PR objectives of removing support for legacy codec versions.
rollup/internal/utils/utils.go (4)
13-18: LGTM: Simplified ChunkMetrics struct with only essential fieldsThe
ChunkMetricsstruct has been appropriately simplified to include only the essentialL1CommitBlobSizefield, removing deprecated metrics that are no longer relevant in CodecV7. This is a good cleanup that reduces complexity.
41-43: LGTM: Simplified metric collection logicThe metrics collection has been streamlined to focus only on blob size metrics, which is appropriate for CodecV7. This simplification improves code maintainability.
53-57: LGTM: Simplified BatchMetrics struct with only essential fieldsSimilar to the
ChunkMetricsstruct, theBatchMetricsstruct has been appropriately simplified to focus only on essential metrics related to blob size. This is a good cleanup that reduces complexity.
75-77: LGTM: Streamlined batch metrics calculationThe batch metrics calculation has been simplified to focus only on blob size, removing unnecessary metrics that are no longer relevant in CodecV7. This is a positive change that aligns with the overall simplification goals.
rollup/internal/controller/watcher/batch_proposer_test.go (3)
74-78: LGTM: Simplified ChunkProposerConfig with updated parametersThe configuration has been appropriately simplified to include only the necessary parameters for the CodecV7 implementation. This aligns with the PR objective of simplifying logic post-Euclid.
90-93: LGTM: Simplified BatchProposerConfig correctly configuredThe batch proposer configuration has been appropriately simplified to include only essential parameters. Using
math.MaxInt32forMaxChunksPerBatcheffectively removes the chunk count limitation, which is consistent with the simplified post-Euclid approach.
240-243: LGTM: BatchProposerConfig with explicit chunk limitThis test case explicitly sets a chunk limit (45) to test the max chunks per batch functionality, which is good for ensuring the limit is respected when provided. This provides valuable test coverage for the configuration parameter.
rollup/internal/controller/watcher/chunk_proposer.go (5)
29-32: LGTM: Simplified ChunkProposer struct with fewer parametersThe
ChunkProposerstruct has been simplified to include only the essential parameters needed for CodecV7, which aligns with the PR objective of simplifying the logic post-Euclid.
58-58: LGTM: Simplified logging with only essential parametersThe log statement has been updated to include only the parameters that are still relevant in the simplified implementation.
143-149: LGTM: Simplified updateDBChunkInfo methodThe chunk update method has been simplified to focus only on essential validation, removing complex logic for handling different codec versions and compression methods.
245-249: LGTM: Simplified validation logic focusing on essential limitsThe validation logic has been simplified to check only essential limits (L2Gas and blob size), which is appropriate for the CodecV7 implementation and aligns with the simplification goals.
213-216:❌ Incorrect review comment
Improve resilience when no parent chunk exists
The current implementation fails when no parent chunk exists, which could cause issues during initial setup or when bootstrapping a new database.
- parentChunk, err := p.chunkOrm.GetLatestChunk(p.ctx) - if err != nil || parentChunk == nil { - return fmt.Errorf("failed to get parent chunk: %w", err) - } + parentChunk, err := p.chunkOrm.GetLatestChunk(p.ctx) + if err != nil { + return fmt.Errorf("failed to get parent chunk: %w", err) + } + if parentChunk == nil { + // No previous chunk - treat as genesis + chunk.PrevL1MessageQueueHash = common.Hash{} + } else { + chunk.PrevL1MessageQueueHash = common.HexToHash(parentChunk.PostL1MessageQueueHash) + }Run this script to verify whether this system needs to handle the case of a missing parent chunk:
🏁 Script executed:
#!/bin/bash # Check for code that handles initialization of genesis chunks # or whether there's an assumption that there's always a parent chunk echo "Checking for genesis chunk initialization in the codebase:" rg -A 2 -B 2 "genesis\s+chunk" --type go echo "" echo "Checking for code that handles nil parent chunks:" rg -A 2 -B 2 "parent.*chunk.*nil" --type go echo "" echo "Looking for initialization functions that might create the first chunk:" rg -A 2 "initialize.*genesis" --type goLength of output: 2702
No changes needed to chunk_proposer: genesis is bootstrapped upstream
The
chunk_proposeris designed to propose subsequent chunks once a genesis chunk already exists. The relayer (l2_relayer.go) and tests (chunk_proposer_test.go) are responsible for inserting the initial (genesis) chunk. Failing fast whenparentChunk == nilprevents silent misuses of this component.
- Genesis chunk insertion lives in
rollup/internal/controller/relayer/l2_relayer.go(during bootstrap)rollup/internal/controller/watcher/chunk_proposer_test.go(in tests)- Keeping the error on
nilparent ensures that callers don’t accidentally skip the required genesis setup.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rollup/internal/controller/relayer/l2_relayer.go (1)
867-867: Replace debug print statement with proper loggingThis
fmt.Printlnstatement should be replaced with proper logging using thelogpackage for consistency with the rest of the codebase.-fmt.Println("packing finalizeBundlePostEuclidV2NoProof", len(dbBatch.BatchHeader), dbBatch.CodecVersion, dbBatch.BatchHeader, new(big.Int).SetUint64(endChunk.TotalL1MessagesPoppedBefore+endChunk.TotalL1MessagesPoppedInChunk), common.HexToHash(dbBatch.StateRoot), common.HexToHash(dbBatch.WithdrawRoot)) +log.Debug("packing finalizeBundlePostEuclidV2NoProof", "header_length", len(dbBatch.BatchHeader), "codec_version", dbBatch.CodecVersion, "total_l1_messages", endChunk.TotalL1MessagesPoppedBefore+endChunk.TotalL1MessagesPoppedInChunk, "state_root", dbBatch.StateRoot, "withdraw_root", dbBatch.WithdrawRoot)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
common/version/version.go(1 hunks)rollup/internal/controller/relayer/l2_relayer.go(1 hunks)rollup/internal/orm/batch.go(1 hunks)rollup/internal/orm/orm_test.go(1 hunks)rollup/tests/rollup_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- rollup/internal/orm/batch.go
- common/version/version.go
- rollup/internal/orm/orm_test.go
- rollup/tests/rollup_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
rollup/internal/controller/relayer/l2_relayer.go (1)
rollup/internal/orm/chunk.go (2)
Chunk(20-64)Chunk(72-74)
🔇 Additional comments (6)
rollup/internal/controller/relayer/l2_relayer.go (6)
161-161: Clean up of genesis chunk initializationThe initialization of the genesis chunk has been simplified to only create a chunk with the genesis block header, removing any legacy codec version handling. This aligns with the refactoring goal of exclusively supporting CodecV7.
363-373: Successfully removed legacy codec version supportThe switch statement now only handles CodecV7 and properly logs an error for other versions, consistent with the PR's objective to simplify logic post-Euclid by removing support for legacy codec versions.
419-427: Context ID format updated for CodecV7 onlyThe context ID format has been updated to always start with "v7", reflecting the exclusive support for CodecV7. This is a consistent change that aligns with the overall refactoring objective.
587-596: Bundle finalization updated to support only CodecV7The switch statement for finalizing bundles now only handles CodecV7, which is consistent with the PR's objective to simplify the codebase by removing legacy codec version support.
794-849: Clean implementation of batch payload construction for CodecV7The function
constructCommitBatchPayloadCodecV7is well-structured and now exclusively handles CodecV7 batch payload construction, removing complexity from supporting multiple codec versions. Good job on simplifying this logic while maintaining functionality.
851-880: Simplified bundle payload construction for CodecV7The function
constructFinalizeBundlePayloadCodecV7now exclusively handles CodecV7 bundle payload construction with and without proof. The implementation is clean and aligns well with the post-Euclid simplification goals.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rollup/internal/controller/relayer/l2_relayer.go (1)
947-947: Remove debug print statement in production codeThis print statement appears to be for debugging purposes and should be replaced with proper logging or removed entirely.
-fmt.Println("packing finalizeBundlePostEuclidV2NoProof", len(dbBatch.BatchHeader), dbBatch.CodecVersion, dbBatch.BatchHeader, new(big.Int).SetUint64(endChunk.TotalL1MessagesPoppedBefore+endChunk.TotalL1MessagesPoppedInChunk), common.HexToHash(dbBatch.StateRoot), common.HexToHash(dbBatch.WithdrawRoot)) +log.Debug("packing finalizeBundlePostEuclidV2NoProof", "header_length", len(dbBatch.BatchHeader), "codec_version", dbBatch.CodecVersion, "messages_popped", endChunk.TotalL1MessagesPoppedBefore+endChunk.TotalL1MessagesPoppedInChunk, "state_root", dbBatch.StateRoot, "withdraw_root", dbBatch.WithdrawRoot)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
common/version/version.go(1 hunks)rollup/conf/config.json(1 hunks)rollup/internal/config/relayer.go(0 hunks)rollup/internal/controller/relayer/l2_relayer.go(1 hunks)rollup/internal/orm/batch.go(1 hunks)
💤 Files with no reviewable changes (1)
- rollup/internal/config/relayer.go
✅ Files skipped from review due to trivial changes (1)
- rollup/internal/orm/batch.go
🚧 Files skipped from review as they are similar to previous changes (2)
- common/version/version.go
- rollup/conf/config.json
🔇 Additional comments (6)
rollup/internal/controller/relayer/l2_relayer.go (6)
210-210: Support for only CodecV7 simplifies chunk initializationThe chunk initialization has been simplified by removing the conditional logic for different codec versions, focusing exclusively on CodecV7. This aligns with the broader updates that enforce CodecV7 as the minimum supported codec version.
442-453: Code now exclusively supports CodecV7 for batch committingThe switch statement has been simplified to support only CodecV7, removing all legacy codec version handling. This is consistent with the post-Euclid refactoring goals.
499-507: Context ID generation updated for CodecV7The context ID now includes a "v7" prefix, which aligns with the CodecV7-only approach. The parsing logic has been appropriately updated to handle this new format.
668-676: Bundle finalization now only supports CodecV7The bundle finalization logic has been simplified to only support CodecV7, consistent with the codec version unification throughout the codebase.
931-960: CodecV7-specific bundle finalization methods addedThe new dedicated methods
finalizeBundlePostEuclidV2andfinalizeBundlePostEuclidV2NoProofprovide clear separation for with-proof and without-proof scenarios, enhancing code readability.
874-929: Simplified batch commit payload construction for CodecV7The
constructCommitBatchPayloadCodecV7function now handles the construction of batch commit payloads exclusively for CodecV7, removing complexity related to supporting multiple codec versions. This approach makes the code more maintainable and less error-prone.
5 participants
Purpose or design rationale of this PR
Related fix in da-codec repo: scroll-tech/da-codec#52.
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
New Features
Bug Fixes
Refactor
Chores