fix: added MAXIMUM_GOSSIP_CLOCK_DISPARITY into account to distinguish future slots

lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex

@@ -31,7 +31,7 @@ defmodule LambdaEthereumConsensus.ForkChoice do
 
     store = Handlers.on_tick(store, time)
 
-    :telemetry.execute([:sync, :store], %{slot: Store.get_current_slot(store)})
+    :telemetry.execute([:sync, :store], %{slot: get_current_slot(store)})
     :telemetry.execute([:sync, :on_block], %{slot: head_slot})
 
     Metrics.block_status(head_root, head_slot, :transitioned)
@@ -111,11 +111,37 @@ defmodule LambdaEthereumConsensus.ForkChoice do
     |> tap(&StoreDb.persist_store/1)
   end
 
+  @spec get_current_slot(Types.Store.t()) :: Types.slot()
+  def get_current_slot(%Types.Store{} = store),
+    do: compute_current_slot(store.time, store.genesis_time)
+
+  @doc """
+  Get the current chain slot based on the system time.
+
+  TODO: There are just 2 uses of this function outside this module:
+   - At the begining of SyncBlocks.run/1 function, to get the head slot
+   - In the Helpers.block_root_by_block_id/1 function
+  """
   @spec get_current_chain_slot() :: Types.slot()
-  def get_current_chain_slot() do
-    time = :os.system_time(:second)
-    genesis_time = StoreDb.fetch_genesis_time!()
-    compute_current_slot(time, genesis_time)
+  def get_current_chain_slot(genesis_time \\ StoreDb.fetch_genesis_time!()),
+    do: compute_current_slot(:os.system_time(:second), genesis_time)
+
+  @doc """
+  Check if a slot is in the future with respect to the systems time.
+  """
+  @spec future_slot?(Types.Store.t(), Types.slot()) :: boolean()
+  def future_slot?(%Types.Store{} = store, slot) do
+    if get_current_slot(store) < get_current_chain_slot(store.genesis_time) do
+      # If the store store slot is in the past, we can safely assume that MAXIMUM_GOSSIP_CLOCK_DISPARITY
+      # will not make a difference, store time is updated once every second and disparity is just 500ms.
+      get_current_slot(store) < slot
+    else
+      # If the store slot is not in the past we need to take the actual system time in milliseconds
+      # to calculate the current slot, having in mind the MAXIMUM_GOSSIP_CLOCK_DISPARITY.
+      :os.system_time(:millisecond)
+      |> compute_currents_slots_within_disparity(store.genesis_time)
+      |> Enum.all?(fn possible_slot -> possible_slot < slot end)
+    end
   end
 
   @spec get_finalized_checkpoint() :: Types.Checkpoint.t()
@@ -279,11 +305,7 @@ defmodule LambdaEthereumConsensus.ForkChoice do
 
     Logger.debug("[Fork choice] Updated fork choice cache", slot: slot)
 
-    %{
-      store
-      | head_root: head_root,
-        head_slot: slot
-    }
+    Store.update_head_info(store, slot, head_root)
   end
 
   defp fetch_store!() do
@@ -294,6 +316,16 @@ defmodule LambdaEthereumConsensus.ForkChoice do
   defp compute_current_slot(time, genesis_time),
     do: div(time - genesis_time, ChainSpec.get("SECONDS_PER_SLOT"))
 
+  defp compute_currents_slots_within_disparity(time_ms, genesis_time) do
+    min_time = div(time_ms - ChainSpec.get("MAXIMUM_GOSSIP_CLOCK_DISPARITY"), 1000)
+    max_time = div(time_ms + ChainSpec.get("MAXIMUM_GOSSIP_CLOCK_DISPARITY"), 1000)
+
+    [
+      compute_current_slot(min_time, genesis_time),
+      compute_current_slot(max_time, genesis_time)
+    ]
+  end
+
   defp compute_fork_digest(slot, genesis_validators_root) do
     Misc.compute_epoch_at_slot(slot)
     |> ChainSpec.get_fork_version_for_epoch()

lib/lambda_ethereum_consensus/fork_choice/handlers.ex

@@ -5,6 +5,7 @@ defmodule LambdaEthereumConsensus.ForkChoice.Handlers do
   require Logger
 
   alias LambdaEthereumConsensus.Execution.ExecutionClient
+  alias LambdaEthereumConsensus.ForkChoice
   alias LambdaEthereumConsensus.StateTransition
   alias LambdaEthereumConsensus.StateTransition.Accessors
   alias LambdaEthereumConsensus.StateTransition.EpochProcessing
@@ -38,7 +39,7 @@ defmodule LambdaEthereumConsensus.ForkChoice.Handlers do
     # to ensure that every previous slot is processed with ``on_tick_per_slot``
     seconds_per_slot = ChainSpec.get("SECONDS_PER_SLOT")
     tick_slot = div(time - store.genesis_time, seconds_per_slot)
-    current_slot = Store.get_current_slot(store)
+    current_slot = ForkChoice.get_current_slot(store)
     next_slot_start = (current_slot + 1) * seconds_per_slot
     last_slot_start = tick_slot * seconds_per_slot
 
@@ -69,7 +70,7 @@ defmodule LambdaEthereumConsensus.ForkChoice.Handlers do
 
       # Blocks cannot be in the future. If they are, their
       # consideration must be delayed until they are in the past.
-      Store.get_current_slot(store) < block.slot ->
+      ForkChoice.future_slot?(store, block.slot) ->
         # TODO: handle this error somehow
         {:error, "block is from the future"}
 
