engine: exclude empty requests in requests list

[fjl]

This is to mirror a change in EIP-7685, where we exclude requests with empty request_data from the commitment.

[lucassaldanha]

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)

[lucassaldanha]

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

[jochem-brouwer]

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.

[tersec]

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

[jochem-brouwer]

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)

[tersec]

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.

[fjl]

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.

[fjl]

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.

[lucassaldanha]

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.

[tersec]

This is to mirror a change in EIP-7685, where we exclude requests with empty request_data from the commitment.

Why?

[jochem-brouwer]

@tersec Motivation is here: ethereum/EIPs#8989

The reason is that if requests are empty, then it will not matter on a chain which has 7685 activated whatever requests are activated. Previously, the activated request EIPs would change the "empty hash" (if there are no requests) which would depend upon what EIPs were activated.

[tersec]

@tersec Motivation is here: ethereum/EIPs#8989

The reason is that if requests are empty, then it will not matter on a chain which has 7685 activated whatever requests are activated. Previously, the activated request EIPs would change the "empty hash" (if there are no requests) which would depend upon what EIPs were activated.

I'll clarify: what is the motivation for the engine API to mirror this? It's a change in how the hash is calculated, but that's separate from the engine API.

[fjl]

I'll clarify: what is the motivation for the engine API to mirror this?

We need this to make the list relayed on the engine API the same list as is used for the commitment. I think it's important to keep these two the same.

[lucassaldanha]

We need this to make the list relayed on the engine API the same list as is used for the commitment. I think it's important to keep these two the same.

As far as I can see there is no "technical" reason to do so and they could be different. However, I think this change has two advantages:

1. Remove the need to rely on the element index to identify the type of requests: this is even more important for the future, as we add/remove request types.
2. Reduces the "cognitive load" of thinking about requests. CL sends requests back to EL exactly as they were received. If we don't include empty lists, then EL does not have to remove them before calculating the hash, etc.

To @tersec point, it is not NEEDED but I think it is a nice addition. And the implementation cost on CL side is minimal.

[tersec]

1. Remove the need to rely on the element index to identify the type of requests: this is even more important for the future, as we add/remove request types.

Engine API V3, V4, etc operates within that fork anyway. So if in that fork there's a subset of admissible request types, only those would be part of the interface, both for getPayload and newPayload. The accommodates adding and removing request types.

2. Reduces the "cognitive load" of thinking about requests. CL sends requests back to EL exactly as they were received. If we don't include empty lists, then EL does not have to remove them before calculating the hash, etc.

The CL sending back requests exactly as received is already the case. The EL hash literally adds

-    for r in requests:
-        m.update(sha256(r))
+    for r in block_requests:
+        if len(r) > 1:

That's not particularly different whether the block_requests come in singles or the 3 (currently) lists. So already the EL is doing this "remove them before calculating the hash", right now in the current set of proposals.

[ensi321]

Elements with empty request_data are excluded from the list.

Is it worth mentioning in the case that all 3 (deposit, withdrawal, consolidation) requests are missing, executionRequests will still contain an empty array?

[fjl]

Do you feel strongly that it has to be mentioned? All other parameters are also mandatory, but we do not write "parentBeaconBlockRoot must not be null" either. I feel that the type, Array of DATA, already means it will always be present and a JSON array.

[james-prysm]

if an execution request type is provided but the remaining bytes for the list not provided or is shorter than 1 request I guess we should error and not accept. or if it's too long I guess