Engine API: rearrange version and fork validation in fcU and newPayload (#1848)

hive_integration/nodocker/engine/cancun/step_newpayloads.nim

@@ -211,9 +211,9 @@ method execute*(step: NewPayloads, ctx: CancunTestContext): bool =
             expectedError     = step.fcUOnPayloadRequest.getExpectedError()
             expectedStatus    = PayloadExecutionStatus.valid
             timestamp         = env.clMock.latestHeader.timestamp.uint64            
-            version           = step.fcUOnPayloadRequest.forkchoiceUpdatedVersion(timestamp)
 
           payloadAttributes = step.fcUOnPayloadRequest.getPayloadAttributes(payloadAttributes)
+          let version = step.fcUOnPayloadRequest.forkchoiceUpdatedVersion(timestamp, some(payloadAttributes.timestamp.uint64))
 
           if step.fcUOnPayloadRequest.getExpectInvalidStatus():
             expectedStatus = PayloadExecutionStatus.invalid

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,
       ]
     ),

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:

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"

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

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
   )