engine: clarify order of operation for fcu v3

[lightclient]

I took a stab at making change c) as described by @mkalinin in this comment: #476 (comment)

A few random thoughts I have on this:

• On the surface seems good, however depending on implementation MUST may cause a fair bit of additional complexity (at least for geth). If a request is missing the required V1 param fields, we will fail early in the json-rpc handling due to unmarshal failure. In fact, I don't think we have ever returned the correct error for that. It's only if new non-V1-required fields where we do the checks in the actual method handling. Not sure how it is for others, but I have to imagine that an extremely malformed payloadAttributes will cause problems for most clients early on in the method handling (before applying fcu).
• Is this clarification even that useful? It is probably a bit more optimal, but even if forkchoice isn't applied when there are invalid payload attributes, it will be applied on the next block. And the CL should never even send this malformed data in the first place.

[jflo]

• Not sure how it is for others, but I have to imagine that an extremely malformed payloadAttributes will cause problems for most clients early on in the method handling (before applying fcu).

Besu has a similar situation. Some aspects of JSON validation happen higher up the RPC stack, well before method-specific checks later on.

[lightclient]

@jflo yeah okay I figured we weren't alone. I'm not sure how to square that fact with the spec. We're technically not conformant and to become fully "conformant" would be any annoying and kinda pointless undertaking. I wonder if we could placate the situation by noting something along the lines of client software may fail earlier during json parsing if the request is malformed without apply the forkchoice state. Or should we just leave it as-is and accept we are not perfectly conformant?

[jflo]

I'm gonna accept imperfect conformance, because if I squint I think I can see a messy path where we can refactor and not even try to parse the second payload till much later. This is the price we pay for binding the payload build to the fcu selection, and is likely worth it.

[mkalinin]

If the JSON passed onto the method call is malformed in payloadAttributes part it looks reasonable to respond with the corresponding error and do not process the request at all. Other than that, it should be fine for EL clients to run other checks on payloadAttributes after processing the fork choice state.

UPD: so I can see that there are intermediate cases between the malformed JSON and fully valid payloadAttributes where behaviour can be implementation dependent. What if a validation that payloadAttributes has all required fields and isn't malformed would be run before processing the request aside with a similar validation for the forkchoiceState?

[lightclient]

What if a validation that payloadAttributes has all required fields and isn't malformed would be run before processing the request aside with a similar validation for the forkchoiceState?

Haa I think even this would be a bit challenging for us to do, because we don't have a separate V1, V2, V3 for each object. We implement it as one object with optional fields that are added in later versions. So we only check that these optional fields exist later on.

I think my pref to resolve this would be to say:

• if the json is malformed, return error immediately
• if the payload attributes are incorrect, clients MAY apply the forkchoice state before validating attributes

Is there a reason for the second bullet to be a MUST? It feels like the CL will quickly send the next block and fcu it as latest and it's unlikely the client will again build a block and have an error in the payload attributes?

[mkalinin]

if the payload attributes are incorrect, clients MAY apply the forkchoice state before validating attributes

I think we should ask CL devs how do they handle errors in the case of fcU. It would be unfortunate if they treat payloadAttributes error as if the head payload itself is VALID (neither SYNCING nor INVALID) but then when they the request again (or setting the head later on) would learn that the payload is INVALID or SYNCING (as SYNCING eventually may turn to INVALID)

So MAY is kind of ambiguous in this case. If CL treats the payload attributes error as if the payload was not validated and they have to turn into optimistic mode then this change looks fine and I think we should say explicitly that in the case of the JSON-RPC error response a payload must not be considered as VALID

[mkalinin]

even this would be a bit challenging for us to do, because we don't have a separate V1, V2, V3 for each object. We implement it as one object with optional fields that are added in later versions. So we only check that these optional fields exist later on.

Btw, enforcing Invalid params to be returned when the forkchoiceState doesn’t have all required fields can lead into the same issue if forkchoiceState will be extended which is unlikely but still.

I think that we can say in the spec that a client MAY NOT apply the forkchoiceState and run payload validation if either the state or the attributes check fails. This will leave a room for implementations and emphasise (in the opposite to "MAY apply") that CL must not assume any payload status if the error is returned.

[mkalinin]

Re-iterating on this. Curious what are your thoughts on the following behavior:

• Invalid payload attributes is only returned when the specific check fails, i.e. the timestamp of supposed parent is greater
• Invalid payload attributes and Unsupported fork does not roll back the fork choice state. So the fork choice state must be validated and applied before running the checks corresponding to these errors, particularly it means that these checks aren’t run when SYNCING
• Other than that the spec doesn’t define whether the forkchoice state was applied or not and whether it was reverted or not allowing EL clients to return other validation errors at any point of the execution flow

edit: this behaviour wouldn't break existing CL assumptions

[lightclient]

Thanks for bumping this @mkalinin. I haven't been working on the engine api for a couple weeks and this issues takes a little time to re-load into my mind.

Reading your last comment, that behavior makes sense. Although I am having a difficult time understanding what needs to be updated in this PR to match it? Or is this PR okay as-is?

Revisiting my original concern in this PR, I think we can modify our behavior so that every field is optional, which will allow us to validate the payloadAttributes later in the flow (i.e. after the fc state is applied).

What do you think?

[mkalinin]

@lightclient

Revisiting my original concern in this PR, I think we can modify our behavior so that every field is optional, which will allow us to validate the payloadAttributes later in the flow (i.e. after the fc state is applied).

Considering that this was not only Geth’s concern but at least Besu’s as well, i’d rather not expect every EL to be capable of doing that. Thus, i’d propose the following:

1. If forkchoiceState does not match the expected fields, the client MUST return -32602: Invalid params.

2. If the client is SYNCING it MUST skip the validation of payloadAttributes.

3. If payloadAttributes does not match the expected fields, client MUST return -38003: Invalid params. If this check fails the forkchoice state MAY NOT be applied.

4. Client software MUST run the following checks over payloadAttributes after successfully applying the forkchoice state:

   1. payloadAttributes.timestamp falls within the time frame of the Cancun fork, return -38005: Unsupported fork if fails,
   2. payloadAttributes.timestamp is greater than the timestamp of a block referenced by forkchoiceState.headBlockHash, return -38003: Invalid payload attributes if fails,
   3. if any of the above checks fails the forkchoice state MUST NOT be rolled back.

[lightclient]

closed in favor of #498