fix: handle non-modulus-width elements in emulated IsZero - #1806
Open
0x5ea000000 wants to merge 3 commits into
Open
fix: handle non-modulus-width elements in emulated IsZero#18060x5ea000000 wants to merge 3 commits into
0x5ea000000 wants to merge 3 commits into
Conversation
IsZero assumed the element it reduces always comes back with exactly the modulus limb count. Elements built by FromBits can legally carry a different number of limbs with zero overflow, and Reduce's fast path returns zero-overflow elements unchanged regardless of width: - wider than the modulus (e.g. a 512-bit FromBits result over a 4-limb modulus): the limb-wise comparison against the modulus indexed out of range and panicked at circuit definition; the value may also be a multiple of p larger than 2p, which the 0-or-p check misclassifies. Reachable from std via scalarMulFakeGLV's IsZero(s) on any wide FromBits scalar. - narrower than the modulus: the comparison checked only the given prefix of the modulus limbs, returning a false positive for an element equal to the low limbs of p. Force a full modular reduction to the modulus width when the reduced element is wider, and compare against every modulus limb, treating missing high limbs as zero. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A constant element wider than the modulus reached the reduction added in the previous commit, whose constant fast path folds to a constant element with a value-dependent limb count — zero limbs when the value is 0 mod p — which the limb-indexing checks below cannot handle. Evaluate IsZero at compile time for constant elements instead, before any reduction. Covers the constant-wide case and avoids emitting constraints for constants generally. Reported by Cursor Bugbot on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
0x5ea000000
force-pushed
the
fix/emulated-iszero-non-modulus-width
branch
from
August 12, 2026 05:05
d9ad695 to
6d8d338
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 6d8d338. Configure here.
In small-field mode (single-limb params over a large native field, e.g. Goldilocks over BN254), the wide-element reduction in IsZero routed through mulMod's toSingleLimbElement, which recomposes limbs natively and keeps the overflow at 0. For elements wider than the native field the recomposition wraps, making IsZero answer for the wrong value; at three limbs the understated overflow already undersizes the quotient range check, failing honest proving. Fold the limbs with mod-p-reduced coefficients instead before the reduction: the single-limb result is congruent mod p, fits the native field by the small-field-mode invariant, and declares an honest overflow for smallMulMod's accounting. Extend the non-modulus-width tests to 8x the modulus limb count, which for Goldilocks (512 bits over a ~254-bit native field) exercises the native-wrap regime; these cases fail without this fix. Reported by Cursor Bugbot on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
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.

Description
emulated.Field.IsZeroassumed the element it reduces always comes back with exactly the modulus limb count. Elements built byField.FromBitscan legally carry a different number of limbs with zero overflow, andField.Reduce's fast path returns zero-overflow elements unchanged regardless of width. This brokeIsZeroin two ways:FromBitsresult over a 4-limb modulus): the limb-wiseca == pcomparison indexed out of range and panicked at circuit definition (index out of range [4] with length 4). This is reachable from std:scalarMulFakeGLVcallsIsZero(s)on the incoming scalar, so passing a wideFromBitsscalar (e.g. a SHA-512 output for Ed25519-style verification) intoScalarMul/MultiScalarMulon a fakeGLV curve crashes at compile time. Additionally, such a value is not bounded by2p, so the 0-or-p classification would misclassify multiples ofp(2p,p², …) as non-zero even without the panic.p.Limbs, returning a false positive (IsZero == 1) for a non-zero element equal to the low limbs ofp.The fix forces a full modular reduction to the modulus width when the reduced element is wider than the modulus (one
mulModby one, only on this path), and compares against every modulus limb, treating missing high limbs as zero, when it is narrower.An alternative would be to tighten
Reduce's fast path itself (requirelen(a.Limbs) <= NbLimbsin addition tooverflow == 0); that touches every consumer, so this PR keeps the change local toIsZero. Happy to rework in that direction if preferred.Fixes #1805
Type of change
How has this been tested?
TestIsZeroNonModulusWidth(Goldilocks, BN254Fr, Mod1e512) covering:2pandp²as wide zero-overflow elements (≡ 0 mod p),p² + 5(non-zero), zero, and a single-limb element equal to the low limb ofp(non-zero, previously a false positive). The wide cases panic and the narrow case fails before this fix.go test ./std/math/emulated/...passes locally.How has this been benchmarked?
mulModreduction only triggers for elements wider than the modulus, which previously panicked; the width-matched path adds no constraints. The narrow-element path addslen(p.Limbs) - len(ca.Limbs)IsZero gates only for narrow elements.Checklist:
golangci-lintdoes not output errors locally (not installed here;gofmtandgo vetare clean)🤖 Generated with Claude Code
Note
Medium Risk
Correctness fix in a core emulated-field primitive used by scalar multiplication; wrong
IsZeroresults can break circuit soundness. Change is localized toIsZeroand covered by new tests.Overview
Fixes
Field.IsZerofor elements whose limb count differs from the modulus (e.g.FromBitsresults).Reduce's zero-overflow fast path left these unchanged, so the 0-or-p check either panicked on wide elements or returned false positives on narrow ones.Wider than modulus: force a full
mulModreduction first (with a small-field fold that avoids native wraparound). Also short-circuits constant elements at compile time.Narrower than modulus: compare against every modulus limb, treating missing high limbs as zero.
Adds edge-case tests covering
2p,p², high-limb multiples, and single-limb prefixes ofp, plus constant-wide elements.Reviewed by Cursor Bugbot for commit 68a23d8. Bugbot is set up for automated code reviews on this repo. Configure here.