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 API: a bunch of cleanups #338

Merged
merged 1 commit into from
Jan 12, 2023
Merged

Engine API: a bunch of cleanups #338

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
12 changes: 11 additions & 1 deletion src/engine/shanghai.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,13 @@ This structure has the syntax of `PayloadAttributesV1` and appends a single fiel
* method: `engine_newPayloadV2`
* params:
1. [`ExecutionPayloadV2`](#ExecutionPayloadV2)
* timeout: 8s

#### Response

Refer to the response for [`engine_newPayloadV1`](./paris.md#engine_newpayloadv1).
* result: [`PayloadStatusV1`](./paris.md#payloadstatusv1), values of the `status` field are restricted in the following way:
- `INVALID_BLOCK_HASH` status value is supplanted by `INVALID`.
* error: code and message set in case an exception happens while processing the payload.

#### Specification

Expand All @@ -92,6 +95,8 @@ This method follows the same specification as [`engine_newPayloadV1`](./paris.md
Similarly, if the functionality is not activated, client software **MUST** return an `INVALID` status with the appropriate `latestValidHash` if `payloadAttributes.withdrawals` is not `null`.
Blocks without withdrawals **MUST** be expressed with an explicit empty list `[]` value.
Refer to the validity conditions for [`engine_newPayloadV1`](./paris.md#engine_newpayloadv1) to specification of the appropriate `latestValidHash` value.
2. Client software **MAY NOT** validate terminal PoW block conditions during payload validation (point (2) in the [Payload validation](./paris.md#payload-validation) routine).
3. Client software **MUST** return `{status: INVALID, latestValidHash: null, validationError: errorMessage | null}` if the `blockHash` validation has failed.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alternatively, we may spec this part as following:

Suggested change
3. Client software **MUST** return `{status: INVALID, latestValidHash: null, validationError: errorMessage | null}` if the `blockHash` validation has failed.
3. Client software **SHOULD** return `{status: INVALID, latestValidHash: null, validationError: errorMessage | null}` if the `blockHash` validation has failed.

This allows clients and tests to keep using INVALID_BLOCK_HASH, i.e. no mandatory changes to the client code are imposed by this PR. @lightclient thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

if we want to leave the old path open, I think it has to be MUST and then specify it must be one or the other. otherwise, the way SHOULD reads is that they can deprecate the old path and then choice to respond this way or not

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With a MUST V1 responds with INVALID_BLOCK_HASH while V2 must not do that, which makes it more difficult to use the same handler for both versions of the method. I would keep MUST and re-iterate if pushback.


### engine_forkchoiceUpdatedV2

Expand All @@ -101,6 +106,7 @@ This method follows the same specification as [`engine_newPayloadV1`](./paris.md
* params:
1. `forkchoiceState`: `Object` - instance of [`ForkchoiceStateV1`](./paris.md#ForkchoiceStateV1)
2. `payloadAttributes`: `Object|null` - instance of [`PayloadAttributesV2`](#PayloadAttributesV2) or `null`
* timeout: 8s

#### Response

Expand All @@ -113,6 +119,9 @@ This method follows the same specification as [`engine_forkchoiceUpdatedV1`](./p
1. If withdrawal functionality is activated, client software **MUST** return error `-38003: Invalid payload attributes` if `payloadAttributes.withdrawals` is `null`.
Similarly, if the functionality is not activated, client software **MUST** return error `-38003: Invalid payload attributes` if `payloadAttributes.withdrawals` is not `null`.
Blocks without withdrawals **MUST** be expressed with an explicit empty list `[]` value.
2. Client software **MAY NOT** validate terminal PoW block conditions in the following places:
- during payload validation (point (2) in the [Payload validation](./paris.md#payload-validation) routine specification),
- when updating the forkchoice state (point (3) in the [`engine_forkchoiceUpdatedV1`](./paris.md#engine_forkchoiceupdatedv1) method specification).

### engine_getPayloadV2

Expand All @@ -121,6 +130,7 @@ This method follows the same specification as [`engine_forkchoiceUpdatedV1`](./p
* method: `engine_getPayloadV2`
* params:
1. `payloadId`: `DATA`, 8 Bytes - Identifier of the payload build process
* timeout: 1s

#### Response

Expand Down