feat(feynman): L2 fee calculation #1199
Conversation
WalkthroughThis change introduces fork-aware base fee calculation logic in the protocol. The Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CalcBaseFee
participant calcBaseFeeFeynman
participant calcBaseFeeEIP1559
participant extractBaseFeeEIP1559
Caller->>CalcBaseFee: CalcBaseFee(config, parent, parentL1BaseFee, currentHeaderTime)
alt Clique shadow fork
CalcBaseFee-->>Caller: return fixed 0.01 Gwei
else if before Feynman or parent nil
CalcBaseFee-->>Caller: return legacy calcBaseFee
else
CalcBaseFee->>calcBaseFeeFeynman: call with config, parent, overhead
calcBaseFeeFeynman->>calcBaseFeeEIP1559: call with config, parent
calcBaseFeeEIP1559->>extractBaseFeeEIP1559: call with config, parent.BaseFee
extractBaseFeeEIP1559-->>calcBaseFeeEIP1559: returns adjusted base fee
calcBaseFeeEIP1559-->>calcBaseFeeFeynman: returns EIP-1559 base fee
calcBaseFeeFeynman-->>CalcBaseFee: adds overhead and returns
CalcBaseFee-->>Caller: return fork-aware base fee
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 7fbc35d and c68b20339b17f11ea3e0e00a3b5e089a6b47113c.
📒 Files selected for processing (3)
consensus/misc/eip1559.go(1 hunks)params/config.go(2 hunks)params/protocol_params.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
params/config.go (1)
params/protocol_params.go (2)
DefaultBaseFeeChangeDenominator(124-124)DefaultElasticityMultiplier(125-125)
consensus/misc/eip1559.go (3)
params/config.go (1)
ChainConfig(634-677)core/types/block.go (1)
Header(69-122)params/protocol_params.go (1)
InitialBaseFee(126-126)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test
- GitHub Check: build-mock-ccc-geth
- GitHub Check: check
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
params/protocol_params.go (1)
124-126: LGTM! Appropriate renaming for configurable parameters.The renaming from
BaseFeeChangeDenominatorandElasticityMultipliertoDefaultBaseFeeChangeDenominatorandDefaultElasticityMultipliercorrectly indicates these are now default values that can be accessed through chain configuration methods, supporting the Feynman fork's conditional fee calculation logic.params/config.go (1)
1022-1030: LGTM! Clean accessor methods for EIP-1559 parameters.These methods properly expose the default base fee parameters, allowing future customization per chain configuration while maintaining backward compatibility.
consensus/misc/eip1559.go (2)
110-116: Verify the double application of base fee calculation.The implementation applies
calcBaseFeetwice when at the Feynman fork:
- First, it calculates
baseFeeEIP1559using the new EIP-1559 logic- Then, it applies
calcBaseFeeagain on this intermediate resultThis seems to apply the scalar and overhead adjustments twice. Based on the PR description, the formula should be:
l2_fee(tx) = gasUsed(tx)*proving_fee + gasUsed(tx)*l2_base_feeWhere
l2_base_feefollows EIP-1559. The current implementation might not align with this formula.Could you clarify if this double application is intentional? If the goal is to have the EIP-1559 mechanism determine the base component before applying L2-specific adjustments, please add a comment explaining this design choice.
118-162: Well-implemented EIP-1559 base fee calculation.The
calcBaseFeeEIP1559function correctly implements the standard EIP-1559 base fee adjustment logic:
- Proper handling of the first EIP-1559 block
- Correct gas target calculation using elasticity multiplier
- Appropriate base fee adjustments based on gas usage vs target
- Proper bounds checking to prevent negative base fees
Thegaram
left a comment
Thegaram
left a comment
There was a problem hiding this comment.
Current implementation is lgtm, pending discussion on Slack:
- Consider
max(eip1559BaseFee, overhead)so that we don't needextractBaseFeeEIP1559. (Not super important.) - Consider if we want to verify the base fee update, and if yes, we need to read the current
l2BaseFeeOverheadfrom state.
Co-authored-by: Péter Garamvölgyi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
consensus/misc/eip1559.go (1)
110-116: Minor: allocate once & clarify fork check
IsFeynman(big.NewInt(0).Add(parent.Number, common.Big1))allocates a new
big.Intevery block. It’s cheap but unnecessary and slightly obscures
intent. Consider:-nextNumber := new(big.Int).Add(parent.Number, common.Big1) -if !config.IsFeynman(nextNumber) { +nextNumber := new(big.Int).Add(parent.Number, common.Big1) +if !config.IsFeynman(nextNumber) { return calcBaseFee(scalar, overhead, parentL1BaseFee) }Makes the intent (“next block”) explicit and avoids repeated temp allocation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
consensus/misc/eip1559.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
consensus/misc/eip1559.go (3)
params/config.go (1)
ChainConfig(634-677)core/types/block.go (1)
Header(69-122)params/protocol_params.go (1)
InitialBaseFee(126-126)
🪛 GitHub Check: build-mock-ccc-geth
consensus/misc/eip1559.go
[failure] 170-170:
undefined: baseFeeEIP
[failure] 168-168:
undefined: baseFeeEIP
[failure] 167-167:
undefined: baseFeeEIP
[failure] 166-166:
undefined: baseFeeEIP
[failure] 165-165:
no new variables on left side of :=
🪛 GitHub Check: test
consensus/misc/eip1559.go
[failure] 170-170:
undefined: baseFeeEIP
[failure] 168-168:
undefined: baseFeeEIP
[failure] 167-167:
undefined: baseFeeEIP
[failure] 166-166:
undefined: baseFeeEIP
[failure] 165-165:
no new variables on left side of :=
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
consensus/misc/eip1559.go (3)
110-113: Minor: avoid the heap allocation inside the fork check.
big.NewInt(0).Add(parent.Number, common.Big1)allocates every time.
Usingnew(big.Int).Add(...)or a small scratch variable outside the hot path eliminates repeated allocations in block production loops.
145-156: Use distinct variables for clarity & to avoid accidental aliasing.
numis reused as both the delta and the receiver ofAdd, which makes the code harder to audit.
SeparatingdeltaandnewBaseFee(as done in upstream geth) reads clearer and removes any doubt about aliasing side-effects.
172-175: Unnecessary copy and missed receiver – simplify & avoid confusion.The first
Setis discarded immediately becauseaux.Suboverwrites it usingbaseFeeas the operand.
The same result can be achieved with one allocation and without touching the input value:-func extractBaseFeeEIP1559(config *params.ChainConfig, baseFee *big.Int) *big.Int { - aux := new(big.Int).Set(baseFee) - return aux.Sub(baseFee, config.ProvingBaseFee) +func extractBaseFeeEIP1559(config *params.ChainConfig, baseFee *big.Int) *big.Int { + return new(big.Int).Sub(baseFee, config.ProvingBaseFee) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
consensus/misc/eip1559.go(1 hunks)params/config.go(3 hunks)params/protocol_params.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- params/protocol_params.go
- params/config.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
consensus/misc/eip1559.go (3)
params/config.go (1)
ChainConfig(634-678)core/types/block.go (1)
Header(69-122)params/protocol_params.go (1)
InitialBaseFee(126-126)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
consensus/misc/eip1559.go (1)
127-132: Edge-case: first Feynman block after a gas-limit change.
parentGasTarget := parent.GasLimit / config.ElasticityMultiplier()assumes the parent block obeys the post-fork elastic limit rules.
If the fork coincides with a gas-limit change, this division could underflow and distort the first base-fee adjustment. Verify whether such a scenario is possible on Scroll’s network or guard against it here.
ranchalp
force-pushed
the
l2-fee-feynman
branch
2 times, most recently
from
ccbc9d4 to
e58ebfa
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
♻️ Duplicate comments (1)
consensus/misc/eip1559.go (1)
119-125:⚠️ Potential issueNil-pointer panic + missing max-fee clamp in
calcBaseFeeFeynman
config.ProvingBaseFeeis a*big.Int.
If it is omitted in the JSON chain-config this line will dereferenceniland crash the node.
Furthermore, the result is no longer clamped toMaximumL2BaseFee, breaking the invariant enforced in the legacy path.func calcBaseFeeFeynman(config *params.ChainConfig, parent *types.Header) *big.Int { baseFeeEIP1559 := calcBaseFeeEIP1559(config, parent) baseFee := new(big.Int).Set(baseFeeEIP1559) - baseFee.Add(baseFee, config.ProvingBaseFee) + // Defensive: treat a missing proving fee as zero. + if config.ProvingBaseFee != nil { + baseFee.Add(baseFee, config.ProvingBaseFee) + } + + // Preserve protocol maximum. + if baseFee.Cmp(big.NewInt(MaximumL2BaseFee)) > 0 { + baseFee.SetUint64(MaximumL2BaseFee) + } return baseFee }
🧹 Nitpick comments (1)
consensus/misc/eip1559.go (1)
106-110: Avoid magic numbers – reuseparams.InitialBaseFeeinstead of hard-coding 10 000 000The value returned here (0.01 Gwei) is already captured by
params.InitialBaseFee. Duplicating the literal makes future changes error–prone and obscures intent.- return big.NewInt(10000000) // 0.01 Gwei + return new(big.Int).SetUint64(params.InitialBaseFee)
🛑 Comments failed to post (2)
consensus/misc/eip1559.go (2)
129-173: 🛠️ Refactor suggestion
Edge-case correctness gaps in EIP-1559 adjustment
extractBaseFeeEIP1559can return a negative value if the parent base fee is smaller thanProvingBaseFee; subsequent multiplications propagate the sign.
Clamp to0immediately after extraction.The two return paths (
gasUsed > targetand< target) no longer clamp the final fee toMaximumL2BaseFee, unlike the legacy formula. A single mis-configured block could therefore violate consensus rules enforced elsewhere.Consider:
- parentBaseFeeEIP1559 := extractBaseFeeEIP1559(config, parent.BaseFee) + parentBaseFeeEIP1559 := extractBaseFeeEIP1559(config, parent.BaseFee) + if parentBaseFeeEIP1559.Sign() < 0 { + parentBaseFeeEIP1559 = common.Big0 + } ... // after computing newBaseFee - return newBaseFee + if newBaseFee.Cmp(big.NewInt(MaximumL2BaseFee)) > 0 { + newBaseFee.SetUint64(MaximumL2BaseFee) + } + return newBaseFeeThis keeps the forked path behaviourally aligned with the pre-Feynman code.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In consensus/misc/eip1559.go between lines 129 and 173, fix two issues: first, clamp the base fee returned by extractBaseFeeEIP1559 to zero immediately to prevent negative values from propagating; second, after computing the new base fee in both the gasUsed > target and gasUsed < target branches, clamp the final base fee to the MaximumL2BaseFee to ensure it does not exceed consensus limits, maintaining alignment with legacy behavior.
174-177:
⚠️ Potential issueBug in
extractBaseFeeEIP1559– wrong operand & nil-check missing
Subis performed withbaseFeeinstead of the copiedaux, and again assumesconfig.ProvingBaseFeeis non-nil.-func extractBaseFeeEIP1559(config *params.ChainConfig, baseFee *big.Int) *big.Int { - aux := new(big.Int).Set(baseFee) - return aux.Sub(baseFee, config.ProvingBaseFee) +func extractBaseFeeEIP1559(config *params.ChainConfig, baseFee *big.Int) *big.Int { + bf := new(big.Int).Set(baseFee) // work on a copy + if config.ProvingBaseFee != nil { + bf.Sub(bf, config.ProvingBaseFee) + } + if bf.Sign() < 0 { + return common.Big0 + } + return bf }📝 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.func extractBaseFeeEIP1559(config *params.ChainConfig, baseFee *big.Int) *big.Int { bf := new(big.Int).Set(baseFee) // work on a copy if config.ProvingBaseFee != nil { bf.Sub(bf, config.ProvingBaseFee) } if bf.Sign() < 0 { return common.Big0 } return bf }🤖 Prompt for AI Agents
In consensus/misc/eip1559.go around lines 174 to 177, the function extractBaseFeeEIP1559 incorrectly uses baseFee as the minuend in the Sub operation instead of the copied aux, and it does not check if config.ProvingBaseFee is nil before using it. Fix this by performing the Sub operation on aux (aux.Sub(aux, config.ProvingBaseFee)) and add a nil check for config.ProvingBaseFee to avoid potential nil pointer dereference.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
consensus/misc/eip1559.go (1)
120-125: Nil-pointer risk when addingconfig.ProvingBaseFee
config.ProvingBaseFeeis a pointer that may benil(e.g. omitted in the JSON).
(*big.Int).Addwill panic on anilreceiver. Guard and keep the protocol max:- baseFee := new(big.Int).Set(baseFeeEIP1559) - baseFee.Add(baseFee, config.ProvingBaseFee) + baseFee := new(big.Int).Set(baseFeeEIP1559) +if config.ProvingBaseFee != nil { + baseFee.Add(baseFee, config.ProvingBaseFee) +} +if baseFee.Cmp(big.NewInt(MaximumL2BaseFee)) > 0 { + baseFee.SetUint64(MaximumL2BaseFee) +}
🧹 Nitpick comments (1)
consensus/misc/eip1559.go (1)
173-175: Unnecessary copy & argument mix-up
auxis initialised as a copy ofbaseFee, yet the subtraction uses the original
pointer as the first operand, mutating the caller-supplied value:aux := new(big.Int).Set(baseFee) return aux.Sub(baseFee, config.ProvingBaseFee)Safer & clearer:
-aux := new(big.Int).Set(baseFee) -return aux.Sub(baseFee, config.ProvingBaseFee) +aux := new(big.Int).Set(baseFee) +return aux.Sub(aux, config.ProvingBaseFee)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 08b2360073a6abfe41e982fd47c58afc1451e1e7 and 6c5e44d.
📒 Files selected for processing (4)
consensus/misc/eip1559.go(1 hunks)core/state_processor_test.go(1 hunks)params/config.go(2 hunks)params/protocol_params.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/state_processor_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- params/config.go
- params/protocol_params.go
🧰 Additional context used
🪛 GitHub Check: test
consensus/misc/eip1559.go
[failure] 130-130:
cannot use parent.Number (variable of type *big.Int) as uint64 value in argument to config.IsFeynman
[failure] 111-111:
cannot use big.NewInt(0).Add(parent.Number, common.Big1) (value of type *big.Int) as uint64 value in argument to config.IsFeynman
🪛 GitHub Check: build-mock-ccc-geth
consensus/misc/eip1559.go
[failure] 130-130:
cannot use parent.Number (variable of type *big.Int) as uint64 value in argument to config.IsFeynman
[failure] 111-111:
cannot use big.NewInt(0).Add(parent.Number, common.Big1) (value of type *big.Int) as uint64 value in argument to config.IsFeynman
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: semgrep/ci
- GitHub Check: check
- GitHub Check: Analyze (go)
colinlyguo
force-pushed
the
l2-fee-feynman
branch
from
c64d2a7 to
e2f18ee
Compare
5 participants
1. Purpose or design rationale of this PR
Update the L2 fee calculation for the feynman upgrade. Post feynman, the new L2 fee is:
where
l2_base_feefollows vanilla EIP-1559, andproving_feeis set via the existingoverheadparameter. How we update smoothly theoverheadandscalarparameter remains to be decided.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