improve: add some macros to generate big testing suite of fields #131

duguorong009 · 2024-01-24T12:08:52Z

Description

Create some macros to generate the big testing suites for all of fields in the lib

Related issues

Upgrade testing across the lib #76

Changes

Add the field_testing_suite macro for tests of fields
Create new macros for testing
Replace the functions used for testing, with new macros

davidnevadoc

That -700 lines is really good to see! 👍
I have given the PR a pass and it looks good so far. I've left a couple of suggestions.
Will approve it as I check the parts that I'm missing.
Nice work :)

davidnevadoc · 2024-01-28T16:02:49Z

src/tests/field.rs

+        macro_rules! random_multiplication_tests {
+            ($f: ident, $rng: expr) => {
+                let _message = format!("multiplication {}", stringify!($f));
+                let start = start_timer!(|| _message);


I think these timers are quite unnecessary so I suggest removing them form all tests.
We can get rid of the ark-std dev-dependency as well.

Remove the unnecessary timers in b549998

Regarding the ark-std dep, it is used in several places.
For example, it is for feature print-trace, msm testing, and bn256::Fr benchmarking
I think we should keep ark-std dep because of these usages.

Oh, you're right, I forgot about those. Let's keep it then.

davidnevadoc · 2024-01-28T16:06:02Z

src/tests/field.rs

+#[macro_export]
+macro_rules! field_testing_suite {
+    ($field: ident, "field_arithmetic") => {
+        macro_rules! random_multiplication_tests {


Is there a reason for having these inner macros?
The outer macro is really nice, but I think these ones could be normal functions and the code would be simpler. I've made a suggestion here: davidnevadoc@a81fe26

Done f54b70c

CPerezz

LGTM! Thanks for this good work! 👍

CPerezz · 2024-02-01T14:48:57Z

src/bn256/fq12.rs

+            0x3c208c16d87cfd47,
+            0x97816a916871ca8d,
+            0xb85045b68181585d,
+            0x30644e72e131a029,


Can we comment how we obtained this? Or from where?

@CPerezz
I add the comment regarding this frobenius power param in 4b5ba0c .
Pls check & gimme the comments.

CPerezz · 2024-02-01T14:49:15Z

src/bn256/fq2.rs

+        [
+            0x3c208c16d87cfd47,
+            0x97816a916871ca8d,
+            0xb85045b68181585d,
+            0x30644e72e131a029,
+        ]


CPerezz · 2024-02-01T14:49:26Z

src/bn256/fq6.rs

+        [
+            0x3c208c16d87cfd47,
+            0x97816a916871ca8d,
+            0xb85045b68181585d,
+            0x30644e72e131a029,
+        ]


CPerezz · 2024-02-01T14:49:44Z

src/pluto_eris/fields/fp12.rs

+            0x9ffffcd300000001,
+            0xa2a7e8c30006b945,
+            0xe4a7a5fe8fadffd6,
+            0x443f9a5cda8a6c7b,
+            0xa803ca76f439266f,
+            0x0130e0000d7f70e4,
+            0x2400000000002400,


CPerezz · 2024-02-01T14:49:51Z

src/pluto_eris/fields/fp2.rs

+            0x9ffffcd300000001,
+            0xa2a7e8c30006b945,
+            0xe4a7a5fe8fadffd6,
+            0x443f9a5cda8a6c7b,
+            0xa803ca76f439266f,
+            0x0130e0000d7f70e4,
+            0x2400000000002400,


CPerezz · 2024-02-01T14:50:03Z

src/pluto_eris/fields/fp6.rs

+        [
+            0x9ffffcd300000001,
+            0xa2a7e8c30006b945,
+            0xe4a7a5fe8fadffd6,
+            0x443f9a5cda8a6c7b,
+            0xa803ca76f439266f,
+            0x0130e0000d7f70e4,
+            0x2400000000002400,
+        ]


davidnevadoc

Nice work, LGTM!

adria0

Amazing refactor! :)

is assert_eq!(Fr::DELTA, GENERATOR.pow([1u64 << Fr::S])) implemented? I only see it for MULTIPLICATIVE_GENERATOR

adria0 · 2024-02-02T10:52:55Z

src/secp256r1/fq.rs

+    crate::field_testing_suite!(Fq, "conversion");
+    crate::field_testing_suite!(Fq, "serialization");
+    crate::field_testing_suite!(Fq, "quadratic_residue");
+    // crate::field_testing_suite!(Fq, "bits");


Oops, I forgot it. 😅
Thanks for picking! @adria0
Remove it in 91a3f9e.

duguorong009 · 2024-02-02T12:26:15Z

Amazing refactor! :)

is assert_eq!(Fr::DELTA, GENERATOR.pow([1u64 << Fr::S])) implemented? I only see it for MULTIPLICATIVE_GENERATOR

I get rid of this test on purpose.
The reason is that the MULTIPLICATIVE_GENERATOR is, in essence, the GENERATOR.
You can check it here.
https://github.com/privacy-scaling-explorations/halo2curves/blob/main/src/bn256/fr.rs#L263

duguorong009 added 14 commits January 24, 2024 10:31

feat: add the basic "field_testing_suite" macro

1f9d74b

feat: add the macro branches for common field tests

0b75e1b

fix: add more tests to some fields

d353b11

chore: remove useless clippy rule

06d5101

feat: add the macro of testing "bits"(PrimeFieldBits)

cd26f1d

feat: add the macro branch for "serialization_check" testing

8fa2c06

feat: add the macro branch for testing constants

4dfc897

feat: add the macro branch to test the sqrt

0bc508e

feat: add the macro branch to test the "ZETA" constant

5683c2c

chore: rename the test into "field_arithmetic"

ddfbaee

feat: add the macro branch to test the "frobenius"

5b06cce

feat: add the macro for some extension field(Fp2, Fq2) tests

fec2407

feat: add the macro for some extension field(Fp6, Fq6) tests

6dc4660

feat: add the macro for some extension field(Fp12, Fq12) tests

fd73fb9

duguorong009 marked this pull request as ready for review January 25, 2024 17:03

duguorong009 requested review from CPerezz and davidnevadoc January 25, 2024 17:03

feat: add the macro for testing the "from_uniform_bytes"

a801e59

davidnevadoc reviewed Jan 28, 2024

View reviewed changes

duguorong009 added 4 commits January 29, 2024 10:10

chore: remove the unnecessary ark_std timer

b549998

chore: fmt

7771e6a

refactor: roll back some macros to functions

f54b70c

refactor: move the tests into "test" module

fb60967

duguorong009 requested a review from davidnevadoc January 29, 2024 11:58

Merge branch 'main' into gr@refactor-tests-1

224b5b3

duguorong009 requested a review from adria0 February 1, 2024 14:18

CPerezz approved these changes Feb 1, 2024

View reviewed changes

chore: add comment about frobenius testing param

4b5ba0c

davidnevadoc approved these changes Feb 2, 2024

View reviewed changes

adria0 reviewed Feb 2, 2024

View reviewed changes

chore: get rid of leftover

91a3f9e

duguorong009 added 2 commits February 3, 2024 00:24

fix: reduce the testing counts for extension field in "test_field"

3d587a8

chore: add the comment for reducing number of tests in "test_field"

b7c21ad

davidnevadoc approved these changes Feb 4, 2024

View reviewed changes

duguorong009 added this pull request to the merge queue Feb 5, 2024

Merged via the queue into privacy-scaling-explorations:main with commit f96f7f9 Feb 5, 2024
11 checks passed

CPerezz mentioned this pull request Feb 8, 2024

bump: v0.6.1 #138

Merged

jonathanpwang mentioned this pull request Apr 24, 2024

[feat] v0.6.1 from upstream (cyclone MSM) axiom-crypto/halo2curves#27

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve: add some macros to generate big testing suite of fields #131

improve: add some macros to generate big testing suite of fields #131

duguorong009 commented Jan 24, 2024

davidnevadoc left a comment

davidnevadoc Jan 28, 2024

duguorong009 Jan 29, 2024 •

edited

Loading

davidnevadoc Jan 29, 2024

davidnevadoc Jan 28, 2024

duguorong009 Jan 29, 2024

CPerezz left a comment

CPerezz Feb 1, 2024

duguorong009 Feb 2, 2024 •

edited

Loading

CPerezz Feb 1, 2024

CPerezz Feb 1, 2024

CPerezz Feb 1, 2024

CPerezz Feb 1, 2024

CPerezz Feb 1, 2024

davidnevadoc left a comment

adria0 left a comment

adria0 Feb 2, 2024

duguorong009 Feb 2, 2024

duguorong009 commented Feb 2, 2024

improve: add some macros to generate big testing suite of fields #131

improve: add some macros to generate big testing suite of fields #131

Conversation

duguorong009 commented Jan 24, 2024

Description

Related issues

Changes

davidnevadoc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duguorong009 Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPerezz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duguorong009 Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidnevadoc left a comment

Choose a reason for hiding this comment

adria0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duguorong009 commented Feb 2, 2024

duguorong009 Jan 29, 2024 •

edited

Loading

duguorong009 Feb 2, 2024 •

edited

Loading