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.

Sign up for GitHub

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: exclude empty requests in requests list #599

Merged
merged 3 commits into from
Dec 2, 2024
Merged

engine: exclude empty requests in requests list #599

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6d523bc
engine: exclude empty requests in requests list
fjl Oct 22, 2024
5f0c46e
engine: update text
fjl Oct 25, 2024
eded877
Apply suggestions from code review
mkalinin Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/engine/prague.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ Method parameter list is extended with `executionRequests`.
1. `executionPayload`: [`ExecutionPayloadV3`](./cancun.md#executionpayloadv3).
2. `expectedBlobVersionedHashes`: `Array of DATA`, 32 Bytes - Array of expected blob versioned hashes to validate.
3. `parentBeaconBlockRoot`: `DATA`, 32 Bytes - Root of the parent beacon block.
4. `executionRequests`: `Array of DATA` - List of execution layer triggered requests. Each list element is the corresponding request type's `request_data` as defined by [EIP-7685](https://eips.ethereum.org/EIPS/eip-7685). Elements of the list **MUST** be ordered by `request_type` in ascending order.
4. `executionRequests`: `Array of DATA` - List of execution layer triggered requests. Each list element is the corresponding request type's `requests` as defined by [EIP-7685](https://eips.ethereum.org/EIPS/eip-7685). Elements of the list **MUST** be ordered by `request_type` in ascending order. Elements with empty `request_data` are excluded from the list.
Copy link
Contributor

@lucassaldanha lucassaldanha Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we remove empty elements from the array, how are we gonna map the elements back to each request type? Currently, the assumption is that the element position is the identifier of their type.

On CL side, we assume that requests[0] is gonna be deposits, requests[1] are withdrawals and request[2] are consolidations.

We had previous iterations of this design where we sent the request type as part of the request (the first byte of the data). I think if we want to make this change to remove elements with empty request_data from the list we would need to add that identifier back (e.g. request_type ++ request_data)

Copy link
Contributor

Choose a reason for hiding this comment

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

For reference, Teku's implementation of the decoding:
https://github.com/Consensys/teku/blob/master/ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/execution/versions/electra/ExecutionRequestsDataCodec.java#L40-L78

Copy link
Member

Choose a reason for hiding this comment

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

Yes, great comment, and also for completeness I would go back to request_type ++ request_data, because that re-opens the possibility to have multiple request_data of the same type.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use case of multiple request_data of the same type in the engine API requests list?

Copy link
Member

Choose a reason for hiding this comment

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

There currently is none, but it allows flexibility for the future. Note that previous version of 7685 featured this, for instance: https://github.com/ethereum/EIPs/blob/4dc51ba1e3cdd63439247415da7edf36b32f9e79/EIPS/eip-7685.md (edition 26 Sep)

Copy link
Contributor

@tersec tersec Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Flexibility to do what? The engine API gets replaced, basically, each fork anyway, with a slightly-different-but-recognizable set of calls with slightly-different-but-recognizable calls

So if there "currently is none", then it's false flexibility, and with this change CLs would have to do much more specific checks for such encodings, and/or their semantics defined, for no visible advantage suggested so far. I commented a month ago in a previous iteration of these discussions at #577 (comment) about some of these.

In the Electra engine API, and that's what this is about, not some potential future (because those can be redefined anyway), it is and should be an error to randomly have two lists of deposit or consolidation requests.

If there's a specific, for-Electra reason to do this, that can be discussed, but "it allows flexibility for the future" is not a compelling reason in the face of the disadvantages of precisely this kind of "flexibility".

The only thing it adds is more pointless edge cases and failure modes to check for and detect.

Copy link
Collaborator Author

@fjl fjl Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My intention here is going back to request_type ++ request_data for each element, i.e. include the type byte. This is why I changed it back to being a list of requests instead of a list of request_data items.

The type byte is needed to distinguish the items when there are missing ones. I would not go as far as allowing multiple list items for a request_type. In fact it doesn't work for the CL because it needs to be able to map back.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The rules on the CL are the same as we had in previous iterations. One element is allowed per type byte, and the types have to be given in increasing order. In practice this means you simply need to check that the request_type of the element is > the one of the previous element. That's it.

Copy link
Contributor

@lucassaldanha lucassaldanha Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is why I changed it back to being a list of requests instead of a list of request_data items

You are correct. When I read it the first time I didn't notice the reference to request instead of request_data.

I think making the change to include the type and remove empty elements is fine. The other suggested change to allow many elements of the same type I think complicates the logic for no concrete benefit.

I support this change as I don't like the fact that we have to implicitly use the element position as their type identifier.


#### Response

Expand Down Expand Up @@ -79,7 +79,7 @@ This method follows the same specification as [`engine_getPayloadV3`](./cancun.m

1. Client software **MUST** return `-38005: Unsupported fork` error if the `timestamp` of the built payload does not fall within the time frame of the Prague fork.

2. The call **MUST** return `executionRequests` list representing execution layer triggered requests obtained from the `executionPayload` transaction execution. Each element of the list corresponds to the request type's `request_data` as defined by [EIP-7685](https://eips.ethereum.org/EIPS/eip-7685). Elements of the list **MUST** be ordered by `request_type` in ascending order.
2. The call **MUST** return `executionRequests` list representing execution layer triggered requests obtained from the `executionPayload` transaction execution. Each element of the list corresponds to the request type's `requests` as defined by [EIP-7685](https://eips.ethereum.org/EIPS/eip-7685). Elements of the list **MUST** be ordered by `request_type` in ascending order. Elements with empty `request_data` are excluded from the list.

### Update the methods of previous forks

Expand Down
Loading