feat: L1Block uses average L1 basefee by tynes · Pull Request #2583 · ethereum-optimism/optimism

tynes · 2022-05-18T21:24:31Z

Description
This PR adds L1 basefee smoothing. The final numbers are pending but I think its ok to merge this for now and then tweak the numbers later pending internal devnet simulations

changeset-bot · 2022-05-18T21:24:33Z

⚠️ No Changeset found

Latest commit: 7182950

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

mergify · 2022-05-18T21:36:05Z

This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?

mergify · 2022-05-18T21:58:21Z

Hey @tynes! This PR has merge conflicts. Please fix them before continuing review.

contracts-bedrock/contracts/L2/L1Block.sol

tynes · 2022-05-19T04:50:20Z

I created some figures based on the formula used in this PR to see how changes in the basefee would impact the average value. You can see that the more skewed the ratio is, the less volatile the changes are

average * (uint256(7) / uint256(10)) + basefee * (uint256(3) / uint256(10))

average * (uint256(8) / uint256(10)) + basefee * (uint256(2) / uint256(10))

average * (uint256(5) / uint256(10)) + basefee * (uint256(5) / uint256(10))

trianglesphere · 2022-05-31T21:59:51Z

Leaving comment to update gas usage of the L1 Info Deposit (see #2582). The other option (annoying) is to not meter the L1 Info transaction's gas usage at all (but that does have some nice benefits).

mergify · 2022-06-02T20:30:48Z

Hey @tynes! This PR has merge conflicts. Please fix them before continuing review.

tynes · 2022-06-02T21:12:58Z

@K-Ho - Any progress on the numbers that we can use for this? We could do something that is "good enough" for devnet/testnet and tweak later on if necessary

K-Ho · 2022-06-03T23:34:04Z

@K-Ho - Any progress on the numbers that we can use for this? We could do something that is "good enough" for devnet/testnet and tweak later on if necessary

Currently working out the best formula to use w Michael. For now let’s just leave the averaging as a TODO and just use the vanilla basefee

trianglesphere

Formula's wrong

packages/contracts-bedrock/contracts/L2/L1Block.sol

tynes · 2022-06-07T22:28:40Z

Formula's wrong

I'm gonna write the formula as a library and test directly

trianglesphere · 2022-06-07T22:32:35Z

I'm gonna write the formula as a library and test directly

That's one good solution. The reason that this wasn't caught in the tests is that you compared the formula to itself rather than hardcoded values which would have made it obvious that the formula was not producing numbers we expect.

mergify · 2022-06-07T23:47:47Z

Hey @tynes! This PR has merge conflicts. Please fix them before continuing review.

tynes · 2022-06-08T03:16:53Z

I'm gonna write the formula as a library and test directly

That's one good solution. The reason that this wasn't caught in the tests is that you compared the formula to itself rather than hardcoded values which would have made it obvious that the formula was not producing numbers we expect.

I've turned the logic into a library but its not yet properly tested. I think that I am using too many decimals (1e18), the basefee could technically grow large enough to lose precision. Maybe we should use less decimals (1e3) ?
Also do you think the best way to test this would be to differentially test it against an implementation in another language to ensure that there aren't precision errors?

tynes · 2022-06-08T03:20:39Z

packages/contracts-bedrock/contracts/test/L1Block.t.sol

    }