@@ -235,8 +236,10 @@ defmodule LambdaEthereumConsensus.ForkChoice.Handlers do
       )
 
       is_first_block = new_store.proposer_boost_root == <<0::256>>
+
       # TODO: store block timeliness data?
-      is_timely = Store.get_current_slot(new_store) == block.slot and is_before_attesting_interval
+      is_timely =
+        ForkChoice.get_current_slot(new_store) == block.slot and is_before_attesting_interval
 
       state = new_state_info.beacon_state
 
@@ -283,12 +286,13 @@ defmodule LambdaEthereumConsensus.ForkChoice.Handlers do
   end
 
   defp on_tick_per_slot(%Store{} = store, time) do
-    previous_slot = Store.get_current_slot(store)
+    previous_slot = ForkChoice.get_current_slot(store)
 
     # Update store time
     store = %Store{store | time: time}
 
-    current_slot = Store.get_current_slot(store)
+    # Why is this needed? the previous line shoud be immediate.
+    current_slot = ForkChoice.get_current_slot(store)
 
     store
     # If this is a new slot, reset store.proposer_boost_root
@@ -394,10 +398,10 @@ defmodule LambdaEthereumConsensus.ForkChoice.Handlers do
       target.root != Store.get_checkpoint_block(store, block_root, target.epoch) ->
         {:error, "mismatched head and target blocks"}
 
-      # Attestations can only affect the fork choice of subsequent slots.
+      # Attestations can only affect the fork choice of subsequent slots (that's why the - 1).
       # Delay consideration in the fork choice until their slot is in the past.
       # TODO: delay consideration
-      Store.get_current_slot(store) <= attestation.data.slot ->
+      ForkChoice.future_slot?(store, attestation.data.slot - 1) ->
         {:error, "attestation is for a future slot"}
 
       true ->

lib/lambda_ethereum_consensus/p2p/gossip/beacon_block.ex

@@ -17,11 +17,9 @@ defmodule LambdaEthereumConsensus.P2P.Gossip.BeaconBlock do
 
   @impl true
   def handle_gossip_message(store, _topic, msg_id, message) do
-    slot = ForkChoice.get_current_chain_slot()
-
     with {:ok, uncompressed} <- :snappyer.decompress(message),
          {:ok, signed_block} <- Ssz.from_ssz(uncompressed, SignedBeaconBlock),
-         :ok <- validate(signed_block, slot) do
+         :ok <- validate(signed_block, store) do
       Logger.info("[Gossip] Block received, block.slot: #{signed_block.message.slot}.")
       Libp2pPort.validate_message(msg_id, :accept)
       PendingBlocks.add_block(store, signed_block)
@@ -63,8 +61,9 @@ defmodule LambdaEthereumConsensus.P2P.Gossip.BeaconBlock do
   ### Private functions
   ##########################
 
-  @spec validate(SignedBeaconBlock.t(), Types.slot()) :: :ok | {:ignore, String.t()}
-  defp validate(%SignedBeaconBlock{message: block}, current_slot) do
+  @spec validate(SignedBeaconBlock.t(), Types.Store.t()) :: :ok | {:ignore, String.t()}
+  defp validate(%SignedBeaconBlock{message: block}, store) do
+    current_slot = ForkChoice.get_current_slot(store)
     min_slot = current_slot - ChainSpec.get("SLOTS_PER_EPOCH")
 
     cond do
@@ -73,9 +72,9 @@ defmodule LambdaEthereumConsensus.P2P.Gossip.BeaconBlock do
         {:ignore,
          "Block too old: block.slot=#{block.slot}. Current slot: #{current_slot}. Minimum expected slot: #{min_slot}"}
 
-      block.slot > current_slot ->
+      ForkChoice.future_slot?(store, block.slot) ->
         {:ignore,
-         "Block is from the future: block.slot=#{block.slot}. Current slot: #{current_slot}."}
+         "Block is from the future: block.slot=#{block.slot}. Current store calculated slot: #{current_slot}."}
 
       true ->
         :ok

lib/types/store.ex

@@ -3,6 +3,7 @@ defmodule Types.Store do
     The Store struct is used to track information required for the fork choice algorithm.
   """
 
+  alias LambdaEthereumConsensus.ForkChoice
   alias LambdaEthereumConsensus.ForkChoice.Head
   alias LambdaEthereumConsensus.ForkChoice.Simple.Tree
   alias LambdaEthereumConsensus.StateTransition
@@ -110,13 +111,9 @@ defmodule Types.Store do
     end
   end
 
-  def get_current_slot(%__MODULE__{time: time, genesis_time: genesis_time}) do
-    # NOTE: this assumes GENESIS_SLOT == 0
-    div(time - genesis_time, ChainSpec.get("SECONDS_PER_SLOT"))
-  end
-
+  # We probably want to move this to a more appropriate module
   def get_current_epoch(store) do
-    store |> get_current_slot() |> Misc.compute_epoch_at_slot()
+    store |> ForkChoice.get_current_slot() |> Misc.compute_epoch_at_slot()
   end
 
   def get_ancestor(%__MODULE__{} = store, root, slot) do
@@ -242,9 +239,15 @@ defmodule Types.Store do
     end
   end
 
-  defp update_head_info(store) do
+  @spec update_head_info(t()) :: t()
+  def update_head_info(store) do
     {:ok, head_root} = Head.get_head(store)
     %{slot: head_slot} = Blocks.get_block!(head_root)
+    update_head_info(store, head_slot, head_root)
+  end
+
+  @spec update_head_info(t(), Types.slot(), Types.root()) :: t()
+  def update_head_info(store, head_slot, head_root) do
     %{store | head_root: head_root, head_slot: head_slot}
   end