From 19c51e59001eb5d958cf02cd6158287d5cb79f27 Mon Sep 17 00:00:00 2001
From: tersec <tersec@users.noreply.github.com>
Date: Wed, 28 Oct 2020 13:04:21 +0000
Subject: [PATCH] use MAXIMUM_GOSSIP_CLOCK_DISPARITY for BeaconBlock
 verification (#1918)

---
 beacon_chain/attestation_aggregation.nim |  3 ---
 beacon_chain/block_pools/clearance.nim   | 20 ++++++++++----------
 beacon_chain/eth2_processor.nim          |  2 +-
 beacon_chain/spec/datatypes.nim          |  3 ++-
 4 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/beacon_chain/attestation_aggregation.nim b/beacon_chain/attestation_aggregation.nim
index 8243ca3b5..3eea893c9 100644
--- a/beacon_chain/attestation_aggregation.nim
+++ b/beacon_chain/attestation_aggregation.nim
@@ -18,9 +18,6 @@ import
 logScope:
   topics = "att_aggr"
 
-const
-  MAXIMUM_GOSSIP_CLOCK_DISPARITY = 500.millis
-
 # https://github.com/ethereum/eth2.0-specs/blob/v1.0.0-rc.0/specs/phase0/validator.md#aggregation-selection
 func is_aggregator*(committee_len: uint64, slot_signature: ValidatorSig): bool =
   let
diff --git a/beacon_chain/block_pools/clearance.nim b/beacon_chain/block_pools/clearance.nim
index 42bd8dc2f..d0dc71905 100644
--- a/beacon_chain/block_pools/clearance.nim
+++ b/beacon_chain/block_pools/clearance.nim
@@ -11,7 +11,7 @@ import
   std/tables,
   chronicles,
   metrics, stew/results,
-  ../extras,
+  ../extras, ../time,
   ../spec/[crypto, datatypes, digest, helpers, signatures, state_transition],
   ./block_pools_types, ./chain_dag, ./quarantine
 
@@ -265,7 +265,7 @@ proc addRawBlock*(
 # https://github.com/ethereum/eth2.0-specs/blob/v1.0.0-rc.0/specs/phase0/p2p-interface.md#beacon_block
 proc isValidBeaconBlock*(
        dag: ChainDAGRef, quarantine: var QuarantineRef,
-       signed_beacon_block: SignedBeaconBlock, current_slot: Slot,
+       signed_beacon_block: SignedBeaconBlock, wallTime: BeaconTime,
        flags: UpdateFlags):
        Result[void, (ValidationResult, BlockError)] =
   logScope:
@@ -277,14 +277,14 @@ proc isValidBeaconBlock*(
   # verification could be quite a bit more expensive than the rest. This is an
   # externally easy-to-invoke function by tossing network packets at the node.
 
-  # The block is not from a future slot
-  # TODO allow `MAXIMUM_GOSSIP_CLOCK_DISPARITY` leniency, especially towards
-  # seemingly future slots.
-  # TODO using +1 here while this is being sorted - should queue these until
-  #      they're within the DISPARITY limit
-  if not (signed_beacon_block.message.slot <= current_slot + 1):
+  # [IGNORE] The block is not from a future slot (with a
+  # MAXIMUM_GOSSIP_CLOCK_DISPARITY allowance) -- i.e. validate that
+  # signed_beacon_block.message.slot <= current_slot (a client MAY queue future
+  # blocks for processing at the appropriate slot).
+  if not (signed_beacon_block.message.slot <=
+      (wallTime + MAXIMUM_GOSSIP_CLOCK_DISPARITY).slotOrZero):
     debug "block is from a future slot",
-      current_slot
+      wallSlot = wallTime.toSlot()
     return err((ValidationResult.Ignore, Invalid))
 
   # [IGNORE] The block is from a slot greater than the latest finalized slot --
@@ -346,7 +346,7 @@ proc isValidBeaconBlock*(
     # checks are performed there. In usual paths beacon_node adds blocks via
     # ChainDAGRef.add(...) directly, with no additional validity checks.
     debug "parent unknown, putting block in quarantine",
-      current_slot = shortLog(current_slot)
+      current_slot = wallTime.toSlot()
     if not quarantine.add(dag, signed_beacon_block):
       warn "Block quarantine full"
     return err((ValidationResult.Ignore, MissingParent))
diff --git a/beacon_chain/eth2_processor.nim b/beacon_chain/eth2_processor.nim
index 83679ed82..7f247e2b8 100644
--- a/beacon_chain/eth2_processor.nim
+++ b/beacon_chain/eth2_processor.nim
@@ -263,7 +263,7 @@ proc blockValidator*(
   debug "Block received", delay
 
   let blck = self.chainDag.isValidBeaconBlock(
-    self.quarantine, signedBlock, wallSlot, {})
+    self.quarantine, signedBlock, wallTime, {})
 
   self.dumpBlock(signedBlock, blck)
 
diff --git a/beacon_chain/spec/datatypes.nim b/beacon_chain/spec/datatypes.nim
index aab793e07..6065005e8 100644
--- a/beacon_chain/spec/datatypes.nim
+++ b/beacon_chain/spec/datatypes.nim
@@ -71,8 +71,9 @@ const
     # TODO: This needs revisiting.
     # Why was the validator WITHDRAWAL_PERIOD altered in the spec?
 
-  # https://github.com/ethereum/eth2.0-specs/blob/v0.12.3/specs/phase0/p2p-interface.md#configuration
+  # https://github.com/ethereum/eth2.0-specs/blob/v1.0.0-rc.0/specs/phase0/p2p-interface.md#configuration
   ATTESTATION_PROPAGATION_SLOT_RANGE* = 32
+  MAXIMUM_GOSSIP_CLOCK_DISPARITY* = 500.millis
 
   SLOTS_PER_ETH1_VOTING_PERIOD* =
     EPOCHS_PER_ETH1_VOTING_PERIOD * SLOTS_PER_EPOCH