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

engine: clarify payloadAttributes checks and fcu v3 processing flow #498

Merged
merged 2 commits into from
Dec 13, 2023
Merged

engine: clarify payloadAttributes checks and fcu v3 processing flow #498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f72f1fa
Update sequence of checks run over payloadAttributes
mkalinin Dec 1, 2023
e474732
Clarify matching statements
mkalinin Dec 4, 2023
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
14 changes: 11 additions & 3 deletions src/engine/cancun.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,19 @@ Refer to the response for [`engine_forkchoiceUpdatedV2`](./shanghai.md#engine_fo

#### Specification

This method follows the same specification as [`engine_forkchoiceUpdatedV2`](./shanghai.md#engine_forkchoiceupdatedv2) with addition of the following:
This method follows the same specification as [`engine_forkchoiceUpdatedV2`](./shanghai.md#engine_forkchoiceupdatedv2) with the following changes to the processing flow:

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.
1. Client software **MUST** verify that `forkchoiceState` matches the [`ForkchoiceStateV1`](./paris.md#ForkchoiceStateV1) structure and return `-32602: Invalid params` on failure.

2. Extend point (7) of the `engine_forkchoiceUpdatedV1` [specification](./paris.md#specification-1) by defining the following sequence of checks that **MUST** be run over `payloadAttributes`:

1. `payloadAttributes` matches the [`PayloadAttributesV3`](#payloadattributesv3) structure, return `-38003: Invalid payload attributes` on failure.
Copy link
Contributor

@flcl42 flcl42 Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

@spencer-tb spencer-tb Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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


2. `payloadAttributes.timestamp` falls within the time frame of the Cancun fork, return `-38005: Unsupported fork` on failure.

3. `payloadAttributes.timestamp` is greater than `timestamp` of a block referenced by `forkchoiceState.headBlockHash`, return `-38003: Invalid payload attributes` on failure.

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.
4. If any of the above checks fails, the `forkchoiceState` update **MUST NOT** be rolled back.

### engine_getPayloadV3

Expand Down
10 changes: 7 additions & 3 deletions src/engine/paris.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,11 +216,15 @@ The payload build process is specified as follows:

6. Client software **MUST** return `-38002: Invalid forkchoice state` error if the payload referenced by `forkchoiceState.headBlockHash` is `VALID` and a payload referenced by either `forkchoiceState.finalizedBlockHash` or `forkchoiceState.safeBlockHash` does not belong to the chain defined by `forkchoiceState.headBlockHash`.

7. Client software **MUST** ensure that `payloadAttributes.timestamp` is greater than `timestamp` of a block referenced by `forkchoiceState.headBlockHash`. If this condition isn't held client software **MUST** respond with `-38003: Invalid payload attributes` and **MUST NOT** begin a payload build process. In such an event, the `forkchoiceState` update **MUST NOT** be rolled back.
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:

8. Client software **MUST** begin a payload build process building on top of `forkchoiceState.headBlockHash` and identified via `buildProcessId` value if `payloadAttributes` is not `null` and the forkchoice state has been updated successfully. The build process is specified in the [Payload building](#payload-building) section.
1. Verify that `payloadAttributes.timestamp` is greater than `timestamp` of a block referenced by `forkchoiceState.headBlockHash` and return `-38003: Invalid payload attributes` on failure.

9. Client software **MUST** respond to this method call in the following way:
2. If `payloadAttributes` passes all valdiation steps, begin a payload build process building on top of `forkchoiceState.headBlockHash` and identified via `buildProcessId` value. The build process is specified in the [Payload building](#payload-building) section.

3. If `payloadAttributes` validation fails, the `forkchoiceState` update **MUST NOT** be rolled back.

8. Client software **MUST** respond to this method call in the following way:
* `{payloadStatus: {status: SYNCING, latestValidHash: null, validationError: null}, payloadId: null}` if `forkchoiceState.headBlockHash` references an unknown payload or a payload that can't be validated because requisite data for the validation is missing
* `{payloadStatus: {status: INVALID, latestValidHash: validHash, validationError: errorMessage | null}, payloadId: null}` obtained from the [Payload validation](#payload-validation) process if the payload is deemed `INVALID`
* `{payloadStatus: {status: INVALID, latestValidHash: 0x0000000000000000000000000000000000000000000000000000000000000000, validationError: errorMessage | null}, payloadId: null}` obtained either from the [Payload validation](#payload-validation) process or as a result of validating a terminal PoW block referenced by `forkchoiceState.headBlockHash`
Expand Down
Loading