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

op-contracts: Add script to upgrade from 1.8.0 L2OO to 1.8.0 permissioned. #13315

Open
wants to merge 19 commits into
base: proposal/op-contracts/v1.8.0
Choose a base branch
from

Conversation

ajsutton
Copy link
Contributor

@ajsutton ajsutton commented Dec 9, 2024

Description

Adds an upgrade package to transition from 1.8.0 in L2OO configuration to 1.8.0 in permissioned configuration. This is based on the 1.3.0->1.6.0-permissioned docker image with minor changes to handle deploying the correct version of contracts like AnchorStateRegistry which was not released as part of 1.8.0, so needs to be deployed via a blueprint of the 1.6.0 implementation.

A runbook using this is added in ethereum-optimism/superchain-ops#393

Metadata

Part of #12990

@ajsutton ajsutton marked this pull request as ready for review December 9, 2024 04:36
@ajsutton ajsutton requested a review from a team as a code owner December 9, 2024 04:36
@ajsutton ajsutton requested review from refcell and removed request for a team December 9, 2024 04:36
@ajsutton
Copy link
Contributor Author

ajsutton commented Dec 9, 2024

Simulated execution of upgrade on sepolia devnet: https://dashboard.tenderly.co/public/safe/safe-apps/simulator/682a6103-b284-427b-8b80-a4ba29f02f75/state-diff
It's not a particularly realistic case as it's already running permissionless proofs but useful to check things. There's an unexpected change in SystemConfig though with key 0x0000000000000000000000000000000000000000000000000000000000000066 being zeroed. That's the scalar which is complicated by an unreleased Ecotone change to the contracts which is now shipping with Holocene. I'm guessing the upgrade didn't reinitialise SystemConfig and it's not possible anymore to reinitialise it without changing the scalar. It may be better to not try and reinitialise since we're making the changes we need via StorageSetter anyway.

Copy link
Contributor

@Inphi Inphi left a comment

Choose a reason for hiding this comment

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

The DeployUpgrade script looks good to me.

Could you add a README explaining how to use the upgrade scripts.

# We need to re-generate the SystemConfig initialization call
# We want to use the exact same values that the SystemConfig is already using
SYSTEM_CONFIG_OWNER=$(cast call "$SYSTEM_CONFIG_PROXY" "owner()")
SYSTEM_CONFIG_BASE_FEE_SCALAR=$(cast call "$SYSTEM_CONFIG_PROXY" "basefeeScalar()")
Copy link
Contributor

@tynes tynes Dec 9, 2024

Choose a reason for hiding this comment

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

This is where the bug is. I believe that you need to call scalar() and then assert its on version 1 (most significant byte is 0x01) then decode it with the inverse of

scalar = (uint256(0x01) << 248) | (uint256(_blobbasefeeScalar) << 32) | _basefeeScalar;

The logic is also here for decoding

Copy link
Contributor

Choose a reason for hiding this comment

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

$ cast call 0xa6b72407e2dc9EBF84b839B69A24C88929cf20F7 "basefeeScalar()"
0x0000000000000000000000000000000000000000000000000000000000000000

Can confirm it does return 0

$ cast call 0xa6b72407e2dc9EBF84b839B69A24C88929cf20F7 "scalar()"
0x010000000000000000000000000000000000000000000000000d273000001db0

this does not return 0, this is the scalar value

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a consistency problem due to new storage slots being introduced for the basefee scalar rather than it being a function that modifies the single source of truth of the scalar

see:

basefeeScalar = _basefeeScalar;
blobbasefeeScalar = _blobbasefeeScalar;

This probably should have been implemented in a way that didn't add new storage slots to prevent this consistency issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the re-initialization of SystemConfig from this task as it assumes that the chain is already running op-contracts/v1.8.0 and the StorageSetter is already being used to zero out the L2OO address and set the DisputeGameFactory address so the initialisation would be a no-op apart from this scalar issue.

How to handle the scalar is being tracked in ethereum-optimism/superchain-ops#394 but that is part of the upgrade to op-contracts/v1.8.0 and this runbook is starting from already running 1.8.0 (just in L2OO config but the SystemConfig should already have been upgraded).

## Usage

For full instructions, including the required preparation and infrastructure changes to perform this upgrade, refer to
the [full runbook](https://github.com/ethereum-optimism/superchain-ops/blob/a8a3f2a1aeda8d916081be3f4e2a8526286faa4d/runbooks/1.8.0-l2oo-to-permissioned-fps.md).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll need to update this link one the runbook is merged.

There's no simple way to re-init with the same values because of the way scalar is handled in setEcotoneGasConfig
We aren't re-initializing so it's important it doesn't change versions. If it does for some reason that needs to be properly investigated.
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.

4 participants