Field generator macros #154
Conversation
kilic
force-pushed
the
field-gen-macro
branch
2 times, most recently
from
ce1512f
to
84edae1
Compare
|
|
||
| let impl_arith_always_const = arith::impl_arith_always_const(&field, num_limbs, inv64); | ||
|
|
||
| let impl_from_uniform_bytes = from_uniform |
There was a problem hiding this comment.
Can we enforce that the elements of from_uniform are within [size, 2*size]?
I think we would get unexpected/buggy behavior if someone accidentally added values outside that range.
There was a problem hiding this comment.
should be handled now in compile time after 851ed19
src/derive/field/tower.rs
Outdated
|
|
||
| impl FromUniformBytes<{ $field::SIZE * 2 }> for $tower { | ||
| fn from_uniform_bytes(bytes: &[u8; $field::SIZE * 2]) -> Self { | ||
| Self::new($field::from_uniform_bytes(bytes), $field::ZERO) |
There was a problem hiding this comment.
I've seen that we had this implementation where the c1 part of the element is fixed to 0 but I can't remember why we needed it in the first place. The tests don't seem to need it. I'd remove it altogether.
src/derive/field/tower.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl FromUniformBytes<{ $field::SIZE * 2 * 2 }> for $tower { |
There was a problem hiding this comment.
The last *2 is easily understandable since we are dealing with a quadratic extension, and therefore we need 2 base field elements. The first one though... not so much.
I find more logical something along the lines of: ( SIZE + k/8 ) * 2) where k is the target bit security.
For the usual case where we have SIZE = 32 and k=128 the result is the same. But for Bls12-381 and Pluto/Eris we will get 48 * 4 = 192 instead of (48+32)*2= 160 and 56*4 = 224 instead of (56+32)*2 = 176.
There was a problem hiding this comment.
Maybe it's necessary to pass this value explicitly to the macro like you do on the prime fields.
| let d_i = fmtid!("d_{}", i); | ||
| gen.extend(quote! { let ( #d_i, carry) = adc(self.0[#i], rhs.0[#i], #carry); }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Can we add the original comment (or something similar):
// Attempt to subtract the modulus, to ensure the value
// is smaller than the modulus.
I think this part is not so obvious so it makes it more understandable.
| } | ||
| } | ||
|
|
||
| fn impl_from_mont(field: &syn::Ident, num_limbs: usize, inv: u64) -> TokenStream { |
There was a problem hiding this comment.
This is new function right?
| } | ||
| } | ||
|
|
||
| impl From<$field> for [u64; 4] { |
There was a problem hiding this comment.
I assume this is now exposed through montgomery_reduce_256 directly, but I think it is good to have the From trait. Is it implemented elsewhere?
| @@ -313,16 +187,10 @@ impl Field for Fp12 { | |||
| } | |||
|
|
|||
| fn sqrt(&self) -> CtOption<Self> { | |||
| // The square root method is typically only required for finding y-coordinate | |||
There was a problem hiding this comment.
I'd rather keep the comment, otherwise looks like this is WIP 😅
| for &b in tmp.as_ref().iter().rev() { | ||
| write!(f, "{:02x}", b)?; | ||
| } |
There was a problem hiding this comment.
This seems to assume little endian, should we conditionally call rev here by endian?
| let right = other.to_repr(); | ||
| left.as_ref().iter() | ||
| .zip(right.as_ref().iter()) | ||
| .rev() |
| impl WithSmallOrderMulGroup<3> for $tower { | ||
| // $field::ZETA ^2 | ||
| const ZETA: Self = $tower { | ||
| c0: ZETA, |
There was a problem hiding this comment.
We could remove the ZETA constant in fq2.rs by using the generated one?
| c0: ZETA, | |
| c0: $field::ZETA, |
This PR adds field generator macros and as a result removes many duplicates. There is an important breaking change that it now wraps all field representations for example as
ReprFq([u8; SIZE]).Also it will help to add new fields and thus new curves. For example base field for BLS12-381 which has 6 limbs internal representation can be added as: