Isthmus: Updates for L2 withdrawals root in header #12848

vdamle · 2024-11-06T15:46:10Z

Description

This PR proposes changes to add:

storage root of the L2ToL1MessagePasser contract to the ExecutionPayload.
a new p2p gossip topic (v4) for Isthmus blocks.
update the SSZ encoding & gossip to account for with withdrawalsRoot.
Uses the Isthmus timestamp activations in Config: Add support for Isthmus #12847

Tests

Action tests are in a separate PR

Additional context

Addresses changes to Consensus Layer to support L2 withdrawals root in the block header, as outlined in #12044

Spec changes: ethereum-optimism/specs#394
EL changes:

Metadata

None.

vdamle · 2024-11-06T15:54:16Z

📚 $\text{Stack Overview}$

Pulls submitted in this stack:

This comment was automatically generated by st.

go.mod

op-node/p2p/gossip.go

op-node/rollup/attributes/engine_consolidate.go

codecov · 2024-12-16T03:17:47Z

Codecov Report

Attention: Patch coverage is 52.44444% with 107 lines in your changes missing coverage. Please review.

Project coverage is 47.33%. Comparing base (a1df3e1) to head (3009e6d).
Report is 211 commits behind head on develop.

Files with missing lines	Patch %	Lines
op-service/eth/ssz.go	15.38%	16 Missing and 6 partials ⚠️
op-service/sources/l2_client.go	0.00%	22 Missing ⚠️
op-service/eth/types.go	0.00%	12 Missing ⚠️
op-node/p2p/gossip.go	64.51%	9 Missing and 2 partials ⚠️
op-service/sources/l1_rpc_check.go	68.42%	4 Missing and 2 partials ⚠️
op-program/client/l2/engineapi/block_processor.go	0.00%	4 Missing and 1 partial ⚠️
op-program/client/l2/engineapi/l2_engine_api.go	79.16%	4 Missing and 1 partial ⚠️
op-service/eth/block_info.go	0.00%	4 Missing ⚠️
op-service/sources/l2_rpc_check.go	50.00%	3 Missing and 1 partial ⚠️
op-service/sources/types.go	85.18%	2 Missing and 2 partials ⚠️
... and 7 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #12848      +/-   ##
===========================================
- Coverage    47.48%   47.33%   -0.15%     
===========================================
  Files          945      947       +2     
  Lines        79017    79136     +119     
  Branches       760      760              
===========================================
- Hits         37519    37458      -61     
- Misses       38656    38831     +175     
- Partials      2842     2847       +5

