Deterministic tests #345

adria0 · 2024-05-31T15:54:01Z

Make tests deterministic

Create new crate halo2_debug with some utilities, it can be reused for Adding debugging & testing utils #343
Instead using OsRng, use a test_rng() fo random and a one_rng() for deterministic randomness.
Only use rand_chacha for seeded randomness.
Remove integrated dhat profiling, it is activated in cargo test in M3 as also in coverage, creating crashes (since it's already activated). Also remove the associated heap-profining feature (that is activated with --all-features)

…into feat/deterministic_tests

codecov-commenter · 2024-06-03T08:28:04Z

Codecov Report

Attention: Patch coverage is 80.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 82.08%. Comparing base (80e69b9) to head (a309ab1).
Report is 1 commits behind head on main.

Files	Patch %	Lines
halo2_debug/src/lib.rs	65.21%	8 Missing ⚠️
halo2_backend/src/poly/ipa/commitment.rs	77.77%	2 Missing ⚠️
halo2_backend/src/poly/kzg/commitment.rs	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #345      +/-   ##
==========================================
+ Coverage   81.90%   82.08%   +0.18%     
==========================================
  Files          82       83       +1     
  Lines       17019    17218     +199     
==========================================
+ Hits        13939    14133     +194     
- Misses       3080     3085       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ed255 · 2024-06-03T11:12:50Z

15,756 lines diff is a lot of lines to review!
Maybe you can remove halo2_proofs/dhat-heap.json :P

adria0 · 2024-06-03T11:38:15Z

15,756 lines diff is a lot of lines to review! Maybe you can remove halo2_proofs/dhat-heap.json :P

what a fancy PR! let's me remove it.

update: removed in ed4c33b

ed255

Taken a first look, left some comments and questions :)

halo2_debug/src/lib.rs

halo2_proofs/tests/frontend_backend_split.rs

halo2_debug/src/lib.rs

ed255 · 2024-06-03T15:37:01Z

halo2_debug/src/lib.rs

