Skip to content

feat!: Fixing the bytesize for be, le byte serialization, adding new functionality and tests#157

Merged
TomAFrench merged 10 commits intomainfrom
kb/add_to_be_bytes_from_le_bytes
Apr 14, 2025
Merged

feat!: Fixing the bytesize for be, le byte serialization, adding new functionality and tests#157
TomAFrench merged 10 commits intomainfrom
kb/add_to_be_bytes_from_le_bytes

Conversation

@kashbrti
Copy link
Copy Markdown
Contributor

@kashbrti kashbrti commented Apr 7, 2025

Description

Our current to_le_bytes and from_be_bytes implementations required the bytesize to be passed as a generic.
This was offering a nonideal interface as when the bytesize was not large enough, it would cause a non-comprehensive
error.
After discussions with Zac, I decided to fix the size of the byte representations.

Also, to facilitate the blob work, I've added a to_be_bytes functionality, which is needed for BLS point compression.
This PR adds two new methods:

  • to_be_bytes()
  • from_le_bytes()
    These two functions have also allowed us to write fuzz tests for the byte serialization functions.
    On another note, I fixed a small bug I found, a line that was commented out for testing reasons during the switch to u128 limbs, which I just uncommented. This resolves last limb of derive_from_seed is always 1 #156 as well.

I have also included the changes in #155 in this PR to avoid merging issues.

Problem*

Resolves

Summary*

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

Comment thread src/benchmarks/bignum_benchmarks.nr Outdated
}

// TODO: add the benchmarks for serialization to the macro (currently dealing with trait visibility issues)
#[export]
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried adding these to the macro, but I was getting complaints that the trait that implements the function is out of bounds.

@kashbrti
Copy link
Copy Markdown
Contributor Author

kashbrti commented Apr 7, 2025

@TomAFrench, there's a compiler panic that has been resolved in the nightly version.
Are we planning on pushing a new Nargo release soon?
I believe this should be related to the issue I had in #151 with associated constants.

@kashbrti kashbrti marked this pull request as ready for review April 8, 2025 11:16
Copy link
Copy Markdown

@MirandaWood MirandaWood left a comment

Choose a reason for hiding this comment

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

Not familiar with Noir benchmarks but logic etc. LGTM, thanks for adding! 🚀

@TomAFrench
Copy link
Copy Markdown
Member

I'd like to avoid having two completely separate codepaths for serde logic just depending on the endianness. We should be able to avoid this and share logic between then (in the ultra-simple case just reverse the input/output array as appropriate).

@kashbrti
Copy link
Copy Markdown
Contributor Author

kashbrti commented Apr 8, 2025

@TomAFrench I'll just revert the arrays then. wondering how this will affect the number of constraints though. Given that these are runtime arrays, this will introduce additional RAM gates = the number of bytes the representation needs.

@TomAFrench
Copy link
Copy Markdown
Member

I would expect this to be free in ACIR and negligible overhead in brillig. We shouldn't be using RAM gates at all in this situation.

@kashbrti
Copy link
Copy Markdown
Contributor Author

kashbrti commented Apr 8, 2025

Did a bit of experimenting. The circuit size goes from 2894 to 2905, which I believe is an acceptable increase.

@TomAFrench TomAFrench enabled auto-merge (squash) April 14, 2025 21:20
@TomAFrench TomAFrench merged commit 3a2664e into main Apr 14, 2025
7 checks passed
@github-project-automation github-project-automation Bot moved this from 👀 To Triage to ✅ Done in Noir Libraries Apr 14, 2025
@github-actions github-actions Bot mentioned this pull request Apr 14, 2025
@TomAFrench
Copy link
Copy Markdown
Member

image

Looks like these new tests are really heavy @kashbrti.

env:
CARGO_TERM_COLOR: always
MINIMUM_NOIR_VERSION: v1.0.0-beta.3
MINIMUM_NOIR_VERSION: v1.0.0-beta.4
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kashbrti should we update this repo's README:

This library is tested with all stable releases since 1.0.0-beta.0 as well as nightly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

last limb of derive_from_seed is always 1

4 participants