query EL and builder relay for bids in parallel (#4749)

* outline for comparing bids from builder and engine API in BN

* set up async

* decision scaffold

* clean up logging

* Refactor proposeBlockMEV

* Update beacon_chain/validators/validator_duties.nim

Co-authored-by: zah <zahary@status.im>

* Update beacon_chain/validators/validator_duties.nim

Co-authored-by: zah <zahary@status.im>

* use typedescs instead of explicit generic parameters

---------

Co-authored-by: zah <zahary@status.im>

beacon_chain/validators/validator_duties.nim

@@ -85,7 +85,7 @@ declarePublicGauge(attached_validator_balance_total,
 logScope: topics = "beacval"
 
 type
-  ForkedBlockResult* = Result[ForkedBeaconBlock, string]
+  ForkedBlockResult = Result[ForkedBeaconBlock, string]
 
   SyncStatus* {.pure.} = enum
     synced
@@ -392,7 +392,7 @@ proc makeBeaconBlockForHeadAndSlot*(
               get_expected_withdrawals(forkyState.data))
             if  withdrawals_root.isNone or
                 hash_tree_root(withdrawals) != withdrawals_root.get:
-              # TODO: Why don't we fallback to the EL payload here?
+              # If engine API returned a block, will use that
               return err("Builder relay provided incorrect withdrawals root")
             # Otherwise, the state transition function notices that there are
             # too few withdrawals.
@@ -675,12 +675,12 @@ proc getBlindedBlockParts[EPH: ForkyExecutionPayloadHeader](
 
   return ok((executionPayloadHeader.get, forkedBlck))
 
-proc proposeBlockMEV[
+proc getBuilderBid[
     SBBB: bellatrix_mev.SignedBlindedBeaconBlock |
           capella_mev.SignedBlindedBeaconBlock](
     node: BeaconNode, head: BlockRef, validator: AttachedValidator, slot: Slot,
     randao: ValidatorSig, validator_index: ValidatorIndex):
-    Future[Opt[BlockRef]] {.async.} =
+    Future[Result[SBBB, string]] {.async.} =
   # Used by the BN's own validators, but not the REST server
   when SBBB is bellatrix_mev.SignedBlindedBeaconBlock:
     type EPH = bellatrix.ExecutionPayloadHeader
@@ -695,7 +695,7 @@ proc proposeBlockMEV[
   if blindedBlockParts.isErr:
     # Not signed yet, fine to try to fall back on EL
     beacon_block_builder_missed_with_fallback.inc()
-    return Opt.none BlockRef
+    return err blindedBlockParts.error()
 
   # These, together, get combined into the blinded block for signing and
   # proposal through the relay network.
@@ -730,23 +730,18 @@ proc proposeBlockMEV[
         Result[SBBB, string].err("getBlindedBlock timed out")
 
   if blindedBlock.isErr:
-    info "proposeBlockMEV: getBlindedBeaconBlock failed",
+    return err blindedBlock.error()
-      slot, head = shortLog(head), validator_index, blindedBlock,
-      error = blindedBlock.error
-    return Opt.none BlockRef
 
-  # Before unblindAndRouteBlockMEV, can fall back to EL; after, cannot
+  return blindedBlock
+
+proc proposeBlockMEV(node: BeaconNode, blindedBlock: auto):
+    Future[Result[BlockRef, string]] {.async.} =
   let unblindedBlockRef = await node.unblindAndRouteBlockMEV(
     blindedBlock.get)
   return if unblindedBlockRef.isOk and unblindedBlockRef.get.isSome:
     beacon_blocks_proposed.inc()
-    unblindedBlockRef.get
+    ok(unblindedBlockRef.get.get)
   else:
-    # Signal to the caller that a signed, blinded beacon block was sent to the
-    # builder API server, at which point no local EL fallback can occur. Using
-    # non-`none` opt with the same head indicates this to proposeBlock(), with
-    # any non-`none` return value indicating this in general.
-    #
     # unblindedBlockRef.isOk and unblindedBlockRef.get.isNone indicates that
     # the block failed to validate and integrate into the DAG, which for the
     # purpose of this return value, is equivalent. It's used to drive Beacon
@@ -756,11 +751,7 @@ proc proposeBlockMEV[
         unblindedBlockRef.error
       else:
         "Unblinded block failed either to validate or integrate into validated store"
-    warn "proposeBlockMEV: blinded block either not successfully unblinded or not successfully proposed",
+    err errMsg
-      head = shortLog(head), slot, validator_index,
-      validator = shortLog(validator),
-      err = errMsg, blindedBlck = shortLog(blindedBlock.get)
-    Opt.some head
 
 proc makeBlindedBeaconBlockForHeadAndSlot*[
     BBB: bellatrix_mev.BlindedBeaconBlock | capella_mev.BlindedBeaconBlock](
@@ -820,85 +811,104 @@ proc makeBlindedBeaconBlockForHeadAndSlot*[
     else:
       return err("Attempt to create pre-Bellatrix blinded block")
 
-proc proposeBlock(node: BeaconNode,
+proc proposeBlockAux(
-                  validator: AttachedValidator,
+    SBBB: typedesc, EPS: typedesc, node: BeaconNode,
-                  validator_index: ValidatorIndex,
+    validator: AttachedValidator, validator_index: ValidatorIndex,
-                  head: BlockRef,
+    head: BlockRef, slot: Slot, randao: ValidatorSig, fork: Fork,
-                  slot: Slot): Future[BlockRef] {.async.} =
+    genesis_validators_root: Eth2Digest): Future[BlockRef] {.async.} =
-  if head.slot >= slot:
+  # Collect bids
-    # We should normally not have a head newer than the slot we're proposing for
+  let usePayloadBuilder =
-    # but this can happen if block proposal is delayed
-    warn "Skipping proposal, have newer head already",
-      headSlot = shortLog(head.slot),
-      headBlockRoot = shortLog(head.root),
-      slot = shortLog(slot)
-    return head
-
-  let
-    fork = node.dag.forkAtEpoch(slot.epoch)
-    genesis_validators_root = node.dag.genesis_validators_root
-    randao =
-      block:
-        let res = await validator.getEpochSignature(
-          fork, genesis_validators_root, slot.epoch)
-        if res.isErr():
-          warn "Unable to generate randao reveal",
-               validator = shortLog(validator), error_msg = res.error()
-          return head
-        res.get()
-
     if node.config.payloadBuilderEnable:
-    let failsafeInEffect =
       withState(node.dag.headState):
         # Head slot, not proposal slot, matters here
-        livenessFailsafeInEffect(
+        # TODO it might make some sense to allow use of builder API if local
+        # EL fails -- i.e. it would change priorities, so any block from the
+        # execution layer client would override builder API. But it seems an
+        # odd requirement to produce no block at all in those conditions.
+        not livenessFailsafeInEffect(
           forkyState.data.block_roots.data, forkyState.data.slot)
-    if not failsafeInEffect:
-      let newBlockMEV =
-        if slot.epoch >= node.dag.cfg.DENEB_FORK_EPOCH:
-          debugRaiseAssert $denebImplementationMissing & ": proposeBlock"
-          await proposeBlockMEV[
-              capella_mev.SignedBlindedBeaconBlock](
-            node, head, validator, slot, randao, validator_index)
-        elif slot.epoch >= node.dag.cfg.CAPELLA_FORK_EPOCH:
-          await proposeBlockMEV[
-              capella_mev.SignedBlindedBeaconBlock](
-            node, head, validator, slot, randao, validator_index)
     else:
-          await proposeBlockMEV[
+      false
-              bellatrix_mev.SignedBlindedBeaconBlock](
-            node, head, validator, slot, randao, validator_index)
 
-      if newBlockMEV.isSome:
+  let
-        # This might be equivalent to the `head` passed in, but it signals that
+    payloadBuilderBidFut =
-        # `submitBlindedBlock` ran, so don't do anything else. Otherwise, it is
+      if usePayloadBuilder:
-        # fine to try again with the local EL.
+        getBuilderBid[SBBB](node, head, validator, slot, randao, validator_index)
-        if newBlockMEV.get == head:
+      else:
-          # Returning same block as head indicates failure to generate new block
+        let fut = newFuture[Result[SBBB, string]]("builder-bid")
+        fut.complete(Result[SBBB, string].err(
+          "either payload builder disabled or liveness failsafe active"))
+        fut
+    engineBlockFut = makeBeaconBlockForHeadAndSlot(
+      EPS, node, randao, validator_index, node.graffitiBytes, head, slot)
+
+  # getBuilderBid times out after BUILDER_PROPOSAL_DELAY_TOLERANCE, with 1 more
+  # second for remote validators. makeBeaconBlockForHeadAndSlot times out after
+  # 1 second.
+  await allFutures(payloadBuilderBidFut, engineBlockFut)
+  doAssert payloadBuilderBidFut.finished and engineBlockFut.finished
+
+  let builderBidAvailable =
+    if payloadBuilderBidFut.completed:
+      if payloadBuilderBidFut.read().isOk:
+        true
+      elif usePayloadBuilder:
+        info "Payload builder error",
+          slot, head = shortLog(head), validator = shortLog(validator),
+          err = payloadBuilderBidFut.read().error()
+        false
+      else:
+        # Effectively the same case, but without the log message
+        false
+    else:
+      info "Payload builder bid future failed",
+        slot, head = shortLog(head), validator = shortLog(validator),
+        err = payloadBuilderBidFut.error.msg
+      false
+
+  let engineBidAvailable =
+    if engineBlockFut.completed:
+      if engineBlockFut.read().isOk:
+        true
+      else:
+        info "Engine block building error",
+          slot, head = shortLog(head), validator = shortLog(validator),
+          err = payloadBuilderBidFut.read().error()
+        false
+    else:
+      info "Engine block building failed",
+        slot, head = shortLog(head), validator = shortLog(validator),
+        err = engineBlockFut.error.msg
+      false
+
+  let useBuilderBlock =
+    if builderBidAvailable:
+      if engineBidAvailable:
+        true
+      else:
+        true
+    else:
+      if not engineBidAvailable:
+        return head   # errors logged in router
+      false
+
+  if useBuilderBlock:
+    let
+      blindedBlock = payloadBuilderBidFut.read()
+      # Before proposeBlockMEV, can fall back to EL; after, cannot without
+      # risking slashing.
+      maybeUnblindedBlock = await proposeBlockMEV(node, blindedBlock)
+
+    return maybeUnblindedBlock.valueOr:
+      warn "Blinded block proposal incomplete",
+        head = shortLog(head), slot, validator_index,
+        validator = shortLog(validator),
+        err = maybeUnblindedBlock.error,
+        blindedBlck = shortLog(blindedBlock.get)
       beacon_block_builder_missed_without_fallback.inc()
-        return newBlockMEV.get
+      return head
 
-  # TODO Compare the value of the MEV block and the execution block
+  var forkedBlck = engineBlockFut.read().get
-  #      obtained from the EL below:
-
-  let newBlock =
-    if slot.epoch >= node.dag.cfg.DENEB_FORK_EPOCH:
-      await makeBeaconBlockForHeadAndSlot(
-        deneb.ExecutionPayloadForSigning,
-        node, randao, validator_index, node.graffitiBytes, head, slot)
-    elif slot.epoch >= node.dag.cfg.CAPELLA_FORK_EPOCH:
-      await makeBeaconBlockForHeadAndSlot(
-        capella.ExecutionPayloadForSigning,
-        node, randao, validator_index, node.graffitiBytes, head, slot)
-    else:
-      await makeBeaconBlockForHeadAndSlot(
-        bellatrix.ExecutionPayloadForSigning,
-        node, randao, validator_index, node.graffitiBytes, head, slot)
-
-  if newBlock.isErr():
-    return head # already logged elsewhere!
-
-  var forkedBlck = newBlock.get()
 
   withBlck(forkedBlck):
     var blobs_sidecar = deneb.BlobsSidecar(
@@ -974,6 +984,49 @@ proc proposeBlock(node: BeaconNode,
 
     return newBlockRef.get()
 
+proc proposeBlock(node: BeaconNode,
+                  validator: AttachedValidator,
+                  validator_index: ValidatorIndex,
+                  head: BlockRef,
+                  slot: Slot): Future[BlockRef] {.async.} =
+  if head.slot >= slot:
+    # We should normally not have a head newer than the slot we're proposing for
+    # but this can happen if block proposal is delayed
+    warn "Skipping proposal, have newer head already",
+      headSlot = shortLog(head.slot),
+      headBlockRoot = shortLog(head.root),
+      slot = shortLog(slot)
+    return head
+
+  let
+    fork = node.dag.forkAtEpoch(slot.epoch)
+    genesis_validators_root = node.dag.genesis_validators_root
+    randao = block:
+      let res = await validator.getEpochSignature(
+        fork, genesis_validators_root, slot.epoch)
+      if res.isErr():
+        warn "Unable to generate randao reveal",
+             validator = shortLog(validator), error_msg = res.error()
+        return head
+      res.get()
+
+  template proposeBlockContinuation(type1, type2: untyped): auto =
+    await proposeBlockAux(
+      type1, type2, node, validator, validator_index, head, slot, randao, fork,
+        genesis_validators_root)
+
+  return
+    if slot.epoch >= node.dag.cfg.DENEB_FORK_EPOCH:
+      debugRaiseAssert $denebImplementationMissing & ": proposeBlock"
+      proposeBlockContinuation(
+        capella_mev.SignedBlindedBeaconBlock, deneb.ExecutionPayloadForSigning)
+    elif slot.epoch >= node.dag.cfg.CAPELLA_FORK_EPOCH:
+      proposeBlockContinuation(
+        capella_mev.SignedBlindedBeaconBlock, capella.ExecutionPayloadForSigning)
+    else:
+      proposeBlockContinuation(
+        bellatrix_mev.SignedBlindedBeaconBlock, bellatrix.ExecutionPayloadForSigning)
+
 proc handleAttestations(node: BeaconNode, head: BlockRef, slot: Slot) =
   ## Perform all attestations that the validators attached to this node should
   ## perform during the given slot