sep/027: upgrade and re-initialize SystemConfig
#418
Conversation
geoknee
changed the title
SystemConfig
tasks/sep/026-holocene-system-config-upgrade-and-init-multi-chain/VALIDATION.md
Outdated
Show resolved
Hide resolved
geoknee
changed the title
SystemConfig SystemConfig
geoknee
force-pushed
the
gk/025-holocene-sys-cfg
branch
2 times, most recently
from
50aeb48 to
2e881b4
Compare
tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/NestedSignFromJson.s.sol
Outdated
Show resolved
Hide resolved
tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/NestedSignFromJson.s.sol
Outdated
Show resolved
Hide resolved
tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/OVERVIEW.md
Outdated
Show resolved
Hide resolved
existing task in flight for 025
…ave them, e.g. metal)
…kip only if not found in the TOML file
| import {NestedMultisigBuilder} from "@base-contracts/script/universal/NestedMultisigBuilder.sol"; | ||
| import {SystemConfigUpgrade} from "script/verification/SystemConfigUpgrade.s.sol"; | ||
|
|
||
| contract SystemConfigUpgradeEcotoneScalars is SystemConfigUpgrade { |
There was a problem hiding this comment.
Same comment about adding explanatory code comments
| uint256 reencodedScalar = | ||
| (uint256(0x01) << 248) | (uint256(sysCfg.blobbasefeeScalar()) << 32) | sysCfg.basefeeScalar(); |
There was a problem hiding this comment.
Is this only needed temporarily since we are upgrading the SystemConfig to a version that no longer needs encoding?
There was a problem hiding this comment.
The sysCfg.blobbasefeeScalar() and sysCfg.basefeeScalar() getters read from dedicated storage slots. When we did the upgrade incorrectly previously, these storage slots were not updated causing the getters to return 0. Now, we are checking they return values consistent with what you get if you call sysCfg.scalar() and decode them by hand.
Essentially we implicitly construct the expected values for the two new getters by reencoding what we get after the upgrade and checking it matches the previous scalar value.
I could try redesigning this so it is more explicit, and decode the expected values from the previous.scalar and compare the new getter's results with those?
| if ( | ||
| uint8(previous.scalar >> 248) == 1 // most significant bit | ||
| ) { |
There was a problem hiding this comment.
Why is this behind an if block? A code comment for what this method is testing would be helpful for people not familiar with this encoding
There was a problem hiding this comment.
Tidied that up here 5253651 PTAL hopefully it is clear now.
tasks/sep/020-holocene-system-config-upgrade-multi-chain/README.md
Outdated
Show resolved
Hide resolved
|
|
||
| contract NestedSignFromJson is OriginalNestedSignFromJson, CouncilFoundationNestedSign { | ||
| string constant l1ChainName = "sepolia"; | ||
| string constant release = "v1.8.0-rc.4"; |
There was a problem hiding this comment.
The gov proposal has passed so we don't need to use the RC anymore and should be using the finalized release, which is what we want for mainnet also
| string constant release = "v1.8.0-rc.4"; | |
| string constant release = "v1.8.0"; |
tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/OVERVIEW.md
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,30 @@ | |||
| # Holocene Hardfork Upgrade - `SystemConfig` | |||
|
|
|||
| Status: DRAFT, NOT READY TO SIGN | |||
There was a problem hiding this comment.
what do we need for this to become ready to sign?
| Status: DRAFT, NOT READY TO SIGN | |
| Status: DRAFT, NOT READY TO SIGN |
There was a problem hiding this comment.
I updated it now, there are no blockers other than review of this PR.
| This upgrades the `SystemConfig` in the | ||
| [v1.8.0-rc.4](https://github.com/ethereum-optimism/optimism/tree/v1.8.0-rc.4) release. |
There was a problem hiding this comment.
we should just use 1.8.0 now that it's passed
| This upgrades the `SystemConfig` in the | |
| [v1.8.0-rc.4](https://github.com/ethereum-optimism/optimism/tree/v1.8.0-rc.4) release. | |
| This upgrades the `SystemConfig` in the | |
| [v1.8.0](https://github.com/ethereum-optimism/optimism/tree/v1.8.0) release. |
There was a problem hiding this comment.
I agree that passing governance process is a prompt for us finalizing the release. We have some options on the ordering of next steps however:
- We can (should) make the final tag now in any case.
- We can update the superchain registry TOML files with the finalized tag, either
a. by adding a new release tostandard-versions-mainnet.tomland keeping the existingv1.8.0-rc.4entry. We would then want to go back and remove the latter when all tasks referencing it have been executed.
b. by modifying the existing release but waiting to do that until all tasks including this one are executed
c. by modifying the existing release and modifying any existing tasks that reference it.
Curious what you think makes the most sense? I'd prefer if we can settle on a flow which is as clear and consistent as possible given we will go through this on each and every hard fork / upgrade. I'm inclined towards option b.
|
|
||
| ## Pre-deployments | ||
|
|
||
| - `SystemConfig` - `0x33b83E4C305c908B2Fc181dDa36e230213058d7d` |
There was a problem hiding this comment.
Any messages you can link to so I can verify this address?
There was a problem hiding this comment.
This is actually the same address from the previous (incorrect) upgrade
.There was a problem hiding this comment.
See also ethereum-optimism/optimism#13009
tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/README.md
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,85 @@ | |||
| # Validation | |||
There was a problem hiding this comment.
I have not yet validated this file or run the simulation
…ain/README.md Co-authored-by: Matt Solomon <matt@mattsolomon.dev>
TODO
020to signify that it was incorrect and that this task supercedes itCloses #394