From c5eee883bc7defe23a4aad5659e1e16f4e847f39 Mon Sep 17 00:00:00 2001 From: Rodrigo Oliveri Date: Tue, 24 Sep 2024 18:36:47 -0300 Subject: [PATCH 1/6] Initial tests of fixing the ETh1data default for tiebreakers --- .../execution/execution_chain.ex | 70 +++++++++++++------ .../validator/block_builder.ex | 2 +- 2 files changed, 49 insertions(+), 23 deletions(-) diff --git a/lib/lambda_ethereum_consensus/execution/execution_chain.ex b/lib/lambda_ethereum_consensus/execution/execution_chain.ex index 52fc7b376..10bcf8910 100644 --- a/lib/lambda_ethereum_consensus/execution/execution_chain.ex +++ b/lib/lambda_ethereum_consensus/execution/execution_chain.ex @@ -138,7 +138,7 @@ defmodule LambdaEthereumConsensus.Execution.ExecutionChain do if has_majority?(eth1_data_votes, eth1_data) do case update_deposit_tree(new_state, eth1_data) do - {:ok, new_tree} -> %{state | deposit_tree: new_tree, current_eth1_data: eth1_data} + {:ok, new_tree} -> %{new_state | deposit_tree: new_tree, current_eth1_data: eth1_data} _ -> new_state end else @@ -193,16 +193,18 @@ defmodule LambdaEthereumConsensus.Execution.ExecutionChain do %{ eth1_chain: eth1_chain, eth1_data_votes: seen_votes, - deposit_tree: deposit_tree - }, + deposit_tree: deposit_tree, + current_eth1_data: default + } = state, slot ) do + Logger.info("Computing eth1 vote for slot #{slot}") + IO.inspect(state, label: "state", limit: :infinity, pretty: true) period_start = voting_period_start_time(slot) - follow_time = ChainSpec.get("SECONDS_PER_ETH1_BLOCK") * ChainSpec.get("ETH1_FOLLOW_DISTANCE") blocks_to_consider = eth1_chain - |> Enum.filter(&candidate_block?(&1.timestamp, period_start, follow_time)) + |> Enum.filter(&candidate_block?(&1.timestamp, period_start)) |> Enum.reverse() # TODO: backfill chain @@ -217,43 +219,64 @@ defmodule LambdaEthereumConsensus.Execution.ExecutionChain do # TODO: fetch asynchronously with {:ok, new_deposits} <- ExecutionClient.get_deposit_logs(block_number_min..block_number_max) do - get_first_valid_vote(blocks_to_consider, seen_votes, deposit_tree, new_deposits) + get_first_valid_vote(blocks_to_consider, seen_votes, deposit_tree, new_deposits, default) end end end - defp get_first_valid_vote(blocks_to_consider, seen_votes, deposit_tree, new_deposits) do + defp get_first_valid_vote(blocks_to_consider, seen_votes, deposit_tree, new_deposits, default) do + Logger.info("Processing new deposits and get first valid vote") grouped_deposits = Enum.group_by(new_deposits, &Map.fetch!(&1, :block_number)) - {valid_votes, _last_tree} = + {valid_votes, _last_tree, last_eth1_data} = blocks_to_consider - |> Enum.reduce({MapSet.new(), deposit_tree}, fn block, {set, tree} -> + |> Enum.reduce({MapSet.new(), deposit_tree, nil}, fn block, {set, tree, last_eth1_data} -> + IO.inspect(block.block_number, label: "block_number in get_first_valid_vote") new_tree = case grouped_deposits[block.block_number] do nil -> tree deposits -> update_tree_with_deposits(tree, deposits) end - data = %Eth1Data{ - deposit_root: DepositTree.get_root(new_tree), - deposit_count: DepositTree.get_deposit_count(new_tree), - block_hash: block.block_hash - } + data = get_eth1_data(block, new_tree) |> IO.inspect(label: "data in get_first_valid_vote") - {MapSet.put(set, data), new_tree} + if data.deposit_count >= default.deposit_count, + do: {MapSet.put(set, data), new_tree, data}, + else: {set, new_tree, last_eth1_data} end) + IO.inspect(valid_votes, label: "valid_votes", limit: :infinity, pretty: true) + IO.inspect(seen_votes, label: "seen_votes", limit: :infinity, pretty: true) + + # Default vote on latest eth1 block data in the period range unless eth1 chain is not live + default_vote = last_eth1_data || default |> IO.inspect(label: "default_vote") + # Tiebreak by smallest distance to period start result = seen_votes - |> Stream.filter(&MapSet.member?(valid_votes, &1)) - |> Enum.max(fn {_, count1}, {_, count2} -> count1 >= count2 end, fn -> nil end) + |> Stream.filter(fn {eth1_data, _count_and_distance} -> MapSet.member?(valid_votes, eth1_data) end) + |> tap(&IO.inspect(&1 |> Enum.to_list(), label: "valid_votes_seen", limit: :infinity, pretty: true)) + |> Enum.max(fn {_, {count1, dist1}}, {_, {count2, dist2}} -> + cond do + count1 > count2 -> true + count1 == count2 && dist1 >= dist2 -> true + true -> false + end + end, fn -> nil end) |> IO.inspect(label: "result") case result do - # Use the first vote if there is a tie - nil -> {:ok, List.last(valid_votes)} + nil -> {:ok, default_vote} {eth1_data, _} -> {:ok, eth1_data} end + |> tap(&IO.inspect(LambdaEthereumConsensus.Utils.format_shorten_binary(elem(&1, 1).block_hash), label: "result", pretty: true)) + end + + defp get_eth1_data(block, tree) do + %Eth1Data{ + deposit_root: DepositTree.get_root(tree), + deposit_count: DepositTree.get_deposit_count(tree), + block_hash: block.block_hash + } end defp update_tree_with_deposits(tree, []), do: tree @@ -262,9 +285,12 @@ defmodule LambdaEthereumConsensus.Execution.ExecutionChain do DepositTree.push_leaf(tree, deposit.data) |> update_tree_with_deposits(rest) end - defp candidate_block?(timestamp, period_start, follow_time) do - # follow_time = SECONDS_PER_ETH1_BLOCK * ETH1_FOLLOW_DISTANCE - timestamp in (period_start - follow_time * 2)..(period_start - follow_time) + defp candidate_block?(timestamp, period_start) do + follow_time = ChainSpec.get("SECONDS_PER_ETH1_BLOCK") * 0 # ChainSpec.get("ETH1_FOLLOW_DISTANCE") + IO.inspect("ts: #{timestamp}, period_start: #{period_start}, follow_time: #{follow_time}", label: "candidate_block") + IO.inspect("current result: #{inspect(timestamp + follow_time <= period_start and timestamp + follow_time * 2 >= period_start)}") + IO.inspect("old result: #{inspect(timestamp in (period_start - follow_time * 2)..(period_start - follow_time))}") + timestamp + follow_time <= period_start and timestamp + follow_time * 2 >= period_start end defp voting_period_start_time(slot) do diff --git a/lib/lambda_ethereum_consensus/validator/block_builder.ex b/lib/lambda_ethereum_consensus/validator/block_builder.ex index fc1d5edc7..157ae9582 100644 --- a/lib/lambda_ethereum_consensus/validator/block_builder.ex +++ b/lib/lambda_ethereum_consensus/validator/block_builder.ex @@ -299,7 +299,7 @@ defmodule LambdaEthereumConsensus.Validator.BlockBuilder do {:error, reason} -> # Default to the last eth1 data on error Logger.error("Failed to fetch eth1 vote: #{reason}") - head_state.eth1_data + head_state.eth1_data |> IO.inspect(label: "old eth1_data") {:ok, nil} -> head_state.eth1_data From fe068b2fcb1249afb2ed01e1b37eba0c18723701 Mon Sep 17 00:00:00 2001 From: Rodrigo Oliveri Date: Wed, 25 Sep 2024 19:12:35 -0300 Subject: [PATCH 2/6] Fixed some changes and removed IO.inspect --- .../execution/execution_chain.ex | 29 +++++++------------ .../validator/block_builder.ex | 2 +- lib/types/deposit_tree.ex | 1 + 3 files changed, 13 insertions(+), 19 deletions(-) diff --git a/lib/lambda_ethereum_consensus/execution/execution_chain.ex b/lib/lambda_ethereum_consensus/execution/execution_chain.ex index 10bcf8910..cdf025eff 100644 --- a/lib/lambda_ethereum_consensus/execution/execution_chain.ex +++ b/lib/lambda_ethereum_consensus/execution/execution_chain.ex @@ -195,11 +195,9 @@ defmodule LambdaEthereumConsensus.Execution.ExecutionChain do eth1_data_votes: seen_votes, deposit_tree: deposit_tree, current_eth1_data: default - } = state, + }, slot ) do - Logger.info("Computing eth1 vote for slot #{slot}") - IO.inspect(state, label: "state", limit: :infinity, pretty: true) period_start = voting_period_start_time(slot) blocks_to_consider = @@ -231,44 +229,42 @@ defmodule LambdaEthereumConsensus.Execution.ExecutionChain do {valid_votes, _last_tree, last_eth1_data} = blocks_to_consider |> Enum.reduce({MapSet.new(), deposit_tree, nil}, fn block, {set, tree, last_eth1_data} -> - IO.inspect(block.block_number, label: "block_number in get_first_valid_vote") new_tree = case grouped_deposits[block.block_number] do nil -> tree deposits -> update_tree_with_deposits(tree, deposits) end - data = get_eth1_data(block, new_tree) |> IO.inspect(label: "data in get_first_valid_vote") + data = get_eth1_data(block, new_tree) - if data.deposit_count >= default.deposit_count, + # Spec says: get_eth1_data(block).deposit_count >= state.eth1_data.deposit_count, but this clearly + # overrites eth1data even when no deposits ocurred, eth2book instead says: + # "Filter out any blocks that have a deposit count less than state.eth1_data.deposit_count: we've already seen these." + # Which make a lot more sense. + if data.deposit_count > default.deposit_count, do: {MapSet.put(set, data), new_tree, data}, else: {set, new_tree, last_eth1_data} end) - IO.inspect(valid_votes, label: "valid_votes", limit: :infinity, pretty: true) - IO.inspect(seen_votes, label: "seen_votes", limit: :infinity, pretty: true) - # Default vote on latest eth1 block data in the period range unless eth1 chain is not live - default_vote = last_eth1_data || default |> IO.inspect(label: "default_vote") + default_vote = (last_eth1_data || default) # Tiebreak by smallest distance to period start result = seen_votes |> Stream.filter(fn {eth1_data, _count_and_distance} -> MapSet.member?(valid_votes, eth1_data) end) - |> tap(&IO.inspect(&1 |> Enum.to_list(), label: "valid_votes_seen", limit: :infinity, pretty: true)) |> Enum.max(fn {_, {count1, dist1}}, {_, {count2, dist2}} -> cond do count1 > count2 -> true - count1 == count2 && dist1 >= dist2 -> true + count1 == count2 && dist1 > dist2 -> true true -> false end - end, fn -> nil end) |> IO.inspect(label: "result") + end, fn -> nil end) case result do nil -> {:ok, default_vote} {eth1_data, _} -> {:ok, eth1_data} end - |> tap(&IO.inspect(LambdaEthereumConsensus.Utils.format_shorten_binary(elem(&1, 1).block_hash), label: "result", pretty: true)) end defp get_eth1_data(block, tree) do @@ -286,10 +282,7 @@ defmodule LambdaEthereumConsensus.Execution.ExecutionChain do end defp candidate_block?(timestamp, period_start) do - follow_time = ChainSpec.get("SECONDS_PER_ETH1_BLOCK") * 0 # ChainSpec.get("ETH1_FOLLOW_DISTANCE") - IO.inspect("ts: #{timestamp}, period_start: #{period_start}, follow_time: #{follow_time}", label: "candidate_block") - IO.inspect("current result: #{inspect(timestamp + follow_time <= period_start and timestamp + follow_time * 2 >= period_start)}") - IO.inspect("old result: #{inspect(timestamp in (period_start - follow_time * 2)..(period_start - follow_time))}") + follow_time = ChainSpec.get("SECONDS_PER_ETH1_BLOCK") * ChainSpec.get("ETH1_FOLLOW_DISTANCE") timestamp + follow_time <= period_start and timestamp + follow_time * 2 >= period_start end diff --git a/lib/lambda_ethereum_consensus/validator/block_builder.ex b/lib/lambda_ethereum_consensus/validator/block_builder.ex index 157ae9582..fc1d5edc7 100644 --- a/lib/lambda_ethereum_consensus/validator/block_builder.ex +++ b/lib/lambda_ethereum_consensus/validator/block_builder.ex @@ -299,7 +299,7 @@ defmodule LambdaEthereumConsensus.Validator.BlockBuilder do {:error, reason} -> # Default to the last eth1 data on error Logger.error("Failed to fetch eth1 vote: #{reason}") - head_state.eth1_data |> IO.inspect(label: "old eth1_data") + head_state.eth1_data {:ok, nil} -> head_state.eth1_data diff --git a/lib/types/deposit_tree.ex b/lib/types/deposit_tree.ex index d235046ec..4e16c8bfb 100644 --- a/lib/types/deposit_tree.ex +++ b/lib/types/deposit_tree.ex @@ -130,6 +130,7 @@ defmodule Types.DepositTree do {:node, {create_node(leaves_left, depth - 1), create_node(leaves_right, depth - 1)}} end + defp finalize_tree({:zero, depth}, 0 = _deposit_count, _), do: {:zero, depth} defp finalize_tree({:finalized, _} = node, _, _), do: node defp finalize_tree({:leaf, {hash, _}}, _, _), do: {:finalized, {hash, 1}} From d00f04efbcc14003d76368841d35c2a069724732 Mon Sep 17 00:00:00 2001 From: Rodrigo Oliveri Date: Thu, 26 Sep 2024 11:02:13 -0300 Subject: [PATCH 3/6] Some comments added after testing --- lib/lambda_ethereum_consensus/execution/execution_chain.ex | 2 +- lib/types/deposit_tree.ex | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/lambda_ethereum_consensus/execution/execution_chain.ex b/lib/lambda_ethereum_consensus/execution/execution_chain.ex index cdf025eff..be108b4a9 100644 --- a/lib/lambda_ethereum_consensus/execution/execution_chain.ex +++ b/lib/lambda_ethereum_consensus/execution/execution_chain.ex @@ -247,7 +247,7 @@ defmodule LambdaEthereumConsensus.Execution.ExecutionChain do end) # Default vote on latest eth1 block data in the period range unless eth1 chain is not live - default_vote = (last_eth1_data || default) + default_vote = last_eth1_data || default # Tiebreak by smallest distance to period start result = diff --git a/lib/types/deposit_tree.ex b/lib/types/deposit_tree.ex index 4e16c8bfb..b650e71e3 100644 --- a/lib/types/deposit_tree.ex +++ b/lib/types/deposit_tree.ex @@ -130,6 +130,8 @@ defmodule Types.DepositTree do {:node, {create_node(leaves_left, depth - 1), create_node(leaves_right, depth - 1)}} end + # This new clause needed to transform zero in get_finalized into a [], this bug was + # similar to one present in teku: https://github.com/Consensys/teku/pull/7628 defp finalize_tree({:zero, depth}, 0 = _deposit_count, _), do: {:zero, depth} defp finalize_tree({:finalized, _} = node, _, _), do: node defp finalize_tree({:leaf, {hash, _}}, _, _), do: {:finalized, {hash, 1}} From f19dc2a6d6220c7eabb14ce4a3c5b67931c5dc2d Mon Sep 17 00:00:00 2001 From: Rodrigo Oliveri Date: Thu, 26 Sep 2024 12:43:03 -0300 Subject: [PATCH 4/6] refactored a function and fixed formatting --- .../execution/execution_chain.ex | 70 +++++++++++-------- 1 file changed, 40 insertions(+), 30 deletions(-) diff --git a/lib/lambda_ethereum_consensus/execution/execution_chain.ex b/lib/lambda_ethereum_consensus/execution/execution_chain.ex index be108b4a9..61fb8bfbc 100644 --- a/lib/lambda_ethereum_consensus/execution/execution_chain.ex +++ b/lib/lambda_ethereum_consensus/execution/execution_chain.ex @@ -223,43 +223,30 @@ defmodule LambdaEthereumConsensus.Execution.ExecutionChain do end defp get_first_valid_vote(blocks_to_consider, seen_votes, deposit_tree, new_deposits, default) do - Logger.info("Processing new deposits and get first valid vote") - grouped_deposits = Enum.group_by(new_deposits, &Map.fetch!(&1, :block_number)) - - {valid_votes, _last_tree, last_eth1_data} = - blocks_to_consider - |> Enum.reduce({MapSet.new(), deposit_tree, nil}, fn block, {set, tree, last_eth1_data} -> - new_tree = - case grouped_deposits[block.block_number] do - nil -> tree - deposits -> update_tree_with_deposits(tree, deposits) - end - - data = get_eth1_data(block, new_tree) + Logger.debug( + "Processing new deposits: #{inspect(new_deposits)} and get first valid vote, with default: #{inspect(default)}" + ) - # Spec says: get_eth1_data(block).deposit_count >= state.eth1_data.deposit_count, but this clearly - # overrites eth1data even when no deposits ocurred, eth2book instead says: - # "Filter out any blocks that have a deposit count less than state.eth1_data.deposit_count: we've already seen these." - # Which make a lot more sense. - if data.deposit_count > default.deposit_count, - do: {MapSet.put(set, data), new_tree, data}, - else: {set, new_tree, last_eth1_data} - end) + {valid_votes, last_eth1_data} = + get_valid_votes(blocks_to_consider, deposit_tree, new_deposits, default) # Default vote on latest eth1 block data in the period range unless eth1 chain is not live default_vote = last_eth1_data || default - # Tiebreak by smallest distance to period start + # Tiebreak by smallest distance to period start seen_votes is a %{eth1_data -> {count, dist}} result = seen_votes - |> Stream.filter(fn {eth1_data, _count_and_distance} -> MapSet.member?(valid_votes, eth1_data) end) - |> Enum.max(fn {_, {count1, dist1}}, {_, {count2, dist2}} -> - cond do - count1 > count2 -> true - count1 == count2 && dist1 > dist2 -> true - true -> false - end - end, fn -> nil end) + |> Stream.filter(fn {eth1_data, _} -> MapSet.member?(valid_votes, eth1_data) end) + |> Enum.max( + fn {_, {count1, dist1}}, {_, {count2, dist2}} -> + cond do + count1 > count2 -> true + count1 == count2 && dist1 > dist2 -> true + true -> false + end + end, + fn -> nil end + ) case result do nil -> {:ok, default_vote} @@ -267,6 +254,29 @@ defmodule LambdaEthereumConsensus.Execution.ExecutionChain do end end + defp get_valid_votes(blocks_to_consider, deposit_tree, new_deposits, default) do + grouped_deposits = Enum.group_by(new_deposits, &Map.fetch!(&1, :block_number)) + + blocks_to_consider + |> Enum.reduce({MapSet.new(), deposit_tree, nil}, fn block, {set, tree, last_eth1_data} -> + new_tree = + case grouped_deposits[block.block_number] do + nil -> tree + deposits -> update_tree_with_deposits(tree, deposits) + end + + data = get_eth1_data(block, new_tree) + + # Spec says: get_eth1_data(block).deposit_count >= state.eth1_data.deposit_count, but this clearly + # overrites eth1data even when no deposits ocurred, eth2book instead says: + # "Filter out any blocks that have a deposit count less than state.eth1_data.deposit_count: + # we've already seen these." Which make a lot more sense. + if data.deposit_count > default.deposit_count, + do: {MapSet.put(set, data), new_tree, data}, + else: {set, new_tree, last_eth1_data} + end) + end + defp get_eth1_data(block, tree) do %Eth1Data{ deposit_root: DepositTree.get_root(tree), From 83b3a3a241c0730d95387b5d08fe06c4711053b8 Mon Sep 17 00:00:00 2001 From: Rodrigo Oliveri Date: Thu, 26 Sep 2024 12:54:53 -0300 Subject: [PATCH 5/6] Removed unnedded comments and fixed a comparision to match the spec --- lib/lambda_ethereum_consensus/execution/execution_chain.ex | 6 +----- lib/types/deposit_tree.ex | 2 -- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/lib/lambda_ethereum_consensus/execution/execution_chain.ex b/lib/lambda_ethereum_consensus/execution/execution_chain.ex index 61fb8bfbc..2d5b930b9 100644 --- a/lib/lambda_ethereum_consensus/execution/execution_chain.ex +++ b/lib/lambda_ethereum_consensus/execution/execution_chain.ex @@ -267,11 +267,7 @@ defmodule LambdaEthereumConsensus.Execution.ExecutionChain do data = get_eth1_data(block, new_tree) - # Spec says: get_eth1_data(block).deposit_count >= state.eth1_data.deposit_count, but this clearly - # overrites eth1data even when no deposits ocurred, eth2book instead says: - # "Filter out any blocks that have a deposit count less than state.eth1_data.deposit_count: - # we've already seen these." Which make a lot more sense. - if data.deposit_count > default.deposit_count, + if data.deposit_count >= default.deposit_count, do: {MapSet.put(set, data), new_tree, data}, else: {set, new_tree, last_eth1_data} end) diff --git a/lib/types/deposit_tree.ex b/lib/types/deposit_tree.ex index b650e71e3..4e16c8bfb 100644 --- a/lib/types/deposit_tree.ex +++ b/lib/types/deposit_tree.ex @@ -130,8 +130,6 @@ defmodule Types.DepositTree do {:node, {create_node(leaves_left, depth - 1), create_node(leaves_right, depth - 1)}} end - # This new clause needed to transform zero in get_finalized into a [], this bug was - # similar to one present in teku: https://github.com/Consensys/teku/pull/7628 defp finalize_tree({:zero, depth}, 0 = _deposit_count, _), do: {:zero, depth} defp finalize_tree({:finalized, _} = node, _, _), do: node defp finalize_tree({:leaf, {hash, _}}, _, _), do: {:finalized, {hash, 1}} From 14dfd592fe77a922192709d8a7c82695af4a246e Mon Sep 17 00:00:00 2001 From: Rodrigo Oliveri Date: Thu, 26 Sep 2024 14:59:13 -0300 Subject: [PATCH 6/6] Added test for the empty case of the deposit tree --- test/unit/deposit_tree_test.exs | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/test/unit/deposit_tree_test.exs b/test/unit/deposit_tree_test.exs index 352466ac9..fd2297f44 100644 --- a/test/unit/deposit_tree_test.exs +++ b/test/unit/deposit_tree_test.exs @@ -11,7 +11,17 @@ defmodule Unit.DepositTreeTest do doctest DepositTree - # Testcases taken from EIP-4881 + # Testcases taken from EIP-4881 + empty case + @snapshot_empty %DepositTreeSnapshot{ + finalized: [], + deposit_root: + Base.decode16!("D70A234731285C6804C2A4F56711DDB8C82C99740F207854891028AF34E27E5E"), + deposit_count: 0, + execution_block_hash: + Base.decode16!("C0B2CBA66FA21E555461E6B699E0F280A5C4A9CD7AE724D79F711E57460FFB2B"), + execution_block_height: 0 + } + @snapshot_1 %DepositTreeSnapshot{ finalized: [ Base.decode16!("7AF7DA533B0DC64B690CB0604F5A81E40ED83796DD14037EA3A55383B8F0976A") @@ -108,4 +118,16 @@ defmodule Unit.DepositTreeTest do assert DepositTree.get_snapshot(tree) == @snapshot_2 end + + test "finalizing an empty tree is equal to itself" do + eth1_data = %Eth1Data{ + deposit_root: @snapshot_empty.deposit_root, + deposit_count: @snapshot_empty.deposit_count, + block_hash: @snapshot_empty.execution_block_hash + } + + tree = DepositTree.from_snapshot(@snapshot_empty) |> DepositTree.finalize(eth1_data, 0) + + assert tree == DepositTree.from_snapshot(@snapshot_empty) + end end