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

New Poseidon library #245

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

New Poseidon library #245

wants to merge 12 commits into from

Conversation

cedoor
Copy link
Member

@cedoor cedoor commented Apr 9, 2024

Description

Note

No need for in-depth review here, but still required to merge. Open to any discussion of course.

This PR originated from #238, a proposal for using Noble, an audited library with many implementations that can be used on ZK-Kit to make code more secure.

This PR aimed to verify whether the performance and size of the bundle were equal to or better than the alternatives. The work led to the following conclusion: Noble seems solid, but the abstract implementation of Poseidon, which allows implementations with arbitrary parameters and constants to be built on top of it, still seems slow in comparison to circomlibjs and poseidon-lite. The bundle size is the same as that of poseidon-lite.

However, this branch could still be merged and remain in an experimental state (the package will not be shown in the readme nor published), pending any progress.

A future work that might be worth spending time on is to provide a version of Poseidon with the optimization introduced by Filecoin in Neptune, which could increase performance by sacrificing the bundle size.

Results

name ops margin samples promise min max mean median marginOfError
PoseidonLite v0.2.0 - 50 hashes 128 1.5 82 0.007449643571428571 0.012513912857142857 0.007838777611498256 0.007758738071428572 0.0005438473932657464 1.5016815772915084
CircomlibJS Poseidon v0.0.8 - 50 hashes 116 1.56 77 0.008292171428571428 0.0132828536 0.00858911488262214 0.008471577833333334 0.0006014991010496103 1.5642180021499086
ZK-Kit Poseidon - 50 hashes 78 0.33 68 0.012469885 0.013173915 0.012799256480147065 0.012803638499999999 0.0001790579589654917 0.33251443458087543

Related Issue(s)

Re #238

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
  • I have made corresponding changes to the documentation
  • 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
  • Any dependent changes have been merged and published in downstream modules

@cedoor cedoor marked this pull request as ready for review April 11, 2024 15:12
@cedoor cedoor requested review from 0xbok, artwyman and 0xjei April 11, 2024 15:12
@artwyman
Copy link
Collaborator

Check if I'm reading your performance chart right: It seems that the mean is much slower than the other libaries, but the median is much faster. If that's accurate it could be hiding a first-time setup cost issue, which might be reasonable to accept or address separately.

However it looks like the min/max values don't make sense (maybe they're reversed) and the mean/median is larger than the max in particular in most cases, so I don't know if I trust this chart in general. Can you check that it's being generated correctly?

The structure of the library seems reasonable (similar to poseidon-lite), but I haven't checked over all the code in detail.

@0xjei 0xjei removed their request for review April 17, 2024 14:23
@cedoor cedoor requested a review from sripwoud as a code owner July 16, 2024 13:07
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