search

ethereum-optimism / superchain-registry

An index of chains which serves as the source of truth for who’s in the Superchain Ecosystem
MIT License
84 stars 99 forks source link

Overhaul how we store / discover fault proofs contract addresses in chain configs & validation/standard packages #604

Open geoknee opened 2 months ago

geoknee commented 2 months ago

There should now be two delayed WETH addresses—one for the permissioned game and one for the permissionless game.

mds1 commented 2 months ago

Just adding some extra details:

ajsutton commented 2 months ago

Just to take a step back - what are we trying to accomplish here? The DelayedWETH used is configured for each game type implementation and can be found by starting from the DisputeGameFactory, loading the implementation and calling weth(). As we go forward there will be more game types added and they may or may not share DelayedWETH instances (and we may go back to a single DelayedWETH). Dispute game implementations are designed to be pluggable but in superchain-registry we seem to be fighting against that and I'm not entirely sure why.

It feels like we're making a rod for our own back by trying to make superchain-registry a secondary source of truth about the deployed contracts rather than having the blockchain be that source of truth. I get verifying the deployed contracts have the code we expect and correct owners etc, I'm less clear about why we are trying to track the specific contract addresses of things like DelayedWETH rather than just finding them via the chain itself (and then verifying the code/ownership etc).

mds1 commented 2 months ago

Historically the superchain registry has served as a reference of all contract addresses for a chain, and other tools consume this data and treat it a a source of truth. So what we are trying to accomplish here is keeping the superchain registry up to date, as it's currently not reflective of the OP Mainnet architecture—this means either listing both DelayedWETH contracts, or removing both and simply not mentioning DelayedWETH.

To your point, another way to keep it up to date is only list a minimal set of contract addresses and let consumers make calls to discover the rest of the contracts. If we go this route, the extreme version is only listing the SystemConfig address in the registry since everything else can be discovered from that. There are various middle grounds here with different sets of contracts being listed/unlisted. I know this "onchain discovery" approach has been discussed in the past and from what I recall there are two main issues:

One caveat is that my understanding here may be out of data depending on the superchain registry product vision, so @geoknee or @tessr may have more insight here

ajsutton commented 2 months ago

Given that superchain-registry is regularly out of date like this (ie after pretty much every upgrade for at least a short period), I don't think it makes a very good source of truth. :)

I'd pretty strongly advocate for reducing the amount of contracts you try to list here to just the main entry points. Just SystemConfig might be a bit too restrictive given things like the OptimismPortal address are very fixed and a very common entry point. Whereas things behind the fault dispute game implementation aren't meant to be an entry point and can and will change without warning so I'd say they shouldn't be listed here.

mds1 commented 2 months ago

Given that superchain-registry is regularly out of date like this (ie after pretty much every upgrade for at least a short period), I don't think it makes a very good source of truth. :)

Yea, this is definitely a problem we need to solve. We were discussing this in discord a bit here: https://discord.com/channels/1244729134312198194/1285620551808716963

I'm ok with reducing the amount of proofs-specific contracts listed to whatever proofs team suggests though, as long as there are no downstream implications to consider. Maybe short terms we just add the missing DelayedWETH contracts and medium term we figure out this solution?

ajsutton commented 2 months ago

For proofs, I'd list the DisputeGameFactory and stop there to be honest. IMO given the current listing of DelayedWETH is wrong and it's gone this long without being noticed, I'd lean towards just removing DelayedWETH addresses to solve the inconsistency. But I'm happy to go with what you and George think given you have more context on how the superchain-registry is used than I do.

ajsutton commented 2 months ago

To provide some more background on why DisputeGameFactory is the point to stop - every network upgrade will need to deploy new FaultDisputeGame implementation contracts and these aren't proxied so the address will change frequently. But existing games continue to use the older implementation since you can't change the rules of the game part way through. So listing the game implementation addresses needs frequent updating, but is also potentially misleading since both old and new contracts wind up in use depending on the game. And then things like MIPS.sol, AnchorStateRegistry, DelayedWETH etc all hang off the fault game implementation and are implementation details of those. They may be different for different dispute games. Some of them do use proxies so have more stable addresses but it's definitely not something that's guaranteed and they are expected to be used only in the context of those dispute games.

mds1 commented 2 months ago

Thanks for these details, this logic makes sense to me and I'm onboard simplifying the superchain registry maintenance by only storing DisputeGameFactory

As for when to do it, I'll defer to @geoknee and co. here on given that I'm unsure what other tooling the change might impact. However it would be nice to make some change soon, to get this repo back to an accurate state. Perhaps the best path forward here is:

geoknee commented 2 months ago

I don't think there would be a big impact on downstream things, apart from our own validation checks which reference these contracts via the addresses stored in the chain configs. We check the following relationships:

https://github.com/ethereum-optimism/superchain-registry/blob/818111652e9d06e63adf24a4d49f0218de710acb/validation/standard/standard-config-roles-universal.toml#L29-L37

The checks were introduced here https://github.com/ethereum-optimism/superchain-registry/pull/144. @ajsutton if you think it makes sense we could: keep the checks remove the addresses for the contracts and discover them on chain during the course of the validation check?

ajsutton commented 2 months ago

@geoknee Yes, I think the validation checks would be far more meaningful if the address was discovered onchain - then you know you're actually monitoring the contracts that are actually in use. Given we want to monitor those rules for all game types and what types are deployed can vary from chain to chain, my suggestion would be to iterate through all the supported game types (currently on mainnet that's 0 for permissionless and 1 for permissioned) but the total list is:

CannonGameType       GameType = 0
    PermissionedGameType GameType = 1
    AsteriscGameType     GameType = 2
    AsteriscKonaGameType GameType = 3
    FastGameType         GameType = 254
    AlphabetGameType     GameType = 255

FastGameType and AlphabetGameType are for testing, it may be worth a check to verify they aren't deployed.

The game implementations are stored in a map so can't be iterated unfortunately, but its easy enough to request the implementation of each type to check (and no worse than the current hard coded address approach). So with cast that would be:

cast-mainnet call 0xe5965Ab5962eDc7477C8520243A95517CD252fA9 'gameImpls(uint32)(address)' 0

for at least 0 and 1. Then check it returns 0x0000000000000000000000000000000000000000 for 254 and 255 (and 2 & 3 if you want to ensure they aren't deployed until they're actually governance approved). Game type is actually a uint32 so you won't be able to iterate all values and do a completely exhaustive check. You could check that the respectedGameType in OptimismPortal is one of the types you do monitor though since that's ultimately what matters for withdrawals.

And then to get the DelayedWETH contract:

cast-mainnet call <impl-addr> 'weth()(address)'

and to get AnchorStateRegistry:

cast-mainnet call <impl-addr> 'anchorStateRegistry()(address)'

And for game type 1 you can check the challenger and proposer.

That will monitor the contracts that will be used for all newly created games (which is what is currently being monitored). For existing games you'd have to fetch each game and run the checks on it which would be very expensive.

geoknee commented 2 weeks ago

Thanks @ajsutton I am planning on tackling this as a part of the Holocene work -- I don't want to update the MIPS address, for example, if the longer term plan is to not store it at all.

I started looking into it in more detail and I am wondering about AnchorStateRegistry and PreImageOracle. Should these addresses be stored? From what I can tell, AnchorStateRegistry points to FaultDisputeGame, so is there an argument of including the former and not the latter?

ajsutton commented 2 weeks ago

Yeah I think we want to keep recording AnchorStateRegistry - with some of the incident response improvements it will become more central to the system and so will only have one shared across all games (it's proxied already so will be upgraded in place). That said, it's FaultDisputeGame that holds the reference to AnchorStateRegistry not the other way around. Though AnchorStateRegistry does have a reference to both SuperchainConfig and DisputeGameFactory which makes things a little circular currently.

PreimageOracle isn't currently proxied which I think is fine so we may wind up with multiple versions for different games when it changes in the future. I think it's reasonable to omit and just discover it from the dispute games.