Fix tests panic in debug #764

moCello · 2023-08-16T09:52:13Z

Resolves: #763

codecov · 2023-08-16T10:04:25Z

Codecov Report

Merging #764 (60c4901) into master (8691d2b) will increase coverage by 0.01%.
Report is 3 commits behind head on master.
The diff coverage is 90.82%.

❗ Current head 60c4901 differs from pull request most recent head 0721229. Consider uploading reports for the commit 0721229 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #764      +/-   ##
==========================================
+ Coverage   85.05%   85.07%   +0.01%     
==========================================
  Files          57       57              
  Lines        4363     4327      -36     
==========================================
- Hits         3711     3681      -30     
+ Misses        652      646       -6

Files Changed	Coverage Δ
src/commitment_scheme/kzg10/key.rs	`85.32% <ø> (ø)`
src/fft/polynomial.rs	`43.57% <0.00%> (-0.23%)`	⬇️
src/proof_system/quotient_poly.rs	`100.00% <ø> (ø)`
src/proof_system/widget.rs	`88.95% <0.00%> (ø)`
tests/common/mod.rs	`69.23% <ø> (-7.24%)`	⬇️
src/composer.rs	`91.02% <66.66%> (-0.09%)`	⬇️
tests/logic.rs	`96.36% <92.00%> (+2.19%)`	⬆️
tests/range.rs	`94.73% <94.73%> (+8.37%)`	⬆️
tests/append_gate.rs	`93.10% <100.00%> (+0.42%)`	⬆️
tests/assert_point.rs	`83.05% <100.00%> (ø)`
... and 8 more

... and 3 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8691d2b...0721229. Read the comment docs.

By repeatedly running the setup, the public parametesr where unnecessarily recreated which slowed down the tests.

vlopes11

Looks good overall! A comment regarding the correct change on enforcing range checks of even values

vlopes11 · 2023-08-16T13:50:41Z

src/composer.rs