+// Check the a test proof against a known hash
+// Note that this function is only called in CI in "cargo test --all-fetaures"
+pub fn assert_test_proof<D: AsRef<[u8]>>(hex: &str, data: D) {
+    if cfg!(all(feature = "thread-safe-region", not(coverage))) {


I have two questions about this:

The halo2_debug crate doesn't have any feature, which makes me think this is never activated

What's the rationale behind using thread-safe-region for this? When this feature is not enabled in the frontend, a region is not thread safe, and thus parallel witness generation is not supported; but it this feature shouldn't affect the determinism of the proof generation.

Cause I'm getting different proofs if I activate test_mycircuit_full_legacy with thread-safe-region!

Cause I'm getting different proofs if I activate test_mycircuit_full_legacy with thread-safe-region!

I see, we should investigate why this is happening.

Also please take a look at my point 1 :) Let me know if I'm missing something, but I tried running the tests with --all-features and the assertion was not triggered.

halo2_proofs/tests/frontend_backend_split.rs

ed255 · 2024-06-04T08:51:13Z

halo2_debug/src/lib.rs

+use rand_core::OsRng;
+use tiny_keccak::Hasher;
+
+// One number generator, that can be used as a deterministic Rng, outputing fixed values.


BTW, the comments you added could be done with /// and then they become documentation.

davidnevadoc

I have a doubt about the scope of the PR.
This PR intend makes the tests deterministic, but does it intend to make the proof generation deterministic too? 🤔
I like the idea of having the random input of tests determined by a fixed seed, so that the tests can be easily reproduced. However, having the same mechanism for the random values of the protocol, like the blinders, may be a security issue. What is the idea for this type of randomness? @adria0 @ed255

davidnevadoc · 2024-06-04T20:50:53Z

halo2_backend/src/poly/kzg/commitment.rs

-    fn new(k: u32) -> Self {
-        Self::setup(k, OsRng)
+    fn new(k: u32, rng: impl RngCore) -> Self {
+        Self::setup(k, rng)


Can we put the new rng here directly (in setup()) instead of having it in new()?.
We just want randomness in the setup of mock KZG parameters, but we don't need it in IPA so IMO it's better not to modify the common interface of ParamsProver.

davidnevadoc · 2024-06-04T20:52:19Z

halo2_backend/src/poly/kzg/commitment.rs

        use crate::poly::EvaluationDomain;
        use halo2curves::bn256::{Bn256, Fr};

        let engine = H2cEngine::new();
-        let params = ParamsKZG::<Bn256>::new(K);
+        let params = ParamsKZG::<Bn256>::new(K, test_rng());


This randomness is used to generate mock params for testing, so test_rng() is fine here.

davidnevadoc · 2024-06-04T20:54:22Z

halo2_backend/src/poly/kzg/commitment.rs

@@ -477,7 +476,7 @@ mod test {

        let b = domain.lagrange_to_coeff(a.clone());

-        let alpha = Blind(Fr::random(OsRng));
+        let alpha = Blind(Fr::random(test_rng()));


But this randomness is part of the real protocol, here we need some security guarantees so we probably want to use a different randomness source rather than test_rng(). I think keeping OsRng here is fine.

davidnevadoc · 2024-06-04T20:55:05Z

halo2_backend/src/poly/multiopen_test.rs

@@ -256,7 +256,7 @@ mod test {

        let mut transcript = T::init(vec![]);

-        let blind = Blind::new(&mut OsRng);
+        let blind = Blind::new(&mut test_rng());


Same as previous comment, this randomness is not part of the testing.

davidnevadoc · 2024-06-04T20:55:41Z

halo2_backend/src/poly/multiopen_test.rs

@@ -297,7 +297,7 @@ mod test {

        let prover = P::new(params);
        prover
-            .create_proof(&mut OsRng, &mut transcript, queries)
+            .create_proof(&mut test_rng(), &mut transcript, queries)


Not 100% on this one, but I'd say its not part of testing either.

davidnevadoc · 2024-06-04T20:57:51Z

halo2_middleware/Cargo.toml

@@ -35,7 +35,7 @@ rayon = "1.8"
 ark-std = { version = "0.3" }
 proptest = "1"
 group = "0.13"
-rand_xorshift = "0.3.0"
+rand_chacha = "0.3.0"


Why is chacha used instead of xorshift?
I believe we are using xorshift in halo2curves. Should we make the switch there too?

Mainly just having one seed randomness generator for the whole halo2 package. Makes any sense to use more than one?

Just to use the same seed random generator in the whole halo2 package. I do not see the point to use two if not is really needed.

adria0 · 2024-06-05T07:38:41Z

I have a doubt about the scope of the PR. This PR intend makes the tests deterministic, but does it intend to make the proof generation deterministic too? 🤔 I like the idea of having the random input of tests determined by a fixed seed, so that the tests can be easily reproduced. However, having the same mechanism for the random values of the protocol, like the blinders, may be a security issue. What is the idea for this type of randomness? @adria0 @ed255

Also, this is not related to determinitic tests, so I remove it, rethink, and make a future PR, if needed.

adria0 · 2024-06-05T07:45:50Z

@ed255 @davidnevadoc I'll be rolling back some changes and I prefer to replace this PR to keep clean history again.

adria0 added 8 commits May 27, 2024 11:26

Improve ZAL

afbf46d

Small fixes

1a7d21b

test inputs

426231e

Deterministic tests

8a04849

Merge branch 'main' of github.com:privacy-scaling-explorations/halo2 …

38d6fa1

…into feat/deterministic_tests

test_rng, thread-safe-region failing

c6991f0

Fix CI

24b5deb

tune coverage

bfbbcf1

adria0 force-pushed the feat/deterministic_tests branch from c2b5fbc to bfbbcf1 Compare June 3, 2024 08:20

adria0 marked this pull request as ready for review June 3, 2024 08:30

adria0 mentioned this pull request Jun 3, 2024

[Meta] Improve unit test coverage #327

Closed

ed255 mentioned this pull request Jun 3, 2024

feat: add halo2_debug package #346

Merged

adria0 mentioned this pull request Jun 3, 2024

improve: apply tachyon optimizations(1) #342

Merged

adria0 added 2 commits June 3, 2024 13:43

Remove leftover

ed4c33b

Remove integrated heap profiling. Rename crate to halo2_debug.

0c54819

adria0 force-pushed the feat/deterministic_tests branch from a1eaa6d to 0c54819 Compare June 3, 2024 13:34

ed255 reviewed Jun 3, 2024

View reviewed changes

halo2_debug/src/lib.rs Show resolved Hide resolved

halo2_proofs/tests/frontend_backend_split.rs Outdated Show resolved Hide resolved

halo2_debug/src/lib.rs Outdated Show resolved Hide resolved

ed255 reviewed Jun 3, 2024

View reviewed changes

Use same vector test for split circuits

a309ab1

ed255 reviewed Jun 4, 2024

View reviewed changes

davidnevadoc requested changes Jun 4, 2024

View reviewed changes

adria0 force-pushed the feat/deterministic_tests branch from 8046baf to 4512d61 Compare June 5, 2024 06:57

Simplify

22f7f7e

adria0 force-pushed the feat/deterministic_tests branch from 4512d61 to 22f7f7e Compare June 5, 2024 07:21

adria0 closed this Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deterministic tests #345

Deterministic tests #345

adria0 commented May 31, 2024 •

edited

Loading

codecov-commenter commented Jun 3, 2024 •

edited

Loading

ed255 commented Jun 3, 2024

adria0 commented Jun 3, 2024 •

edited

Loading

ed255 left a comment

ed255 Jun 3, 2024

adria0 Jun 4, 2024

ed255 Jun 4, 2024 •

edited

Loading

ed255 Jun 4, 2024

davidnevadoc left a comment

davidnevadoc Jun 4, 2024

davidnevadoc Jun 4, 2024

davidnevadoc Jun 4, 2024

davidnevadoc Jun 4, 2024

davidnevadoc Jun 4, 2024

davidnevadoc Jun 4, 2024

adria0 Jun 5, 2024

adria0 Jun 5, 2024

adria0 commented Jun 5, 2024

adria0 commented Jun 5, 2024

Deterministic tests #345

Deterministic tests #345

Conversation

adria0 commented May 31, 2024 • edited Loading

codecov-commenter commented Jun 3, 2024 • edited Loading

Codecov Report

ed255 commented Jun 3, 2024

adria0 commented Jun 3, 2024 • edited Loading

ed255 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ed255 Jun 4, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adria0 commented Jun 5, 2024

adria0 commented Jun 5, 2024

adria0 commented May 31, 2024 •

edited

Loading

codecov-commenter commented Jun 3, 2024 •

edited

Loading

adria0 commented Jun 3, 2024 •

edited

Loading

ed255 Jun 4, 2024 •

edited

Loading