Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add bitlist and bitvector ssz support #494

Merged
merged 19 commits into from
Jan 4, 2024

Conversation

f3r10
Copy link
Contributor

@f3r10 f3r10 commented Dec 4, 2023

Motivation

Closes #479 and #480 only encoding/decoding support

@f3r10 f3r10 requested a review from a team as a code owner December 4, 2023 11:49
@f3r10 f3r10 changed the title feat: add bitlist ssz support feat: add bitlist and bitvector ssz support (encoding/decoding) Dec 4, 2023
@f3r10 f3r10 changed the title feat: add bitlist and bitvector ssz support (encoding/decoding) feat: add bitlist and bitvector ssz support Dec 4, 2023
lib/ssz_ex.ex Outdated Show resolved Hide resolved
lib/ssz_ex.ex Show resolved Hide resolved
lib/ssz_ex.ex Outdated Show resolved Hide resolved
lib/ssz_ex.ex Outdated Show resolved Hide resolved
test/unit/ssz_ex_test.exs Show resolved Hide resolved
@f3r10 f3r10 requested a review from Arkenan December 14, 2023 01:14
Copy link
Collaborator

@Arkenan Arkenan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I reviewed the PR and requested some more changes. Also, this is a nit, but can we have the encoding functions first and the decoding functions afterwards?

lib/ssz_ex.ex Outdated Show resolved Hide resolved
lib/ssz_ex.ex Outdated Show resolved Hide resolved
lib/ssz_ex.ex Show resolved Hide resolved
lib/ssz_ex.ex Outdated Show resolved Hide resolved
lib/ssz_ex.ex Outdated Show resolved Hide resolved
lib/ssz_ex.ex Outdated Show resolved Hide resolved
@f3r10 f3r10 requested a review from Arkenan December 20, 2023 19:29
Copy link
Collaborator

@Arkenan Arkenan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Fernando, last changes of the PR probably. Thanks for the work on this.

lib/ssz_ex.ex Outdated
Comment on lines 149 to 178
num_bytes = byte_size(bit_list)
num_bits = num_bytes * @bits_per_byte
padding = num_bits - bit_size(bit_list)
formatted_bit_list = <<0::size(padding), bit_list::bitstring>>
len = length_of_bitlist(formatted_bit_list)

cond do
len < 0 ->
{:error, "missing length information"}

div(len, @bits_per_byte) + 1 != num_bytes ->
{:error, "invalid byte count"}

len > max_size ->
{:error, "out of bounds"}

true ->
pre_trailing_bits_size = (num_bytes - 1) * @bits_per_byte
<<pre::size(pre_trailing_bits_size), last_byte::binary>> = formatted_bit_list
<<_trailing::1, last::size(7)>> = last_byte

cond do
last == 0 ->
{:ok, pre}

pre == 0 ->
{:ok, <<last::size(len)>>}

true ->
{:ok, <<pre::size(pre_trailing_bits_size), last::size(len - pre_trailing_bits_size)>>}
Copy link
Collaborator

@Arkenan Arkenan Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still overly complicated and doesn't address the padding problem. We can simplify this to something shorter:

Again, let's address this. It can be done in a simpler way:

<<pre::size(byte_size(bitlist)-1), last_byte::1>> = bitlist
decoded = <<pre, remove_trailing_bit(last_byte)>>
cond do
  decoded == <<>> -> {:error, "missing length information"}
  # Other edge cases
  true -> decoded
end

And that's it. The only missing thing is to code the removal of the trailing bit. Instead of using the length_of_bitlist you can just code a new function that's similar to leading_zeros (matching each possible byte) that ignores the first 1 bit and returns the rest. In the case of a <<0>> byte you can just return <<>> and the rest of the functionality will stil work.

lib/ssz_ex.ex Outdated
end
end

defp encode_bitvector(bit_vector, size) when bit_size(bit_vector) == size, do: {:ok, bit_vector}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of {:ok, bit_vector} here let's return {:ok, BitVector.to_bytes(bit_vector)}, which is a fix that has been added a couple of weeks ago (BitVector.new() was modified too).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a warning about the opaque type:
Call does not have expected opaque term of type LambdaEthereumConsensus.Utils.BitVector.t() in the 1st position 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a BitVector.bit_size guard in the BitVector and use it here to see if the issue gets fixed? On guards: https://hexdocs.pm/elixir/main/Kernel.html#defguard/1

@f3r10 f3r10 requested a review from Arkenan January 4, 2024 14:23
Copy link
Collaborator

@Arkenan Arkenan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Arkenan Arkenan merged commit f2fe509 into lambdaclass:main Jan 4, 2024
9 checks passed
h3lio5 pushed a commit to h3lio5/lambda_ethereum_consensus that referenced this pull request Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SSZ] Support bitlists
2 participants