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

Support bw6-761 #188

Merged
merged 27 commits into from
Oct 21, 2023
Merged

Support bw6-761 #188

merged 27 commits into from
Oct 21, 2023

Conversation

liuxiaobleach
Copy link
Contributor

No description provided.

@liuxiaobleach liuxiaobleach changed the title Create bw6-761.json Support bw6-761 Sep 14, 2023
@DmytroTym DmytroTym self-assigned this Sep 14, 2023
@DmytroTym DmytroTym self-requested a review September 14, 2023 12:31
@DmytroTym DmytroTym removed their assignment Sep 14, 2023
@DmytroTym
Copy link
Contributor

I'll try to explain my latest commit and since it's quite a bit of code, I'll mention people who wrote the code I modified or are otherwise necessary to ask
First, this PR aims to add BW6-671 curve to icicle. Only CUDA part is implemented in the latest commit, with wrappers (first and foremost go) still to be written. @liuxiaobleach instead of declaring scalar field for this curve separately, I re-used base field of BLS12-377, hopefully that makes sense. I also needed to add twiddles and other NTT-related info to BLS12-377's base field which is not generated by our script, so we should avoid re-using the script for this curve. But I guess it's the case anyway because of the reversed twiddles for the scalar field @jeremyfelder. More generally, I found our script to be way too restrictive in creating a new curve. I feel like @ChickenLover who is now implementing fields for STAKS might feel the same way. It's nice to have something that helps you generate certain parameters but very often we need to edit script-generated files which is probably not a good sign for this approach
@vhnatyk I removed scalar_field_t and only left scalar_t, I don't really see a reason for the two of them to exist
@ImmanuelSegol I changed the script in several places, some fixes, but also to include nonresidue value that was hardcoded before as well as cherrypicking @LeonHibnik 's PR that added Montgomery values. Also please check my edits to README about how to use the script, some of them are just my personal preferences though tbh
A couple of issues with this commit: first, @jeremyfelder some BLS12-381 CUDA files were edited by the script, seems like all the tests pass, but maybe I should revert these changes. Second, @jeremyfelder 4 coset evaluation themed tests fail for BLS12-377, I'm assuming because of the flipped twiddles. @ChickenLover I had to comment out poseidon_multi_cuda_bls12_381 function and corresponding test because there's no extern equivalent on the CUDA side

icicle/curves/index.cu Show resolved Hide resolved
src/test_bls12_381.rs Outdated Show resolved Hide resolved
@jeremyfelder
Copy link
Collaborator

first, @jeremyfelder some BLS12-381 CUDA files were edited by the script, seems like all the tests pass, but maybe I should revert these changes.

This is fine. I think those functions were missing from this curve since we haven't run the generation script on all curves in a little bit.

Second, @jeremyfelder 4 coset evaluation themed tests fail for BLS12-377, I'm assuming because of the flipped twiddles.

I assume you are talking about Rust tests. Makes sense from flipped twiddles. Shouldn't be too difficult to add an option to reverse the twiddles in the short term. Long term we the solution of generating twiddles on the fly.

@DmytroTym
Copy link
Contributor

Closes #191 and #113

src/curves/bw6_761.rs Outdated Show resolved Hide resolved
src/test_bw6_761.rs Outdated Show resolved Hide resolved
DmytroTym
DmytroTym previously approved these changes Oct 19, 2023
Copy link
Contributor

@DmytroTym DmytroTym left a comment

Choose a reason for hiding this comment

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

This is not a fully complete implementation of BW6-671 as Rust wrappers are still missing, but for the sake of C++ and Golang I think this can be merged into main. One slight issue I have with Golang wrappers is that the tests are pretty weak and don't cover all functionality as well as don't check correctness agains gnark (or any other CPU library). This is because interaction with gnark has been factored into the iciclegnark repo.
Either way, we have correctness in gnark's groth16 prover, and IMO it's best to address aforementioned issues as part of the new API.

ImmanuelSegol
ImmanuelSegol previously approved these changes Oct 19, 2023
@jeremyfelder jeremyfelder merged commit fd62fe5 into ingonyama-zk:main Oct 21, 2023
10 of 11 checks passed
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.

None yet

4 participants