Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend EIP-4844 to include withdrawals #322

Merged
merged 4 commits into from
Dec 16, 2022

Conversation

Inphi
Copy link
Contributor

@Inphi Inphi commented Nov 10, 2022

Updates the EIP-4844 spec extension to include withdrawals. The ExecutionPayloadV2 header is updated to do this. As with #197, this change isn't part of the core spec.

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

nice work :)

@@ -5,6 +5,27 @@ This extension is backwards-compatible, but not part of the initial Engine API.

## Structures

### ExecutionPayloadV2
Copy link
Member

Choose a reason for hiding this comment

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

how does this interact w/ the existing ExecutionPayloadV2?

is this extension an alternative fork of the spec?

or should this be a V3 to include the excessDataGas field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's an alternative fork where withdrawals and 4844 are part of the same upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so we reuse the existing v2 apis rather than create another one

Copy link
Member

Choose a reason for hiding this comment

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

likely worth adding a note to this effect in this document

Copy link
Contributor Author

@Inphi Inphi Nov 11, 2022

Choose a reason for hiding this comment

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

This is already noted here in the document. I can edit it if the wording isn't clear enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added more wording to clarify this, including adding V2 API methods to this spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's an alternative fork where withdrawals and 4844 are part of the same upgrade.

This is different than the consensus spec. The consensus spec is treating them as different forks that are going from bellatrix -> capella -> eip4844. If we implement engine API as it is, then the Capella block processing will fail

Copy link
Contributor

Choose a reason for hiding this comment

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

So I think this should be V3

@Inphi
Copy link
Contributor Author

Inphi commented Nov 21, 2022

@ralexstokes any further comments?

@ralexstokes
Copy link
Member

I think as long as implementers know what is going on here then this is fine -- we will need to update this in the long run however as we can't have two different v2's

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

Generally LGTM. Minor nit: based on the feedback in ACD 150 it seems very unlikely withdrawals and 4844 will be done at the same time, so you may want to consider renaming the structures/methods to V3.

@Inphi
Copy link
Contributor Author

Inphi commented Dec 5, 2022

@lightclient updated the method and type names to V3.
cc: @realbigsean

@lightclient lightclient merged commit 7976d3d into ethereum:main Dec 16, 2022
@Inphi Inphi deleted the eip-4844-withdrawals branch December 16, 2022 18:04
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.

5 participants