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: speed up sqrt and inversion for FP, FP2, using unconstrained #17

Merged
merged 2 commits into from
Aug 21, 2024

Conversation

huitseeker
Copy link

@huitseeker huitseeker commented Aug 16, 2024

  • Added new optional 'sphinx-zkvm' dependency and 'zkvm' feature in Cargo.toml
  • Modified sqrt method in Fp2 file, adding differentiated execution logic based on the target_os
  • Adjusted Fp2 operations to handle quadratic nonresidue during the square root computation effectively and to conform to environment specifications.

This is just a thought experiment: it seems we're not actively using either.

@huitseeker huitseeker requested a review from wwared August 16, 2024 17:49
@wwared wwared force-pushed the unconstrained_zkvm_invert branch from 22045d0 to 061e266 Compare August 16, 2024 19:11
@huitseeker huitseeker force-pushed the unconstrained_zkvm_invert branch 22 times, most recently from 313ebb3 to c5e40f9 Compare August 21, 2024 19:48
- Added new optional 'sphinx-zkvm' dependency and 'zkvm' feature in Cargo.toml
- Modified `sqrt` method in `Fp2` file, adding differentiated execution logic based on the `target_os`
- Adjusted `Fp2` operations to handle quadratic nonresidue during the square root computation effectively and to conform to environment specifications.
- Modified the `Cargo.toml` file to restrict `sphinx-zkvm` dependency to a `zkvm` target OS context, mitigating potential circular dependency issues
- Removed `zkvm` from the feature flags in the `Cargo.toml` file
- Introduced a `sqrt_impl` macro in `src/fp2.rs` to refactor the square root calculations in `sqrt` method
- Did the same with a inv_impl for the inverse
@huitseeker huitseeker force-pushed the unconstrained_zkvm_invert branch from 2d4992b to d162718 Compare August 21, 2024 19:52
Copy link

@wwared wwared left a comment

Choose a reason for hiding this comment

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

I believe the remaining CI failures for the LCs are due to the Cargo.lock versions in the LC repo and should be fixed by a cargo update follow up PR in the LC repo

Thanks for tackling this!

@huitseeker huitseeker merged commit d5eb6f6 into zkvm Aug 21, 2024
9 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.

2 participants