-
Notifications
You must be signed in to change notification settings - Fork 141
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
Make generic Groth16 implementation #758
Conversation
4ffcca8
to
3d538bd
Compare
)?; | ||
i += G2_SIZE; | ||
|
||
let n = u64::from_le_bytes( |
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.
should we verify that n>0?
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.
let's double check that this is backward compatible
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.
That's checked in test_prepare_pvk_bytes
in api_tests.rs.
pub(crate) mod generic_api; | ||
|
||
#[derive(Debug, Deserialize)] | ||
pub struct Proof<G1: Pairing> |
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.
while it's ok to how we use it today, isn't in general this is symmetric in the sense that a proof can be G2, G1, G2, where pairing is a trait of G1?
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.
I'm not sure I follow. You'd prefer a proof to be (G1, G2, G1) because it's smaller, assuming that we've always put G1 to be the smallest, right? But yes, the pairing is symmetric, so there's nothing stopping us from implementing Pairing
as a trait for G1::Other
for all G1: Pairing
.
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.
Can we add regression tests in a different PR (with fixed bytes of vk & proofs that worked with the previous version, including of edge cases) and only afterwards submit this one? I want to make sure we are not breaking the current APIs
b03b8c1
to
ed4cd5c
Compare
)?; | ||
i += G2_SIZE; | ||
|
||
let n = u64::from_le_bytes( |
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.
let's double check that this is backward compatible
Added in #772. |
77199dd
to
caeda1a
Compare
This PR replaces the BLS12-381 Groth16 implementation with a generic implementation that uses the
GroupElement
andPairing
abstractions. This is much simpler and wraps calls to external libraries.We can also use this for the BN254 Groth16 implementation, but it requires that we first add the BN254 construction to the supported groups.
Benchmarks have demonstrated, that the performance is unchanged.