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

Electra payload body engine methods #14000

Merged
merged 2 commits into from
May 17, 2024
Merged

Conversation

kasey
Copy link
Contributor

@kasey kasey commented May 15, 2024

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This adds support for unblinding electra blocks. That includes a rewrite of the code for interacting with the engine API via engine_getPayloadBodiesByHashV1 so that we can deal with the fact that we can no longer call only one engine method, because engine_getPayloadBodiesByHashV2 needs to be used for post-electra blocks.

I also added a fallback path to request bodies via the engine_getPayloadBodiesByRangeV(1|2) methods when engine_getPayloadBodiesByHashV(1|2) responds with a nil value, because the engine API docs for engine_getPayloadBodiesByHashV1 stipulates Client software MAY NOT respond to requests for finalized blocks by hash..

To support testing I added a new engine api mock server type in mock_test.go as well as types and utility methods to support un/marshaling requests, parameters (hashes and uints), converting ExecutionPayloads -> a payload bodies that can be easily marshaled.

Most of the test coverage has been preserved by way of ReconstructFull(Bellatrix)Block(Batch) using the new code under the hood, but a number of new tests for the new request type were also added.

@kasey kasey marked this pull request as ready for review May 15, 2024 03:07
@kasey kasey requested a review from a team as a code owner May 15, 2024 03:07
@kasey kasey force-pushed the electra-engine-payload-methods branch from e41b7de to 63fef7b Compare May 15, 2024 03:08
beacon-chain/execution/payload_body_test.go Show resolved Hide resolved
beacon-chain/execution/payload_body_test.go Show resolved Hide resolved
ExcessBlobGas: ebg,
BlobGasUsed: bgu,
DepositReceipts: pb.JsonDepositRequestsToProto(body.DepositRequests),
WithdrawalRequests: pb.JsonWithdrawalRequestsToProto(body.WithdrawalRequests),
}, big.NewInt(0)) // We can't get the block value and don't care about the block value for this instance
Copy link
Member

Choose a reason for hiding this comment

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

I know this isn't code you changed, but why can't we get the value? header.ValueInWei() or the gwei version?

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 doesn't exist because it isn't part of the payload, it is a separate field in the new payload response or builder api response for a blinded block bid. This doesn't really belong in the ExecutionData interface. It's a hack for bundling values together during proposals. I actually have another branch to remove this value from the ExecutionData.

@prestonvanloon prestonvanloon added the Electra electra hardfork label May 15, 2024
@@ -510,49 +522,33 @@ func (s *Service) HeaderByNumber(ctx context.Context, number *big.Int) (*types.H
}

// GetPayloadBodiesByHash returns the relevant payload bodies for the provided block hash.
func (s *Service) GetPayloadBodiesByHash(ctx context.Context, executionBlockHashes []common.Hash) ([]*pb.ExecutionPayloadBodyV1, error) {
func (s *Service) GetPayloadBodiesByHash(ctx context.Context, executionBlockHashes []common.Hash) ([]*pb.ExecutionPayloadBodyV1Or2, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just have this be ExecutionPayloadBody since it includes all versions. And just put a note that it's for both V1 and V2?

Suggested change
func (s *Service) GetPayloadBodiesByHash(ctx context.Context, executionBlockHashes []common.Hash) ([]*pb.ExecutionPayloadBodyV1Or2, error) {
func (s *Service) GetPayloadBodiesByHash(ctx context.Context, executionBlockHashes []common.Hash) ([]*pb.ExecutionPayloadBody, error) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair enough!

@kasey kasey force-pushed the electra-engine-payload-methods branch from befb14d to 1c68c21 Compare May 16, 2024 02:13
// ExecutionPayloadBody represents the engine API ExecutionPayloadV1 or ExecutionPayloadV2 type.
type ExecutionPayloadBody struct {
Transactions []hexutil.Bytes `json:"transactions"`
Withdrawals []*Withdrawal `json:"withdrawals"`
Copy link
Contributor

Choose a reason for hiding this comment

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

why do Withdrawals need to be a pointer but WithdrawalRequests not a pointer?

Copy link
Contributor Author

@kasey kasey May 16, 2024

Choose a reason for hiding this comment

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

(Deposit|Withdrawal)Requests are a new type where I'm trying out something different, whereas Withdrawals uses the existing pattern with its own un/marshal methods. I want to compare the ux of the two approaches - see also the functional helpers that the new types use.

@kasey kasey force-pushed the electra-engine-payload-methods branch from e5f8d40 to 78bc05c Compare May 17, 2024 05:24
@prestonvanloon
Copy link
Member

Tests failing

Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@saolyn saolyn left a comment

Choose a reason for hiding this comment

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

LGTM

@kasey kasey added this pull request to the merge queue May 17, 2024
Merged via the queue into develop with commit 4616860 May 17, 2024
17 checks passed
@kasey kasey deleted the electra-engine-payload-methods branch May 17, 2024 21:13
ErnestK pushed a commit to ErnestK/prysm that referenced this pull request May 19, 2024
* Combined v1/v2 payload body handling

* prevent overflows when dealing with electra fixture

---------

Co-authored-by: Kasey Kirkham <kasey@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Electra electra hardfork
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants