-
Notifications
You must be signed in to change notification settings - Fork 11
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 BN254 #55
Add BN254 #55
Conversation
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.
Oh right, ok, you're only using gnark repo to fetch the generators it see.
LGTM to me ! (merely adding a reference to the tests you're using but that's it!)
Thanks !!!
pairing/bn254/point_test.go
Outdated
} | ||
} | ||
|
||
func TestHashToPoint(t *testing.T) { |
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 you include inside the code which reference are you taking, so it's not lost in the ethers ?
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.
Oh yes, of course. It's actually taken from the contract tests at https://github.com/kevincharm/bls-bn254, but let me quickly make a script in there to make it as easy as possible to generate test vectors. Then I'll add a link/instructions to it in the code here.
Thanks for the review @nikkolasg ! Updated the test with instructions on how to compute the reference values. |
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.
So, this is basically the same code as in https://github.com/ethereum/go-ethereum/tree/master/crypto/bn256/cloudflare, right?
Edit, yeah you said so. Let me review the Kyber specifics then.
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.
Here's a first batch of comments
pairing/bn254/README.md
Outdated
branch `master`: | ||
|
||
``` | ||
BenchmarkG1-4 10000 154995 ns/op | ||
BenchmarkG2-4 3000 541503 ns/op | ||
BenchmarkGT-4 1000 1267811 ns/op | ||
BenchmarkPairing-4 1000 1630584 ns/op | ||
``` | ||
|
||
branch `lattices`: | ||
|
||
``` | ||
BenchmarkG1-4 20000 92198 ns/op | ||
BenchmarkG2-4 5000 340622 ns/op | ||
BenchmarkGT-4 2000 635061 ns/op | ||
BenchmarkPairing-4 1000 1629943 ns/op | ||
``` | ||
|
||
official version: | ||
|
||
``` | ||
BenchmarkG1-4 1000 2268491 ns/op | ||
BenchmarkG2-4 300 7227637 ns/op | ||
BenchmarkGT-4 100 15121359 ns/op | ||
BenchmarkPairing-4 50 20296164 ns/op | ||
``` |
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.
Are these the actual benchmark for this version or did they get carried over from the previous repo?
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.
Carried over from current repo (bn256 package). Should I just remove these?
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.
Yeah, let's remove them
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.
Removed these.
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.
Thanks, almost there I'm 25/28 through it
pairing/bn254/point.go
Outdated
// `mapToPoint` implements a specialised SW mapping for BN curves from the paper | ||
// | ||
// Fouque, P.-A. and M. Tibouchi, "Indifferentiable Hashing to Barreto--Naehrig Curves", | ||
// In Progress in Cryptology - | ||
// LATINCRYPT 2012, pages 1-17, | ||
// DOI 10.1007/978-3-642-33481-8_1, 2012, | ||
// <https://doi.org/10.1007/978-3-642-33481-8_1>. | ||
// | ||
// Ref implementations: | ||
// | ||
// https://github.com/herumi/mcl/blob/5f4449efd08388009f9abce06c44fc26730193e7/include/mcl/bn.hpp#L343 | ||
// https://github.com/thehubbleproject/hubble-contracts/blob/f1c13fe4e1a0dc9ab1f150895de7c0e654ee46b0/contracts/libs/BLS.sol#L139 | ||
func mapToPoint(x *big.Int) (*big.Int, *big.Int) { |
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.
How different is this from the simplified SW mapping https://datatracker.ietf.org/doc/html/rfc9380#section-6.6.2 ?
Seems Fouque-Tibouchi is a making the SW construction explicit in the case of BN curves, and according to the SSWU paper for BLS, it seems that it's really the same as the SSWU but for BN curves:
Fouque and Tibouchi [FT12] also analyze the original construction of Shallue and van de Woestijne
specifically for the case of Barreto-Naehrig curves [BN06]; in Section 3, we give a very similar construction tailored to the BLS12-381 curve
So I guess it's the same but for BN. And so it counts as SSWU ✅
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.
Fouque-Tibouchi references the Shallue-van de Woestijne paper and not the Ulas paper so I think that this is actually the "SVDW" mapping described in 6.6.1 rather than SSWU.
pairing/bn254/suite.go
Outdated
var DEFAULT_DOMAIN_G1 = []byte("BLS_SIG_BN254G1_XMD:KECCAK-256_SSWU_RO_NUL_") | ||
var DEFAULT_DOMAIN_G2 = []byte("BLS_SIG_BN254G2_XMD:KECCAK-256_SSWU_RO_NUL_") |
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.
This doesn't matches RFC9380 naming convention:
CURVE_ID || "_" || HASH_ID || "_" || MAP_ID || "_" || ENC_VAR || "_"
The fields CURVE_ID, HASH_ID, MAP_ID, and ENC_VAR are ASCII-encoded strings of at most 64 characters each. Fields MUST contain only ASCII characters between 0x21 and 0x7E (inclusive), except that underscore (i.e., 0x5F) is not allowed.
So, I think this would be more correct:
CURVE_ID = BN254G1
HASH_ID = XMD:KECCAK-256
MAP_ID = SSWU (see other comment, but I think it checks out that this is SSWU for BN curves)
ENC_VAR=RO
So:
var DEFAULT_DOMAIN_G1 = []byte("BLS_SIG_BN254G1_XMD:KECCAK-256_SSWU_RO_NUL_") | |
var DEFAULT_DOMAIN_G2 = []byte("BLS_SIG_BN254G2_XMD:KECCAK-256_SSWU_RO_NUL_") | |
var DEFAULT_DOMAIN_G1 = []byte("BN254G1_XMD:KECCAK-256_SSWU_RO_") | |
var DEFAULT_DOMAIN_G2 = []byte("BN254G2_XMD:KECCAK-256_SSWU_RO_") |
What is the RO_NUL_
in the original proposal, why the extra _NUL
?
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 was following the other DSTs in this file: https://github.com/drand/drand/blob/master/crypto/schemes.go#L221
But since this lib is not actually about BLS sigs it makes sense to not include that in the default DST, and set it on the drand side.
Also I think SSWU
should actually instead be SVDW
as stated in https://datatracker.ietf.org/doc/html/rfc9380#section-8.10.
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 having a hard time convincing myself either way. It could be SVDW but it looks like Fouque-Tibouchi came after the first simplifications for SVDW were found which are SSWU, no?
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.
SSWU in RFC9380 seems to explicitly refer to the algorithm defined in 6.6.2. In our case, BN254 has curve A parameter == 0 so we actually need to additionally compute an isogenous curve as in 6.6.3. From reading Wahby & Boneh pg2, it sounds like the Fouque-Tibouchi mapping is explicitly different to all the others? SvdW, Fouque-Tibouchi, and the SSWU mappings all output different points:
❯ sage suite_bn254.sage
u: 7105195380181880595384217009108718366423089053558315283835256316808390512725
svdw:
15712026073284912390314437469998384224444098668487062629391055065992760594476 12286200326952730997678485294504458874299852441720220164574895986935631271221
ft:
19485131671658523517646027848906165907640971588430452127920614621547697012573 7252485661626054658053752721536032361940074412825453078837989033903251969412
generic sswu:
(7433244435151743403934667274157583038597013229141355912918907345679928483392 : 3341345691842296612745507125415299735564087771630588448932624272206506288268 : 1)
optimised sswu:
(7433244435151743403934667274157583038597013229141355912918907345679928483392 : 3341345691842296612745507125415299735564087771630588448932624272206506288268 : 1)
Code here: https://github.com/kevincharm/draft-irtf-cfrg-hash-to-curve/pull/1/files
So I think it's neither SVDW or SSWU?
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.
This is great, thanks a lot @kevincharm, we now know we don't have an easy way out I guess!
Let me cheat: @armfazh, I see your name on the RFC, can you help us out here?
How should we define our Suite ID for the curve BN254 if we're using Fouque-Tibouchi for hash to curve?
Or in the frame of writing RFC9380, did you somehow conclude it HAD to be either SSWU or SVDW but not Fouque-Tibouchi and so you've decided to not offer an FT option for some reasons?
For BN254, since AB = 0 we need to use an isogenous curve to do SSWU sadly and that seems more expensive / less practical to do in Solidity. So it seems our two options are either SVDW or FT, with the latter being more optimised for BN curves.
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.
Yeah, then let's use SVDW 👍🏻 Thanks a lot
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.
SVDW will work ok, also I recommend to describe the suite in full in a RFC-like document for further reference.
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.
@AnomalRoil I've updated the code to use SVDW mapping, and tested it against vectors from sagemath scripts from the RFC (and also the new Solidity & ts implementation at https://github.com/kevincharm/bls-bn254).
Also replaced the expand_message_xmd
implementation with a better one from kilic/bls12-381 (and tested it against the gnark one, both of these modified to use keccak256 instead of sha256).
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.
Thanks a lot! I'll review it next week at the earliest 🙈
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.
No worries, I really appreciate your time in reviewing this :)
This reverts commit 8e53658.
replaying fix from geth, see: ethereum/go-ethereum@ec64358 When using -buildmode=shared, R15 is clobbered by a global variable access; use a different register instead.
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 was a big one! But LGTM now, a few extra changes come to mind, but nothing blocking.
* implement ethereum bn254 * pull in missing changes from geth * make G2 points affine in ValidatePairing * move bn254 to own dir * fix bn254 GT Base and Null * test G1 suite against gnark-crypto * adapt G2 test to gnark-crypto, fix bn254 marshal ids * fix G1 hashToPoint reference tests * fix pairing/bn254/gfp_amd64.s * use keccak256 instead of sha256 in bn254 * implement TestGT for bn254 * implement hashToField, expandMsgXmd * implement mapToPoint * implement hashToPoint * rename toPointG1 -> fromBigInt * add comments on mapToPoint, cleanup * fix TestPointG1_HashToPoint with new keccak256 impl * use ref implementation of expand_message_xmd * panic if DST length > 255 * remove unnecessary iteration in expandMsgXmd * update tests to use full length DSTs * set DST in group instead of suite * fix: actually write bytes in zeroPadBytes * fix: leftpad intermediate prb in expandMsgXmd * fix: leftpad intermediate xored hashes in expandMsgXmd * add instructions to create hash-to-point reference values, remove old TODO * remove irrelevant issue400 test for bn254 * use CX instead of R15 in amd64 replaying fix from geth, see: ethereum/go-ethereum@ec64358 When using -buildmode=shared, R15 is clobbered by a global variable access; use a different register instead. * remove gfp.h in bn254 * use geth's Unmarshal func: error out on invalid coordinates * make constants z0 and z1 private * check that random points are different in TestG1Ops * implement Stringer interface for bn254 groups * remove irrelevant benchmarks * implement SVDW from RFC9380 for bn254 G1 map-to-point * encode the right variable in hashToField * borrow expand_message_xmd implementation from kilic-bls12381 * update test vectors in TestPointG1_HashToPoint, TestHashToField * test expandMsgXmd against a copy of gnark's * fix default DST for hash-to-curve * remove unused fromBigInt func * add sources for gfp exp/sqrt functions * use curveB constant in g(x) * create new DST buffers instead of passing refs
This PR adds a BN254 (the curve used in the EVM) package, to support operations required for BLS-on-BN254.
Most of the code has been taken from geth. This is not the same curve as the one that already exists in this repo (it has different parameters).
I have implemented a constant-time hash-to-point function for G1 that uses a specialised SW mapping for BN curves as described in Indifferentiable Hashing to Barreto–Naehrig Curves. I mostly just ported it from hubble and it's tested against mcl.Now using the general SVDW mapping described in RFC9380 6.6.1 to comply with the RFC, while also being cheaper to implement in the EVM.Important to note that
expand_message_xmd
in this implementation uses H = keccak256 instead of sha256 (SHA-3 is included as a recommended choice in rfc9380) for maximal compatibility with EVM networks. The motivation is that some rollups such as Scroll do not support sha256.Accompanying contracts, tests and JS lib can be found at bls-bn254. Onchain
hashToPoint
costs around 50k-75k gas. Here is an example tx that validates an example beacon, which involves callinghashToPoint
,isValidSignature
andverifySingle
(among other logic) and costs only ~234k gas.