Fix silent (0, 0) return in Fp2 sqrt for purely-real non-residue inputs - #620
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR fixes a purely-real-input edge case ( ChangesPurely-Real Square Root Edge Case Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a fallback path in sqrt_if_square_generic to correctly handle the purely-real-input edge case (a0, 0) where a0 is a non-residue in Fp, along with a corresponding regression test. Feedback is provided regarding a performance regression in the fallback path, where computing the modular inverse of NonResidue on every call is highly inefficient; using sqrt_ratio_if_square is suggested instead to avoid the expensive field inversion.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Greptile SummaryThis PR fixes a correctness bug in
Confidence Score: 4/5The fix is correct for all valid Fp2 inputs; the two suggestions are non-blocking hardening improvements. The core algorithm change is sound: for every valid Fp2 input the fallback either correctly overrides a wrong (0,0) result or is a no-op. The two observations are style/hardening points that do not affect correctness on any reachable input today. The fallback block in square_root_fp2.nim (lines 202-237) is the critical new logic; t_fp2_sqrt.nim warrants a second look for the early-return behaviour in purelyRealNonResidueSqrtCheck. Important Files Changed
Reviews (1): Last reviewed commit: "Fix silent (0, 0) return in Fp2 sqrt for..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Excellent, thank you! |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
The following is an automated LLM review ran locally. No action need, for informational purpose only. Some recommendations might be wrong (like skipping invsqrt if divisor is zero, defeating constant-time ... 🤷) Review SummaryThis PR fixes a silent correctness bug in
Important Files Changed
Context Diagrams
flowchart TD
Start["sqrt_if_square_generic a: Fp2"] --> SaveState
SaveState["Save a0_orig & a1_isZero"] --> MainPath
MainPath["Adj-Rodríguez Algorithm 8<br/>Complex method unchanged"] --> MainResult
MainResult["result = t1.sqrt_if_square()<br/>Compute via sqrt_invsqrt<br/>Write a.c0 a.c1 conditionally"] --> FallbackEntry
FallbackEntry["Purely-real fallback<br/>runs unconditionally constant time"] --> InitFB
InitFB["fbC0 = 0, fbC1 = 0"] --> CandA
CandA["candA = a0_orig<br/>candAOk = candA.sqrt_if_square()"] --> Branch
Branch{"fromComplexExtension?"}
CandBComplex["candB = -a0_orig<br/>candBOk = candB.sqrt_if_square()"]
CandBGeneral["beta = NonResidue<br/>candBOk = candB.sqrt_ratio_if_square a0_orig beta"]
Branch -->|"Yes β=-1"| CandBComplex
Branch -->|"No general β"| CandBGeneral
CandBComplex --> SelectFB
CandBGeneral --> SelectFB
SelectFB["fbC0.ccopy(candA, candAOk)<br/>fbC1.ccopy(candB, !candAOk && candBOk)"] --> UseCheck
UseCheck["useFallback = a1_isZero && result"] --> Override
Override["a.c0.ccopy(fbC0, useFallback)<br/>a.c1.ccopy(fbC1, useFallback)"] --> ReturnPath
ReturnPath["return result"]
Production Readiness & Safety VerdictProduction Readiness: 3/5 Safety Verdict: Needs changes before merge The core fix is mathematically correct and constant-time safe — both math-crypto reviewers independently verified the Euler's criterion proof, the fallback formula for both complex and general extensions, and the absence of timing side channels. However, four reviewers independently flagged a ~60-75% performance regression on every SummariesFindingsOrder by file path (alphabetical), then by line number. This enables deterministic comparison between runs and agents.
Per-File Summary
LGTMThese are positive changes in the diff, not issues. They are excluded from Detailed Findings, per-file severity counts, and Recommendations.
Detailed FindingsHighAddress Before Merge — These issues cause likely failures or significant vulnerabilities. [PERF] PERF-A-001, PERF-B-001, ARCH-A-001, ARCH-B-001: Unconditional fallback doubles cost of every
|
| ID | Category | File:Line | Issue | Reason Unverified |
|---|---|---|---|---|
| BUG-A-002 | Bug | square_root_fp2.nim:220-224 |
Dead-code fromComplexExtension path fails for curves where −1 is a QR |
False positive — For complex extensions, β = -1 is a QNR by definition, and QNR × QNR = QR, so sqrt(-a0) exists when a0 is QNR. The mathematical concern was incorrect. The real issue is that this branch is simply dead code (no curve uses both complex extension and sqrt_if_square_generic). |
| PERF-B-002 | Performance | square_root_fp2.nim:220-232 |
sqrt_ratio_if_square could be simplified for complex-extension curves |
False positive — The else branch is the only one that runs for BLS12_377 (which doesn't use complex extension). The claim about simplification is about the when branch which is dead code. The sqrt_ratio_if_square approach in the else branch is the correct and optimal choice. |
| CONS-A-002 | Consistency | square_root_fp2.nim:219 |
var candBOk{.noInit.}: SecretBool is unique pattern in entire codebase |
False positive — {.noInit.} on SecretBool is used in 4+ other files: ec_scalar_mul.nim:299,389, ec_scalar_mul_vartime.nim:419, gt_exponentiations.nim:72, gt_exponentiations_vartime.nim:276. Not unique. |
| QA-002 | QA | square_root_fp2.nim:205 |
candidateB formula imprecise for generic extension |
Low confidence — The comment says candidateB = (0, sqrt_fp(a0 / β)) and the code computes sqrt_ratio_if_square(a0_orig, beta) where beta = NonResidue. This is correct. The concern about β = u² notation is a readability suggestion, not a factual error. Kept at Low severity. |
Recommendations
Prioritize in this order:
- High findings — Fix before merge
- Medium findings — Fix in next iteration or track as technical debt
- Low findings — Address when convenient, or acknowledge as accepted trade-offs
- Informational findings — No action required
Immediate Actions (High)
- PERF-A-001/PERF-B-001/ARCH-A-001/ARCH-B-001 (
square_root_fp2.nim:202-242) — Benchmark the ~60-75% performance regression on BLS12_377 pairing and hash-to-curve workloads. Document the constant-time tradeoff. Consider whether purely-real inputs are frequent enough to warrant a conditional branch with compensating delay.
Technical Debt (Medium)
- COV-A-001 (
square_root_fp2.nim:214-216) — Add a test case for(QR, 0)input, e.g.,(4, 0)or(9, 0), to exercise the candidateA fallback path. - COV-A-002/COV-B-004 (
t_fp2_sqrt.nim:88) — Add standalone test cases for(0, 0)and(1, 0)inputs. - COV-B-001 (
t_fp2_sqrt.nim:101-103) — Add negated root verification:nr.neg(root); nr.square(); check: bool(nr == x). - COV-B-002 (
t_fp2_sqrt.nim:101-103) — Add structural shape assertion:check: bool(root.c0.isZero())andcheck: not bool(root.c1.isZero())for non-QR inputs.
Technical Debt (Low)
- BUG-A-001/BUG-B-002 (
square_root_fp2.nim:195) — Consider guardingsqrt_invsqrtwhent2is zero to avoid wastedinvsqrt(0)computation. - BUG-B-001 (
square_root_fp2.nim:219,234) — InitializecandBOkto a safe default for clarity. - BUG-A-003 (
square_root_fp2.nim:240-242) — No change needed. Accept as-is (fallback overwrites QR result with same value). - COV-A-003/QA-004 (
square_root_fp2.nim:220-224) — No change needed. Document that the complex extension branch is dead code with current curves. - COV-A-004 (
square_root_fp2.nim:240-242) — Add a BLS12_377-specific non-square rejection test. - COV-B-003 (
t_fp2_sqrt.nim:50-70) — No change needed. Confirms regression test was necessary. - ARCH-A-003 (
square_root_fp2.nim:218-234) — No change needed. Pattern is consistent with the rest of the file. - ARCH-A-002 (
t_fp2_sqrt.nim:117-122) — Clarify the test comment to note that only BLS12_377 was affected. - ARCH-B-002 (
square_root_fp2.nim:99-145) — Add a comment insqrt_if_square_optexplaining how the purely-real QNR case is handled. - CONS-A-001/CONS-B-001/CONS-B-002 (
square_root_fp2.nim:210-242) — Accept as-is or standardize naming within the fallback block. - CONS-A-003 (
square_root_fp2.nim:173) — Add{.noInit.}tolet a0_origpertowers.nimconvention. - PERF-A-002 (
square_root_fp2.nim:229-231) — ReplacesetOne()+*=with directbeta = NonResidueor passNonResiduedirectly. - QA-001 (
square_root_fp2.nim:166) — Add "(for nonzero a0:)" prefix to the Euler's criterion proof comment. - QA-003 (
t_fp2_sqrt.nim:84) — Replace "alignment step" with "rotation step".
2 participants
Summary
sqrt_if_squareoverFp2[BLS12_377](and thesqrt_if_square_genericcode path more generally) silently returns(0, 0)withresult = truewhen fed an input of shape(a0, 0)wherea0is a non-residue in Fp. The input is a valid square inFp²and has a non-zero square root — the algorithm just doesn't find it.The optimized code path (
sqrt_if_square_opt, used by BLS12_381, BN254, and every other curve constantine supports) is not affected because itssqrt_rotate_extensionalignment step handles this stratum structurally. Only BLS12_377 uses the generic path, becausesqrt(sqrt(QNR)) ∉ Fp²[BLS12_377]and so the rotation trick cannot be implemented there.Reproduction (pre-fix)
The same probe on BLS12_381 / BN254_Snarks returns the correct non-zero root because those curves go through the (already-correct) optimized path.
Why this happens
sqrt_if_square_genericis a faithful implementation of Adj-Rodríguez 2012/685 Algorithm 8 (the "complex method"), which Scott §6.3 (ePrint 2020/1497) and Aardal et al. Algorithm 3 (ePrint 2024/1563) all build on. All three papers omit the purely-real-input edge case. Scott's correctness argument (§6.3) reads:This reasoning requires
b ≠ 0— whenb = 0the product is0(not a QNR) and the proof's conclusion fails. Tracing the algorithm witha = (a0, 0), a0 non-QR:t1 ← a0²−β·0² = a0².t1 ← sqrt(t1) = ±a0(deterministic).t2 ← (a0 + t1)/2,t3 ← (a0 − t1)/2. One isa0, one is0.quadResidTest ← t2.isSquare(). Sincea0is non-QR and0is a QR (byFp.isSquare's convention), the algorithm pickst2 = 0.sqrt_invsqrt(t1, t3, 0)returns(t1, t3) = (0, 0).a.c0 ← 0,a.c1 ← 0.result = true(set from step 2, sincea0²is a QR), withaoverwritten to(0, 0).The actual square root in this case is
(0, sqrt(a0/β)), which lives inFp²(one of{a0, a0/β}is always a QR in Fp becauseβitself is a non-residue).The fix
After the existing algorithm runs, compute two candidate purely-real-input roots in constant time:
(sqrt_fp(a0), 0)— valid iffa0 ∈ QR(Fp).(0, sqrt_fp(a0/β))— valid iffa0/β ∈ QR(Fp).For nonzero
a0 ∈ Fp, at least one of{a0, a0/β}is a QR (sinceβis itself a non-residue: QR × non-QR = non-QR, non-QR × non-QR = QR), so the fallback always finds the root.Constant-time-select the fallback into
a.c0/a.c1iffa.c1was zero. Theresultflag is already correct —(a0, 0)witha0 ≠ 0is a square inFp², soresult = truewas set correctly by the existingt1.sqrt_if_square()on the norm.The patch:
a.c0anda.c1.isZero()at the top of the function (before the existing algorithm mutatesa).ccopy/*=/sqrt_if_squareon unconditional code paths.No change to the algorithm's correctness on
a.c1 ≠ 0inputs — the fallbackccopys only fire whena.c1 == 0.Constant-time and performance impact
Fp.sqrt_if_squarecalls, oneFp.inv(only in theelsebranch — i.e.BLS12_377), and threeccopys. No data-dependent branches.sqrt_if_square_generic. Since this path is only invoked forBLS12_377, the impact is BLS12_377-specific.when a.fromComplexExtension()branch, candidate B reduces to(0, sqrt(−a0))andcandA + candBare mutually exclusive — this branch never actually triggers in constantine today (complex-extension curves all usesqrt_if_square_opt), but we keep the code arm for safety in case the dispatcher ever sends complex extensions through the generic path.What's tested
The regression test
[𝔽p2] (non-QR, 0) sqrt regression for <Curve>exercises the bug shape across allTestCurves(BN254_Nogami, BN254_Snarks, BLS12_377, BLS12_381):Pre-fix this test fails for BLS12_377 (
sqrt(x)^2 == xis false;root.isZero()is true) and passes for the other three curves. Post-fix all four pass.Exploit assessment
Reachability of
(non-QR, 0)shape from production callers:EC_ShortW_Aff/Jac/Proj.recoverYhash_to_curve(h2c_map_to_isocurve_swu.nim)sqrt_rotate_extension) — not affected by the generic path.lowlevel_extension_fields.sqrt_if_square(public API)Fp²[BLS12_377]arithmetic directly (e.g., recursive SNARK proving stacks, Celo BLS sigs, the Zexe family of recursive proofs) is exposed if they ever callsqrt_if_squareon an attacker-controlled input that ends up purely real.Production constantine callers don't trigger the bug today, but the function is part of constantine's public API and external Nim/C/Go/Rust users via the
lowlevel_*exports can hit it. Worth fixing for correctness and defense-in-depth.Curves directly relevant to Ethereum L1 (BLS12_381, BN254_Snarks) were never affected because they use the optimized path.
Test plan
nim c -d:release -p:. tests/math_extension_fields/t_fp2_sqrt.nim && ./tests/math_extension_fields/t_fp2_sqrt— all 11 tests pass (4 random sqrt across curves + 4 new regression tests across curves + 3 historical bug regressions).nim c -d:release -p:. tests/math_extension_fields/t_fp2.nim && ./...— full Fp2 arithmetic suite passes; no regression from the fallback's side effects.blst_fp2_sqrt-style standalone test reproduces the bug pre-fix and confirms the fix post-fix across BLS12_381, BN254_Snarks, BLS12_377.References
sqrt_rotate_extensiondesign. [Optim] Square root on Fp2 supranational/blst#2 (comment)Summary by CodeRabbit
Bug Fixes
Tests