check that parent of added block is sufficiently recent (#1269)

Otherwise, we might introduce a fork into the DAG that is no longer
viable, creating trouble for both sync and fork choice

beacon_chain/beacon_node.nim

@@ -560,10 +560,10 @@ proc runForwardSyncLoop(node: BeaconNode) {.async.} =
     let sm = now(chronos.Moment)
     for blk in list:
       let res = node.storeBlock(blk)
-      # We going to ignore `BlockError.Old` errors because we have working
+      # We going to ignore `BlockError.Unviable` errors because we have working
       # backward sync and it can happens that we can perform overlapping
       # requests.
-      if res.isErr and res.error != BlockError.Old:
+      if res.isErr and res.error != BlockError.Unviable:
         return res
     discard node.updateHead()
 

beacon_chain/block_pools/block_pools_types.nim

@@ -24,9 +24,18 @@ import
 
 type
   BlockError* = enum
-    MissingParent
-    Old
-    Invalid
+    MissingParent ##\
+      ## We don't know the parent of this block so we can't tell if it's valid
+      ## or not - it'll go into the quarantine and be reexamined when the parent
+      ## appears or be discarded if finality obsoletes it
+
+    Unviable ##\
+      ## Block is from a different history / fork than the one we're interested
+      ## in (based on our finalized checkpoint)
+
+    Invalid ##\
+      ## Block is broken / doesn't apply cleanly - whoever sent it is fishy (or
+      ## we're buggy)
 
   Quarantine* = object
     ## Keeps track of unsafe blocks coming from the network

beacon_chain/block_pools/clearance.nim

@@ -151,13 +151,14 @@ proc add*(
       blockRoot = shortLog(blockRoot),
       cat = "filtering"
 
-    return err Old
+    return err Unviable
 
   let parent = dag.blocks.getOrDefault(blck.parent_root)
 
   if parent != nil:
     if parent.slot >= blck.slot:
-      # TODO Malicious block? inform peer dag?
+      # A block whose parent is newer than the block itself is clearly invalid -
+      # discard it immediately
       notice "Invalid block slot",
         blck = shortLog(blck),
         blockRoot = shortLog(blockRoot),
@@ -165,6 +166,25 @@ proc add*(
 
       return err Invalid
 
+    if parent.slot < dag.finalizedHead.slot:
+      # We finalized a block that's newer than the parent of this block - this
+      # block, although recent, is thus building on a history we're no longer
+      # interested in pursuing. This can happen if a client produces a block
+      # while syncing - ie it's own head block will be old, but it'll create
+      # a block according to the wall clock, in its own little world - this is
+      # correct - from their point of view, the head block they have is the
+      # latest thing that happened on the chain and they're performing their
+      # duty correctly.
+      debug "Unviable block, dropping",
+        blck = shortLog(blck),
+        finalizedHead = shortLog(dag.finalizedHead),
+        tail = shortLog(dag.tail),
+        blockRoot = shortLog(blockRoot),
+        cat = "filtering"
+
+      return err Unviable
+
+
     # The block might have been in either of `orphans` or `missing` - we don't
     # want any more work done on its behalf
     quarantine.orphans.del(blockRoot)