Skip to content

Make BLAKE3 Limb Length Generic#263

Merged
RobinLinus merged 13 commits into
BitVM:mainfrom
chainwayxyz:dynamic_limb_length_blake3
Mar 17, 2025
Merged

Make BLAKE3 Limb Length Generic#263
RobinLinus merged 13 commits into
BitVM:mainfrom
chainwayxyz:dynamic_limb_length_blake3

Conversation

@just-erray

Copy link
Copy Markdown
Contributor

This PR changes the compact BLAKE3 code, so that it can work with various limb lengths. Previously, it worked only with 29, since the underlying BigInt limb length for field values was 29. By changing only 4 lines inside the BLAKE3 function, this update improves usability for future and outside use (since using a limb length of 4 enables the script for the unpacked version) while also serving as a small optimization for other cases.

Note that this PR also includes some small optimizations. It is currently in draft status due to significant conflicts with an open PR. Once that PR is merged, this code will undergo substantial revisions.

@uncomputable

Copy link
Copy Markdown
Contributor

Thanks for waiting for #257 :) We should be able to merge that PR soon.

@AaronFeickert

Copy link
Copy Markdown
Contributor

Please rebase now that #257 is merged.

@just-erray just-erray closed this Mar 15, 2025
@just-erray
just-erray force-pushed the dynamic_limb_length_blake3 branch from fb8eb47 to 22d4892 Compare March 15, 2025 06:07
@just-erray just-erray reopened this Mar 15, 2025
@just-erray

Copy link
Copy Markdown
Contributor Author

Since the two PRs were too conflicting, I reset this branch and reapplied the changes on top of it. I also cleaned up lib.rs a bit while I got my hands dirty, but nothing major. I left fuzz test only running with limb length 29, since other tests should be more than enough for the others. It should be ready to merge now.

@just-erray
just-erray marked this pull request as ready for review March 15, 2025 06:24
Comment thread bitvm/src/hash/blake3.rs Outdated
Comment thread bitvm/src/hash/blake3.rs Outdated
Comment thread bitvm/src/hash/blake3.rs Outdated
Comment thread bitvm/src/hash/blake3.rs
Comment thread bitvm/src/hash/blake3.rs Outdated
Comment thread bitvm/src/hash/blake3.rs Outdated
Comment thread bitvm/src/hash/blake3.rs
Comment thread bitvm/src/hash/blake3.rs Outdated
Comment thread bitvm/src/lib.rs
Comment thread bitvm/src/lib.rs Outdated

@uncomputable uncomputable left a comment

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.

Since we don't use limb lengths other than 29 and we have an upcoming audit, can we keep the function signatures of the blake3 functions the same and introduce private functions with the more general limb parameter?

  • blake3_compute_script + blake3_compute_script_with_limbs
  • blake3_push_message_script + blake3_push_message_script_with_limbs
  • blake3_verify_output_script + blake3_verify_output_script_with_limbs

(Feel free to choose a different naming scheme)

Also, can you introduce a separate function to unit test different limb lengths? Iterating over all possible limb lengths just makes CI run for much longer. Especially, please use the old method for test_single_byte and test_official_test_vectors. Those tests run 10 seconds in the old version, and they will run for minutes your generalized version.

Comment thread bitvm/src/hash/blake3.rs Outdated
@just-erray

just-erray commented Mar 15, 2025

Copy link
Copy Markdown
Contributor Author

Since we don't use limb lengths other than 29 and we have an upcoming audit, can we keep the function signatures of the blake3 functions the same and introduce private functions with the more general limb parameter?

Renaming it to the first version seems sensible without making it private, since the point is them being usable from outside.

Also, can you introduce a separate function to unit test different limb lengths? Iterating over all possible limb lengths just makes CI run for much longer. Especially, please use the old method for test_single_byte and test_official_test_vectors. Those tests run 10 seconds in the old version, and they will run for minutes your generalized version.

It should only take around 3-4 times as much currently, which only takes 15 seconds in my environment. Extensive tests will be rarely necessary and aren't activated for CI. (Edit: Nevertheless, I'm removing one of the limb lengths from non-extensive tests, that should make it a little bit faster)

Comment thread bitvm/src/hash/blake3.rs
@uncomputable

uncomputable commented Mar 15, 2025

Copy link
Copy Markdown
Contributor

Extensive tests will be rarely necessary and aren't activated for CI

The point of unit tests is to run them, no?

If a test takes too long, we should mark is as #[ignored]. For example, we could adapt the test vectors and single byte units to iterate over all limb sizes and mark these tests as #[ignored]. We can still run these tests via cargo test --ignored but they don't slow down the remaining CI.

@just-erray

Copy link
Copy Markdown
Contributor Author

If a test takes too long, we should mark is as #[ignored]. For example, we could adapt the test vectors and single byte units to iterate over all limb sizes and mark these tests as #[ignored]. We can still run these tests via cargo test --ignored but they don't slow down the remaining CI.

This solution seems better, changing it

@just-erray

Copy link
Copy Markdown
Contributor Author

This solution seems better, so I am changing it.

I couldn't find a better solution than duplicating each function with wrappers in this case, and this seems like an unsustainable and messy approach. If there's a better way to handle this, adding two variants of the tests with #[ignore] would definitely be the better option. If not, I think leaving it as a variable and allowing developers to enable it when something related to limb length changes would be the better approach. In both cases, I don't think there will be any significant differences between them, since developers will have control over running tests while editing the code.

@uncomputable

Copy link
Copy Markdown
Contributor

I will try to come up with a solution and post a gist.

@uncomputable

Copy link
Copy Markdown
Contributor

Feel free to take a look at this: https://gist.github.com/uncomputable/021d98fac2bae02ace0b9005f4ba3484

Run the ignored tests with cargo test -- --ignored.

@just-erray

Copy link
Copy Markdown
Contributor Author

This looks nice. I think editing it slightly so that normal tests run with useful limbs (since it adds at most 10 seconds to the total test time) and ignored tests run with all limb lengths should make it good to go.

@uncomputable

Copy link
Copy Markdown
Contributor

Feel free to arrange the code as you want. I mainly wanted to show that it's possible to ignore the expensive tests without duplicating code.

Comment thread bitvm/src/chunk/wrap_hasher.rs

@uncomputable uncomputable left a comment

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.

ACK 2dcd8ad Looks good to me. Thanks for accommodating my feedback. I haven't run the tests because my laptop is too weak. @AaronFeickert

@RobinLinus
RobinLinus merged commit 6bb49ea into BitVM:main Mar 17, 2025
@just-erray
just-erray deleted the dynamic_limb_length_blake3 branch March 17, 2025 05:30
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.

4 participants