Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clarify the place of payloadAttributes check against Cancun timeframes #476

Closed
wants to merge 1 commit into from
Closed

Clarify the place of payloadAttributes check against Cancun timeframes #476

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/engine/cancun.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ This method follows the same specification as [`engine_forkchoiceUpdatedV2`](./s

1. Client software **MUST** check that provided set of parameters and their fields strictly matches the expected one and return `-32602: Invalid params` error if this check fails. Any field having `null` value **MUST** be considered as not provided.

2. Client software **MUST** return `-38005: Unsupported fork` error if the `payloadAttributes` is set and the `payloadAttributes.timestamp` does not fall within the time frame of the Cancun fork.
2. Client software **MUST** return `-38005: Unsupported fork` error if the `payloadAttributes` is set and the `payloadAttributes.timestamp` does not fall within the time frame of the Cancun fork. Client software **MUST** run this check after applying the forkchoice state and, if the check fails, the forkchoice state update **MUST NOT** be rolled back.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@lightclient lightclient Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


### engine_getPayloadV3

Expand Down Expand Up @@ -184,4 +184,4 @@ For the following methods:

a validation **MUST** be added:

1. Client software **MUST** return `-38005: Unsupported fork` error if the `timestamp` of payload or payloadAttributes greater or equal to the Cancun activation timestamp.
1. Client software **MUST** return `-38005: Unsupported fork` error if the `timestamp` of payload or payloadAttributes greater or equal to the Cancun activation timestamp.
Loading