Isthmus: L2 Withdrawals root spec updates #396

vdamle · 2024-09-27T10:59:33Z

Description

Spec Updates for Isthumus: L2 Withdrawals root spec updates

Tests

None

Metadata

Fixes Isthumus: Spec updates for L2 withdrawals-root in block header #394

specs/protocol/holocene/exec-engine.md

tynes · 2024-09-27T21:16:16Z

Nice work on this so far!

protolambda

Great start

specs/protocol/holocene/exec-engine.md

vdamle · 2024-10-07T09:55:21Z

specs/protocol/rollup-node-p2p.md

@@ -277,6 +282,11 @@ A block is structured as the concatenation of:
  - `signature`: A `secp256k1` signature, always 65 bytes, `r (uint256), s (uint256), y_parity (uint8)`
  - `parentBeaconBlockRoot`: L1 origin parent beacon block root, always 32 bytes
  - `payload`: A SSZ-encoded `ExecutionPayload`, always the remaining bytes.
+- V4 topic
+  - `signature`: A `secp256k1` signature, always 65 bytes, `r (uint256), s (uint256), y_parity (uint8)`
+  - `parentBeaconBlockRoot`: L1 origin parent beacon block root, always 32 bytes


Question: Is my assumption correct that this is an additive list?

the block header RLP list? Kind of, attributes have been reused / changed meaning in the past, but it's meant to be backward compatible.
The SSZ version that is used for Gossip, with the parentBeaconBlockRoot added to it, is already non-standard, and not strictly speaking SSZ anymore (due to a missing offset value).

vdamle · 2024-10-07T09:58:06Z

specs/protocol/rollup-node-p2p.md

@@ -403,6 +416,8 @@ Implementations may opt for a different limit, since this sync method is optiona
  matching the `ExecutionPayload` SSZ definition of the L1 Merge, L2 Bedrock and L2 Regolith, L2 Canyon versions.
 - `1`: SSZ-encoded `ExecutionPayloadEnvelope` with Snappy framing compression,
  matching the `ExecutionPayloadEnvelope` SSZ definition of the L2 Ecotone version.
+- `2`: SSZ-encoded `ExecutionPayload` with Snappy framing compression,


From the proposed changes here and as further elaborated here, the ExecutionPayload (and not ExecutionPayloadEnvelope) will be extended with withdrawalsRoot

ajsutton

