Deterministic tests

[adria0]

• New crate halo2_debug with some utilities, it can be reused for Adding debugging & testing utils #343
• 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-profiling feature (that is activated with --all-features)
• Add new feature vector-tests, note that the main idea here is that this feature is activated via --all-features so the vector tests are specific to --all-features

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 53.33333% with 7 lines in your changes missing coverage. Please review.

Project coverage is 82.06%. Comparing base (32599e8) to head (3d7cca4).

Files	Patch %	Lines
halo2_debug/src/lib.rs	53.33%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #349      +/-   ##
==========================================
- Coverage   82.09%   82.06%   -0.03%     
==========================================
  Files          83       84       +1     
  Lines       17228    17243      +15     
==========================================
+ Hits        14143    14151       +8     
- Misses       3085     3092       +7     

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

[davidnevadoc]

I am not sure about the change in the PCS. Particularly, the addition of a rng parameter in new.
Seems like IPA doesn't use it and KZG keeps using OsRng.

I wonder if there is a cleaner way to get deterministic params for the tests. What do you think about leaving new as is, and adding a function that generates some fixed kzg parameters for testing?

[adria0]

I am not sure about the change in the PCS. Particularly, the addition of a rng parameter in new. Seems like IPA doesn't use it and KZG keeps using OsRng.

I wonder if there is a cleaner way to get deterministic params for the tests. What do you think about leaving new as is, and adding a function that generates some fixed kzg parameters for testing?

Sure, a good option, there's the ::setup there

[ed255]

Overall looks good to me! Left a few comments to discuss.

[ed255]

We need to make sure this feature is enabled on test in the CI (this way we can detect the PRs that change the proof output). Is this the case?
If not, maybe we can add vector-tests as a default feature?

[adria0]

I think that this can be removed at all now, just keeping not(coverage) , let's me check again.

[adria0]

@ed255 the main point here is that results can differ when turning some features (like circuit-params), so we want a feature that is only activated when we test it with --all-features. I keep it as is.

[ed255]

I see, but then we're not testing this in the CI. We're only testing these sets of features:

• batch,dev-graph,gadget-traces
• batch,dev-graph,gadget-traces,test-dev-graph,thread-safe-region,sanity-checks,circuit-params

I think we should have a job in the CI that runs the tests with vector-tests enabled.

[adria0]

Another option is to change the "all features" in CI to --all-features instead "--features batch,dev-graph,gadget-traces,test-dev-graph,thread-safe-region,sanity-checks,circuit-params". Not sure, testing with --all-features is more straighforward that specifying them manually if there's no conflicting options.

[davidnevadoc]

Just a small nit. LGTM!

[davidnevadoc]

should we have &mut rng here instead of re-initializing with test_rng()?

[adria0]

Just a cosmetic thing, IMO.

[davidnevadoc]

Ah, that's my point! It's not the same thing, the function call resets the rng, not sure if it is important though.

[adria0]

So, you say that if some random in the protocol is the same that tau (SRS's one) could lead to some kind of weakness in tests?

Edit: I would say "cosmetic" in the sense that does adds/removes any property from the test.

[ed255]

I would say that "cosmetically" passing rng (or &mut rng) fits better as it's the same pattern used in the other tests.

[adria0]

@ed255 recheck again plz. Happened that with multithreading we get different results with OsX and Linux, so I limited the number of threads and fixed the CI.

[ed255]

Just a small suggestion for the documentation of test_result but otherwise LGTM!