refactor(fiat-crypto): Inversion #66
Conversation
|
We should avoid editing the generated code, and only write wrappers around it. Otherwise, if we ever need to regenerate, we won't be able to apply the changes cleanly. This is kind of annoying to work around, considering that the generated code generates C-style APIs, but it's important for long-term maintainability and verifiability. |
agreed, fixed! |
src/fields/u32/fp/wrapper.rs
Outdated
| let mut d = 1; | ||
| let mut f: [u32; 13] = [0u32; 13]; | ||
| fiat::fp_msat(&mut f); | ||
| let mut g = [0u32; 13]; |
There was a problem hiding this comment.
I think it might be clearer in these inversion methods to define a constant N_LIMBS so it's clear that e.g. 13 is N_LIMBS + 1, 12 is N_LIMBS, etc.
There was a problem hiding this comment.
added global constants NUM_LIMBS and PRIME_ORDER
src/fields/u32/fp/wrapper.rs
Outdated
| let mut i = 0; | ||
| let mut j = 0; | ||
|
|
||
| while j < 6 { |
There was a problem hiding this comment.
I think this should be 12 instead of 6 (to review I was comparing with the impl_bernstein_yang_invert macro where j goes up to n_limbs)
There was a problem hiding this comment.
This is a good catch! Fixed by changing limbs to a constant.
src/fields/u32/fp/wrapper.rs
Outdated
| use super::*; | ||
|
|
||
| #[test] | ||
| fn inversion_test() { |
There was a problem hiding this comment.
It would be cool if these were proptests i.e. something like:
fn fp_strategy() -> BoxedStrategy<Fp> {
any::<[u8; 48]>()
.prop_map(|bytes| Fp::from_bytes(&bytes))
.boxed()
}
proptest! {
#[test]
fn inversion_proptest(element in fp_strategy().prop_filter("Non-zero element", |e| e != &Fp::zero())) {
let inverse = element.inverse().unwrap();
assert_eq!(element.mul(inverse), Fp::one());
}
}There was a problem hiding this comment.
Good point. In #67, we implement a proptest for inversion, but I'll include this test as well.
|
Per the recent indexing changes (packing two @cronokirby let's carry over the inversion proptest to |
References #65.
Referenced BLS12-377 curve construction parameters from ZEXE paper (
p.44). Retrieved BLS12-377 twisted edwards curve construction parameters from here.Note: I slightly modified the input bounds of the fiat-generated
divstepto make the output bounds explicitly return. This is merely a semantic change that simplifies the iterations in theinversefunction.Update: This change has been reverted.