Looks good generally. I've left a bunch of comments but primarily aimed at ensuring we're being really clear in the spec rather than disagreeing with the design (apart from the withdrawalRoot for genesis which seems weird to me but maybe I'm missing something).

ajsutton · 2024-10-08T22:46:02Z

specs/protocol/isthmus/exec-engine.md

+Changes to the L2 block header are applied when it is considering data from a L1 Block whose timestamp
+is greater than or equal to the activation timestamp.


I'm not sure what this actually means. Is this when the L2 block's origin is an L1 block with timestamp > activation or if the L2 block came from a batch included on L1 after activation time (and is that when the channel starts or ends?).

Also, why would different parts of the L2 block activate Isthmus at different times?

I believe it is the former -- the L2 block's origin is an L1 block with timestamp > activation.

Also, why would different parts of the L2 block activate Isthmus at different times?

Definitely didn't mean to imply the above. Also not sure where I've implied the above! Please let me know and I'll fix it.

ajsutton · 2024-10-08T22:47:38Z

specs/protocol/isthmus/exec-engine.md

+that is returned by `eth_getProof` at the given block number -- it is the storage root of the post state,
+similar to how the state root of the post state is stored in the block header.


Would it be simpler to say this should be the account store root from the world state identified by the stateRoot field in the block header?

Thanks for the suggestion, that definitely sounds more straight forward. Will make the change

ajsutton · 2024-10-08T22:49:17Z

specs/protocol/isthmus/exec-engine.md

+For an accurate storage root to be determined, the block state has to be committed first and only then, the
+`withdrawalsRoot` can be set accurately. Hence the `withdrawalsRoot` attribute should be set right after setting
+the state root attribute in the header.


This seems like an implementation detail which isn't necessarily true. The withdrawals root could easily be set once its final state is known even if hashing some other part of the world state trie is ongoing to calculate the overall state root.

This is true, if the world state is being built concurrently, so its an implementation detail. The most important thing is that its the post state

Ah, makes sense! I've removed this.

ajsutton · 2024-10-08T22:53:10Z

specs/protocol/isthmus/exec-engine.md

@@ -63,6 +79,33 @@ places a burden on users of the system in a post-fault-proofs world, where:
 Placing the [`L2ToL1MessagePasser`][l2-to-l1-mp] account storage root in the `withdrawalsRoot` field alleviates this burden
 for users and protocol participants alike, allowing them to propose and verify other proposals with lower operating costs.

+#### Genesis Block
+
+If isthumus is active at the genesis block, the withdrawals root is the empty withdrawals root, regardless of L2 state.


Why wouldn't this be set to match the actual L2 state? Given genesis is the initial safe head having this be inconsistent could lead to creating invalid proposals.

This saves some effort when building the L2 genesis. If its going to break the proposer, its definitely something that we can support. There is not technically much utility about making a claim about the genesis state to facilitate a withdrawal since there are none in the genesis state of a standard config chain

It's not so much that there's utility in proposing for the genesis block, but that this breaks two invariants:

That starting from Isthmus the withdrawalsRoot header is the L2ToL1MessagePasser storage root

That the safe head can always be used to produce a valid proposal

Everywhere that reads the withdrawalRoot would need to have special case handling for genesis. So I'd be very inclined to have the extra state read in L2 genesis generation to make all other cases simple.

As an example of the bugs that come from this special case - this PR adds a paragraph to proposal.md that says the withdrawalsRoot can be used post-Isthmus but forgets the genesis special case where it can't.

The withdrawal-contract genesis storage-root will keep changing, and having many special genesis values to put in the block-header makes things complicated. It couples the state content and block-header generation to tightly. We should just default the withdrawals-root in the genesis block to an empty withdrawals MPT root. I think not being allowed to withdraw from the genesis block is fine, as there aren't supposed to be withdrawals anyway.

As an example of the bugs that come from this special case - this PR adds a paragraph to proposal.md that says the withdrawalsRoot can be used post-Isthmus but forgets the genesis special case where it can't.

🤦🏼‍♂️ Indeed, I will add that.

ajsutton · 2024-10-08T22:58:41Z

specs/protocol/isthmus/exec-engine.md

+withdrawals. The header `withdrawalsRoot` MPT hash can be any non-null value in this case. Verification is performed
+after a header is available so that the header's timestamp can be checked to see if Isthumus is active.


Do we need to be clear about what this validation is and exactly when it is performed? ie does a node have to go back and recheck the header of every block after sync completes or just performs that check as each header is received?

Actually, how does a node do that verification if it's snap syncing given it won't have the world state? I think just verifying the final block is enough since it's parent hash commits to the previous world states and the withdrawalsRoot header and snap sync assumes that the chain it downloads has valid execution of transactions (since it skips executing them).

Verifying the final block is indeed enough, but this is definitely a weird edge case around reorging that I am not exactly sure how snapsync handles

Agree the wording makes it unclear, will clarify that the validation is performed on the final synced block.

ajsutton · 2024-10-08T23:01:26Z

specs/protocol/isthmus/exec-engine.md

+
+##### Transaction Simulation
+
+An empty withdrawals root should be included at the end of all applied simulated transactions, when the block is sealed.


Why is this empty and not the actual root? Is the state root field not updated for the simulated transactions?

Also, what does simulated transactions mean in this context? I'm assuming for some RPC calls but we probably should be clear about the context here.

The withdrawals root isn't in EVM context? Transaction simulation via eth_call doesn't build a block?

I see that the wording doesn't make clear the situations in which this is applicable. There is a eth_simulate proposed addition where transactions can be simulated spread across multiple blocks. The intent here is to specify that in such cases, an empty withdrawal root is included in the block header. Will clarify this.

ajsutton · 2024-10-08T23:01:36Z

specs/protocol/isthmus/exec-engine.md

+
+## Block Body Withdrawals List
+
+withdrawals list in the block body is encoded as an empty RLP list.


Suggested change

withdrawals list in the block body is encoded as an empty RLP list.

Withdrawals list in the block body is encoded as an empty RLP list.

tynes · 2024-10-11T19:50:39Z

specs/protocol/isthmus/exec-engine.md

+
+#### Backwards Compatibility Considerations
+
+Beginning at Shanghai and prior to Isthmus activation, the `withdrawalsRoot` field is set to the MPT root


Which OP Stack is shanghai in? Could be better to use the OP Stack fork name

Good point - Per this issue and blog post , it is in Canyon hard fork. Modified to specify Canyon.

vdamle force-pushed the vd/h-l2-withdrawals-root branch from 8d6e224 to eab4f18 Compare September 27, 2024 18:37

tynes reviewed Sep 27, 2024

View reviewed changes

specs/protocol/holocene/exec-engine.md Outdated Show resolved Hide resolved

tynes reviewed Sep 27, 2024

View reviewed changes

specs/protocol/holocene/exec-engine.md Outdated Show resolved Hide resolved

protolambda reviewed Sep 28, 2024

View reviewed changes

specs/protocol/holocene/exec-engine.md Outdated Show resolved Hide resolved

specs/protocol/holocene/exec-engine.md Outdated Show resolved Hide resolved

specs/protocol/holocene/exec-engine.md Outdated Show resolved Hide resolved

vdamle force-pushed the vd/h-l2-withdrawals-root branch from 1e8e5a2 to 82d0756 Compare October 7, 2024 05:25

vdamle changed the title ~~Holocene: L2 Withdrawals root spec updates~~ Isthumus: L2 Withdrawals root spec updates Oct 7, 2024

vdamle commented Oct 7, 2024

View reviewed changes

vdamle marked this pull request as ready for review October 8, 2024 09:17

vdamle requested review from sebastianst, axelKingsley, ajsutton, clabby and maurelian as code owners October 8, 2024 09:17

vdamle requested review from tynes and protolambda October 8, 2024 09:19

vdamle force-pushed the vd/h-l2-withdrawals-root branch from dd44455 to 4dd7ad6 Compare October 8, 2024 16:46

vdamle requested a review from mslipper as a code owner October 9, 2024 10:13

vdamle changed the title ~~Isthumus: L2 Withdrawals root spec updates~~ Isthmus: L2 Withdrawals root spec updates Oct 9, 2024

ajsutton reviewed Oct 11, 2024

View reviewed changes

tynes reviewed Oct 11, 2024

View reviewed changes

tynes mentioned this pull request Oct 14, 2024

Holocene: eip-1559 parameters in extraData #428

Open

Vinod Damle added 5 commits October 15, 2024 22:33

Migrate diffs from holocene -> isthumus

2266721

CL spec updates

58b0959

update fault proofs spec

d4b5c2d

Lint & build fixes

2649e95

updates based on review comments

e634c80

vdamle force-pushed the vd/h-l2-withdrawals-root branch from 84e0d42 to e634c80 Compare October 16, 2024 02:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Isthmus: L2 Withdrawals root spec updates #396

Isthmus: L2 Withdrawals root spec updates #396

vdamle commented Sep 27, 2024 •

edited

Loading

tynes commented Sep 27, 2024

protolambda left a comment

vdamle Oct 7, 2024

protolambda Oct 7, 2024

vdamle Oct 7, 2024

ajsutton left a comment

ajsutton Oct 8, 2024

vdamle Oct 11, 2024

ajsutton Oct 8, 2024

vdamle Oct 11, 2024

ajsutton Oct 8, 2024

tynes Oct 11, 2024

vdamle Oct 11, 2024

ajsutton Oct 8, 2024

tynes Oct 11, 2024

ajsutton Oct 13, 2024

ajsutton Oct 13, 2024

protolambda Oct 15, 2024

vdamle Oct 15, 2024

ajsutton Oct 8, 2024

tynes Oct 11, 2024

vdamle Oct 15, 2024 •

edited

Loading

ajsutton Oct 8, 2024

tynes Oct 11, 2024

vdamle Oct 15, 2024

ajsutton Oct 8, 2024

tynes Oct 11, 2024

vdamle Oct 15, 2024

		Changes to the L2 block header are applied when it is considering data from a L1 Block whose timestamp
		is greater than or equal to the activation timestamp.

		that is returned by `eth_getProof` at the given block number -- it is the storage root of the post state,
		similar to how the state root of the post state is stored in the block header.

		withdrawals. The header `withdrawalsRoot` MPT hash can be any non-null value in this case. Verification is performed
		after a header is available so that the header's timestamp can be checked to see if Isthumus is active.


		##### Transaction Simulation

		An empty withdrawals root should be included at the end of all applied simulated transactions, when the block is sealed.


		## Block Body Withdrawals List

		withdrawals list in the block body is encoded as an empty RLP list.


		#### Backwards Compatibility Considerations

		Beginning at Shanghai and prior to Isthmus activation, the `withdrawalsRoot` field is set to the MPT root

Isthmus: L2 Withdrawals root spec updates #396

Are you sure you want to change the base?

Isthmus: L2 Withdrawals root spec updates #396

Conversation

vdamle commented Sep 27, 2024 • edited Loading

tynes commented Sep 27, 2024

protolambda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajsutton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdamle Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdamle commented Sep 27, 2024 •

edited

Loading

vdamle Oct 15, 2024 •

edited

Loading