sep/027: upgrade and re-initialize `SystemConfig` #418

geoknee · 2024-12-17T14:36:47Z

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

Closes #394

tasks/sep/026-holocene-system-config-upgrade-and-init-multi-chain/VALIDATION.md

tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/NestedSignFromJson.s.sol

script/verification/Verification.s.sol

tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/OVERVIEW.md

existing task in flight for 025

Closes #423

…ave them, e.g. metal)

…kip only if not found in the TOML file

This reverts commit ef18399.

tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/NestedSignFromJson.s.sol

mds1 · 2025-01-06T22:12:21Z

script/verification/SystemConfigUpgrade.s.sol

+    constructor(string memory l1ChainName, string memory l2ChainName, string memory release)
+        SuperchainRegistry(l1ChainName, l2ChainName, release)


Since l1ChainName and l2ChainName are storage variable names in the SuperchainRegistry contract, using them as argument names in the constructors shadows those variable names

Suggested change

constructor(string memory l1ChainName, string memory l2ChainName, string memory release)

SuperchainRegistry(l1ChainName, l2ChainName, release)

constructor(string memory _l1ChainName, string memory _l2ChainName, string memory _release)

SuperchainRegistry(_l1ChainName, _l1ChainName, _release)

mds1 · 2025-01-06T22:13:57Z

script/verification/SystemConfigUpgrade.s.sol

