-
Notifications
You must be signed in to change notification settings - Fork 78
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
Poseidon circomlib #670
base: dev
Are you sure you want to change the base?
Poseidon circomlib #670
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #670 +/- ##
==========================================
+ Coverage 55.55% 55.75% +0.20%
==========================================
Files 193 192 -1
Lines 20854 20831 -23
==========================================
+ Hits 11585 11614 +29
+ Misses 9269 9217 -52 ☔ View full report in Codecov by Sentry. |
c6fcdd6
to
1311c48
Compare
Sorry, the last force-push was me pressing the "Update with rebase" button. I was expecting GH to show that it made the rebase, not I. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work! love the generic implementation that caters for all rates up to 16 :)
@@ -0,0 +1,168 @@ | |||
use std::{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason that we are dynamically generating these constants as part of the build step, instead of storing them statically in the repo? is it to prevent them becoming stale and outdated when the upstream generation method changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hypothetically, it could be used to keep up with upstream.
I was mainly thinking about ease of audit.
ofc, we could commit them to the repo if there are strong opinions about that.
|
||
let mut f = File::create(&dest_path)?; | ||
|
||
writeln!(f, "use halo2_proofs::halo2curves::bn256::Fr as F;")?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way (or downside) to directly serialise and store (round_const, mds, mds_inv)
without having to convert them into this specific rust format? am just worried about the maintainability (and readability) of the following manual rust code/syntax generation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you feel it is not readable and the overhead of deserialization is negligible, feel free to make a PR for that.
Since I wrote that script, I'm obviously biased and can't say how readable it is.
This PR adds a functionality to compute a Poseidon hash compatible with https://github.com/iden3/circomlib/blob/master/circuits/poseidon.circom
The build script generates the Poseidon constants and writes them to file.