crypto/bn256/google: canonicalize coordinates before testing them - #35559
Open
4waan wants to merge 4 commits into
Open
crypto/bn256/google: canonicalize coordinates before testing them#355594waan wants to merge 4 commits into
4waan wants to merge 4 commits into
Conversation
MakeAffine reads z three ways that all assume a canonical value: Bits reports the magnitude, IsInfinity only recognises an exact zero, and ModInverse returns nil for a z that is a non-zero multiple of P. Add and Double leave z unreduced, so reduce it first. On the curve that nil went straight into big.Int.Mul and panicked; on the twist such a z has a zero norm, so Invert returns zero and the point at infinity silently became the affine point 0 : 0 : 1. Unreachable today: Add reduces z and Double doubles a reduced value, so z stays in [0, 2P-2] and even, and z = P is ruled out because P is odd. Reducing holds regardless, including for a caller that mutates the big.Ints G1.CurvePoints hands out. No consensus impact either way, since crypto/bn256 resolves to gnark on amd64/arm64.
The z == 1 path returned without touching x and y, which the group law leaves as unreduced as z was: Add and Double do not reduce them and Negative leaves y negative. G1.Neg of an affine point therefore produced a representation MakeAffine would not normalize, so comparing two mathematically equal points by their coordinates was unsound. Reduce them on the way out, which covers every source rather than just Negative. Set t as well. twistPoint.Add never writes it, so a point that reaches MakeAffine already affine can carry a stale t, and the Miller loop does read t. curvePoint.t is never read and is set for consistency.
IsOne tests y through big.Int.Bits, which reports the magnitude, so -1 read as 1. Both callers pass reduced values today, gfP12.IsOne because it calls Minimal first and twistPoint.MakeAffine because it now reduces z, but a false IsOne on a twist z skips normalization and a false IsOne in GT decides PairingCheck. Make the test itself sound.
Invert discarded the return value, so on failure inv kept whatever the pool last held. The result was still correct by accident: ModInverse fails only when the norm x²+y² is zero, and P ≡ 3 mod 4 makes -1 a non-residue, so a zero norm forces both components of a to zero and the multiplications below wipe inv either way. Return zero explicitly rather than resting on that. Zero is also what cloudflare's exponentiation-based inverse and gnark produce for 0⁻¹, so the three backends agree.
4waan
force-pushed
the
bn256-google-canonical-coordinates
branch
from
August 21, 2026 05:43
d842634 to
2f512c7
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
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.
big.Intcoordinates in the google backend are lazily reduced..AddandDoublewrite values outside[0, P)andNegativewrites a negativeyEach commit makes each one of 4 predicates (that read those values as if they were canoncial) agree with the representation it is actually handed.MakeAffineinverted an unreducedz.ModInversereturns nil when no inverse exists. On the curve that nil went straight intobig.Int.Muland panicked; on the twist the samezhas a zero norm, soInvertreturned zero and the point at infinity silently became the affine point0 : 0 : 1. Reducingzfirst removes the case instead of detecting it: after reductionz ∈ [1, P)at the inversion, coprime to primeP. That holds for any input, including a caller who mutates thebig.IntsG1.CurvePointshands out, which a nil check would only catch after the fact.MakeAffinereturned non-canonical coordinates. Thez == 1path returned without touchingxandy, soG1.Negof an affine point produced a representationMakeAffinewould not normalize, and comparing two mathematically equal points by their coordinates was unsound. Fixing it atMakeAffinerather than atNegativecoversAddandDoubleas well. That path now also writest(twistPoint.Addnever sets it so a point arriving already affine can carry a stalet) and the Miller loop readst.gfP2.IsOnetestedythroughBitswhich reports the magnitude so-1read as1.gfP2.Invertdiscarded theModInverseresult. This one was correct by accident, and the commit says so rather than claiming a bug... the inverse fails only when the normx²+y²is zero, andP ≡ 3 mod 4makes-1a non-residue, so a zero norm forces both components ofato zero and the multiplications wipe the stale value either way. Returning zero explicitly stops that resting on a coincidence, and matches what cloudflare's exponentiation-based inverse and gnark produce for0⁻¹.Testing
canonical_test.goadds five tests. Four fail against current master, checked per test in isolation rather than as a batch;TestGFp2InvertZeropasses both ways and is there to pin the0⁻¹ = 0convention against a future refactor ofInvert.(independent of #38885 and branched from master) that one fixes reachable aliasing in
Double, this one touchesMakeAffine,IsOneandInvert.. two can be in review in parallel