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

interop: Specify deposit handling #258

Merged
merged 23 commits into from
Aug 29, 2024
Merged

interop: Specify deposit handling #258

merged 23 commits into from
Aug 29, 2024

Conversation

Inphi
Copy link
Contributor

@Inphi Inphi commented Jun 25, 2024

Description

Add early specification of deposits for interop. Introduces a "deposit context" to derivation as a useful model for thinking about deposits in the EVM.

@Inphi Inphi marked this pull request as ready for review June 25, 2024 13:31
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.

I like the idea of the deposit context, offers a pretty clean separation.

Wondering how we can lift this outside of explicit actions, though - having each deposit (or the beginning/last deposit) hook into a call to L1Block seems expensive over time.

Maybe prior to executing the L1 info transaction, the block executor can set the transient isDepositing variable to true in L1Block, and then manually clear it after all deposits have been processed? This would ensure there's no gas impact on the downstream depositors, and it becomes a consensus rule.

specs/interop/predeploys.md Outdated Show resolved Hide resolved
#### `isDeposit`

```solidity
function isDeposit() (return bool)
Copy link
Member

Choose a reason for hiding this comment

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

Would fit very well as a transient storage variable.

@tynes
Copy link
Contributor

tynes commented Jun 25, 2024

We out of band decided on a sandwich approach where the L1 attributes tx sets isDeposit = true then derivation creates another deposit tx that is at the tail end of the deposits that sets isDeposit = false

@Inphi
Copy link
Contributor Author

Inphi commented Jun 28, 2024

I'm realizing that we need stronger guarantees on the cumulative gas usage of the L1 attributes tx and user deposits. Otherwise, the isDeposit = false transaction sequenced after user deposits can fail. It'll be very bad if isDeposit remains set to true as that would mean all executing messages are treated as deposits and thus not handled until isDeposit is successfully reset. To illustrate:

  1. L1 info deposit transaction succeeds: setting L1 block attributes and isDeposit = true
  2. User deposits are processed.
  3. L1 info deposit transaction to set isDeposit = false fails due to insufficient gas

The issue still occurs even if there aren't any user deposits as the first L1 info tx may be enough to fill the entire block (in theory, though unlikely in practice).

We could re-examine the ResourceConfig.maxResourceLimit setting to ensure there's ample gas available for both L1 attributes txs. Then the only thing left to do is to have a hard gas limit on reduce the gasLimit of the initial L1 attributes tx. Basically the invariant must be:

l1AttributeTx.gasLimit + resourceConfig.maxResourceLimit + isDepositFalseTx.gasLimit <= 30000000

Right now this isn't the case on mainnet. As the l1AttributeTx.gasLimit is the dominating term above at 150 M. Even though it typically uses much less gas than that, we should guarantee that there's always sufficient gas available.
However, these gas and resource limits seem appropriate to me and I don't think we should reduce them.

Other options as we've discussed earlier that don't have this problem:

  • Update execution to honor isSystemTx=true for deposits. This eliminates gas concerns for executing isDeposit = true.
  • Mutate deposits once again. Ensures that isDeposit is always reset at the expense of broken deposit gas estimation. Not great UX.
  • isDeposit updates outside of the EVM via transient storage. Requires changes to the execution engine.

We could also simply live with the possibility of interop being disabled temporarily. While this may be OK for a devnet, it shouldn't happen on mainnet.

@tynes
Copy link
Contributor

tynes commented Jun 28, 2024

This 100% is a valid concern, I think we should:

We could re-examine the ResourceConfig.maxResourceLimit
and lower it if necessary.

We shouldn't have to think about this, its left over from old decisions and I think that we can solve this problem using a deposit queue which i made a draft spec for here: #82

Copy link
Contributor

@skeletor-spaceman skeletor-spaceman left a comment

Choose a reason for hiding this comment

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

renaming resetIsDeposit in favor of depositsComplete

specs/interop/derivation.md Outdated Show resolved Hide resolved
specs/interop/predeploys.md Outdated Show resolved Hide resolved
specs/interop/predeploys.md Outdated Show resolved Hide resolved
specs/interop/predeploys.md Outdated Show resolved Hide resolved
@tynes
Copy link
Contributor

tynes commented Aug 13, 2024

Looks like this needs a rebase now @skeletor-spaceman, happy to review again when its ready

@skeletor-spaceman
Copy link
Contributor

skeletor-spaceman commented Aug 13, 2024

happy to review again when its ready

@tynes merge ready :)

@tynes
Copy link
Contributor

tynes commented Aug 13, 2024

@skeletor-spaceman The CI is failing

specs/interop/predeploys.md Outdated Show resolved Hide resolved
specs/interop/predeploys.md Outdated Show resolved Hide resolved
Copy link
Contributor

@0xDiscotech 0xDiscotech left a comment

Choose a reason for hiding this comment

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

Solid spec! Left few suggestions and a question

specs/interop/derivation.md Outdated Show resolved Hide resolved
specs/interop/derivation.md Outdated Show resolved Hide resolved
specs/interop/derivation.md Outdated Show resolved Hide resolved
specs/interop/derivation.md Outdated Show resolved Hide resolved
specs/interop/predeploys.md Outdated Show resolved Hide resolved
specs/interop/predeploys.md Outdated Show resolved Hide resolved
specs/interop/upgrade.md Show resolved Hide resolved
specs/interop/predeploys.md Outdated Show resolved Hide resolved
specs/interop/upgrade.md Outdated Show resolved Hide resolved
@tynes tynes merged commit d683e31 into main Aug 29, 2024
1 check passed
@tynes tynes deleted the inphi/interop-deposits branch August 29, 2024 20:27
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.

5 participants