fix(sender): log msg on error - #1769
Conversation
Sorry, something went wrong.
WalkthroughAdds raw RPC client wiring across sender, watcher, and tests; switches blob base-fee retrieval to eth_blobBaseFee RPC; bumps internal version tag to v4.7.6; enhances gas estimate error logging to include the full message payload; updates tests and consumers to return/accept *rpc.Client alongside *ethclient.Client. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Sender
participant Geth as gethclient (ethclient)
participant RPC as Raw RPC (rpc.Client)
participant Est as Estimator
Note over Sender,Geth: Header fetch unchanged (baseFee from header)
Sender->>Geth: Fetch header (blockNumber, timestamp, baseFee)
Geth-->>Sender: Header
Note over Sender,RPC: New: blob base-fee via RPC call
Sender->>RPC: eth_blobBaseFee (blockNumber / pending)
RPC-->>Sender: blobBaseFee (hexutil.Big)
Sender->>Est: EstimateGas(msg, baseFee, blobBaseFee)
Est-->>Sender: gasEstimate / error
Note right of Est: Errors are logged with full msg via fmt.Sprintf("%+v", msg)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labelsbump-version Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review detailsConfiguration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 2ecc42e and cfe6fd2.
📒 Files selected for processing (2)- common/version/version.go (1 hunks)
- rollup/internal/controller/sender/estimategas.go (2 hunks)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
rollup/internal/controller/sender/estimategas.go (1)common/version/version.go (1)5-5: LGTM!
The fmt import is necessary for the enhanced logging on line 122.
8-8: LGTM!
Version bump from v4.7.5 to v4.7.6 is appropriate for this fix.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 45.45455% with 6 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## develop #1769 +/- ##
========================================
Coverage 36.53% 36.54%
========================================
Files 247 247
Lines 21188 21180 -8
========================================
- Hits 7742 7741 -1
+ Misses 12616 12610 -6
+ Partials 830 829 -1
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.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)rollup/internal/controller/sender/estimategas.go (1)🧹 Nitpick comments (4)123-123: Reconsider logging the full message payload.
Logging the complete msg structure with fmt.Sprintf("%+v", msg) introduces significant concerns:
- Privacy/Compliance Risk: The Data field may contain sensitive transaction payloads or user identifiers. Logging full calldata violates best practices for handling potentially sensitive data.
- Log Volume: Transaction calldata can be extremely large, especially for blob transactions, which will bloat logs significantly during error conditions.
Per the previous review, consider logging only essential fields that aren't already covered by caller logs (From, nonce, To are already logged elsewhere). If debugging requires the Data field, log its length or a truncated version.
Apply this diff to log selective fields:
- log.Error("estimateGasLimit EstimateGas failure without access list", "error", err, "msg", fmt.Sprintf("%+v", msg)) + log.Error("estimateGasLimit EstimateGas failure without access list", "error", err, + "gasPrice", msg.GasPrice, "gasTipCap", msg.GasTipCap, "gasFeeCap", msg.GasFeeCap, + "blobGasFeeCap", msg.BlobGasFeeCap, "dataLen", len(msg.Data), "blobHashCount", len(msg.BlobHashes))
rollup/internal/controller/sender/sender.go (3)📜 Review detailsrollup/internal/controller/sender/estimategas.go (1)837-838: Use appropriate log level for informational data.
This log uses Warn level to record routine operational data (block number). Since this function is called frequently in normal operation, warning-level logs will create noise and obscure genuine issues. Use log.Debug or log.Info instead, and consider removing the redundant function name from the message.
Apply this diff:
- log.Warn("getBlockNumberAndTimestampAndBaseFeeAndBlobFee", "number", header.Number.Uint64()) + log.Debug("retrieved block header", "number", header.Number.Uint64())
842-842: Use appropriate log level and avoid redundant value logging.
This log uses Warn level for routine base fee data and logs the same value in both string and uint64 formats. Use log.Debug or log.Info instead, and choose one representation to reduce log volume.
Apply this diff:
- log.Warn("getBlockNumberAndTimestampAndBaseFeeAndBlobFee", "baseFee", header.BaseFee.String(), "baseFeeUint64", baseFee) + log.Debug("retrieved base fee", "baseFee", baseFee)
848-848: Use appropriate log level and avoid redundant value logging.
This log uses Warn level for routine blob base fee data and logs the same value in both string and uint64 formats. Use log.Debug or log.Info instead, and choose one representation to reduce log volume.
Apply this diff:
- log.Warn("getBlockNumberAndTimestampAndBaseFeeAndBlobFee", "blobBaseFee", misc.CalcBlobFee(*excess).String(), "blobBaseFeeUint64", blobBaseFee) + log.Debug("retrieved blob base fee", "blobBaseFee", blobBaseFee)85-85: Use appropriate log level for gas estimation data.
This log uses Warn level to record routine gas estimation parameters. Warning-level logs should be reserved for conditions that require attention. Use log.Debug or log.Info for normal operational data.
Apply this diff:
- log.Warn("estimateBlobGas", "blobBaseFee", blobBaseFee, "blobGasFeeCap", blobGasFeeCap.String()) + log.Debug("estimated blob gas fees", "blobBaseFee", blobBaseFee, "blobGasFeeCap", blobGasFeeCap.String())
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between cfe6fd2 and 2880bd5.
📒 Files selected for processing (2)- rollup/internal/controller/sender/estimategas.go (3 hunks)
- rollup/internal/controller/sender/sender.go (1 hunks)
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.go
- GitHub Check: bridgehistoryapi-db-cli
- GitHub Check: rollup-db-cli
- GitHub Check: bridgehistoryapi-api
- GitHub Check: coordinator-cron
- GitHub Check: coordinator-api
- GitHub Check: bridgehistoryapi-fetcher
- GitHub Check: blob_uploader
- GitHub Check: gas_oracle
- GitHub Check: rollup_relayer
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review detailsConfiguration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 2880bd5 and 93604d9.
📒 Files selected for processing (1)- rollup/internal/controller/sender/sender.go (4 hunks)
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.go
- GitHub Check: rollup-db-cli
- GitHub Check: bridgehistoryapi-api
- GitHub Check: bridgehistoryapi-fetcher
- GitHub Check: blob_uploader
- GitHub Check: rollup_relayer
- GitHub Check: gas_oracle
- GitHub Check: coordinator-cron
- GitHub Check: bridgehistoryapi-db-cli
- GitHub Check: coordinator-api
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)rollup/internal/controller/sender/estimategas.go (1)🧹 Nitpick comments (4)122-122: Reconsider logging the full message payload.
This change logs the complete msg structure, which raises the same concerns previously identified:
- Privacy/Compliance Risk: The Data field may contain sensitive transaction payloads or user identifiers that should not be logged per compliance best practices.
- Log Volume: Transaction calldata can be extremely large, especially for blob transactions, potentially bloating logs significantly during error conditions.
Consider logging only non-sensitive, essential fields (gas parameters, data length, blob hash count) instead of the full message dump.
Based on the existing review comment and coding best practices.
rollup/tests/bridge_test.go (1)📜 Review detailsrollup/internal/controller/watcher/l1_watcher.go (3)41-44: Unused RPC clients declared at package level.
l1RawClient and l2RawClient are captured from the new triple-return API but appear unused in this file. If these are intentionally reserved for future use, consider adding a brief comment. Otherwise, use blank identifiers (_) as done in other test files to avoid confusion.
// clients - l1RawClient *rpc.Client - l1Client *ethclient.Client - l2RawClient *rpc.Client - l2Client *ethclient.Client + l1Client *ethclient.Client + l2Client *ethclient.ClientAnd in setupEnv:
- l1RawClient, l1Client, err = testApps.GetPoSL1Client() + _, l1Client, err = testApps.GetPoSL1Client() assert.NoError(t, err) - l2RawClient, l2Client, err = testApps.GetL2GethClient() + _, l2Client, err = testApps.GetL2GethClient()Also applies to: 97-99
23-33: Consider naming the eth client field more explicitly
Storing both rpcClient *rpc.Client and client *ethclient.Client on L1WatcherClient is reasonable. For readability, you might consider renaming client → ethClient to make it obvious at call sites which client is being used, especially now that both live on the struct.
35-56: Constructor wiring from raw RPC client looks correct; optionally guard/document nil
The updated constructor that accepts *rpc.Client and derives an *ethclient.Client via ethclient.NewClient(rpcClient) is a clean way to centralize wiring. Behavior around a nil rpcClient remains unchecked, similar to the prior *ethclient.Client usage; if this is a programming precondition, consider either:
- adding a fast if rpcClient == nil { panic(...) } / early return, or
- documenting the non-nil requirement in a comment.
Otherwise the initialization and persisted height logic look unchanged and correct.
80-95: RPC-based blob base fee retrieval is fine; consider logging and compatibility
Switching from local CalcBlobFee to an eth_blobBaseFee RPC call via w.rpcClient.CallContext(w.ctx, &hex, "eth_blobBaseFee") is reasonable, and the hexutil.Big usage plus Uint64() conversion look correct.
Two things to double-check:
- Runtime compatibility: ensure all L1 nodes you target actually support eth_blobBaseFee; otherwise this will now hard-fail where the previous local computation would have continued to work.
- Observability: today, on error you just return fmt.Errorf(...) without logging. Given this is watcher infra, adding a log.Warn (with method name and maybe height context) before returning could make debugging much easier.
If you’re confident about node support and have higher-level logging, this can stay as-is.
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 93604d9 and 692fd0f.
⛔ Files ignored due to path filters (1)- rollup/go.sum is excluded by !**/*.sum
- common/testcontainers/testcontainers.go (2 hunks)
- common/testcontainers/testcontainers_test.go (1 hunks)
- rollup/cmd/gas_oracle/app/app.go (1 hunks)
- rollup/go.mod (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)
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_test.go
- rollup/internal/controller/sender/sender.go
- rollup/internal/controller/relayer/relayer_test.go
rollup/internal/controller/watcher/l1_watcher.go (1)rollup/cmd/gas_oracle/app/app.go (2)
- NewL1WatcherClient (36-56)
rollup/internal/config/l1.go (1)rollup/internal/controller/watcher/l1_watcher_test.go (2)rollup/internal/controller/watcher/l1_watcher.go (1)
- L1Config (4-14)
- NewL1WatcherClient (36-56)
rollup/internal/config/l1.go (1)⏰ 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). (9)rollup/internal/controller/watcher/l1_watcher.go (1)
- L1Config (4-14)
- NewL1WatcherClient (36-56)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: goimports-lint
- GitHub Check: check
- GitHub Check: check
rollup/internal/controller/sender/estimategas.go (1)rollup/go.mod (1)5-5: Import added for enhanced logging.
The fmt import supports the new logging format at Line 122.
rollup/cmd/gas_oracle/app/app.go (1)55-55: Dependency upgrade noted.
The upgrade of github.com/deckarep/golang-set from a pseudo-version to v1.8.0 is a standard version resolution.
common/testcontainers/testcontainers.go (2)69-76: RPC client wiring updated correctly.
The changes properly introduce raw RPC client dialing and pass it to the L1 watcher, aligning with the broader RPC-based flow in this PR. The error message accurately reflects the change from Eth client to raw RPC endpoint.
rollup/tests/gas_oracle_test.go (2)171-180: Public API signature change handled correctly.
GetPoSL1Client now returns both the raw RPC client and the wrapped ethclient, following a consistent pattern. The error handling properly returns nil, nil, err on failure.
Note: This is a breaking change for all callers, but the PR shows that dependent test files have been updated accordingly.
227-233: Public API signature change handled correctly.
GetL2GethClient now returns both the raw RPC client and the wrapped ethclient, mirroring the pattern used in GetPoSL1Client. The implementation correctly leverages the existing GetL2Client() method.
rollup/internal/controller/watcher/l1_watcher_test.go (1)113-113: L1 watcher instantiation updated correctly.
Consistent with the change at Line 39, properly passing l1RawClient to the constructor.
39-39: Perfect! I found the complete flow. The gas oracle test functions are called from TestFunction in bridge_test.go, which explicitly calls setupEnv(t) at the beginning. This ensures that l1RawClient and other package-level variables are properly initialized before the gas oracle tests execute.
Verification complete: l1RawClient is properly initialized.
The test execution flow is:
- TestFunction() calls setupEnv(t) (line 203 in bridge_test.go)
- setupEnv() initializes l1RawClient via testApps.GetPoSL1Client() (line 63 in bridge_test.go)
- Test subtests run via t.Run(), including testImportL1GasPrice and testImportDefaultL1GasPriceDueToL1GasPriceSpike
- Both gas oracle tests can now safely use the initialized l1RawClient package-level variable
The review comment's approval of the code changes is justified.
rollup/internal/controller/sender/sender_test.go (1)24-28: Test setup updated for new API signature.
The test correctly unpacks the three return values from GetPoSL1Client, capturing the RPC client and discarding the ethclient (which the watcher internally creates). The RPC client is properly passed to NewL1WatcherClient.
tests/integration-test/integration_test.go (1)97-98: Test updated for new API signature.
The test correctly unpacks the three return values from GetPoSL1Client, appropriately discarding the RPC client (first return value) since only the ethclient is needed for the test operations.
rollup/internal/controller/relayer/relayer_test.go (1)82-85: Test updated for new API signature.
The test correctly unpacks the three return values from GetL2GethClient, discarding the RPC client since only the ethclient is required for header retrieval and subsequent operations.
tests/integration-test/contracts_test.go (1)77-78: LGTM!
The update correctly adapts to the new GetL2GethClient() triple-return signature by discarding the unused RPC client.
common/testcontainers/testcontainers_test.go (1)27-28: LGTM!
Both calls correctly adapt to the new GetL2GethClient() API signature.
Also applies to: 66-67
rollup/internal/controller/sender/sender.go (2)35-37: LGTM!
The test correctly adapts to the updated API signatures for both GetL2GethClient() and GetPoSL1Client().
Also applies to: 43-45
rollup/internal/controller/watcher/l1_watcher.go (1)70-71: LGTM!
The new rpcClient field is properly initialized in NewSender and correctly wired to the gethClient via gethclient.New(rpcClient). This enables the RPC-based blob base fee retrieval.
Also applies to: 145-146
844-856: Good rationale for switching to RPC-based blob base fee retrieval.
The comment clearly documents why the RPC call is preferred over local CalcBlobFee calculation (L1 node configuration sync issues) and acknowledges the acceptable timing mismatch. The implementation correctly handles the eth_blobBaseFee response using hexutil.Big.
3-20: Confirm mixed go-ethereum import paths are intentional
This file now pulls hexutil from github.com/ethereum/go-ethereum/common/hexutil while using the scroll-tech fork for core/types, ethclient, and rpc. Please confirm this mix is deliberate and you don’t instead want github.com/scroll-tech/go-ethereum/common/hexutil for consistency and to avoid potential version skew.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review detailsConfiguration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 692fd0f and a20959c.
📒 Files selected for processing (7)- common/testcontainers/testcontainers.go (2 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/sender/sender.go (4 hunks)
- rollup/internal/controller/watcher/l1_watcher.go (5 hunks)
- rollup/tests/bridge_test.go (3 hunks)
- rollup/tests/bridge_test.go
- rollup/internal/controller/sender/sender.go
- common/testcontainers/testcontainers.go
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/relayer/l1_relayer.go
rollup/internal/config/relayer.go (1)rollup/internal/controller/relayer/l1_relayer_metrics.go (1)
- GasOracleConfig (99-118)
common/observability/ginmetrics/types.go (2)⏰ 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)
- Counter (18-18)
- Gauge (20-20)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
rollup/internal/controller/relayer/l1_relayer_metrics.go (1)rollup/internal/controller/watcher/l1_watcher.go (5)16-16: LGTM! Metric addition is well-structured.
The new counter metric follows the established naming convention and is properly initialized. The metric will correctly track when gas oracle fees exceed configured limits.
Also applies to: 47-50
6-6: LGTM!
The import additions are necessary and correctly support the RPC-based blob base fee retrieval.
Also applies to: 10-10, 14-14
25-26: LGTM!
Maintaining both the raw RPC client and the typed SDK client is a sound design pattern that enables flexibility for both low-level RPC calls (like eth_blobBaseFee) and high-level typed operations.
49-50: LGTM!
The constructor implementation correctly stores the raw RPC client and derives the typed client using the standard ethclient.NewClient pattern.
85-95: eth_blobBaseFee is a valid standard RPC method; overflow concern is mitigated by documented assumption.
The RPC method is confirmed as part of EIP-4844 and is supported by mainstream Ethereum clients (Geth, Besu, reth).
Regarding overflow handling: the code explicitly documents via comment that "A correct L1 node could not return a value that overflows uint64". While the suggested overflow check would be more defensive, the pattern is consistent with the identical implementation in sender.go:854 and reflects an intentional design decision. The blob base fee is economically bounded in practice, making overflow unlikely to occur in production scenarios.
If you prefer defensive programming over trusting node behavior, the suggested overflow check remains valid:
// A correct L1 node could not return a value that overflows uint64 - blobBaseFee := blobBaseFeeHex.ToInt().Uint64() + blobBaseFeeInt := blobBaseFeeHex.ToInt() + if !blobBaseFeeInt.IsUint64() { + return fmt.Errorf("blob base fee %s exceeds uint64 max", blobBaseFeeInt.String()) + } + blobBaseFee := blobBaseFeeInt.Uint64()
36-36: All callers of NewL1WatcherClient have been properly updated to pass *rpc.Client.
Verification confirms that all five call sites correctly pass the raw RPC client:
- rollup/cmd/gas_oracle/app/app.go:75 — passes l1RpcClient created via rpc.Dial() → *rpc.Client ✓
- rollup/internal/controller/watcher/l1_watcher_test.go:27 — passes rawClient from testApps.GetPoSL1Client() (first return) → *rpc.Client ✓
- rollup/tests/gas_oracle_test.go:39, 113 — pass l1RawClient from testApps.GetPoSL1Client() (first return) → *rpc.Client ✓
The test infrastructure method GetPoSL1Client() in common/testcontainers/testcontainers.go:171 explicitly returns (*rpc.Client, *ethclient.Client, error), and all callers correctly extract and use the first return value.
Sorry, something went wrong.
| // set limit | ||
| if baseFee > r.cfg.GasOracleConfig.L1BaseFeeLimit { | ||
| log.Error("L1 base fee exceed max limit, set to max limit", "baseFee", baseFee, "maxLimit", r.cfg.GasOracleConfig.L1BaseFeeLimit) | ||
| r.metrics.rollupL1RelayerGasPriceOracleFeeOverLimitTotal.Inc() | ||
| baseFee = r.cfg.GasOracleConfig.L1BaseFeeLimit | ||
| } | ||
| if blobBaseFee > r.cfg.GasOracleConfig.L1BlobBaseFeeLimit { | ||
| log.Error("L1 blob base fee exceed max limit, set to max limit", "blobBaseFee", blobBaseFee, "maxLimit", r.cfg.GasOracleConfig.L1BlobBaseFeeLimit) | ||
| r.metrics.rollupL1RelayerGasPriceOracleFeeOverLimitTotal.Inc() | ||
| blobBaseFee = r.cfg.GasOracleConfig.L1BlobBaseFeeLimit | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
Critical: Fee capping logic requires validation of limit values.
This enforcement logic unconditionally compares fees against configured limits without checking if the limits are valid (non-zero). As noted in the config file review, if L1BaseFeeLimit or L1BlobBaseFeeLimit are 0 (their default values when unconfigured), this code will cap all non-zero fees to 0, breaking the gas oracle.
Add validation before the comparison:
}
+ // Only enforce limits if they are configured (non-zero)
+ if r.cfg.GasOracleConfig.L1BaseFeeLimit > 0 && baseFee > r.cfg.GasOracleConfig.L1BaseFeeLimit {
- if baseFee > r.cfg.GasOracleConfig.L1BaseFeeLimit {
log.Error("L1 base fee exceed max limit, set to max limit", "baseFee", baseFee, "maxLimit", r.cfg.GasOracleConfig.L1BaseFeeLimit)
r.metrics.rollupL1RelayerGasPriceOracleFeeOverLimitTotal.Inc()
baseFee = r.cfg.GasOracleConfig.L1BaseFeeLimit
}
+ if r.cfg.GasOracleConfig.L1BlobBaseFeeLimit > 0 && blobBaseFee > r.cfg.GasOracleConfig.L1BlobBaseFeeLimit {
- if blobBaseFee > r.cfg.GasOracleConfig.L1BlobBaseFeeLimit {
log.Error("L1 blob base fee exceed max limit, set to max limit", "blobBaseFee", blobBaseFee, "maxLimit", r.cfg.GasOracleConfig.L1BlobBaseFeeLimit)
r.metrics.rollupL1RelayerGasPriceOracleFeeOverLimitTotal.Inc()
blobBaseFee = r.cfg.GasOracleConfig.L1BlobBaseFeeLimit
}Alternative: Validate limits at initialization time in NewLayer1Relayer (line 50) to ensure they're configured with sensible values before the relayer starts.
📝 Committable suggestion‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // set limit | |
| if baseFee > r.cfg.GasOracleConfig.L1BaseFeeLimit { | |
| log.Error("L1 base fee exceed max limit, set to max limit", "baseFee", baseFee, "maxLimit", r.cfg.GasOracleConfig.L1BaseFeeLimit) | |
| r.metrics.rollupL1RelayerGasPriceOracleFeeOverLimitTotal.Inc() | |
| baseFee = r.cfg.GasOracleConfig.L1BaseFeeLimit | |
| } | |
| if blobBaseFee > r.cfg.GasOracleConfig.L1BlobBaseFeeLimit { | |
| log.Error("L1 blob base fee exceed max limit, set to max limit", "blobBaseFee", blobBaseFee, "maxLimit", r.cfg.GasOracleConfig.L1BlobBaseFeeLimit) | |
| r.metrics.rollupL1RelayerGasPriceOracleFeeOverLimitTotal.Inc() | |
| blobBaseFee = r.cfg.GasOracleConfig.L1BlobBaseFeeLimit | |
| } | |
| // set limit | |
| // Only enforce limits if they are configured (non-zero) | |
| if r.cfg.GasOracleConfig.L1BaseFeeLimit > 0 && baseFee > r.cfg.GasOracleConfig.L1BaseFeeLimit { | |
| log.Error("L1 base fee exceed max limit, set to max limit", "baseFee", baseFee, "maxLimit", r.cfg.GasOracleConfig.L1BaseFeeLimit) | |
| r.metrics.rollupL1RelayerGasPriceOracleFeeOverLimitTotal.Inc() | |
| baseFee = r.cfg.GasOracleConfig.L1BaseFeeLimit | |
| } | |
| if r.cfg.GasOracleConfig.L1BlobBaseFeeLimit > 0 && blobBaseFee > r.cfg.GasOracleConfig.L1BlobBaseFeeLimit { | |
| log.Error("L1 blob base fee exceed max limit, set to max limit", "blobBaseFee", blobBaseFee, "maxLimit", r.cfg.GasOracleConfig.L1BlobBaseFeeLimit) | |
| r.metrics.rollupL1RelayerGasPriceOracleFeeOverLimitTotal.Inc() | |
| blobBaseFee = r.cfg.GasOracleConfig.L1BlobBaseFeeLimit | |
| } |
In rollup/internal/controller/relayer/l1_relayer.go around lines 176 to 186, the fee capping compares baseFee and blobBaseFee against configured limits without validating those limits, so a zero (unset) limit will incorrectly cap fees to 0; update the logic to first check that r.cfg.GasOracleConfig.L1BaseFeeLimit and L1BlobBaseFeeLimit are > 0 before applying the cap (skip capping and avoid incrementing the over-limit metric when the limit is non-positive), and/or add validation in NewLayer1Relayer (around line 50) to enforce sensible, non-zero defaults or return an error if limits are unset so the relayer never runs with zero limits.
Sorry, something went wrong.
Purpose or design rationale of this PR
Describe your change. Make sure to answer these three questions: What does this PR do? Why does it do it? How does it do it?
Add more logs.
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
Bug Fixes / Diagnostics
Tests
✏️ Tip: You can customize this high-level summary in your review settings.