o1-labs / o1js

TypeScript framework for zk-SNARKs and zkApps
https://docs.minaprotocol.com/en/zkapps/how-to-write-a-zkapp
Apache License 2.0
531 stars 122 forks source link

Support Secp256r1 #1885

Closed mitschabaude closed 1 month ago

mitschabaude commented 1 month ago

closes https://github.com/o1-labs/o1js/issues/1724

this adds ECDSA (and general) support for the curve secp256r1, also known as P-256, which is the standard curve used in ECDSA outside the crypto world, due to its recommendation by NIST; see P-256 in this document

Most changes are in https://github.com/o1-labs/o1js-bindings/pull/310

changes here:

Efficiency

Sadly, secp256r1 doesn't support the endomorphism we have implemented, because that endomorphism crucially relies on the a = 0 property. Therefore, in our ECDSA circuit the GLV method is not used, which means significant constraint savings get lost:

mitschabaude commented 1 month ago

@Trivo25 can you get me the necessary reviewers for this?

mitschabaude commented 1 month ago

Thanks for the review @Geometer1729! I also need one here and then I need you to merge both PRs since I don't have permission to

mitschabaude commented 1 month ago

Oh and @Trivo25 I also need admin review for changing the bindings commit

Geometer1729 commented 1 month ago

@mitschabaude Can you run npm run build:update-bindings and push the changes that creates in o1js-bindings? I think the bindings not having been recompiled may be why CI is failing.

mitschabaude commented 1 month ago

@mitschabaude Can you run npm run build:update-bindings and push the changes that creates in o1js-bindings? I think the bindings not having been recompiled may be why CI is failing.

no @Geometer1729, I didn't change anything that needs compiling. also, CI isn't failing, just the benchmark job is, and that one always fails on forks

mitschabaude commented 1 month ago

updating the bindings is just needed when you need to recompile ocaml or rust sources

Geometer1729 commented 1 month ago

Oh, I thought it was also affected by some of the content of o1js-bindings. Fair enough sorry about that.

45930 commented 1 month ago

Amazing work! Florian has final approval, but it LGTM

mitschabaude commented 1 month ago

the test is failing because we're still running node 18 in CI, where the web crypto API is missing 😬

do you mind if I change it to node 20?

mitschabaude commented 1 month ago

need workflow approval again 😢 @Trivo25

mitschabaude commented 1 month ago

and again @Trivo25!

mitschabaude commented 1 month ago

@nicc @mrmr1993 that's why I need write access to the repo ^^ https://github.com/o1-labs/o1js/pull/1885#issuecomment-2436878827 https://github.com/o1-labs/o1js/pull/1885#issuecomment-2437116301

mitschabaude commented 1 month ago

CI is green, ready to approve & merge 🚀

EDIT: and the bindings PR!

Trivo25 commented 1 month ago

@hattyhattington17 can we get your eyes on this too?

Trivo25 commented 1 month ago

@mitschabaude fyi I will get this merged before the next release