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.

[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: https://github.com/ethereum/EIPs/pull/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.

[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

[mkalinin]

I agree with @tersec on not having a req on dropping empty requests by CL as EL will be doing this job anyway (during commitment computation) and passing a few bytes over Engine API is free, but if the majority of devs agree that empty requests should be dropped I am fine with that.

What I also think is weird is to have a req on the ordering:

• In case of getPayload, the ordering req turns into a redundant verification on CL side
• In case of newPayload, EL will have to do the same verification; what if EL do the ordering instead of verifying the order?

We can easily do these two things in the Engine API and I admit that these constraints are unlikely to bite us in the future, but they seem to be unnecessary. So not strongly against but feel weird when see us introducing something that isn't necessary

[fjl]

In case of newPayload, EL will have to do the same verification; what if EL do the ordering instead of verifying the order?

If the order given by CL is incorrrect, the block could fail validation because the requests will be hashed in wrong order by the commitment. We don't specifically order the requests before submitting them into the hash. The commitment 'requires the order' on paper, but in practice ordering within EL will always be correct, because we collect the requests in the right order during block processing.

[tersec]

but in practice

Not great to rely on

[mkalinin]

We don't specifically order the requests before submitting them into the hash.

I see. This sounds a bit odd to me, the order is a part of the commitment computations and the computing function relies on it to be externally set which leaks that responsibility to the caller for no reason IMHO. afaics, sorting will be trivial in terms of computations in this case, so what will be the gain of not sorting requests by type in this case?

[fjl]

I find it a bit pointless to sort the requests, since we specifically collect them in the correct order.

In Geth, and I assume in other EL implementations as well, after processing the block, we perform three calls:

1. collect deposit events into a byte array, and store it into the block requests list (type 0x00)
2. call the withdrawals contract and store the output to the list (type 0x01)
3. call the consolidations contract and store the output to the list (type 0x02)

At each step, we add the corresponding element into the list of block requests (if element non-empty). At the very end of the block, we compute the commitment over the list. If there will ever be a fork where we change how type 0x00, 0x01 or 0x02 requests are gathered, we may have to introduce explicit sorting into the logic. But as of now, and for the foreseeable future, there is no need for sorting since the requests collection will always happen in order of request type.

W.r.t. the engine API, on the CL side, the implementation likely traverses the block structure in type order as well. There is no good reason to collect them in another order. So the CL will also come up with the requests in the correct ordering without ever having to sort it explicitly.

We do want to ensure there are no bugs though, and that's the point of the validation. The CL is supposed to validate the ordering, and so is the EL when it gets the requests in newPayload. For Geth, the 'validation' will be that there is a mismatch with the block requestsHash if the CL submits requests in the wrong order.

[fjl]

About empty requests:

For a given block, if a request type has no data, it does not count towards the block requestsHash. The presence of empty requests is not observable in the consensus block either. An SSZ empty list is just empty data, it doesn't make a difference whether you explicitly store empty requests or just leave it empty.

This means, whether we transmit empty requests over the engine API or not does not lead to any observable difference in the block. I am proposing to not transmit them on the engine API because it simplifies the EL implementation: we can drop empty requests very early during processing, and no other part of the code ever has to worry about them again. They will simply not be a part of the 'block requests list' and thus will not be included as part of the response of getPayload.

[fjl]

Ultimately I just want the spec to allow leaving out empty requests on the engine API. It's more convenient for our implementation. If it is more convenient for the CLs to send them, we can permit to send them. As explained, it doesn't make any difference in the block.

I also suspect that, as request types become disused over time, it'd be weird if we had to keep sending them just to satisfy the API. But that's a weaker reason and doesn't apply today.

Perhaps a possible resolution could be changing the wording to:

Elements with empty request_data MAY be excluded from the list.