-
Notifications
You must be signed in to change notification settings - Fork 838
client: fixes for new engine api method validations for hive pr-834 #2973
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.
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
Merged
Merged
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
53b5b15
client: fixes for new engine api method validations for hive pr-834
g11tech aeaa2e8
fix spec
g11tech 0ba73f8
handle null values hive sends
g11tech 00a41f0
fix remaining errors
g11tech ea681b9
return blobs of already build payload
g11tech f31e345
fix client spec
g11tech d39364c
small fix
g11tech 90cb5de
Merge branch 'master' into hive-pr834-fixes
holgerd77 bb93642
Merge branch 'master' into hive-pr834-fixes
holgerd77 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,14 @@ import { | |
|
|
||
| import { PendingBlock } from '../../miner' | ||
| import { short } from '../../util' | ||
| import { INTERNAL_ERROR, INVALID_PARAMS, TOO_LARGE_REQUEST, UNSUPPORTED_FORK } from '../error-code' | ||
| import { | ||
| INTERNAL_ERROR, | ||
| INVALID_PARAMS, | ||
| TOO_LARGE_REQUEST, | ||
| UNKNOWN_PAYLOAD, | ||
| UNSUPPORTED_FORK, | ||
| validEngineCodes, | ||
| } from '../error-code' | ||
| import { CLConnectionManager, middleware as cmMiddleware } from '../util/CLConnectionManager' | ||
| import { middleware, validators } from '../validation' | ||
|
|
||
|
|
@@ -24,6 +31,7 @@ import type { VMExecution } from '../../execution' | |
| import type { BlobsBundle } from '../../miner' | ||
| import type { FullEthereumService } from '../../service' | ||
| import type { ExecutionPayload } from '@ethereumjs/block' | ||
| import type { Common } from '@ethereumjs/common' | ||
| import type { VM } from '@ethereumjs/vm' | ||
|
|
||
| const zeroBlockHash = zeros(32) | ||
|
|
@@ -103,7 +111,7 @@ type ExecutionPayloadBodyV1 = { | |
|
|
||
| const EngineError = { | ||
| UnknownPayload: { | ||
| code: -32001, | ||
| code: UNKNOWN_PAYLOAD, | ||
| message: 'Unknown payload', | ||
| }, | ||
| } | ||
|
|
@@ -360,6 +368,34 @@ const getPayloadBody = (block: Block): ExecutionPayloadBodyV1 => { | |
| } | ||
| } | ||
|
|
||
| function validateHardforkRange( | ||
| chainCommon: Common, | ||
| methodVersion: number, | ||
| checkNotBeforeHf: Hardfork | null, | ||
| checkNotAfterHf: Hardfork | null, | ||
| timestamp: bigint | ||
| ) { | ||
| if (checkNotBeforeHf !== null) { | ||
| const hfTimeStamp = chainCommon.hardforkTimestamp(checkNotBeforeHf) | ||
| if (hfTimeStamp !== null && timestamp < hfTimeStamp) { | ||
| throw { | ||
| code: UNSUPPORTED_FORK, | ||
| message: `V${methodVersion} cannot be called pre-${checkNotBeforeHf}`, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (checkNotAfterHf !== null) { | ||
| const nextHFTimestamp = chainCommon.nextHardforkBlockOrTimestamp(checkNotAfterHf) | ||
| if (nextHFTimestamp !== null && timestamp >= nextHFTimestamp) { | ||
| throw { | ||
| code: UNSUPPORTED_FORK, | ||
| message: `V${methodVersion + 1} MUST be called post-${checkNotAfterHf}`, | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * engine_* RPC module | ||
| * @memberof module:rpc/modules | ||
|
|
@@ -1090,6 +1126,23 @@ export class Engine { | |
| private async forkchoiceUpdatedV1( | ||
| params: [forkchoiceState: ForkchoiceStateV1, payloadAttributes: PayloadAttributesV1 | undefined] | ||
| ): Promise<ForkchoiceResponseV1 & { headBlock?: Block }> { | ||
| const payloadAttributes = params[1] | ||
| if (payloadAttributes !== undefined && payloadAttributes !== null) { | ||
| if (Object.keys(payloadAttributes).length > 3) { | ||
| throw { | ||
| code: INVALID_PARAMS, | ||
| message: 'PayloadAttributesV1 MUST be used for forkchoiceUpdatedV2', | ||
| } | ||
| } | ||
| validateHardforkRange( | ||
| this.chain.config.chainCommon, | ||
| 1, | ||
| null, | ||
| Hardfork.Paris, | ||
| BigInt(payloadAttributes.timestamp) | ||
| ) | ||
| } | ||
|
|
||
| return this.forkchoiceUpdated(params) | ||
| } | ||
|
|
||
|
|
@@ -1101,6 +1154,24 @@ export class Engine { | |
| ): Promise<ForkchoiceResponseV1 & { headBlock?: Block }> { | ||
| const payloadAttributes = params[1] | ||
| if (payloadAttributes !== undefined && payloadAttributes !== null) { | ||
| if ( | ||
| Object.values(payloadAttributes).filter((attr) => attr !== null && attr !== undefined) | ||
| .length > 4 | ||
| ) { | ||
| throw { | ||
| code: INVALID_PARAMS, | ||
| message: 'PayloadAttributesV{1|2} MUST be used for forkchoiceUpdatedV2', | ||
| } | ||
| } | ||
|
|
||
| validateHardforkRange( | ||
| this.chain.config.chainCommon, | ||
| 2, | ||
| null, | ||
| Hardfork.Shanghai, | ||
| BigInt(payloadAttributes.timestamp) | ||
| ) | ||
|
|
||
| const shanghaiTimestamp = this.chain.config.chainCommon.hardforkTimestamp(Hardfork.Shanghai) | ||
| const ts = BigInt(payloadAttributes.timestamp) | ||
| const withdrawals = (payloadAttributes as PayloadAttributesV2).withdrawals | ||
|
|
@@ -1137,14 +1208,24 @@ export class Engine { | |
| ): Promise<ForkchoiceResponseV1 & { headBlock?: Block }> { | ||
| const payloadAttributes = params[1] | ||
| if (payloadAttributes !== undefined && payloadAttributes !== null) { | ||
| const cancunTimestamp = this.chain.config.chainCommon.hardforkTimestamp(Hardfork.Cancun) | ||
| const ts = BigInt(payloadAttributes.timestamp) | ||
| if (ts < cancunTimestamp!) { | ||
| if ( | ||
| Object.values(payloadAttributes).filter((attr) => attr !== null && attr !== undefined) | ||
| .length > 5 | ||
| ) { | ||
| throw { | ||
| code: UNSUPPORTED_FORK, | ||
| message: 'PayloadAttributesV{1|2} MUST be used before Cancun is activated', | ||
| code: INVALID_PARAMS, | ||
| message: 'PayloadAttributesV3 MUST be used for forkchoiceUpdatedV3', | ||
| } | ||
| } | ||
|
|
||
| validateHardforkRange( | ||
| this.chain.config.chainCommon, | ||
| 3, | ||
| Hardfork.Cancun, | ||
| // this could be valid post cancun as well, if not then update the valid till hf here | ||
| null, | ||
| BigInt(payloadAttributes.timestamp) | ||
| ) | ||
| } | ||
|
|
||
| return this.forkchoiceUpdated(params) | ||
|
|
@@ -1158,7 +1239,7 @@ export class Engine { | |
| * 1. payloadId: DATA, 8 bytes - identifier of the payload building process | ||
| * @returns Instance of {@link ExecutionPayloadV1} or an error | ||
| */ | ||
| private async getPayload(params: [Bytes8], payloadVersion?: number) { | ||
| private async getPayload(params: [Bytes8], payloadVersion: number) { | ||
| const payloadId = params[0] | ||
| try { | ||
| const built = await this.pendingBlock.build(payloadId) | ||
|
|
@@ -1178,22 +1259,40 @@ export class Engine { | |
| this.executedBlocks.set(bytesToUnprefixedHex(block.hash()), block) | ||
| const executionPayload = blockToExecutionPayload(block, value, blobs) | ||
|
|
||
| if (payloadVersion === 3) { | ||
| const cancunTimestamp = this.chain.config.chainCommon.hardforkTimestamp(Hardfork.Cancun) | ||
| if ( | ||
| cancunTimestamp !== null && | ||
| BigInt(executionPayload.executionPayload.timestamp) < cancunTimestamp | ||
| ) { | ||
| throw { | ||
| code: UNSUPPORTED_FORK, | ||
| message: 'getPayloadV3 cannot be called on payloads pre-Cancun', | ||
| } | ||
| } | ||
| let checkNotBeforeHf: Hardfork | null | ||
| let checkNotAfterHf: Hardfork | null | ||
|
|
||
| switch (payloadVersion) { | ||
| case 3: | ||
| checkNotBeforeHf = Hardfork.Cancun | ||
| checkNotAfterHf = Hardfork.Cancun | ||
| break | ||
|
|
||
| case 2: | ||
| // no checks to be done for before as valid till paris | ||
| checkNotBeforeHf = null | ||
| checkNotAfterHf = Hardfork.Shanghai | ||
| break | ||
|
|
||
| case 1: | ||
| checkNotBeforeHf = null | ||
| checkNotAfterHf = Hardfork.Paris | ||
| break | ||
|
|
||
| default: | ||
| throw Error(`Invalid payloadVersion=${payloadVersion}`) | ||
| } | ||
|
|
||
| validateHardforkRange( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here with |
||
| this.chain.config.chainCommon, | ||
| payloadVersion, | ||
| checkNotBeforeHf, | ||
| checkNotAfterHf, | ||
| BigInt(executionPayload.executionPayload.timestamp) | ||
| ) | ||
| return executionPayload | ||
| } catch (error: any) { | ||
| if (error === EngineError.UnknownPayload) throw error | ||
| if (validEngineCodes.includes(error.code)) throw error | ||
| throw { | ||
| code: INTERNAL_ERROR, | ||
| message: error.message ?? error, | ||
|
|
@@ -1202,12 +1301,12 @@ export class Engine { | |
| } | ||
|
|
||
| async getPayloadV1(params: [Bytes8]) { | ||
| const { executionPayload } = await this.getPayload(params) | ||
| const { executionPayload } = await this.getPayload(params, 1) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here |
||
| return executionPayload | ||
| } | ||
|
|
||
| async getPayloadV2(params: [Bytes8]) { | ||
| const { executionPayload, blockValue } = await this.getPayload(params) | ||
| const { executionPayload, blockValue } = await this.getPayload(params, 2) | ||
| return { executionPayload, blockValue } | ||
| } | ||
|
|
||
|
|
||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is somewhat implicit, but this check for
forkchoiceUpdatedV1does not seem to be in the engine api specs. It states that it must be updated forengine_forkchoiceUpdatedV2(so if you call it >= Cancun it throws Unsupported fork) but by spec currently it does not listengine_forkchiceUpdatedV1, see https://github.com/ethereum/execution-apis/blob/main/src/engine/cancun.md#update-the-methods-of-previous-forksThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are some small-ish problems wiht the current spec and those changes:
Client software MUST return -32602: Invalid params error if the wrong version of the structure is used in the method call.(ofengine_newPayloadV2)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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)