Isthmus: L2 withdrawals-root in block header [TRACKER]

[protolambda]

Plan

We are going to use a feature-branch feature-config-flag approach,
to keep this withdrawals work optional to Holocene.

If not complete by Holocene devnet stage, we do not merge the feature-branches,
and thus do not include it in the hardfork.

TODOs, in sequence:

• Review the TODO lists. Communicate any questions / concerns.
• Review, but don't merge, the op-geth PR.
• Open a specs PR with the described combined above spec changes.
  You may use a feature-branch, but a single specs PR is fine too.
  Previous basic Holocene spec work can be found in specs#374.
• Implement the remaining op-geth TODOs
• Open a monorepo op-node feature-branch PR, for the initial basic changes,
  with go.mod update to point to the latest commit of the op-geth with the right changes.
• Open further monorepo op-node PRs, to finish the op-node changes.
• Update one of the internal devnet nodes to dev release candidates of the above, verify that it stays in sync.
• If all of the above is complete and reviewed,
  we enable the feature-config-flag and include it in the Holocene devnet/testnet phase.
• During Holocene devnet we do a sync-test, to ensure the EL block-body syncing works.

Spec TODOs

General notes: keep the specs categorized in the holocene directory, and file names consistent across upgrades:
e.g. Execution layer changes go exec-engine.md.

• Specs note that the block-body withdrawals list is encoded as an empty RLP list.
• Specs note that any block-after-transaction simulation should include an empty withdrawals-root;
  the block is sealed at the end of a block.
• Specs note that the withdrawals-root in the genesis block,
  if Holocene is active, is always the empty withdrawals root, regardless of L2 state.
• Specs note that the withdrawals-root starting at shanghai, pre-Holocene,
  is set to an empty withdrawals list, this equals the same root as an empty storage root.
  The withdrawals are there in L2 state, just not elevated into the withdrawals-root.
  Pre-Holocene output-root construction should be careful not to use the header withdrawals-root.
• Specs note that at the time of state-processing, with an in-progress header,
  the withdrawals-root should not be available to the EVM / application layer.
• Specs Engine-API modification: ExecutableData gets a withdrawalsRoot attribute.
• Specs note that for an accurate storage-root to be known,
  the block state has to be committed first, before the withdrawals storage root can be set accurately.
  This attribute should thus be set right after setting the state-root attribute of the header.
• Specs EL P2P note: block-bodies and block-headerts responses are handled the same in isolation:
  we compute the body-withdrawal-hashes from the received withdrawals list in the block body.
  Then, when we compare the body-withdrawal-hashes to the header withdrawal-hashes,
  and if optimism holocene is active for the given header, we check if the body-withdrawal-hash matches the empty list root,
  while allowing the header-withdrawal-hash to be any non-null value.
  By verifying at this later stage, we have the header timestamp, and can apply the fork logic conditionally.
• Specs update to describe CL ExecutionPayload SSZ encoding with new withdrawals-root included.
  • We remove the withdrawals list from the encoding, diverging from the L1 beacon-chain ExecutionPayload encoding.
• Spec that we continue to use engine_newPayloadV3 engine-API,
  but now with the additional ExecutionPayload attribute, that is omitted pre-Holocene.
• Specs update to describe new CL P2P gossip topic, with updated ExecutionPayload SSZ encoding,
  and a gossip-validation rule that checks if the withdrawals root is non-nil, and withdrawals list is empty.
  This spec change goes into the main rollup-node-p2p.md, to be consistent with the other gossip topics.
• Update fault-proof-program spec that describes the output-claim verification,
  and link to the Holocene specs change of the block header.

Created ethereum-optimism/specs#394 to track work for spec updates

CL / op-node TODOs

• Update the ExecutionPayload type to have the optional withdrawals root
• Update the SSZ encoding implementation of the payload.
  To conditionally either encode the empty-list (pre-holocene) (simply just an offset constant) or an empty list and a 32 fixed-size value.
• op-node/p2p update to activate new gossip topic for Holocene.
• Update op-node/rollup/attributes/engine_consolidate.go:
  • pre-Shanghai: attributes must have nil withdrawals list and nil withdrawals-root.
  • pre-Holocene: attributes must have empty withdrawals list (different than nil!), execution-payload must have empty withdrawals list and nil withdrawals-root.
  • post-Holocene: attributes must have empty withdrawals list, execution-payload must have empty withdrawals list and non-nil withdrawals-root.
• Action-test to cover L2 block before, at, and after Holocene.
  All 3 cases with and without withdrawal-storage change (by inserting a withdrawal tx in L2).
• Add a note to op-node/api.go node.OutputAtBlock that the OutputV0AtBlock function handles the Holocene change.
• Change op-service/sources/l2_client.go L2Client.OutputV0AtBlock to
  check if Holocene is active based on the timestamp of the retrieved block-header.
  If it is, then don't fetch the storage or storage-proof,
  but use the Withdrawals-hash from the block-header to construct the output-root.
• Ensure that the op-service/sources/eth_client.go functionality of block/header verification does not break. In particular, update the withdrawals-list verification of the block-body, to enforce an empty non-nil list. And the header must have a non-nil withdrawals-root after genesis.
• Update op-program output-root construction to not retrieve the withdrawals-root from storage, but from the block-header.
  While technically compatible without this change, there are other op-program changes,
  and we want to prevent the arbitrary storage-lookup preimage needs that come with the state-access.

op-geth TODOs

• Review the op-geth draft PR, to be reused as feature-branch tracker.
• Json roundtrip of block and header types
• RLP roundtrip of block and header types
• ExecutableData json encode/decode test
• Geth note (clarify, and include in fork.yaml): t8ntool was not updated,
  it doesn't support some OP-Stack features already, so it is not worth it to handle the withdrawals root case.
  It is also just unfortunate code-duplication in this tool that it does not use the same sealer as Geth consensus does.
• Describe the feature-branch changes in fork.yaml.
• Make sure the op-geth commits are tidy, description format should match upstream commits.

[ajsutton]

I would strongly prefer to avoid using feature branches and instead make inclusion in Holocene optional by using a feature toggle specific to this functionality. ie instead of IsHoloceneEnabled use IsL2WithdrawalsRootEnabled. While we are on track to include this in Holocene IsL2WithdrawalsRootEnabled can just delegate to IsHoloceneEnabled and if we decide not to include it we simply change it to return false.

Using a feature branch breaks continuous integration so we wind up having to deal with merge conflicts and aren't testing the the combined code until close to release which significantly increases the risk of bugs going undetected.

[sebastianst]

Agree with @ajsutton on using feature flags instead of feature branches. We've used a light version of this as a showcase at 1958945.

[clabby]

Specs update to describe CL ExecutionPayload SSZ encoding with new withdrawals-root included.
-> We remove the withdrawals list from the encoding, diverging from the L1 beacon-chain ExecutionPayload encoding.

I disagree that we should be changing up the ExecutionPayload. Creating a new type here and/or having disparate encoding rules is pretty painful, especially when they're not just additive.

What do you think about just keeping the withdrawals list non-nil + empty, like we do post-Shanghai? LMK if I'm missing something, but it doesn't seem like we need the bridge storage root in the payload, nor change up the payload's understanding of beacon-withdrawals being empty.

[protolambda]

What do you think about just keeping the withdrawals list non-nil + empty, like we do post-Shanghai? LMK if I'm missing something, but it doesn't seem like we need the bridge storage root in the payload, nor change up the payload's understanding of beacon-withdrawals being empty.

We do need it in ExecutionPayload, otherwise we're unable to relay the withdrawals-root on p2p gossip, and then the op-node won't be able to insert it as unsafe block into the EL node.

We can keep the list around as always-empty list for compatibility, but the new attribute will break the gossip payload regardless.
And we'll need to break the engine API with a new function argument if we don't add it as optional RPC field to the ExecutableData (the RPC equivalent of ExecutionPayload). I was hoping to add it to the execution-payload-envelope, but then more has to change in the RPC than would have if we modify the payload itself. Open to suggestions for a better diff though.

[protolambda]

Ok, feature-flags, not feature-branch, it is. But please do document the changes / PRs, so the shared diff can be reviewed easily, especially if this work is picked up by a newer contributor. It's important that the changes are all consistent with each other and the specs.

[BlocksOnAChain]

@vdamle as agreed with @tynes and @protolambda on discord, I assigned this issue to you, so we can start looking at it, as part of the holocene hardfork work.

[clabby]

@protolambda Makes sense. I prefer the idea of extending the type rather than removing the old field on top of that.

[sebastianst]

@vdamle I created a new milestone for this feature, please add all related issues and PRs to it going forward.