Skip to content

Isthmus: L2 Withdrawals root spec updates#396

Merged
protolambda merged 9 commits intomainfrom
vd/h-l2-withdrawals-root
Oct 25, 2024
Merged

Isthmus: L2 Withdrawals root spec updates#396
protolambda merged 9 commits intomainfrom
vd/h-l2-withdrawals-root

Conversation

@vdamle
Copy link
Contributor

@vdamle vdamle commented Sep 27, 2024

Description

Spec Updates for Isthumus: L2 Withdrawals root spec updates

Tests

None

Metadata

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

tynes commented Sep 27, 2024

Nice work on this so far!

Copy link
Contributor

@protolambda protolambda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start

@vdamle vdamle force-pushed the vd/h-l2-withdrawals-root branch from 1e8e5a2 to 82d0756 Compare October 7, 2024 05:25
@vdamle vdamle changed the title Holocene: L2 Withdrawals root spec updates Isthumus: L2 Withdrawals root spec updates Oct 7, 2024
@vdamle vdamle marked this pull request as ready for review October 8, 2024 09:17
@vdamle vdamle requested review from protolambda and tynes October 8, 2024 09:19
@vdamle vdamle force-pushed the vd/h-l2-withdrawals-root branch from dd44455 to 4dd7ad6 Compare October 8, 2024 16:46
@vdamle vdamle requested a review from mslipper as a code owner October 9, 2024 10:13
@vdamle vdamle changed the title Isthumus: L2 Withdrawals root spec updates Isthmus: L2 Withdrawals root spec updates Oct 9, 2024
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@tynes tynes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, would like approval from @ajsutton or @protolambda before merge

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good generally, just a couple of bits that I think could use more clarity. For the P2P stuff I don't have all the context so may just be misunderstanding things.

I am very strongly of the opinion that it's a mistake to have the genesis root hard code an empty hash for withdrawalsRoot instead of using the actual value from the account storage. The stateRoot field will be changing with any changes to the actual storage root anyway.

@vdamle vdamle force-pushed the vd/h-l2-withdrawals-root branch from a76969a to f19e845 Compare October 22, 2024 14:05
@vdamle vdamle requested a review from ajsutton October 22, 2024 16:30
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@protolambda protolambda merged commit 0ca4559 into main Oct 25, 2024
@protolambda protolambda deleted the vd/h-l2-withdrawals-root branch October 25, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

4 participants