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

sepolia-019: Holocene FP upgrade #374

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

Conversation

sebastianst
Copy link
Member

@sebastianst sebastianst commented Nov 22, 2024

Description

OP Sepolia Holocene FP upgrade - sets new FDG and PDG implementations.

Tests

Verifications added for the new DGs, and correct MIPS vm.

Towards ethereum-optimism/optimism#13033

clabby
clabby previously approved these changes Nov 22, 2024
Copy link
Member

@clabby clabby left a comment

Choose a reason for hiding this comment

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

Nicely done. Ran through the simulation and checked the deployments, looks good to me.

tasks/sep/019-fp-holocene-upgrade/NestedSignFromJson.s.sol Outdated Show resolved Hide resolved
@@ -36,6 +36,8 @@ is ready".

Make sure your ledger is still unlocked and run the following commands depending on which Safe you are signing on behalf of.

Note: during development of the Solidity script, simulations can be run without any Ledger by exporting `SIMULATE_WITHOUT_LEDGER=1` in your shell, or by adding it to the `.env` file.
Copy link
Member

Choose a reason for hiding this comment

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

nice, thanks for documenting this :)


DisputeGameFactory constant dgfProxy = DisputeGameFactory(0x05F9613aDB30026FFd634f38e5C4dFd30a197Fa1);
address constant livenessGuard = 0xc26977310bC89DAee5823C2e2a73195E85382cC7;
bytes32 constant absolutePrestate = 0x03925193e3e89f87835bbdf3a813f60b2aa818a36bbe71cd5d8fd7e79f5e8afe;
Copy link
Member

Choose a reason for hiding this comment

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

Confirmed this is the correct prestate for the op-program/v1.4.0-rc.1 tag.

…SignFromJson.s.sol

Co-authored-by: clabby <ben@clab.by>
Copy link
Member

@clabby clabby left a comment

Choose a reason for hiding this comment

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

re-approval after the suggestion was added

@@ -0,0 +1,6 @@
ETH_RPC_URL="https://ethereum-sepolia.publicnode.com"
SUPERCHAIN_CONFIG_ADDR=0xC2Be75506d5724086DEB7245bd260Cc9753911Be
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 used

Suggested change
SUPERCHAIN_CONFIG_ADDR=0xC2Be75506d5724086DEB7245bd260Cc9753911Be


## Objective

Upgrades the Fault Proof contracts of OP Seplia for the Holocene hardfork.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Upgrades the Fault Proof contracts of OP Seplia for the Holocene hardfork.
Upgrades the Fault Proof contracts of OP Sepolia for the Holocene hardfork.

Comment on lines +11 to +12
This upgrades the Fault Proof contracts in the
[op-contracts/v1.8.0-rc.2](https://github.com/ethereum-optimism/optimism/tree/op-contracts/v1.8.0-rc.2) release.
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +16 to +18
- `MIPS` - `0x69470D6970Cd2A006b84B1d4d70179c892cFCE01`
- `FaultDisputeGame` - `0x5e0877a8F6692eD470013e651c4357d0C4941e6C`
- `PermissionedDisputeGame` - `0x4Ed046e66c96600DaE1a4ec39267bB0cE476E8cc`
Copy link
Contributor

Choose a reason for hiding this comment

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

Any way I can verify these are the correct addresses, e.g. is there a superchain registry PR?

allowed[4] = livenessGuard;
}

