fix(gas-oracle): check blob gas fee before entering default gas price mode #1565
Conversation
colinlyguo
requested review from
jonastheis,
georgehao,
Thegaram and
NazariiDenha
WalkthroughThe pull request includes several changes across multiple files, primarily focusing on updating the version number from Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
🧹 Outside diff range and nitpick comments (3)
rollup/conf/config.json (1)
23-24: Document the threshold configurationThe new blob fee threshold configuration would benefit from documentation explaining:
- The rationale behind the chosen threshold value (100000000000)
- The relationship between default and threshold values
- The impact on gas oracle behavior
Consider adding comments in the configuration file or updating the relevant documentation to explain these values.
rollup/internal/config/relayer.go (1)
92-94: Enhance field documentation with units and relationship details.The new field's purpose is clear, but the documentation could be more specific about:
- The units of the threshold (wei, gwei, etc.)
- How it relates to
L1BlobBaseFeeDefault- What constitutes a reasonable threshold value
Consider updating the comment to:
- // L1BlobBaseFeeThreshold the threshold of L1 blob base fee to enter the default gas price mode + // L1BlobBaseFeeThreshold the threshold of L1 blob base fee (in wei) to enter the default gas price mode. + // When the L1 blob base fee exceeds this threshold, the system will use L1BlobBaseFeeDefault instead.rollup/internal/controller/relayer/l1_relayer.go (1)
Line range hint
169-175: Consider restructuring the error handling for better readability.The current nested conditions mixing error handling with business logic could be clearer. Consider separating the error handling:
-if reachTimeout, err = r.commitBatchReachTimeout(); reachTimeout && block.BlobBaseFee > r.cfg.GasOracleConfig.L1BlobBaseFeeThreshold && err == nil { - if r.lastBaseFee == r.cfg.GasOracleConfig.L1BaseFeeDefault && r.lastBlobBaseFee == r.cfg.GasOracleConfig.L1BlobBaseFeeDefault { - return - } - baseFee = r.cfg.GasOracleConfig.L1BaseFeeDefault - blobBaseFee = r.cfg.GasOracleConfig.L1BlobBaseFeeDefault -} else if err != nil { - return -} +reachTimeout, err := r.commitBatchReachTimeout() +if err != nil { + return +} + +if reachTimeout && block.BlobBaseFee > r.cfg.GasOracleConfig.L1BlobBaseFeeThreshold { + if r.lastBaseFee == r.cfg.GasOracleConfig.L1BaseFeeDefault && r.lastBlobBaseFee == r.cfg.GasOracleConfig.L1BlobBaseFeeDefault { + return + } + baseFee = r.cfg.GasOracleConfig.L1BaseFeeDefault + blobBaseFee = r.cfg.GasOracleConfig.L1BlobBaseFeeDefault +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
common/version/version.go(1 hunks)rollup/conf/config.json(1 hunks)rollup/internal/config/relayer.go(1 hunks)rollup/internal/controller/relayer/l1_relayer.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- common/version/version.go
🔇 Additional comments (3)
rollup/conf/config.json (2)
23-24: Verify threshold alignment with L1 gas dynamics
The large gap between default (1) and threshold (100000000000) values needs verification against typical L1 blob gas fee patterns to ensure it properly handles various network conditions.
#!/bin/bash
# Search for any test configurations or documentation about blob gas fees
rg "blob.*gas.*fee" -g '!*.json' -A 5 -B 5
# Look for any related constants or test values
ast-grep --pattern 'const.*blob.*fee'Consider:
- Documenting the reasoning behind these specific values
- Adding configuration validation to ensure the threshold is always greater than the default
- Implementing monitoring for when these thresholds are hit
23-24:
Review the blob base fee default value
The l1_blob_base_fee_default value of 1 seems unusually low for a base fee. This might cause the system to frequently hit the threshold condition, potentially defeating the purpose of preventing false positives.
Consider adjusting this value to be more in line with typical L1 blob base fees to prevent unnecessary threshold triggers.
rollup/internal/config/relayer.go (1)
92-94: Verify configuration loading and validation.
Let's ensure the configuration is properly loaded and validated.
✅ Verification successful
Based on the results, I can now generate the final response:
Configuration is properly loaded and utilized in the codebase
The L1BlobBaseFeeThreshold configuration:
- Is properly defined in the configuration struct with JSON mapping
- Has a default value set in
rollup/conf/config.json - Is actively used in gas price control logic in
l1_relayer.gowhere it's compared against block.BlobBaseFee to determine gas price mode
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check configuration loading and default values
# Look for configuration loading and validation
rg -A 5 "L1BlobBaseFeeThreshold" --type go
# Check for any validation logic
ast-grep --pattern 'if $config.L1BlobBaseFeeThreshold {
$$$
}'
# Look for sample configurations
fd -e json -e yaml -e toml | xargs rg "l1_blob_base_fee_threshold"
Length of output: 1600
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
rollup/cmd/gas_oracle/app/app.go (1)
Line range hint
87-90: Consider harmonizing L1 and L2 relayer initialization patterns.The L2 relayer still receives a
chainConfigparameter and aninitGenesisflag (set to false), while these have been removed from L1 relayer. This inconsistency in initialization patterns could lead to maintenance challenges.Consider:
- Aligning the initialization patterns between L1 and L2 relayers
- If genesis config is truly not needed for L2 relayer, consider removing it there as well
- Document why the approaches differ if the inconsistency is intentional
rollup/internal/config/relayer.go (1)
92-94: LGTM! Consider adding validation for the threshold value.The new field
L1BlobBaseFeeThresholdis well-documented and properly integrated into the configuration structure. It aligns with the PR's objective of preventing false positives when entering default gas price mode.Consider adding validation logic to ensure:
- The threshold is not set lower than
L1BlobBaseFeeDefault- The threshold has a reasonable upper bound to prevent overflow issues
- The threshold value makes economic sense in relation to typical L1 blob base fees
This will help prevent misconfiguration that could lead to unexpected behavior in production.
rollup/internal/controller/relayer/l1_relayer_test.go (1)
60-62: Consider adding test cases for blob gas fee checksWhile the test covers basic confirmation scenarios, given that this PR specifically addresses gas oracle behavior during incidents, consider adding test cases that verify:
- The new blob gas fee threshold check
- Scenarios where the system should/shouldn't enter default gas price mode
rollup/tests/gas_oracle_test.go (1)
Line range hint
1-275: Add test coverage for non-fee related stuck batches.According to the PR objectives, commit batches can get stuck due to factors other than high fees. Consider adding test cases that:
- Simulate scenarios where batches are stuck due to non-fee related issues
- Verify that the system doesn't enter default mode in such cases when blob gas fee is normal
Would you like me to help draft these additional test cases?
rollup/internal/controller/relayer/l1_relayer.go (1)
166-166: LGTM! Enhanced default mode entry logic with blob fee check.The additional check
block.BlobBaseFee > r.cfg.GasOracleConfig.L1BlobBaseFeeThresholdimproves the accuracy of entering default gas price mode by considering blob base fees.Consider adding a code comment explaining the threshold's purpose and its role in preventing false positives.
- if reachTimeout, err = r.commitBatchReachTimeout(); reachTimeout && block.BlobBaseFee > r.cfg.GasOracleConfig.L1BlobBaseFeeThreshold && err == nil { + // Only enter default mode if both timeout is reached and blob base fee exceeds threshold + // This prevents false positives when batches are stuck for reasons other than high fees + if reachTimeout, err = r.commitBatchReachTimeout(); reachTimeout && block.BlobBaseFee > r.cfg.GasOracleConfig.L1BlobBaseFeeThreshold && err == nil {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
common/version/version.go(1 hunks)rollup/cmd/gas_oracle/app/app.go(1 hunks)rollup/conf/config.json(1 hunks)rollup/internal/config/relayer.go(1 hunks)rollup/internal/controller/relayer/l1_relayer.go(3 hunks)rollup/internal/controller/relayer/l1_relayer_test.go(3 hunks)rollup/tests/gas_oracle_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- common/version/version.go
🔇 Additional comments (11)
rollup/conf/config.json (1)
23-24: Verify the relationship between threshold and default values
The current configuration has l1_blob_base_fee_threshold (0) < l1_blob_base_fee_default (1). This relationship needs verification as it could cause immediate triggering of the default mode.
✅ Verification successful
Let me gather more information about how these parameters are used in the codebase.
Based on the code findings, I can now provide a final response:
The threshold and default values relationship is correctly configured
The code in rollup/internal/controller/relayer/l1_relayer.go shows that the default mode is triggered when block.BlobBaseFee > L1BlobBaseFeeThreshold, which means having threshold (0) < default (1) is the intended configuration. The system switches to the default fee when the actual blob base fee exceeds the threshold, providing a fallback mechanism when fees are non-zero.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any validation logic or usage patterns of these parameters
# Look for validation checks between threshold and default values
rg -A 5 "l1_blob_base_fee_(threshold|default)"
# Search for the gas oracle implementation to understand the logic
ast-grep --pattern 'type GasOracleConfig struct {
$$$
L1BlobBaseFeeThreshold $_ `$$$`
L1BlobBaseFeeDefault $_ `$$$`
$$$
}'
Length of output: 1433
Script:
#!/bin/bash
# Search for files that use these parameters to understand the logic
rg -A 10 "L1BlobBaseFee(Threshold|Default)" --type go
# Search for any validation or comparison logic between these parameters
ast-grep --pattern 'if $$.L1BlobBaseFee$$ {
$$$
}'
# Look for any configuration validation
ast-grep --pattern 'func $$(GasOracle|Config|Validate)$$ {
$$$
L1BlobBaseFee
$$$
}'
Length of output: 3113
rollup/cmd/gas_oracle/app/app.go (3)
Line range hint 91-103: LGTM: Block header fetching with reorg protection.
The L1 watcher implementation correctly handles potential reorgs by using number-1 as a safety margin when fetching block headers.
Line range hint 106-107: Verify gas price oracle processing intervals.
The different polling intervals (10s for L1 vs 2s for L2) suggest different gas price dynamics between layers. This aligns with the PR's focus on improving gas price mode handling.
Let's verify the gas price processing implementations:
✅ Verification successful
The different polling intervals (10s for L1 vs 2s for L2) are correctly implemented and justified.
The code review shows that the gas price oracle processing intervals are properly implemented in rollup/cmd/gas_oracle/app/app.go:
go utils.Loop(subCtx, 10*time.Second, l1relayer.ProcessGasPriceOracle)
go utils.Loop(subCtx, 2*time.Second, l2relayer.ProcessGasPriceOracle)The different intervals reflect the distinct characteristics of each layer:
- L1 (10s): Matches Ethereum's block time and allows for base fee/blob base fee stabilization
- L2 (2s): More frequent updates needed due to faster block times and local gas price dynamics
The implementation is consistent with the gas oracle architecture where:
- L1Relayer updates L2's gas price oracle contract with L1 fees
- L2Relayer updates L1's gas price oracle contract with L2 fees
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify gas price processing implementations
# Expected: Find different processing strategies for L1 and L2
# Search for L1 gas price processing implementation
ast-grep --pattern 'func ($r *Layer1Relayer) ProcessGasPriceOracle($ctx context.Context) {
$$$
}'
# Search for L2 gas price processing implementation
ast-grep --pattern 'func ($r *Layer2Relayer) ProcessGasPriceOracle($ctx context.Context) {
$$$
}'
Length of output: 203
Script:
#!/bin/bash
# Let's try a broader search for gas price related functions and configurations
# Search for gas price related functions
rg -l "GasPrice" --type go
# Search for polling or processing intervals
rg -A 5 "interval|poll.*time" --type go
# Search for gas oracle configurations
rg -A 5 "gas.*oracle" --type go -i
Length of output: 120893
83-86: Verify the removal of genesis config from L1 relayer initialization.
The L1 relayer initialization has been simplified by removing the genesis config parameter. This aligns with the PR objectives to improve gas price mode handling.
Let's verify the L1 relayer's gas price mode behavior:
✅ Verification successful
Let me gather more information about the L1 relayer initialization changes to verify the removal of genesis config.
Let me check one more aspect to understand the complete context of the changes.
Based on the gathered information, I can now provide the final response:
The removal of genesis config from L1 relayer initialization is correct and safe.
The changes align with the PR objectives to prevent unexpected default gas price mode entry. The L1 relayer now:
- Properly handles gas price updates through
ProcessGasPriceOracle - Has safeguards against high gas fees with
commitBatchReachTimeoutcheck - Falls back to configured default values (
L1BaseFeeDefault,L1BlobBaseFeeDefault) when needed - Maintains proper initialization without requiring genesis config
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the gas price mode handling in L1 relayer
# Expected: Find the blob gas fee check implementation
# Search for blob gas fee threshold configuration
rg -A 5 "L1BlobBaseFeeThreshold"
# Search for blob gas fee check implementation
ast-grep --pattern 'func ($r *Layer1Relayer) ProcessGasPriceOracle($ctx context.Context) {
$$$
}'
Length of output: 1483
Script:
#!/bin/bash
# Search for L1 relayer initialization changes
ast-grep --pattern 'func NewLayer1Relayer($ctx context.Context, $db $_,$cfg $_,$type $_,$registry $_) (*Layer1Relayer, error) {
$$$
}'
# Search for any genesis-related code in L1 relayer
rg -A 5 "genesis" rollup/internal/controller/relayer/l1_relayer.go
Length of output: 480
Script:
#!/bin/bash
# Check the previous version of L1 relayer initialization
git log -p --all -S "NewLayer1Relayer" -- rollup/internal/controller/relayer/l1_relayer.go
# Check the gas oracle configuration structure
rg -A 10 "type GasOracleConfig struct" rollup/internal/config/
Length of output: 12622
rollup/internal/config/relayer.go (1)
92-94: Verify the configuration usage and defaults.
Let's ensure the new threshold configuration is properly integrated across the codebase.
✅ Verification successful
Let me check the configuration file content and the specific usage in the relayer code to better understand the implementation.
Let me check the commit batch timeout logic to fully understand how this threshold interacts with the timeout condition.
Let me try a different approach to understand the complete gas oracle configuration and its usage.
Based on the gathered information, I can now provide a final response about the configuration usage.
Configuration is properly integrated but default value needs attention
The L1BlobBaseFeeThreshold is correctly integrated in the gas oracle logic with the following implementation details:
- It's used in the L1 relayer to determine when to switch to default gas prices during timeout conditions
- The configuration file sets a default value of 0, which effectively disables the threshold check
- The threshold works in conjunction with the commit batch timeout check to manage gas price updates during high fee periods
The implementation is sound, but having a threshold of 0 means the system will always enter default gas price mode during timeouts, which might not be the intended behavior.
• rollup/conf/config.json: Consider setting a non-zero l1_blob_base_fee_threshold value
• rollup/internal/controller/relayer/l1_relayer.go: The threshold check is properly implemented in the gas oracle update logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the configuration usage and defaults
# Check for configuration files containing the new field
echo "Checking configuration files for l1_blob_base_fee_threshold:"
rg -l "l1_blob_base_fee_threshold"
# Check how the threshold is used in the gas oracle logic
echo -e "\nChecking usage of L1BlobBaseFeeThreshold in code:"
rg "L1BlobBaseFeeThreshold" --type go
# Look for any hardcoded default values
echo -e "\nChecking for potential hardcoded values:"
rg "blob.*fee.*threshold" --type go
Length of output: 1045
Script:
#!/bin/bash
# Check the configuration file content
echo "Content of config.json:"
cat rollup/conf/config.json
# Check the relayer implementation context
echo -e "\nContext around L1BlobBaseFeeThreshold usage in l1_relayer.go:"
rg -B 5 -A 5 "L1BlobBaseFeeThreshold" rollup/internal/controller/relayer/l1_relayer.go
Length of output: 5155
Script:
#!/bin/bash
# Check the commit batch timeout implementation
echo "Checking commitBatchReachTimeout implementation:"
rg -B 10 -A 5 "func.*commitBatchReachTimeout" rollup/internal/controller/relayer/l1_relayer.go
# Check for any related constants or variables
echo -e "\nChecking for related timeout configurations:"
rg "check_committed_batches_window" rollup/internal/controller/relayer/
Length of output: 1080
Script:
#!/bin/bash
# Check the gas oracle configuration structure and usage
echo "Checking GasOracleConfig structure:"
rg -B 5 -A 10 "type GasOracleConfig struct" rollup/internal/config/
# Check the shouldUpdateGasOracle implementation
echo -e "\nChecking shouldUpdateGasOracle implementation:"
rg -B 10 -A 10 "func.*shouldUpdateGasOracle" rollup/internal/controller/relayer/l1_relayer.go
Length of output: 2713
rollup/internal/controller/relayer/l1_relayer_test.go (2)
38-40: LGTM: Basic relayer creation test updated correctly
The removal of the chain config parameter aligns with the simplified NewLayer1Relayer signature while maintaining the essential test coverage for instance creation and cleanup.
93-96: 🛠️ Refactor suggestion
Enhance error scenario coverage for gas oracle processing
While the test covers various error cases, consider adding scenarios specific to the new blob gas fee check:
- When blob gas fee is above threshold
- When blob gas fee check fails
- Edge cases around default mode entry decisions
Let's check if there are any related test files that might need similar updates:
rollup/tests/gas_oracle_test.go (1)
33-33: Add test coverage for blob gas fee check.
While this test covers the basic gas price importing functionality, it should be extended to verify the new blob gas fee check behavior mentioned in the PR objectives. Consider adding test cases that:
- Verify the system doesn't enter default mode when blob gas fee is below threshold
- Verify the system enters default mode when both blob gas fee and other conditions are met
rollup/internal/controller/relayer/l1_relayer.go (3)
32-32: LGTM! Simplified struct by removing unused chainCfg field.
The removal of the chainCfg field reduces unnecessary complexity without impacting functionality.
166-166: Add test coverage for the new blob base fee threshold condition.
As noted in previous reviews, test coverage is needed for:
- Default mode entry when both timeout and blob base fee threshold are exceeded
- Default mode prevention when timeout occurs but blob base fee is below threshold
- Edge cases around the threshold boundary
50-50: Verify all callers of NewLayer1Relayer are updated.
The constructor signature has been simplified by removing the chainCfg parameter. Let's verify all callers are updated accordingly.
✅ Verification successful
All callers of NewLayer1Relayer have been updated correctly
The verification shows all callers are using the updated signature without the chainCfg parameter:
rollup/tests/gas_oracle_test.go(2 calls)rollup/internal/controller/relayer/l1_relayer_test.go(3 calls)rollup/cmd/gas_oracle/app/app.go(1 call)
All calls are passing the correct parameters in the new order: context, db, relayerConfig, serviceType, and registerer. No references to chainCfg were found in any of the calls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all callers of NewLayer1Relayer to ensure they're updated
# Search for NewLayer1Relayer calls
rg -A 3 "NewLayer1Relayer\("
# Look specifically for any remaining references to chainCfg in the calls
rg -B 2 -A 2 "NewLayer1Relayer.*chainCfg"
Length of output: 2671
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1565 +/- ##
===========================================
- Coverage 52.39% 52.38% -0.01%
===========================================
Files 157 157
Lines 12647 12646 -1
===========================================
- Hits 6626 6625 -1
Misses 5447 5447
Partials 574 574
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Purpose or design rationale of this PR
gas-oracle will enter default gas price mode when commit batch is stuck.
This is unexpected during incidents, instead of due to high fees.
This PR adds an extra check to decrease these false positive cases.
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