feat(chunk proposer): add config parameter to limit chunk proposing by L2 gas #1622
Conversation
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 detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rollup/internal/controller/watcher/batch_proposer_test.go (1)
210-210: Good use of numeric separator for readabilityUsing the numeric separator in
20_000_000improves code readability for large constants.Consider using the same formatting style (
20_000_000) consistently across all test cases for better readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (10)
rollup/internal/controller/watcher/bundle_proposer_test.go (1)
99-99: Parameter added correctly for MaxL2GasPerChunkThe addition of the
MaxL2GasPerChunkparameter to the ChunkProposerConfig properly integrates the new L2 gas limit feature into the bundle proposer tests, with the value set tomath.MaxUint64which 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 testsThe addition of the
MaxL2GasPerChunkparameter 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 casesSetting
MaxL2GasPerChunktomath.MaxUint64in these tests ensures that the gas limit won't be a constraint in scenarios testing other limitations.
393-393: Consistent parameter values for related testsSetting
MaxL2GasPerChunktomath.MaxUint64in thetestBatchProposerMaxChunkNumPerBatchLimitCodecV4test maintains consistent testing behavior across related test cases.rollup/cmd/rollup_relayer/app/app.go (1)
97-99: Good defensive coding with parameter validationThe added validation check for
MaxL2GasPerChunkensures 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 structureAdding the
maxL2Gasfield to the test structure properly supports testing the new L2 gas limit feature.
67-76: Comprehensive test coverage for edge caseGood addition of a test case specifically for when
MaxL2GasPerChunkis set to 0, ensuring the system behaves correctly when invalid configurations are provided.
134-147: Excellent test case for L2 gas limit behaviorThe 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 initializationThe new parameter is properly passed from the test configuration to the
NewChunkProposerfunction, ensuring the test cases will correctly validate the L2 gas limit behavior.
249-249: Consistent parameter usage in blob size limit testsSetting
MaxL2GasPerChunktomath.MaxUint64in the blob size limit tests ensures that the blob size constraints are tested without interference from gas limits.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rollup/conf/config.json (1)
97-97: New L2 Gas Limit Parameter AdditionThe new configuration parameter
"max_l2_gas_per_chunk": 20000000is 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.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rollup/conf/config.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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. 🚀 New features to boost your workflow:
|
github-actions
bot
dismissed stale reviews from Thegaram and colinlyguo
via
6a72d31
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
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Chores
New Features