Skip to content

Conversation

@yelhousni
Copy link
Collaborator

@yelhousni yelhousni commented Nov 12, 2025

Description

Add P-256 (secp256r1) elliptic curve (Fr, Fp and G arithmetics + ECDSA).

Type of change

  • New feature (non-breaking change which adds functionality)

How has this been tested?

How has this been benchmarked?

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Note

Introduce the P-256 (secp256r1) curve with full field/group ops, MSM, hash-to-curve, and ECDSA, and wire it into the registry, docs, and generators.

  • New Curve: secp256r1 (P-256)
    • Add full package ecc/secp256r1: fp/fr field arithmetic, G1 (affine/jacobian), multiexp, hash-to-curve (SVDW), ECDSA (sign/verify/recover), (de)serialization, tests/benchmarks.
    • Implement MSM (batch-affine/jacobian), scalar folding, and hash-to-G1 helpers.
  • Core Integration:
    • Register curve in ecc: add SECP256R1 ID, base/scalar moduli, string mapping; expose generators and curve coefficients.
    • Update docs (ecc.md) to list P-256; refresh package header to include ECDSA, Poseidon2, etc.
    • Extend codegen config/templates to support secp256r1 (multiexp, ECDSA, marshal, hash-to-curve).
  • Misc:
    • Tweak secp256k1 hash-to-curve comments/links to RFC 9380.

Written by Cursor Bugbot for commit 5cb46b2. This will update automatically on new commits. Configure here.

@yelhousni yelhousni added this to the v0.19.N milestone Nov 12, 2025
@yelhousni yelhousni requested a review from ivokub November 12, 2025 17:41
@yelhousni yelhousni self-assigned this Nov 12, 2025
ivokub
ivokub previously approved these changes Nov 13, 2025
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I left a few comments, but I think they are suitable for future PRs.

However the only thing I would do is to remove the SizeG1AffineCompressed constant or make it equal 64.

Or perhaps we could look if there is any other compression method?

@yelhousni
Copy link
Collaborator Author

@ivokub Added Hash-to-G1 and MSM and fixed review comments 👍

@yelhousni yelhousni requested a review from ivokub November 13, 2025 21:41
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the changes. Looks good now.

However, the comment by cursor is valid for v not being reset in the loops. It is fixed in #725 but that one isn't merged yet as it breaks test generation on gnark side.

Lets confirm in what order we should merge the PRs.

@yelhousni
Copy link
Collaborator Author

Thanks for addressing the changes. Looks good now.

However, the comment by cursor is valid for v not being reset in the loops. It is fixed in #725 but that one isn't merged yet as it breaks test generation on gnark side.

Lets confirm in what order we should merge the PRs.

We can merge this one

@ivokub
Copy link
Collaborator

ivokub commented Nov 14, 2025

Thanks for addressing the changes. Looks good now.
However, the comment by cursor is valid for v not being reset in the loops. It is fixed in #725 but that one isn't merged yet as it breaks test generation on gnark side.
Lets confirm in what order we should merge the PRs.

We can merge this one

You're right. I'll merge this branch and then in #725 will just regenerate to fix the v. Then can completely unblock on gnark side.

@ivokub ivokub merged commit a35ed76 into master Nov 14, 2025
9 checks passed
@ivokub ivokub deleted the feat/p256 branch November 14, 2025 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants