update attestation and block validation to v0.12.2; clean up getAncestorAt()/get_ancestor() (#1417)

* update attestation validation to v0.12.2; clean up getAncestorAt()/get_ancestor()

* update beacon block validation to v0.12.2

AllTests-mainnet.md

@@ -54,7 +54,7 @@ OK: 7/7 Fail: 0/7 Skip: 0/7
 OK: 5/5 Fail: 0/5 Skip: 0/5
 ## BlockRef and helpers [Preset: mainnet]
 ```diff
-+ getAncestorAt sanity [Preset: mainnet]                                                     OK
++ get_ancestor sanity [Preset: mainnet]                                                      OK
 + isAncestorOf sanity [Preset: mainnet]                                                      OK
 ```
 OK: 2/2 Fail: 0/2 Skip: 0/2

beacon_chain/attestation_aggregation.nim

@@ -99,11 +99,14 @@ proc isValidAttestationSlot(
   true
 
 func checkPropagationSlotRange(data: AttestationData, current_slot: Slot): bool =
+  # https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/p2p-interface.md#beacon_attestation_subnet_id
   # TODO clock disparity
+  # attestation.data.slot + ATTESTATION_PROPAGATION_SLOT_RANGE >=
+  # current_slot >= attestation.data.slot
   (data.slot + ATTESTATION_PROPAGATION_SLOT_RANGE >= current_slot) and
     (current_slot >= data.slot)
 
-# https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/p2p-interface.md#attestation-subnets
+# https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/p2p-interface.md#beacon_attestation_subnet_id
 proc isValidAttestation*(
     pool: var AttestationPool, attestation: Attestation, current_slot: Slot,
     topicCommitteeIndex: uint64): bool =
@@ -154,11 +157,8 @@ proc isValidAttestation*(
 
   # The block being voted for (attestation.data.beacon_block_root) passes
   # validation.
-  # We rely on the chain DAG to have been validated, so check for the
+  # We rely on the chain DAG to have been validated, so check for the existence
-  # existence of the block in the pool.
+  # of the block in the pool.
-  # TODO: consider a "slush pool" of attestations whose blocks have not yet
-  # propagated - i.e. imagine that attestations are smaller than blocks and
-  # therefore propagate faster, thus reordering their arrival in some nodes
   let attestationBlck = pool.chainDag.getRef(attestation.data.beacon_block_root)
   if attestationBlck.isNil:
     debug "Block not found"
@@ -173,16 +173,42 @@ proc isValidAttestation*(
   let tgtBlck = pool.chainDag.getRef(attestation.data.target.root)
   if tgtBlck.isNil:
     debug "Target block not found"
+    pool.addUnresolved(attestation)
     pool.quarantine.addMissing(attestation.data.beacon_block_root)
-    return
+    return false
+
+  # The current finalized_checkpoint is an ancestor of the block defined by
+  # attestation.data.beacon_block_root -- i.e. get_ancestor(store,
+  # attestation.data.beacon_block_root,
+  # compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)) ==
+  # store.finalized_checkpoint.root
+  let
+    ancestor = get_ancestor(
+      attestationBlck,
+      compute_start_slot_at_epoch(
+        pool.chainDag.headState.data.data.finalized_checkpoint.epoch))
+    finalized_root =
+      pool.chainDag.headState.data.data.finalized_checkpoint.root
+  if ancestor.isNil:
+    debug "Can't find ancestor block"
+    return false
+  # TODO spec doesn't note the pre-finalization case, but maybe necessary
+  if not (finalized_root in [Eth2Digest(), ancestor.root]):
+    debug "Incorrect ancestor block",
+      ancestor_root = ancestor.root,
+      finalized_root = pool.chainDag.headState.data.data.finalized_checkpoint.root
+    return false
 
-  # TODO this could be any state in the target epoch
   pool.chainDag.withState(
       pool.chainDag.tmpState,
       tgtBlck.atSlot(attestation.data.target.epoch.compute_start_slot_at_epoch)):
-    # https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/p2p-interface.md#attestation-subnets
+    # https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/p2p-interface.md#beacon_attestation_subnet_id
-    # [REJECT] The attestation is for the correct subnet (i.e.
+    # [REJECT] The attestation is for the correct subnet -- i.e.
-    # compute_subnet_for_attestation(state, attestation) == subnet_id).
+    # compute_subnet_for_attestation(committees_per_slot,
+    # attestation.data.slot, attestation.data.index) == subnet_id, where
+    # committees_per_slot = get_committee_count_per_slot(state,
+    # attestation.data.target.epoch), which may be pre-computed along with the
+    # committee information for the signature check.
     let
       epochInfo = blck.getEpochInfo(state)
       requiredSubnetIndex =

beacon_chain/block_pools/chain_dag.nim

@@ -94,9 +94,11 @@ func isAncestorOf*(a, b: BlockRef): bool =
     doAssert b.slot > b.parent.slot
     b = b.parent
 
-func getAncestorAt*(blck: BlockRef, slot: Slot): BlockRef =
+func get_ancestor*(blck: BlockRef, slot: Slot): BlockRef =
+  ## https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/fork-choice.md#get_ancestor
   ## Return the most recent block as of the time at `slot` that not more recent
   ## than `blck` itself
+  doAssert not blck.isNil
 
   var blck = blck
 
@@ -115,29 +117,6 @@ func getAncestorAt*(blck: BlockRef, slot: Slot): BlockRef =
 
     blck = blck.parent
 
-func get_ancestor*(blck: BlockRef, slot: Slot): BlockRef =
-  ## https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/fork-choice.md#get_ancestor
-  ## Return ancestor at slot, or nil if queried block is older
-  var blck = blck
-
-  var depth = 0
-  const maxDepth = (100'i64 * 365 * 24 * 60 * 60 div SECONDS_PER_SLOT.int)
-
-  while true:
-    if blck.slot == slot:
-      return blck
-
-    if blck.slot < slot:
-      return nil
-
-    if blck.parent.isNil:
-      return nil
-
-    doAssert depth < maxDepth
-    depth += 1
-
-    blck = blck.parent
-
 iterator get_ancestors_in_epoch(blockSlot: BlockSlot): BlockSlot =
   let min_slot =
     blockSlot.slot.compute_epoch_at_slot.compute_start_slot_at_epoch
@@ -162,7 +141,7 @@ func atSlot*(blck: BlockRef, slot: Slot): BlockSlot =
   ## particular moment in time, or when imagining what it will look like in the
   ## near future if nothing happens (such as when looking ahead for the next
   ## block proposal)
-  BlockSlot(blck: blck.getAncestorAt(slot), slot: slot)
+  BlockSlot(blck: blck.get_ancestor(slot), slot: slot)
 
 func atEpochStart*(blck: BlockRef, epoch: Epoch): BlockSlot =
   ## Return the BlockSlot corresponding to the first slot in the given epoch

beacon_chain/block_pools/clearance.nim

@@ -253,7 +253,7 @@ proc addRawBlock*(
 
   return err MissingParent
 
-# https://github.com/ethereum/eth2.0-specs/blob/v0.11.1/specs/phase0/p2p-interface.md#global-topics
+# https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/p2p-interface.md#beacon_block
 proc isValidBeaconBlock*(
        dag: ChainDAGRef, quarantine: var QuarantineRef,
        signed_beacon_block: SignedBeaconBlock, current_slot: Slot,
@@ -277,16 +277,15 @@ proc isValidBeaconBlock*(
       current_slot
     return err(Invalid)
 
-  # The block is from a slot greater than the latest finalized slot (with a
+  # [IGNORE] The block is from a slot greater than the latest finalized slot --
-  # MAXIMUM_GOSSIP_CLOCK_DISPARITY allowance) -- i.e. validate that
+  # i.e. validate that signed_beacon_block.message.slot >
-  # signed_beacon_block.message.slot >
   # compute_start_slot_at_epoch(state.finalized_checkpoint.epoch)
   if not (signed_beacon_block.message.slot > dag.finalizedHead.slot):
     debug "block is not from a slot greater than the latest finalized slot"
     return err(Invalid)
 
-  # The block is the first block with valid signature received for the proposer
+  # [IGNORE] The block is the first block with valid signature received for the
-  # for the slot, signed_beacon_block.message.slot.
+  # proposer for the slot, signed_beacon_block.message.slot.
   #
   # While this condition is similar to the proposer slashing condition at
   # https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/validator.md#proposer-slashing
@@ -325,19 +324,14 @@ proc isValidBeaconBlock*(
         existing_block = shortLog(blck.message)
       return err(Invalid)
 
-  # If this block doesn't have a parent we know about, we can't/don't really
+  # [IGNORE] The block's parent (defined by block.parent_root) has been seen
-  # trace it back to a known-good state/checkpoint to verify its prevenance;
+  # (via both gossip and non-gossip sources) (a client MAY queue blocks for
-  # while one could getOrResolve to queue up searching for missing parent it
+  # processing once the parent block is retrieved).
-  # might not be the best place. As much as feasible, this function aims for
+  #
-  # answering yes/no, not queuing other action or otherwise altering state.
+  # And implicitly:
+  # [REJECT] The block's parent (defined by block.parent_root) passes validation.
   let parent_ref = dag.getRef(signed_beacon_block.message.parent_root)
   if parent_ref.isNil:
-    # This doesn't mean a block is forever invalid, only that we haven't seen
-    # its ancestor blocks yet. While that means for now it should be blocked,
-    # at least, from libp2p propagation, it shouldn't be ignored. TODO, if in
-    # the future this block moves from pending to being resolved, consider if
-    # it's worth broadcasting it then.
-
     # Pending dag gets checked via `ChainDAGRef.add(...)` later, and relevant
     # checks are performed there. In usual paths beacon_node adds blocks via
     # ChainDAGRef.add(...) directly, with no additional validity checks. TODO,
@@ -347,8 +341,29 @@ proc isValidBeaconBlock*(
     quarantine.add(dag, signed_beacon_block)
     return err(MissingParent)
 
-  # The proposer signature, signed_beacon_block.signature, is valid with
+  # [REJECT] The current finalized_checkpoint is an ancestor of block -- i.e.
-  # respect to the proposer_index pubkey.
+  # get_ancestor(store, block.parent_root,
+  # compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)) ==
+  # store.finalized_checkpoint.root
+  let
+    finalized_checkpoint = dag.headState.data.data.finalized_checkpoint
+    ancestor = get_ancestor(
+      parent_ref, compute_start_slot_at_epoch(finalized_checkpoint.epoch))
+
+  if ancestor.isNil:
+    debug "couldn't find ancestor block"
+    return err(Invalid)
+
+  if not (finalized_checkpoint.root in [ancestor.root, Eth2Digest()]):
+    debug "block not descendent of finalized block"
+    return err(Invalid)
+
+  # [REJECT] The block is proposed by the expected proposer_index for the
+  # block's slot in the context of the current shuffling (defined by
+  # parent_root/slot). If the proposer_index cannot immediately be verified
+  # against the expected shuffling, the block MAY be queued for later
+  # processing while proposers for the block's branch are calculated -- in such
+  # a case do not REJECT, instead IGNORE this message.
   let
     proposer = getProposer(dag, parent_ref, signed_beacon_block.message.slot)
 
@@ -362,6 +377,8 @@ proc isValidBeaconBlock*(
       expected_proposer = proposer.get()[0]
     return err(Invalid)
 
+  # [REJECT] The proposer signature, signed_beacon_block.signature, is valid
+  # with respect to the proposer_index pubkey.
   if not verify_block_signature(
       dag.headState.data.data.fork,
       dag.headState.data.data.genesis_validators_root,

beacon_chain/spec/beaconstate.nim

@@ -508,6 +508,7 @@ func get_indexed_attestation*(state: BeaconState, attestation: TrustedAttestatio
 # Attestation validation
 # ------------------------------------------------------------------------------------------
 # https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/beacon-chain.md#attestations
+# https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/p2p-interface.md#beacon_attestation_subnet_id
 
 func check_attestation_slot_target*(data: AttestationData): Result[void, cstring] =
   if not (data.target.epoch == compute_epoch_at_slot(data.slot)):

beacon_chain/validator_duties.nim

@@ -498,7 +498,7 @@ proc handleValidatorDuties*(
     const TRAILING_DISTANCE = 1
     let
       aggregationSlot = slot - TRAILING_DISTANCE
-      aggregationHead = getAncestorAt(head, aggregationSlot)
+      aggregationHead = get_ancestor(head, aggregationSlot)
 
     broadcastAggregatedAttestations(
       node, aggregationHead, aggregationSlot, TRAILING_DISTANCE)

tests/test_block_pool.nim

@@ -35,7 +35,7 @@ suiteReport "BlockRef and helpers" & preset():
       not s2.isAncestorOf(s1)
       not s1.isAncestorOf(s0)
 
-  timedTest "getAncestorAt sanity" & preset():
+  timedTest "get_ancestor sanity" & preset():
     let
       s0 = BlockRef(slot: Slot(0))
       s1 = BlockRef(slot: Slot(1), parent: s0)
@@ -43,17 +43,17 @@ suiteReport "BlockRef and helpers" & preset():
       s4 = BlockRef(slot: Slot(4), parent: s2)
 
     check:
-      s0.getAncestorAt(Slot(0)) == s0
+      s0.get_ancestor(Slot(0)) == s0
-      s0.getAncestorAt(Slot(1)) == s0
+      s0.get_ancestor(Slot(1)) == s0
 
-      s1.getAncestorAt(Slot(0)) == s0
+      s1.get_ancestor(Slot(0)) == s0
-      s1.getAncestorAt(Slot(1)) == s1
+      s1.get_ancestor(Slot(1)) == s1
 
-      s4.getAncestorAt(Slot(0)) == s0
+      s4.get_ancestor(Slot(0)) == s0
-      s4.getAncestorAt(Slot(1)) == s1
+      s4.get_ancestor(Slot(1)) == s1
-      s4.getAncestorAt(Slot(2)) == s2
+      s4.get_ancestor(Slot(2)) == s2
-      s4.getAncestorAt(Slot(3)) == s2
+      s4.get_ancestor(Slot(3)) == s2
-      s4.getAncestorAt(Slot(4)) == s4
+      s4.get_ancestor(Slot(4)) == s4
 
 suiteReport "BlockSlot and helpers" & preset():
   timedTest "atSlot sanity" & preset():