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

engine: extract execution requests from payload #587

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rkrasiuk
Copy link
Contributor

@rkrasiuk rkrasiuk commented Sep 18, 2024

Description

Follow up on ethereum/consensus-specs#3875:

  • define ExecutionRequestsV1
  • remove requests field from ExecutionPayloadV4
  • add requests field to ExecutionPayloadBodyV2
  • add executionRequests parameter to engine_newPayloadV4
  • add requests field to engine_getPayloadV4
  • update requirements for engine_getPayloadBodiesByHashV1 and engine_getPayloadBodiesByRangeV2

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.

awesome, thank you!

I think we are missing a change to engine_getPayloadV4 as well

the result object also needs the execution_requests, either in the payload type, or as a sibling property like you did for engine_newPayloadV4

@ralexstokes
Copy link
Member

#577 takes the approach to just include the execution_requests in the ExecutionPayloadV4 type, which generally has support from the recent ACD discussions around that change

@rkrasiuk
Copy link
Contributor Author

@ralexstokes updated engine_getPayloadV4

@rkrasiuk
Copy link
Contributor Author

@ralexstokes when are we going to make decision around #577? also, seems like it needs to be rebased on top of this PR to extract requests from ExecutionPayloadV4

@ralexstokes
Copy link
Member

@rkrasiuk hopefully on ACDC tomorrow! (ethereum/pm#1154)

although what I meant is that if we go with the approach in #577 we will want to change this PR to reflect that, instead of changing #577

if we keep this PR like it is now, we can remove ExecutionPayloadV4 as currently it has the same definition as ExecutionPayloadV3

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.

2 participants