-
Notifications
You must be signed in to change notification settings - Fork 138
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
improve: add some macros to generate big testing suite of curves #129
improve: add some macros to generate big testing suite of curves #129
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be testing the PrimeFiedBits
remember! Also we need to see if for BN is necessary to add further testing for lookup-based ops or small scalars.
Also, each curve & Field should have tests for their associated constants to check they're correct.
Make sure all of the constants are indeed checked! :)
Aside from that, the rest looks really nice! I like the direction this is going towards!
Just left some comments :)
Changes look much better @duguorong009 ! I like them a lot! Let's try to extend it to the Field tests and see how the solution looks like! |
@CPerezz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having a #[cfg(test)]
on each test fn,
can't you have a module which is under the test flag and have all the macros without it inside.
I talk about thinks like this:
halo2curves/src/bn256/curve.rs
Lines 201 to 273 in 762a329
#[cfg(test)] | |
crate::curve_testing_suite!(G1, G2); | |
#[cfg(test)] | |
crate::curve_testing_suite!(G1, "hash_to_curve"); | |
#[cfg(test)] | |
crate::curve_testing_suite!(G1, "endo_consistency"); | |
#[cfg(test)] | |
crate::curve_testing_suite!( | |
G1, | |
"endo", | |
[ | |
0x8b17ea66b99c90dd, | |
0x5bfc41088d8daaa7, | |
0xb3c4d79d41a91758, | |
0x00, | |
] | |
); | |
#[cfg(test)] | |
crate::curve_testing_suite!( | |
G1, | |
"svdw_map_to_curve", | |
( | |
// Precomputed constants taken from https://github.com/ConsenSys/gnark-crypto/blob/441dc0ffe639294b8d09e394f24ba7575577229c/internal/generator/config/bn254.go#L26-L32. | |
[ | |
"4", | |
"10944121435919637611123202872628637544348155578648911831344518947322613104291", | |
"8815841940592487685674414971303048083897117035520822607866", | |
"7296080957279758407415468581752425029565437052432607887563012631548408736189", | |
], | |
// List of (u, (Q.x, Q.y)) taken from https://github.com/ConsenSys/gnark-crypto/blob/441dc0ffe639294b8d09e394f24ba7575577229c/ecc/bn254/hash_vectors_test.go#L4-L28 | |
[ | |
( | |
"0xcb81538a98a2e3580076eed495256611813f6dae9e16d3d4f8de7af0e9833e1", | |
( | |
"0x1bb8810e2ceaf04786d4efd216fc2820ddd9363712efc736ada11049d8af5925", | |
"0x1efbf8d54c60d865cce08437668ea30f5bf90d287dbd9b5af31da852915e8f11", | |
), | |
), | |
( | |
"0xba35e127276e9000b33011860904ddee28f1d48ddd3577e2a797ef4a5e62319", | |
( | |
"0xda4a96147df1f35b0f820bd35c6fac3b80e8e320de7c536b1e054667b22c332", | |
"0x189bd3fbffe4c8740d6543754d95c790e44cd2d162858e3b733d2b8387983bb7", | |
), | |
), | |
( | |
"0x11852286660cd970e9d7f46f99c7cca2b75554245e91b9b19d537aa6147c28fc", | |
( | |
"0x2ff727cfaaadb3acab713fa22d91f5fddab3ed77948f3ef6233d7ea9b03f4da1", | |
"0x304080768fd2f87a852155b727f97db84b191e41970506f0326ed4046d1141aa", | |
), | |
), | |
( | |
"0x174d1c85d8a690a876cc1deba0166d30569fafdb49cb3ed28405bd1c5357a1cc", | |
( | |
"0x11a2eaa8e3e89de056d1b3a288a7f733c8a1282efa41d28e71af065ab245df9b", | |
"0x60f37c447ac29fd97b9bb83be98ddccf15e34831a9cdf5493b7fede0777ae06", | |
), | |
), | |
( | |
"0x73b81432b4cf3a8a9076201500d1b94159539f052a6e0928db7f2df74bff672", | |
( | |
"0x27409dccc6ee4ce90e24744fda8d72c0bc64e79766f778da0c1c0ef1c186ea84", | |
"0x1ac201a542feca15e77f30370da183514dc99d8a0b2c136d64ede35cd0b51dc0", | |
), | |
), | |
] | |
) | |
); |
Which hold a lot of unnecessary #[cfg(test)]
.
src/bn256/curve.rs
Outdated
[ | ||
0x8b17ea66b99c90dd, | ||
0x5bfc41088d8daaa7, | ||
0xb3c4d79d41a91758, | ||
0x00, | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we import this from the EndoParams directly?
I see it risky to duplicate constants arround.
Also, please add some comments if there's no alternative to duplication in order to know what that is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we compute it by ourseleves here or something similar? I think it makes it complex if we indeed need to pass more parameters to the macro.
Maybe if it gets too complex we can leave it as is and adding a comment saying how the number was obtained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the z_other
in this test quite absurd.
It is checking that a variable that is declared in the test is the other 3-root of unity and nothing else.
It doesn't even check that is not the value being used as ZETA
.
I would remove this variable altogether and remove it from the macro as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I missed the outdated tag, just saw that you did exactly this. Great! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CPerezz @davidnevadoc
Here, I tried to add the comments. 9ee6301
src/grumpkin/curve.rs
Outdated
G1, | ||
"endo", | ||
[ | ||
0xe4bd44e5607cfd48, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed z_other
since it is ZETA
, in essence. f7f5aa8
src/tests/curve.rs
Outdated
assert_eq!(affine_point, affine_point_rec); | ||
} | ||
} | ||
macro_rules! projective_to_affine_affine_to_projective { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
macro_rules! projective_to_affine_affine_to_projective { | |
macro_rules! projective_affine_roundtrip { |
Is shorter and means the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 71069a7
Thanks for the comments! @CPerezz BTW, I think it is better to add only changes for curve testing, in this PR. |
Sounds good to me!!! Address all the stuff and set this to ready and I'll review ASAP! On the meantime, you can branch out from here and start the |
Try to remove the unnecessary attrs in d205675 |
src/tests/curve.rs
Outdated
($c: ident) => { | ||
assert!(bool::from($c::identity().is_on_curve())); | ||
assert!(bool::from($c::generator().is_on_curve())); | ||
assert!(bool::from($c::identity().is_on_curve())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these checks duplicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for indication! @davidnevadoc
Here, I removed the duplicates. 9ee6301
src/bn256/curve.rs
Outdated
[ | ||
0x8b17ea66b99c90dd, | ||
0x5bfc41088d8daaa7, | ||
0xb3c4d79d41a91758, | ||
0x00, | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the z_other
in this test quite absurd.
It is checking that a variable that is declared in the test is the other 3-root of unity and nothing else.
It doesn't even check that is not the value being used as ZETA
.
I would remove this variable altogether and remove it from the macro as well.
@CPerezz @davidnevadoc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
Great work!! 🥳
src/tests/curve.rs
Outdated
println!( | ||
"{:?} \n{:?}", | ||
projective_repr.as_ref(), | ||
affine_repr.as_ref() | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, removing now. ( 😅 )
d13e9dd
53dd906
Description
Related issues
Changes
curve_testing_suite
macro for tests of curves