-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fjord: Add FastLZ compression into L1CostFunc #9618
Fjord: Add FastLZ compression into L1CostFunc #9618
Conversation
WalkthroughWalkthroughThe update brings significant enhancements across various components. Key changes include adding a Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Additional comments not posted (6)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
3f53d5c
to
13b327d
Compare
6413a3e
to
c537fb0
Compare
Co-authored-by: Michael de Hoog <[email protected]> Co-authored-by: Danyal Prout <[email protected]> Co-authored-by: Yukai Tu <[email protected]> Co-authored-by: angel-ding-cb <[email protected]>
e236d6f
to
03f6bea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good. One test uses unsafe Go & CGO and needs a fix for GC safety I believe. And a smart-contracts engineer should confirm the fjordGasPriceOracleCodeHash
and test tx that is referenced in the fjord_fork_test.go
are sufficient (I did check that first layer of the upgrade tx execution is a regular contract deployment, but not past that, and EVM bytecode can behave different depending on the simulation environment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
9eb5f88
* Add FastLZ for better L1Cost estimation Co-authored-by: Michael de Hoog <[email protected]> Co-authored-by: Danyal Prout <[email protected]> Co-authored-by: Yukai Tu <[email protected]> Co-authored-by: angel-ding-cb <[email protected]> * fix all the tests * fix: upate GPO network transactions to match spec * Update GPO contracts * update to 1d model / add tests * update allocs and test framework to support new fjord contracts * add fuzz testing * increase minimum estimation to 100 / update circleci for e2e fuzz tests * use linear regression for l1 gas used * Add differential fastlz fuzzing between solady/cgo fastlz/geth fastlz * Review feedback * Bump geth * fix: ensure we don't gc the data during fastlz compression * Replace common.Hex2Bytes with common.FromHex --------- Co-authored-by: Danyal Prout <[email protected]> Co-authored-by: Yukai Tu <[email protected]> Co-authored-by: angel-ding-cb <[email protected]> Co-authored-by: Danyal Prout <[email protected]>
Reopening #8635, targeting Fjord rather than Ecotone.
Description
We found that the FastLZ algorithm is a pretty good estimate for the actual results we'd see from zlib compressing the batches we write to L1 (albeit with a different
scalar
as the compression ratios aren't quite as good). See https://github.com/roberto-bayardo/compression-analysis and this sheet.This PR introduces a
flzCompress
call into theGasPriceOracle
. Companion op-geth PR is here: ethereum-optimism/op-geth#249Actual GPO update was split out into a new PR: #10534
See specs update here: ethereum-optimism/specs#25
Tests
Added tests comparing compression results from solidity + golang + a reference C implementation.
Additional context
The current naive L1Cost approach:
works pretty well on average, but penalizes very compressible txs (e.g.), and undercharges incompressible txs (e.g.). This change makes the
L1CostFunc
much fairer compared to real-world L1 costs.