Broadcast blocks before integrating in ChainDAG

This PR fixes two issues with block publishing:

* Gossip-valid blocks are published before integrating them into the
chain, giving broadcasting a head start, both for rest block and
* Outright invalid blocks from the API that could lead to the descoring
of the node are no longer broadcast

Bonus:

* remove undocumented and duplicated `post_v1_validator_block` JSON-RPC
call

beacon_chain/rpc/rest_beacon_api.nim

@@ -799,6 +799,7 @@ proc installBeaconApiHandlers*(router: var RestRouter, node: BeaconNode) =
       return RestApiResponse.jsonError(Http503, BeaconNodeInSyncError)
     if not(res.get()):
       return RestApiResponse.jsonError(Http202, BlockValidationError)
+
     return RestApiResponse.jsonMsgResponse(BlockValidationSuccess)
 
   # https://ethereum.github.io/beacon-APIs/#/Beacon/getBlock

beacon_chain/rpc/rpc_beacon_api.nim

@@ -401,28 +401,18 @@ proc installBeaconApiHandlers*(rpcServer: RpcServer, node: BeaconNode) {.
       )
 
   rpcServer.rpc("post_v1_beacon_blocks") do (blck: phase0.SignedBeaconBlock) -> int:
-    if not(node.syncManager.inProgress):
+    let res = await sendBeaconBlock(node, ForkedSignedBeaconBlock.init(blck))
-      raise newException(CatchableError,
+    if res.isErr():
-                         "Beacon node is currently syncing, try again later.")
+      raise (ref CatchableError)(msg: $res.error())
-    let head = node.dag.head
+
-    if head.slot >= blck.message.slot:
+    if res.get():
-      node.network.broadcastBeaconBlock(blck)
+      # The block was validated successfully and has been broadcast.
-      # The block failed validation, but was successfully broadcast anyway.
+      # It has also been integrated into the beacon node's database.
-      # It was not integrated into the beacon node's database.
+      return 200
-      return 202
     else:
-      let res = await proposeSignedBlock(
+      # The block failed validation, but was successfully broadcast anyway.
-        node, head, AttachedValidator(),
+      # It was not integrated into the beacon node''s database.
-        ForkedSignedBeaconBlock.init(blck))
+      return 202
-      if res == head:
-        node.network.broadcastBeaconBlock(blck)
-        # The block failed validation, but was successfully broadcast anyway.
-        # It was not integrated into the beacon node''s database.
-        return 202
-      else:
-        # The block was validated successfully and has been broadcast.
-        # It has also been integrated into the beacon node's database.
-        return 200
 
   rpcServer.rpc("get_v1_beacon_blocks_blockId") do (
       blockId: string) -> phase0.TrustedSignedBeaconBlock:

beacon_chain/rpc/rpc_validator_api.nim

