Skip to content

Add FastLZ compression into L1CostFunc#8635

Closed
mdehoog wants to merge 7 commits intoethereum-optimism:developfrom
mdehoog:flz-l1-cost-func
Closed

Add FastLZ compression into L1CostFunc#8635
mdehoog wants to merge 7 commits intoethereum-optimism:developfrom
mdehoog:flz-l1-cost-func

Conversation

@mdehoog
Copy link
Contributor

@mdehoog mdehoog commented Dec 15, 2023

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 the GasPriceOracle. Companion op-geth PR is here: ethereum-optimism/op-geth#202

Tests

TODO

Additional context

The current naive L1Cost approach:

l1fee = ((0s in calldata) * 4 + (1s in calldata) * 16 + 188) * l1BaseFee * 0.684

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.

@codecov
Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (b4c39a9) 34.57% compared to head (fb82c26) 34.62%.
Report is 77 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8635      +/-   ##
===========================================
+ Coverage    34.57%   34.62%   +0.05%     
===========================================
  Files          167      168       +1     
  Lines         7162     7159       -3     
  Branches      1212     1211       -1     
===========================================
+ Hits          2476     2479       +3     
+ Misses        4537     4529       -8     
- Partials       149      151       +2     
Flag Coverage Δ
cannon-go-tests 63.48% <ø> (ø)
chain-mon-tests 27.14% <ø> (ø)
common-ts-tests 26.74% <ø> (ø)
contracts-bedrock-tests 20.35% <0.00%> (+0.17%) ⬆️
contracts-ts-tests 12.25% <ø> (ø)
core-utils-tests 44.03% <ø> (ø)
sdk-next-tests 42.18% <ø> (ø)
sdk-tests 42.18% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ckages/contracts-bedrock/src/L2/GasPriceOracle.sol 0.00% <0.00%> (ø)
...es/contracts-bedrock/src/libraries/Compression.sol 0.00% <0.00%> (ø)

... and 3 files with indirect coverage changes

@tynes
Copy link
Contributor

tynes commented Dec 15, 2023

I would like this contract to be backwards compatible with branching logic based on an isEclipse bool

/// gas over actually compressing the data, given we only need the length.
/// @dev Returns the length of the compressed data.
/// @custom:attribution Solady <https://github.com/Vectorized/solady>
function flzCompressLen(bytes memory data) internal pure returns (uint256 n) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put this in a library inside of src/libraries? Maybe Compression.sol or something like that. Then we need to differentially test against the Go version. It would be easier to do so by having the Go code in the monorepo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tynes Sure, i'll move it to a separate file in libraries.

I don't think op-geth has any dependencies on the monorepo yet, to avoid circular deps; but we can differentially test it by pulling in the op-geth commit here.

)

replace github.com/ethereum/go-ethereum v1.13.5 => github.com/ethereum-optimism/op-geth v1.101304.2-0.20231130012434-cd5316814d08
replace github.com/ethereum/go-ethereum v1.13.5 => github.com/mdehoog/op-geth v0.0.0-20231216052905-14963121633c
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ethereum-optimism/op-geth#202 will need to be merged first

@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot added Stale and removed Stale labels Dec 31, 2023
@tynes
Copy link
Contributor

tynes commented Jan 3, 2024

I would like to close this PR as we have agreed that this functionality will not go into ecotone to not feature bloat it but could be considered for the following hardfork

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants