fix: errors "mismatched aggregation bits length" and "invalid signatu…

…re" (lambdaclass#510)

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 """

lib/lambda_ethereum_consensus/state_transition/misc.ex

@@ -230,19 +230,16 @@ 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()
   def compute_signing_root(ssz_object, domain) 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

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(<<last_byte>>) - 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
 

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