@@ -129,7 +129,7 @@ pub trait Composer: Sized + Index<Witness, Output = BlsScalar> {
    ) -> Witness {
        // the bits are iterated as chunks of two; hence, we require an even
        // number
-        debug_assert_eq!(num_bits & 1, 0);
+        assert_eq!(num_bits & 1, 0, "number of bits must be even");


Not sure if it makes sense to add production panic to check circuit development consistency. I'd rather keep it as debug_assert_eq and have tests checking for such case

I agree that it's weird to deliberately panic in production, but I think in this case it makes sense since the alternative would be that the circuit would produce an incorrect proof. The other alternative I see is to return an error but that means changing the function signature.

I have the same feeling on panicking like this, but more because it signals that we should be erroring rather than panicking. The library should validate user input against what it expects.

There is an important distinction tho: the validation is static and should be performed at compilation time.

Meaning: when we create a circuit implementation, we create it for a static type (i.e. impl Circuit for Foo). It means it could be a waste to move such check to runtime (i.e. make the function fallible & Err).

If we move everything to static time, then it probably makes more sense. In short: if the function receives a constant, we can just assert it and it would fail at compile time if the user attempts to create a circuit with wrong bits boundaries:

fn append_logic_component<BITS: usize>( &mut self, a: Witness, b: Witness, is_component_xor: bool, ) -> Witness { assert_eq!(BITS & 1, 0, "some msg");

The code above will not compile for invalid bits.

In fact, the problem above probably applies elsewhere. We should probably use more static stuff for other components instead of running test checks. Probably is_component_xor should also be a constant.

TLDR; components should receive only witnesses as arguments. Everything else is a constant

I pushed a version with the range and logic components erroring on uneven bits.
But generally I think we all agree that the circuit description should not depend on the circuit struct input. This is in line with our earlier discussion of remodeling the Circuit trait in general.

For now though I think throwing an error at compilation time when the bits are uneven seems like a good solution to me.

We can also circumvent any misuse if we let the user pass the amount of bit-pairs instead of bits. In the components themselves we multiply the bit-pairs by 2 and can therefore always be sure the total amount of bits is dividable by two.

It would be more cognitive overhead on the circuit creator but would make sure the components can not be misused without the need to panic or error

I did an attempt on my above proposal combined with @vlopes11 suggestion of making the logic and range components generic over the bit (bit-halves in this case).

@vlopes11 and @ureeves please check it out under the branch: mocello/763_fix_debug_panic_bit_pairs and let me know whether you like that one or the latest changes on this branch more.

I very much like the direction of "making illegal state unrepresentable". Comes with tons of benefits. I vote for the bit_pairs branch that you suggested.

I also like the solution with the BIT_PAIRS most.

Pushed to the branch, please have a final look and approve (if you approve that is ;) )

src/composer.rs

ureeves

The instinct to want to actually panic on this makes sense, but I think it makes more sense to error in this situation. Here's the book on when to panic.

I'm open to being persuaded otherwise though.

ureeves · 2023-08-16T14:47:01Z

src/composer.rs

@@ -129,7 +129,7 @@ pub trait Composer: Sized + Index<Witness, Output = BlsScalar> {
    ) -> Witness {
        // the bits are iterated as chunks of two; hence, we require an even
        // number
-        debug_assert_eq!(num_bits & 1, 0);
+        assert_eq!(num_bits & 1, 0, "number of bits must be even");


I have the same feeling on panicking like this, but more because it signals that we should be erroring rather than panicking. The library should validate user input against what it expects.

With the introduction of the const generic `BIT_PAIRS` it is now impossible for the user to create an invalid circuit and removes the need to panic or error.

ureeves

LGTM

xevisalle · 2023-08-30T15:36:31Z

README.md

@@ -50,8 +50,10 @@ impl Circuit for TestCircuit {
        composer.append_gate(constraint);

        // Check that a and b are in range
-        composer.component_range(a, 6);
-        composer.component_range(b, 5);
+        const HALF_SIX: usize = 3;


what's the reason for that?

The range (and logic) component is now generic over a constant BIT_PAIRS. This was done so that the user has no way to construct an invalid circuit (internally we group the bits in pairs which made circuits over an uneven number of bits invalid).

In the case of the above example, we chose 3 for the generic BIT_PAIRS, which will result in a circuit that checks whether the witness is encoded in 6 bits or less.
To make that connection between 3 and 6 clear I chose the name HALF_SIX.

moCello force-pushed the mocello/763_fix_debug_panic branch 3 times, most recently from 84a1163 to 773d1fd Compare August 16, 2023 10:46

moCello requested review from ureeves, vlopes11, ZER0 and HDauven August 16, 2023 10:47

moCello force-pushed the mocello/763_fix_debug_panic branch from 773d1fd to 280837b Compare August 16, 2023 10:49

moCello marked this pull request as ready for review August 16, 2023 12:02

Remove setup from common test module

2998f3d

By repeatedly running the setup, the public parametesr where unnecessarily recreated which slowed down the tests.

moCello force-pushed the mocello/763_fix_debug_panic branch from 280837b to 8a35ca3 Compare August 16, 2023 12:12

vlopes11 reviewed Aug 16, 2023

View reviewed changes

moCello force-pushed the mocello/763_fix_debug_panic branch from a56c77e to 5eae14a Compare August 16, 2023 14:39

moCello requested a review from vlopes11 August 16, 2023 14:40

ureeves requested changes Aug 16, 2023

View reviewed changes

moCello requested a review from ureeves August 16, 2023 15:58

moCello force-pushed the mocello/763_fix_debug_panic branch 3 times, most recently from 61c0470 to 3cf75ca Compare August 18, 2023 10:50

moCello added 2 commits August 18, 2023 12:59

Fix locic and range test panic in debug mode

b99fdc8

With the introduction of the const generic `BIT_PAIRS` it is now impossible for the user to create an invalid circuit and removes the need to panic or error.

Update CHANGELOG

0721229

moCello force-pushed the mocello/763_fix_debug_panic branch from 3cf75ca to 0721229 Compare August 18, 2023 10:59

ureeves approved these changes Aug 21, 2023

View reviewed changes

moCello merged commit 5be0ffe into master Aug 21, 2023
5 checks passed

moCello deleted the mocello/763_fix_debug_panic branch August 21, 2023 11:19

xevisalle reviewed Aug 30, 2023

View reviewed changes

moCello mentioned this pull request Jan 3, 2024

Fix "alloc" feature for tests #808

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tests panic in debug #764

Fix tests panic in debug #764

moCello commented Aug 16, 2023

codecov bot commented Aug 16, 2023 •

edited

Loading

vlopes11 left a comment

vlopes11 Aug 16, 2023

moCello Aug 16, 2023

ureeves Aug 16, 2023

vlopes11 Aug 16, 2023

vlopes11 Aug 16, 2023

moCello Aug 16, 2023

moCello Aug 17, 2023

moCello Aug 17, 2023

ureeves Aug 17, 2023

moCello Aug 18, 2023

ureeves left a comment

ureeves Aug 16, 2023

ureeves left a comment

xevisalle Aug 30, 2023

moCello Sep 11, 2023

Fix tests panic in debug #764

Fix tests panic in debug #764

Conversation

moCello commented Aug 16, 2023

codecov bot commented Aug 16, 2023 • edited Loading

Codecov Report

vlopes11 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ureeves left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ureeves left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 16, 2023 •

edited

Loading