return block result details (#1049)

beacon_chain/beacon_node.nim

@@ -290,7 +290,8 @@ proc onAttestation(node: BeaconNode, attestation: Attestation) =
 
   node.attestationPool.add(attestation)
 
-proc storeBlock(node: BeaconNode, signedBlock: SignedBeaconBlock): bool =
+proc storeBlock(
+    node: BeaconNode, signedBlock: SignedBeaconBlock): Result[void, BlockError] =
   let blockRoot = hash_tree_root(signedBlock.message)
   debug "Block received",
     signedBlock = shortLog(signedBlock.message),
@@ -302,8 +303,7 @@ proc storeBlock(node: BeaconNode, signedBlock: SignedBeaconBlock): bool =
     dump(node.config.dumpDir / "incoming", signedBlock, blockRoot)
 
   beacon_blocks_received.inc()
-  if node.blockPool.add(blockRoot, signedBlock).isNil:
+  discard ? node.blockPool.add(blockRoot, signedBlock)
-    return false
 
   # The block we received contains attestations, and we might not yet know about
   # all of them. Let's add them to the attestation pool - in case they block
@@ -315,7 +315,7 @@ proc storeBlock(node: BeaconNode, signedBlock: SignedBeaconBlock): bool =
     signedBlock.message.slot.epoch + 1 >= currentSlot.slot.epoch:
     for attestation in signedBlock.message.body.attestations:
       node.onAttestation(attestation)
-  return true
+  ok()
 
 proc onBeaconBlock(node: BeaconNode, signedBlock: SignedBeaconBlock) =
   # We received a block but don't know much about it yet - in particular, we
@@ -525,7 +525,7 @@ proc runSyncLoop(node: BeaconNode) {.async.} =
           local_head_slot = getLocalHeadSlot()
     let sm = now(chronos.Moment)
     for blk in list:
-      if not(node.storeBlock(blk)):
+      if node.storeBlock(blk).isErr:
         return false
     discard node.updateHead()
 

beacon_chain/block_pool.nim

@@ -7,12 +7,15 @@
 
 import
   extras, beacon_chain_db,
+  stew/results,
   spec/[crypto, datatypes, digest]
 
 
 import
   block_pools/[block_pools_types, clearance, candidate_chains, quarantine]
 
+export results
+
 # Block_Pools
 # --------------------------------------------
 #
@@ -51,7 +54,7 @@ template finalizedHead*(pool: BlockPool): BlockSlot =
   pool.dag.finalizedHead
 
 proc add*(pool: var BlockPool, blockRoot: Eth2Digest,
-          signedBlock: SignedBeaconBlock): BlockRef {.gcsafe.} =
+          signedBlock: SignedBeaconBlock): Result[BlockRef, BlockError] {.gcsafe.} =
   add(pool.dag, pool.quarantine, blockRoot, signedBlock)
 
 export parent        # func parent*(bs: BlockSlot): BlockSlot

beacon_chain/block_pools/block_pools_types.nim

@@ -23,6 +23,11 @@ import
 # the last finalized block known.
 
 type
+  BlockError* = enum
+    MissingParent
+    Old
+    Invalid
+
   Quarantine* = object
     ## Keeps track of unsafe blocks coming from the network
     ## and that cannot be added to the chain

beacon_chain/block_pools/clearance.nim

@@ -7,12 +7,14 @@
 
 import
   chronicles, tables,
-  metrics,
+  metrics, stew/results,
   ../ssz, ../state_transition, ../extras,
   ../spec/[crypto, datatypes, digest, helpers],
 
   block_pools_types, candidate_chains
 
+export results
+
 # Clearance
 # ---------------------------------------------
 #
