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

Implement CurveExt::hash_to_curve for {bn256::G1,grumpkin::G1} #47

Merged

Conversation

han0110
Copy link
Contributor

@han0110 han0110 commented May 29, 2023

This PR takes the hash_to_field from pasta_curves and implements the rest CurveExt::hash_to_curve for these curves:

  • bn256::G1 and grumpkin::G1

    With Shallue-van de Woestijne method. Originally I was exploring implementation of Simplified SWU for AB == 0 for them, but after playing with the sage script to find their isogeny it turns out the degree is so high (59) which might be even slower than the SVDW method (I saw in gnark they are also using SVDW method for BN256).

    Although the spec doens't prepare test vectors for these 2 curves, I found in gnark they have the output of the same input from the spec with the same method for bn254 here, so perhaps checking with that we can have more confidence on the implementation. This PR aims for letting the CI to run the check for us, and the difference between pasta_curves and spec could be found in this diff, simply speaking the difference in hash_to_field are:

    1. Use sha256 instead of blake2b
    2. Use k = 128 instead of k = 256 (which seems more accurate)
    3. Interpret the digest as big-endian integer and reduce it with modulus directly instead of using from_u512

    For the first point it seems fine, but for the other 2 I'm not sure if we should follow the implementation of pasta_curves or the spec..

  • secp256k1::Secp256k1

    With Simplified SWU for AB == 0. Parameters are taken from 3-isogeny map for secp256k1 and implementation is modified from https://github.com/zcash/pasta_curves.
    It turns out we need to update the new_curve_impl to support a != 0 curve to have isogeny of secp256k1 to do the addition (we could copy paste the addition algorithm but it seems not looking good). So I'd keep this for future work.

And internally the hash function is using blake2b following pasta_curves, not sure if we need to make it generic but at least for IPA parameter generation I think it's fine to just use blake2b.

@huitseeker
Copy link
Contributor

huitseeker commented Jun 8, 2023

I can confirm this branch forms the last bit needed to create a Nova that's working and tested using the Grumpkin/bn256 cycle implemented in this repo: microsoft/Nova@main...huitseeker:Nova:bn-gr-h2c (joint work with @leonardoalt)

@han0110 han0110 force-pushed the feature/hash-to-curve branch 2 times, most recently from bc24236 to 3ba1790 Compare June 11, 2023 06:06
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

This LGTM, especially given the other PR #50

I've opened han0110#1 to give one direction in which one could use the provided field elements in gnark-crypto to test the existing map-to-curve funtionality

@han0110 han0110 changed the title Implement CurveExt::hash_to_curve for {bn256::G1,grumpkin::G1,secp256k1::Secp256k1} Implement CurveExt::hash_to_curve for {bn256::G1,grumpkin::G1} Jun 13, 2023
@han0110
Copy link
Contributor Author

han0110 commented Jun 13, 2023

I've opened han0110#1 to give one direction in which one could use the provided field elements in gnark-crypto to test the existing map-to-curve funtionality

Thanks for sharing the idea for testing, I've merged the PR and now it should be easier to review the implementation with those test vector.

@Dimitri-Koshelev
Copy link

You can verify this implementation against my own as well: mratsim/constantine#190.

This includes Sage code to derive H2C constants: https://github.com/mratsim/constantine/pull/190/files#diff-cbd719879e1fff4703a911a07be9385db426686f3313c61509d06a4ebff579ca

If H2C via SVDW is a bottleneck, the work from @dishport optimizes it:

@mratsim, thank you for citing my works. However, some of them are not relevant anymore. The state-of-the art in H2C research is exhibited in my last article https://eprint.iacr.org/2021/1082.

@kilic kilic self-requested a review June 21, 2023 07:36
@han0110 han0110 merged commit 8cc62b8 into privacy-scaling-explorations:main Jun 21, 2023
@han0110 han0110 mentioned this pull request Jun 23, 2023
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