From c2e55e4bacc3a66809f85d700382f1bdf813fcd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Mon, 11 Dec 2023 16:31:52 -0300 Subject: [PATCH] fix: errors "mismatched aggregation bits length" and "invalid signature" (#510) --- .../state_transition/accessors.ex | 60 ++++----- .../state_transition/misc.ex | 9 +- .../state_transition/operations.ex | 116 ++++++++++-------- .../state_transition/state_transition.ex | 1 - ...tion_mainnet.ex => indexed_attestation.ex} | 0 5 files changed, 88 insertions(+), 98 deletions(-) rename lib/ssz_types/beacon_chain/{indexed_attestation_mainnet.ex => indexed_attestation.ex} (100%) diff --git a/lib/lambda_ethereum_consensus/state_transition/accessors.ex b/lib/lambda_ethereum_consensus/state_transition/accessors.ex index 574bfb59b..588641ba9 100644 --- a/lib/lambda_ethereum_consensus/state_transition/accessors.ex +++ b/lib/lambda_ethereum_consensus/state_transition/accessors.ex @@ -433,21 +433,14 @@ defmodule LambdaEthereumConsensus.StateTransition.Accessors do @spec get_indexed_attestation(BeaconState.t(), Attestation.t()) :: {:ok, IndexedAttestation.t()} | {:error, binary()} def get_indexed_attestation(%BeaconState{} = state, attestation) do - case get_attesting_indices(state, attestation.data, attestation.aggregation_bits) do - {:ok, indices} -> - attesting_indices = indices - sorted_attesting_indices = Enum.sort(attesting_indices) - - res = %IndexedAttestation{ - attesting_indices: sorted_attesting_indices, - data: attestation.data, - signature: attestation.signature - } - - {:ok, res} - - {:error, reason} -> - {:error, reason} + with {:ok, indices} <- + get_attesting_indices(state, attestation.data, attestation.aggregation_bits) do + %IndexedAttestation{ + attesting_indices: Enum.sort(indices), + data: attestation.data, + signature: attestation.signature + } + |> then(&{:ok, &1}) end end @@ -457,33 +450,22 @@ defmodule LambdaEthereumConsensus.StateTransition.Accessors do @spec get_attesting_indices(BeaconState.t(), SszTypes.AttestationData.t(), SszTypes.bitlist()) :: {:ok, MapSet.t()} | {:error, binary()} def get_attesting_indices(%BeaconState{} = state, data, bits) do - case get_beacon_committee(state, data.slot, data.index) do - {:ok, committee} -> - bit_list = bitstring_to_list(bits) - - res = - committee - |> Stream.with_index() - |> Stream.filter(fn {_value, index} -> Enum.at(bit_list, index) == "1" end) - |> Stream.map(fn {value, _index} -> value end) - |> MapSet.new() - - {:ok, res} - - {:error, reason} -> - {:error, reason} + with {:ok, committee} <- get_beacon_committee(state, data.slot, data.index) do + committee + |> Stream.with_index() + |> Stream.filter(fn {_value, index} -> participated?(bits, index) end) + |> Stream.map(fn {value, _index} -> value end) + |> MapSet.new() + |> then(&{:ok, &1}) end end - def bitstring_to_list(binary) when is_binary(binary) do - binary - |> :binary.bin_to_list() - |> Enum.reduce("", fn byte, acc -> - acc <> Integer.to_string(byte, 2) - end) - # Exclude last bit - |> String.slice(0..-2) - |> String.graphemes() + defp participated?(bits, index) do + # The bit order inside the byte is reversed (e.g. bits[0] is the 8th bit). + # Here we keep the byte index the same, but reverse the bit index. + bit_index = index + 7 - 2 * rem(index, 8) + <<_::size(bit_index), flag::1, _::bits>> = bits + flag == 1 end @doc """ diff --git a/lib/lambda_ethereum_consensus/state_transition/misc.ex b/lib/lambda_ethereum_consensus/state_transition/misc.ex index 074474086..5e152cebb 100644 --- a/lib/lambda_ethereum_consensus/state_transition/misc.ex +++ b/lib/lambda_ethereum_consensus/state_transition/misc.ex @@ -230,11 +230,8 @@ defmodule LambdaEthereumConsensus.StateTransition.Misc do Return the signing root for the corresponding signing data. """ @spec compute_signing_root(SszTypes.bytes32(), SszTypes.domain()) :: SszTypes.root() - def compute_signing_root(<<_::256>> = data, domain) do - Ssz.hash_tree_root!(%SszTypes.SigningData{ - object_root: data, - domain: domain - }) + def compute_signing_root(<<_::256>> = root, domain) do + Ssz.hash_tree_root!(%SszTypes.SigningData{object_root: root, domain: domain}) end @spec compute_signing_root(any(), SszTypes.domain()) :: SszTypes.root() @@ -242,7 +239,7 @@ defmodule LambdaEthereumConsensus.StateTransition.Misc do ssz_object |> Ssz.hash_tree_root!() |> compute_signing_root(domain) end - @spec compute_signing_root(any(), module, SszTypes.domain()) :: SszTypes.root() + @spec compute_signing_root(any(), module(), SszTypes.domain()) :: SszTypes.root() def compute_signing_root(ssz_object, schema, domain) do ssz_object |> Ssz.hash_tree_root!(schema) |> compute_signing_root(domain) end diff --git a/lib/lambda_ethereum_consensus/state_transition/operations.ex b/lib/lambda_ethereum_consensus/state_transition/operations.ex index 82d9cd779..64bd832d7 100644 --- a/lib/lambda_ethereum_consensus/state_transition/operations.ex +++ b/lib/lambda_ethereum_consensus/state_transition/operations.ex @@ -644,11 +644,18 @@ defmodule LambdaEthereumConsensus.StateTransition.Operations do """ @spec process_attestation(BeaconState.t(), Attestation.t()) :: {:ok, BeaconState.t()} | {:error, binary()} - def process_attestation(state, attestation) do + def process_attestation(state, %Attestation{data: data} = attestation) do # TODO: optimize (takes ~3s) - with :ok <- verify_attestation_for_process(state, attestation) do + with :ok <- check_valid_target_epoch(data, state), + :ok <- check_epoch_matches(data), + :ok <- check_valid_slot_range(data, state), + :ok <- check_committee_count(data, state), + {:ok, beacon_committee} <- Accessors.get_beacon_committee(state, data.slot, data.index), + :ok <- check_matching_aggregation_bits_length(attestation, beacon_committee), + {:ok, indexed_attestation} <- Accessors.get_indexed_attestation(state, attestation), + :ok <- check_valid_signature(state, indexed_attestation) do # TODO: optimize (takes ~1s) - process_attestation(state, attestation.data, attestation.aggregation_bits) + process_attestation(state, data, attestation.aggregation_bits) end end @@ -740,34 +747,6 @@ defmodule LambdaEthereumConsensus.StateTransition.Operations do defp update_state(state, false, updated_epoch_participation), do: %{state | previous_epoch_participation: updated_epoch_participation} - def verify_attestation_for_process(state, %Attestation{data: data} = attestation) do - with {:ok, beacon_committee} <- Accessors.get_beacon_committee(state, data.slot, data.index), - {:ok, indexed_attestation} <- Accessors.get_indexed_attestation(state, attestation) do - cond do - invalid_target_epoch?(data, state) -> - {:error, "Invalid target epoch"} - - epoch_mismatch?(data) -> - {:error, "Epoch mismatch"} - - invalid_slot_range?(data, state) -> - {:error, "Invalid slot range"} - - exceeds_committee_count?(data, state) -> - {:error, "Index exceeds committee count"} - - mismatched_aggregation_bits_length?(attestation, beacon_committee) -> - {:error, "Mismatched aggregation bits length"} - - not valid_signature?(state, indexed_attestation) -> - {:error, "Invalid signature"} - - true -> - :ok - end - end - end - @doc """ Provide randomness to the operation of the beacon chain. """ @@ -835,41 +814,74 @@ defmodule LambdaEthereumConsensus.StateTransition.Operations do end end - defp invalid_target_epoch?(data, state) do - data.target.epoch < Accessors.get_previous_epoch(state) || - data.target.epoch > Accessors.get_current_epoch(state) + defp check_valid_target_epoch(data, state) do + if data.target.epoch in [ + Accessors.get_previous_epoch(state), + Accessors.get_current_epoch(state) + ] do + :ok + else + {:error, "Invalid target epoch"} + end end - defp epoch_mismatch?(data) do - data.target.epoch != Misc.compute_epoch_at_slot(data.slot) + defp check_epoch_matches(data) do + if data.target.epoch == Misc.compute_epoch_at_slot(data.slot) do + :ok + else + {:error, "Epoch mismatch"} + end end - defp invalid_slot_range?(data, state) do - state.slot < data.slot + ChainSpec.get("MIN_ATTESTATION_INCLUSION_DELAY") || - state.slot > data.slot + ChainSpec.get("SLOTS_PER_EPOCH") + defp check_valid_slot_range(data, state) do + if data.slot + ChainSpec.get("MIN_ATTESTATION_INCLUSION_DELAY") <= state.slot and + state.slot <= data.slot + ChainSpec.get("SLOTS_PER_EPOCH") do + :ok + else + {:error, "Invalid slot range"} + end end - defp exceeds_committee_count?(data, state) do - data.index >= Accessors.get_committee_count_per_slot(state, data.target.epoch) + defp check_committee_count(data, state) do + if data.index >= Accessors.get_committee_count_per_slot(state, data.target.epoch) do + {:error, "Index exceeds committee count"} + else + :ok + end end - defp mismatched_aggregation_bits_length?(attestation, beacon_committee) do - length_of_bitstring(attestation.aggregation_bits) - 1 != length(beacon_committee) + defp check_matching_aggregation_bits_length(attestation, beacon_committee) do + if length_of_bitlist(attestation.aggregation_bits) == length(beacon_committee) do + :ok + else + {:error, "Mismatched aggregation bits length"} + end end - defp valid_signature?(state, indexed_attestation) do - Predicates.is_valid_indexed_attestation(state, indexed_attestation) + defp check_valid_signature(state, indexed_attestation) do + if Predicates.is_valid_indexed_attestation(state, indexed_attestation) do + :ok + else + {:error, "Invalid signature"} + end end - defp length_of_bitstring(binary) when is_binary(binary) do - binary - |> :binary.bin_to_list() - |> Enum.reduce("", fn byte, acc -> - acc <> Integer.to_string(byte, 2) - end) - |> String.length() + defp length_of_bitlist(bitlist) when is_binary(bitlist) do + bit_size = bit_size(bitlist) + <<_::size(bit_size - 8), last_byte>> = bitlist + bit_size - leading_zeros(<>) - 1 end + defp leading_zeros(<<1::1, _::7>>), do: 0 + defp leading_zeros(<<0::1, 1::1, _::6>>), do: 1 + defp leading_zeros(<<0::2, 1::1, _::5>>), do: 2 + defp leading_zeros(<<0::3, 1::1, _::4>>), do: 3 + defp leading_zeros(<<0::4, 1::1, _::3>>), do: 4 + defp leading_zeros(<<0::5, 1::1, _::2>>), do: 5 + defp leading_zeros(<<0::6, 1::1, _::1>>), do: 6 + defp leading_zeros(<<0::7, 1::1>>), do: 7 + defp leading_zeros(<<0::8>>), do: 8 + def process_bls_to_execution_change(state, signed_address_change) do address_change = signed_address_change.message diff --git a/lib/lambda_ethereum_consensus/state_transition/state_transition.ex b/lib/lambda_ethereum_consensus/state_transition/state_transition.ex index fcce6062d..16c9446d8 100644 --- a/lib/lambda_ethereum_consensus/state_transition/state_transition.ex +++ b/lib/lambda_ethereum_consensus/state_transition/state_transition.ex @@ -113,7 +113,6 @@ defmodule LambdaEthereumConsensus.StateTransition do Bls.valid?(proposer.pubkey, signing_root, signed_block.signature) end - # TODO: uncomment when implemented def process_block(state, block) do verify_and_notify_new_payload = &Execution.verify_and_notify_new_payload/1 diff --git a/lib/ssz_types/beacon_chain/indexed_attestation_mainnet.ex b/lib/ssz_types/beacon_chain/indexed_attestation.ex similarity index 100% rename from lib/ssz_types/beacon_chain/indexed_attestation_mainnet.ex rename to lib/ssz_types/beacon_chain/indexed_attestation.ex