fix(sender): log msg on error #1769
Conversation
WalkthroughAdds raw RPC client wiring across sender, watcher, and tests; switches blob base-fee retrieval to 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 labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/version/version.go(1 hunks)rollup/internal/controller/sender/estimategas.go(2 hunks)
⏰ 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). (3)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (2)
rollup/internal/controller/sender/estimategas.go (1)
5-5: LGTM!The
fmtimport is necessary for the enhanced logging on line 122.common/version/version.go (1)
8-8: LGTM!Version bump from v4.7.5 to v4.7.6 is appropriate for this fix.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ 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. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
rollup/internal/controller/sender/estimategas.go (1)
123-123: Reconsider logging the full message payload.Logging the complete
msgstructure withfmt.Sprintf("%+v", msg)introduces significant concerns:
- Privacy/Compliance Risk: The
Datafield 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))
🧹 Nitpick comments (4)
rollup/internal/controller/sender/sender.go (3)
837-838: Use appropriate log level for informational data.This log uses
Warnlevel 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. Uselog.Debugorlog.Infoinstead, 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
Warnlevel for routine base fee data and logs the same value in both string and uint64 formats. Uselog.Debugorlog.Infoinstead, 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
Warnlevel for routine blob base fee data and logs the same value in both string and uint64 formats. Uselog.Debugorlog.Infoinstead, 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)rollup/internal/controller/sender/estimategas.go (1)
85-85: Use appropriate log level for gas estimation data.This log uses
Warnlevel to record routine gas estimation parameters. Warning-level logs should be reserved for conditions that require attention. Uselog.Debugorlog.Infofor 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())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rollup/internal/controller/sender/estimategas.go(3 hunks)rollup/internal/controller/sender/sender.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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.go
⏰ 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). (12)
- 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
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rollup/internal/controller/sender/sender.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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.go
⏰ 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). (12)
- 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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rollup/internal/controller/sender/estimategas.go (1)
122-122: Reconsider logging the full message payload.This change logs the complete
msgstructure, which raises the same concerns previously identified:
- Privacy/Compliance Risk: The
Datafield 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.
🧹 Nitpick comments (4)
rollup/tests/bridge_test.go (1)
41-44: Unused RPC clients declared at package level.
l1RawClientandl2RawClientare 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
rollup/internal/controller/watcher/l1_watcher.go (3)
23-33: Consider naming the eth client field more explicitlyStoring both
rpcClient *rpc.Clientandclient *ethclient.ClientonL1WatcherClientis reasonable. For readability, you might consider renamingclient→ethClientto 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 nilThe updated constructor that accepts
*rpc.Clientand derives an*ethclient.Clientviaethclient.NewClient(rpcClient)is a clean way to centralize wiring. Behavior around a nilrpcClientremains unchecked, similar to the prior*ethclient.Clientusage; 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 compatibilitySwitching from local
CalcBlobFeeto aneth_blobBaseFeeRPC call viaw.rpcClient.CallContext(w.ctx, &hex, "eth_blobBaseFee")is reasonable, and thehexutil.Bigusage plusUint64()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 alog.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.
📜 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 (14)
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)
🧰 Additional context used
🧠 Learnings (1)
📚 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_test.gorollup/internal/controller/sender/sender.gorollup/internal/controller/relayer/relayer_test.go
🧬 Code graph analysis (3)
rollup/tests/gas_oracle_test.go (1)
rollup/internal/controller/watcher/l1_watcher.go (1)
NewL1WatcherClient(36-56)
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/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). (9)
- 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
🔇 Additional comments (16)
rollup/internal/controller/sender/estimategas.go (1)
5-5: Import added for enhanced logging.The
fmtimport supports the new logging format at Line 122.rollup/go.mod (1)
55-55: Dependency upgrade noted.The upgrade of
github.com/deckarep/golang-setfrom a pseudo-version to v1.8.0 is a standard version resolution.rollup/cmd/gas_oracle/app/app.go (1)
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.
common/testcontainers/testcontainers.go (2)
171-180: Public API signature change handled correctly.
GetPoSL1Clientnow returns both the raw RPC client and the wrapped ethclient, following a consistent pattern. The error handling properly returnsnil, nil, erron 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.
GetL2GethClientnow returns both the raw RPC client and the wrapped ethclient, mirroring the pattern used inGetPoSL1Client. The implementation correctly leverages the existingGetL2Client()method.rollup/tests/gas_oracle_test.go (2)
113-113: L1 watcher instantiation updated correctly.Consistent with the change at Line 39, properly passing
l1RawClientto the constructor.
39-39: Perfect! I found the complete flow. The gas oracle test functions are called fromTestFunctionin bridge_test.go, which explicitly callssetupEnv(t)at the beginning. This ensures thatl1RawClientand other package-level variables are properly initialized before the gas oracle tests execute.Verification complete:
l1RawClientis properly initialized.The test execution flow is:
TestFunction()callssetupEnv(t)(line 203 in bridge_test.go)setupEnv()initializesl1RawClientviatestApps.GetPoSL1Client()(line 63 in bridge_test.go)- Test subtests run via
t.Run(), includingtestImportL1GasPriceandtestImportDefaultL1GasPriceDueToL1GasPriceSpike- Both gas oracle tests can now safely use the initialized
l1RawClientpackage-level variableThe review comment's approval of the code changes is justified.
rollup/internal/controller/watcher/l1_watcher_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 toNewL1WatcherClient.rollup/internal/controller/sender/sender_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.tests/integration-test/integration_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.rollup/internal/controller/relayer/relayer_test.go (1)
77-78: LGTM!The update correctly adapts to the new
GetL2GethClient()triple-return signature by discarding the unused RPC client.tests/integration-test/contracts_test.go (1)
27-28: LGTM!Both calls correctly adapt to the new
GetL2GethClient()API signature.Also applies to: 66-67
common/testcontainers/testcontainers_test.go (1)
35-37: LGTM!The test correctly adapts to the updated API signatures for both
GetL2GethClient()andGetPoSL1Client().Also applies to: 43-45
rollup/internal/controller/sender/sender.go (2)
70-71: LGTM!The new
rpcClientfield is properly initialized inNewSenderand correctly wired to thegethClientviagethclient.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
CalcBlobFeecalculation (L1 node configuration sync issues) and acknowledges the acceptable timing mismatch. The implementation correctly handles theeth_blobBaseFeeresponse usinghexutil.Big.rollup/internal/controller/watcher/l1_watcher.go (1)
3-20: Confirm mixed go-ethereum import paths are intentionalThis file now pulls
hexutilfromgithub.com/ethereum/go-ethereum/common/hexutilwhile using thescroll-techfork forcore/types,ethclient, andrpc. Please confirm this mix is deliberate and you don’t instead wantgithub.com/scroll-tech/go-ethereum/common/hexutilfor consistency and to avoid potential version skew.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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)
🚧 Files skipped from review as they are similar to previous changes (3)
- rollup/tests/bridge_test.go
- rollup/internal/controller/sender/sender.go
- common/testcontainers/testcontainers.go
🧰 Additional context used
🧠 Learnings (1)
📚 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/relayer/l1_relayer.go
🧬 Code graph analysis (2)
rollup/internal/controller/relayer/l1_relayer.go (1)
rollup/internal/config/relayer.go (1)
GasOracleConfig(99-118)
rollup/internal/controller/relayer/l1_relayer_metrics.go (1)
common/observability/ginmetrics/types.go (2)
Counter(18-18)Gauge(20-20)
⏰ 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: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (6)
rollup/internal/controller/relayer/l1_relayer_metrics.go (1)
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
rollup/internal/controller/watcher/l1_watcher.go (5)
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.NewClientpattern.
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:854and 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 ofNewL1WatcherClienthave 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— passesl1RpcClientcreated viarpc.Dial()→*rpc.Client✓rollup/internal/controller/watcher/l1_watcher_test.go:27— passesrawClientfromtestApps.GetPoSL1Client()(first return) →*rpc.Client✓rollup/tests/gas_oracle_test.go:39, 113— passl1RawClientfromtestApps.GetPoSL1Client()(first return) →*rpc.Client✓The test infrastructure method
GetPoSL1Client()incommon/testcontainers/testcontainers.go:171explicitly returns(*rpc.Client, *ethclient.Client, error), and all callers correctly extract and use the first return value.
| // 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.
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 | |
| } |
🤖 Prompt for AI Agents
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.
|
Closing in favor of #1772. |
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
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
Bug Fixes / Diagnostics
Tests
✏️ Tip: You can customize this high-level summary in your review settings.