@@ -53,20 +53,6 @@ proc installValidatorApiHandlers*(rpcServer: RpcServer, node: BeaconNode) {.
     else:
       raiseNoAltairSupport()
 
-  rpcServer.rpc("post_v1_validator_block") do (body: phase0.SignedBeaconBlock) -> bool:
-    debug "post_v1_validator_block",
-      slot = body.message.slot,
-      prop_idx = body.message.proposer_index
-    let head = node.doChecksAndGetCurrentHead(body.message.slot)
-
-    if head.slot >= body.message.slot:
-      raise newException(CatchableError,
-        "Proposal is for a past slot: " & $body.message.slot)
-    if head == await proposeSignedBlock(
-        node, head, AttachedValidator(), ForkedSignedBeaconBlock.init(body)):
-      raise newException(CatchableError, "Could not propose block")
-    return true
-
   rpcServer.rpc("get_v1_validator_attestation_data") do (
       slot: Slot, committee_index: CommitteeIndex) -> AttestationData:
     debug "get_v1_validator_attestation_data", slot = slot

beacon_chain/spec/eth2_apis/rpc_validator_calls.nim

@@ -7,8 +7,6 @@ import
 
 proc get_v1_validator_block(slot: Slot, graffiti: GraffitiBytes, randao_reveal: ValidatorSig): phase0.BeaconBlock
 
-proc post_v1_validator_block(body: phase0.SignedBeaconBlock): bool
-
 proc get_v1_validator_attestation_data(slot: Slot, committee_index: CommitteeIndex): AttestationData
 
 proc get_v1_validator_aggregate_attestation(slot: Slot, attestation_data_root: Eth2Digest): Attestation

beacon_chain/validators/validator_duties.nim

@@ -462,32 +462,6 @@ proc makeBeaconBlockForHeadAndSlot*(node: BeaconNode,
       noRollback, # Temporary state - no need for rollback
       cache)
 
-proc proposeSignedBlock*(node: BeaconNode,
-                         head: BlockRef,
-                         validator: AttachedValidator,
-                         newBlock: ForkedSignedBeaconBlock):
-                         Future[BlockRef] {.async.} =
-  let wallTime = node.beaconClock.now()
-
-  return withBlck(newBlock):
-    let newBlockRef = node.blockProcessor[].storeBlock(
-      blck, wallTime.slotOrZero())
-
-    if newBlockRef.isErr:
-      warn "Unable to add proposed block to block pool",
-            newBlock = blck.message, root = blck.root
-      return head
-
-    notice "Block proposed",
-           blockRoot = shortLog(blck.root), blck = shortLog(blck.message),
-           signature = shortLog(blck.signature), validator = shortLog(validator)
-
-    node.network.broadcastBeaconBlock(blck)
-
-    beacon_blocks_proposed.inc()
-
-    newBlockRef.get()
-
 proc proposeBlock(node: BeaconNode,
                   validator: AttachedValidator,
                   validator_index: ValidatorIndex,
@@ -522,20 +496,17 @@ proc proposeBlock(node: BeaconNode,
   if newBlock.isErr():
     return head # already logged elsewhere!
 
-  let blck = newBlock.get()
+  let forkedBlck = newBlock.get()
 
-  # TODO abstract this, or move it into makeBeaconBlockForHeadAndSlot, and in
+  withBlck(forkedBlck):
-  # general this is far too much copy/paste
+    let
-  let forked = case blck.kind:
+      blockRoot = hash_tree_root(blck)
-  of BeaconBlockFork.Phase0:
+      signing_root = compute_block_root(
-    let root = hash_tree_root(blck.phase0Data)
+        fork, genesis_validators_root, slot, blockRoot)
 
-    # TODO: recomputed in block proposal
+      notSlashable = node.attachedValidators
-    let signing_root = compute_block_root(
+        .slashingProtection
-      fork, genesis_validators_root, slot, root)
+        .registerBlock(validator_index, validator.pubkey, slot, signing_root)
-    let notSlashable = node.attachedValidators
-      .slashingProtection
-      .registerBlock(validator_index, validator.pubkey, slot, signing_root)
 
     if notSlashable.isErr:
       warn "Slashing protection activated",
@@ -544,83 +515,58 @@ proc proposeBlock(node: BeaconNode,
         existingProposal = notSlashable.error
       return head
 
-    let signature =
+    let
-      block:
+      signature =
-        let res = await validator.signBlockProposal(fork,
+        block:
-          genesis_validators_root, slot, root, blck)
+          let res = await validator.signBlockProposal(
-        if res.isErr():
+            fork, genesis_validators_root, slot, blockRoot, forkedBlck)
-          error "Unable to sign block proposal",
+          if res.isErr():
-                validator = shortLog(validator), error_msg = res.error()
+            error "Unable to sign block proposal",
-          return head
+                  validator = shortLog(validator), error_msg = res.error()
-        res.get()
+            return head
-    ForkedSignedBeaconBlock.init(
+          res.get()
-      phase0.SignedBeaconBlock(
+      signedBlock =
-        message: blck.phase0Data, root: root, signature: signature)
+        when blck is phase0.BeaconBlock:
-    )
+          phase0.SignedBeaconBlock(
-  of BeaconBlockFork.Altair:
+            message: blck, signature: signature, root: blockRoot)
-    let root = hash_tree_root(blck.altairData)
+        elif blck is altair.BeaconBlock:
+          altair.SignedBeaconBlock(
+            message: blck, signature: signature, root: blockRoot)
+        elif blck is merge.BeaconBlock:
+          merge.SignedBeaconBlock(
+            message: blck, signature: signature, root: blockRoot)
+        else:
+          static: doAssert "Unkown block type"
 
-    # TODO: recomputed in block proposal
+    # We produced the block using a state transition, meaning the block is valid
-    let signing_root = compute_block_root(
+    # enough that it will not be rejected by gossip - it is unlikely but
-      fork, genesis_validators_root, slot, root)
+    # possible that it will be ignored due to extreme timing conditions, for
-    let notSlashable = node.attachedValidators
+    # example a delay in signing.
-      .slashingProtection
+    # We'll start broadcasting it before integrating fully in the chaindag
-      .registerBlock(validator_index, validator.pubkey, slot, signing_root)
+    # so that it can start propagating through the network ASAP.
+    node.network.broadcastBeaconBlock(signedBlock)
 
-    if notSlashable.isErr:
+    let
-      warn "Slashing protection activated",
+      wallTime = node.beaconClock.now()
-        validator = validator.pubkey,
+
-        slot = slot,
+      # storeBlock puts the block in the chaindag, and if accepted, takes care
-        existingProposal = notSlashable.error
+      # of side effects such as event api notification
+      newBlockRef = node.blockProcessor[].storeBlock(
+        signedBlock, wallTime.slotOrZero())
+
+    if newBlockRef.isErr:
+      warn "Unable to add proposed block to block pool",
+        blockRoot = shortLog(blockRoot), blck = shortLog(blck),
+        signature = shortLog(signature), validator = shortLog(validator)
       return head
 
-    let signature =
+    notice "Block proposed",
-      block:
+      blockRoot = shortLog(blockRoot), blck = shortLog(blck),
-        let res = await validator.signBlockProposal(
+      signature = shortLog(signature), validator = shortLog(validator)
-          fork, genesis_validators_root, slot, root, blck)
-        if res.isErr():
-          error "Unable to sign block proposal",
-                validator = shortLog(validator), error_msg = res.error()
-          return head
-        res.get()
 
-    ForkedSignedBeaconBlock.init(
+    beacon_blocks_proposed.inc()
-      altair.SignedBeaconBlock(
-        message: blck.altairData, root: root, signature: signature)
-    )
-  of BeaconBlockFork.Merge:
-    let root = hash_tree_root(blck.mergeData)
 
-    # TODO: recomputed in block proposal
+    return newBlockRef.get()
-    let signing_root = compute_block_root(
-      fork, genesis_validators_root, slot, root)
-    let notSlashable = node.attachedValidators
-      .slashingProtection
-      .registerBlock(validator_index, validator.pubkey, slot, signing_root)
-
-    if notSlashable.isErr:
-      warn "Slashing protection activated",
-        validator = validator.pubkey,
-        slot = slot,
-        existingProposal = notSlashable.error
-      return head
-
-    let signature =
-      block:
-        let res = await validator.signBlockProposal(
-          fork, genesis_validators_root, slot, root, blck)
-        if res.isErr():
-          error "Unable to sign block proposal",
-                validator = shortLog(validator), error_msg = res.error()
-          return head
-        res.get()
-
-    ForkedSignedBeaconBlock.init(
-      merge.SignedBeaconBlock(
-        message: blck.mergeData, root: root, signature: signature)
-    )
-
-  return await node.proposeSignedBlock(head, validator, forked)
 
 proc handleAttestations(node: BeaconNode, head: BlockRef, slot: Slot) =
   ## Perform all attestations that the validators attached to this node should
@@ -874,16 +820,17 @@ proc handleProposal(node: BeaconNode, head: BlockRef, slot: Slot):
     proposerKey = node.dag.validatorKey(proposer.get).get().toPubKey
     validator = node.attachedValidators[].getValidator(proposerKey)
 
-  if validator != nil:
+  return
-    return await proposeBlock(node, validator, proposer.get(), head, slot)
+    if validator == nil:
+      debug "Expecting block proposal",
+        headRoot = shortLog(head.root),
+        slot = shortLog(slot),
+        proposer_index = proposer.get(),
+        proposer = shortLog(proposerKey)
 
-  debug "Expecting block proposal",
+      head
-    headRoot = shortLog(head.root),
+    else:
-    slot = shortLog(slot),
+      await proposeBlock(node, validator, proposer.get(), head, slot)
-    proposer_index = proposer.get(),
-    proposer = shortLog(proposerKey)
-
-  return head
 
 proc makeAggregateAndProof*(
     pool: var AttestationPool, epochRef: EpochRef, slot: Slot, index: CommitteeIndex,
@@ -1246,20 +1193,47 @@ proc sendProposerSlashing*(node: BeaconNode,
 proc sendBeaconBlock*(node: BeaconNode, forked: ForkedSignedBeaconBlock
                      ): Future[SendBlockResult] {.async.} =
   # REST/JSON-RPC API helper procedure.
-  let head = node.dag.head
+  block:
-  if not(node.isSynced(head)):
+    # Start with a quick gossip validation check such that broadcasting the
-    return SendBlockResult.err("Beacon node is currently syncing")
+    # block doesn't get the node into trouble
-  if head.slot >= forked.slot():
+    let res = withBlck(forked):
-    node.network.broadcastBeaconBlock(forked)
+      when blck isnot merge.SignedBeaconBlock:
-    return SendBlockResult.ok(false)
+        validateBeaconBlock(
+          node.dag, node.quarantine, blck, node.beaconClock.now(),
+          {})
+      else:
+       return SendBlockResult.err(
+        "TODO merge block proposal via REST not implemented")
 
-  let res = await node.proposeSignedBlock(head, AttachedValidator(), forked)
+    if not res.isGoodForSending():
-  if res == head:
+      return SendBlockResult.err(res.error()[1])
-    # `res == head` means failure, in such case we need to broadcast block
+
-    # manually because of the specification.
+  # The block passed basic gossip validation - we can "safely" broadcast it now.
-    node.network.broadcastBeaconBlock(forked)
+  # In fact, per the spec, we should broadcast it even if it later fails to
-    return SendBlockResult.ok(false)
+  # apply to our state.
-  return SendBlockResult.ok(true)
+  node.network.broadcastBeaconBlock(forked)
+
+  let
+    head = node.dag.head
+    wallTime = node.beaconClock.now()
+    accepted = withBlck(forked):
+      let newBlockRef = node.blockProcessor[].storeBlock(
+        blck, wallTime.slotOrZero())
+
+      # The boolean we return tells the caller whether the block was integrated
+      # into the chain
+      if newBlockRef.isOk():
+        notice "Block published",
+          blockRoot = shortLog(blck.root), blck = shortLog(blck.message),
+          signature = shortLog(blck.signature)
+        true
+      else:
+        warn "Unable to add proposed block to block pool",
+          blockRoot = shortLog(blck.root), blck = shortLog(blck.message),
+          signature = shortLog(blck.signature), err = newBlockRef.error()
+
+        false
+  return SendBlockResult.ok(accepted)
 
 proc registerDuty*(
     node: BeaconNode, slot: Slot, subnet_id: SubnetId, vidx: ValidatorIndex,