From 4259b29b837264bb8b055035f6c7a629051e69c5 Mon Sep 17 00:00:00 2001 From: Rodrigo Oliveri Date: Tue, 1 Oct 2024 17:08:25 -0300 Subject: [PATCH 1/6] Initial fix --- .../fork_choice/fork_choice.ex | 39 ++++++++++++++++--- .../fork_choice/handlers.ex | 18 +++++---- .../p2p/gossip/beacon_block.ex | 11 +++--- lib/lambda_ethereum_consensus/p2p/peerbook.ex | 9 +++-- lib/types/store.ex | 17 ++++---- 5 files changed, 64 insertions(+), 30 deletions(-) diff --git a/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex b/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex index ad0ee36ad..2603e609f 100644 --- a/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex +++ b/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,6 +111,17 @@ 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) + + @spec get_current_slot(Types.uint64(), Types.uint64()) :: Types.slot() + def get_current_slot(time, genesis_time), + do: compute_current_slot(time, genesis_time) + + # TODO: Some parts of the code calculate the current slot using the previous function given a time + # specifically from the store (this was previously in the Store type module). This one calculates + # it using the system time, we might need to unify. @spec get_current_chain_slot() :: Types.slot() def get_current_chain_slot() do time = :os.system_time(:second) @@ -118,6 +129,19 @@ defmodule LambdaEthereumConsensus.ForkChoice do compute_current_slot(time, genesis_time) end + @doc """ + Check if a slot is in the future with respect to the current time. + """ + @spec future_slot?(Types.slot()) :: boolean() + def future_slot?(slot) do + time = :os.system_time(:second) + genesis_time = StoreDb.fetch_genesis_time!() + + time + |> compute_currents_slots_within_disparity(genesis_time) + |> Enum.all?(fn possible_slot -> possible_slot < slot end) + end + @spec get_finalized_checkpoint() :: Types.Checkpoint.t() def get_finalized_checkpoint() do %{finalized_checkpoint: finalized} = fetch_store!() @@ -279,11 +303,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 +314,13 @@ 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, genesis_time) do + [ + compute_current_slot(time - ChainSpec.get("MAXIMUM_GOSSIP_CLOCK_DISPARITY"), genesis_time), + compute_current_slot(time + ChainSpec.get("MAXIMUM_GOSSIP_CLOCK_DISPARITY"), genesis_time) + ] + end + defp compute_fork_digest(slot, genesis_validators_root) do Misc.compute_epoch_at_slot(slot) |> ChainSpec.get_fork_version_for_epoch() diff --git a/lib/lambda_ethereum_consensus/fork_choice/handlers.ex b/lib/lambda_ethereum_consensus/fork_choice/handlers.ex index 275f8c7df..d35237a31 100644 --- a/lib/lambda_ethereum_consensus/fork_choice/handlers.ex +++ b/lib/lambda_ethereum_consensus/fork_choice/handlers.ex @@ -4,6 +4,7 @@ defmodule LambdaEthereumConsensus.ForkChoice.Handlers do """ require Logger + alias LambdaEthereumConsensus.ForkChoice alias LambdaEthereumConsensus.Execution.ExecutionClient alias LambdaEthereumConsensus.StateTransition alias LambdaEthereumConsensus.StateTransition.Accessors @@ -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?(block.slot) -> # TODO: handle this error somehow {:error, "block is from the future"} @@ -235,8 +236,8 @@ 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 + # TODO: store block timeliness data? we might need to take MAXIMUM_GOSSIP_CLOCK_DISPARITY into account + is_timely = ForkChoice.get_current_slot(new_store) == block.slot and is_before_attesting_interval state = new_state_info.beacon_state @@ -283,12 +284,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 +396,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?(attestation.data.slot - 1) -> {:error, "attestation is for a future slot"} true -> diff --git a/lib/lambda_ethereum_consensus/p2p/gossip/beacon_block.ex b/lib/lambda_ethereum_consensus/p2p/gossip/beacon_block.ex index a1accbab1..b66414de5 100644 --- a/lib/lambda_ethereum_consensus/p2p/gossip/beacon_block.ex +++ b/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) 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()) :: :ok | {:ignore, String.t()} + defp validate(%SignedBeaconBlock{message: block}) do + current_slot = ForkChoice.get_current_chain_slot() min_slot = current_slot - ChainSpec.get("SLOTS_PER_EPOCH") cond do @@ -73,7 +72,7 @@ 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?(block.slot) -> {:ignore, "Block is from the future: block.slot=#{block.slot}. Current slot: #{current_slot}."} diff --git a/lib/lambda_ethereum_consensus/p2p/peerbook.ex b/lib/lambda_ethereum_consensus/p2p/peerbook.ex index 96b9c34a9..e5b21fca8 100644 --- a/lib/lambda_ethereum_consensus/p2p/peerbook.ex +++ b/lib/lambda_ethereum_consensus/p2p/peerbook.ex @@ -41,14 +41,17 @@ defmodule LambdaEthereumConsensus.P2P.Peerbook do Get some peer from the peerbook. """ def get_some_peer() do - # TODO: use some algorithm to pick a good peer, for now it's random + # TODO: This is a very naive implementation of a peer selection algorithm. peerbook = fetch_peerbook!() if peerbook == %{} do nil else - {peer_id, _score} = Enum.random(peerbook) - peer_id + peerbook + |> Enum.sort_by(fn {_peer_id, score} -> score end) + |> Enum.take(4) + |> Enum.random() + |> elem(0) end end diff --git a/lib/types/store.ex b/lib/types/store.ex index 736684034..3709a4b7b 100644 --- a/lib/types/store.ex +++ b/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 From 4423088390055be498a474c67fd306583136f2a8 Mon Sep 17 00:00:00 2001 From: Rodrigo Oliveri Date: Wed, 2 Oct 2024 12:01:23 -0300 Subject: [PATCH 2/6] Last fixes and format --- .../fork_choice/fork_choice.ex | 31 ++++++++++--------- .../fork_choice/handlers.ex | 12 ++++--- .../p2p/gossip/beacon_block.ex | 2 +- lib/types/store.ex | 2 +- 4 files changed, 26 insertions(+), 21 deletions(-) diff --git a/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex b/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex index 2603e609f..48b3ac7f5 100644 --- a/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex +++ b/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex @@ -119,9 +119,12 @@ defmodule LambdaEthereumConsensus.ForkChoice do def get_current_slot(time, genesis_time), do: compute_current_slot(time, genesis_time) - # TODO: Some parts of the code calculate the current slot using the previous function given a time - # specifically from the store (this was previously in the Store type module). This one calculates - # it using the system time, we might need to unify. + # TODO: Some parts of the node calculate the current slot using the previous function given a time + # specifically from the store (this was previously in the Store type module). The following function + # calculates it using the system time, we might need to make sure that each case makes sense. + @doc """ + Get the current chain slot based on the system time. + """ @spec get_current_chain_slot() :: Types.slot() def get_current_chain_slot() do time = :os.system_time(:second) @@ -130,15 +133,12 @@ defmodule LambdaEthereumConsensus.ForkChoice do end @doc """ - Check if a slot is in the future with respect to the current time. + Check if a slot is in the future with respect to the systems time. """ - @spec future_slot?(Types.slot()) :: boolean() - def future_slot?(slot) do - time = :os.system_time(:second) - genesis_time = StoreDb.fetch_genesis_time!() - - time - |> compute_currents_slots_within_disparity(genesis_time) + @spec future_chain_slot?(Types.slot()) :: boolean() + def future_chain_slot?(slot) do + :os.system_time(:millisecond) + |> compute_currents_slots_within_disparity(store.genesis_time) |> Enum.all?(fn possible_slot -> possible_slot < slot end) end @@ -314,10 +314,13 @@ 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, genesis_time) do + 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(time - ChainSpec.get("MAXIMUM_GOSSIP_CLOCK_DISPARITY"), genesis_time), - compute_current_slot(time + ChainSpec.get("MAXIMUM_GOSSIP_CLOCK_DISPARITY"), genesis_time) + compute_current_slot(min_time, genesis_time), + compute_current_slot(max_time, genesis_time) ] end diff --git a/lib/lambda_ethereum_consensus/fork_choice/handlers.ex b/lib/lambda_ethereum_consensus/fork_choice/handlers.ex index d35237a31..94e5e6edb 100644 --- a/lib/lambda_ethereum_consensus/fork_choice/handlers.ex +++ b/lib/lambda_ethereum_consensus/fork_choice/handlers.ex @@ -4,8 +4,8 @@ defmodule LambdaEthereumConsensus.ForkChoice.Handlers do """ require Logger - alias LambdaEthereumConsensus.ForkChoice alias LambdaEthereumConsensus.Execution.ExecutionClient + alias LambdaEthereumConsensus.ForkChoice alias LambdaEthereumConsensus.StateTransition alias LambdaEthereumConsensus.StateTransition.Accessors alias LambdaEthereumConsensus.StateTransition.EpochProcessing @@ -70,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. - ForkChoice.future_slot?(block.slot) -> + ForkChoice.future_chain_slot?(block.slot) -> # TODO: handle this error somehow {:error, "block is from the future"} @@ -236,8 +236,10 @@ defmodule LambdaEthereumConsensus.ForkChoice.Handlers do ) is_first_block = new_store.proposer_boost_root == <<0::256>> - # TODO: store block timeliness data? we might need to take MAXIMUM_GOSSIP_CLOCK_DISPARITY into account - is_timely = ForkChoice.get_current_slot(new_store) == block.slot and is_before_attesting_interval + + # TODO: store block timeliness data? + is_timely = + ForkChoice.get_current_slot(new_store) == block.slot and is_before_attesting_interval state = new_state_info.beacon_state @@ -399,7 +401,7 @@ defmodule LambdaEthereumConsensus.ForkChoice.Handlers do # 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 - ForkChoice.future_slot?(attestation.data.slot - 1) -> + ForkChoice.future_chain_slot?(attestation.data.slot - 1) -> {:error, "attestation is for a future slot"} true -> diff --git a/lib/lambda_ethereum_consensus/p2p/gossip/beacon_block.ex b/lib/lambda_ethereum_consensus/p2p/gossip/beacon_block.ex index b66414de5..1d156fac2 100644 --- a/lib/lambda_ethereum_consensus/p2p/gossip/beacon_block.ex +++ b/lib/lambda_ethereum_consensus/p2p/gossip/beacon_block.ex @@ -72,7 +72,7 @@ defmodule LambdaEthereumConsensus.P2P.Gossip.BeaconBlock do {:ignore, "Block too old: block.slot=#{block.slot}. Current slot: #{current_slot}. Minimum expected slot: #{min_slot}"} - ForkChoice.future_slot?(block.slot) -> + ForkChoice.future_chain_slot?(block.slot) -> {:ignore, "Block is from the future: block.slot=#{block.slot}. Current slot: #{current_slot}."} diff --git a/lib/types/store.ex b/lib/types/store.ex index 3709a4b7b..d0bf876ba 100644 --- a/lib/types/store.ex +++ b/lib/types/store.ex @@ -243,7 +243,7 @@ defmodule Types.Store do 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) + update_head_info(store, head_slot, head_root) end @spec update_head_info(t(), Types.slot(), Types.root()) :: t() From 1fb23ba3860303e844359d627ed1ec823b9186ab Mon Sep 17 00:00:00 2001 From: Rodrigo Oliveri Date: Wed, 2 Oct 2024 14:36:19 -0300 Subject: [PATCH 3/6] Fixed a typo --- lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex b/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex index 48b3ac7f5..89ec74c64 100644 --- a/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex +++ b/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex @@ -137,8 +137,11 @@ defmodule LambdaEthereumConsensus.ForkChoice do """ @spec future_chain_slot?(Types.slot()) :: boolean() def future_chain_slot?(slot) do - :os.system_time(:millisecond) - |> compute_currents_slots_within_disparity(store.genesis_time) + time_ms = :os.system_time(:millisecond) + genesis_time = StoreDb.fetch_genesis_time!() + + time_ms + |> compute_currents_slots_within_disparity(genesis_time) |> Enum.all?(fn possible_slot -> possible_slot < slot end) end From 36e979301d3862d6484c182e350abc4b672adc05 Mon Sep 17 00:00:00 2001 From: Rodrigo Oliveri Date: Wed, 2 Oct 2024 15:07:08 -0300 Subject: [PATCH 4/6] Spec test now passing --- .../fork_choice/fork_choice.ex | 14 ++++++-------- .../fork_choice/handlers.ex | 4 ++-- .../p2p/gossip/beacon_block.ex | 8 ++++---- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex b/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex index 89ec74c64..4eb2e448a 100644 --- a/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex +++ b/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex @@ -115,10 +115,6 @@ defmodule LambdaEthereumConsensus.ForkChoice do def get_current_slot(%Types.Store{} = store), do: compute_current_slot(store.time, store.genesis_time) - @spec get_current_slot(Types.uint64(), Types.uint64()) :: Types.slot() - def get_current_slot(time, genesis_time), - do: compute_current_slot(time, genesis_time) - # TODO: Some parts of the node calculate the current slot using the previous function given a time # specifically from the store (this was previously in the Store type module). The following function # calculates it using the system time, we might need to make sure that each case makes sense. @@ -135,13 +131,15 @@ defmodule LambdaEthereumConsensus.ForkChoice do @doc """ Check if a slot is in the future with respect to the systems time. """ - @spec future_chain_slot?(Types.slot()) :: boolean() - def future_chain_slot?(slot) do + @spec future_slot?(Types.Store.t(), Types.slot()) :: boolean() + def future_slot?(%Types.Store{} = store, slot) do + # Due to ticks being every 1000ms, we need to use the system time instead of + # the store time to account for the 500ms of MAXIMUM_GOSSIP_CLOCK_DISPARITY. + # the time in the store will always be a whole second. time_ms = :os.system_time(:millisecond) - genesis_time = StoreDb.fetch_genesis_time!() time_ms - |> compute_currents_slots_within_disparity(genesis_time) + |> compute_currents_slots_within_disparity(store.genesis_time) |> Enum.all?(fn possible_slot -> possible_slot < slot end) end diff --git a/lib/lambda_ethereum_consensus/fork_choice/handlers.ex b/lib/lambda_ethereum_consensus/fork_choice/handlers.ex index 94e5e6edb..6384ed48a 100644 --- a/lib/lambda_ethereum_consensus/fork_choice/handlers.ex +++ b/lib/lambda_ethereum_consensus/fork_choice/handlers.ex @@ -70,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. - ForkChoice.future_chain_slot?(block.slot) -> + ForkChoice.future_slot?(store, block.slot) -> # TODO: handle this error somehow {:error, "block is from the future"} @@ -401,7 +401,7 @@ defmodule LambdaEthereumConsensus.ForkChoice.Handlers do # 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 - ForkChoice.future_chain_slot?(attestation.data.slot - 1) -> + ForkChoice.future_slot?(store, attestation.data.slot - 1) -> {:error, "attestation is for a future slot"} true -> diff --git a/lib/lambda_ethereum_consensus/p2p/gossip/beacon_block.ex b/lib/lambda_ethereum_consensus/p2p/gossip/beacon_block.ex index 1d156fac2..3ce2b8ab3 100644 --- a/lib/lambda_ethereum_consensus/p2p/gossip/beacon_block.ex +++ b/lib/lambda_ethereum_consensus/p2p/gossip/beacon_block.ex @@ -19,7 +19,7 @@ defmodule LambdaEthereumConsensus.P2P.Gossip.BeaconBlock do def handle_gossip_message(store, _topic, msg_id, message) do with {:ok, uncompressed} <- :snappyer.decompress(message), {:ok, signed_block} <- Ssz.from_ssz(uncompressed, SignedBeaconBlock), - :ok <- validate(signed_block) 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) @@ -61,8 +61,8 @@ defmodule LambdaEthereumConsensus.P2P.Gossip.BeaconBlock do ### Private functions ########################## - @spec validate(SignedBeaconBlock.t()) :: :ok | {:ignore, String.t()} - defp validate(%SignedBeaconBlock{message: block}) do + @spec validate(SignedBeaconBlock.t(), Types.Store.t()) :: :ok | {:ignore, String.t()} + defp validate(%SignedBeaconBlock{message: block}, store) do current_slot = ForkChoice.get_current_chain_slot() min_slot = current_slot - ChainSpec.get("SLOTS_PER_EPOCH") @@ -72,7 +72,7 @@ defmodule LambdaEthereumConsensus.P2P.Gossip.BeaconBlock do {:ignore, "Block too old: block.slot=#{block.slot}. Current slot: #{current_slot}. Minimum expected slot: #{min_slot}"} - ForkChoice.future_chain_slot?(block.slot) -> + ForkChoice.future_slot?(store, block.slot) -> {:ignore, "Block is from the future: block.slot=#{block.slot}. Current slot: #{current_slot}."} From ff76ce5a4e42ca434ad46de8264569ddeff97bf2 Mon Sep 17 00:00:00 2001 From: Rodrigo Oliveri Date: Wed, 2 Oct 2024 15:52:35 -0300 Subject: [PATCH 5/6] Taking into account when the store is in the past for future slot calculations --- .../fork_choice/fork_choice.ex | 33 ++++++++++--------- .../p2p/gossip/beacon_block.ex | 4 +-- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex b/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex index 4eb2e448a..9caa8ef77 100644 --- a/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex +++ b/lib/lambda_ethereum_consensus/fork_choice/fork_choice.ex @@ -115,32 +115,33 @@ defmodule LambdaEthereumConsensus.ForkChoice do def get_current_slot(%Types.Store{} = store), do: compute_current_slot(store.time, store.genesis_time) - # TODO: Some parts of the node calculate the current slot using the previous function given a time - # specifically from the store (this was previously in the Store type module). The following function - # calculates it using the system time, we might need to make sure that each case makes sense. @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) - end + 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 - # Due to ticks being every 1000ms, we need to use the system time instead of - # the store time to account for the 500ms of MAXIMUM_GOSSIP_CLOCK_DISPARITY. - # the time in the store will always be a whole second. - time_ms = :os.system_time(:millisecond) - - time_ms - |> compute_currents_slots_within_disparity(store.genesis_time) - |> Enum.all?(fn possible_slot -> possible_slot < slot end) + 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() diff --git a/lib/lambda_ethereum_consensus/p2p/gossip/beacon_block.ex b/lib/lambda_ethereum_consensus/p2p/gossip/beacon_block.ex index 3ce2b8ab3..ee30d7422 100644 --- a/lib/lambda_ethereum_consensus/p2p/gossip/beacon_block.ex +++ b/lib/lambda_ethereum_consensus/p2p/gossip/beacon_block.ex @@ -63,7 +63,7 @@ defmodule LambdaEthereumConsensus.P2P.Gossip.BeaconBlock do @spec validate(SignedBeaconBlock.t(), Types.Store.t()) :: :ok | {:ignore, String.t()} defp validate(%SignedBeaconBlock{message: block}, store) do - current_slot = ForkChoice.get_current_chain_slot() + current_slot = ForkChoice.get_current_slot(store) min_slot = current_slot - ChainSpec.get("SLOTS_PER_EPOCH") cond do @@ -74,7 +74,7 @@ defmodule LambdaEthereumConsensus.P2P.Gossip.BeaconBlock do 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 From ac973e242389eea7cf4daa6244771880d29f55e1 Mon Sep 17 00:00:00 2001 From: Rodrigo Oliveri Date: Wed, 2 Oct 2024 16:16:25 -0300 Subject: [PATCH 6/6] remove unintended diff --- lib/lambda_ethereum_consensus/p2p/peerbook.ex | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/lambda_ethereum_consensus/p2p/peerbook.ex b/lib/lambda_ethereum_consensus/p2p/peerbook.ex index e5b21fca8..96b9c34a9 100644 --- a/lib/lambda_ethereum_consensus/p2p/peerbook.ex +++ b/lib/lambda_ethereum_consensus/p2p/peerbook.ex @@ -41,17 +41,14 @@ defmodule LambdaEthereumConsensus.P2P.Peerbook do Get some peer from the peerbook. """ def get_some_peer() do - # TODO: This is a very naive implementation of a peer selection algorithm. + # TODO: use some algorithm to pick a good peer, for now it's random peerbook = fetch_peerbook!() if peerbook == %{} do nil else - peerbook - |> Enum.sort_by(fn {_peer_id, score} -> score end) - |> Enum.take(4) - |> Enum.random() - |> elem(0) + {peer_id, _score} = Enum.random(peerbook) + peer_id end end