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 engine api support #13978

Merged
merged 6 commits into from
May 12, 2024
Merged

electra engine api support #13978

merged 6 commits into from
May 12, 2024

Conversation

kasey
Copy link
Contributor

@kasey kasey commented May 10, 2024

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Adds support for interacting with the prague engine api to support the electra fork.

@kasey kasey force-pushed the electra-engine-api branch from b1826ea to 35dbd1c Compare May 10, 2024 00:39
@kasey kasey marked this pull request as ready for review May 10, 2024 14:49
@kasey kasey requested a review from a team as a code owner May 10, 2024 14:49
@kasey kasey requested review from nalepae, rkapka and nisdas May 10, 2024 14:49
@kasey kasey force-pushed the electra-engine-api branch from c07469a to 40a8276 Compare May 10, 2024 16:09
@kasey kasey requested review from prestonvanloon, terencechain and james-prysm and removed request for nalepae and nisdas May 10, 2024 16:48
@kasey kasey force-pushed the electra-engine-api branch from 74ca2e1 to 4bde397 Compare May 10, 2024 20:55
@terencechain terencechain changed the title WIP: electra engine api support electra engine api support May 10, 2024
return blocks.WrappedExecutionPayloadDeneb(
&pb.ExecutionPayloadDeneb{
return blocks.WrappedExecutionPayloadElectra(
&pb.ExecutionPayloadElectra{
Copy link
Member

Choose a reason for hiding this comment

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

We are missing DepositReceipts and WithdrawalRequests here. I think we will need an ExecutionPayloadBodyV2 and a new version of engine_getPayloadBodiesByRoot to accompany that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is going to be a bigger change and still a little ambiguous with upstream specs. Can we handle this in a follow-up PR?

}

func (j *ExecutionPayloadElectraJSON) ElectraDepositReceipts() []*DepositReceipt {
rcpt := make([]*DepositReceipt, 0, len(j.DepositRequests))
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why we dont want to define the size make([]*DepositReceipt, len(j.DepositRequests))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, I can write it the other way if you prefer!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it create the fixed size slices and assign to index

@kasey kasey force-pushed the electra-engine-api branch from 8b424cd to 8fe64c5 Compare May 11, 2024 15:50
@kasey kasey enabled auto-merge May 12, 2024 15:31
@kasey kasey force-pushed the electra-engine-api branch from 53343c9 to 6133f51 Compare May 12, 2024 15:42
if j.ExcessBlobGas == nil {
return errors.New("missing required field 'excessBlobGas' for ExecutionPayload")
}
return nil
Copy link
Member

Choose a reason for hiding this comment

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

Do we need check for nils here for deposit receipts and withdrawal requests?

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 think if those fields are not present in the EL response, their keys could be omitted in the response? In which case the unmarshaled fields would be an empty list or possibly nil.

If those fields are either nil or empty lists, eg ElectraExecutionLayerWithdrawalRequests will return an empty list as len and range are safe on nil list values. So I think it's safe for them to be nil as far as the unmarshaling process is concerned, and I think we probably don't want to treat such a payload as invalid. This is equivalent to how we deal with Withdrawals:

if dec.ExecutionPayload.Withdrawals == nil {
	dec.ExecutionPayload.Withdrawals = make([]*Withdrawal, 0)
}

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

@kasey kasey added this pull request to the merge queue May 12, 2024
Merged via the queue into develop with commit de177f7 May 12, 2024
16 of 17 checks passed
@kasey kasey deleted the electra-engine-api branch May 12, 2024 20:41
ErnestK pushed a commit to ErnestK/prysm that referenced this pull request May 19, 2024
* electra engine api support

* add marshaling support for ExecutionPayloadElectra

* add receipts to json tests

* deep source

* simplify slice handling

* deep source lint about type/method order

---------

Co-authored-by: Kasey Kirkham <kasey@users.noreply.github.com>
nisdas pushed a commit that referenced this pull request Jul 4, 2024
* electra engine api support

* add marshaling support for ExecutionPayloadElectra

* add receipts to json tests

* deep source

* simplify slice handling

* deep source lint about type/method order

---------

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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants