client: fixes for new engine api method validations for hive pr-834 #2973
Conversation
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
g11tech
force-pushed
the
hive-pr834-fixes
branch
from
914f6f0 to
f31e345
Compare
g11tech
added
PR state: merge ready
package: client
target: master
| } | ||
| validateHardforkRange( | ||
| this.chain.config.chainCommon, | ||
| 1, |
There was a problem hiding this comment.
I think it is somewhat implicit, but this check for forkchoiceUpdatedV1 does not seem to be in the engine api specs. It states that it must be updated for engine_forkchoiceUpdatedV2 (so if you call it >= Cancun it throws Unsupported fork) but by spec currently it does not list engine_forkchiceUpdatedV1, see https://github.com/ethereum/execution-apis/blob/main/src/engine/cancun.md#update-the-methods-of-previous-forks
There was a problem hiding this comment.
hmmm yes, should we PR to update specs because obviously lower version can't be used in higher version fork because of missing fields
There was a problem hiding this comment.
I think there are some small-ish problems wiht the current spec and those changes:
- We have 3 files, Paris / Shanghai / Cancun. In Cancun we introduce new rules which override the rules of the other files (this becomes clear once you read all files). The rule which it overrides if for example this Shanghai rule:
Client software MUST return -32602: Invalid params error if the wrong version of the structure is used in the method call.(ofengine_newPayloadV2) - And building upon the previous: in Cancun we should use
INVALID_PARAMSif we callengine_newPayloadV1(if we are on Shanghai), but this thus implies we are aware (and ready) for Cancun. If we don't know about Cancun and we want to change the spec, then now the behavior changes: if we are on Shanghai, and we callengine_newPayloadV1, then we should returnUNSUPPORTED_FORK.
So if we change the spec this starts to get somewhat confusing. I do think we should change the spec though because it is more logical. I am also not aware what happens if we change the behavior, are there CLs relying on exact error codes to change behavior, or do they treat each error as an error in general?
There was a problem hiding this comment.
may be we can address those issues separately, for now we can may be merge this as it passes cancun fully (and V1 checks also makes sense)
| } | ||
|
|
||
| validateHardforkRange( |
There was a problem hiding this comment.
Same here with engine_getPayloadV1
| @@ -1202,12 +1301,12 @@ export class Engine { | |||
| } | |||
|
|
|||
| async getPayloadV1(params: [Bytes8]) { | |||
| const { executionPayload } = await this.getPayload(params) | |||
| const { executionPayload } = await this.getPayload(params, 1) | |||
|
Updated this via UI |
|
Updated this via UI |
|
Yes, would agree that spec questions can addressed separately, will merge for now. |
addresses the new validations added in
passes all
engine-cancuntests: