Repository navigation
D1: reject n >= maxbits() in Num2BitsNeg and Num2BinNeg - #157
Draft
OBrezhniev wants to merge 1 commit into
Draft
OBrezhniev wants to merge 1 commit into
OBrezhniev wants to merge 1 commit into
Conversation
OBrezhniev
force-pushed
the
fix/circomlib_buses_review
branch
from
September 29, 2026 18:39
b85175e to
7198ad7
Compare
OBrezhniev
force-pushed
the
fix/num2bitsneg-width
branch
from
September 29, 2026 18:39
82847fc to
2a37e58
Compare
Both templates document their result as the n bits of 2**n - in. That holds only while 2**n is a field element. At n = maxbits() the constant is reduced, and the templates compute (2**n mod p) - in instead, which is neither a negation modulo 2**n nor modulo p. Measured earlier on Num2BitsNeg(n)(1): exact for n = 252 and 253, wrapped for 254, 255 and 260. Nothing in the library uses either template at that width. Both now assert n < maxbits(), so the parameter is rejected at compile time with the reason in the docstring. In Num2BinNeg the alias-check branch for n >= maxbits() is removed as unreachable: below maxbits() the n-bit value is below the prime and the unique tag holds on its own. Both docstrings also had the example attributed to the wrong template. Tests move from 254 to 253, the widest width now allowed, where the expected value is the integer 2**253 - in and the tests check that rather than restating the template's own constraint: bitify_neg_test and its suite, and binnum_test, whose negation part gets its own input (inneg) because the shared input's q - 1 case cannot be negated in 253 bits. binnum.js passes it; a boundary case covers 2**253 - 1 -> 1, 2**253 -> 0 and 2**253 + 1 -> no solution, and bitify.js checks that n = 254 is rejected at compile time for both templates. For upstream: binnum_test.circom instantiated Num2BinNeg(254) in 5013906, so this changes a recent upstream test. Documented as D1 in the review notes; the alternative is to leave the width and correct only the docstrings. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D1w729xzwemQigkf3PCisD
OBrezhniev
force-pushed
the
fix/num2bitsneg-width
branch
from
September 29, 2026 20:08
2a37e58 to
cbebbaa
Compare
OBrezhniev
force-pushed
the
fix/circomlib_buses_review
branch
from
September 29, 2026 20:08
7198ad7 to
d5f158d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #149 (
fix/circomlib_buses_review); this PR shows only its own commits. Independent of the other stacked PRs unless stated. Full suite passes on the branch.Draft, for discussion: this changes
binnum_test.circom'sNum2BinNeg(254)from upstream commit 5013906. The alternative is to leave the width and correct only the docstrings.Reject n >= maxbits() in Num2BitsNeg and Num2BinNeg
Both templates document their result as the n bits of 2n - in. That
holds only while 2n is a field element. At n = maxbits() the constant
is reduced, and the templates compute (2n mod p) - in instead, which
is neither a negation modulo 2n nor modulo p. Measured earlier on
Num2BitsNeg(n)(1): exact for n = 252 and 253, wrapped for 254, 255 and
260. Nothing in the library uses either template at that width.
Both now assert n < maxbits(), so the parameter is rejected at compile
time with the reason in the docstring. In Num2BinNeg the alias-check
branch for n >= maxbits() is removed as unreachable: below maxbits() the
n-bit value is below the prime and the unique tag holds on its own.
Both docstrings also had the example attributed to the wrong template.
Tests move from 254 to 253, the widest width now allowed, where the
expected value is the integer 2253 - in and the tests check that
rather than restating the template's own constraint: bitify_neg_test
and its suite, and binnum_test, whose negation part gets its own input
(inneg) because the shared input's q - 1 case cannot be negated in 253
bits. binnum.js passes it; a boundary case covers 2253 - 1 -> 1,
2253 -> 0 and 2253 + 1 -> no solution, and bitify.js checks that
n = 254 is rejected at compile time for both templates.
For upstream: binnum_test.circom instantiated Num2BinNeg(254) in
5013906, so this changes a recent upstream test. Documented as D1 in the
review notes; the alternative is to leave the width and correct only
the docstrings.
🤖 Generated with Claude Code
https://claude.ai/code/session_01D1w729xzwemQigkf3PCisD