tasks(sep/base): add Base Sepolia MCP L1 playbook

sbvegan commented 1 month ago

This PR adds the Base Sepolia MCP L1 upgrade task.

Review instructions:

Follow this folder's instructions to execute playbook and make sure it does not revert
Verify:
1. That the Validations file is exactly what you see on Tenderly
2. That the Validations file is consistent with what we'd expect via our notion analysis
3. That the hardcoded values in ./tasks/sep/base-001-MCP-L1/SignFromJson.s.sol are all accurate and sensible
Diff the new tasks/sep/metal-001-MCP-L1/SignFromJson.s.sol against the existing tasks/sep/base-001-MCP-L1/SignFromJson.s.sol to better see the changes, since most of the file is a copy/paste of that.

sbvegan commented 4 weeks ago

Description

We’re putting together Base Sepolia’s MCP upgrade transaction and during the validation of the Safe bundle there were unexpected storage layout changes. This is a point where we want to pause and take a close look at the changes to make sure we're confident in the expected results of the upgrade. The following points can be used to ensure the storage layout changes are a non-issue:

Check what version Base is on, and compare it with the MCP version
- If storage variables (not immutables or constants) were only appended, layout is compatible
- If storage variables were changed in other ways, make sure the initialize function properly re-sets all variables
Perform the same check for contracts that each contract inherits from. For these contracts, appending is only safe if the __gap size shrinks by the amount of storage variables added.

L1 Contract Comparison

