fix finalization cleanup

* add block pool finalization test

beacon_chain/block_pool.nim

@@ -828,29 +828,27 @@ proc updateHead*(pool: BlockPool, newHead: BlockRef) =
     "Block graph should always lead to a finalized block"
 
   if finalizedHead != pool.finalizedHead:
-    pool.finalizedHead = finalizedHead
-
     var cur = finalizedHead.blck
     while cur != pool.finalizedHead.blck:
       # Finalization means that we choose a single chain as the canonical one -
       # it also means we're no longer interested in any branches from that chain
-      # up to the finalization point
+      # up to the finalization point.
+      # The new finalized head should not be cleaned! We start at its parent and
+      # clean everything including the old finalized head.
+      cur = cur.parent
+
+      pool.delFinalizedStateIfNeeded(cur)
 
-      # TODO technically, if we remove from children the gc should free the block
-      #      because it should become orphaned, via mark&sweep if nothing else,
-      #      though this needs verification
       # TODO what about attestations? we need to drop those too, though they
       #      *should* be pretty harmless
-      # TODO remove from database as well.. here, or using some GC-like setup
+      if cur.parent != nil: # This happens for the genesis / tail block
-      #      that periodically cleans it up?
         for child in cur.parent.children:
           if child != cur:
             pool.blocks.del(child.root)
             pool.delBlockAndState(child.root)
-        else:
-          pool.delFinalizedStateIfNeeded(child)
         cur.parent.children = @[cur]
-      cur = cur.parent
+
+    pool.finalizedHead = finalizedHead
 
     let hlen = pool.heads.len
     for i in 0..<hlen:

beacon_chain/spec/crypto.nim

@@ -122,10 +122,10 @@ func pubKey*(pk: ValidatorPrivKey): ValidatorPubKey =
   else:
     pk.getKey
 
-func init(T: type VerKey): VerKey =
+func init*(T: type VerKey): VerKey =
   result.point.inf()
 
-func init(T: type Signature): Signature =
+func init*(T: type Signature): Signature =
   result.point.inf()
 
 func combine*[T](values: openarray[BlsValue[T]]): BlsValue[T] =

tests/test_block_pool.nim

@@ -10,8 +10,8 @@
 import
   options, sequtils, unittest, chronicles,
   ./testutil, ./testblockutil,
-  ../beacon_chain/spec/[datatypes, digest],
+  ../beacon_chain/spec/[datatypes, digest, helpers, validator],
-  ../beacon_chain/[beacon_node_types, block_pool, beacon_chain_db, ssz]
+  ../beacon_chain/[beacon_node_types, block_pool, beacon_chain_db, extras, ssz]
 
 suite "BlockRef and helpers" & preset():
   timedTest "isAncestorOf sanity" & preset():
@@ -186,3 +186,38 @@ when const_preset == "minimal": # Too much stack space used on mainnet
       check:
         pool.head.blck == b1Add
         pool.headState.data.data.slot == b1Add.slot
+
+  suite "BlockPool finalization tests" & preset():
+    setup:
+      var
+        db = makeTestDB(SLOTS_PER_EPOCH)
+        pool = BlockPool.init(db)
+
+    timedTest "prune heads on finalization" & preset():
+      block:
+        # Create a fork that will not be taken
+        var
+          blck = makeBlock(pool.headState.data.data, pool.head.blck.root,
+            BeaconBlockBody())
+        discard pool.add(hash_tree_root(blck.message), blck)
+
+      for i in 0 ..< (SLOTS_PER_EPOCH * 4):
+        if i == 1:
+          # There are 2 heads now because of the fork at slot 1
+          check:
+            pool.tail.children.len == 2
+            pool.heads.len == 2
+        var
+          cache = get_empty_per_epoch_cache()
+          blck = makeBlock(pool.headState.data.data, pool.head.blck.root,
+            BeaconBlockBody(
+              attestations: makeFullAttestations(
+                pool.headState.data.data, pool.head.blck.root,
+                pool.headState.data.data.slot, cache, {skipValidation})))
+        let added = pool.add(hash_tree_root(blck.message), blck)
+        pool.updateHead(added)
+
+      check:
+        pool.heads.len() == 1
+        pool.head.justified.slot.compute_epoch_at_slot() == 3
+        pool.tail.children.len == 1

tests/testblockutil.nim

@@ -93,7 +93,9 @@ proc addBlock*(
 
   # TODO ugly hack; API needs rethinking
   var new_body = body
+  if skipValidation notin flags:
     new_body.randao_reveal = privKey.genRandaoReveal(state.fork, state.slot + 1)
+
   new_body.eth1_data = Eth1Data()
 
   var
@@ -171,11 +173,8 @@ proc makeAttestation*(
     sig =
       if skipValidation notin flags:
         bls_sign(
-          hackPrivKey(validator), @(msg.data),
+          hackPrivKey(validator), msg.data,
-          get_domain(
+          get_domain(state, DOMAIN_BEACON_ATTESTER, data.target.epoch))
-            state,
-            DOMAIN_BEACON_ATTESTER,
-            data.target.epoch))
       else:
         ValidatorSig()
 
@@ -208,3 +207,33 @@ proc makeAttestation*(
     find_beacon_committee(state, validator_index, cache)
   makeAttestation(state, beacon_block_root, committee, slot, index,
     validator_index, cache, flags)
+
+proc makeFullAttestations*(
+    state: BeaconState, beacon_block_root: Eth2Digest, slot: Slot,
+    cache: var StateCache,
+    flags: UpdateFlags = {}): seq[Attestation] =
+  # Create attestations in which the full committee participates for each shard
+  # that should be attested to during a particular slot
+  let
+    count = get_committee_count_at_slot(state, slot)
+
+  for index in 0..<count:
+    let
+      committee = get_beacon_committee(state, slot, index, cache)
+      data = makeAttestationData(state, slot, index, beacon_block_root)
+      msg = hash_tree_root(data)
+
+    var
+      attestation = Attestation(
+        aggregation_bits: CommitteeValidatorsBits.init(committee.len),
+        data: data,
+        signature: ValidatorSig(kind: Real, blsValue: Signature.init())
+      )
+    for j in 0..<committee.len():
+      attestation.aggregation_bits.setBit j
+      if skipValidation notin flags:
+        attestation.signature.combine(bls_sign(
+          hackPrivKey(state.validators[committee[j]]), msg.data,
+          get_domain(state, DOMAIN_BEACON_ATTESTER, data.target.epoch)))
+
+    result.add attestation