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

Make generic Groth16 implementation #758

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

Conversation

jonas-lj
Copy link
Contributor

@jonas-lj jonas-lj commented Apr 4, 2024

This PR replaces the BLS12-381 Groth16 implementation with a generic implementation that uses the GroupElement and Pairing 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.

@jonas-lj jonas-lj requested review from benr-ml and joyqvq April 8, 2024 17:42
@jonas-lj jonas-lj marked this pull request as ready for review April 8, 2024 17:42
fastcrypto-zkp/src/bls12381/api.rs Outdated Show resolved Hide resolved
fastcrypto-zkp/src/bls12381/api.rs Outdated Show resolved Hide resolved
fastcrypto-zkp/src/groth16/generic_api.rs Outdated Show resolved Hide resolved
fastcrypto/src/serde_helpers.rs Outdated Show resolved Hide resolved
fastcrypto-zkp/src/groth16/generic_api.rs Outdated Show resolved Hide resolved
)?;
i += G2_SIZE;

let n = u64::from_le_bytes(
Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Contributor Author

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.

fastcrypto-zkp/src/groth16/generic_api.rs Outdated Show resolved Hide resolved
pub(crate) mod generic_api;

#[derive(Debug, Deserialize)]
pub struct Proof<G1: Pairing>
Copy link
Contributor

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?

Copy link
Contributor Author

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.

fastcrypto-zkp/src/groth16/mod.rs Outdated Show resolved Hide resolved
@jonas-lj jonas-lj requested a review from benr-ml April 11, 2024 09:03
Copy link
Contributor

@benr-ml benr-ml left a 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

fastcrypto-zkp/src/bls12381/api.rs Outdated Show resolved Hide resolved
)?;
i += G2_SIZE;

let n = u64::from_le_bytes(
Copy link
Contributor

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

@jonas-lj jonas-lj requested a review from benr-ml April 15, 2024 14:31
@jonas-lj
Copy link
Contributor Author

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

Added in #772.

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.

None yet

2 participants