ethereum-optimism / optimism

Optimism is Ethereum, scaled.
https://optimism.io
MIT License
5.21k stars 2.99k forks source link

Interop: Ban Deposits that Trigger Executing Messages for Devnet #10867

Open tynes opened 1 month ago

tynes commented 1 month ago

It was determined in https://github.com/ethereum-optimism/optimism/issues/10870 that there was a missing edge case with deposit handling in the context of interop without the "only EOA" invariant. The semantics are being defined in https://github.com/ethereum-optimism/design-docs/pull/13 but it was decided that we should not implement this as part of the devnet release and instead just ban deposits that trigger executing messages completely.

The only way to do this reliably is by hacking into the state transition function. Since we don't have an explicit goal of being multiclient and we want to run the devnet to see how stable our software is, we can modify the STF temporarily to ban deposits and then remove this hack when we have a proper solution implemented.

An easy way to hack this solution is to hook into someplace around applyTransaction and revert deposit transactions that trigger an executing message event.

While this code is temporary and not going to live forever, it will help us in the short term be able to reliably test the end to end flow of the system on a devnet.

See below proposal of using system-deposits for devnet 1, and then later solution that builds on top of that.

protolambda commented 1 week ago

Based on conversations last week around the deposit-tx milestone: we can make the "is deposit" boolean visible to the inbox contract, to disable force-inclusion of interop messages, which could otherwise pass through invalid messages.

Proposal to not modify the execution engine, and instead use system-deposits to add the behavior, and use statically pre-declared messages as mechanism to verify interop messages:

Functionality to add now for first devnet:

Functionality to add next (later devnet iteration):

If there is an external reorg, or unavailable data, we can't immediately verify the message declared in a deposit. To mitigate this, we can enforce the statically declared message deposit to only pull in a message older than a sequence-window, and otherwise mark the deposit as invalid (change sender to X).

tynes commented 1 week ago

We will want to spec this new behavior in the predeploys. The L1Block contract would need a new getter, something like isDeposit()(bool). The CrossL2Inbox would add an invariant if (l1Block.isDeposit() == false) revert Unauthorized(); and then we can relax that restriction in the future like @protolambda said

One major question is whether the isDeposit() function should be only callable by the CrossL2Inbox. Do we want arbitrary smart contracts to be able to know this information? It would be used to build censorship features like https://github.com/ethereum-optimism/design-docs/pull/30

tynes commented 1 week ago

We could avoid mutating deposits by adding another function on the OptimismPortal that emits the deposit transaction event but this still does not solve the problem with the sequence window. So my current understanding of the proposed solution is fully in derivation, so the deposit is mutated if 1) the identifier doesn't point to the message or 2) the message is newer than the sequencing window of the remote chain.

This would mean we need to standardize the sequencing window between chains, since there is no way to know what the value is, unless we use the superchain registry config or assume that they are always the same. I don't love using the superchain registry approach as that is not scalable, perhaps we hardfork in a constant value and then make it configurable in the future (for quality of life for L3s)

The function that registers the events in the CrossL2Inbox, would that be onlyDeposit() && onlyEOA()? This would allow the derivation pipeline to do static analysis, this would be a reliable way to ensure static analysis

Inphi commented 1 week ago

One major question is whether the isDeposit() function should be only callable by the CrossL2Inbox. Do we want arbitrary smart contracts to be able to know this information? It would be used to build censorship features like ethereum-optimism/design-docs#30

I'd lean towards not expanding ABI use scope in principle. If there's demand for a valid use-case (non-censoring, etc) for a no-auth isDeposit then we can add it in later.

tynes commented 1 week ago

In my mind there are 2 ways to think about this problem, one is completely within the smart contracts and one is completely within the derivation pipeline. The smart contract based solution was partially described as well as the derivation pipeline based solution. We should explore the tradeoffs between these two, once we introduce mutating deposits then it may become tech debt eventually as its a new class of behavior