specs icon indicating copy to clipboard operation
specs copied to clipboard

Holocene: Add `L2ToL1MessagePasser` account storage root to Header `withdrawalsRoot`

Open clabby opened this issue 1 year ago • 14 comments

Overview

Introduces an addition to the Granite hardfork, where upon activation, consensus will require L2 block headers to contain the 32 byte account storage root of the L2ToL1MessagePasser predeploy after the block's execution.

clabby avatar May 09 '24 18:05 clabby

[!WARNING] This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite. Learn more

  • #177 Graphite 👈
  • #259 Graphite
  • main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @clabby and the rest of your teammates on Graphite Graphite

clabby avatar May 09 '24 18:05 clabby

I would like us to consider adding it to the L1 attributes transaction's calldata instead of in the extradata field for 2 reasons:

  1. L1 may repurpose the extradata field into a feature that we would want to adopt so we would need to make the sort of change that I am suggesting anyways
  2. We were considering using the extradata field for interop, to which I came to the conclusion in 1) and I no longer want to use the extradata field.

You can fetch the L1 attributes tx and pull the output root from its calldata, its still in the history and a proof of tx inclusion in a header can be used for a light client proof.

Note: great spec btw! well formatted and documented

tynes avatar May 09 '24 18:05 tynes

I would like us to consider adding it to the L1 attributes transaction's calldata instead of in the extradata field for 2 reasons:

  1. L1 may repurpose the extradata field into a feature that we would want to adopt so we would need to make the sort of change that I am suggesting anyways
  2. We were considering using the extradata field for interop, to which I came to the conclusion in 1) and I no longer want to use the extradata field.

You can fetch the L1 attributes tx and pull the output root from its calldata, its still in the history and a proof of tx inclusion in a header can be used for a light client proof.

Note: great spec btw! well formatted and documented

1 does make sense (re: forwards compat), though for this, creates a bit of an issue. If we submit the account storage root in the L1 info transaction, we must do 1 of two things:

  1. Execute the full block before the L1 info transaction is crafted fully, such that you have the account storage root of the message passer at the end of the block. This gets hairy - what if a call references the storage root in L1Block that also modifies the account storage root of the message passer? We can optimistically execute with all values except for the message passer storage root, but this adds complexity and doesn't cover all of our bases.
  2. Submit the account storage root of the message passer for the previous block's post-state, which seems messy. These values are intended to be fetched off-chain rather than used on-chain, so even though in the eyes of the EVM, the accounting is correct, it is not off-chain. We'd have to query L1Block at block_number + 1.

The message passer account storage root also doesn't need to be available within the EVM, as the sentMessages mapping is publicly available for other contracts' consumption.

clabby avatar May 09 '24 18:05 clabby

Submit the account storage root of the message passer for the previous block's post-state, which seems messy. These values are intended to be fetched off-chain rather than used on-chain, so even though in the eyes of the EVM, the accounting is correct, it is not off-chain. We'd have to query L1Block at block_number + 1.

This is definitely what I was thinking. I don't think its that big of a deal but you are right that its different than the header which contains post execution information and in this case in the calldata it would be pre execution data. You are right that the EVM doesn't care about this information, its more about reserving extradata for future usage. If the root goes in the extradata, then interop will certainly need to use the calldata method I am describing. (Likely not in the first release, but we know we need this design to scale). So we are both contending for the same thing and basically what I am sayig is that neither of us should get it and prioritize L1 taking it eventually :P

We shouldn't argue on too far in the future, just sharing some researchy thoughts:

  • in interop + root in calldata world, could emit the root in an event to make it consumable across other chains to make bridge rebalancing + withdrawing more seamless
  • could pave path towards moving to more simple commitment scheme for withdrawals, altho we should probably just adopt verkle

tynes avatar May 09 '24 18:05 tynes

A few issues with the L1Block transaction:

  • It's a breaking L2 contract-change. Maybe possible, since Fjord makes fee changes, which may overlap and thus not increase surface of the fork all that much.
  • The storage-root we are embedding is only readily available within the execution-engine. Whereas the L1Block info is created in the op-node. Mutating the transaction in the execution-engine would be new.

And missing from the specs: the L1-block-attributes, derived from L1, are compared to the L2 blocks, for safe-block validation. The storage root is not currently part of the batch data, so this would not work. It would be more like a block-output, that is handled just like the receipts-root and the state-root are.

To verify the block-attributes, we would need to isolate this 32 bytes, and just optimistically accept it in op-node during consolidation. And then add a rule in op-geth, when importing the block, to check the storage-root against the actual storage (if it can, i.e. non-archive nodes may have to skip this for older blocks).

Using extraData may make this feature much cleaner: the consensus package in geth does the block-header sealing and validation at the end of the block, which we can extend with this storage-root insertion / check. And then during op-node block-attributes comparison, it is easily separated.

While I agree with @tynes that it would be better to avoid extraData, this does seem like a really meaningful usage of it for the op-stack. And we can wait for L1 to re-use it forever, they might not.

protolambda avatar May 09 '24 19:05 protolambda

Putting it in the withdrawals root makes a lot of sense h/t @clabby

Test case:

  • Make sure that the account root can be fetched if there is no diff (no withdrawals) in more than 256 blocks (past the snapshot's history). This may be different for different EL clients

Another thought is EL change vs CL change. This is useful to ensure that you are proving against a good withdrawal. The user can check against an RPC to ensure that they aren't proving against a malicious claim. It also makes it so that you don't need historical eth_getProof when running a sequencer.

Output root proof: version || state root || block hash (in some order) - now its easy to compute a output root proof (preimage) given a block

tynes avatar May 09 '24 19:05 tynes

Withdrawals root definitely makes sense for this. If we do want to use the extraData field though, we can take a leaf out of clique & IBFT's book and just extend the extraData field. Because Clique stores the validator info in extraData, there's very widespread support for it being longer than 32 bytes so we can also take advantage of that and just append our data to whatever value L1 winds up using it for in the future. And definitely a big fan of not requiring an archive node to compute an output root.

ajsutton avatar May 14 '24 02:05 ajsutton

I think its fine to target this towards granite now as we know its not landing in fjord

tynes avatar May 15 '24 12:05 tynes

Also @ajsutton @protolambda the proposal has been updated away from using extraData and towards using withdrawalsRoot - which works great for us, it is our withdrawals root

tynes avatar May 15 '24 12:05 tynes

I would be supportive of updating this PR to be proposed for inclusion in granite

tynes avatar May 20 '24 14:05 tynes

I don't think we can add this for Fjord, but I will add it to the scope of work item for Granite. CC: @sebastianst @protolambda @tynes for visibility and confirmations

BlocksOnAChain avatar May 23 '24 10:05 BlocksOnAChain

Happy to put this into Granite!

sebastianst avatar May 23 '24 10:05 sebastianst

This requires no changes to the batch serialization as all of the data is present in the EL

tynes avatar Jun 28 '24 22:06 tynes

As agreed on the ENG staff call, I'm assigning this to @protolambda as tech lead that will own this implementation for Granite hardfork.

BlocksOnAChain avatar Jul 18 '24 15:07 BlocksOnAChain

Closing this in favor of https://github.com/ethereum-optimism/specs/pull/374

tynes avatar Sep 17 '24 00:09 tynes