+import {Simulation} from "@base-contracts/script/universal/Simulation.sol";
+import {NestedMultisigBuilder} from "@base-contracts/script/universal/NestedMultisigBuilder.sol";
+
+contract SystemConfigUpgrade is SuperchainRegistry {


Can you add comments explaining what this contract is supposed to do and when it should be used? Would be good to have explanatory comments in Verification.s.sol as well

mds1 · 2025-01-06T22:15:32Z

script/verification/SystemConfigUpgrade.s.sol

+import {console2 as console} from "forge-std/console2.sol";
+import {Vm} from "forge-std/Vm.sol";
+import {LibString} from "solady/utils/LibString.sol";
+import {SuperchainRegistry} from "script/verification/Verification.s.sol";
+import "@eth-optimism-bedrock/src/dispute/lib/Types.sol";
+import {ISystemConfig} from "./ISystemConfig.sol";
+import {IResourceMetering} from "./IResourceMetering.sol";
+import {MIPS} from "@eth-optimism-bedrock/src/cannon/MIPS.sol";
+import {Simulation} from "@base-contracts/script/universal/Simulation.sol";
+import {NestedMultisigBuilder} from "@base-contracts/script/universal/NestedMultisigBuilder.sol";


removing unused imports

Suggested change

import {console2 as console} from "forge-std/console2.sol";

import {Vm} from "forge-std/Vm.sol";

import {LibString} from "solady/utils/LibString.sol";

import {SuperchainRegistry} from "script/verification/Verification.s.sol";

import "@eth-optimism-bedrock/src/dispute/lib/Types.sol";

import {ISystemConfig} from "./ISystemConfig.sol";

import {IResourceMetering} from "./IResourceMetering.sol";

import {MIPS} from "@eth-optimism-bedrock/src/cannon/MIPS.sol";

import {Simulation} from "@base-contracts/script/universal/Simulation.sol";

import {NestedMultisigBuilder} from "@base-contracts/script/universal/NestedMultisigBuilder.sol";

import {LibString} from "solady/utils/LibString.sol";

import {SuperchainRegistry} from "script/verification/Verification.s.sol";

import {ISystemConfig} from "./ISystemConfig.sol";

import {IResourceMetering} from "./IResourceMetering.sol";

mds1 · 2025-01-06T22:19:24Z

script/verification/SystemConfigUpgrade.s.sol

+        address[] memory exceptions = new address[](4);
+        exceptions[0] = previous.owner; // NOTE this can be removed for mainnet
+        exceptions[1] = address(uint160(uint256((previous.batcherHash))));
+        exceptions[2] = previous.unsafeBlockSigner;
+        exceptions[3] = previous.batchInbox;


We can make this conditional based off chain ID to remove the need for the note (untested code, please review/test it before applying suggestion)

Suggested change

address[] memory exceptions = new address[](4);

exceptions[0] = previous.owner; // NOTE this can be removed for mainnet

exceptions[1] = address(uint160(uint256((previous.batcherHash))));

exceptions[2] = previous.unsafeBlockSigner;

exceptions[3] = previous.batchInbox;

uint256 len = block.chainid == 1 ? 3 : 4; // Mainnet doesn't need owner exception.

address[] memory exceptions = new address[](len);

uint256 i = 0;

exceptions[i++] = address(uint160(uint256((previous.batcherHash))));

exceptions[i++] = previous.unsafeBlockSigner;

exceptions[i++] = previous.batchInbox;

if (block.chainid != 1) exceptions[i++] = previous.owner;

mds1 · 2025-01-06T22:21:04Z

script/verification/SystemConfigUpgradeEcotoneScalars.s.sol

+    constructor(string memory l1ChainName, string memory l2ChainName, string memory release)
+        SystemConfigUpgrade(l1ChainName, l2ChainName, release)


same comment on variable shadowing

Suggested change

constructor(string memory l1ChainName, string memory l2ChainName, string memory release)

SystemConfigUpgrade(l1ChainName, l2ChainName, release)

constructor(string memory _l1ChainName, string memory _l2ChainName, string memory _release)

SystemConfigUpgrade(_l1ChainName, _l2ChainName, _release)

mds1 · 2025-01-06T22:21:14Z

script/verification/SystemConfigUpgradeEcotoneScalars.s.sol

+import {console2 as console} from "forge-std/console2.sol";
+import {Vm} from "forge-std/Vm.sol";
+import {LibString} from "solady/utils/LibString.sol";
+import {SuperchainRegistry} from "script/verification/Verification.s.sol";
+import "@eth-optimism-bedrock/src/dispute/lib/Types.sol";
+import {ISystemConfig} from "./ISystemConfig.sol";
+import {IResourceMetering} from "./IResourceMetering.sol";
+import {MIPS} from "@eth-optimism-bedrock/src/cannon/MIPS.sol";
+import {Simulation} from "@base-contracts/script/universal/Simulation.sol";
+import {NestedMultisigBuilder} from "@base-contracts/script/universal/NestedMultisigBuilder.sol";


removing unused imports

Suggested change

import {console2 as console} from "forge-std/console2.sol";

import {Vm} from "forge-std/Vm.sol";

import {LibString} from "solady/utils/LibString.sol";

import {SuperchainRegistry} from "script/verification/Verification.s.sol";

import "@eth-optimism-bedrock/src/dispute/lib/Types.sol";

import {ISystemConfig} from "./ISystemConfig.sol";

import {IResourceMetering} from "./IResourceMetering.sol";

import {MIPS} from "@eth-optimism-bedrock/src/cannon/MIPS.sol";

import {Simulation} from "@base-contracts/script/universal/Simulation.sol";

import {NestedMultisigBuilder} from "@base-contracts/script/universal/NestedMultisigBuilder.sol";

import {console2 as console} from "forge-std/console2.sol";

import {LibString} from "solady/utils/LibString.sol";

import {ISystemConfig} from "./ISystemConfig.sol";

mds1 · 2025-01-06T22:21:50Z

script/verification/SystemConfigUpgradeEcotoneScalars.s.sol

+import {NestedMultisigBuilder} from "@base-contracts/script/universal/NestedMultisigBuilder.sol";
+import {SystemConfigUpgrade} from "script/verification/SystemConfigUpgrade.s.sol";
+
+contract SystemConfigUpgradeEcotoneScalars is SystemConfigUpgrade {


Same comment about adding explanatory code comments

mds1 · 2025-01-06T22:22:35Z

script/verification/SystemConfigUpgradeEcotoneScalars.s.sol

+        uint256 reencodedScalar =
+            (uint256(0x01) << 248) | (uint256(sysCfg.blobbasefeeScalar()) << 32) | sysCfg.basefeeScalar();


Is this only needed temporarily since we are upgrading the SystemConfig to a version that no longer needs encoding?

mds1 · 2025-01-06T22:25:09Z

script/verification/SystemConfigUpgradeEcotoneScalars.s.sol

+        if (
+            uint8(previous.scalar >> 248) == 1 // most significant bit
+        ) {


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

mds1 · 2025-01-06T22:29:02Z

script/verification/Verification.s.sol

+    function tryReadAddress(string memory toml, string memory key) internal pure returns (address) {
+        try vmSafe.parseTomlAddress(toml, key) returns (address a) {
+            return a;
+        } catch {
+            console.log("failed to read address for ", key);
+            return address(0);
+        }
+    }


We can delete this, update our forge-std version to the latest release, and instead use: https://github.com/foundry-rs/forge-std/blob/b5a86914561f38735ef1fc357685de3e7c92dc48/src/StdToml.sol#L148

You can either use it like this:

stdToml.readAddressOr(toml, "$.addresses.AnchorStateRegistryProxy", address(0));

or like this, similar to LibString

using StdToml for string; // --- snip --- toml.readAddressOr("$.addresses.AnchorStateRegistryProxy", address(0));

mds1 · 2025-01-06T22:30:24Z

tasks/sep/020-holocene-system-config-upgrade-multi-chain/README.md

@@ -2,6 +2,8 @@

 Status: [EXECUTED](https://sepolia.etherscan.io/tx/0x88bd1d85740af3741e2ed96d6fd07f2abb4541afc667625480bf6a28451c4d6d)

+> ⚠️ **Warning:** This task is incorrect and is superceded by [Task 027](../027-holocene-system-config-upgrade-and-init-multi-chain/)


nit (according to my spellchecker extension, I also thought your spelling was correct)

Suggested change

> ⚠️ **Warning:** This task is incorrect and is superceded by [Task 027](../027-holocene-system-config-upgrade-and-init-multi-chain/)

> ⚠️ **Warning:** This task is incorrect and is superseded by [Task 027](../027-holocene-system-config-upgrade-and-init-multi-chain/)

mds1 · 2025-01-06T22:32:01Z

tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/NestedSignFromJson.s.sol

+
+contract NestedSignFromJson is OriginalNestedSignFromJson, CouncilFoundationNestedSign {
+    string constant l1ChainName = "sepolia";
+    string constant release = "v1.8.0-rc.4";


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

Suggested change

string constant release = "v1.8.0-rc.4";

string constant release = "v1.8.0";

mds1 · 2025-01-06T22:36:44Z

tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/OVERVIEW.md

I'd suggest deleting this file since we don't need it, just one less thing to review

mds1 · 2025-01-06T22:37:15Z

tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/README.md

@@ -0,0 +1,30 @@
+# Holocene Hardfork Upgrade - `SystemConfig`
+
+Status: DRAFT, NOT READY TO SIGN


what do we need for this to become ready to sign?

Suggested change

Status: DRAFT, NOT READY TO SIGN

Status: DRAFT, NOT READY TO SIGN

mds1 · 2025-01-06T22:39:23Z

tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/README.md

+This upgrades the `SystemConfig` in the
+[v1.8.0-rc.4](https://github.com/ethereum-optimism/optimism/tree/v1.8.0-rc.4) release.


we should just use 1.8.0 now that it's passed

Suggested change

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.

mds1 · 2025-01-06T22:41:17Z

tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/README.md

+
+## Pre-deployments
+
+- `SystemConfig` - `0x33b83E4C305c908B2Fc181dDa36e230213058d7d`


Any messages you can link to so I can verify this address?

mds1 · 2025-01-06T22:42:01Z

tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/README.md

+## Simulation
+
+Please see the "Simulating and Verifying the Transaction" instructions in [NESTED.md](../../../NESTED.md).
+When simulating, ensure the logs say `Using script /your/path/to/superchain-ops/tasks/<path>/NestedSignFromJson.s.sol`.


Suggested change

When simulating, ensure the logs say `Using script /your/path/to/superchain-ops/tasks/<path>/NestedSignFromJson.s.sol`.

When simulating, ensure the logs say `Using script /your/path/to/superchain-ops/tasks/027-holocene-system-config-upgrade-and-init-multi-chain/NestedSignFromJson.s.sol`.

mds1 · 2025-01-06T22:42:37Z

tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/VALIDATION.md

@@ -0,0 +1,85 @@
+# Validation


I have not yet validated this file or run the simulation

geoknee · 2025-01-07T11:40:04Z

script/verification/SystemConfigUpgradeEcotoneScalars.s.sol

+            require(reencodedScalar == previous.scalar, "scalar-100 (reencoding produced incorrect result)");
+            // We do this check last because it would fail if the scalar is wrong, and we get less debug info from it.
+            // It checks all of the other fields which should not have changed (via a hash).
+            super.checkSystemConfigUpgrade();


I think we should call this unconditionally, and pull the scalar check out of the base contract into this one.

geoknee changed the title ~~first pass at task sep 025~~ Sep 026: upgrade and re-initialize SystemConfig Dec 17, 2024

geoknee mentioned this pull request Dec 17, 2024

Fix Holocene SystemConfig upgrade scripts ethereum-optimism/optimism#13412

Merged

3 tasks

geoknee commented Dec 17, 2024

View reviewed changes

tasks/sep/026-holocene-system-config-upgrade-and-init-multi-chain/VALIDATION.md Outdated Show resolved Hide resolved

geoknee changed the title ~~Sep 026: upgrade and re-initialize SystemConfig~~ Sep 027: upgrade and re-initialize SystemConfig Dec 18, 2024

geoknee force-pushed the gk/025-holocene-sys-cfg branch 2 times, most recently from 50aeb48 to 2e881b4 Compare December 19, 2024 14:58

sebastianst reviewed Dec 19, 2024

View reviewed changes

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

geoknee commented Jan 3, 2025

View reviewed changes

script/verification/Verification.s.sol Outdated Show resolved Hide resolved

geoknee commented Jan 3, 2025

View reviewed changes

tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/OVERVIEW.md Outdated Show resolved Hide resolved

geoknee added 21 commits January 4, 2025 00:07

first pass at task sep 025

e5623ea

bump task index

14500b8

existing task in flight for 025

undo change to submodule

9c83d58

add exceptions for EOAs in SystemConfig storage

cc782b3

bump task sequence number

dcd607c

change validation script to use Seb's new framework

c04293c

addAllowedStorageAccess for each system config

eb21f18

SystemConfigUpgrade exposes code exceptions thru getter

4014438

introduce SystemConfigUpgradeEcotoneScalars

fcda94c

add deprecation notice to task 020

d130513

remove unecessary forge build commands

ef18399

Closes #423

add some logging

35b4718

reactivate all chains

1355535

update to superbundle

bb163d9

TEMP: do not try to load all contract addresses (some chains do not h…

0b39283

…ave them, e.g. metal)

improve err msg

85aac3a

fix superbundle

ca3699e

use dynamic storage array

7b7f0d1

validate in a smarter way for chains migrating from pre ecotone scalars

80aa1cc

SuperchainRegistry: instead of skipping missing addresses globally, s…

7e0c7fe

…kip only if not found in the TOML file

update overview and readme

127d370

geoknee requested review from a team as code owners January 4, 2025 00:23

geoknee requested review from smartcontracts and Ethnical January 4, 2025 00:23

geoknee added 3 commits January 6, 2025 18:00

undo changes to submodule

e568b3a

undo change to submodule

35c68f8

Revert "remove unecessary forge build commands"

94a0e19

This reverts commit ef18399.

geoknee commented Jan 6, 2025

View reviewed changes

tasks/sep/027-holocene-system-config-upgrade-and-init-multi-chain/NestedSignFromJson.s.sol Show resolved Hide resolved

mds1 reviewed Jan 6, 2025

View reviewed changes

geoknee added 3 commits January 7, 2025 11:23

avoid shadowing variables

5098753

add explanatory comment and removed unused imports

ab72af4

avoid shadowing variabls

ddad3df

geoknee commented Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sep/027: upgrade and re-initialize `SystemConfig` #418

sep/027: upgrade and re-initialize `SystemConfig` #418

geoknee commented Dec 17, 2024 •

edited

Loading

mds1 Jan 6, 2025

geoknee Jan 7, 2025

mds1 Jan 6, 2025

geoknee Jan 7, 2025

mds1 Jan 6, 2025

geoknee Jan 7, 2025

mds1 Jan 6, 2025

mds1 Jan 6, 2025

geoknee Jan 7, 2025

mds1 Jan 6, 2025 •

edited

Loading

geoknee Jan 7, 2025

mds1 Jan 6, 2025

geoknee Jan 7, 2025

mds1 Jan 6, 2025

mds1 Jan 6, 2025

mds1 Jan 6, 2025

mds1 Jan 6, 2025

mds1 Jan 6, 2025

mds1 Jan 6, 2025

mds1 Jan 6, 2025

mds1 Jan 6, 2025

mds1 Jan 6, 2025

mds1 Jan 6, 2025

mds1 Jan 6, 2025

geoknee Jan 7, 2025

		constructor(string memory l1ChainName, string memory l2ChainName, string memory release)
		SuperchainRegistry(l1ChainName, l2ChainName, release)

-        address[] memory exceptions = new address[](4);
-        exceptions[0] = previous.owner; // NOTE this can be removed for mainnet
-        exceptions[1] = address(uint160(uint256((previous.batcherHash))));
-        exceptions[2] = previous.unsafeBlockSigner;
-        exceptions[3] = previous.batchInbox;
+        uint256 len = block.chainid == 1 ? 3 : 4; // Mainnet doesn't need owner exception.
+        address[] memory exceptions = new address[](len);
+        uint256 i = 0;
+        exceptions[i++] = address(uint160(uint256((previous.batcherHash))));
+        exceptions[i++] = previous.unsafeBlockSigner;
+        exceptions[i++] = previous.batchInbox;
+        if (block.chainid != 1) exceptions[i++] = previous.owner;

		constructor(string memory l1ChainName, string memory l2ChainName, string memory release)
		SystemConfigUpgrade(l1ChainName, l2ChainName, release)

		uint256 reencodedScalar =
		(uint256(0x01) << 248) \| (uint256(sysCfg.blobbasefeeScalar()) << 32) \| sysCfg.basefeeScalar();

		@@ -2,6 +2,8 @@

		Status: [EXECUTED](https://sepolia.etherscan.io/tx/0x88bd1d85740af3741e2ed96d6fd07f2abb4541afc667625480bf6a28451c4d6d)

		> ⚠️ Warning: This task is incorrect and is superceded by [Task 027](../027-holocene-system-config-upgrade-and-init-multi-chain/)

	string constant release = "v1.8.0-rc.4";
	string constant release = "v1.8.0";

		@@ -0,0 +1,30 @@
		# Holocene Hardfork Upgrade - `SystemConfig`

		Status: DRAFT, NOT READY TO SIGN

		This upgrades the `SystemConfig` in the
		[v1.8.0-rc.4](https://github.com/ethereum-optimism/optimism/tree/v1.8.0-rc.4) release.


		## Pre-deployments

		- `SystemConfig` - `0x33b83E4C305c908B2Fc181dDa36e230213058d7d`

	When simulating, ensure the logs say `Using script /your/path/to/superchain-ops/tasks/<path>/NestedSignFromJson.s.sol`.
	When simulating, ensure the logs say `Using script /your/path/to/superchain-ops/tasks/027-holocene-system-config-upgrade-and-init-multi-chain/NestedSignFromJson.s.sol`.

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

Are you sure you want to change the base?

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

Conversation

geoknee commented Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mds1 Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sep/027: upgrade and re-initialize `SystemConfig` #418

sep/027: upgrade and re-initialize `SystemConfig` #418

geoknee commented Dec 17, 2024 •

edited

Loading

mds1 Jan 6, 2025 •

edited

Loading