The following l1 contracts are changing in this upgrade. They’ve been pulled from the `VALIDATION.md file, which is what the Tenderly simulation UI shows from the safe bundle simulated state changes.

Relevant Commits

MCP Release Implementation: https://github.com/ethereum-optimism/optimism/commit/e6ef3a900c42c8722e72c2e2314027f85d12ced5
- Commit: e6ef3a900c42c8722e72c2e2314027f85d12ced5

Deployment commit compared with MCP

Base Sepolia’s Deployment Implementation: https://github.com/ethereum-optimism/optimism/commit/a7ff5a811612fa338d0a6d6dd72dc2ec9badef6d
- Self reported commit: a7ff5a811612fa338d0a6d6dd72dc2ec9badef6d
  - https://github.com/base-org/contract-deployments/blob/main/sepolia/2023-09-26-deploy/.env
Diff of Base Deploy Commit & MCP Commit: https://github.com/ethereum-optimism/optimism/compare/a7ff5a811612fa338d0a6d6dd72dc2ec9badef6d...e6ef3a900c42c8722e72c2e2314027f85d12ced5

L1StandardBridge upgrade commit compared with MCP

Their L1StandardBridge implementation was upgraded after initial deployment
- Current Implementation: https://github.com/ethereum-optimism/optimism/commit/e3ba24e72085d85bb5584dda33a03ccf60db86f0
  - Self reported commit: e3ba24e72085d85bb5584dda33a03ccf60db86f0
    - https://github.com/base-org/contract-deployments/blob/main/sepolia/2023-10-07-upgrade-l1-standard-bridge/.env
Diff of Upgrade Commit & MCP Commit: https://github.com/ethereum-optimism/optimism/compare/e3ba24e72085d85bb5584dda33a03ccf60db86f0...e6ef3a900c42c8722e72c2e2314027f85d12ced5

Note: The GitHub commit diff tool doesn’t allow you to compare on the file level, so I just searched the contract name in the Files changed tab.

`0x21eFD066e581FA55Ef105170Cc04d74386a09190` (`L1ERC721BridgeProxy`)

Links:

Diff: https://github.com/ethereum-optimism/optimism/compare/a7ff5a811612fa338d0a6d6dd72dc2ec9badef6d...e6ef3a900c42c8722e72c2e2314027f85d12ced5

packages/contracts-bedrock/src/L1/L1ERC721Bridge.sol
- No storage variable changes
packages/contracts-bedrock/snapshots/storageLayout/L1ERC721Bridge.json
- Did not exist at deployment commit
packages/contracts-bedrock/deployments/mainnet/L1ERC721Bridge.json
packages/contracts-bedrock/deployments/sepolia/L1ERC721Bridge.json

`0x49f53e41452c74589e85ca1677426ba426459e85` (`OptimismPortalProxy`)

Links:

Diff: https://github.com/ethereum-optimism/optimism/compare/a7ff5a811612fa338d0a6d6dd72dc2ec9badef6d...e6ef3a900c42c8722e72c2e2314027f85d12ced5

packages/contracts-bedrock/src/L1/OptimismPortal.sol

reinitializer(2) to initializer
guardian removed

/// @notice Address that has the ability to pause and unpause withdrawals.
/// @custom:network-specific
address public guardian;

spacer and superchainConfig added

/// @custom:legacy
/// @custom:spacer paused
/// @notice Spacer for backwards compatibility.
bool private spacer_53_0_1;

/// @notice Contract of the Superchain Config.
SuperchainConfig public superchainConfig;

`0x709c2B8ef4A9feFc629A8a2C1AF424Dc5BD6ad1B` (`AddressManager`)

Links:

Diff:

packages/contracts-bedrock/scripts/interfaces/IAddressManager.sol
- Implementation doesn’t exist for this legacy contract
packages/contracts-bedrock/snapshots/abi/AddressManager.json
- Did not exist at deployment

`0x84457ca9d0163fbc4bbfe4dfbb20ba46e48df254` (`L2OutputOracleProxy`)

Links:

Diff:

packages/contracts-bedrock/src/L1/L2OutputOracle.sol

SUBMISSION_INTERVAL, L2_BLOCK_TIME, and FINALIZATION_PERIOD_SECONDS immutables were removed

/// @notice The interval in L2 blocks at which checkpoints must be submitted.
///         Although this is immutable, it can safely be modified by upgrading the
///         implementation contract.
///         Public getter is legacy and will be removed in the future. Use `submissionInterval`
///         instead.
/// @custom:legacy
uint256 public immutable SUBMISSION_INTERVAL;

/// @notice The time between L2 blocks in seconds. Once set, this value MUST NOT be modified.
///         Public getter is legacy and will be removed in the future. Use `l2BlockTime`
///         instead.
/// @custom:legacy
uint256 public immutable L2_BLOCK_TIME;

/// @notice The minimum time (in seconds) that must elapse before a withdrawal can be finalized.
///         Public getter is legacy and will be removed in the future. Use
//          `finalizationPeriodSeconds` instead.
/// @custom:legacy
uint256 public immutable FINALIZATION_PERIOD_SECONDS;

submissionInterval, l2BlockTime, and finalizationPeriodSeconds state variables added. They were not just appended, the proposer and challenger sit in between.

    /// @notice The interval in L2 blocks at which checkpoints must be submitted.
/// @custom:network-specific
uint256 public submissionInterval;

/// @notice The time between L2 blocks in seconds. Once set, this value MUST NOT be modified.
/// @custom:network-specific
uint256 public l2BlockTime;

/// @notice The address of the challenger. Can be updated via upgrade.
/// @custom:network-specific
address public challenger;

/// @notice The address of the proposer. Can be updated via upgrade.
/// @custom:network-specific
address public proposer;

/// @notice The minimum time (in seconds) that must elapse before a withdrawal can be finalized.
/// @custom:network-specific
uint256 public finalizationPeriodSeconds;

reinitializer(2) to initializer

packages/contracts-bedrock/deployments/mainnet/L2OutputOracle.json

`0xb1efB9650aD6d0CC1ed3Ac4a0B7f1D5732696D37` (`OptimismMintableERC20FactoryProxy`)

Links:

Diff:

packages/contracts-bedrock/src/universal/OptimismMintableERC20Factory.sol

spacer and gap added

    /// @custom:spacer OptimismMintableERC20Factory's initializer slot spacing
/// @notice Spacer to avoid packing into the initializer slot
bytes30 private spacer_0_2_30;

/// @notice Address of the StandardBridge on this chain.
/// @custom:network-specific
address public bridge;

/// @notice Reserve extra slots in the storage layout for future upgrades.
///         A gap size of 49 was chosen here, so that the first slot used in a child contract
///         would be 1 plus a multiple of 50.
uint256[49] private __gap;

reinitializer(2) to initializer

Contracts this inherits from
- packages/contracts-bedrock/src/universal/Semver.sol
  - deleted and the ISemver.sol interface didn’t appear, so it was unchanged
- packages/contracts-bedrock/src/dispute/interfaces/IInitializable.sol
  - initialize() was changed from external to payable
  - Initializable.sol did not appear, so it was unchanged

`0x5D335Aa7d93102110879e3B54985c5F08146091E` (`L1CrossDomainMessengerProxy`)

Links:

Diff:

packages/contracts-bedrock/src/L1/L1CrossDomainMessenger.sol

PORTAL removed

/// @custom:network-specific
/// @custom:legacy
OptimismPortal public PORTAL;

superchainConfig and portal added

/// @notice Contract of the SuperchainConfig.
SuperchainConfig public superchainConfig;

/// @notice Contract of the OptimismPortal.
/// @custom:network-specific
OptimismPortal public portal;

reinitializer(2) to initializer

`0xf272670eb55e895584501d564AfEB048bEd26194` (`SystemConfigProxy`)

Links:

Diff:

packages/contracts-bedrock/src/L1/SystemConfig.sol
- startBlock removed
```
/// @notice The block at which the op-node can start searching for logs from.
uint256 public startBlock;
```
- Only constants added
- reinitializer(2) to initializer

`0xfd0Bf71F60660E2f608ed56e1659C450eB113120` (`L1StandardBridgeProxy`)

Links:

Commits:

Diff:

packages/contracts-bedrock/src/L1/L1StandardBridge.sol

superchainConfig added

/// @notice Address of the SuperchainConfig contract.
SuperchainConfig public superchainConfig;

sbvegan commented 3 weeks ago

After review, all of these state changes look safe and expected. There is one specific call out from taking a close look at the implementation contracts. In regard to the SystemConfig, similar to OptimismPortal, the removed variable does not have a storage setter call meaning we’ll have a dangling storage value. Unlike OptimismPortal, no future upgrade would clear this. Having this dangling storage poses no risk currently, but is just a bit dirty and is possible (but unlikely) to be an issue in the future if we forget about it.

For example, we add a slot to SystemConfig that we intend to leave at 0 during deployment, and for Base this value would not be zero. In reality we should explicitly initialize to zero in this case.
Interestingly, Base Mainnet SystemConfig version (1.3.0) is older than their sepolia one, and is one we used in production, and therefore does not have this same risk. This lends itself towards the “leave as-is” category.

Safe now, but may be unsafe in the future, for Sepolia only. We could manually add an additional storage setter to the input.json transaction bundle to clear this. However this can always be cleared in a separate, future, dedicated “upgrade” that Base does, which is probably the best approach.

ethereum-optimism / superchain-ops