Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add secp256r1 add/double precompiles #400

Closed
wants to merge 8 commits into from

Conversation

shuklaayush
Copy link

@shuklaayush shuklaayush commented Mar 18, 2024

Adds SECP256R1_ADD and SECP256R1_DOUBLE precompiles for secp256r1 (P-256) operations. The implementation is similar to secp256k1 and I reused the WeierstrassAdd and WeierstrassDouble chips

Run the tests:

cargo t -r test_secp256r1_add_simple && cargo t -r test_secp256r1_double_simple && cargo t -r test_secp256r1_mul_simple

Since the constant $a \neq 0$ for this curve, the constraints in the WeierstrassDouble chip were failing on padding rows. I modified it to only check the constraints on rows where the is_real column is set

Addresses #230

@shuklaayush shuklaayush marked this pull request as draft March 18, 2024 20:48
@shuklaayush
Copy link
Author

shuklaayush commented Mar 19, 2024

@puma314 For full signature verification, I'll have to add a precompile for secp256r1 point decompression (akin to SECP256K1_DECOMPRESS). The SECP256K1_DECOMPRESS precompile assumes that the curve constant $a = 0$ which is not the case for secp256r1.
Should I generalize this precompile for all SW curves (by adding more columns to the air) or should I create a new precompile specifically for secp256r1? I'm thinking of following up with this in a separate PR

@shuklaayush shuklaayush marked this pull request as ready for review March 19, 2024 13:28
@puma314
Copy link
Contributor

puma314 commented Mar 28, 2024

Hey @shuklaayush this is great work! We recently refactored a lot of stuff around our precompiles for EC ops to be more general, so I think you should probably merge in the latest main to this PR? I can take a look afterwards!

@shuklaayush
Copy link
Author

@puma314 Done! Should be good for review

@kayleegeorge kayleegeorge mentioned this pull request May 14, 2024
@puma314
Copy link
Contributor

puma314 commented Jun 11, 2024

At some point we should add these precompiles & get them audited, but closing for now since there are a ton of merge conflicts.

@puma314 puma314 closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants