-
Notifications
You must be signed in to change notification settings - Fork 99
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
experimental: allow the batch inbox to be a contract #284
base: main
Are you sure you want to change the base?
experimental: allow the batch inbox to be a contract #284
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not detailed enough to be implemented solely from the spec. If you are serious about this, please update this spec to be detailed to the point where it is possible to implement by just looking at the spec. We will also need the ability to make the batch inbox dynamic so that chains that use empty accounts today can migrate to using smart contract based batch inboxes in the future
@tynes Thank you for your insightful advice and for highlighting the migration issue—it's an excellent point. We'll update the specification with more comprehensive details. However, we have a question regarding the OP Stack upgrade process: Could you clarify the upgrade mechanism and how it ensures all components remain synchronized with |
@blockchaindevsh The batcher will need to be aware of the fact that the batch inbox is dynamic. It will need to do one of the following:
Option 4 is better than 3 because it removes the need for an L1 archive node, it also reduces complexity around missed events with option 1. Everything is tradeoffs. |
@tynes Upon reviewing the custom gas token implementation in the develop branch, it appears that it only takes effect during the initial contract deployment and subsequently operates solely within the contract itself. Neither the op-node nor the op-batcher perform any processing related to this feature. I'm uncertain if I've overlooked any aspects of its implementation. Let's assume that the batcher should submit to the new inbox immediately upon its change in the We extensively discussed the batch submission process when the inbox changes. It appears that regardless of the strategy employed by the batcher to fetch inbox address, there remains a possibility of inadvertently submitting to a stale inbox. (
To mitigate this, after a transaction is submitted, the batcher should verify whether the inbox has changed at the L1 block that included the batch. If a change is detected, it should resubmit the transaction to the updated inbox.
We concluded that it would be simpler and less prone to errors if the batcher queries the latest inbox directly from L1 immediately before submitting each batch(similar to option 3, but on demand instead of periodically). |
How would |
Does the option 4 mean that the DepositContractAddress is currently part of L2 state while the BatchInboxAddress is not? |
@tynes The caller is responsible for passing a canonical value:
|
@blockchaindevsh You are caught up in implementation details. You did not answer my question. If you are interested in working on a spec for this feature, please ensure that it is fully specified. Right now it is very unclear how this feature works based on your description. If you aren't sure how to write a spec, look at other documents in this repo, such as the custom gas token feature. @qzhodl The batch inbox address and the deposit contract address are the same thing, just with different names. That is left over tech debt |
@tynes I've updated the specification with additional details, drawing heavily from the custom gas token specification. |
…0000000000000000000000000000
@tynes , I was wondering if you could provide any further advice regarding the inbox specification ? |
specs/experimental/inbox-contract.md
Outdated
The `marshalBinaryInboxFork` function is added to [`L1BlockInfo`](https://github.com/ethereum-optimism/optimism/blob/5e317379fae65b76f5a6ee27581f0e62d2fe017a/op-node/rollup/derive/l1_block_info.go#L41). This new function incorporates an additional `batchInbox` parameter for the [`setL1BlockValuesEcotone`](https://github.com/ethereum-optimism/optimism/blob/5e317379fae65b76f5a6ee27581f0e62d2fe017a/packages/contracts-bedrock/src/L2/L1Block.sol#L136) function: | ||
|
||
```golang | ||
func (info *L1BlockInfo) marshalBinaryInboxFork() ([]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should define the spec in terms of the serialization instead of source code. Its not super clear exactly what this is doing without knowing the implementation details of various functions used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is gone after following the new design pattern in f5b8178.
I think we want to make this spec more generic and move it into the |
I've updated the spec to follow the design pattern in #122 and moved it into the |
@blockchaindevsh I started a PR that we can build on in #358 This document is good but it focuses a lot on particular implementation details. These are important but not necessarily for the specs - we want to describe what should happen and some why but not links to source code I would also like to understand a bit better on how your proposal around the dynamic batch inbox impacts the fault proof |
Description
This specification aims to allow the batch inbox to be a contract, enabling customized batch submission conditions such as:
Note: This specification can serve as a preliminary step towards addressing #221.