Fork choice - almost free pruning - fix #1534 (#1535)

* initial - cheaper pruning - addresses  #1534

* Pass tests: update offset when pruning, proper handling of pruned parents

* Use options instead of nil for nilable newHead (finalization passing but rootcause not solved)

* First line of defense against stackoverflow in tests

* Fix compute_delta offset after pruning

* Rebase fix - medalla ready

* Remove Option[BlockRef]

beacon_chain/attestation_pool.nim

@@ -380,6 +380,8 @@ proc resolve*(pool: var AttestationPool, wallSlot: Slot) =
     pool.addResolved(a.blck, a.attestation, wallSlot)
 
 proc selectHead*(pool: var AttestationPool, wallSlot: Slot): BlockRef =
+  ## Trigger fork choice and returns the new head block.
+  ## Can return `nil`
   let newHead = pool.forkChoice.get_head(pool.chainDag, wallSlot)
 
   if newHead.isErr:

beacon_chain/beacon_node_common.nim

@@ -62,6 +62,8 @@ const
 
 # Metrics
 proc updateHead*(node: BeaconNode, wallSlot: Slot): BlockRef =
+  ## Trigger fork choice and returns the new head block.
+  ## Can return `nil`
   node.processor[].updateHead(wallSlot)
 
 template findIt*(s: openarray, predicate: untyped): int =

beacon_chain/block_pools/chain_dag.nim

@@ -695,7 +695,8 @@ proc updateHead*(dag: ChainDAGRef, newHead: BlockRef) =
   ## of operations naturally becomes important here - after updating the head,
   ## blocks that were once considered potential candidates for a tree will
   ## now fall from grace, or no longer be considered resolved.
-  doAssert newHead.parent != nil or newHead.slot == 0
+  doAssert not newHead.isNil()
+  doAssert not newHead.parent.isNil() or newHead.slot == 0
   logScope:
     newHead = shortLog(newHead)
     pcs = "fork_choice"

beacon_chain/eth2_processor.nim

@@ -59,11 +59,15 @@ type
     aggregatesQueue*: AsyncQueue[AggregateEntry]
 
 proc updateHead*(self: var Eth2Processor, wallSlot: Slot): BlockRef =
+  ## Trigger fork choice and returns the new head block.
+  ## Can return `nil`
   # Check pending attestations - maybe we found some blocks for them
   self.attestationPool[].resolve(wallSlot)
 
   # Grab the new head according to our latest attestation data
   let newHead = self.attestationPool[].selectHead(wallSlot)
+  if newHead.isNil():
+    return nil
 
   # Store the new head in the chain DAG - this may cause epochs to be
   # justified and finalized

beacon_chain/fork_choice/fork_choice.nim

@@ -19,6 +19,7 @@ import
   ../block_pools/[spec_cache, chain_dag]
 
 export sets, results, fork_choice_types
+export proto_array.len
 
 # https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/fork-choice.md
 # This is a port of https://github.com/sigp/lighthouse/pull/804
