Execution API: execution requests serialization#14498
Execution API: execution requests serialization#14498james-prysm wants to merge 2 commits intodevelopfrom
Conversation
| return er, nil | ||
| } | ||
|
|
||
| func verifyExecutionRequests(er *ExecutionRequests) error { |
There was a problem hiding this comment.
Did I miss any verifications?
| // GetDecodedExecutionRequests decodes the byte array in ExecutionBundleElectra and returns a ExecutionRequests container object | ||
| // based on specifications from EIP-7685 | ||
| func (eb *ExecutionBundleElectra) GetDecodedExecutionRequests() (*ExecutionRequests, error) { | ||
| if len(eb.ExecutionRequests) == 0 { |
There was a problem hiding this comment.
should this error out or return empty?
There was a problem hiding this comment.
I think this should be the empty requests, we need to guarantee that the slice is not nil before though.
| // GetDecodedExecutionRequests decodes the byte array in ExecutionBundleElectra and returns a ExecutionRequests container object | ||
| // based on specifications from EIP-7685 | ||
| func (eb *ExecutionBundleElectra) GetDecodedExecutionRequests() (*ExecutionRequests, error) { | ||
| if len(eb.ExecutionRequests) == 0 { |
There was a problem hiding this comment.
I think this should be the empty requests, we need to guarantee that the slice is not nil before though.
| if uint64(len(er.Deposits)) > params.BeaconConfig().MaxDepositRequestsPerPayload { | ||
| return fmt.Errorf("too many deposits requested: received %d, expected %d", len(er.Deposits), params.BeaconConfig().MaxDepositRequestsPerPayload) | ||
| } | ||
| if uint64(len(er.Withdrawals)) > params.BeaconConfig().MaxWithdrawalsPerPayload { |
There was a problem hiding this comment.
This should be withdrawals requests per payload, not withdrawals
| return fmt.Errorf("too many deposits requested: received %d, expected %d", len(er.Deposits), params.BeaconConfig().MaxDepositRequestsPerPayload) | ||
| } | ||
| if uint64(len(er.Withdrawals)) > params.BeaconConfig().MaxWithdrawalsPerPayload { | ||
| return fmt.Errorf("too many withdrawals requested: received %d, expected %d", len(er.Withdrawals), params.BeaconConfig().MaxWithdrawalRequestsPerPayload) |
| er := &ExecutionRequests{} | ||
| if err := processRequestBytes(eb.ExecutionRequests, er); err != nil { |
There was a problem hiding this comment.
if you are passing an empty request container here anyway, why not use the signature
func processRequestBytes([]byte); (*ExecutionRequests, error)
| } | ||
|
|
||
| // Recursive call with the remaining bytes | ||
| return processRequestBytes(remainingBytes, requests) |
There was a problem hiding this comment.
I think a loop instead of a recursive algorithm is better here, the tail recursion elimination in this case is simple.
| } | ||
|
|
||
| // EncodeExecutionRequests is a helper function that takes a ExecutionRequests container and converts it into a hash used for `engine_NewPayloadV4` | ||
| func EncodeExecutionRequests(eb *ExecutionRequests) (common.Hash, error) { |
There was a problem hiding this comment.
Pending questions here on whether ordering would matter to the EL. cc @fjl
|
Closing this because of the spec changes @james-prysm feel free to reopen if you reuse the branch |
2 participants
What type of PR is this?
Other
What does this PR do? Why is it needed?
implements the serialization of common hash for use in
engine_NewPayloadV4as well as decoding of the byte array inengine_getPayloadV4prerequisite for #14492
Which issues(s) does this PR fix?
part of ethereum/consensus-specs#3818
Other notes for review
Acknowledgements