search

ethereum-optimism / specs

OP Stack Specifications
https://specs.optimism.io
Creative Commons Zero v1.0 Universal
102 stars 91 forks source link

Feat: Decoupled `OptimismPortal` #361

Open tynes opened 2 months ago

tynes commented 2 months ago

The OptimismPortal currently enshrines the proof system in its implementation. You can observe this fact with the following links:

We should move the proof system outside of the OptimismPortal and instead only enshrine the concept of an authorized caller that can pass through a Types.WithdrawalTransaction to execute. It would be assumed that this other caller did verify the proof.

This will help to reduce the size of the OptimismPortal contract, as it is right at the codesize limits. We do not want to be in a position where we need to move quickly and cannot due to the code being too large. This problem was hit when porting custom gas token to the OptimismPortal2.

This will also help to make it easier to maintain things like op-succinct. This will unblock us from removing the L2OutputOracle from the codebase without breaking other integrations.

The upgrade path can be straight forward for tooling like viem, if the semver is above a certain version then viem can grab the address of the authorized caller contract from the portal and then send its transaction to the authorized caller rather than the portal. The authorized caller can maintain the same ABI of the portal for simplicity.

The name for the authorized caller could be "verifier" as its role would be to verify proofs.

ajsutton commented 2 months ago

I would suggest the portal retains the same ABI but can forward to the authorised caller as needed (ie build in backwards compatibility). Breaking the withdrawal API is a big pain in the neck and something we should avoid if possible. Otherwise this will likely be delayed until something else needs to break the API and I'm not sure what that would be...

clabby commented 1 month ago

I would suggest the portal retains the same ABI but can forward to the authorised caller as needed (ie build in backwards compatibility). Breaking the withdrawal API is a big pain in the neck and something we should avoid if possible. Otherwise this will likely be delayed until something else needs to break the API and I'm not sure what that would be...

Yeah, we should be able to manage this for the critical path (proveWithdrawalTransaction + finalizeWithdrawalTransaction + events)

As for the administrative functions (setRespectedGameType + blacklistDisputeGame) & state vars (respectedGameType, disputeGameBlacklist, respectedGameTypeUpdatedAt), what do you think about these? These should not be consumed by anyone else, except for maybe viem. It would be very nice to move these to a different location (without a fall-through, so that people like succinct don't need to have dead code in their bridge.)

If I could wave a magic wand here, what I'd like to do (roughly) is:

  1. Create a new interface, IProofSystem. This would have:
    • A view function for fetching an output proposal by index.
    • A view function for checking a withdrawal's validity. Things like timestamps, etc. should be passed in. It would be on the portal to pass in honest values. It should not need to call-back to reference external state.
    • For other, lower-level concerns (i.e. the blacklist, respected type, etc.), these can be hidden underneath the interface's function impls.
  2. Remove DG-specific functions and state vars from the OptimismPortal2.
  3. Add in the IProofSubmitter to the portal, re-using the slot that the DisputeGameFactory is currently stored in.
  4. Add gaps to the respectedGameType, respectedGameTypeUpdatedAt, and disputeGameBlacklist slots.
  5. Retain the other ABI verbatim (proveWithdrawalTransaction + finalizeWithdrawalTransaction, events, etc.)

Usage Analysis

With this, breaking the API would not be so painful, it doesn't look like. It would also remove periphery codepaths from our most important contract, which is always good. However on the front of breaking the core withdrawal path API, we should not do that imo.

Monitoring is another concern here though. What would definitely change my opinion here is if these changes complicated or added risk to our monitoring path, cc @ajsutton.

ajsutton commented 1 month ago

Yeah agreed the key priority is maintaining compatibility with the withdrawal flow. Moving things like respectedGameType, disputeGameBlacklist and respectedGameTypeUpatedAt is likely to break some integrations like ENS but I feel like they're digging into a lot of implementation details in a way that isn't particularly ideal for them now anyway. They really just want a function that gives the latest finalized output root which some of the proposed AnchorStateRegistry changes may give if it only updates after the air gap and has a single anchor state for all game types.

I'd expect that respectedGameType is a key part of the withdrawal workflow and we need to preserve compatibility for it. You need to know what type of game to use when proving your withdrawal. We could deprecate it and keep something around for backwards compatibility for a while then delete later but I don't think should just break it. That one use in viem likely translates to a lot of users depending on it.

Same for checkWithdrawal, it seems important to expose a simple way for callers to ask if a withdrawal is ready to be finalized or not. Ideally we'd have a method that makes it easy to check if a given set of params (eg a game) is suitable to be used to prove a withdrawal - it could then check the blacklist as well which viem doesn't seem to be doing and may cause issues if we ever need to blacklist games.

The other question for me is what the point of entry should be for people trying to use the ABI. Do you need to make calls to both OptimismPortal and the IProofSubmitter implementation? If so, all withdrawal code needs to support all possible proof submitter implementations separately. Ideally I think a withdrawal could be completed using only OptimismPortal and be independent of the proof submitter implementation but I'm not sure how you'd find the appropriate game. Not the end of the world if we can't make it that general but having the the main usage be agnostic to the plugin implementation buys us a lot of flexibility. e.g. Succinct could use a different IProofSubmitter implementation and not need to have a custom integration with viem and any other withdrawal tools.

For the fall-through code, I'd say that people like succinct maintaining a fork of the OP Stack can easily remove it - they're already maintaining far more complex changes in the fork.

The functions with permissioned access are fine to just move as we know they're only used in limited fashion.

For monitoring, this shouldn't affect dispute-mon since it only monitors the dispute games. It will likely require updates to our other withdrawal monitoring though and I'm not familiar with how flexible that code is. I don't see anything in this that would hide information and cause monitoring problems - should just be a matter of updating the code to adjust where it gets the info from.