simplify ELMonitor fcU payload attributes handling (#4696)

beacon_chain/beacon_node_light_client.nim

@@ -67,7 +67,8 @@ proc initLightClient*(
               discard await node.elManager.forkchoiceUpdated(
                 headBlockHash = payload.block_hash,
                 safeBlockHash = beaconHead.safeExecutionPayloadHash,
-                finalizedBlockHash = beaconHead.finalizedExecutionPayloadHash)
+                finalizedBlockHash = beaconHead.finalizedExecutionPayloadHash,
+                payloadAttributes = NoPayloadAttributes)
           else: discard
 
     optimisticProcessor = initOptimisticProcessor(

beacon_chain/consensus_object_pools/consensus_manager.nim

@@ -112,10 +112,9 @@ proc expectBlock*(self: var ConsensusManager, expectedSlot: Slot): Future[bool]
 
   return fut
 
-from eth/async_utils import awaitWithTimeout
 from web3/engine_api_types import
-  ForkchoiceUpdatedResponse,
+  ForkchoiceUpdatedResponse, PayloadExecutionStatus, PayloadStatusV1,
-  PayloadExecutionStatus, PayloadStatusV1, PayloadAttributesV1
+  PayloadAttributesV1
 
 func `$`(h: BlockHash): string = $h.asEth2Digest
 
@@ -168,9 +167,10 @@ proc updateExecutionClientHead(self: ref ConsensusManager,
   # Can't use dag.head here because it hasn't been updated yet
   let (payloadExecutionStatus, latestValidHash) =
     await self.elManager.forkchoiceUpdated(
-      headExecutionPayloadHash,
+      headBlockHash = headExecutionPayloadHash,
-      newHead.safeExecutionPayloadHash,
+      safeBlockHash = newHead.safeExecutionPayloadHash,
-      newHead.finalizedExecutionPayloadHash)
+      finalizedBlockHash = newHead.finalizedExecutionPayloadHash,
+      payloadAttributes = NoPayloadAttributes)
 
   case payloadExecutionStatus
   of PayloadExecutionStatus.valid:
@@ -340,32 +340,29 @@ proc runProposalForkchoiceUpdated*(
   if headBlockHash.isZero:
     return
 
-  let
+  try:
-    payloadAttributes = withState(self.dag.headState):
+    let safeBlockHash = beaconHead.safeExecutionPayloadHash
+
+    withState(self.dag.headState):
+      template callForkchoiceUpdated(fcPayloadAttributes: auto) =
+        let (status, _) = await self.elManager.forkchoiceUpdated(
+          headBlockHash, safeBlockHash,
+          beaconHead.finalizedExecutionPayloadHash,
+          payloadAttributes = fcPayloadAttributes)
+        debug "Fork-choice updated for proposal", status
+
+      static: doAssert high(ConsensusFork) == ConsensusFork.Deneb
       when stateFork >= ConsensusFork.Capella:
-        ForkedPayloadAttributes(
+        callForkchoiceUpdated(PayloadAttributesV2(
-          kind: ForkedPayloadAttributesKind.v2,
-          v2: PayloadAttributesV2(
           timestamp: Quantity timestamp,
           prevRandao: FixedBytes[32] randomData,
           suggestedFeeRecipient: feeRecipient,
           withdrawals: toEngineWithdrawals get_expected_withdrawals(forkyState.data)))
       else:
-        ForkedPayloadAttributes(
+        callForkchoiceUpdated(PayloadAttributesV1(
-          kind: ForkedPayloadAttributesKind.v1,
-          v1: PayloadAttributesV1(
           timestamp: Quantity timestamp,
           prevRandao: FixedBytes[32] randomData,
           suggestedFeeRecipient: feeRecipient))
-  try:
-    let
-      safeBlockHash = beaconHead.safeExecutionPayloadHash
-      (status, _) = await self.elManager.forkchoiceUpdated(
-        headBlockHash,
-        safeBlockHash,
-        beaconHead.finalizedExecutionPayloadHash,
-        payloadAttributes = payloadAttributes)
-    debug "Fork-choice updated for proposal", status
   except CatchableError as err:
     error "Engine API fork-choice update failed", err = err.msg
 

beacon_chain/eth1/eth1_monitor.nim

@@ -112,22 +112,15 @@ type
     hasConsensusViolation: bool
       ## The local chain contradicts the observed consensus on the network
 
-  ForkedPayloadAttributesKind* {.pure.} = enum
+  NoPayloadAttributesType = object
-    v1
+    ## A type with exactly one value, and which is not constructed via a `nil`
-    v2
+    ## value for a ref object, which which Nim 1.6 crashes with an ICE.
-
-  ForkedPayloadAttributes* = ref object
-    case kind*: ForkedPayloadAttributesKind
-    of ForkedPayloadAttributesKind.v1:
-      v1*: PayloadAttributesV1
-    of ForkedPayloadAttributesKind.v2:
-      v2*: PayloadAttributesV2
 
   NextExpectedPayloadParams* = object
     headBlockHash*: Eth2Digest
     safeBlockHash*: Eth2Digest
     finalizedBlockHash*: Eth2Digest
-    payloadAttributes: ForkedPayloadAttributes
+    payloadAttributes*: PayloadAttributesV2
 
   ELManager* = ref object
     eth1Network: Option[Eth1Network]
@@ -241,6 +234,9 @@ type
     GetPayloadV2Response |
     CancunExecutionPayloadAndBlobs
 
+const
+  NoPayloadAttributes* = default(NoPayloadAttributesType)
+
 declareCounter failed_web3_requests,
   "Failed web3 requests"
 
@@ -616,10 +612,10 @@ proc close(connection: ELConnection): Future[void] {.async.} =
     awaitWithTimeout(connection.web3.get.close(), 30.seconds):
       debug "Failed to close data provider in time"
 
-proc isConnected(connection: ELConnection): bool =
+func isConnected(connection: ELConnection): bool =
   connection.web3.isSome
 
-proc getJsonRpcRequestHeaders(jwtSecret: Option[seq[byte]]):
+func getJsonRpcRequestHeaders(jwtSecret: Option[seq[byte]]):
     auto =
   if jwtSecret.isSome:
     let secret = jwtSecret.get
@@ -694,65 +690,29 @@ func areSameAs(expectedParams: Option[NextExpectedPayloadParams],
                randomData: Eth2Digest,
                feeRecipient: Eth1Address,
                withdrawals: seq[WithdrawalV1]): bool =
-  if not(expectedParams.isSome and
+  expectedParams.isSome and
     expectedParams.get.headBlockHash == latestHead and
     expectedParams.get.safeBlockHash == latestSafe and
-         expectedParams.get.finalizedBlockHash == latestFinalized):
+    expectedParams.get.finalizedBlockHash == latestFinalized and
-    return false
+    expectedParams.get.payloadAttributes.timestamp.uint64 == timestamp and
-
+    expectedParams.get.payloadAttributes.prevRandao.bytes == randomData.data and
-  if expectedParams.get.payloadAttributes == nil:
+    expectedParams.get.payloadAttributes.suggestedFeeRecipient == feeRecipient and
-    return false
+    expectedParams.get.payloadAttributes.withdrawals == withdrawals
-
-  case expectedParams.get.payloadAttributes.kind
-  of ForkedPayloadAttributesKind.v1:
-    expectedParams.get.payloadAttributes.v1.timestamp.uint64 == timestamp and
-    expectedParams.get.payloadAttributes.v1.prevRandao.bytes == randomData.data and
-    expectedParams.get.payloadAttributes.v1.suggestedFeeRecipient == feeRecipient and
-    withdrawals.len == 0
-  of ForkedPayloadAttributesKind.v2:
-    expectedParams.get.payloadAttributes.v2.timestamp.uint64 == timestamp and
-    expectedParams.get.payloadAttributes.v2.prevRandao.bytes == randomData.data and
-    expectedParams.get.payloadAttributes.v2.suggestedFeeRecipient == feeRecipient and
-    expectedParams.get.payloadAttributes.v2.withdrawals == withdrawals
-
-template makeForkedPayloadAttributes(
-    GetPayloadResponseType: type BellatrixExecutionPayloadWithValue,
-    timestamp: uint64,
-    randomData: Eth2Digest,
-    suggestedFeeRecipient: Eth1Address,
-    withdrawals: seq[WithdrawalV1]): ForkedPayloadAttributes =
-  ForkedPayloadAttributes(
-    kind: ForkedPayloadAttributesKind.v1,
-    v1: engine_api.PayloadAttributesV1(
-      timestamp: Quantity timestamp,
-      prevRandao: FixedBytes[32] randomData.data,
-      suggestedFeeRecipient: suggestedFeeRecipient))
-
-template makeForkedPayloadAttributes(
-    GetPayloadResponseType: typedesc[engine_api.GetPayloadV2Response|CancunExecutionPayloadAndBlobs],
-    timestamp: uint64,
-    randomData: Eth2Digest,
-    suggestedFeeRecipient: Eth1Address,
-    withdrawals: seq[WithdrawalV1]): ForkedPayloadAttributes =
-  ForkedPayloadAttributes(
-    kind: ForkedPayloadAttributesKind.v2,
-    v2: engine_api.PayloadAttributesV2(
-      timestamp: Quantity timestamp,
-      prevRandao: FixedBytes[32] randomData.data,
-      suggestedFeeRecipient: suggestedFeeRecipient,
-      withdrawals: withdrawals))
 
 proc forkchoiceUpdated(rpcClient: RpcClient,
                        state: ForkchoiceStateV1,
-                       payloadAttributes: ForkedPayloadAttributes): Future[ForkchoiceUpdatedResponse] =
+                       payloadAttributes: PayloadAttributesV1 |
-  if payloadAttributes == nil:
+                                          PayloadAttributesV2 |
+                                          NoPayloadAttributesType):
+                       Future[ForkchoiceUpdatedResponse] =
+  when payloadAttributes is NoPayloadAttributesType:
     rpcClient.engine_forkchoiceUpdatedV1(state, none PayloadAttributesV1)
+  elif payloadAttributes is PayloadAttributesV1:
+    rpcClient.engine_forkchoiceUpdatedV1(state, some payloadAttributes)
+  elif payloadAttributes is PayloadAttributesV2:
+    rpcClient.engine_forkchoiceUpdatedV2(state, some payloadAttributes)
   else:
-    case payloadAttributes.kind
+    static: doAssert false
-    of ForkedPayloadAttributesKind.v1:
-      rpcClient.engine_forkchoiceUpdatedV1(state, some payloadAttributes.v1)
-    of ForkedPayloadAttributesKind.v2:
-      rpcClient.engine_forkchoiceUpdatedV2(state, some payloadAttributes.v2)
 
 func computeBlockValue(blk: ExecutionPayloadV1): UInt256 {.raises: [RlpError, Defect].} =
   for transactionBytes in blk.transactions:
@@ -777,17 +737,29 @@ proc getPayloadFromSingleEL(
     elif not headBlock.isZero:
       engine_api_last_minute_forkchoice_updates_sent.inc(1, [connection.engineUrl.url])
 
+      when GetPayloadResponseType is BellatrixExecutionPayloadWithValue:
         let response = await rpcClient.forkchoiceUpdated(
           ForkchoiceStateV1(
             headBlockHash: headBlock.asBlockHash,
             safeBlockHash: safeBlock.asBlockHash,
             finalizedBlockHash: finalizedBlock.asBlockHash),
-        makeForkedPayloadAttributes(
+          PayloadAttributesV1(
-          GetPayloadResponseType,
+            timestamp: Quantity timestamp,
-          timestamp,
+            prevRandao: FixedBytes[32] randomData.data,
-          randomData,
+            suggestedFeeRecipient: suggestedFeeRecipient))
-          suggestedFeeRecipient,
+      elif GetPayloadResponseType is engine_api.GetPayloadV2Response or GetPayloadResponseType is CancunExecutionPayloadAndBlobs:
-          withdrawals))
+        let response = await rpcClient.forkchoiceUpdated(
+          ForkchoiceStateV1(
+            headBlockHash: headBlock.asBlockHash,
+            safeBlockHash: safeBlock.asBlockHash,
+            finalizedBlockHash: finalizedBlock.asBlockHash),
+          PayloadAttributesV2(
+            timestamp: Quantity timestamp,
+            prevRandao: FixedBytes[32] randomData.data,
+            suggestedFeeRecipient: suggestedFeeRecipient,
+            withdrawals: withdrawals))
+      else:
+        static: doAssert false
 
       if response.payloadStatus.status != PayloadExecutionStatus.valid or
          response.payloadId.isNone:
@@ -820,7 +792,7 @@ proc getPayloadFromSingleEL(
   else:
     return await engine_api.getPayload(rpcClient, GetPayloadResponseType, payloadId)
 
-proc cmpGetPayloadResponses(lhs, rhs: SomeEnginePayloadWithValue): int =
+func cmpGetPayloadResponses(lhs, rhs: SomeEnginePayloadWithValue): int =
   cmp(distinctBase lhs.blockValue, distinctBase rhs.blockValue)
 
 template EngineApiResponseType*(T: type bellatrix.ExecutionPayloadForSigning): type =
@@ -960,7 +932,7 @@ proc waitELToSyncDeposits(connection: ELConnection,
       await sleepAsync(seconds(30))
       rpcClient = await connection.connectedRpcClient()
 
-proc networkHasDepositContract(m: ELManager): bool =
+func networkHasDepositContract(m: ELManager): bool =
   not m.cfg.DEPOSIT_CONTRACT_ADDRESS.isDefaultValue
 
 func mostRecentKnownBlock(m: ELManager): BlockHash =
@@ -1039,7 +1011,7 @@ type
     oldStatusIsOk
     disagreement
 
-proc compareStatuses(prevStatus, newStatus: PayloadExecutionStatus): StatusRelation =
+func compareStatuses(prevStatus, newStatus: PayloadExecutionStatus): StatusRelation =
   case prevStatus
   of PayloadExecutionStatus.syncing:
     if newStatus == PayloadExecutionStatus.syncing:
@@ -1089,7 +1061,7 @@ type
     selectedResponse: Option[int]
     disagreementAlreadyDetected: bool
 
-proc init(T: type ELConsensusViolationDetector): T =
+func init(T: type ELConsensusViolationDetector): T =
   ELConsensusViolationDetector(selectedResponse: none int,
                                disagreementAlreadyDetected: false)
 
@@ -1168,7 +1140,8 @@ proc sendNewPayload*(m: ELManager,
 proc forkchoiceUpdatedForSingleEL(
     connection: ELConnection,
     state: ref ForkchoiceStateV1,
-    payloadAttributes: ForkedPayloadAttributes):
+    payloadAttributes: PayloadAttributesV1 | PayloadAttributesV2 |
+                       NoPayloadAttributesType):
     Future[PayloadStatusV1] {.async.} =
   let
     rpcClient = await connection.connectedRpcClient()
@@ -1187,7 +1160,8 @@ proc forkchoiceUpdatedForSingleEL(
 
 proc forkchoiceUpdated*(m: ELManager,
                         headBlockHash, safeBlockHash, finalizedBlockHash: Eth2Digest,
-                        payloadAttributes: ForkedPayloadAttributes = nil):
+                        payloadAttributes: PayloadAttributesV1 | PayloadAttributesV2 |
+                                           NoPayloadAttributesType):
                         Future[(PayloadExecutionStatus, Option[BlockHash])] {.async.} =
   doAssert not headBlockHash.isZero
 
@@ -1205,11 +1179,26 @@ proc forkchoiceUpdated*(m: ELManager,
   if m.elConnections.len == 0:
     return (PayloadExecutionStatus.syncing, none BlockHash)
 
+  when payloadAttributes is PayloadAttributesV2:
+    template payloadAttributesV2(): auto = payloadAttributes
+  elif payloadAttributes is PayloadAttributesV1:
+    template payloadAttributesV2(): auto = PayloadAttributesV2(
+      timestamp: payloadAttributes.timestamp,
+      prevRandao: payloadAttributes.prevRandao,
+      suggestedFeeRecipient: payloadAttributes.suggestedFeeRecipient,
+      withdrawals: @[])
+  elif payloadAttributes is NoPayloadAttributesType:
+    template payloadAttributesV2(): auto =
+      # Because timestamp and prevRandao are both 0, won't false-positive match
+      (static(default(PayloadAttributesV2)))
+  else:
+    static: doAssert false
+
   m.nextExpectedPayloadParams = some NextExpectedPayloadParams(
     headBlockHash: headBlockHash,
     safeBlockHash: safeBlockHash,
     finalizedBlockHash: finalizedBlockHash,
-    payloadAttributes: payloadAttributes)
+    payloadAttributes: payloadAttributesV2)
 
   let
     state = newClone ForkchoiceStateV1(
@@ -1258,12 +1247,12 @@ proc forkchoiceUpdated*(m: ELManager,
 
 proc forkchoiceUpdatedNoResult*(m: ELManager,
                                 headBlockHash, safeBlockHash, finalizedBlockHash: Eth2Digest,
-                                payloadAttributes: ForkedPayloadAttributes = nil) {.async.} =
+                                payloadAttributes: PayloadAttributesV1 | PayloadAttributesV2) {.async.} =
   discard await m.forkchoiceUpdated(
     headBlockHash, safeBlockHash, finalizedBlockHash, payloadAttributes)
 
 # TODO can't be defined within exchangeConfigWithSingleEL
-proc `==`(x, y: Quantity): bool {.borrow, noSideEffect.}
+func `==`(x, y: Quantity): bool {.borrow.}
 
 proc exchangeConfigWithSingleEL(m: ELManager, connection: ELConnection) {.async.} =
   let rpcClient = await connection.connectedRpcClient()
@@ -1686,7 +1675,7 @@ template getBlockProposalData*(m: ELManager,
   getBlockProposalData(
     m.eth1Chain, state, finalizedEth1Data, finalizedStateDepositIndex)
 
-proc new*(T: type ELConnection,
+func new*(T: type ELConnection,
           engineUrl: EngineApiUrl): T =
   ELConnection(
     engineUrl: engineUrl,

beacon_chain/gossip_processing/block_processor.nim

@@ -213,7 +213,7 @@ proc storeBackfillBlock(
 
 from web3/engine_api_types import PayloadExecutionStatus, PayloadStatusV1
 from ../eth1/eth1_monitor import
-  ELManager, asEngineExecutionPayload, sendNewPayload,
+  ELManager, NoPayloadAttributes, asEngineExecutionPayload, sendNewPayload,
   forkchoiceUpdated, forkchoiceUpdatedNoResult
 
 proc expectValidForkchoiceUpdated(
@@ -222,7 +222,10 @@ proc expectValidForkchoiceUpdated(
     receivedBlock: ForkySignedBeaconBlock): Future[void] {.async.} =
   let
     (payloadExecutionStatus, _) = await elManager.forkchoiceUpdated(
-      headBlockHash, safeBlockHash, finalizedBlockHash)
+      headBlockHash = headBlockHash,
+      safeBlockHash = safeBlockHash,
+      finalizedBlockHash = finalizedBlockHash,
+      payloadAttributes = NoPayloadAttributes)
     receivedExecutionBlockHash =
       when typeof(receivedBlock).toFork >= ConsensusFork.Bellatrix:
         receivedBlock.message.body.execution_payload.block_hash
@@ -260,7 +263,6 @@ from ../consensus_object_pools/spec_cache import get_attesting_indices
 from ../spec/datatypes/phase0 import TrustedSignedBeaconBlock
 from ../spec/datatypes/altair import SignedBeaconBlock
 
-from eth/async_utils import awaitWithTimeout
 from ../spec/datatypes/bellatrix import ExecutionPayload, SignedBeaconBlock
 from ../spec/datatypes/capella import
   ExecutionPayload, SignedBeaconBlock, asTrusted, shortLog
@@ -511,7 +513,8 @@ proc storeBlock*(
       discard await elManager.forkchoiceUpdated(
         headBlockHash = self.consensusManager[].optimisticExecutionPayloadHash,
         safeBlockHash = newHead.get.safeExecutionPayloadHash,
-        finalizedBlockHash = newHead.get.finalizedExecutionPayloadHash)
+        finalizedBlockHash = newHead.get.finalizedExecutionPayloadHash,
+        payloadAttributes = NoPayloadAttributes)
     else:
       let
         headExecutionPayloadHash =

beacon_chain/nimbus_light_client.nim

@@ -7,7 +7,7 @@
 
 import
   std/os,
-  chronicles, chronicles/chronos_tools, chronos, stew/io2,
+  chronicles, chronos, stew/io2,
   eth/db/kvstore_sqlite3, eth/keys,
   ./eth1/eth1_monitor,
   ./gossip_processing/optimistic_processor,
@@ -114,7 +114,8 @@ programMain:
               discard await elManager.forkchoiceUpdated(
                 headBlockHash = payload.block_hash,
                 safeBlockHash = payload.block_hash,  # stub value
-                finalizedBlockHash = ZERO_HASH)
+                finalizedBlockHash = ZERO_HASH,
+                payloadAttributes = NoPayloadAttributes)
         else: discard
     optimisticProcessor = initOptimisticProcessor(
       getBeaconTime, optimisticHandler)