Merge nonnative #79
Merge nonnative #79
Conversation
Co-authored-by: Nicholas Ward <[email protected]>
Add support for `no_std`, plus some `clippy` fixes
Rest of the clippy fixes
Fix `is_eq`
Tweaks in example in `README.md`
The prior PR (#9) multiplied by `self.z` instead of `self.z.inverse()`.
This is related to a Zexe issue arkworks-rs/snark#289 Basically, ToBitsGadget for FpVar may produce shorter output for a constant value, which is undesired for a few reasons.
* Fix pre_eq_reduce The pre_eq_reduce should directly reduce the value to the normal form rather than checking it. * fmt
* allow cs being none for into_edwards
Co-authored-by: Pratyush Mishra <[email protected]> Co-authored-by: weikeng <[email protected]>
Co-authored-by: Daira Hopwood <[email protected]>
src/fields/nonnative/README.md
Outdated
| let res = (&a_times_b + &c_times_d)?.reduce()?; | ||
| ``` | ||
|
|
||
| It performs only one *reduce* operation and is roughly 2x faster than the first implementation. |
There was a problem hiding this comment.
Why not have the default multiplication output NonNativeFieldMulResultVar, and reduce the inputs to multiplication implicitly? (You would need to keep track of potential overflow from long sequences of additions, and perhaps also handle reduction automatically in that case, but I think that's also a hazard with the current API.)
There was a problem hiding this comment.
For API reasons we can't return NonNativeFieldMulResultVar directly (the bound on FieldVar requires returning Self, which is NonNativeFieldVar. But I guess internally we could track the reduction state, and reduce if we invoke operations which require the canonical form (eg: to_bytes). cc @weikengchen ?
src/fields/nonnative/mod.rs
Outdated
| pub mod reduce; | ||
|
|
||
| /// a macro for computing ceil(log2(x)) for a field element x | ||
| #[doc(hidden)] | ||
| #[macro_export] | ||
| macro_rules! overhead { |
There was a problem hiding this comment.
btw @weikengchen is there a reason these two are public? AFAIK they are not used outside the crate, right?
There was a problem hiding this comment.
I checked our use of these two modules.
-
paramshas been used somewhere else: https://github.com/arkworks-rs/sponge/blob/c9f7d24a5f10c415e9370a079cb060d95400b938/src/constraints/mod.rs#L19 (bits_le_to_nonnative) and https://github.com/arkworks-rs/crypto-primitives/blob/1a71386a10ef627834e4c3b14ca982811f23192d/src/snark/constraints.rs (NonNativeFieldInputVar). -
I think it is okay to make
reduceprivate.
I feel both should be private. And the two use cases above may suggest that we should move NonNativeFieldInputVar and bits_le_to_nonnative into here, and the external world should just call them. What do you think?
There was a problem hiding this comment.
I made the overhead macro and the reduce module private, and will file an issue for the other two.
Description
Add
NonnativeFieldVartor1cs-std.Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pendingsection inCHANGELOG.mdFiles changedin the Github PR explorer