@@ -34,7 +36,7 @@ func getOrResolve*(dag: CandidateChains, quarantine: var Quarantine, root: Eth2D
 proc add*(
     dag: var CandidateChains, quarantine: var Quarantine,
     blockRoot: Eth2Digest,
-    signedBlock: SignedBeaconBlock): BlockRef {.gcsafe.}
+    signedBlock: SignedBeaconBlock): Result[BlockRef, BlockError] {.gcsafe.}
 
 proc addResolvedBlock(
     dag: var CandidateChains, quarantine: var Quarantine,
@@ -105,7 +107,7 @@ proc addResolvedBlock(
 proc add*(
     dag: var CandidateChains, quarantine: var Quarantine,
     blockRoot: Eth2Digest,
-    signedBlock: SignedBeaconBlock): BlockRef {.gcsafe.} =
+    signedBlock: SignedBeaconBlock): Result[BlockRef, BlockError] {.gcsafe.} =
   ## return the block, if resolved...
   ## the state parameter may be updated to include the given block, if
   ## everything checks out
@@ -122,7 +124,7 @@ proc add*(
       blockRoot = shortLog(blockRoot),
       cat = "filtering"
 
-    return blockRef[]
+    return ok blockRef[]
 
   quarantine.missing.del(blockRoot)
 
@@ -138,7 +140,7 @@ proc add*(
       blockRoot = shortLog(blockRoot),
       cat = "filtering"
 
-    return
+    return err Old
 
   let parent = dag.blocks.getOrDefault(blck.parent_root)
 
@@ -150,7 +152,7 @@ proc add*(
         blockRoot = shortLog(blockRoot),
         parentBlock = shortLog(parent)
 
-      return
+      return err Invalid
 
     # The block might have been in either of pending or missing - we don't want
     # any more work done on its behalf
@@ -181,7 +183,7 @@ proc add*(
         blockRoot = shortLog(blockRoot),
         cat = "filtering"
 
-      return
+      return err Invalid
     # Careful, tmpState.data has been updated but not blck - we need to create
     # the BlockRef first!
     dag.tmpState.blck = addResolvedBlock(
@@ -189,7 +191,7 @@ proc add*(
       dag.tmpState.data.data, blockRoot, signedBlock, parent)
     dag.putState(dag.tmpState.data, dag.tmpState.blck)
 
-    return dag.tmpState.blck
+    return ok dag.tmpState.blck
 
   # TODO already checked hash though? main reason to keep this is because
   # the pending dag calls this function back later in a loop, so as long
@@ -204,7 +206,7 @@ proc add*(
 
   if blck.parent_root in quarantine.missing or
       blck.parent_root in quarantine.pending:
-    return
+    return err MissingParent
 
   # This is an unresolved block - put its parent on the missing list for now...
   # TODO if we receive spam blocks, one heurestic to implement might be to wait
@@ -239,6 +241,8 @@ proc add*(
     missing = quarantine.missing.len,
     cat = "filtering"
 
+  return err MissingParent
+
 # https://github.com/ethereum/eth2.0-specs/blob/v0.11.1/specs/phase0/p2p-interface.md#global-topics
 proc isValidBeaconBlock*(
        dag: CandidateChains, quarantine: var Quarantine,

beacon_chain/validator_duties.nim

@@ -189,30 +189,31 @@ proc proposeBlock(node: BeaconNode,
     (blockRoot, newBlock)
 
   let newBlockRef = node.blockPool.add(nroot, nblck)
-  if newBlockRef == nil:
+  if newBlockRef.isErr:
     warn "Unable to add proposed block to block pool",
       newBlock = shortLog(newBlock.message),
       blockRoot = shortLog(blockRoot),
       cat = "bug"
+
     return head
 
   info "Block proposed",
     blck = shortLog(newBlock.message),
-    blockRoot = shortLog(newBlockRef.root),
+    blockRoot = shortLog(newBlockRef[].root),
     validator = shortLog(validator),
     cat = "consensus"
 
   if node.config.dumpEnabled:
-    dump(node.config.dumpDir, newBlock, newBlockRef)
+    dump(node.config.dumpDir, newBlock, newBlockRef[])
     node.blockPool.withState(
-        node.blockPool.tmpState, newBlockRef.atSlot(newBlockRef.slot)):
+        node.blockPool.tmpState, newBlockRef[].atSlot(newBlockRef[].slot)):
-      dump(node.config.dumpDir, hashedState, newBlockRef)
+      dump(node.config.dumpDir, hashedState, newBlockRef[])
 
   node.network.broadcast(node.topicBeaconBlocks, newBlock)
 
   beacon_blocks_proposed.inc()
 
-  return newBlockRef
+  return newBlockRef[]
 
 
 proc handleAttestations(node: BeaconNode, head: BlockRef, slot: Slot) =

research/block_sim.nim

@@ -130,7 +130,7 @@ cli do(slots = SLOTS_PER_EPOCH * 6,
           state.fork, state.genesis_validators_root, newBlock.message.slot,
           blockRoot, privKey)
 
-      let added = blockPool.add(blockRoot, newBlock)
+      let added = blockPool.add(blockRoot, newBlock).tryGet()
       blockPool.updateHead(added)
 
   for i in 0..<slots:

tests/test_attestation_pool.nim

@@ -152,7 +152,7 @@ suiteReport "Attestation pool processing" & preset():
     let
       b1 = addTestBlock(state.data.data, blockPool.tail.root)
       b1Root = hash_tree_root(b1.message)
-      b1Add = blockPool.add(b1Root, b1)
+      b1Add = blockPool.add(b1Root, b1)[]
       head = pool.selectHead()
 
     check:
@@ -161,7 +161,7 @@ suiteReport "Attestation pool processing" & preset():
     let
       b2 = addTestBlock(state.data.data, b1Root)
       b2Root = hash_tree_root(b2.message)
-      b2Add = blockPool.add(b2Root, b2)
+      b2Add = blockPool.add(b2Root, b2)[]
       head2 = pool.selectHead()
 
     check:
@@ -172,7 +172,7 @@ suiteReport "Attestation pool processing" & preset():
     let
       b10 = makeTestBlock(state.data.data, blockPool.tail.root)
       b10Root = hash_tree_root(b10.message)
-      b10Add = blockPool.add(b10Root, b10)
+      b10Add = blockPool.add(b10Root, b10)[]
       head = pool.selectHead()
 
     check:
@@ -183,7 +183,7 @@ suiteReport "Attestation pool processing" & preset():
         graffiti = Eth2Digest(data: [1'u8, 0, 0, 0 ,0 ,0 ,0 ,0 ,0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
       )
       b11Root = hash_tree_root(b11.message)
-      b11Add = blockPool.add(b11Root, b11)
+      b11Add = blockPool.add(b11Root, b11)[]
 
       bc1 = get_beacon_committee(
         state.data.data, state.data.data.slot, 1.CommitteeIndex, cache)

tests/test_block_pool.nim

@@ -110,7 +110,7 @@ suiteReport "Block pool processing" & preset():
 
   timedTest "Simple block add&get" & preset():
     let
-      b1Add = pool.add(b1Root, b1)
+      b1Add = pool.add(b1Root, b1)[]
       b1Get = pool.get(b1Root)
 
     check:
@@ -121,7 +121,7 @@ suiteReport "Block pool processing" & preset():
       pool.heads[0].blck == b1Add
 
     let
-      b2Add = pool.add(b2Root, b2)
+      b2Add = pool.add(b2Root, b2)[]
       b2Get = pool.get(b2Root)
 
     check:
@@ -138,7 +138,7 @@ suiteReport "Block pool processing" & preset():
     let
       b4 = addTestBlock(stateData.data.data, b2Root)
       b4Root = hash_tree_root(b4.message)
-      b4Add = pool.add(b4Root, b4)
+      b4Add = pool.add(b4Root, b4)[]
     # TODO fix addTestBlock to work on HashedBeaconState
     stateData.data.root = hash_tree_root(stateData.data.data)
 
@@ -176,13 +176,13 @@ suiteReport "Block pool processing" & preset():
       blocks[0..<2] == [BlockRef nil, nil] # block 3 is missing!
 
   timedTest "Reverse order block add & get" & preset():
-    discard pool.add(b2Root, b2)
+    check: pool.add(b2Root, b2).error == MissingParent
 
     check:
       pool.get(b2Root).isNone() # Unresolved, shouldn't show up
       FetchRecord(root: b1Root, historySlots: 1) in pool.checkMissing()
 
-    discard pool.add(b1Root, b1)
+    check: pool.add(b1Root, b1).isOk
 
     let
       b1Get = pool.get(b1Root)
@@ -217,8 +217,8 @@ suiteReport "Block pool processing" & preset():
 
   timedTest "Can add same block twice" & preset():
     let
-      b10 = pool.add(b1Root, b1)
+      b10 = pool.add(b1Root, b1)[]
-      b11 = pool.add(b1Root, b1)
+      b11 = pool.add(b1Root, b1)[]
 
     check:
       b10 == b11
@@ -226,7 +226,7 @@ suiteReport "Block pool processing" & preset():
 
   timedTest "updateHead updates head and headState" & preset():
     let
-      b1Add = pool.add(b1Root, b1)
+      b1Add = pool.add(b1Root, b1)[]
 
     pool.updateHead(b1Add)
 
@@ -236,8 +236,8 @@ suiteReport "Block pool processing" & preset():
 
   timedTest "updateStateData sanity" & preset():
     let
-      b1Add = pool.add(b1Root, b1)
+      b1Add = pool.add(b1Root, b1)[]
-      b2Add = pool.add(b2Root, b2)
+      b2Add = pool.add(b2Root, b2)[]
       bs1 = BlockSlot(blck: b1Add, slot: b1.message.slot)
       bs1_3 = b1Add.atSlot(3.Slot)
       bs2_3 = b2Add.atSlot(3.Slot)
@@ -296,7 +296,7 @@ when const_preset == "minimal":  # These require some minutes in mainnet
         # Create a fork that will not be taken
         var
           blck = makeTestBlock(pool.headState.data.data, pool.head.blck.root)
-        discard pool.add(hash_tree_root(blck.message), blck)
+        check: pool.add(hash_tree_root(blck.message), blck).isOk
 
       for i in 0 ..< (SLOTS_PER_EPOCH * 6):
         if i == 1:
@@ -311,7 +311,7 @@ when const_preset == "minimal":  # These require some minutes in mainnet
             attestations = makeFullAttestations(
               pool.headState.data.data, pool.head.blck.root,
               pool.headState.data.data.slot, cache, {}))
-        let added = pool.add(hash_tree_root(blck.message), blck)
+        let added = pool.add(hash_tree_root(blck.message), blck)[]
         pool.updateHead(added)
 
       check:
@@ -342,7 +342,7 @@ when const_preset == "minimal":  # These require some minutes in mainnet
             attestations = makeFullAttestations(
               pool.headState.data.data, pool.head.blck.root,
               pool.headState.data.data.slot, cache, {}))
-        let added = pool.add(hash_tree_root(blck.message), blck)
+        let added = pool.add(hash_tree_root(blck.message), blck)[]
         pool.updateHead(added)
 
       # Advance past epoch so that the epoch transition is gapped
@@ -356,7 +356,7 @@ when const_preset == "minimal":  # These require some minutes in mainnet
           pool.headState.data.data, pool.head.blck.root,
           pool.headState.data.data.slot, cache, {}))
 
-      let added = pool.add(hash_tree_root(blck.message), blck)
+      let added = pool.add(hash_tree_root(blck.message), blck)[]
       pool.updateHead(added)
 
       let