@@ -34,6 +35,7 @@ export sets, results, fork_choice_types
 func compute_deltas(
        deltas: var openarray[Delta],
        indices: Table[Eth2Digest, Index],
+       indices_offset: Index,
        votes: var openArray[VoteTracker],
        old_balances: openarray[Gwei],
        new_balances: openarray[Gwei]
@@ -324,6 +326,7 @@ proc find_head*(
   var deltas = newSeq[Delta](self.proto_array.indices.len)
   ? deltas.compute_deltas(
     indices = self.proto_array.indices,
+    indices_offset = self.proto_array.nodes.offset,
     votes = self.votes,
     old_balances = self.balances,
     new_balances = justified_state_balances
@@ -362,18 +365,19 @@ proc get_head*(self: var ForkChoice,
     self.checkpoints.justified.epochRef.effective_balances,
   )
 
-func maybe_prune*(
+func prune*(
        self: var ForkChoiceBackend, finalized_root: Eth2Digest
      ): FcResult[void] =
   ## Prune blocks preceding the finalized root as they are now unneeded.
-  self.proto_array.maybe_prune(finalized_root)
+  self.proto_array.prune(finalized_root)
 
 func prune*(self: var ForkChoice): FcResult[void] =
-  self.backend.maybe_prune(self.checkpoints.finalized.root)
+  self.backend.prune(self.checkpoints.finalized.root)
 
 func compute_deltas(
        deltas: var openarray[Delta],
        indices: Table[Eth2Digest, Index],
+       indices_offset: Index,
        votes: var openArray[VoteTracker],
        old_balances: openarray[Gwei],
        new_balances: openarray[Gwei]
@@ -414,7 +418,7 @@ func compute_deltas(
       # Ignore the current or next vote if it is not known in `indices`.
       # We assume that it is outside of our tree (i.e., pre-finalization) and therefore not interesting.
       if vote.current_root in indices:
-        let index = indices.unsafeGet(vote.current_root)
+        let index = indices.unsafeGet(vote.current_root) - indices_offset
         if index >= deltas.len:
           return err ForkChoiceError(
             kind: fcInvalidNodeDelta,
@@ -425,7 +429,7 @@ func compute_deltas(
           # TODO: is int64 big enough?
 
       if vote.next_root in indices:
-        let index = indices.unsafeGet(vote.next_root)
+        let index = indices.unsafeGet(vote.next_root) - indices_offset
         if index >= deltas.len:
           return err ForkChoiceError(
             kind: fcInvalidNodeDelta,
@@ -471,7 +475,7 @@ when isMainModule:
       new_balances.add 0
 
     let err = deltas.compute_deltas(
-      indices, votes, old_balances, new_balances
+      indices, indices_offset = 0, votes, old_balances, new_balances
     )
 
     doAssert err.isOk, "compute_deltas finished with error: " & $err
@@ -506,7 +510,7 @@ when isMainModule:
       new_balances.add Balance
 
     let err = deltas.compute_deltas(
-      indices, votes, old_balances, new_balances
+      indices, indices_offset = 0, votes, old_balances, new_balances
     )
 
     doAssert err.isOk, "compute_deltas finished with error: " & $err
@@ -545,7 +549,7 @@ when isMainModule:
       new_balances.add Balance
 
     let err = deltas.compute_deltas(
-      indices, votes, old_balances, new_balances
+      indices, indices_offset = 0, votes, old_balances, new_balances
     )
 
     doAssert err.isOk, "compute_deltas finished with error: " & $err
@@ -583,7 +587,7 @@ when isMainModule:
       new_balances.add Balance
 
     let err = deltas.compute_deltas(
-      indices, votes, old_balances, new_balances
+      indices, indices_offset = 0, votes, old_balances, new_balances
     )
 
     doAssert err.isOk, "compute_deltas finished with error: " & $err
@@ -631,7 +635,7 @@ when isMainModule:
     )
 
     let err = deltas.compute_deltas(
-      indices, votes, old_balances, new_balances
+      indices, indices_offset = 0, votes, old_balances, new_balances
     )
 
     doAssert err.isOk, "compute_deltas finished with error: " & $err
@@ -670,7 +674,7 @@ when isMainModule:
       new_balances.add NewBalance
 
     let err = deltas.compute_deltas(
-      indices, votes, old_balances, new_balances
+      indices, indices_offset = 0, votes, old_balances, new_balances
     )
 
     doAssert err.isOk, "compute_deltas finished with error: " & $err
@@ -714,7 +718,7 @@ when isMainModule:
 
 
     let err = deltas.compute_deltas(
-      indices, votes, old_balances, new_balances
+      indices, indices_offset = 0, votes, old_balances, new_balances
     )
 
     doAssert err.isOk, "compute_deltas finished with error: " & $err
@@ -753,7 +757,7 @@ when isMainModule:
 
 
     let err = deltas.compute_deltas(
-      indices, votes, old_balances, new_balances
+      indices, indices_offset = 0, votes, old_balances, new_balances
     )
 
     doAssert err.isOk, "compute_deltas finished with error: " & $err

beacon_chain/fork_choice/fork_choice_types.nim

@@ -51,6 +51,7 @@ type
     # -------------------------
     # TODO: Extra error modes beyond Proto/Lighthouse to be reviewed
     fcUnknownParent
+    fcPruningFromOutdatedFinalizedRoot
 
   AttErrorKind* = enum
     attFromFuture
@@ -106,14 +107,21 @@ type
     of fcUnknownParent:
       child_root*: Eth2Digest
       parent_root*: Eth2Digest
+    of fcPruningFromOutdatedFinalizedRoot:
+      finalizedRoot*: Eth2Digest
 
   FcResult*[T] = Result[T, ForkChoiceError]
 
+  ProtoNodes* = object
+    buf*: seq[ProtoNode]
+    offset*: int ##\
+    ## Substracted from logical Index
+    ## to get the physical index
+
   ProtoArray* = object
-    prune_threshold*: int
     justified_epoch*: Epoch
     finalized_epoch*: Epoch
-    nodes*: seq[ProtoNode]
+    nodes*: Protonodes
     indices*: Table[Eth2Digest, Index]
 
   ProtoNode* = object
@@ -169,3 +177,4 @@ func shortlog*(vote: VoteTracker): auto =
   )
 
 chronicles.formatIt VoteTracker: it.shortLog
+chronicles.formatIt ForkChoiceError: $it

beacon_chain/fork_choice/proto_array.nim

@@ -23,8 +23,6 @@ logScope:
 
 export results
 
-const DefaultPruneThreshold* = 256
-
 # https://github.com/ethereum/eth2.0-specs/blob/v0.11.1/specs/phase0/fork-choice.md
 # This is a port of https://github.com/sigp/lighthouse/pull/804
 # which is a port of "Proto-Array": https://github.com/protolambda/lmd-ghost
@@ -33,7 +31,7 @@ const DefaultPruneThreshold* = 256
 # - Prysmatic writeup: https://hackmd.io/bABJiht3Q9SyV3Ga4FT9lQ#High-level-concept
 # - Gasper Whitepaper: https://arxiv.org/abs/2003.03052
 
-# Helper
+# Helpers
 # ----------------------------------------------------------------------
 
 func tiebreak(a, b: Eth2Digest): bool =
@@ -57,6 +55,21 @@ template unsafeGet*[K, V](table: Table[K, V], key: K): V =
   except KeyError as exc:
     raiseAssert(exc.msg)
 
+func `[]`(nodes: ProtoNodes, idx: Index): Option[ProtoNode] {.inline.} =
+  ## Retrieve a ProtoNode at "Index"
+  if idx < nodes.offset:
+    return none(ProtoNode)
+  let i = idx - nodes.offset
+  if i >= nodes.buf.len:
+    return none(ProtoNode)
+  return some(nodes.buf[i])
+
+func len*(nodes: ProtoNodes): int {.inline.} =
+  nodes.buf.len
+
+func add(nodes: var ProtoNodes, node: ProtoNode) {.inline.} =
+  nodes.buf.add node
+
 # Forward declarations
 # ----------------------------------------------------------------------
 
@@ -71,8 +84,7 @@ func node_leads_to_viable_head(self: ProtoArray, node: ProtoNode): FcResult[bool
 func init*(T: type ProtoArray,
            justified_epoch: Epoch,
            finalized_root: Eth2Digest,
-           finalized_epoch: Epoch,
+           finalized_epoch: Epoch): T =
-           prune_threshold = DefaultPruneThreshold): T =
   let node = ProtoNode(
     root: finalized_root,
     parent: none(int),
@@ -84,10 +96,9 @@ func init*(T: type ProtoArray,
   )
 
   T(
-    prune_threshold: DefaultPruneThreshold,
     justified_epoch: justified_epoch,
     finalized_epoch: finalized_epoch,
-    nodes: @[node],
+    nodes: ProtoNodes(buf: @[node], offset: 0),
     indices: {node.root: 0}.toTable()
   )
 
@@ -110,6 +121,7 @@ func apply_score_changes*(
   ## 3. Compare the current node with the parent's best-child,
   ##    updating if the current node should become the best-child
   ## 4. If required, update the parent's best-descendant with the current node or its best-descendant
+  doAssert self.indices.len == self.nodes.len # By construction
   if deltas.len != self.indices.len:
     return err ForkChoiceError(
              kind: fcInvalidDeltaLen,
@@ -121,24 +133,15 @@ func apply_score_changes*(
   self.finalized_epoch = finalized_epoch
 
   # Iterate backwards through all the indices in `self.nodes`
-  for node_index in countdown(self.nodes.len - 1, 0):
+  for node_physical_index in countdown(self.nodes.len - 1, 0):
-    template node: untyped {.dirty.}= self.nodes[node_index]
+    template node: untyped {.dirty.}= self.nodes.buf[node_physical_index]
       ## Alias
       # This cannot raise the IndexError exception, how to tell compiler?
 
     if node.root == default(Eth2Digest):
       continue
 
-    if node_index notin {0..deltas.len-1}:
+    let node_delta = deltas[node_physical_index]
-      # TODO: Here `deltas.len == self.indices.len` from the previous check
-      #       and we can probably assume that
-      #       `self.indices.len == self.nodes.len` by construction
-      #       and avoid this check in a loop or altogether
-      return err ForkChoiceError(
-        kind: fcInvalidNodeDelta,
-        index: node_index
-      )
-    let node_delta = deltas[node_index]
 
     # Apply the delta to the node
     # We fail fast if underflow, which shouldn't happen.
@@ -147,26 +150,47 @@ func apply_score_changes*(
     if weight < 0:
       return err ForkChoiceError(
         kind: fcDeltaUnderflow,
-        index: node_index
+        index: node_physical_index
       )
     node.weight = weight
 
     # If the node has a parent, try to update its best-child and best-descendant
     if node.parent.isSome():
-      # TODO: Nim `options` module could use some {.inline.}
+      let parent_logical_index = node.parent.unsafeGet()
-      #       and a mutable overload for unsafeGet
+      let parent_physical_index = parent_logical_index - self.nodes.offset
-      #       and a "no exceptions" (only panics) implementation.
+      if parent_physical_index < 0:
-      let parent_index = node.parent.unsafeGet()
+        # Orphan, for example
-      if parent_index notin {0..deltas.len-1}:
+        #          0
+        #         / \
+        #        2   1
+        #            |
+        #            3
+        #            |
+        #            4
+        # -------pruned here ------
+        #          5   6
+        #          |
+        #          7
+        #          |
+        #          8
+        #         / \
+        #        9  10
+        #
+        # with 5 the canonical chain and 6 a discarded fork
+        # that will be pruned next.
+        break
+
+      if parent_physical_index >= deltas.len:
         return err ForkChoiceError(
           kind: fcInvalidParentDelta,
-          index: parent_index
+          index: parent_physical_index
         )
 
       # Back-propagate the nodes delta to its parent.
-      deltas[parent_index] += node_delta
+      deltas[parent_physical_index] += node_delta
 
-      ? self.maybe_update_best_child_and_descendant(parent_index, node_index)
+      let node_logical_index = node_physical_index + self.nodes.offset
+      ? self.maybe_update_best_child_and_descendant(parent_logical_index, node_logical_index)
 
   return ok()
 
@@ -199,7 +223,7 @@ func on_block*(
       parent_root: parent
     )
 
-  let node_index = self.nodes.len
+  let node_logical_index = self.nodes.offset + self.nodes.buf.len
 
   let node = ProtoNode(
     root: root,
@@ -211,10 +235,10 @@ func on_block*(
     best_descendant: none(int)
   )
 
-  self.indices[node.root] = node_index
+  self.indices[node.root] = node_logical_index
   self.nodes.add node
 
-  ? self.maybe_update_best_child_and_descendant(parent_index, node_index)
+  ? self.maybe_update_best_child_and_descendant(parent_index, node_logical_index)
 
   return ok()
 
@@ -239,52 +263,43 @@ func find_head*(
       block_root: justified_root
     )
 
-  if justified_index notin {0..self.nodes.len-1}:
+  let justified_node = self.nodes[justified_index]
+  if justified_node.isNone():
     return err ForkChoiceError(
       kind: fcInvalidJustifiedIndex,
       index: justified_index
     )
 
-  template justified_node: untyped = self.nodes[justified_index]
+  let best_descendant_index = justified_node.get().best_descendant.get(justified_index)
-    # Alias, IndexError are defects
+  let best_node = self.nodes[best_descendant_index]
-
+  if best_node.isNone():
-  let best_descendant_index = justified_node.best_descendant.get(justified_index)
-
-  if best_descendant_index notin {0..self.nodes.len-1}:
     return err ForkChoiceError(
       kind: fcInvalidBestDescendant,
       index: best_descendant_index
     )
-  template best_node: untyped = self.nodes[best_descendant_index]
-    # Alias, IndexError are defects
 
   # Perform a sanity check to ensure the node can be head
-  if not self.node_is_viable_for_head(best_node):
+  if not self.node_is_viable_for_head(best_node.get()):
     return err ForkChoiceError(
       kind: fcInvalidBestNode,
       start_root: justified_root,
       justified_epoch: self.justified_epoch,
       finalized_epoch: self.finalized_epoch,
-      head_root: justified_node.root,
+      head_root: justified_node.get().root,
-      head_justified_epoch: justified_node.justified_epoch,
+      head_justified_epoch: justified_node.get().justified_epoch,
-      head_finalized_epoch: justified_node.finalized_epoch
+      head_finalized_epoch: justified_node.get().finalized_epoch
     )
 
-  head = best_node.root
+  head = best_node.get().root
   return ok()
 
-# TODO: pruning can be made cheaper by keeping the new offset as a field
+func prune*(
-#       in proto_array instead of scanning the table to substract the offset.
-#       In that case pruning can always be done and does not need a threshold for efficiency.
-#       https://github.com/protolambda/eth2-py-hacks/blob/ae286567/proto_array.py
-func maybe_prune*(
        self: var ProtoArray,
        finalized_root: Eth2Digest
      ): FcResult[void] =
   ## Update the tree with new finalization information.
   ## The tree is pruned if and only if:
   ## - The `finalized_root` and finalized epoch are different from current
-  ## - The number of nodes in `self` is at least `self.prune_threshold`
   ##
   ## Returns error if:
   ## - The finalized epoch is less than the current one
@@ -300,69 +315,34 @@ func maybe_prune*(
       block_root: finalized_root
     )
 
-  if finalized_index < self.prune_threshold:
+  if finalized_index == self.nodes.offset:
-    # Pruning small numbers of nodes incurs more overhead than leaving them as is
+    # Nothing to do
     return ok()
 
-  # Remove the `self.indices` key/values for the nodes slated for deletion
+  if finalized_index < self.nodes.offset:
-  if finalized_index notin {0..self.nodes.len-1}:
     return err ForkChoiceError(
-      kind: fcInvalidNodeIndex,
+      kind: fcPruningFromOutdatedFinalizedRoot,
-      index: finalized_index
+      finalizedRoot: finalized_root
     )
 
   trace "Pruning blocks from fork choice",
     finalizedRoot = shortlog(finalized_root),
     pcs = "prune"
 
-  for node_index in 0 ..< finalized_index:
+  let final_phys_index = finalized_index-self.nodes.offset
-    self.indices.del(self.nodes[node_index].root)
+  for node_index in 0 ..< final_phys_index:
+    self.indices.del(self.nodes.buf[node_index].root)
 
   # Drop all nodes prior to finalization.
   # This is done in-place with `moveMem` to avoid costly reallocations.
   static: doAssert ProtoNode.supportsCopyMem(), "ProtoNode must be a trivial type"
-  let tail = self.nodes.len - finalized_index
+  let tail = self.nodes.len - final_phys_index
   # TODO: can we have an unallocated `self.nodes`? i.e. self.nodes[0] is nil
-  moveMem(self.nodes[0].addr, self.nodes[finalized_index].addr, tail * sizeof(ProtoNode))
+  moveMem(self.nodes.buf[0].addr, self.nodes.buf[final_phys_index].addr, tail * sizeof(ProtoNode))
-  self.nodes.setLen(tail)
+  self.nodes.buf.setLen(tail)
 
-  # Adjust the indices map
+  # update offset
-  for index in self.indices.mvalues():
+  self.nodes.offset = finalized_index
-    index -= finalized_index
-    if index < 0:
-      return err ForkChoiceError(
-        kind: fcIndexUnderflow,
-        underflowKind: fcUnderflowIndices
-      )
-
-  # Iterate through all the existing nodes and adjust their indices to match
-  # the new layout of `self.nodes`
-  for node in self.nodes.mitems():
-    # If `node.parent` is less than `finalized_index`, set it to None
-    if node.parent.isSome():
-      let new_parent = node.parent.unsafeGet() - finalized_index
-      if new_parent < 0:
-        node.parent = none(Index)
-      else:
-        node.parent = some(new_parent)
-
-    if node.best_child.isSome():
-      let new_best_child = node.best_child.unsafeGet() - finalized_index
-      if new_best_child < 0:
-        return err ForkChoiceError(
-          kind: fcIndexUnderflow,
-          underflowKind: fcUnderflowBestChild
-        )
-      node.best_child = some(new_best_child)
-
-    if node.best_descendant.isSome():
-      let new_best_descendant = node.best_descendant.unsafeGet() - finalized_index
-      if new_best_descendant < 0:
-        return err ForkChoiceError(
-          kind: fcIndexUnderflow,
-          underflowKind: fcUnderflowBestDescendant
-        )
-      node.best_descendant = some(new_best_descendant)
 
   return ok()
 
@@ -383,37 +363,36 @@ func maybe_update_best_child_and_descendant(
   ## 3. The child is not the best child but becomes the best child
   ## 4. The child is not the best child and does not become the best child
 
-  if child_index notin {0..self.nodes.len-1}:
+  let child = self.nodes[child_index]
+  if child.isNone():
     return err ForkChoiceError(
       kind: fcInvalidNodeIndex,
       index: child_index
     )
-  if parent_index notin {0..self.nodes.len-1}:
+
+  let parent = self.nodes[parent_index]
+  if parent.isNone():
     return err ForkChoiceError(
       kind: fcInvalidNodeIndex,
       index: parent_index
     )
 
-  # Aliases
+  let child_leads_to_viable_head = ? self.node_leads_to_viable_head(child.get())
-  template child: untyped = self.nodes[child_index]
-  template parent: untyped = self.nodes[parent_index]
-
-  let child_leads_to_viable_head = ? self.node_leads_to_viable_head(child)
 
   let # Aliases to the 3 possible (best_child, best_descendant) tuples
     change_to_none = (none(Index), none(Index))
     change_to_child = (
         some(child_index),
         # Nim `options` module doesn't implement option `or`
-        if child.best_descendant.isSome(): child.best_descendant
+        if child.get().best_descendant.isSome(): child.get().best_descendant
         else: some(child_index)
       )
-    no_change = (parent.best_child, parent.best_descendant)
+    no_change = (parent.get().best_child, parent.get().best_descendant)
 
   # TODO: state-machine? The control-flow is messy
   let (new_best_child, new_best_descendant) = block:
-    if parent.best_child.isSome:
+    if parent.get().best_child.isSome:
-      let best_child_index = parent.best_child.unsafeGet()
+      let best_child_index = parent.get().best_child.unsafeGet()
       if best_child_index == child_index and not child_leads_to_viable_head:
         # The child is already the best-child of the parent
         # but it's not viable to be the head block => remove it
@@ -423,15 +402,15 @@ func maybe_update_best_child_and_descendant(
         # that the best-descendant of the parent is up-to-date.
         change_to_child
       else:
-        if best_child_index notin {0..self.nodes.len-1}:
+        let best_child = self.nodes[best_child_index]
+        if best_child.isNone():
           return err ForkChoiceError(
             kind: fcInvalidBestDescendant,
             index: best_child_index
           )
-        let best_child = self.nodes[best_child_index]
 
         let best_child_leads_to_viable_head =
-          ? self.node_leads_to_viable_head(best_child)
+          ? self.node_leads_to_viable_head(best_child.get())
 
         if child_leads_to_viable_head and not best_child_leads_to_viable_head:
           # The child leads to a viable head, but the current best-child doesn't
@@ -439,14 +418,16 @@ func maybe_update_best_child_and_descendant(
         elif not child_leads_to_viable_head and best_child_leads_to_viable_head:
           # The best child leads to a viable head, but the child doesn't
           no_change
-        elif child.weight == best_child.weight:
+        elif child.get().weight == best_child.get().weight:
           # Tie-breaker of equal weights by root
-          if child.root.tiebreak(best_child.root):
+          if child.get().root.tiebreak(best_child.get().root):
             change_to_child
           else:
             no_change
         else: # Choose winner by weight
-          if child.weight >= best_child.weight:
+          let cw = child.get().weight
+          let bw = best_child.get().weight
+          if cw >= bw:
             change_to_child
           else:
             no_change
@@ -458,8 +439,8 @@ func maybe_update_best_child_and_descendant(
         # There is no current best-child but the child is not viable
         no_change
 
-  self.nodes[parent_index].best_child = new_best_child
+  self.nodes.buf[parent_index - self.nodes.offset].best_child = new_best_child
-  self.nodes[parent_index].best_descendant = new_best_descendant
+  self.nodes.buf[parent_index - self.nodes.offset].best_descendant = new_best_descendant
 
   return ok()
 
@@ -471,13 +452,13 @@ func node_leads_to_viable_head(
   let best_descendant_is_viable_for_head = block:
     if node.best_descendant.isSome():
       let best_descendant_index = node.best_descendant.unsafeGet()
-      if best_descendant_index notin {0..self.nodes.len-1}:
+      let best_descendant = self.nodes[best_descendant_index]
+      if best_descendant.isNone:
         return err ForkChoiceError(
             kind: fcInvalidBestDescendant,
             index: best_descendant_index
           )
-      let best_descendant = self.nodes[best_descendant_index]
+      self.node_is_viable_for_head(best_descendant.get())
-      self.node_is_viable_for_head(best_descendant)
     else:
       false
 

beacon_chain/validator_duties.nim

@@ -7,7 +7,7 @@
 
 import
   # Standard library
-  os, tables, strutils,
+  std/[os, tables, strutils],
 
   # Nimble packages
   stew/[objects], stew/shims/macros,
@@ -439,7 +439,11 @@ proc broadcastAggregatedAttestations(
 proc handleValidatorDuties*(
     node: BeaconNode, lastSlot, slot: Slot) {.async.} =
   ## Perform validator duties - create blocks, vote and aggregate existing votes
-  var head = node.updateHead(slot)
+  let maybeHead = node.updateHead(slot)
+  if maybeHead.isNil():
+    error "Couldn't update head - cannot proceed with validator duties"
+    return
+  var head = maybeHead
   if node.attachedValidators.count == 0:
     # Nothing to do because we have no validator attached
     return
@@ -496,7 +500,11 @@ proc handleValidatorDuties*(
   template sleepToSlotOffsetWithHeadUpdate(extra: chronos.Duration, msg: static string) =
     if await node.beaconClock.sleepToSlotOffset(extra, slot, msg):
       # Time passed - we might need to select a new head in that case
-      head = node.updateHead(slot)
+      let maybeHead = node.updateHead(slot)
+      if not maybeHead.isNil():
+        head = maybeHead
+      else:
+        error "Couldn't update head"
 
   sleepToSlotOffsetWithHeadUpdate(
     seconds(int64(SECONDS_PER_SLOT)) div 3, "Waiting to send attestations")

tests/fork_choice/interpreter.nim

@@ -57,7 +57,6 @@ type
       target_epoch*: Epoch
     of Prune: # ProtoArray specific
       finalized_root*: Eth2Digest
-      prune_threshold*: int
       expected_len*: int
 
 func apply(ctx: var ForkChoiceBackend, id: int, op: Operation) =
@@ -97,8 +96,7 @@ func apply(ctx: var ForkChoiceBackend, id: int, op: Operation) =
     )
     debugEcho "    Processed att target 0x", op.block_root, " from validator ", op.validator_index, " for epoch ", op.target_epoch
   of Prune:
-    ctx.proto_array.prune_threshold = op.prune_threshold
+    let r = ctx.prune(op.finalized_root)
-    let r = ctx.maybe_prune(op.finalized_root)
     doAssert r.isOk(), &"prune (op #{id}) returned an error: {r.error}"
     doAssert ctx.proto_array.nodes.len == op.expected_len,
       &"prune (op #{id}): the resulting length ({ctx.proto_array.nodes.len}) was not expected ({op.expected_len})"

tests/fork_choice/scenarios/votes.nim

@@ -622,25 +622,7 @@ proc setup_votes(): tuple[fork_choice: ForkChoiceBackend, ops: seq[Operation]] =
     expected_head: fakeHash(9)
   )
 
-  # Pruning below the prune threshold doesn't prune
+  # Ensure that pruning does prune.
-  result.ops.add Operation(
-    kind: Prune,
-    finalized_root: fakeHash(5),
-    prune_threshold: high(int),
-    expected_len: 11
-  )
-
-  # Prune shouldn't have changed the head
-  result.ops.add Operation(
-    kind: FindHead,
-    justified_epoch: Epoch(2),
-    justified_root: fakeHash(5),
-    finalized_epoch: Epoch(2),
-    justified_state_balances: balances,
-    expected_head: fakeHash(9)
-  )
-
-  # Ensure that pruning above the prune threshold does prune.
   #
   #
   #          0
@@ -658,10 +640,12 @@ proc setup_votes(): tuple[fork_choice: ForkChoiceBackend, ops: seq[Operation]] =
   #          8
   #         / \
   #        9  10
+  # Note: 5 and 6 become orphans
+  # - 5 is the new root
+  # - 6 is a discarded chain
   result.ops.add Operation(
     kind: Prune,
     finalized_root: fakeHash(5),
-    prune_threshold: 1,
     expected_len: 6
   )
 

tests/test_attestation_pool.nim

@@ -8,7 +8,7 @@
 {.used.}
 
 import
-  unittest,
+  std/[unittest, options],
   chronicles,
   stew/byteutils,
   ./testutil, ./testblockutil,
@@ -39,6 +39,8 @@ func combine(tgt: var Attestation, src: Attestation, flags: UpdateFlags) =
 
 template wrappedTimedTest(name: string, body: untyped) =
   # `check` macro takes a copy of whatever it's checking, on the stack!
+  # This leads to stack overflow
+  # We can mitigate that by wrapping checks in proc
   block: # Symbol namespacing
     proc wrappedTest() =
       timedTest name:
@@ -60,7 +62,7 @@ suiteReport "Attestation pool processing" & preset():
     check:
       process_slots(state.data, state.data.data.slot + 1)
 
-  timedTest "Can add and retrieve simple attestation" & preset():
+  wrappedTimedTest "Can add and retrieve simple attestation" & preset():
     var cache = StateCache()
     let
       # Create an attestation for slot 1!
@@ -79,7 +81,7 @@ suiteReport "Attestation pool processing" & preset():
     check:
       attestations.len == 1
 
-  timedTest "Attestations may arrive in any order" & preset():
+  wrappedTimedTest "Attestations may arrive in any order" & preset():
     var cache = StateCache()
     let
       # Create an attestation for slot 1!
@@ -108,7 +110,7 @@ suiteReport "Attestation pool processing" & preset():
     check:
       attestations.len == 1
 
-  timedTest "Attestations should be combined" & preset():
+  wrappedTimedTest "Attestations should be combined" & preset():
     var cache = StateCache()
     let
       # Create an attestation for slot 1!
@@ -130,7 +132,7 @@ suiteReport "Attestation pool processing" & preset():
     check:
       attestations.len == 1
 
-  timedTest "Attestations may overlap, bigger first" & preset():
+  wrappedTimedTest "Attestations may overlap, bigger first" & preset():
     var cache = StateCache()
 
     var
@@ -155,7 +157,7 @@ suiteReport "Attestation pool processing" & preset():
     check:
       attestations.len == 1
 
-  timedTest "Attestations may overlap, smaller first" & preset():
+  wrappedTimedTest "Attestations may overlap, smaller first" & preset():
     var cache = StateCache()
     var
       # Create an attestation for slot 1!
@@ -179,7 +181,7 @@ suiteReport "Attestation pool processing" & preset():
     check:
       attestations.len == 1
 
-  timedTest "Fork choice returns latest block with no attestations":
+  wrappedTimedTest "Fork choice returns latest block with no attestations":
     var cache = StateCache()
     let
       b1 = addTestBlock(state.data, chainDag.tail.root, cache)
@@ -207,7 +209,7 @@ suiteReport "Attestation pool processing" & preset():
     check:
       head2 == b2Add[]
 
-  timedTest "Fork choice returns block with attestation":
+  wrappedTimedTest "Fork choice returns block with attestation":
     var cache = StateCache()
     let
       b10 = makeTestBlock(state.data, chainDag.tail.root, cache)
@@ -264,7 +266,7 @@ suiteReport "Attestation pool processing" & preset():
       # Two votes for b11
       head4 == b11Add[]
 
-  timedTest "Trying to add a block twice tags the second as an error":
+  wrappedTimedTest "Trying to add a block twice tags the second as an error":
     var cache = StateCache()
     let
       b10 = makeTestBlock(state.data, chainDag.tail.root, cache)
@@ -291,12 +293,13 @@ suiteReport "Attestation pool processing" & preset():
     doAssert: b10Add_clone.error == Duplicate
 
   wrappedTimedTest "Trying to add a duplicate block from an old pruned epoch is tagged as an error":
+    # Note: very sensitive to stack usage
+
     chainDag.updateFlags.incl {skipBLSValidation}
-    pool.forkChoice.backend.proto_array.prune_threshold = 1
     var cache = StateCache()
     let
-      b10 = makeTestBlock(state.data, chainDag.tail.root, cache)
+      b10 = newClone(makeTestBlock(state.data, chainDag.tail.root, cache))
-      b10Add = chainDag.addRawBlock(quarantine, b10) do (
+      b10Add = chainDag.addRawBlock(quarantine, b10[]) do (
           blckRef: BlockRef, signedBlock: SignedBeaconBlock,
           epochRef: EpochRef, state: HashedBeaconState):
         # Callback add to fork choice if valid
@@ -306,7 +309,7 @@ suiteReport "Attestation pool processing" & preset():
 
     doAssert: head == b10Add[]
 
-    let block_ok = state_transition(defaultRuntimePreset, state.data, b10, {}, noRollback)
+    let block_ok = state_transition(defaultRuntimePreset, state.data, b10[], {}, noRollback)
     doAssert: block_ok
 
     # -------------------------------------------------------------
@@ -323,14 +326,14 @@ suiteReport "Attestation pool processing" & preset():
       let committees_per_slot =
         get_committee_count_per_slot(state.data.data, Epoch epoch, cache)
       for slot in start_slot ..< start_slot + SLOTS_PER_EPOCH:
-        let new_block = makeTestBlock(
+        let new_block = newClone(makeTestBlock(
-          state.data, block_root, cache, attestations = attestations)
+          state.data, block_root, cache, attestations = attestations))
         let block_ok = state_transition(
-          defaultRuntimePreset, state.data, new_block, {skipBLSValidation}, noRollback)
+          defaultRuntimePreset, state.data, new_block[], {skipBLSValidation}, noRollback)
         doAssert: block_ok
 
         block_root = new_block.root
-        let blockRef = chainDag.addRawBlock(quarantine, new_block) do (
+        let blockRef = chainDag.addRawBlock(quarantine, new_block[]) do (
             blckRef: BlockRef, signedBlock: SignedBeaconBlock,
             epochRef: EpochRef, state: HashedBeaconState):
           # Callback add to fork choice if valid
@@ -368,10 +371,10 @@ suiteReport "Attestation pool processing" & preset():
     doAssert: chainDag.finalizedHead.slot != 0
 
     pool[].prune()
-    doAssert: b10.root notin pool.forkChoice.backend
+    doAssert: b10[].root notin pool.forkChoice.backend
 
     # Add back the old block to ensure we have a duplicate error
-    let b10Add_clone = chainDag.addRawBlock(quarantine, b10_clone) do (
+    let b10Add_clone = chainDag.addRawBlock(quarantine, b10_clone[]) do (
           blckRef: BlockRef, signedBlock: SignedBeaconBlock,
           epochRef: EpochRef, state: HashedBeaconState):
         # Callback add to fork choice if valid

tests/test_block_pool.nim

@@ -17,8 +17,18 @@ import
 when isMainModule:
   import chronicles # or some random compile error happens...
 
+template wrappedTimedTest(name: string, body: untyped) =
+  # `check` macro takes a copy of whatever it's checking, on the stack!
+  # This leads to stack overflow
+  # We can mitigate that by wrapping checks in proc
+  block: # Symbol namespacing
+    proc wrappedTest() =
+      timedTest name:
+        body
+    wrappedTest()
+
 suiteReport "BlockRef and helpers" & preset():
-  timedTest "isAncestorOf sanity" & preset():
+  wrappedTimedTest "isAncestorOf sanity" & preset():
     let
       s0 = BlockRef(slot: Slot(0))
       s1 = BlockRef(slot: Slot(1), parent: s0)
@@ -35,7 +45,7 @@ suiteReport "BlockRef and helpers" & preset():
       not s2.isAncestorOf(s1)
       not s1.isAncestorOf(s0)
 
-  timedTest "get_ancestor sanity" & preset():
+  wrappedTimedTest "get_ancestor sanity" & preset():
     let
       s0 = BlockRef(slot: Slot(0))
       s1 = BlockRef(slot: Slot(1), parent: s0)
@@ -55,7 +65,7 @@ suiteReport "BlockRef and helpers" & preset():
       s4.get_ancestor(Slot(3)) == s2
       s4.get_ancestor(Slot(4)) == s4
 
-  timedTest "epochAncestor sanity" & preset():
+  wrappedTimedTest "epochAncestor sanity" & preset():
     let
       s0 = BlockRef(slot: Slot(0))
     var cur = s0
@@ -72,7 +82,7 @@ suiteReport "BlockRef and helpers" & preset():
       ancestor.blck.epochAncestor(ancestor.blck.slot.epoch) != ancestor
 
 suiteReport "BlockSlot and helpers" & preset():
-  timedTest "atSlot sanity" & preset():
+  wrappedTimedTest "atSlot sanity" & preset():
     let
       s0 = BlockRef(slot: Slot(0))
       s1 = BlockRef(slot: Slot(1), parent: s0)
@@ -86,7 +96,7 @@ suiteReport "BlockSlot and helpers" & preset():
 
       s4.atSlot(Slot(0)).blck == s0
 
-  timedTest "parent sanity" & preset():
+  wrappedTimedTest "parent sanity" & preset():
     let
       s0 = BlockRef(slot: Slot(0))
       s00 = BlockSlot(blck: s0, slot: Slot(0))
@@ -114,18 +124,18 @@ suiteReport "Block pool processing" & preset():
       b1Root = hash_tree_root(b1.message)
       b2 = addTestBlock(stateData.data, b1Root, cache)
       b2Root {.used.} = hash_tree_root(b2.message)
-  timedTest "getRef returns nil for missing blocks":
+  wrappedTimedTest "getRef returns nil for missing blocks":
     check:
       dag.getRef(default Eth2Digest) == nil
 
-  timedTest "loadTailState gets genesis block on first load" & preset():
+  wrappedTimedTest "loadTailState gets genesis block on first load" & preset():
     let
       b0 = dag.get(dag.tail.root)
 
     check:
       b0.isSome()
 
-  timedTest "Simple block add&get" & preset():
+  wrappedTimedTest "Simple block add&get" & preset():
     let
       b1Add = dag.addRawBlock(quarantine, b1, nil)
       b1Get = dag.get(b1.root)
@@ -193,7 +203,7 @@ suiteReport "Block pool processing" & preset():
       dag.getBlockRange(Slot(3), 2, blocks.toOpenArray(0, 1)) == 2
       blocks[2..<2].len == 0
 
-  timedTest "Reverse order block add & get" & preset():
+  wrappedTimedTest "Reverse order block add & get" & preset():
     let missing = dag.addRawBlock(quarantine, b2, nil)
     check: missing.error == MissingParent
 
@@ -235,7 +245,7 @@ suiteReport "Block pool processing" & preset():
       dag2.heads.len == 1
       dag2.heads[0].root == b2Root
 
-  timedTest "Adding the same block twice returns a Duplicate error" & preset():
+  wrappedTimedTest "Adding the same block twice returns a Duplicate error" & preset():
     let
       b10 = dag.addRawBlock(quarantine, b1, nil)
       b11 = dag.addRawBlock(quarantine, b1, nil)
@@ -244,7 +254,7 @@ suiteReport "Block pool processing" & preset():
       b11.error == Duplicate
       not b10[].isNil
 
-  timedTest "updateHead updates head and headState" & preset():
+  wrappedTimedTest "updateHead updates head and headState" & preset():
     let
       b1Add = dag.addRawBlock(quarantine, b1, nil)
 
@@ -254,7 +264,7 @@ suiteReport "Block pool processing" & preset():
       dag.head == b1Add[]
       dag.headState.data.data.slot == b1Add[].slot
 
-  timedTest "updateStateData sanity" & preset():
+  wrappedTimedTest "updateStateData sanity" & preset():
     let
       b1Add = dag.addRawBlock(quarantine, b1, nil)
       b2Add = dag.addRawBlock(quarantine, b2, nil)
@@ -311,7 +321,7 @@ suiteReport "chain DAG finalization tests" & preset():
       quarantine = QuarantineRef()
       cache = StateCache()
 
-  timedTest "prune heads on finalization" & preset():
+  wrappedTimedTest "prune heads on finalization" & preset():
     # Create a fork that will not be taken
     var
       blck = makeTestBlock(dag.headState.data, dag.head.root, cache)
@@ -386,7 +396,7 @@ suiteReport "chain DAG finalization tests" & preset():
       hash_tree_root(dag2.headState.data.data) ==
         hash_tree_root(dag.headState.data.data)
 
-  timedTest "orphaned epoch block" & preset():
+  wrappedTimedTest "orphaned epoch block" & preset():
     var prestate = (ref HashedBeaconState)()
     for i in 0 ..< SLOTS_PER_EPOCH:
       if i == SLOTS_PER_EPOCH - 1:
@@ -426,7 +436,7 @@ suiteReport "chain DAG finalization tests" & preset():
       quarantine = QuarantineRef()
       cache = StateCache()
 
-  timedTest "init with gaps" & preset():
+  wrappedTimedTest "init with gaps" & preset():
     for i in 0 ..< (SLOTS_PER_EPOCH * 6 - 2):
       var
         blck = makeTestBlock(