function _postCheck(Vm.AccountAccess[] memory accesses, Simulation.Payload memory) internal override {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function _postCheck(Vm.AccountAccess[] memory accesses, Simulation.Payload memory) internal override {
function _postCheck(Vm.AccountAccess[] memory accesses, Simulation.Payload memory) internal view override {

- All addresses (in section headers and storage values) match the provided name, using the Etherscan and Superchain Registry links provided. This validates the bytecode deployed at the addresses contains the correct logic.
- All key values match the semantic meaning provided, which can be validated using the storage layout links provided.

## State Changes
Copy link
Contributor

Choose a reason for hiding this comment

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

Validation is missing the state overrides section. Let's make sure to add this so signers (especially on mainnet) understand why the state overrides are present. You can pull these from another nested ceremony's validations, they are always identical

- All key values match the semantic meaning provided, which can be validated using the storage layout links provided.

## State Changes

Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to call out the:

  • The nonce increments that happen
  • The approvedHashes mapping being set

Similar to overrides, check out other validation files for a template. The nonce increment can be a bullet point explaining each, the approved hashes is a bit more involved as you can see

- **Key**: `0xffdfc1249c027f9191656349feb0761381bb32c9f557e01f419fd08754bf5a1b` <br/>
**Before**: `0x000000000000000000000000d9d616e4a03a8e7cc962396c9f8d4e3d306097d3` <br/>
**After**: `0x0000000000000000000000005e0877a8f6692ed470013e651c4357d0c4941e6c` <br/>
**Meaning**: Updates the CANNON game type implementation. Verify that the new implementation is set using `cast call 0x05F9613aDB30026FFd634f38e5C4dFd30a197Fa1 "gameImpls(uint32)(address)" 0`. Where `0` is the [`CANNON` game type](https://github.com/ethereum-optimism/optimism/blob/op-contracts/v1.4.0/packages/contracts-bedrock/src/dispute/lib/Types.sol#L28).
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, let's be a little more explicit here, and same for the below storage change as well

Suggested change
**Meaning**: Updates the CANNON game type implementation. Verify that the new implementation is set using `cast call 0x05F9613aDB30026FFd634f38e5C4dFd30a197Fa1 "gameImpls(uint32)(address)" 0`. Where `0` is the [`CANNON` game type](https://github.com/ethereum-optimism/optimism/blob/op-contracts/v1.4.0/packages/contracts-bedrock/src/dispute/lib/Types.sol#L28).
**Meaning**: Updates the CANNON game type implementation. Verify that the new implementation is set using `cast call 0x05F9613aDB30026FFd634f38e5C4dFd30a197Fa1 "gameImpls(uint32)(address)" 0`. Where `0` is the [`CANNON` game type](https://github.com/ethereum-optimism/optimism/blob/op-contracts/v1.4.0/packages/contracts-bedrock/src/dispute/lib/Types.sol#L28). Upon executing this call you will see the returned address is `0xD9d616E4a03a8e7cC962396C9f8D4e3d306097D3`, matching the "Before" value of this slot, demonstrating this slot storing the address of the cannon implementation

Comment on lines +186 to +191
just install
cd tasks/sep/019-fp-holocene-upgrade
SIMULATE_WITHOUT_LEDGER=true just \
--dotenv-path $(pwd)/.env \
--justfile ../../../nested.just \
simulate foundation 0
Copy link
Contributor

Choose a reason for hiding this comment

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

in CI we should run it for both council and foundation, however if we get all sigs before this is merged we can change status to [EXECUTED](https://txHashUrl) and remove this task from CI

Suggested change
just install
cd tasks/sep/019-fp-holocene-upgrade
SIMULATE_WITHOUT_LEDGER=true just \
--dotenv-path $(pwd)/.env \
--justfile ../../../nested.just \
simulate foundation 0
just install
cd tasks/sep/019-fp-holocene-upgrade
SIMULATE_WITHOUT_LEDGER=true just \
--dotenv-path $(pwd)/.env \
--justfile ../../../nested.just \
simulate foundation 0
SIMULATE_WITHOUT_LEDGER=true just \
--dotenv-path $(pwd)/.env \
--justfile ../../../nested.just \
simulate council 0

- The following state changes (and none others) are made to that contract. This validates that no unexpected state changes occur.
- All addresses (in section headers and storage values) match the provided name, using the Etherscan and Superchain Registry links provided. This validates the bytecode deployed at the addresses contains the correct logic.
- All key values match the semantic meaning provided, which can be validated using the storage layout links provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

When simulating as the council, there will be liveness guard state changes that we need to mention here (can similarly reference a prior validations file as a template)

using LibString for string;

DisputeGameFactory constant dgfProxy = DisputeGameFactory(0x05F9613aDB30026FFd634f38e5C4dFd30a197Fa1);
address constant livenessGuard = 0xc26977310bC89DAee5823C2e2a73195E85382cC7;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should read this from storage, see

bytes32 livenessGuardSlot = 0x4a204f620c8c5ccdca3fd54d003badd85ba500436a431f0cbda4f558c93c34c8;
as an example

contract NestedSignFromJson is OriginalNestedSignFromJson {
using LibString for string;

DisputeGameFactory constant dgfProxy = DisputeGameFactory(0x05F9613aDB30026FFd634f38e5C4dFd30a197Fa1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be read from the superchain registry instead of hardcoding it, for example

function _getContractSet() internal view returns (Types.ContractSet memory _proxies) {

address constant livenessGuard = 0xc26977310bC89DAee5823C2e2a73195E85382cC7;
bytes32 constant absolutePrestate = 0x03925193e3e89f87835bbdf3a813f60b2aa818a36bbe71cd5d8fd7e79f5e8afe;
address constant newMips = 0x69470D6970Cd2A006b84B1d4d70179c892cFCE01;
address constant oracle = 0x92240135b46fc1142dA181f550aE8f595B858854;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the same preimage oracle as op-contracts/v1.6.0? If so let's also read this from the superchain registry instead of hardcoding it

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.

3 participants