feat: update base fee via contract #1189
Open
+137
−91
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Walkthrough
This change removes CLI flags, configuration fields, and related logic for L2 base fee scalar and overhead, centralizing their management via package-level variables updated by chain events. It also introduces new configuration fields for the L2 system config address and event topics, and bumps the patch version.
## Changes
| Files / Areas Changed | Summary |
|--------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------|
| `cmd/geth/main.go`, `cmd/geth/usage.go` | Removed `L2BaseFeeScalarFlag` and `L2BaseFeeOverheadFlag` from CLI flags and help groups. |
| `cmd/utils/flags.go` | Deleted flag declarations, flag handling, and the `setBaseFee` function. Removed imports and flag setup. |
| `consensus/misc/eip1559.go` | Centralized L2 base fee parameters as package-level variables with mutex; added `UpdateL2BaseFeeScalar` and `UpdateL2BaseFeeOverhead`. |
| `consensus/misc/eip1559_test.go` | Updated tests to use `UpdateL2BaseFeeScalar` and `UpdateL2BaseFeeOverhead` instead of direct config assignment. |
| `core/blockchain.go` | Added logic to update L2 base fee parameters from chain events during block processing; added metrics timer. |
| `eth/backend.go` | Removed assignment of base fee scalar/overhead to chain config during initialization; added dynamic state-based initialization. |
| `eth/ethconfig/config.go` | Removed `BaseFeeScalar` and `BaseFeeOverhead` fields from `Config` struct. |
| `params/config.go` | Added `L2SystemConfigAddress` to `L1Config`. Removed base fee fields from `ScrollConfig`. Updated `String()` and added accessor method.|
| `params/version.go` | Bumped `VersionPatch` from 49 to 50. |
| `rollup/rcfg/config.go` | Added variables for L2 base fee update event topics and storage slots with detailed comments. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant BlockChain
participant Log
participant misc as EIP1559 Package
BlockChain->>Log: Process block logs
alt If log is L2BaseFeeUpdate event
Log->>misc: UpdateL2BaseFeeScalar or UpdateL2BaseFeeOverhead(new value)
misc-->>Log: Update internal base fee parameters
end
BlockChain->>misc: CalcBaseFee(...)
misc-->>BlockChain: Return calculated base feePossibly related PRs
Suggested labels
Suggested reviewers
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
rollup/rcfg/config.go (1)
52-56: Address TODO comments before deployment.The L2 base fee configuration variables are initialized with placeholder values and contain TODO comments indicating they need proper assignment. These must be set to correct values before this code can be used in production.
Please ensure these values are properly configured:
L2BaseFeeUpdateTopicshould contain the actual event topic hash from the L2 system config contractL2BaseFeeOverheadSlotandL2BaseFeeScalarSlotshould contain the correct storage slot positionsDo you need help determining the correct values for these variables, or should we create an issue to track setting these properly?
core/blockchain.go (1)
275-293: Review the L2 base fee update implementation for robustness.The implementation correctly processes L2 system config events to update base fee parameters, but consider these potential improvements:
Event validation: Add validation that the log has sufficient topics before accessing
r.Topics[1]andr.Topics[2]to prevent panics.Error handling: Consider adding error handling around
misc.UpdateL2BaseFeeParams()in case the update fails.Performance consideration: The code iterates through all logs for every block. For blocks with many logs, this could be expensive, but it's likely acceptable given the importance of fee updates.
Example improvement for safety:
for _, r := range logs { - if r.Address == l2SystemConfigAddress && r.Topics[0] == rcfg.L2BaseFeeUpdateTopic { + if r.Address == l2SystemConfigAddress && len(r.Topics) >= 3 && r.Topics[0] == rcfg.L2BaseFeeUpdateTopic { scalar := r.Topics[1].Big() overhead := r.Topics[2].Big() - misc.UpdateL2BaseFeeParams(scalar, overhead) + if err := misc.UpdateL2BaseFeeParams(scalar, overhead); err != nil { + log.Warn("Failed to update L2 base fee params", "err", err, "blockNumber", block.NumberU64()) + } else { log.Info("Updated L2 base fee coefficients", "blockNumber", block.NumberU64(), "blockHash", block.Hash().Hex(), "scalar", scalar, "overhead", overhead) + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/geth/main.go(0 hunks)cmd/geth/usage.go(0 hunks)cmd/utils/flags.go(0 hunks)consensus/misc/eip1559.go(3 hunks)consensus/misc/eip1559_test.go(3 hunks)core/blockchain.go(4 hunks)eth/backend.go(0 hunks)eth/ethconfig/config.go(0 hunks)params/config.go(4 hunks)params/version.go(1 hunks)rollup/rcfg/config.go(1 hunks)
💤 Files with no reviewable changes (5)
- cmd/geth/main.go
- eth/ethconfig/config.go
- eth/backend.go
- cmd/geth/usage.go
- cmd/utils/flags.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
rollup/rcfg/config.go (1)
common/types.go (2)
HexToHash(66-66)BigToHash(62-62)
consensus/misc/eip1559_test.go (1)
consensus/misc/eip1559.go (2)
UpdateL2BaseFeeParams(46-52)MinBaseFee(87-89)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (13)
params/version.go (1)
27-27: LGTM! Version bump is appropriate.The patch version increment from 49 to 50 correctly reflects the deployment of the new L2 base fee management features introduced in this PR.
consensus/misc/eip1559.go (3)
22-22: LGTM! Proper import for synchronization.The addition of the
syncpackage is necessary for the mutex-based thread safety implementation.
36-44: LGTM! Well-designed package-level variable structure.The use of package-level variables with default values and mutex protection is a good architectural choice for centralizing L2 base fee parameter management. The default values match the previous configuration defaults, ensuring backward compatibility.
81-84: LGTM! Proper mutex usage for thread safety.The read lock acquisition before accessing the package-level variables ensures thread-safe access to the base fee parameters. The defer statement ensures proper lock release.
consensus/misc/eip1559_test.go (3)
125-125: LGTM! Test properly updated for new parameter management.The test correctly uses
UpdateL2BaseFeeParamsto set the base fee parameters before testing the calculation logic. The values (10000000, 1) are appropriate for this test scenario.
144-144: LGTM! Test uses correct default values.The test properly uses the default base fee parameters (34000000000000, 15680000) that match the package-level defaults, ensuring consistency with the refactored implementation.
153-153: LGTM! Test updated to use explicit values.The test correctly uses explicit big integer values instead of the removed constants, maintaining the same test logic while adapting to the new parameter management approach.
core/blockchain.go (3)
37-37: LGTM! Necessary imports for L2 base fee functionality.The imports for
consensus/miscandrollup/rcfgpackages are required for the new L2 base fee update mechanism implemented in this file.Also applies to: 49-49
60-61: LGTM! Proper metrics for L2 base fee monitoring.The new metrics follow the existing naming conventions and provide valuable observability for L2 base fee values and update performance.
280-280:❓ Verification inconclusive
Verify L2SystemConfigAddress configuration is properly set.
The L2 base fee update functionality depends on
L2SystemConfigAddressbeing correctly configured. Currently, this address is set to empty in the chain configurations with TODO comments.Please ensure that:
- The L2SystemConfigAddress is set to the correct deployed contract address for each network
- The address corresponds to a contract that emits the expected L2BaseFeeUpdateTopic events
- The event structure matches the expected format with scalar and overhead in topics[1] and topics[2]
🏁 Script executed:
#!/bin/bash # Verify L2SystemConfigAddress configuration across different networks echo "Checking L2SystemConfigAddress configuration in chain configs:" rg -A 2 -B 2 "L2SystemConfigAddress.*common\.Address" --type goLength of output: 1261
Confirm L2SystemConfigAddress Is Configured
The L2 base‐fee update logic in
core/blockchain.gowill be inactive untilL2SystemConfigAddressis set. Currently both Sepolia and Mainnet entries inparams/config.goare still defaulting to an empty address:File: params/config.go
// Sepolia L2SystemConfigAddress: common.Address{}, // TODO // Mainnet L2SystemConfigAddress: common.Address{}, // TODOPlease ensure you:
- Populate
L2SystemConfigAddresswith the correct deployed L2SystemConfig contract address for each network.- Confirm the contract emits the expected
L2BaseFeeUpdateTopicevents.- Verify that
topics[1]andtopics[2]correspond toscalarandoverheadin your event decoding.params/config.go (3)
719-719: LGTM! Proper field addition to L1Config struct.The L2SystemConfigAddress field is correctly added with appropriate type and JSON serialization tag.
727-728: LGTM! String method properly updated for new field.The L1Config String method correctly includes the new L2SystemConfigAddress field with proper hex formatting, maintaining consistency with other address fields.
759-760: LGTM! ScrollConfig String method updated correctly.The String method properly excludes the removed BaseFeeScalar and BaseFeeOverhead fields, maintaining consistency with the transition to dynamic L2 base fee updates.
colinlyguo
previously approved these changes
May 23, 2025
colinlyguo
reviewed
May 23, 2025
jonastheis
reviewed
May 23, 2025
consensus/misc/eip1559.go
Outdated
| BaseFeePrecision = new(big.Int).SetUint64(1e18) | ||
|
|
||
| // scalar and overhead are updated automatically in `Blockchain.writeBlockWithState`. | ||
| baseFeeScalar = new(big.Int).SetUint64(34000000000000) |
There was a problem hiding this comment.
these parameters are set via UpdateL2BaseFeeParams which is called when processing a block that contains the update event/log. What I don't understand: how is this persisted across node startups? What if a node read and updated these values and shuts down. Now it won't process this block anymore and needs to read from state if these values are at default values no?
There was a problem hiding this comment.
I was thinking it would update them when it starts processing blocks. But you're right, it's unlikely that we'll encounter another update event shortly after startup. I'll add code to initialize these from state.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
1. Purpose or design rationale of this PR
Revert most changes from #1183.
Update L2 base fee coefficients automatically after block import using a new system contract scroll-tech/scroll-contracts#114.
2. 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:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Bug Fixes
Chores