Deterministic tests - Githubissues

privacy-scaling-explorations / halo2

https://privacy-scaling-explorations.github.io/halo2/

Other

208 stars 129 forks source link

Deterministic tests #345

Closed adria0 closed 5 months ago

adria0 commented 6 months ago

Make tests deterministic

Create new crate halo2_debug with some utilities, it can be reused for https://github.com/privacy-scaling-explorations/halo2/issues/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)

codecov-commenter commented 5 months ago

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 :warning:
halo2_backend/src/poly/ipa/commitment.rs	77.77%	2 Missing :warning:
halo2_backend/src/poly/kzg/commitment.rs	71.42%	2 Missing :warning:

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ed255 commented 5 months ago

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

adria0 commented 5 months ago

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

adria0 commented 5 months ago

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 commented 5 months ago

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