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

Moving requests out of ExecutionPayload #8600

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

lucassaldanha
Copy link
Member

@lucassaldanha lucassaldanha commented Sep 16, 2024

PR Description

  • Removes withdrawal_requests, deposit_requests and consolidation_requests from Execution Payload (execution payload is exactly like Deneb, removed all Electra classes for it);
  • Added execution_requests into Beacon Block Body (Electra)
  • Kept ExecutionPayloadV4 classes around as we will soon add some Electra-specific logic into them.
  • Updating reference tests to v1.5.0-alpha.6
  • Temporarily disabling slashing tests (will be re-enabled as part of Update Electra penalty computation. EIP7125 #8612)

Reference spec PR: https://github.com/ethereum/consensus-specs/pull/3875/files

Fixed Issue(s)

fixes #8593

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@lucassaldanha lucassaldanha added the DO NOT MERGE Not ready to merge label Sep 16, 2024
@lucassaldanha lucassaldanha changed the title [DO NOT MERGE] Moving requests out of ExecutionPayload Moving requests out of ExecutionPayload Sep 19, 2024
@lucassaldanha lucassaldanha removed the DO NOT MERGE Not ready to merge label Sep 19, 2024
@lucassaldanha lucassaldanha marked this pull request as ready for review September 19, 2024 22:20

public class BeaconBlockBodyElectra extends BeaconBlockBodyAltair {

@JsonProperty("execution_payload")
public final ExecutionPayloadElectra executionPayload;
public final ExecutionPayloadDeneb executionPayload;
Copy link
Contributor

Choose a reason for hiding this comment

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

is it worth a comment here explaining that we use ExecutionPayloadDeneb because no changes have been introduced in Electra? It's is clear now but it might be confusing in the future.

@@ -88,7 +91,8 @@ class BlindedBeaconBlockBodyElectraImpl
syncAggregate,
executionPayloadHeader,
blsToExecutionChanges,
blobKzgCommitments);
blobKzgCommitments,
executionRequests);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this missing its correspondent get method?

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.

Move requests out of execution_payload into beacon_block.body
2 participants