Open
Conversation
18 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Adds missing P-256-specific algebra needed for increment-and-check map-to-curve by introducing an internal Fp² tower implementation and a Cardano depressed-cubic solver over secp256r1/fp.
Changes:
- Introduce
internal/fptowerFp² type (E2) with core arithmetic plusSqrt/Cbrthelpers. - Add
CardanoRoots(c)to solvex³ − 3x + c = 0over the P-256 base field. - Add property-based tests/benchmarks for the new Fp² ops and Cardano solver.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ecc/secp256r1/internal/fptower/e2.go | New Fp² element type and arithmetic, including sqrt/cbrt helpers used by Cardano. |
| ecc/secp256r1/internal/fptower/e2_test.go | Property tests for algebraic identities + microbenchmarks for E2 ops. |
| ecc/secp256r1/internal/fptower/generators_test.go | Gopter generators for Fp and E2 test inputs. |
| ecc/secp256r1/cardano.go | New Cardano solver returning Fp roots of x³ − 3x + c = 0. |
| ecc/secp256r1/cardano_test.go | Property tests for CardanoRoots correctness. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
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
Add Fp2 arithmetic and Cardano cubic solver for secp256r1 (P-256), used by the increment-and-check map-to-curve construction in gnark.
P-256 has a ≠ 0 (y² = x³ − 3x + b), so the y-increment method requires solving the depressed cubic x³ − 3x + c = 0 to recover x from a candidate y. This PR implements:
ecc/secp256r1/internal/fptower/: Fp2 = Fp[u]/(u²+1) arithmetic (valid since q ≡ 3 mod 4), including:expByCbrtHelperfor x^{(q-4)/9} reusesfp.ExpByCbrtHelperQMinus4Div9ecc/secp256r1/cardano.go:CardanoRoots(c)— solves x³ − 3x + c = 0 over Fp handling all three discriminant cases:Companion PR in gnark: gnark uses
CardanoRootsin the map-to-curve hint for P-256 y-increment witness generation.Type of change
How has this been tested?
TestCardanoRoots— verifies roots satisfy x³ − 3x + c = 0 for 256 test values derived from P-256 curve pointsTestE2ReceiverIsOperand— aliasing safety for all E2 operations (Add, Sub, Mul, Square, Neg, Double, Inverse, Conjugate, MulByElement, Sqrt)TestE2Ops— algebraic identity tests via gopter property-based fuzzing (50 random inputs each): add/sub inverse, mul/inverse, double inverse, neg inverse, square==mul, MulByElement inverse, conjugate properties, Legendre on squares, sqrt correctness, cbrt correctnessHow has this been benchmarked?
go test -bench=. ./ecc/secp256r1/internal/fptower/Checklist:
golangci-lintdoes not output errors locallyNote
Medium Risk
Adds new finite-field extension arithmetic and a cubic root/solver used in elliptic-curve mapping; subtle math or corner-case bugs could break correctness even though changes are additive and well-tested.
Overview
Adds a new
ecc/secp256r1/internal/fptowerFp2implementation (E2) for P-256, including core arithmetic plusSqrtandCbrtroutines needed for higher-level algebra.Introduces
CardanoRootsinecc/secp256r1/cardano.goto compute roots ofx³ − 3x + c = 0overFp, handling discriminant cases (including anFp2fallback when the discriminant is a non-square) and precomputing a cube root of unity for the 3-root case.Adds property-based tests for both
E2operations andCardanoRoots(including checks derived from real curve points), plus micro-benchmarks forE2primitives.Written by Cursor Bugbot for commit 2c33f2d. This will update automatically on new commits. Configure here.