+    function test_averageBasefee() external {
+        uint256 expect = Lib_FeeSmoothing.rollingAverage(0, 3);


This is not sufficient as a test, we need unit tests that are realistic

tynes · 2022-06-08T03:22:00Z

packages/contracts-bedrock/contracts/test/GasPriceOracle.t.sol

    function test_l1BaseFee() external {
        uint256 l1BaseFee = gasOracle.l1BaseFee();
-        assertEq(l1BaseFee, 100);
+        uint256 expect = Lib_FeeSmoothing.rollingAverage(0, 100);


The value here is 30 which I believe is realistic if 70% weighted towards prev value and 30% weighted towards next value. We need unit tests for the rollingAverage function, not 100% sure of the best way to go about it

Adds simple L1 fee smoothing to improve the UX so that the L1 fee spikes do not impact end users as much.

mergify · 2022-06-08T14:29:13Z

Hey @tynes! This PR has merge conflicts. Please fix them before continuing review.

trianglesphere · 2022-06-08T18:05:43Z

I'm gonna write the formula as a library and test directly

That's one good solution. The reason that this wasn't caught in the tests is that you compared the formula to itself rather than hardcoded values which would have made it obvious that the formula was not producing numbers we expect.

I've turned the logic into a library but its not yet properly tested. I think that I am using too many decimals (1e18), the basefee could technically grow large enough to lose precision. Maybe we should use less decimals (1e3) ? Also do you think the best way to test this would be to differentially test it against an implementation in another language to ensure that there aren't precision errors?

I don't actually care that much about precision errors, just that the output is close enough.

In terms of decimals, during. the Yuga labs spike, the L1 base fee got up to 8 terra-wei (8e12). A uint256 can hold up to 1e77. So using 18 decimals is totally fine & won't overflow (it makes the effective max base fee 1e59 which is stupidly large).

github-actions · 2022-07-06T02:15:45Z

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.

mergify bot requested review from Inphi and mslipper May 18, 2022 21:25

tynes force-pushed the feat/avg-l1-basefee branch from 0f7069b to 7f73802 Compare May 18, 2022 21:35

mergify bot added the conflict label May 18, 2022

tynes force-pushed the feat/avg-l1-basefee branch 2 times, most recently from 0ad43c4 to 5ca8db2 Compare May 18, 2022 22:05

mergify bot removed the conflict label May 18, 2022

tynes force-pushed the feat/avg-l1-basefee branch from 5ca8db2 to 51aa096 Compare May 18, 2022 22:08

protolambda suggested changes May 18, 2022

View reviewed changes

contracts-bedrock/contracts/L2/L1Block.sol Outdated Show resolved Hide resolved

tynes mentioned this pull request May 19, 2022

op-node: Adjust L1 Info Transaction Gas Parameters #2582

Merged

mergify bot added the conflict label Jun 2, 2022

tynes force-pushed the feat/avg-l1-basefee branch from ab04bae to bad2edc Compare June 7, 2022 20:22

mergify bot removed the conflict label Jun 7, 2022

trianglesphere suggested changes Jun 7, 2022

View reviewed changes

packages/contracts-bedrock/contracts/L2/L1Block.sol Outdated Show resolved Hide resolved

mergify bot added the conflict label Jun 7, 2022

tynes force-pushed the feat/avg-l1-basefee branch from 58b9751 to 78bbe86 Compare June 8, 2022 03:13

mergify bot removed the conflict label Jun 8, 2022

tynes force-pushed the feat/avg-l1-basefee branch from 78bbe86 to c4e5777 Compare June 8, 2022 03:20

tynes commented Jun 8, 2022

View reviewed changes

tynes added 2 commits June 7, 2022 20:43

contracts-bedrock: L1 basefee smoothing

6758ead

Adds simple L1 fee smoothing to improve the UX so that the L1 fee spikes do not impact end users as much.

op-bindings: regenerate

7182950

tynes force-pushed the feat/avg-l1-basefee branch from c4e5777 to 7182950 Compare June 8, 2022 03:43

mergify bot added the conflict label Jun 8, 2022

mslipper marked this pull request as draft June 21, 2022 17:14

github-actions bot added the Stale label Jul 6, 2022

tynes removed the Stale label Jul 6, 2022

tynes closed this Jul 11, 2022

Comments

Conversation

tynes commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

mergify bot commented May 18, 2022

Uh oh!

mergify bot commented May 18, 2022

Uh oh!

Uh oh!

tynes commented May 19, 2022

Uh oh!

trianglesphere commented May 31, 2022

Uh oh!

mergify bot commented Jun 2, 2022

Uh oh!

tynes commented Jun 2, 2022

Uh oh!

K-Ho commented Jun 3, 2022

Uh oh!

trianglesphere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tynes commented Jun 7, 2022

Uh oh!

trianglesphere commented Jun 7, 2022

Uh oh!

mergify bot commented Jun 7, 2022

Uh oh!

tynes commented Jun 8, 2022

Uh oh!

tynes Jun 8, 2022

Choose a reason for hiding this comment

Uh oh!

tynes Jun 8, 2022

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jun 8, 2022

Uh oh!

trianglesphere commented Jun 8, 2022

Uh oh!

github-actions bot commented Jul 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tynes commented May 18, 2022 •

edited

Loading

changeset-bot bot commented May 18, 2022 •

edited

Loading