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

Use noble cryptography libraries #238

Open
0xbok opened this issue Apr 8, 2024 · 3 comments
Open

Use noble cryptography libraries #238

0xbok opened this issue Apr 8, 2024 · 3 comments
Labels
dependencies 📦 Changes in dependencies refactoring ♻️ A code change that neither fixes a bug nor adds a feature

Comments

@0xbok
Copy link
Collaborator

0xbok commented Apr 8, 2024

Noble is a set of libraries providing "Audited & minimal JS implementation" of several cryptographic primitives implemented in zk-kit.

Some links: https://github.com/paulmillr/noble-curves, https://github.com/paulmillr/noble-hashes, https://paulmillr.com/noble/

It can be used to define babyjubjub curve, import poseidon, blake and sha hashes.

As discussed with @cedoor already, it's worthwhile to use these libraries as they are audited and optimized, and it can fix known and unkown bugs (or devation from standard) in zk-kit: like how eddsa secret key is derived currently.

@cedoor cedoor added dependencies 📦 Changes in dependencies refactoring ♻️ A code change that neither fixes a bug nor adds a feature labels Apr 8, 2024
@cedoor cedoor added this to the Beta milestone Apr 8, 2024
@cedoor cedoor added this to ZK-Kit Apr 8, 2024
@cedoor cedoor moved this to ♻️ Grooming in ZK-Kit Apr 8, 2024
@cedoor
Copy link
Member

cedoor commented Apr 8, 2024

@artwyman, @ctrlc03 tagging you here as this would be a big change that will affect the following packages (used by MACI and some PCDs):

Those shouldn't be breaking changes, but there could be other issues related to this one, to follow the EdDSA standard, e.g. using the Noble sha-3 hash (audited) rather than the current blake1.

Curious to know what you think about this.

@cedoor cedoor added the semaphore-v4-audit Issues related to the Semaphore V4 audit label Apr 8, 2024
@artwyman
Copy link
Collaborator

artwyman commented Apr 8, 2024

For non-breaking changes, my primary concerns to understand would be the impact on performance and bundle download size. circomlibjs has a very large set of poseidon constants for all sizes, so switching to poseidon-lite was a big win for bundle size. I'd be curious if these noble libraries have been optimized for that.

For breaking changes, I'm most concerned about cryptographic compatibility, rather than simple API changes. We have lots of EdDSA tickets (containing Poseidon hashes and EdDSA signatures) already in the wild which need to remain verifiable. Those were created using circomlibjs, but I've tested compatibility and intend to switch to zk-kit for them. There aren't yet any PODs (new PCD format for tickets and other things) in the wild, but there will be soon (early May) so changes to the cryptographic algorithms would need to either happen quickly, or we'd need a backward-compatible option there as well. These two concerns together might argue for providing both hash options (blake1 vs. sha-3) as different configurations in the library. It would be okay if we had to make "breaking" API changes to opt for the blake1 hash in use cases which require it, but it would be important to be able to support both hashes in the same codebase.

Side note just to confirm my understanding: I think the hashing happens only in the JS code, while verification circuits operate on the secret scalar post-hash. So I'm only considering our TypeScript code for the need to be explicitly backward compatible, while assuming our EdDSAPoseidon verification circuits should still work as-is for signatures generated with either hash. Let me know if I'm misunderstanding the impact here.

@cedoor
Copy link
Member

cedoor commented Apr 8, 2024

@artwyman thanks for your input!

I'd be curious if these noble libraries have been optimized for that.

I agree, that's what I'd like to test first.

For breaking changes, I'm most concerned about cryptographic compatibility, rather than simple API changes.

So, I forgot to add some info here. We're going to have a breaking change anyway due to this bug: #239, which would make public keys different regardless of the hash used. We'll add more information there as soon as possible. In the meantime, I'll ping you to add context.

I see your concerts tho!

cedoor added a commit that referenced this issue Apr 9, 2024
@cedoor cedoor mentioned this issue Apr 9, 2024
9 tasks
@cedoor cedoor removed this from the Beta milestone Apr 19, 2024
@cedoor cedoor removed the semaphore-v4-audit Issues related to the Semaphore V4 audit label Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies 📦 Changes in dependencies refactoring ♻️ A code change that neither fixes a bug nor adds a feature
Projects
Status: ♻️ Grooming
Development

No branches or pull requests

3 participants