Clarify the place of payloadAttributes check against Cancun timeframes

[mkalinin]

Explicitly state that checking payloadAttributes.timestamp against Cancun time frames must be run after applying the forkchoice state and the result of the check must not affect the forkchoice update.

cc @LukaszRozmej @marioevz

[LukaszRozmej]

This should apply also to point 1. if the Invalid params comes from payload validation

[lightclient]

I think the add line might be clearer if it is point 1? Something like: "Apply the forkchoice state before verifying the payload attributes, do not roll back state if attributes are invalid"

[mkalinin]

By receiving Unsupported fork or Invalid payload attributes errors CL can conclude that the forkchoice state has been applied successfully, while if Invalid params is received it wouldn't be clear which part of the request this error relates to. The order of checks looks pretty messy in this particular method call but I think that that the consistency of data passed onto the request should be validated before any state updates

[lightclient]

I don't think I quite understand your point here. Are you saying you think that if 1) here fails, the forkchoice should not be updated?

Generally it feels like we should say very explicitly, if the forkchoice update state is valid, clients MUST update the forkchoice before we even look at the params. But it seems like you might be saying first check that the params is well formed and then after forkchoice update, you need verify the semantic checks on the params?

[mkalinin]

If 1) fails it can also mean that e.g. headBlockHash wasn't provided thus the forkchoice update can't happen.

So, the pipeline can be as follows:

1. Check that the forkchoice state is well formed, return Invalid params if not
2. Apply the forkchoice state and if VALID and payloadAttributes are provided goto 3, break otherwise
3. Check that payloadAttributes are well formed (all fields are provided) and if not return either Invalid params OR Invalid payload attributes
4. Check payloadAttributes.timestamp against headBlock.timestamp, return Invalid payload attributes if failed
5. Check payloadAttributes.timestamp against Cancun timeframes, return Unsupported fork if failed

In 3) if Invalid params is returned, there will be no possibility for CL to understand that the forkchoice state has been updated if it was. I am not standing strong on that we should preserve such behavior but it is a slight change in the semantics and I see three options here:

a) check that the request is well formed before doing any downstream processing and return Invalid params if fails even in the case when the forkchoice state part is valid, i.e. do not apply the forkchoice state if payloadAttributes aren't well formed
b) return Invalid params error separately for payloadAttributes, notify CL client devs about this change
c) return Invalid payload attributes if payloadAttributes aren't well formed and leave Invalid params error for the forkchoice state only

I am slightly in favour of c) because it is simple and seems to fit well for the optional parameter which payloadAttributes is

[lightclient]

Thanks for this, I understand now. I agree with you that it is probably important for CL to know for certain if the forkchoice was applied or not, so it is good to not allow Invalid params to be returned both before fcu update and after.

After thinking about this, I am also in favour of c). I think it will allow the specification to read a bit more clearly (e.g. first check fcu state and apply, then check payload attributes).

[lightclient]

I took a stab at c), pls take a look: #479

[mkalinin]

Closing in favour of #498