Engine API: a bunch of cleanups#338
Merged
mkalinin merged 1 commit intoethereum:mainfrom Jan 12, 2023
Merged
Conversation
lightclient
approved these changes
Jan 9, 2023
mkalinin
commented
Jan 10, 2023
| 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. |
Contributor
Author
There was a problem hiding this comment.
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?
Contributor
There was a problem hiding this comment.
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
Contributor
Author
There was a problem hiding this comment.
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.
2 tasks
siladu
added a commit
to hyperledger/besu
that referenced
this pull request
Jan 18, 2023
Update engine API to include some new changes introduced in - ethereum/execution-apis#338 - ethereum/execution-apis#337 Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com> Co-authored-by: Simon Dudley <simon.dudley@consensys.net>
yperbasis
added a commit
to erigontech/erigon
that referenced
this pull request
Jan 23, 2023
elenduuche
pushed a commit
to elenduuche/besu
that referenced
this pull request
Aug 16, 2023
Update engine API to include some new changes introduced in - ethereum/execution-apis#338 - ethereum/execution-apis#337 Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com> Co-authored-by: Simon Dudley <simon.dudley@consensys.net>
eum602
pushed a commit
to lacchain/besu
that referenced
this pull request
Nov 3, 2023
Update engine API to include some new changes introduced in - ethereum/execution-apis#338 - ethereum/execution-apis#337 Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: Zhenyang Shi <wcgcyx@gmail.com> Co-authored-by: Simon Dudley <simon.dudley@consensys.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR proposes the following cleanups to the Shanghai part of the spec:
INVALID_BLOCK_HASHstatus and supplant it withINVALID, see [RFC] Engine API: remove INVALID_BLOCK_HASH #270 for details (should be a one liner, if not let's debate)If EL client implementations use the same piece of code for both
V1andV2the latter two changes may become a problem as they seem to be backwards incompatible withV1. Though, practically it isn't that true as the transition has happened quite long time ago and having no TTD validation should not break anything except for Hive tests (though we should probably remove transition tests from Hive). All CL clients handlestatus: INVALID, latestValidHash: nullin the same way asstatus: INVALID_BLOCK_HASH, latestValidHash: null, replacing the latter with the former must not change anything.