Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: Solver consistency test fails for commitment circuit with small modulus #871

Closed
ivokub opened this issue Oct 18, 2023 · 1 comment
Closed
Labels
bug Something isn't working P3: Low Issue priority: low

Comments

@ivokub
Copy link
Collaborator

ivokub commented Oct 18, 2023

Description

In test TestSolverConsistency for circuit commitCircuit we get test failures when checking commitment != 0. But when we use the small field (modulus 47), then the probability of commitment being randomly 0 is high and we hit it with high probability (and in practice as inputs are fixed).

Expected Behavior

TestSolverConsistency/commit succeeds

Actual Behavior

It doesnt:

errSCS :<nil>
        errR1CS :<nil>
        errEngine(const=false): [assertIsDifferent] 0 != 0
        test.isSolvedEngine.func1
        	gnark/test/solver_test.go:204
        runtime.gopanic
        	go1.21.0/src/runtime/panic.go:914
        test.(*engine).AssertIsDifferent
        	gnark/test/engine.go:450
        circuits.(*commitCircuit).Define
        	gnark/internal/backend/circuits/commit.go:16
        
        errEngine(const=true): [assertIsDifferent] 0 != 0
        test.isSolvedEngine.func1
        	gnark/test/solver_test.go:204
        runtime.gopanic
        	go1.21.0/src/runtime/panic.go:914
        test.(*engine).AssertIsDifferent
        	gnark/test/engine.go:450
        circuits.(*commitCircuit).Define
        	gnark/internal/backend/circuits/commit.go:16
        
        witness: [1, 2]

Possible Fix

We could build the commitment in test engine such that we avoid 0 outputs, but that would be cheating. I would rather omit running consistency test for this circuit. But the consistency test uses all integration tests, so we need to have some option to avoid running some of the tests for some circuits. This is also useful for some other edge cases (imo there was also some issue with some other circuits where we have hardcoded which ones do avoid).

@ivokub ivokub added bug Something isn't working P3: Low Issue priority: low labels Oct 18, 2023
@ivokub
Copy link
Collaborator Author

ivokub commented Nov 22, 2023

Fixed in #919

@ivokub ivokub closed this as completed Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P3: Low Issue priority: low
Projects
None yet
Development

No branches or pull requests

1 participant