feat(feynman): add new compression-aware rollup fee formula #1196
Conversation
WalkthroughThis change updates the L1 data fee calculation logic to support a new "Feynman" phase, introducing compression ratio and penalty factor considerations. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant FeeModule
participant CompressionLib
Caller->>FeeModule: CalculateL1DataFee(tx, state, config, blockNum, blockTime)
alt Feynman phase
FeeModule->>CompressionLib: estimateTxCompressionRatio(tx.data, blockNum, blockTime, config)
CompressionLib-->>FeeModule: compressionRatio
FeeModule->>FeeModule: calculatePenalty(compressionRatio, penaltyThreshold, penaltyFactor)
FeeModule->>FeeModule: calculateEncodedL1DataFeeFeynman(...)
else Curie/Pre-Curie
FeeModule->>FeeModule: calculate fee using legacy method
end
FeeModule-->>Caller: fee or error
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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 (
|
|
We need to decide how and when we will update both For |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
core/blockchain_test.go (1)
3760-3760: Consider updating variable name for consistency.The variable
commitScalaris now retrieved fromrcfg.ExecScalarSlot, but the variable name still reflects the old naming convention. For better code readability and consistency with the Feynman upgrade changes, consider renaming the variable toexecScalar.- commitScalar := statedb.GetState(rcfg.L1GasPriceOracleAddress, rcfg.ExecScalarSlot) + execScalar := statedb.GetState(rcfg.L1GasPriceOracleAddress, rcfg.ExecScalarSlot)This would also require updating the variable references in the subsequent assertions (lines 3775, 3788) to maintain consistency.
consensus/misc/curie.go (1)
21-21: Consider renamingInitialCommitScalartoInitialExecScalarfor consistency.The storage slot has been renamed from
CommitScalarSlottoExecScalarSlot, but the valueInitialCommitScalarstill uses the old naming convention. For better code clarity and consistency, consider renaming it toInitialExecScalar.rollup/fees/rollup_fee.go (1)
209-210: Implement the compression ratio calculation.The compression ratio is currently hardcoded to 1 as a placeholder. According to the PR objectives, this should be replaced with the actual
est_compression_ratio(tx)implementation.Do you want me to help implement the compression ratio estimation function or open a new issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
consensus/misc/curie.go(1 hunks)core/blockchain_test.go(1 hunks)ethclient/ethclient_test.go(1 hunks)ethclient/gethclient/gethclient_test.go(1 hunks)params/config.go(1 hunks)rollup/fees/rollup_fee.go(5 hunks)rollup/rcfg/config.go(1 hunks)rollup/tracing/tracing.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
ethclient/ethclient_test.go (2)
rollup/rcfg/config.go (1)
ExecScalarSlot(35-35)common/types.go (1)
BigToHash(62-62)
consensus/misc/curie.go (2)
rollup/rcfg/config.go (3)
L1GasPriceOracleAddress(27-27)ExecScalarSlot(35-35)InitialCommitScalar(39-39)common/types.go (1)
BigToHash(62-62)
rollup/tracing/tracing.go (1)
rollup/rcfg/config.go (1)
ExecScalarSlot(35-35)
ethclient/gethclient/gethclient_test.go (2)
rollup/rcfg/config.go (1)
ExecScalarSlot(35-35)common/types.go (1)
BigToHash(62-62)
rollup/rcfg/config.go (1)
common/types.go (1)
BigToHash(62-62)
core/blockchain_test.go (1)
rollup/rcfg/config.go (2)
L1GasPriceOracleAddress(27-27)ExecScalarSlot(35-35)
rollup/fees/rollup_fee.go (1)
rollup/rcfg/config.go (2)
ExecScalarSlot(35-35)Precision(28-28)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: check
🔇 Additional comments (8)
ethclient/ethclient_test.go (1)
204-204: LGTM: Consistent storage slot key renameThe change from
CommitScalarSlottoExecScalarSlotaligns with the broader Feynman upgrade refactor while maintaining the same storage value.rollup/tracing/tracing.go (1)
546-546: LGTM: Storage proof slot updated correctlyThe change ensures tracing captures the renamed storage slot while maintaining the same underlying functionality.
params/config.go (1)
991-994: Placeholder implementation needs completion before activationThe
IsFeynmanmethod currently returnsfalseunconditionally. Ensure this is properly implemented with actual fork block logic before the Feynman upgrade is activated.ethclient/gethclient/gethclient_test.go (1)
88-88: LGTM: Consistent storage slot key rename in testThe change from
CommitScalarSlottoExecScalarSlotmaintains test consistency with the broader Feynman upgrade refactor.rollup/fees/rollup_fee.go (4)
54-54: Field rename looks good.The rename from
commitScalartoexecScalaris consistent with the broader refactoring across the codebase.
77-88: Fee calculation logic properly handles all fork phases.The implementation correctly differentiates between pre-Curie, Curie, and Feynman fee calculations, with appropriate parameter updates.
169-169: Storage slot reading updated correctly.The function now reads from the renamed
ExecScalarSlot, maintaining consistency with the contract storage layout changes.
283-287: Good addition of overflow protection.The check to ensure
rollupFeefits into uint64 is important for circuit compatibility and prevents potential issues with large fee values.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
rollup/fees/rollup_fee.go (2)
81-81: Verify theIsFeynmanfunction implementation.The code uses
config.IsFeynman(blockNumber), but according to the PR objectives, this function needs to be implemented. Please ensure this function is properly defined in theparams.ChainConfigtype.
279-279: Verify theIsFeynmanfunction implementation.The code uses
config.IsFeynman(blockNumber), but according to the PR objectives, this function needs to be implemented. Please ensure this function is properly defined in theparams.ChainConfigtype.
🧹 Nitpick comments (3)
rollup/fees/rollup_fee.go (3)
209-210: Implement the compression ratio estimation function.The compression ratio is currently hardcoded to 1 (scaled by precision) as a placeholder. According to the PR objectives, the
est_compression_ratio(tx)function needs to be implemented to properly estimate the compression ratio (≥ 1) for each transaction.Would you like me to help implement the compression ratio estimation function or open an issue to track this task?
275-275: Consider renaming l1DataFee to rollupFee for consistency.In
EstimateL1DataFeeForMessage, the variable was renamed froml1DataFeetorollupFeeto reflect the Feynman terminology. For consistency, consider applying the same rename inCalculateL1DataFee.Apply this diff to maintain consistency:
- var l1DataFee *big.Int + var rollupFee *big.Int if !config.IsCurie(blockNumber) { - l1DataFee = calculateEncodedL1DataFee(raw, gpoState.overhead, gpoState.l1BaseFee, gpoState.scalar) + rollupFee = calculateEncodedL1DataFee(raw, gpoState.overhead, gpoState.l1BaseFee, gpoState.scalar) } else if !config.IsFeynman(blockNumber) { - l1DataFee = calculateEncodedL1DataFeeCurie(raw, gpoState.l1BaseFee, gpoState.l1BlobBaseFee, gpoState.commitScalar, gpoState.blobScalar) + rollupFee = calculateEncodedL1DataFeeCurie(raw, gpoState.l1BaseFee, gpoState.l1BlobBaseFee, gpoState.commitScalar, gpoState.blobScalar) } else { - l1DataFee = calculateEncodedL1DataFeeFeynman(raw, gpoState.l1BaseFee, gpoState.l1BlobBaseFee, gpoState.commitScalar, gpoState.blobScalar) + rollupFee = calculateEncodedL1DataFeeFeynman(raw, gpoState.l1BaseFee, gpoState.l1BlobBaseFee, gpoState.commitScalar, gpoState.blobScalar) }Also update the subsequent uses of this variable in the function.
Also applies to: 277-283
163-172: Track the scalar update requirement for Feynman activation.According to the PR objectives, the
commitScalarandblobScalarvalues need to be updated on the contract when applying the Feynman upgrade. Currently, the code reads these values from storage slots but doesn't include the update mechanism.Ensure that:
- The scalar update transaction is prepared and tested before Feynman activation
- The initial values are set according to the PR discussion (e.g.,
compression_scalar = 1)- The verification scalar computation follows the proposed heuristics
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rollup/fees/rollup_fee.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
rollup/fees/rollup_fee.go (1)
rollup/rcfg/config.go (1)
Precision(28-28)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
rollup/fees/rollup_fee.go (1)
198-228: Fee calculation implementation aligns with the Feynman formula.The implementation correctly follows the formula from the PR objectives:
rollup_fee(tx) = est_compression_ratio(tx) * tx_size * (exec_scalar * l1_base_fee + blob_scalar * l1_blob_base_fee)The double division by precision (lines 224-225) is correct since both the compression ratio and scalars are scaled by precision.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
rollup/fees/rollup_fee.go (1)
90-92: Still passingcommitScalarwhere Feynman needsexecScalarThis was flagged in a previous review and is still unresolved – the Feynman formula requires
execScalar = compression_scalar + commit_scalar + verification_scalarbut the call is made with
gpoState.commitScalaronly.Either compute
execScalarbeforehand or extendgpoStateto hold it, then pass the correct value:- rollupFee = calculateEncodedL1DataFeeFeynman(raw, gpoState.l1BaseFee, gpoState.l1BlobBaseFee, gpoState.commitScalar, gpoState.blobScalar) + execScalar := new(big.Int).Add(gpoState.commitScalar, gpoState.compressionScalar) + execScalar.Add(execScalar, gpoState.verificationScalar) + rollupFee = calculateEncodedL1DataFeeFeynman( + raw, + gpoState.l1BaseFee, + gpoState.l1BlobBaseFee, + execScalar, + gpoState.blobScalar, + )
🧹 Nitpick comments (1)
rollup/fees/rollup_fee.go (1)
216-218: AvoidInt64()onbig.Int
rcfg.Precision.Int64()is safe today (1 000 000 000 < 2^31), but tight coupling toint64leaks an implementation detail and would break ifPrecisiongrows.-compressionRatio := big.NewInt(rcfg.Precision.Int64()) +compressionRatio := new(big.Int).Set(rcfg.Precision)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rollup/fees/rollup_fee.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
rollup/fees/rollup_fee.go (1)
rollup/rcfg/config.go (1)
Precision(28-28)
🪛 GitHub Check: check
rollup/fees/rollup_fee.go
[failure] 88-88:
cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman
[failure] 88-88:
cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman
[failure] 286-286:
cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman) (typecheck)
[failure] 286-286:
cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman) (typecheck)
🪛 GitHub Check: build-mock-ccc-geth
rollup/fees/rollup_fee.go
[failure] 88-88:
cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman
[failure] 286-286:
cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman
🪛 GitHub Check: test
rollup/fees/rollup_fee.go
[failure] 88-88:
cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman
[failure] 286-286:
cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman
🪛 GitHub Actions: CI
rollup/fees/rollup_fee.go
[error] 88-88: Type mismatch error: cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
rollup/fees/rollup_fee.go (1)
3-8:⚠️ Potential issueRemove unused import ‒ the file currently fails to compile
"time"is imported but never referenced, triggering the CI error:"time" imported and not used-import ( - "bytes" - "math" - "math/big" - "time" +import ( + "bytes" + "math" + "math/big" )🧰 Tools
🪛 GitHub Check: test
[failure] 7-7:
"time" imported and not used🪛 GitHub Check: build-mock-ccc-geth
[failure] 7-7:
"time" imported and not used🪛 GitHub Check: check
[failure] 7-7:
"time" imported and not used
[failure] 7-7:
"time" imported and not used🪛 GitHub Actions: CI
[error] 7-7: "time" imported and not used
♻️ Duplicate comments (3)
rollup/fees/rollup_fee.go (3)
285-290:⚠️ Potential issueSecond
IsFeynmancall still causes a type-mismatch build errorCI reports:
cannot use blockNumber (*big.Int) as uint64 in argument to config.IsFeynmanEither convert or change the helper’s signature, e.g.:
-} else if !config.IsFeynman(blockNumber) { +} else if !config.IsFeynman(blockNumber.Uint64()) {Make sure both call-sites use the same parameter type.
Otherwise the package will not build.🧰 Tools
🪛 GitHub Check: test
[failure] 287-287:
cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman🪛 GitHub Check: build-mock-ccc-geth
[failure] 287-287:
cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman🪛 GitHub Check: check
[failure] 287-287:
cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman) (typecheck)
[failure] 287-287:
cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman) (typecheck)
85-94:⚠️ Potential issue
execScalaris never passed – Feynman branch still receivescommitScalarPer the PR spec:
exec_scalar = compression_scalar + commit_scalar + verification_scalarYet the call forwards
gpoState.commitScalaras theexecScalarargument, so the
fee is underestimated.-} else { - rollupFee = calculateEncodedL1DataFeeFeynman( - raw, gpoState.l1BaseFee, gpoState.l1BlobBaseFee, - gpoState.commitScalar, // <- wrong - gpoState.blobScalar, - ) +} else { + // TODO(feat-feynman): add compressionScalar & verificationScalar to GPO storage + execScalar := new(big.Int).Set(gpoState.commitScalar) // placeholder until the extra scalars are wired in + rollupFee = calculateEncodedL1DataFeeFeynman( + raw, gpoState.l1BaseFee, gpoState.l1BlobBaseFee, + execScalar, + gpoState.blobScalar, + ) }
206-236: 🛠️ Refactor suggestionReduce rounding error in the Feynman fee by dividing once
Two successive
Div(..., rcfg.Precision)introduce two rounding steps.
Combine them into a single division byprecision²to keep precision
identical but with one rounding:-// Divide by rcfg.Precision (once for ratio, once for scalar) -l1DataFee.Div(l1DataFee, rcfg.Precision) -l1DataFee.Div(l1DataFee, rcfg.Precision) +precisionSq := new(big.Int).Mul(rcfg.Precision, rcfg.Precision) +l1DataFee.Div(l1DataFee, precisionSq)
🧹 Nitpick comments (1)
rollup/fees/rollup_fee.go (1)
217-220: Explicit TODO for placeholder compression ratio
compressionRatiois hard-coded to1. Add a clear TODO so it isn’t
forgotten during the rollout.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between a39317a and f1348bbfcabdf23a81e0076e79171438bbc6a949.
📒 Files selected for processing (6)
accounts/abi/bind/backends/simulated.go(1 hunks)eth/tracers/api.go(1 hunks)internal/ethapi/api.go(2 hunks)les/odr_test.go(2 hunks)light/odr_test.go(1 hunks)rollup/fees/rollup_fee.go(5 hunks)
✅ Files skipped from review due to trivial changes (3)
- eth/tracers/api.go
- les/odr_test.go
- internal/ethapi/api.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
light/odr_test.go (1)
rollup/fees/rollup_fee.go (1)
EstimateL1DataFeeForMessage(66-96)
🪛 GitHub Check: test
rollup/fees/rollup_fee.go
[failure] 7-7:
"time" imported and not used
[failure] 287-287:
cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman
🪛 GitHub Check: build-mock-ccc-geth
rollup/fees/rollup_fee.go
[failure] 7-7:
"time" imported and not used
[failure] 287-287:
cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman
🪛 GitHub Check: check
rollup/fees/rollup_fee.go
[failure] 7-7:
"time" imported and not used
[failure] 7-7:
"time" imported and not used
[failure] 287-287:
cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman) (typecheck)
[failure] 287-287:
cannot use blockNumber (variable of type *big.Int) as uint64 value in argument to config.IsFeynman) (typecheck)
🪛 GitHub Actions: CI
rollup/fees/rollup_fee.go
[error] 7-7: "time" imported and not used
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
accounts/abi/bind/backends/simulated.go (1)
640-646: Call update looks goodThe additional
head.Timeargument matches the new
EstimateL1DataFeeForMessagesignature. No issues spotted.light/odr_test.go (1)
200-206: Test updated correctlyThe extra
header.Timeparameter keeps the test compiling with the new
fee-estimation API.
ranchalp
force-pushed
the
l1data-to-rollupfee
branch
from
8278b6e to
d0e730b
Compare
ranchalp
force-pushed
the
l1data-to-rollupfee
branch
from
d0e730b to
ce2498e
Compare
colinlyguo
force-pushed
the
l1data-to-rollupfee
branch
from
65181f4 to
1fbe3ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
cmd/evm/internal/t8ntool/execution.go(1 hunks)core/state_processor.go(5 hunks)go.mod(1 hunks)miner/scroll_worker.go(1 hunks)rollup/fees/rollup_fee.go(7 hunks)rollup/fees/rollup_fee_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/evm/internal/t8ntool/execution.go
- core/state_processor.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
rollup/fees/rollup_fee.go (7)
9-9: LGTM - Required imports for new functionality.The new imports support the compression functionality (
da-codec/encoding) and error logging (log) needed for the Feynman fee calculation.Also applies to: 14-14
65-66: LGTM - New penalty parameters properly added.The
penaltyThresholdandpenaltyFactorfields are correctly added to support the compression-based penalty mechanism in Feynman.
69-69: Function signature updated correctly for time-aware fee calculation.The addition of
blockTimeparameter enables the Feynman phase detection and is properly propagated through the call chain.Also applies to: 81-81
170-204: Well-implemented compression ratio estimation with robust error handling.The function correctly:
- Handles edge cases (empty data, compression failures)
- Uses appropriate logging for error conditions
- Ensures compression ratio ≥ 1.0 for consistency with DA batch compression
- Implements the formula:
compression_ratio = size(tx) * PRECISION / size(zstd(tx))
206-215: Clean and correct penalty calculation implementation.The logic correctly implements: penalty = 1.0 if compression ratio ≥ threshold, otherwise apply penalty factor.
241-284: Feynman fee formula implementation is mathematically correct.The function properly implements the formula:
rollup_fee(tx) = (execScalar * l1BaseFee + blobScalar * l1BlobBaseFee) * size(tx) * penalty(tx) / PRECISION²The two divisions by
rcfg.Precisioncorrectly account for the scalar precision and penalty precision.
319-319: Phase selection logic correctly implemented.The conditional logic properly selects:
- Pre-Curie:
!config.IsCurie(blockNumber)- Curie:
!config.IsFeynman(blockTime)(between Curie and Feynman)- Feynman: default case
The use of
blockNumberfor Curie andblockTimefor Feynman is consistent with their respective fork activation mechanisms.Also applies to: 335-352
rollup/fees/rollup_fee_test.go (4)
9-9: LGTM - Import added for chain configuration support.The
paramsimport is correctly added to support chain configuration in compression ratio tests.
37-96: Excellent test coverage for Feynman fee calculation.The test comprehensively covers:
- No penalty scenario (compression ratio ≥ threshold)
- Penalty scenario (compression ratio < threshold)
- Clear mathematical verification with expected values
- Both test cases properly validate the fee formula implementation
98-121: Good compression ratio testing with realistic scenarios.Tests cover:
- Empty data edge case
- Compressible data with patterns
- Proper use of test chain configuration
123-139: Complete penalty calculation test coverage.Tests validate both penalty conditions:
- No penalty when ratio ≥ threshold
- Penalty applied when ratio < threshold
Thegaram
changed the title
colinlyguo
left a comment
colinlyguo
left a comment
There was a problem hiding this comment.
Let's merge this da-codec PR and update the commit first.
colinlyguo
dismissed stale reviews from georgehao, Thegaram, and themself
via
bad7cac
1. Purpose or design rationale of this PR
This PR updates the L1 data fee (aka rollup fee) to use a new formula starting at the Feynman upgrade.
Original formula (post-Curie):
New formula (post-Feynman):
Note:
commitScalar,blobScalar,execScalar,compressionRatio,penaltyThreshold,penaltyFactor,penaltyare all scaled byPRECISION(1e9) to avoid losing precision.penalty_thresholdandpenalty_factorare new parameters stored in L2 state that control the penalty mechanism for poorly compressible transactions.commitScalarandblobScalaron the contract during the application of the Feynman upgrade.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