engine: clarify payloadAttributes checks and fcu v3 processing flow

[mkalinin]

This PR incorporates changes proposed by #476, #479 and results from the conversations happening in-person during the DevConnect and in the Discord after the event.

The list of proposed changes:

1. Update fcu v1 spec to explicitly state that payloadAttributes validations and build process can only be run after forkchoiceState is processed and only if the head is VALID implying that if a client is SYNCING no payloadAttributes validations are run.
2. State that forkchoiceState fields must be validated before any processing starts with the corresponding error on failure.
3. Define a new sequence of payloadAttributes checks that must be run after applying the fork choice state.
   The first step of this sequence is to verify that payloadAttributes matches the expected fields and return -38003: Invalid payload attributes on failure. This step is what can cause additional engineering efforts on EL side. The problem here is that EL clients define PayloadAttributesV1 fields as required and if any of them is missed then the method will return with an error before processing the forkchoiceState, this behaviour would contradict the proposed change. cc @jflo @lightclient @MarekM25 @yperbasis

Hive test scenarios covering forkchoiceState and payloadAttributes checks should be validated when this PR gets merged. cc @marioevz

Supersedes #476 #479

[lightclient]

LGTM

[jflo]

adding -38003 here helps a lot

[g11tech]

lgtm

[marioevz]

LGTM!

Just as a heads up, this change will result in adding hive tests where we send malformed payload attributes to syncing clients, which very likely will result in new failures. We will tag all EL devs on the hive PR that implements these tests.

[mkalinin]

LGTM!

Just as a heads up, this change will result in adding hive tests where we send malformed payload attributes to syncing clients, which very likely will result in new failures. We will tag all EL devs on the hive PR that implements these tests.

@marioevz note that this change also includes the following statement to the fcU V1 that is inherently applicable to all further versions:

7. Client software MUST process provided payloadAttributes after successfully applying the forkchoiceState and only if the payload referenced by forkchoiceState.headBlockHash is VALID. The processing flow is as follows:

One of the intentions of that statement is skipping payloadAttributes validation and processing if the response is supposed to be SYNCING. If that intention isn’t clear from the change then I think it should be clarified additionally.

[flcl42]

It's not clear how we extend point 7 of Paris fork. Might be better just to add this rules to Paris also in expected order

[mkalinin]

One of this rules is in Paris which is checking the timestamp wrt parent block’s timestamp. Other two are introduced in Cancun, so they aren’t relevant for Paris. The point (7) in Cancun has an extended list of checks wrt to Paris

[spencer-tb]

I also had some confusion wrt to this. We have an engine api test:

ForkchoiceUpdatedV2 To Request Shanghai Payload, Non-Null Beacon Root

where we essentially fcuV2 before Cancun, with a Shanghai timestamp and a non null parentBeaconBlockRoot (incorrect Payload Attributes structure).

As we are before Cancun my assumption would be that the extended points to (7) are not yet applied. Most clients return -38003: Invalid payload attributes for this case. The addition to Paris only validates the Payload Attributes based on the timestamp. So to my understanding we don't explicitly state what to return in this case.

So I agree with @flcl42 that we could add the Payload Attributes structure check to Paris. Apologies if I am missing something here

[spencer-tb]

Ahh, I just noticed that this check would be covered by: https://github.com/ethereum/execution-apis/blob/main/src/engine/shanghai.md#engine_forkchoiceupdatedv2. And the appropraite response should be -32602: Invalid params.

As most clients return -38003: Invalid payload attributes would it make sense to change it here. Similarly, I personally found this tricky to determine based on the current spec regarding the Payload Attributes structure check (but I could be the stupid odd one out).

I added a potential change for the latter here: main...spencer-tb:execution-apis:src/engine/payload-attributes-updates

[mkalinin]

why should the parent.timestamp check be removed from cancun?
Also, i am hesitant to change the previous versions of any methods in the spec unless the change is coherent with the behavior of the majority of EL clients

[mkalinin]

Do EL clients check payloadAttributes structure in case of V1? If we remove a check from V3 then it is not clear in which order they are applied which may result in the return error message to be different on the same payloadAttributes in different client implementations

[spencer-tb]

why should the parent.timestamp check be removed from cancun?

My fault - it should be kept, I wasn't taking into consideration the order of the checks!

I'd like to double check our tests for the payloadAttributes structure in case of V1. Will get back to you :)