fix binary decomposition of 0

[lightning-li]

Description

Fixes # (852)

Type of change

• Bug fix (non-breaking change which fixes an issue)

How has this been tested?

• Added test cases for api.AssertIsLessEq

Checklist:

• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• I did not modify files generated from templates
• golangci-lint does not output errors locally
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[CLAassistant]

All committers have signed the CLA.

[ivokub]

Suggested edit:

diff --git a/internal/regression_tests/issue_836_test.go b/internal/regression_tests/issue_836_test.go
index 99c27f3a..0830c291 100644
--- a/internal/regression_tests/issue_836_test.go
+++ b/internal/regression_tests/issue_836_test.go
@@ -79,12 +79,17 @@ func TestIssue836Cmp(t *testing.T) {
 		Right:     5,
 		ExpCmpRes: -1,
 	}
+	assignmentHintBad2 := CmpCircuit{
+		Left:      10,
+		Right:     0,
+		ExpCmpRes: -1,
+	}
 	toReplaceHint, err := getNBitsHint()
 	if err != nil {
 		t.Fatalf("couldn't find hint to replace: %v", err)
 	}
 	assert.CheckCircuit(&CmpCircuit{}, test.WithValidAssignment(&assignmentNoHintGood), test.WithInvalidAssignment(&assignmentNoHintBad))
-	assert.CheckCircuit(&CmpCircuit{}, test.WithInvalidAssignment(&assignmentHintBad), test.NoTestEngine(), test.WithSolverOpts(solver.OverrideHint(toReplaceHint, maliciousNbitsHint)))
+	assert.CheckCircuit(&CmpCircuit{}, test.WithInvalidAssignment(&assignmentHintBad), test.WithInvalidAssignment(&assignmentHintBad2), test.NoTestEngine(), test.WithSolverOpts(solver.OverrideHint(toReplaceHint, maliciousNbitsHint)))
 }
 
 func TestIssue836AssertIsLess(t *testing.T) {
@@ -110,8 +115,7 @@ func TestIssue836AssertIsLess(t *testing.T) {
 		t.Fatalf("couldn't find hint to replace: %v", err)
 	}
 	assert.CheckCircuit(&AssertIsLessOrEqCircuit{}, test.WithValidAssignment(&assignmentNoHintGood), test.WithInvalidAssignment(&assignmentNoHintBad))
-	assert.CheckCircuit(&AssertIsLessOrEqCircuit{}, test.WithInvalidAssignment(&assignmentHintBad), test.NoTestEngine(), test.WithSolverOpts(solver.OverrideHint(toReplaceHint, maliciousNbitsHint)))
-	assert.CheckCircuit(&AssertIsLessOrEqCircuit{}, test.WithInvalidAssignment(&assignmentHintBad2), test.NoTestEngine(), test.WithSolverOpts(solver.OverrideHint(toReplaceHint, maliciousNbitsHint)))
+	assert.CheckCircuit(&AssertIsLessOrEqCircuit{}, test.WithInvalidAssignment(&assignmentHintBad), test.WithInvalidAssignment(&assignmentHintBad2), test.NoTestEngine(), test.WithSolverOpts(solver.OverrideHint(toReplaceHint, maliciousNbitsHint)))
 }
 
 func TestIssue836MathCmpAssertIsLessEqBounded(t *testing.T) {
@@ -128,15 +132,19 @@ func TestIssue836MathCmpAssertIsLessEqBounded(t *testing.T) {
 		Left:  10,
 		Right: 5,
 	}
+	assignmentHintBad2 := MathCmpAssertIsLessOrEqCircuitBounded{
+		Left:  10,
+		Right: 0,
+	}
 	toReplaceHint, err := getNBitsHint()
 	if err != nil {
 		t.Fatalf("couldn't find hint to replace: %v", err)
 	}
 	assert.CheckCircuit(&MathCmpAssertIsLessOrEqCircuitBounded{}, test.WithValidAssignment(&assignmentNoHintGood), test.WithInvalidAssignment(&assignmentNoHintBad))
-	assert.CheckCircuit(&MathCmpAssertIsLessOrEqCircuitBounded{}, test.WithInvalidAssignment(&assignmentHintBad), test.NoTestEngine(), test.WithSolverOpts(solver.OverrideHint(toReplaceHint, maliciousNbitsHint)))
+	assert.CheckCircuit(&MathCmpAssertIsLessOrEqCircuitBounded{}, test.WithInvalidAssignment(&assignmentHintBad), test.WithInvalidAssignment(&assignmentHintBad2), test.NoTestEngine(), test.WithSolverOpts(solver.OverrideHint(toReplaceHint, maliciousNbitsHint)))
 }
 
-func TestIssueXXXMathCmpAssertIsLessEqFull(t *testing.T) {
+func TestIssue836MathCmpAssertIsLessEqFull(t *testing.T) {
 	assert := test.NewAssert(t)
 	assignmentNoHintGood := MathCmpAssertIsLessOrEqCircuitFull{
 		Left:  5,
@@ -150,12 +158,16 @@ func TestIssueXXXMathCmpAssertIsLessEqFull(t *testing.T) {
 		Left:  10,
 		Right: 5,
 	}
+	assignmentHintBad2 := MathCmpAssertIsLessOrEqCircuitFull{
+		Left:  10,
+		Right: 0,
+	}
 	toReplaceHint, err := getNBitsHint()
 	if err != nil {
 		t.Fatalf("couldn't find hint to replace: %v", err)
 	}
 	assert.CheckCircuit(&MathCmpAssertIsLessOrEqCircuitFull{}, test.WithValidAssignment(&assignmentNoHintGood), test.WithInvalidAssignment(&assignmentNoHintBad))
-	assert.CheckCircuit(&MathCmpAssertIsLessOrEqCircuitFull{}, test.WithInvalidAssignment(&assignmentHintBad), test.NoTestEngine(), test.WithSolverOpts(solver.OverrideHint(toReplaceHint, maliciousNbitsHint)))
+	assert.CheckCircuit(&MathCmpAssertIsLessOrEqCircuitFull{}, test.WithInvalidAssignment(&assignmentHintBad), test.WithInvalidAssignment(&assignmentHintBad2), test.NoTestEngine(), test.WithSolverOpts(solver.OverrideHint(toReplaceHint, maliciousNbitsHint)))
 }
 
 func maliciousNbitsHint(mod *big.Int, inputs []*big.Int, results []*big.Int) error {

[ivokub]

Yes, we had overlooked the case for 0 vs modulus.

Everything is good but I suggested a few more tests for avoiding regressions in the future. I also suggested a test name rename to track the initial issue.

For some reason I wasn't able to push to your branch even though maintainer edits were allowed. This feature usually works but I'm not sure what happened now. I'll approve after incorporating the suggested edits.

[ivokub]

Thanks! It was a good catch. Welcome as a gnark contributor!

[ivokub]

It seems we need to update the circuit statistics.

Can you do:

cd internal/stats/generate/
go run main.go -s

and then commit the updated internal/stats/latest.stats file? Thanks.

[lightning-li]

It seems we need to update the circuit statistics.

Can you do:

cd internal/stats/generate/
go run main.go -s

and then commit the updated internal/stats/latest.stats file? Thanks.

done~

[ivokub]

Merging with failed tests - it seems that CI doesn't have access to some secrets when PRs are based on external forks.