feat(chunk proposer): add config parameter to limit chunk proposing by L2 gas - #1622
feat(chunk proposer): add config parameter to limit chunk proposing by L2 gas#1622jonastheis merged 8 commits into
Conversation
Sorry, something went wrong.
WalkthroughThis update increments the software version from v4.4.96 to v4.4.97 in the version file and adds a new configuration field for maximum Layer 2 gas per chunk. The changes extend to the chunk proposer and utilities by introducing logic to monitor, record, and act upon Layer 2 gas usage during chunk proposals. The pull request also updates related logging and metrics functionalities. Changes
Possibly related PRs
Suggested reviewers
Poem
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 (1.62.2)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package zstd: could not load export data: no export data for "github.com/scroll-tech/da-codec/encoding/zstd"" 📜 Recent review details Configuration used: CodeRabbit UI Reviewing files that changed from the base of the PR and between 493c811 and 6a72d31. 📒 Files selected for processing (1)
🪧 Tips ChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (.coderabbit.yaml)
Documentation and Community
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)rollup/internal/controller/watcher/batch_proposer_test.go (1)📜 Review details210-210: Good use of numeric separator for readability
Using the numeric separator in 20_000_000 improves code readability for large constants.
Consider using the same formatting style (20_000_000) consistently across all test cases for better readability.
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 3de8f61 and b74dd30.
📒 Files selected for processing (4)- rollup/cmd/rollup_relayer/app/app.go (1 hunks)
- rollup/internal/controller/watcher/batch_proposer_test.go (4 hunks)
- rollup/internal/controller/watcher/bundle_proposer_test.go (1 hunks)
- rollup/internal/controller/watcher/chunk_proposer_test.go (13 hunks)
- GitHub Check: tests
- GitHub Check: tests
rollup/internal/controller/watcher/bundle_proposer_test.go (1)99-99: Parameter added correctly for MaxL2GasPerChunk
The addition of the MaxL2GasPerChunk parameter to the ChunkProposerConfig properly integrates the new L2 gas limit feature into the bundle proposer tests, with the value set to math.MaxUint64 which effectively means "no limit" for testing purposes.
rollup/internal/controller/watcher/batch_proposer_test.go (3)114-114: Proper integration of L2 gas limit in batch proposer tests
The addition of the MaxL2GasPerChunk parameter with a reasonable limit of 20,000,000 is consistent with the PR objective to introduce a limit for L2 gas per chunk.
297-297: Parameter consistency in limit test cases
Setting MaxL2GasPerChunk to math.MaxUint64 in these tests ensures that the gas limit won't be a constraint in scenarios testing other limitations.
393-393: Consistent parameter values for related tests
Setting MaxL2GasPerChunk to math.MaxUint64 in the testBatchProposerMaxChunkNumPerBatchLimitCodecV4 test maintains consistent testing behavior across related test cases.
rollup/cmd/rollup_relayer/app/app.go (1)97-99: Good defensive coding with parameter validation
The added validation check for MaxL2GasPerChunk ensures the application fails fast with a clear error message if the configuration is invalid. This follows the same pattern as other configuration validation in the file.
rollup/internal/controller/watcher/chunk_proposer_test.go (5)24-24: New parameter for L2 gas limit properly added to test structure
Adding the maxL2Gas field to the test structure properly supports testing the new L2 gas limit feature.
67-76: Comprehensive test coverage for edge case
Good addition of a test case specifically for when MaxL2GasPerChunk is set to 0, ensuring the system behaves correctly when invalid configurations are provided.
134-147: Excellent test case for L2 gas limit behavior
The new test case "MaxL2GasPerChunkIsSecondBlock" with a specific limit of 1,153,000 provides targeted coverage for the scenario where the second block would exceed the L2 gas limit. The detailed comment explaining the test's purpose is particularly helpful.
198-198: Complete integration in NewChunkProposer initialization
The new parameter is properly passed from the test configuration to the NewChunkProposer function, ensuring the test cases will correctly validate the L2 gas limit behavior.
249-249: Consistent parameter usage in blob size limit tests
Setting MaxL2GasPerChunk to math.MaxUint64 in the blob size limit tests ensures that the blob size constraints are tested without interference from gas limits.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)rollup/conf/config.json (1)📜 Review details97-97: New L2 Gas Limit Parameter Addition
The new configuration parameter "max_l2_gas_per_chunk": 20000000 is clearly added under the "chunk_proposer_config" section. Its naming is consistent with other parameters in the section, and the provided value appears to be within an expected range. Please ensure that the unit of this value (likely in gas units) is well documented elsewhere in the codebase or accompanying documentation, so that future maintainers clearly understand its intended usage and limits.
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between b74dd30 and 493c811.
📒 Files selected for processing (1)- rollup/conf/config.json (1 hunks)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
Sorry, something went wrong.
Codecov ReportAttention: Patch coverage is 58.82353% with 7 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## develop #1622 +/- ##
===========================================
+ Coverage 41.32% 41.33% +0.01%
===========================================
Files 222 222
Lines 18226 18243 +17
===========================================
+ Hits 7531 7541 +10
- Misses 9963 9970 +7
Partials 732 732
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry.
|
Sorry, something went wrong.
Purpose or design rationale of this PR
Add config parameter to limit chunk proposing by L2 gas
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 in common/version.go been updated or have you added bump-version label to this PR?
Breaking change label
Does this PR have the breaking-change label?
Summary by CodeRabbit
Chores
New Features