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

Add BLS12-381 #162

Merged
merged 5 commits into from
Jul 17, 2024
Merged

Add BLS12-381 #162

merged 5 commits into from
Jul 17, 2024

Conversation

kilic
Copy link
Collaborator

@kilic kilic commented May 30, 2024

Another take of BLS12 addition with a PR chain #154 #160 #161.

This PR also introduces compile time flag configuration to follow zcash serialization for BLS12381. However it leaves uncompressed encoding as it was, without usage of flags.

@dragan2234
Copy link

dragan2234 commented Jun 4, 2024

@kilic just for the record, I was trying to generate simple proof with bls12-381 with this branch and still getting an error:

the trait bound `halo2curves::bls12381::G1Affine: halo2_proofs::halo2curves::serde::SerdeObject` is not satisfied
required for `halo2curves::bls12381::G1Affine` to implement `halo2_backend::helpers::SerdeCurveAffine`
required for `ParamsKZG<Bls12381>` to implement `Params<halo2curves::bls12381::G1Affine>`

and

the trait bound `halo2curves::bls12381::G2Affine: halo2_proofs::halo2curves::serde::SerdeObject` is not satisfied
required for `halo2curves::bls12381::G2Affine` to implement `halo2_backend::helpers::SerdeCurveAffine`
required for `ParamsKZG<Bls12381>` to implement `Params<halo2curves::bls12381::G1Affine>`

@kilic
Copy link
Collaborator Author

kilic commented Jun 5, 2024

@dragan2234 both bls12381::G1Affine and bls12381::G2Affine does implement CurveAffine and SerdeObject which are only bounds for SerdeCurveAffine. In your workspace does all SerdeObject resolve to the one in this PR?

@dragan2234
Copy link

@kilic This is my Cargo.toml:

halo2curves = { git = "https://github.com/kilic/halo2curves.git", branch = "bls12" }

I double-checked the files and it's the same as in this PR.

SerdeObject for groups is done in macro in derive/curve.rs if I understand correctly.
and the line where it fails:

    let vk = keygen_vk(&params, &MyCircuit).expect("keygen_vk should not fail");

it also says:

keygen.rs(23, 8): required by a bound in `keygen_vk`

which resolves to:

/// Generate a `VerifyingKey` from an instance of `Circuit`.
/// By default, selector compression is turned **ON**.
///
/// **NOTE**: This `keygen_vk` is legacy one, assuming that `compress_selector: true`.
/// Hence, it is HIGHLY recommended to pair this util with `keygen_pk`.
/// In addition, when using this for key generation, user MUST use `compress_selectors: true`.
pub fn keygen_vk<C, P, ConcreteCircuit>(
    params: &P,
    circuit: &ConcreteCircuit,
) -> Result<VerifyingKey<C>, Error>
where
    C: CurveAffine,
    P: Params<C>,
    ConcreteCircuit: Circuit<C::Scalar>,
    C::Scalar: FromUniformBytes<64>,
{
    keygen_vk_custom(params, circuit, true)
}

Line:

    P: Params<C>,

where Params is:

pub trait Params<C: CurveAffine>: Sized + Clone + Debug {

@kilic
Copy link
Collaborator Author

kilic commented Jun 5, 2024

@dragan2234 On my end I'm able to run halo2_proof::tests::plonk_api with BLS12-381

  • change halo2curves dependency in all (front, back, middle, ..) crates
  • change ParamsKZG::<Bn256>::new(K) to ParamsKZG::<Bls12381>::new(K)

@dragan2234
Copy link

@kilic thanks! I didn't change the dependency in other crates, the bug is fixed now (still can't produce proofs, but I think it's not related to the curve this time, but my circuit)

Copy link
Contributor

@davidnevadoc davidnevadoc left a comment

Choose a reason for hiding this comment

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

At last!! Thanks for the work @kilic :)
LGTM 👍

@@ -361,6 +362,16 @@ pub(crate) fn impl_field(input: TokenStream) -> TokenStream {
});
(borrow as u8) & 1 == 1
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Returns whether or not this element is strictly lexicographically
/// larger than its negation.

) => {


// **Compressed formats**:
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be good to keep these docs in src/derive/serde.rs

@@ -68,412 +61,125 @@ macro_rules! new_curve_impl {
$constant_b:expr,
$curve_id:literal,
$hash_to_curve:expr,
$flag_config:expr,
standard_sign
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 add implementation for the non-standard sign as well?

@davidnevadoc
Copy link
Contributor

Ready to merge? @kilic

@kilic kilic added this pull request to the merge queue Jul 17, 2024
Merged via the queue into privacy-scaling-explorations:main with commit 758cbf9 Jul 17, 2024
12 checks passed
This was referenced Jul 23, 2024
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.

3 participants