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

Support bls12 381 temporarilly #87

Closed
wants to merge 8 commits into from
Closed

Support bls12 381 temporarilly #87

wants to merge 8 commits into from

Conversation

CPerezz
Copy link
Member

@CPerezz CPerezz commented Sep 11, 2023

This is an attempt to temporarily providing support for Bls12-381 in halo2curves.

This will require first to complete: privacy-scaling-explorations/bls12_381#4 so that CurveExt is implemented for G1Projective so that the trait requirements for CruveEndo are met.

@CPerezz
Copy link
Member Author

CPerezz commented Sep 14, 2023

See: privacy-scaling-explorations/bls12_381#4 (comment)

Somehow we still fail endo tests for bls although all the constants for it are correct (at least tests for them pass). Note that the tests don't include ZETA-related tests which are exactly the ones we need to pass here. But is weird as the way to derive Z does not seem crazy and yet does not work. I'm definitely missing something here.

And also, the endo params have been derived twice resulting on the exact same thing.

@CPerezz
Copy link
Member Author

CPerezz commented Sep 14, 2023

This was the test used in Gnark to derive the parameters:

func TestGenGLV(t *testing.T) {

	var lambda, r big.Int
	var l Lattice

	// // bls
	r.SetString("73eda753299d7d483339d80809a1d80553bda402fffe5bfeffffffff00000001", 16)
	lambda.SetString("ac45a4010001a40200000000ffffffff", 16)

	// // bn
	// r.SetString("30644e72e131a029b85045b68181585d2833e84879b9709143e1f593f0000001", 16)
	// lambda.SetString("30644e72e131a029048b6e193fd84104cc37a73fec2bc5e9b8ca0b2d36636f23", 16)

	// //  grumpkin
	// r.SetString("30644e72e131a029b85045b68181585d97816a916871ca8d3c208c16d87cfd47", 16)
	// lambda.SetString("30644e72e131a0295e6dd9e7e0acccb0c28f069fbb966e3de4bd44e5607cfd48", 16)

	// // book
	// r.SetString("1461501637330902918203687013445034429194588307251", 10)
	// lambda.SetString("903860042511079968555273866340564498116022318806", 10)

	// // secp
	// r.SetString("fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141", 16)
	// lambda.SetString("5363ad4cc05c30e0a5261c028812645a122e22ea20816678df02967c1b23bd72", 16)

	// // eq
	// r.SetString("40000000000000000000000000000000224698fc094cf91b992d30ed00000001", 16)
	// lambda.SetString("12ccca834acdba712caad5dc57aab1b01d1f8bd237ad31491dad5ebdfdfe4ab9", 16)

	// // ep
	// r.SetString("40000000000000000000000000000000224698fc0994a8dd8c46eb2100000001", 16)
	// lambda.SetString("06819a58283e528e511db4d81cf70f5a0fed467d47c033af2aa9d2e050aa0e4f", 16)

	PrecomputeLattice(&r, &lambda, &l)

	fmt.Printf("a1 0x%x %d\n", l.V1[0].Bytes(), l.V1[0].Sign())
	fmt.Printf("b1 0x%x %d\n", l.V1[1].Bytes(), l.V1[1].Sign())
	fmt.Printf("a2 0x%x %d\n", l.V2[0].Bytes(), l.V2[0].Sign())
	fmt.Printf("b2 0x%x %d\n", l.V2[1].Bytes(), l.V2[1].Sign())

	fmt.Printf("g1 0x%x\n", l.b1.Bytes())
	fmt.Printf("g2 0x%x\n", l.b2.Bytes())

}

Unless lambda is wrong, I'm not sure what could be the issue anymore. Maybe @kilic has a hint?

@mratsim
Copy link
Contributor

mratsim commented Sep 19, 2023

You might want to try my parameter generators here: https://github.com/mratsim/constantine/blob/master/sage/derive_endomorphisms.sage

Even when taking the formulats from Guide to pairing-based cryptography, I had an issue.

And for easier to modify/dump test vectors, you can use this old self-contained file: https://github.com/mratsim/constantine/blob/2613356281ed33626bd9cc8b2cbaf68ae11a0867/sage/lattice_decomposition_bls12_381_g1.sage

@kilic
Copy link
Collaborator

kilic commented Sep 19, 2023

@CPerezz in the script you send parameters are generated are good however there is a misnaming issue that g1 should be gamma2 and g2 should be gamma1. So can you try it with these parameters which are working in my end:

const ENDO_PARAMS_BLS: EndoParameters = EndoParameters {
    // round(b2/n)
    gamma2: [0x63f6e522f6cfee30u64, 0x7c6becf1e01faadd, 0x01, 0x0],
    // round(-b1/n)
    gamma1: [0x02u64, 0x0, 0x0, 0x0],
    b1: [0x01u64, 0x0, 0x0, 0x0],
    b2: [0x0000000100000000, 0xac45a4010001a402, 0x0, 0x0],
};

@CPerezz CPerezz marked this pull request as ready for review September 27, 2023 11:16
@CPerezz
Copy link
Member Author

CPerezz commented Sep 27, 2023

The PR on which this relies in order to be merged is all finalized. So this can start to be reviewed too considering that I'll need to get ridd from the git_dependencies.

@CPerezz CPerezz requested review from han0110 and kilic and removed request for han0110 September 27, 2023 11:17
Cargo.toml Outdated
@@ -33,8 +33,10 @@ serde = { version = "1.0", default-features = false, optional = true }
serde_arrays = { version = "0.1.0", optional = true }
hex = { version = "0.4", optional = true, default-features = false, features = ["alloc", "serde"] }
blake2b_simd = "1"
bls12_381 = { git = "https://github.com/privacy-scaling-explorations/bls12_381", branch = "tmp_curve_ext", features = ["groups", "basefield"] }

Choose a reason for hiding this comment

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

Hey Carlos, depending on a branch would be problematic here.

Every time people install Halo2curve or the CI runs the test, they pull the branch head. So if an incompatible commit is introduced to tmp_curve_ext branch then all the packages break.

We should release a tag on bls12_381 repo and then depends on the tag here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to release a tag and update it here as well as rebasing this!
Will do it within the next days! 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed the release was done already back in October. See: https://github.com/privacy-scaling-explorations/bls12_381/releases/tag/v2023_10_26

Will update this in main

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 9329af1

@CPerezz
Copy link
Member Author

CPerezz commented Dec 18, 2023

Updated @ChihChengLiang 🥳

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.

I think this is finally ready! Thanks for taking care of this :)
LGTM

@CPerezz
Copy link
Member Author

CPerezz commented Dec 20, 2023

I think this is finally ready! Thanks for taking care of this :) LGTM

Notice that this branch will never get merged (at least until we have a crates.io dep instead of a git dep).
This passes by refactoring all the stuff we have with the RFC to Zcash.

Copy link

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

I didn't review the rest of the parts.
LGTM for the dependency update.

@CPerezz
Copy link
Member Author

CPerezz commented Feb 21, 2024

This should now be updated after privacy-scaling-explorations/bls12_381#6 is merged.
And we should also consider what to do with this crate. If we don't publish it, we will never be able to merge it.

cc: @ed255 @han0110 @kilic any opinions?
I think this is relevant as after 4844 and Verkle, BLS will be probably the next standard.

@davidnevadoc
Copy link
Contributor

Superseded by #162

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.

5 participants