Flag	Coverage Δ
cannon-go-tests-32	`62.27% <ø> (-1.98%)`	⬇️
cannon-go-tests-64	`57.35% <ø> (-1.62%)`	⬇️
contracts-bedrock-tests	`91.76% <ø> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
op-node/node/api.go	`41.93% <ø> (ø)`
op-node/p2p/rpc_server.go	`71.67% <100.00%> (+0.09%)`	⬆️
op-program/client/l2/engine.go	`36.27% <100.00%> (ø)`
op-service/sources/l1_client.go	`70.73% <100.00%> (+0.73%)`	⬆️
op-service/testutils/random.go	`92.61% <100.00%> (+0.02%)`	⬆️
op-node/rollup/attributes/engine_consolidate.go	`61.39% <96.29%> (+2.60%)`	⬆️
op-node/rollup/superchain.go	`0.00% <0.00%> (ø)`
op-service/sources/eth_client.go	`27.93% <75.00%> (+0.29%)`	⬆️
op-chain-ops/cmd/check-derivation/main.go	`0.00% <0.00%> (ø)`
op-conductor/conductor/service.go	`60.20% <0.00%> (-0.11%)`	⬇️
... and 12 more

... and 18 files with indirect coverage changes

op-e2e/actions/helpers/env.go

* test all combinations - with/without withdrawal transaction before, at and after isthmus

raise a separate PR for it.

Also fix a test in rollup, with the correct expectation for sepolia holocene time

also include a missing error when p2p validation fails

- do not set withdrawalsRoot if pre-isthmus - op-e2e: create a allocs file for isthmus

* SSZ inferVersion is updated to infer V4 only when withdrawalsRoot is non-nil and it is not equal to the EmptyWithdrawalsHash * Payload/Attribute validation is similarly updated to allow pre-isthmus blocks to contain either a nil WithdrawalsRoot or EmptyWithdrawalsHash value

op-e2e/opgeth/op_geth.go

vdamle · 2025-01-03T12:55:46Z

op-service/eth/types.go

@@ -292,7 +301,7 @@ func (envelope *ExecutionPayloadEnvelope) CheckBlockHash() (actual common.Hash,
 	return blockHash, blockHash == payload.BlockHash
 }

-func BlockAsPayload(bl *types.Block, shanghaiTime *uint64) (*ExecutionPayload, error) {
+func BlockAsPayload(bl *types.Block, config *params.ChainConfig) (*ExecutionPayload, error) {


We now need to check for IsthmusTime as well before setting WithdrawalsRoot in the payload, so we pass in ChainConfig.

vdamle · 2025-01-03T12:56:05Z

op-service/eth/types.go

 	return payload, nil
 }

-func BlockAsPayloadEnv(bl *types.Block, shanghaiTime *uint64) (*ExecutionPayloadEnvelope, error) {
-	payload, err := BlockAsPayload(bl, shanghaiTime)
+func BlockAsPayloadEnv(bl *types.Block, config *params.ChainConfig) (*ExecutionPayloadEnvelope, error) {


We now need to check for IsthmusTime as well before setting WithdrawalsRoot in the payload, so we pass in ChainConfig.

vdamle · 2025-01-03T12:59:26Z

op-service/sources/eth_client.go

+	respChecker RPCRespCheck
+}
+
+type RPCRespCheck interface {


This interface is added so we can enforce any checks on the RPC responses. Since the EthClient wraps both a L1 and L2 client, the interface methods can be implemented (or not) appropriately based on the client type.

vdamle · 2025-01-03T18:56:33Z

@protolambda When testing, I was seeing in the e2e tests that even without isthmus active, the withdrawalsRoot is set to the empty list hash, per pre-existing code in op-geth. There were two unintended effects due to this:

Chain reorgs due to validation failure since AttributesMatchBlock() in engine_consolidate.go would return an error that withdrawalsRoot is not nil pre-isthmus.
SSZ un-marshalling would mistakenly infer that pre-isthmus blocks are V4 blocks.

So I've tweaked the rule in engine_consolidate.go and ssz.go for that one case, so we don't break validation of pre-isthmus blocks:

Pre-Isthmus (& Post Shanghai), ensure that withdrawals list is empty and withdrawalsRoot is either nil or an empty list hash

tynes · 2025-01-24T04:29:34Z

Closing in favor of #13962

This was referenced Nov 6, 2024

Config: Add support for Isthmus #12847

Merged

op-program: use withdrawalsRoot from header if Isthmus #12849

Closed

vdamle changed the title ~~Config: Add support for Isthmus~~ Isthmus: Updates for L2 withdrawals root in header Nov 6, 2024

vdamle mentioned this pull request Nov 6, 2024

Isthmus: Updates for L2 withdrawals root in header #12767

Closed

vdamle force-pushed the vd/l2-withdrawals-root branch from fc6b673 to 79609ed Compare November 13, 2024 09:55

vdamle force-pushed the vd/isthmus-config branch from bb28ebc to 2449a55 Compare November 13, 2024 09:56

vdamle commented Nov 13, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

vdamle mentioned this pull request Nov 14, 2024

Action tests for Isthmus withdrawal root addition #12925

Closed

vdamle mentioned this pull request Dec 3, 2024

[Tracker] Isthmus Upgrade #12703

Open

31 tasks

vdamle commented Dec 4, 2024

View reviewed changes

op-node/p2p/gossip.go Show resolved Hide resolved

vdamle commented Dec 4, 2024

View reviewed changes

op-node/rollup/attributes/engine_consolidate.go Show resolved Hide resolved

vdamle force-pushed the vd/isthmus-config branch from 2449a55 to a011f49 Compare December 4, 2024 21:53

vdamle force-pushed the vd/isthmus-config branch 2 times, most recently from e0cfbf2 to 0aaf2c1 Compare December 16, 2024 02:33

vdamle force-pushed the vd/l2-withdrawals-root branch from 7848fb3 to e7e29f3 Compare December 16, 2024 03:15

vdamle force-pushed the vd/l2-withdrawals-root branch from e7e29f3 to 8d11a2e Compare December 16, 2024 03:39

vdamle force-pushed the vd/isthmus-config branch 3 times, most recently from 349081f to bb148a3 Compare December 16, 2024 21:08

vdamle force-pushed the vd/l2-withdrawals-root branch from 8d11a2e to b85c52a Compare December 17, 2024 17:41

vdamle force-pushed the vd/isthmus-config branch from bb148a3 to 6054bed Compare December 17, 2024 17:43

vdamle force-pushed the vd/l2-withdrawals-root branch 3 times, most recently from d55e52e to 144e2e1 Compare December 17, 2024 19:08

Base automatically changed from vd/isthmus-config to develop December 18, 2024 14:19

vdamle force-pushed the vd/l2-withdrawals-root branch from 144e2e1 to d07938e Compare December 18, 2024 16:34

vdamle commented Dec 20, 2024

View reviewed changes

op-e2e/actions/helpers/env.go Outdated Show resolved Hide resolved

vdamle commented Dec 20, 2024

View reviewed changes

op-e2e/actions/helpers/env.go Outdated Show resolved Hide resolved

Vinod Damle and others added 8 commits January 2, 2025 08:13

Temp commit for action test

7ec365c

Action test for genesis isthmus activation and activation after genesis

58a21b2

* test all combinations - with/without withdrawal transaction before, at and after isthmus

remove action test to move to a separate branch

3f3b6d6

raise a separate PR for it.

Rollup-config: Add IsthmusTime

2cc22d6

Also fix a test in rollup, with the correct expectation for sepolia holocene time

Fix missing WithdrawalsRoot param in ExecutableData

d6b2625

simplify withdrawals checker

1493e0b

also include a missing error when p2p validation fails

fix executionPayload initialization

bc10cb2

- do not set withdrawalsRoot if pre-isthmus - op-e2e: create a allocs file for isthmus

vdamle force-pushed the vd/l2-withdrawals-root branch from 11620de to 3419f1f Compare January 3, 2025 03:59

Update op-geth version to include state access check

e988bf7

vdamle commented Jan 3, 2025

View reviewed changes

op-e2e/opgeth/op_geth.go Outdated Show resolved Hide resolved

vdamle marked this pull request as ready for review January 3, 2025 04:16

vdamle requested review from a team as code owners January 3, 2025 04:16

vdamle requested review from refcell, zhwrd and protolambda January 3, 2025 04:16

vdamle commented Jan 3, 2025

View reviewed changes

vdamle mentioned this pull request Jan 3, 2025

[Isthmus] Add missing Isthmus activation time updates #13549

Closed

remove commented out code

3009e6d

tynes mentioned this pull request Jan 24, 2025

feat: L2 withdrawals root #13962

Merged

tynes closed this Jan 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Isthmus: Updates for L2 withdrawals root in header #12848

Isthmus: Updates for L2 withdrawals root in header #12848

vdamle commented Nov 6, 2024 •

edited

Loading

vdamle commented Nov 6, 2024 •

edited

Loading

codecov bot commented Dec 16, 2024 •

edited

Loading

vdamle Jan 3, 2025

vdamle Jan 3, 2025

vdamle Jan 3, 2025

vdamle commented Jan 3, 2025 •

edited

Loading

tynes commented Jan 24, 2025

Isthmus: Updates for L2 withdrawals root in header #12848

Isthmus: Updates for L2 withdrawals root in header #12848

Conversation

vdamle commented Nov 6, 2024 • edited Loading

vdamle commented Nov 6, 2024 • edited Loading

📚 $\text{Stack Overview}$

codecov bot commented Dec 16, 2024 • edited Loading

Codecov Report

vdamle Jan 3, 2025

Choose a reason for hiding this comment

vdamle Jan 3, 2025

Choose a reason for hiding this comment

vdamle Jan 3, 2025

Choose a reason for hiding this comment

vdamle commented Jan 3, 2025 • edited Loading

tynes commented Jan 24, 2025

vdamle commented Nov 6, 2024 •

edited

Loading

vdamle commented Nov 6, 2024 •

edited

Loading

codecov bot commented Dec 16, 2024 •

edited

Loading

vdamle commented Jan 3, 2025 •

edited

Loading