From 5bfdcd0d27bd6332cff24b2d0c00e413552b79f6 Mon Sep 17 00:00:00 2001 From: andri lim Date: Tue, 24 Oct 2023 11:30:48 +0700 Subject: [PATCH] Engine API: rearrange version and fork validation in fcU and newPayload (#1848) --- .../nodocker/engine/cancun/customizer.nim | 4 +- .../engine/cancun/step_newpayloads.nim | 4 +- .../nodocker/engine/cancun_tests.nim | 6 ++- hive_integration/nodocker/engine/types.nim | 2 +- nimbus/beacon/api_handler/api_forkchoice.nim | 44 +++++++++++-------- nimbus/beacon/api_handler/api_newpayload.nim | 21 +++++---- nimbus/beacon/execution_types.nim | 2 +- 7 files changed, 48 insertions(+), 35 deletions(-) diff --git a/hive_integration/nodocker/engine/cancun/customizer.nim b/hive_integration/nodocker/engine/cancun/customizer.nim index fa892513b..194a7268a 100644 --- a/hive_integration/nodocker/engine/cancun/customizer.nim +++ b/hive_integration/nodocker/engine/cancun/customizer.nim @@ -169,7 +169,7 @@ method forkchoiceUpdatedVersion(cust: UpgradeforkchoiceUpdatedVersion, headTimes let version = procCall forkchoiceUpdatedVersion(EngineAPIVersionResolver(cust), headTimestamp, payloadAttributesTimestamp) doAssert(version != Version.high, "cannot upgrade version " & $Version.high) version.succ - + # Customizer that downgrades the version of the forkchoice directive call to the previous version. type DowngradeforkchoiceUpdatedVersion* = ref object of BaseForkchoiceUpdatedCustomizer @@ -188,7 +188,7 @@ method getPayloadAttributes(cust: TimestampDeltaPayloadAttributesCustomizer, bas var customPayloadAttributes = procCall getPayloadAttributes(cust.BasePayloadAttributesCustomizer, basePayloadAttributes) customPayloadAttributes.timestamp = w3Qty(customPayloadAttributes.timestamp, cust.timestampDelta) return customPayloadAttributes - + type VersionedHashesCustomizer* = ref object of RootRef blobs*: Option[seq[BlobID]] diff --git a/hive_integration/nodocker/engine/cancun/step_newpayloads.nim b/hive_integration/nodocker/engine/cancun/step_newpayloads.nim index d9554f292..b0af64b4e 100644 --- a/hive_integration/nodocker/engine/cancun/step_newpayloads.nim +++ b/hive_integration/nodocker/engine/cancun/step_newpayloads.nim @@ -210,10 +210,10 @@ method execute*(step: NewPayloads, ctx: CancunTestContext): bool = forkchoiceState = env.clMock.latestForkchoice expectedError = step.fcUOnPayloadRequest.getExpectedError() expectedStatus = PayloadExecutionStatus.valid - timestamp = env.clMock.latestHeader.timestamp.uint64 - version = step.fcUOnPayloadRequest.forkchoiceUpdatedVersion(timestamp) + timestamp = env.clMock.latestHeader.timestamp.uint64 payloadAttributes = step.fcUOnPayloadRequest.getPayloadAttributes(payloadAttributes) + let version = step.fcUOnPayloadRequest.forkchoiceUpdatedVersion(timestamp, some(payloadAttributes.timestamp.uint64)) if step.fcUOnPayloadRequest.getExpectInvalidStatus(): expectedStatus = PayloadExecutionStatus.invalid diff --git a/hive_integration/nodocker/engine/cancun_tests.nim b/hive_integration/nodocker/engine/cancun_tests.nim index 3e6f7608b..9d9c9efc5 100644 --- a/hive_integration/nodocker/engine/cancun_tests.nim +++ b/hive_integration/nodocker/engine/cancun_tests.nim @@ -238,6 +238,7 @@ let cancunTestList* = [ ] ), ), + TestDesc( name: "Blob Transaction Ordering, Single Account 2", about: """ @@ -619,7 +620,7 @@ let cancunTestList* = [ run: specExecute, spec: CancunSpec( mainFork: ForkCancun, - forkHeight: 1, + forkHeight: 2, testSequence: @[ NewPayloads( fcUOnPayloadRequest: DowngradeForkchoiceUpdatedVersion( @@ -651,11 +652,12 @@ let cancunTestList* = [ testSequence: @[ NewPayloads( fcUOnPayloadRequest: DowngradeForkchoiceUpdatedVersion( + beaconRoot: some(common.Hash256()), expectedError: engineApiInvalidParams, ), expectationDescription: """ ForkchoiceUpdatedV2 after Cancun with beacon root field must return INVALID_PARAMS_ERROR (code $1) - """% [$engineApiInvalidParams], + """ % [$engineApiInvalidParams], ).TestStep, ] ), diff --git a/hive_integration/nodocker/engine/types.nim b/hive_integration/nodocker/engine/types.nim index 79aded5b2..3c6714034 100644 --- a/hive_integration/nodocker/engine/types.nim +++ b/hive_integration/nodocker/engine/types.nim @@ -163,7 +163,7 @@ template expectErrorCode*(res: untyped, errCode: int, expectedDesc: string) = testCond res.isErr: error "unexpected result, want error, get ok" testCond res.error.find($errCode) != -1: - fatal "DEBUG", msg=expectedDesc + fatal "DEBUG", msg=expectedDesc, got=res.error template expectNoError*(res: untyped, expectedDesc: string) = testCond res.isOk: diff --git a/nimbus/beacon/api_handler/api_forkchoice.nim b/nimbus/beacon/api_handler/api_forkchoice.nim index 24a06892f..cbaba4475 100644 --- a/nimbus/beacon/api_handler/api_forkchoice.nim +++ b/nimbus/beacon/api_handler/api_forkchoice.nim @@ -19,33 +19,39 @@ import {.push gcsafe, raises:[CatchableError].} -template validateVersion(attrsOpt, com, expectedVersion) = +template validateVersion(attrsOpt, com, apiVersion) = if attrsOpt.isSome: let attr = attrsOpt.get version = attr.version timestamp = ethTime attr.timestamp - if com.isCancunOrLater(timestamp): - if version != Version.V3: - raise invalidParams("if timestamp is Cancun or later," & - " payloadAttributes must be PayloadAttributesV3") - elif com.isShanghaiOrLater(timestamp): - if version != Version.V2: - raise invalidParams("if timestamp is Shanghai or later," & - " payloadAttributes must be PayloadAttributesV2") + if apiVersion == Version.V3: + if version != apiVersion: + raise invalidParams("forkChoiceUpdatedV3 expect PayloadAttributesV3" & + " but got PayloadAttributes" & $version) + if not com.isCancunOrLater(timestamp): + raise unsupportedFork( + "forkchoiceUpdatedV3 get invalid payloadAttributes timestamp") else: - if version != Version.V1: - raise invalidParams("if timestamp is earlier than Shanghai," & - " payloadAttributes must be PayloadAttributesV1") - - if expectedVersion == Version.V3 and version != expectedVersion: - raise invalidParams("forkChoiceUpdated" & $expectedVersion & - " expect PayloadAttributes" & $expectedVersion & - " but got PayloadAttributes" & $version) + if com.isCancunOrLater(timestamp): + if version < Version.V3: + raise unsupportedFork("forkChoiceUpdated" & $apiVersion & + " doesn't support payloadAttributes with Cancun timestamp") + if version >= Version.V3: + raise invalidParams("forkChoiceUpdated" & $apiVersion & + " doesn't support PayloadAttributes" & $version) + elif com.isShanghaiOrLater(timestamp): + if version != Version.V2: + raise invalidParams("if timestamp is Shanghai or later," & + " payloadAttributes must be PayloadAttributesV2") + else: + if version != Version.V1: + raise invalidParams("if timestamp is earlier than Shanghai," & + " payloadAttributes must be PayloadAttributesV1") proc forkchoiceUpdated*(ben: BeaconEngineRef, - expectedVersion: Version, + apiVersion: Version, update: ForkchoiceStateV1, attrsOpt: Option[PayloadAttributes]): ForkchoiceUpdatedResponse = @@ -55,7 +61,7 @@ proc forkchoiceUpdated*(ben: BeaconEngineRef, chain = ben.chain blockHash = ethHash update.headBlockHash - validateVersion(attrsOpt, com, expectedVersion) + validateVersion(attrsOpt, com, apiVersion) if blockHash == common.Hash256(): warn "Forkchoice requested update to zero hash" diff --git a/nimbus/beacon/api_handler/api_newpayload.nim b/nimbus/beacon/api_handler/api_newpayload.nim index 7527f98fc..f14e367cd 100644 --- a/nimbus/beacon/api_handler/api_newpayload.nim +++ b/nimbus/beacon/api_handler/api_newpayload.nim @@ -19,7 +19,11 @@ import {.push gcsafe, raises:[CatchableError].} -template validateVersion(com, timestamp, version, expectedVersion) = +template validateVersion(com, timestamp, version, apiVersion) = + if apiVersion == Version.V3: + if not com.isCancunOrLater(timestamp): + raise unsupportedFork("newPayloadV3 expect payload timestamp fall within Cancun") + if com.isCancunOrLater(timestamp): if version != Version.V3: raise invalidParams("if timestamp is Cancun or later, " & @@ -38,14 +42,15 @@ template validateVersion(com, timestamp, version, expectedVersion) = raise invalidParams("if timestamp is earlier than Shanghai, " & "payload must be ExecutionPayloadV1") - if expectedVersion == Version.V3 and version != expectedVersion: - raise invalidParams("newPayload" & $expectedVersion & - " expect ExecutionPayload" & $expectedVersion & - " but got ExecutionPayload" & $version) + if apiVersion == Version.V3: + if version != apiVersion: + raise invalidParams("newPayload" & $apiVersion & + " expect ExecutionPayload" & $apiVersion & + " but got ExecutionPayload" & $version) proc newPayload*(ben: BeaconEngineRef, - expectedVersion: Version, + apiVersion: Version, payload: ExecutionPayload, beaconRoot = none(Web3Hash)): PayloadStatusV1 = @@ -54,7 +59,7 @@ proc newPayload*(ben: BeaconEngineRef, number = payload.blockNumber, hash = payload.blockHash - if expectedVersion == Version.V3: + if apiVersion == Version.V3: if beaconRoot.isNone: raise invalidParams("newPayloadV3 expect beaconRoot but got none") @@ -64,7 +69,7 @@ proc newPayload*(ben: BeaconEngineRef, timestamp = ethTime payload.timestamp version = payload.version - validateVersion(com, timestamp, version, expectedVersion) + validateVersion(com, timestamp, version, apiVersion) var header = blockHeader(payload, ethHash beaconRoot) let blockHash = ethHash payload.blockHash diff --git a/nimbus/beacon/execution_types.nim b/nimbus/beacon/execution_types.nim index 1c030d675..d06f2e793 100644 --- a/nimbus/beacon/execution_types.nim +++ b/nimbus/beacon/execution_types.nim @@ -115,7 +115,7 @@ func V3*(attr: PayloadAttributes): PayloadAttributesV3 = timestamp: attr.timestamp, prevRandao: attr.prevRandao, suggestedFeeRecipient: attr.suggestedFeeRecipient, - withdrawals: attr.withdrawals.get, + withdrawals: attr.withdrawals.get(newSeq[WithdrawalV1]()), parentBeaconBlockRoot: attr.parentBeaconBlockRoot.get )