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(eddsa-poseidon): adds Blake2s hashing for eddsa and conditional imports #329

Conversation

hannahredler
Copy link
Contributor

@hannahredler hannahredler commented Sep 13, 2024

Description

Current implementation of the EdDSA Poseidon uses the Blake-1 hashing algorithm. This PR implements Blake2b algorithm and conditional imports such that the user can choose to use a different hashing algorithm if required.

Related Issue(s)

Partially Closes #152
Adds the first optional hashing algorithm

Other information

The implementation of Blake2b is based on this one, migrated to Typescript and changed to a class structure to mirror the Blake1 implementation. The test cases were lifted directly from the repo to ensure consistent output.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have run yarn style without getting any errors
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

…imports

Currently EdDSA Poseidon hashes using Blake 1, which is now outdated. This commit swaps the default
hashing algorithm for Blake2, whilst also introducing conditional imports so the user can specify
the underlying hashing algorithm if required.

BREAKING CHANGE: n
@hannahredler
Copy link
Contributor Author

@cedoor @artwyman the only thing I haven't managed to get to work is to import the sub packages from outside the project and check they import correctly, after running yarn build. If you could direct me in the best way to ensure this works, I can validate that

@artwyman
Copy link
Collaborator

I don't think I have the time or expertise to review this in detail, but here are a few design discussion points to consider from the point of view of 0xPARC's projects (Zupass and PODs) making use of this library, which might be a reasonable stand-in for

  • I think it's important the packaging allow for avoiding the cost of the unused hashing options here. E.g. if I need only blake-1, and I structure my imports appropriately, I shouldn't see the size impact of blake-2b in my bundle (and vice versa), similar to the way the poseidon-lite module works for different sizes.
  • Adding more hash options definitely seems good. Changing the default in particular is something to consider carefully, due to its impact on existing users, and the validity of their existing signed data. It's certainly very much a breaking change, and could cause compatibility headaches for any developer who doesn't notice it's happened. It should certainly be reflected in the semantic version, and I might go so far as to change the TypeScript interface so as to explicitly break existing code which doesn't get hand updated. I think it would be safer to add blake-2b as an option, but leave the default as-is.

For Zupass in particular, we have existing tickets and PODs signed using the blake-1 variant so we're unlikely to switch anytime soon, and would have to introduce some new version/config fields in our formats in order to do so. I'd expect SemaphoreV4 to be in a similar situation where they care very much about compatibility.

I'd be interested to hear the advantages of blake-2b so as to think carefully about when an upgrade might be warranted. Are there known vulnerabilities in blake-1? Is blake-2b stronger, faster, or superior in some other way?

@hannahredler
Copy link
Contributor Author

hannahredler commented Sep 14, 2024

Thanks for the comment @artwyman.

As far as I understand it, Blake2 is optimised to be faster and more efficient, whilst reliably offering a similar level of security. Thus if changing it could cause a headache down the line, I agree that it might be better to keep it as an option rather than changing the default, and this can always be a decision that is easily taken later.

In terms of bundle size, I have structured the code to only return the compiled version of the chosen algorithm, so that should be working as intended.

Copy link
Member

@cedoor cedoor left a comment

Choose a reason for hiding this comment

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

Hey @hannahredler, thanks for this PR!

I added some comments 👍🏽

packages/eddsa-poseidon/src/index.ts Show resolved Hide resolved
packages/eddsa-poseidon/src/index.ts Outdated Show resolved Hide resolved
packages/eddsa-poseidon/src/blake2b.ts Outdated Show resolved Hide resolved
packages/eddsa-poseidon/src/HashFunction.ts Show resolved Hide resolved
packages/eddsa-poseidon/src/utils.ts Outdated Show resolved Hide resolved
packages/eddsa-poseidon/src/utils.ts Outdated Show resolved Hide resolved
packages/eddsa-poseidon/src/utils.ts Outdated Show resolved Hide resolved
@hannahredler
Copy link
Contributor Author

@cedoor comments addressed - ready for re-review 👍

packages/eddsa-poseidon/src/HashFunction.ts Show resolved Hide resolved
packages/eddsa-poseidon/src/index.ts Show resolved Hide resolved
packages/eddsa-poseidon/src/blake.ts Outdated Show resolved Hide resolved
@sripwoud sripwoud added the feature 🚀 This is enhancing something existing or creating something new label Sep 22, 2024
Copy link
Member

@cedoor cedoor left a comment

Choose a reason for hiding this comment

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

LGTM, just two comments before merging!

packages/eddsa-poseidon/src/utils.ts Outdated Show resolved Hide resolved
packages/eddsa-poseidon/tests/blake2.test.ts Outdated Show resolved Hide resolved
@hannahredler hannahredler requested a review from cedoor October 13, 2024 10:09
@cedoor cedoor merged commit 25f30b1 into privacy-scaling-explorations:main Oct 22, 2024
2 checks passed
Copy link

gitpoap-bot bot commented Oct 22, 2024

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2024 ZK-KIT Contributor:

GitPOAP: 2024 ZK-KIT Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🚀 This is enhancing something existing or creating something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support more hash functions to generate the public key in the eddsa-poseidon package
4 participants