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

fix: use u64 in BaseSplitGenerator #1647

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

ed255
Copy link

@ed255 ed255 commented Jan 10, 2025

The BaseSplitGenerator was casting fields (which fit in u64) into usize. This doesn't work in 32-bit architectures where usize is 32 bits, like wasm32. Keeping the value in u64 allows it to work in 32-bit architectures.

We found this error while trying to run a recursive circuit in wasm, where the verification was failing. This issue can be easily tested on an amd64 linux machine by compiling with the target i686 and running the tests. Before this PR these are the tests that fail:

$ cargo test --release --target i686-unknown-linux-gnu
[...]
failures:
    batch_fri::oracle::test::batch_prove_openings
    recursion::conditional_recursive_verifier::tests::test_conditional_recursive_verifier
    recursion::cyclic_recursion::tests::test_cyclic_recursion
    recursion::recursive_verifier::tests::test_recursive_recursive_verifier
    recursion::recursive_verifier::tests::test_recursive_verifier
    recursion::recursive_verifier::tests::test_recursive_verifier_multi_hash
    recursion::recursive_verifier::tests::test_recursive_verifier_one_lookup
    recursion::recursive_verifier::tests::test_recursive_verifier_too_many_rows
    recursion::recursive_verifier::tests::test_recursive_verifier_two_luts

test result: FAILED. 85 passed; 9 failed; 1 ignored; 0 measured; 0 filtered out; finished in 59.47s

Note that I disabled jemalloc to be able to run in 32-bit mode.

After the fix all tests pass in 32-bit mode.

The BaseSplitGenerator was casting fields (which fit in u64) into usize.
This doesn't work in 32-bit architectures where usize is 32 bits, like
wasm32.  Keeping the value at u64 allows it to work in 32-bit
architectures.
Copy link
Collaborator

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

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

The library implies widely that all casts to usize should be not interpreted as 32-bit casts. I'm surprised this gate is your only point of failure, as several other places of the codebase would be broken on a true 32-bit architecture.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants