add tests for unviable blocks (#1271)

* add tests for unviable blocks

also enable finalization tests in all test configs - they're plenty fast
now
also fix newClone for non-rvo cases. sigh.

* fixes

AllTests-mainnet.md

@@ -50,6 +50,12 @@ OK: 7/7 Fail: 0/7 Skip: 0/7
 + Passes through epoch update, no block [Preset: mainnet]                                    OK
 ```
 OK: 5/5 Fail: 0/5 Skip: 0/5
+## BlockPool finalization tests [Preset: mainnet]
+```diff
++ init with gaps [Preset: mainnet]                                                           OK
++ prune heads on finalization [Preset: mainnet]                                              OK
+```
+OK: 2/2 Fail: 0/2 Skip: 0/2
 ## BlockRef and helpers [Preset: mainnet]
 ```diff
 + getAncestorAt sanity [Preset: mainnet]                                                     OK
@@ -259,4 +265,4 @@ OK: 8/8 Fail: 0/8 Skip: 0/8
 OK: 1/1 Fail: 0/1 Skip: 0/1
 
 ---TOTAL---
-OK: 158/161 Fail: 0/161 Skip: 3/161
+OK: 160/163 Fail: 0/163 Skip: 3/163

beacon_chain/spec/datatypes.nim

@@ -607,10 +607,19 @@ template writeValue*(writer: var JsonWriter, value: BitList) =
 
 template newClone*[T: not ref](x: T): ref T =
   # TODO not nil in return type: https://github.com/nim-lang/Nim/issues/14146
+  # TODO use only when x is a function call that returns a new instance!
   let res = new typeof(x) # TODO safe to do noinit here?
   res[] = x
   res
 
+template assignClone*[T: not ref](x: T): ref T =
+  # This is a bit of a mess: if x is an rvalue (temporary), RVO kicks in for
+  # newClone - if it's not, `genericAssign` will be called which is ridiculously
+  # slow - so `assignClone` should be used when RVO doesn't work. sigh.
+  let res = new typeof(x) # TODO safe to do noinit here?
+  assign(res[], x)
+  res
+
 template newClone*[T](x: ref T not nil): ref T =
   newClone(x[])
 

research/block_sim.nim

@@ -61,7 +61,7 @@ cli do(slots = SLOTS_PER_EPOCH * 6,
     attesters: RunningStat
     r = initRand(1)
 
-  let replayState = newClone(blockPool.headState)
+  let replayState = assignClone(blockPool.headState)
 
   proc handleAttestations(slot: Slot) =
     let

tests/spec_block_processing/test_process_deposits.nim

@@ -33,7 +33,7 @@ suiteReport "[Unit - Spec - Block processing] Deposits " & preset():
     # TODO: BLS signature
     timedTest "Deposit " & name & " MAX_EFFECTIVE_BALANCE balance (" &
           $(MAX_EFFECTIVE_BALANCE div 10'u64^9) & " ETH)":
-      var state = newClone(genesisState[])
+      var state = assignClone(genesisState[])
 
       # Test configuration
       # ----------------------------------------
@@ -74,7 +74,7 @@ suiteReport "[Unit - Spec - Block processing] Deposits " & preset():
   valid_deposit(MAX_EFFECTIVE_BALANCE + 1, "over")
 
   timedTest "Validator top-up":
-    var state = newClone(genesisState)
+    var state = assignClone(genesisState[])
 
     # Test configuration
     # ----------------------------------------

tests/spec_epoch_processing/test_process_justification_and_finalization.nim

@@ -221,7 +221,7 @@ proc payload =
     doAssert genesisState.data.validators.len == int NumValidators
 
     setup:
-      var state = newClone(genesisState[])
+      var state = assignClone(genesisState[])
 
     timedTest " Rule I - 234 finalization with enough support":
       finalizeOn234(state[], Epoch 5, sufficient_support = true)

tests/test_block_pool.nim

@@ -10,7 +10,7 @@
 import
   options, sequtils, unittest,
   ./testutil, ./testblockutil,
-  ../beacon_chain/spec/[datatypes, digest, validator, state_transition],
+  ../beacon_chain/spec/[datatypes, digest, helpers, validator, state_transition],
   ../beacon_chain/[beacon_node_types, block_pool, ssz]
 
 when isMainModule:
@@ -240,7 +240,7 @@ suiteReport "Block pool processing" & preset():
       bs1_3 = b1Add.atSlot(3.Slot)
       bs2_3 = b2Add.atSlot(3.Slot)
 
-    var tmpState = newClone(pool.headState)
+    var tmpState = assignClone(pool.headState)
 
     # move to specific block
     pool.updateStateData(tmpState[], bs1)
@@ -280,94 +280,103 @@ suiteReport "Block pool processing" & preset():
       tmpState.blck == b1Add.parent
       tmpState.data.data.slot == bs1.parent.slot
 
-when const_preset == "minimal":  # These require some minutes in mainnet
+suiteReport "BlockPool finalization tests" & preset():
-  import ../beacon_chain/spec/helpers
+  setup:
+    var
+      db = makeTestDB(SLOTS_PER_EPOCH)
+      pool = BlockPool.init(db)
+      cache = get_empty_per_epoch_cache()
 
-  suiteReport "BlockPool finalization tests" & preset():
+  timedTest "prune heads on finalization" & preset():
-    setup:
+    # Create a fork that will not be taken
+    var
+      blck = makeTestBlock(pool.headState.data, pool.head.blck.root, cache)
+      tmpState = assignClone(pool.headState.data)
+    check:
+      process_slots(
+        tmpState[], tmpState.data.slot + (5 * SLOTS_PER_EPOCH).uint64)
+
+    let lateBlock = makeTestBlock(tmpState[], pool.head.blck.root, cache)
+    check: pool.add(hash_tree_root(blck.message), blck).isOk
+
+    for i in 0 ..< (SLOTS_PER_EPOCH * 6):
+      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
-        db = makeTestDB(SLOTS_PER_EPOCH)
-        pool = BlockPool.init(db)
         cache = get_empty_per_epoch_cache()
 
-    timedTest "prune heads on finalization" & preset():
+        blck = makeTestBlock(
-      block:
+          pool.headState.data, pool.head.blck.root, cache,
-        # Create a fork that will not be taken
+          attestations = makeFullAttestations(
-        var
+            pool.headState.data.data, pool.head.blck.root,
-          blck = makeTestBlock(pool.headState.data, pool.head.blck.root, cache)
+            pool.headState.data.data.slot, cache, {}))
-        check: pool.add(hash_tree_root(blck.message), blck).isOk
-
-      for i in 0 ..< (SLOTS_PER_EPOCH * 6):
-        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
-          blck = makeTestBlock(
-            pool.headState.data, pool.head.blck.root, cache,
-            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)[]
-        pool.updateHead(added)
-
-      check:
-        pool.heads.len() == 1
-        pool.head.justified.slot.compute_epoch_at_slot() == 5
-        pool.tail.children.len == 1
-
-      let
-        pool2 = BlockPool.init(db)
-
-      # check that the state reloaded from database resembles what we had before
-      check:
-        pool2.tail.root == pool.tail.root
-        pool2.head.blck.root == pool.head.blck.root
-        pool2.finalizedHead.blck.root == pool.finalizedHead.blck.root
-        pool2.finalizedHead.slot == pool.finalizedHead.slot
-        hash_tree_root(pool2.headState.data.data) ==
-          hash_tree_root(pool.headState.data.data)
-        hash_tree_root(pool2.justifiedState.data.data) ==
-          hash_tree_root(pool.justifiedState.data.data)
-
-    timedTest "init with gaps" & preset():
-      var cache = get_empty_per_epoch_cache()
-      for i in 0 ..< (SLOTS_PER_EPOCH * 6 - 2):
-        var
-          blck = makeTestBlock(
-            pool.headState.data, pool.head.blck.root, cache,
-            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)[]
-        pool.updateHead(added)
-
-      # Advance past epoch so that the epoch transition is gapped
-      check:
-        process_slots(
-          pool.headState.data, Slot(SLOTS_PER_EPOCH * 6 + 2) )
-
-      var blck = makeTestBlock(
-        pool.headState.data, pool.head.blck.root, cache,
-        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)[]
       pool.updateHead(added)
 
-      let
+    check:
-        pool2 = BlockPool.init(db)
+      pool.heads.len() == 1
+      pool.head.justified.slot.compute_epoch_at_slot() == 5
+      pool.tail.children.len == 1
 
-      # check that the state reloaded from database resembles what we had before
+    check:
-      check:
+      # The late block is a block whose parent was finalized long ago and thus
-        pool2.tail.root == pool.tail.root
+      # is no longer a viable head candidate
-        pool2.head.blck.root == pool.head.blck.root
+      pool.add(hash_tree_root(lateBlock.message), lateBlock).error == Unviable
-        pool2.finalizedHead.blck.root == pool.finalizedHead.blck.root
+
-        pool2.finalizedHead.slot == pool.finalizedHead.slot
+    let
-        hash_tree_root(pool2.headState.data.data) ==
+      pool2 = BlockPool.init(db)
-          hash_tree_root(pool.headState.data.data)
+
-        hash_tree_root(pool2.justifiedState.data.data) ==
+    # check that the state reloaded from database resembles what we had before
-          hash_tree_root(pool.justifiedState.data.data)
+    check:
+      pool2.tail.root == pool.tail.root
+      pool2.head.blck.root == pool.head.blck.root
+      pool2.finalizedHead.blck.root == pool.finalizedHead.blck.root
+      pool2.finalizedHead.slot == pool.finalizedHead.slot
+      hash_tree_root(pool2.headState.data.data) ==
+        hash_tree_root(pool.headState.data.data)
+      hash_tree_root(pool2.justifiedState.data.data) ==
+        hash_tree_root(pool.justifiedState.data.data)
+
+  timedTest "init with gaps" & preset():
+    var cache = get_empty_per_epoch_cache()
+    for i in 0 ..< (SLOTS_PER_EPOCH * 6 - 2):
+      var
+        blck = makeTestBlock(
+          pool.headState.data, pool.head.blck.root, cache,
+          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)[]
+      pool.updateHead(added)
+
+    # Advance past epoch so that the epoch transition is gapped
+    check:
+      process_slots(
+        pool.headState.data, Slot(SLOTS_PER_EPOCH * 6 + 2) )
+
+    var blck = makeTestBlock(
+      pool.headState.data, pool.head.blck.root, cache,
+      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)[]
+    pool.updateHead(added)
+
+    let
+      pool2 = BlockPool.init(db)
+
+    # check that the state reloaded from database resembles what we had before
+    check:
+      pool2.tail.root == pool.tail.root
+      pool2.head.blck.root == pool.head.blck.root
+      pool2.finalizedHead.blck.root == pool.finalizedHead.blck.root
+      pool2.finalizedHead.slot == pool.finalizedHead.slot
+      hash_tree_root(pool2.headState.data.data) ==
+        hash_tree_root(pool.headState.data.data)
+      hash_tree_root(pool2.justifiedState.data.data) ==
+        hash_tree_root(pool.justifiedState.data.data)
 

tests/testblockutil.nim

@@ -143,7 +143,7 @@ proc makeTestBlock*(
   # It's a bit awkward - in order to produce a block for N+1, we need to
   # calculate what the state will look like after that block has been applied,
   # because the block includes the state root.
-  var tmpState = newClone(state)
+  var tmpState = assignClone(state)
   addTestBlock(
     tmpState[], parent_root, cache, eth1_data, attestations, deposits,
     graffiti, flags)