Actionable comments posted: 0

rollup/internal/controller/sender/estimategas.go (1)

122-122: Consider the sensitivity of logged transaction data.

The enhanced logging now includes the full serialized msg payload, which contains transaction details including the Data field. 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 CalcBlobFee parameters. 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 exceeds math.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.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 2ecc42e and 7527e4d.

• rollup/go.sum is excluded by !**/*.sum

• 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)

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.

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.go
• rollup/internal/controller/relayer/l1_relayer.go
• rollup/tests/gas_oracle_test.go
• rollup/internal/controller/watcher/l1_watcher.go
• rollup/internal/controller/relayer/l1_relayer_metrics.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/sender/sender.go
• rollup/internal/controller/watcher/l1_watcher.go

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.go (1)

• NewL1WatcherClient (36-56)

common/observability/ginmetrics/types.go (2)

• Counter (18-18)
• Gauge (20-20)

rollup/internal/config/l1.go (1)

• L1Config (4-14)

rollup/internal/controller/watcher/l1_watcher.go (1)

• NewL1WatcherClient (36-56)

• GitHub Check: tests
• GitHub Check: tests
• GitHub Check: tests
• GitHub Check: check
• GitHub Check: check
• GitHub Check: tests
• GitHub Check: tests
• GitHub Check: check

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 GetL2GethClient and GetPoSL1Client signatures 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_blobBaseFee that aren't available through the standard ethclient wrapper. This aligns with the PR objective of replacing misc.CalcBlobFee with 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 GetPoSL1Client signature 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 GetL2GethClient signature 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 GetPoSL1Client and passing it to NewL1WatcherClient. This change enables the watcher to call RPC methods like eth_blobBaseFee directly, which aligns with the PR's objective.

rollup/tests/gas_oracle_test.go (1)

39-39: I'll help you verify the l1RawClient initialization in the test file. Let me search for its definition and how it's being initialized.
<function_calls>


#!/bin/bash

Search 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.0 is outdated; consider upgrading to a newer major version.

The version v1.8.0 exists as a GitHub tag, but it is significantly outdated. The latest version is v2.8.0 (released March 2025). Pinning to v1.8.0 while 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 to v2.x is 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 L1BaseFeeLimit and L1BlobBaseFeeLimit are 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.Dial to rpc.Dial followed by ethclient.NewClient enables raw RPC method calls (like eth_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, L1BaseFeeLimit and L1BlobBaseFeeLimit should 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_total follows the existing naming conventions and will provide visibility when fee limits are exceeded. The metric is correctly incremented in l1_relayer.go when 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.Client and internally creates the ethclient.Client wrapper. 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 the eth_blobBaseFee RPC 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/bash

Search for eth_blobBaseFee usage in codebase

echo "=== Searching for eth_blobBaseFee usage ==="
rg -n "eth_blobBaseFee" --type=go

echo ""
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/bash

Read 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.go

echo ""
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/bash

Check 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 -60

echo ""
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/bash

Check l1_relayer.go for gas oracle logic and blob fee handling

echo "=== Searching for L1BaseFeeLimit and L1BlobBaseFeeLimit ==="
rg -n "L1BlobBaseFeeLimit|L1BaseFeeLimit" --type=go -B2 -A2

echo ""
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() and GetL2GethClient() 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_blobBaseFee RPC response.

---

70-71: LGTM!

Adding the raw rpcClient field enables calling eth_blobBaseFee which isn't exposed by the wrapped ethclient. The clarifying comments are helpful.

---

145-146: LGTM!

The rpcClient is correctly stored and reused for both gethClient initialization and direct RPC calls.

Looks good, left some comments inline.

Actionable comments posted: 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 guards

Switching to rpc.Dial + ethclient.NewClient and passing the raw l1RpcClient into NewL1WatcherClient is consistent with the new raw-client pattern. The log.Crit guard on L1BaseFeeLimit and L1BlobBaseFeeLimit ensures these are non-zero so the gas-oracle can’t silently cap all fees to 0, which matches the intended behavior in l1_relayer.go. To avoid panics on misconfigured configs, you might optionally add explicit nil checks for cfg.L1Config.RelayerConfig and its GasOracleConfig before 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.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 740bc6f and 9be56ff.

• 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)

• common/testcontainers/testcontainers_test.go
• rollup/internal/controller/sender/sender_test.go
• rollup/internal/controller/relayer/l1_relayer.go

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.

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

rollup/internal/config/l1.go (1)

• L1Config (4-14)

rollup/internal/controller/watcher/l1_watcher.go (1)

• NewL1WatcherClient (36-56)

• GitHub Check: tests
• GitHub Check: check
• GitHub Check: tests
• GitHub Check: check
• GitHub Check: tests
• GitHub Check: tests
• GitHub Check: check
• GitHub Check: tests

common/testcontainers/testcontainers.go (1)

170-181: GetPoSL1Client raw RPC refactor looks good

Returning a raw *rpc.Client and letting callers wrap it in ethclient.NewClient is 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 client

Using l1RawClient from GetPoSL1Client and passing it into NewL1WatcherClient matches the updated constructor (which wraps it in an ethclient internally), so the test wiring stays valid.