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

Merged
merged 1 commit into from
Jan 13, 2025
Merged

fix: use u64 in BaseSplitGenerator #1647

merged 1 commit into from
Jan 13, 2025

Conversation

ed255
Copy link
Contributor

@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.

@ed255
Copy link
Contributor Author

ed255 commented Jan 13, 2025

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.

Thanks for this insight. Could you point me to some other areas where the library assumes that usize is 64 bits?

We have not done extensive usage of the library yet, but currently we're able to run a recursive circuit that does some poseidon hashes plus some basic logic in wasm and the verification passes successfully with the patch in this PR.

If we find more issues related to running plonky2 in a 32-bit architecture, are you interested in receiving the fixes?

@Nashtare
Copy link
Collaborator

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.

Thanks for this insight. Could you point me to some other areas where the library assumes that usize is 64 bits?

I don't have any specific line that comes to mind right away, but we often (if not always) used u64 / usize interchangeably, I meant it mostly as a caution note in case you'd expect your application to run bug free.

If we find more issues related to running plonky2 in a 32-bit architecture, are you interested in receiving the fixes?

Absolutely! Feel free to submit PRs fixing those, even if 32-bit architectures aren't our priority, we definitely wouldn't want projects built on plonky2 to be blocked because of such issues.

@Nashtare Nashtare merged commit 892f51b into 0xPolygonZero:main Jan 13, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants