Skip to content
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

sep/027: upgrade and re-initialize SystemConfig across sepolia superchain #418

Open
wants to merge 63 commits into
base: main
Choose a base branch
from

Conversation

geoknee
Copy link
Contributor

@geoknee geoknee commented Dec 17, 2024

Closes #394

See also #454


TODO

  • upgrade to a multi chain task when things are working for OP Sepolia
  • add some breadcrumbs to the previous task 020 to signify that it was incorrect and that this task supercedes it
  • Build on top of @sebastianst 's new validation framework 3591eec

@geoknee geoknee changed the title first pass at task sep 025 Sep 026: upgrade and re-initialize SystemConfig Dec 17, 2024
@geoknee geoknee changed the title Sep 026: upgrade and re-initialize SystemConfig Sep 027: upgrade and re-initialize SystemConfig Dec 18, 2024
@geoknee geoknee force-pushed the gk/025-holocene-sys-cfg branch 2 times, most recently from 50aeb48 to 2e881b4 Compare December 19, 2024 14:58
@geoknee geoknee changed the title sep/027: upgrade and re-initialize SystemConfig sep/027: upgrade and re-initialize SystemConfig across sepolia superchain Jan 9, 2025
require(previousScalar >> 248 == 0, "scalar-101 previous scalar version != 0 or 1");
require(reencodedScalar >> 248 == 1, "scalar-102 reenconded scalar version != 1");
require(sysCfg.blobbasefeeScalar() == uint32(0), "scalar-103 blobbasefeeScalar !=0");
require(reencodedScalar << 8 == previousScalar << 8, "scalar-104 scalar mismatch");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two comments on this:

  1. It's not very clear, if we define a variable or at least put in the revert reason "baseFeeScalar" that would help a lot
  2. The bit shift looks wrong, we probably want << 32 instead of << 8

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is not working indeed, i tried with the mask 0xfffff and this seems to have a more reasonable value @geoknee:
14aa2bca398d7e38779daed1c1b54d44fb9d852fb8378e0b4641ebcf9c47b124

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this here. In fact what I was trying to do was compare the scalars without the scalar version (first byte). 556d1d5

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm,
Just to make sure, you want to compare the if the first byte (0x0) are correctly incremented to (0x1) when the value scalarVersion is 0 right (if yes for me it works as expected).
CleanShot 2025-01-10 at 13 05 23@2x

lib/forge-std Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

@Ethnical Ethnical self-assigned this Jan 9, 2025

function params() external view returns (uint128 prevBaseFee, uint64 prevBoughtGas, uint64 prevBlockNum); // nosemgrep

function __constructor__() external;
Copy link
Contributor

@Ethnical Ethnical Jan 9, 2025

Choose a reason for hiding this comment

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

function __constructor__() external;

NIT: curious to know if this is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

- **Key**: `0x0000000000000000000000000000000000000000000000000000000000000068`
**Before**: `0x0000000000000000000000000000000000000000000000000000000003938700`
**After**: `0xx00000000000000000000000000000000000d273000001db00000000003938700`
**Meaning**: Updates the `basefeeScalar` and `blobbasefeeScalar` storage variables to `7600` and `86200` respectively. These share a slot with the `gasLimit` which remans at `60000000`
Copy link
Contributor

Choose a reason for hiding this comment

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

@geoknee there is an error here the state is updating with 862000.
We should make sure this is desired and this is a simple typo here:

`86200` respectively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right this is a typo. I decoded it again by hand and have corrected the doc.

- **Key**: `0x0000000000000000000000000000000000000000000000000000000000000068`
**Before**: `0x0000000000000000000000000000000000000000000000000000000001c9c380`
**After**: `0x0000000000000000000000000000000000000000000a6fe00000000001c9c380`
**Meaning**: Updates the `basefeeScalar` and `blobbasefeeScalar` storage variables to `68400` and `0` respectively. These share a slot with the `gasLimit` which remans at `30000000`
Copy link
Contributor

Choose a reason for hiding this comment

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

@geoknee there is an error here the state is updating with 684000.
We should make sure this is desired and this is a simple typo here:

to `68400`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right this is a typo. I decoded it again by hand and have corrected the doc.

Copy link
Contributor

@Ethnical Ethnical left a comment

Choose a reason for hiding this comment

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

Super work with the framework for the SuperConfig upgrade! 🔥

The validation "storage" matched the simulation. But some value into the validation file need to be revisited.
There is few things to complete before merge but would be fast!

CleanShot 2025-01-09 at 22 26 06@2x

Copy link
Contributor

@Ethnical Ethnical left a comment

Choose a reason for hiding this comment

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

LGTM! rechecked the tasks validation + simulation make sense to me!

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.

Bug in SystemConfig Holocene Upgrade
4 participants