fix: Fix blob fee overflow on rollup-relayer and gas-oracle #1772
Conversation
WalkthroughGetPoSL1Client now returns a raw Changes
Sequence Diagram(s)sequenceDiagram
participant Sender
participant L1Watcher
participant RPC as L1_RPC_Node
participant Relayer
participant Metrics
Note over Sender,L1Watcher: Both components now hold a raw *rpc.Client
Sender->>RPC: eth_blobBaseFee(call)
RPC-->>Sender: hex blobBaseFee
Sender->>Sender: parse hex -> uint64
L1Watcher->>RPC: eth_blobBaseFee(call) (when ExcessBlobGas)
RPC-->>L1Watcher: hex blobBaseFee
L1Watcher->>L1Watcher: parse hex -> uint64
Relayer->>Relayer: compute candidate baseFee/blobFee
alt value > configured limit
Relayer->>Metrics: increment rollup_layer1_gas_price_oracle_fee_over_limit_total
Relayer->>Relayer: clamp fee to configured limit
end
Relayer->>RPC: submit packed gas-oracle transaction
RPC-->>Relayer: tx response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-11-27T18:50:44.578ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1772 +/- ##
===========================================
- Coverage 36.53% 36.49% -0.05%
===========================================
Files 247 247
Lines 21188 21226 +38
===========================================
+ Hits 7742 7746 +4
- Misses 12616 12656 +40
+ Partials 830 824 -6
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: 0
🧹 Nitpick comments (2)
rollup/internal/controller/sender/estimategas.go (1)
122-122: Consider the sensitivity of logged transaction data.The enhanced logging now includes the full serialized
msgpayload, which contains transaction details including theDatafield. While this is valuable for debugging, be aware that this could log sensitive contract call data or transaction parameters in error scenarios.rollup/internal/controller/sender/sender.go (1)
846-857: Approve with minor observation on uint64 conversion.The RPC-based approach correctly delegates blob base fee computation to the L1 node, fixing the overflow issue from stale local
CalcBlobFeeparameters. The acknowledged timing discrepancy between the header and blob base fee is acceptable for fee estimation.One minor note:
blobBaseFeeHex.ToInt().Uint64()will silently truncate if the value exceedsmath.MaxUint64. The comment is correct that a properly functioning L1 node shouldn't return such values, but for defensive coding, you could add a check:blobBaseFeeInt := blobBaseFeeHex.ToInt() if !blobBaseFeeInt.IsUint64() { return 0, 0, 0, 0, fmt.Errorf("blob base fee overflows uint64: %s", blobBaseFeeInt.String()) } blobBaseFee = blobBaseFeeInt.Uint64()This would provide explicit error handling rather than silent truncation if an L1 node misbehaves.
📜 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 (18)
common/testcontainers/testcontainers.go(2 hunks)common/testcontainers/testcontainers_test.go(1 hunks)common/version/version.go(1 hunks)rollup/cmd/gas_oracle/app/app.go(1 hunks)rollup/go.mod(1 hunks)rollup/internal/config/relayer.go(1 hunks)rollup/internal/controller/relayer/l1_relayer.go(1 hunks)rollup/internal/controller/relayer/l1_relayer_metrics.go(2 hunks)rollup/internal/controller/relayer/relayer_test.go(1 hunks)rollup/internal/controller/sender/estimategas.go(2 hunks)rollup/internal/controller/sender/sender.go(4 hunks)rollup/internal/controller/sender/sender_test.go(1 hunks)rollup/internal/controller/watcher/l1_watcher.go(5 hunks)rollup/internal/controller/watcher/l1_watcher_test.go(1 hunks)rollup/tests/bridge_test.go(3 hunks)rollup/tests/gas_oracle_test.go(2 hunks)tests/integration-test/contracts_test.go(2 hunks)tests/integration-test/integration_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Thegaram
Repo: scroll-tech/scroll PR: 1769
File: rollup/internal/config/relayer.go:112-114
Timestamp: 2025-11-27T18:50:44.560Z
Learning: In `rollup/internal/config/relayer.go`, the fields `L1BaseFeeLimit` and `L1BlobBaseFeeLimit` in `GasOracleConfig` should never be set to 0. Zero values would break the gas oracle fee enforcement logic in `l1_relayer.go` by capping all fees to 0.
Learnt from: Thegaram
Repo: scroll-tech/scroll PR: 1746
File: rollup/internal/controller/sender/sender.go:637-640
Timestamp: 2025-10-18T06:49:24.796Z
Learning: In the file `rollup/internal/controller/sender/sender.go`, the resubmission logic in `createReplacingTransaction` does not convert V0 blob sidecars to V1 when resubmitting transactions after the Fusaka upgrade. This is an accepted edge case because it's unlikely to occur, geth is expected to handle it gracefully, and manual recovery is available if needed.
📚 Learning: 2025-11-27T18:50:44.560Z
Learnt from: Thegaram
Repo: scroll-tech/scroll PR: 1769
File: rollup/internal/config/relayer.go:112-114
Timestamp: 2025-11-27T18:50:44.560Z
Learning: In `rollup/internal/config/relayer.go`, the fields `L1BaseFeeLimit` and `L1BlobBaseFeeLimit` in `GasOracleConfig` should never be set to 0. Zero values would break the gas oracle fee enforcement logic in `l1_relayer.go` by capping all fees to 0.
Applied to files:
rollup/internal/config/relayer.gorollup/internal/controller/relayer/l1_relayer.gorollup/tests/gas_oracle_test.gorollup/internal/controller/watcher/l1_watcher.gorollup/internal/controller/relayer/l1_relayer_metrics.go
📚 Learning: 2025-10-18T06:49:24.796Z
Learnt from: Thegaram
Repo: scroll-tech/scroll PR: 1746
File: rollup/internal/controller/sender/sender.go:637-640
Timestamp: 2025-10-18T06:49:24.796Z
Learning: In the file `rollup/internal/controller/sender/sender.go`, the resubmission logic in `createReplacingTransaction` does not convert V0 blob sidecars to V1 when resubmitting transactions after the Fusaka upgrade. This is an accepted edge case because it's unlikely to occur, geth is expected to handle it gracefully, and manual recovery is available if needed.
Applied to files:
rollup/internal/controller/sender/sender.gorollup/internal/controller/watcher/l1_watcher.go
🧬 Code graph analysis (4)
rollup/cmd/gas_oracle/app/app.go (2)
rollup/internal/config/l1.go (1)
L1Config(4-14)rollup/internal/controller/watcher/l1_watcher.go (1)
NewL1WatcherClient(36-56)
rollup/tests/gas_oracle_test.go (1)
rollup/internal/controller/watcher/l1_watcher.go (1)
NewL1WatcherClient(36-56)
rollup/internal/controller/relayer/l1_relayer_metrics.go (1)
common/observability/ginmetrics/types.go (2)
Counter(18-18)Gauge(20-20)
rollup/internal/controller/watcher/l1_watcher_test.go (2)
rollup/internal/config/l1.go (1)
L1Config(4-14)rollup/internal/controller/watcher/l1_watcher.go (1)
NewL1WatcherClient(36-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (21)
common/version/version.go (1)
8-8: LGTM!The version bump from v4.7.5 to v4.7.6 is appropriate for this bug fix PR and follows semantic versioning conventions.
common/testcontainers/testcontainers_test.go (1)
35-35: LGTM!The test correctly adapts to the updated
GetL2GethClientandGetPoSL1Clientsignatures by discarding the raw RPC client when it's not needed for the test assertions.Also applies to: 43-43
common/testcontainers/testcontainers.go (2)
170-180: LGTM!The updated signature correctly returns both the raw RPC client and the ethclient, enabling callers to access RPC methods like
eth_blobBaseFeethat aren't available through the standard ethclient wrapper. This aligns with the PR objective of replacingmisc.CalcBlobFeewith RPC-based blob base fee retrieval.
226-232: LGTM!The method signature update is consistent with
GetPoSL1Client, providing both the raw RPC client and ethclient to test callers.rollup/internal/controller/sender/sender_test.go (1)
97-97: LGTM!The test correctly adapts to the updated
GetPoSL1Clientsignature by discarding the raw RPC client, which is not needed for this test's operations.tests/integration-test/integration_test.go (1)
82-82: LGTM!The test correctly adapts to the updated
GetL2GethClientsignature by discarding the raw RPC client, which is not needed for this integration test.rollup/internal/controller/watcher/l1_watcher_test.go (1)
24-27: LGTM!The test correctly adapts to the updated API signatures by capturing the raw RPC client from
GetPoSL1Clientand passing it toNewL1WatcherClient. This change enables the watcher to call RPC methods likeeth_blobBaseFeedirectly, which aligns with the PR's objective.rollup/tests/gas_oracle_test.go (1)
39-39: I'll help you verify thel1RawClientinitialization in the test file. Let me search for its definition and how it's being initialized.
<function_calls>
#!/bin/bashSearch for l1RawClient definition/initialization in the test file
rg -n 'l1RawClient' rollup/tests/gas_oracle_test.go -C 3
echo "---"
echo "Also checking for GetPoSL1Client usage:"
echo "---"Check for GetPoSL1Client usage in test file
rg -n 'GetPoSL1Client' rollup/tests/gas_oracle_test.go -C 3
</function_calls>rollup/go.mod (1)
54-54:deckarep/golang-set v1.8.0is outdated; consider upgrading to a newer major version.The version
v1.8.0exists as a GitHub tag, but it is significantly outdated. The latest version isv2.8.0(released March 2025). Pinning tov1.8.0while much newer versions are available may miss important bug fixes and features. Verify whether this older version was selected intentionally for compatibility reasons, or if an upgrade tov2.xis feasible.rollup/internal/controller/relayer/l1_relayer.go (1)
176-187: LGTM! Limit enforcement logic is correct.The fee limit checks properly guard against zero limits and clamp values when exceeded. The metric increment and error logging provide good observability.
Based on learnings, ensure that
L1BaseFeeLimitandL1BlobBaseFeeLimitare never configured to 0 in production, as zero values would incorrectly cap all fees to 0.tests/integration-test/contracts_test.go (1)
27-27: LGTM! Test code correctly adapted to new return signature.The call sites properly handle the three-value return from
GetL2GethClient(), discarding the raw RPC client while using the ethclient as before.Also applies to: 66-66
rollup/cmd/gas_oracle/app/app.go (1)
69-75: LGTM! RPC client initialization pattern is correct.The switch from
ethclient.Dialtorpc.Dialfollowed byethclient.NewClientenables raw RPC method calls (likeeth_blobBaseFee) while maintaining compatibility with existing ethclient usage. The L1 watcher now receives the raw RPC client to support direct RPC calls.rollup/internal/controller/relayer/relayer_test.go (1)
77-77: LGTM! Test initialization correctly adapted.The call properly handles the updated
GetL2GethClient()signature.rollup/internal/config/relayer.go (1)
112-114: Ensure non-zero validation for production configs.The new limit fields provide important safeguards against erroneous gas oracle updates. Based on learnings,
L1BaseFeeLimitandL1BlobBaseFeeLimitshould never be set to 0 in production configurations, as zero values would cap all fees to 0, breaking the gas oracle.Consider adding validation logic to reject zero values for these fields when loading production configurations, or document the requirement clearly in config examples and deployment guides.
Based on learnings, these fields must be non-zero in production.
rollup/internal/controller/relayer/l1_relayer_metrics.go (1)
16-16: LGTM! Metric properly defined and initialized.The new counter metric
rollup_layer1_gas_price_oracle_fee_over_limit_totalfollows the existing naming conventions and will provide visibility when fee limits are exceeded. The metric is correctly incremented inl1_relayer.gowhen either base fee or blob base fee exceeds configured limits.Also applies to: 47-50
rollup/internal/controller/watcher/l1_watcher.go (2)
36-36: LGTM! Constructor refactored to use raw RPC client.The constructor now accepts an
*rpc.Clientand internally creates theethclient.Clientwrapper. This enables direct RPC method calls while maintaining compatibility with existing ethclient-based code.Also applies to: 49-50
87-97: I'll verify the concerns raised in the review comment about theeth_blobBaseFeeRPC method. Let me search the codebase and investigate the issues systematically.
<function_calls>
{
"command": "#!/bin/bash\n# Search for eth_blobBaseFee usage in codebase\necho "=== Searching for eth_blobBaseFee usage ==="\nrg -n "eth_blobBaseFee" --type=go\n\necho ""\necho "=== Searching for blobBaseFee usage ==="\nrg -n "blobBaseFee" --type=go -C2\n\necho ""\necho "=== Looking for L1 node version requirements ==="\nfd -e md -e txt | head -20 | xargs rg -i "version|capability|node.*requirement" -C1"
}
</function_calls>I need to run shell scripts to verify the concerns. Let me generate the verification scripts:
<function_calls>
#!/bin/bashSearch for eth_blobBaseFee usage in codebase
echo "=== Searching for eth_blobBaseFee usage ==="
rg -n "eth_blobBaseFee" --type=goecho ""
echo "=== Searching for blobBaseFee variable usage downstream ==="
rg -n "blobBaseFee" --type=go -C2
</function_calls>Let me also check the specific file mentioned in the review and look for error handling patterns:
<function_calls>
#!/bin/bashRead the l1_watcher.go file around the lines in question
echo "=== Reading l1_watcher.go around lines 87-97 ==="
sed -n '70,110p' rollup/internal/controller/watcher/l1_watcher.goecho ""
echo "=== Searching for CallContext error handling patterns ==="
rg -n "CallContext" --type=go -B1 -A3 | head -50
</function_calls>Now let me check the hexutil package and how ToInt().Uint64() handles potential overflows:
<function_calls>
#!/bin/bashCheck hexutil usage and imports in l1_watcher.go
echo "=== Checking imports and hexutil usage in l1_watcher.go ==="
head -50 rollup/internal/controller/watcher/l1_watcher.go | grep -E "^import|hexutil"echo ""
echo "=== Searching for how blobBaseFee is used after assignment ==="
rg -n "blobBaseFee" --type=go -A5 | head -60echo ""
echo "=== Check if there are other RPC method calls for comparison ==="
rg -n 'CallContext.*"eth_' --type=go -B1 -A2 | head -50
</function_calls>Let me also check the related gas oracle logic mentioned in the learning:
<function_calls>
#!/bin/bashCheck l1_relayer.go for gas oracle logic and blob fee handling
echo "=== Searching for L1BaseFeeLimit and L1BlobBaseFeeLimit ==="
rg -n "L1BlobBaseFeeLimit|L1BaseFeeLimit" --type=go -B2 -A2echo ""
echo "=== Check gas oracle fee enforcement logic ==="
rg -n "gas.*oracle|GasOracleConfig" --type=go -B1 -A2 | head -50
</function_calls>Let me also search for documentation on eth_blobBaseFee and its requirements:
<function_calls>
eth_blobBaseFee RPC method Ethereum L1 node version requirements
</function_calls>rollup/tests/bridge_test.go (1)
22-22: LGTM! Test setup correctly updated for new client initialization.The test properly adapts to the updated
GetPoSL1Client()andGetL2GethClient()signatures, capturing the raw RPC client alongside the ethclient. This enables RPC-level interactions in tests while maintaining compatibility with existing ethclient-based test code.Also applies to: 41-43, 96-98
rollup/internal/controller/sender/sender.go (3)
16-16: LGTM!Import addition is appropriate for parsing the
eth_blobBaseFeeRPC response.
70-71: LGTM!Adding the raw
rpcClientfield enables callingeth_blobBaseFeewhich isn't exposed by the wrapped ethclient. The clarifying comments are helpful.
145-146: LGTM!The
rpcClientis correctly stored and reused for bothgethClientinitialization and direct RPC calls.
frisitano
left a comment
frisitano
left a comment
There was a problem hiding this comment.
Looks good, left some comments inline.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rollup/cmd/gas_oracle/app/app.go (1)
69-83: Gas-oracle L1 client init and fee-limit sanity check are aligned; consider defensive nil guardsSwitching to
rpc.Dial+ethclient.NewClientand passing the rawl1RpcClientintoNewL1WatcherClientis consistent with the new raw-client pattern. Thelog.Critguard onL1BaseFeeLimitandL1BlobBaseFeeLimitensures these are non-zero so the gas-oracle can’t silently cap all fees to 0, which matches the intended behavior inl1_relayer.go. To avoid panics on misconfigured configs, you might optionally add explicit nil checks forcfg.L1Config.RelayerConfigand itsGasOracleConfigbefore accessing these fields and cover this path in a small config-load test.
Based on learnings, this sanity check is important for fee enforcement correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
common/testcontainers/testcontainers.go(1 hunks)common/testcontainers/testcontainers_test.go(1 hunks)rollup/cmd/gas_oracle/app/app.go(1 hunks)rollup/internal/controller/relayer/l1_relayer.go(1 hunks)rollup/internal/controller/sender/sender_test.go(2 hunks)rollup/internal/controller/watcher/l1_watcher_test.go(1 hunks)rollup/tests/bridge_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- common/testcontainers/testcontainers_test.go
- rollup/internal/controller/sender/sender_test.go
- rollup/internal/controller/relayer/l1_relayer.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Thegaram
Repo: scroll-tech/scroll PR: 1769
File: rollup/internal/config/relayer.go:112-114
Timestamp: 2025-11-27T18:50:44.560Z
Learning: In `rollup/internal/config/relayer.go`, the fields `L1BaseFeeLimit` and `L1BlobBaseFeeLimit` in `GasOracleConfig` should never be set to 0. Zero values would break the gas oracle fee enforcement logic in `l1_relayer.go` by capping all fees to 0.
Learnt from: Thegaram
Repo: scroll-tech/scroll PR: 1746
File: rollup/internal/controller/sender/sender.go:637-640
Timestamp: 2025-10-18T06:49:24.796Z
Learning: In the file `rollup/internal/controller/sender/sender.go`, the resubmission logic in `createReplacingTransaction` does not convert V0 blob sidecars to V1 when resubmitting transactions after the Fusaka upgrade. This is an accepted edge case because it's unlikely to occur, geth is expected to handle it gracefully, and manual recovery is available if needed.
📚 Learning: 2025-11-27T18:50:44.560Z
Learnt from: Thegaram
Repo: scroll-tech/scroll PR: 1769
File: rollup/internal/config/relayer.go:112-114
Timestamp: 2025-11-27T18:50:44.560Z
Learning: In `rollup/internal/config/relayer.go`, the fields `L1BaseFeeLimit` and `L1BlobBaseFeeLimit` in `GasOracleConfig` should never be set to 0. Zero values would break the gas oracle fee enforcement logic in `l1_relayer.go` by capping all fees to 0.
Applied to files:
rollup/cmd/gas_oracle/app/app.go
🧬 Code graph analysis (1)
rollup/internal/controller/watcher/l1_watcher_test.go (2)
rollup/internal/config/l1.go (1)
L1Config(4-14)rollup/internal/controller/watcher/l1_watcher.go (1)
NewL1WatcherClient(36-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (2)
common/testcontainers/testcontainers.go (1)
170-181: GetPoSL1Client raw RPC refactor looks goodReturning a raw
*rpc.Clientand letting callers wrap it inethclient.NewClientis a clean separation and matches the rest of the PR; the error path also surfaces the endpoint clearly for debugging.rollup/internal/controller/watcher/l1_watcher_test.go (1)
22-29: L1 watcher setup correctly uses raw RPC clientUsing
l1RawClientfromGetPoSL1Clientand passing it intoNewL1WatcherClientmatches the updated constructor (which wraps it in anethclientinternally), so the test wiring stays valid.
Purpose or design rationale of this PR
Previously we used the l2geth Go SDK's
misc.CalcBlobFeefunction to compute the blob base fee corresponding to a header. However, the internal parameters ofCalcBlobFeehave changed in Osaka and subsequent BPO forks. L2geth did not adopt these parameter updates, and as a result, we computed extermely high blob base fee values, which in turn resulted in u64 overflows, erroneous gas oracle updates, and L1 fee estimation failures.This PR makes the following changes:
misc.CalcBlobFee, we will now call the L1 node'seth_blobBaseFeeAPI.eth_blobBaseFee. The "wrapped client" from l2geth does not support this API.gas-oracleconfigurations (l1_base_fee_limitandl1_blob_base_fee_limit) to better prevent erroneous gas oracle updates in the future.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?Rollout requires config update.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.