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

feat: implement the SSWU hash_to_curve for secp256k1 #110

Conversation

duguorong009
Copy link
Contributor

@duguorong009 duguorong009 commented Dec 6, 2023

Description

Implement the Simplified SWU method(specifically, Simplified SWU for AB == 0) for hash_to_curve of secp256k1 curve

Related issues

Changes

  • implement iso_map_secp256k1 for secp256k1 hash_to_curve
  • update the sswu_hash_to_curve func
  • add a new iso curve for secp256k1 hash_to_curve

@duguorong009
Copy link
Contributor Author

@davidnevadoc
Please review the PR.

Copy link
Contributor

@mratsim mratsim left a comment

Choose a reason for hiding this comment

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

src/hash_to_curve.rs Outdated Show resolved Hide resolved
src/hash_to_curve.rs Outdated Show resolved Hide resolved
src/secp256k1/curve.rs Outdated Show resolved Hide resolved
@duguorong009
Copy link
Contributor Author

@mratsim
I have question about test vectors.
In the test vectors, the hash function used for hash_to_field is the sha256.("ciphersuite": "secp256k1_XMD:SHA-256_SSWU_RO_",)
This hash function is used in expand_message Plus, k value is 128.

On the other hand, halo2curve impl uses the different hash function(blake2b) and k value(256).
https://github.com/privacy-scaling-explorations/halo2curves/blob/main/src/hash_to_curve.rs#L12-L31

So, I think we can't use the test vector you indicated.
What do you think?

cc @davidnevadoc

@mratsim
Copy link
Contributor

mratsim commented Dec 16, 2023

Ideally Hash-to-curve is generic over the hash function and the DST and security parameter k are associated constants (or all are generics or all are associated constants, depending on ergonomics), this way you can keep using Blake2b but can switch to SHA256 just for testing.

@duguorong009
Copy link
Contributor Author

I see what you mean. @mratsim

Current hash_to_field impl of halo2curves repo is based on zcash/pasta_curves implementation.
And it is not generic over hash function and other constants(DST, k, ...).
It is only tailored to use the blake2b hash function and assumed k value(256).
https://github.com/privacy-scaling-explorations/halo2curves/blob/main/src/hash_to_curve.rs#L10-L25
So, I think we need to update the hash_to_field implementation if we want to add the tests using test vectors.
Do you think it is necessary to update the hash_to_field impl so that it can be generic over hash func & constants?

If so, I think it could be a bit much for this PR.
We can work on another issue/PR for hash_to_field func update. Plus, enhance the tests by adding test vectors.

@mratsim
Copy link
Contributor

mratsim commented Dec 16, 2023

Current hash_to_field impl of halo2curves repo is based on zcash/pasta_curves implementation.
And it is not generic over hash function and other constants(DST, k, ...).
It is only tailored to use the blake2b hash function and assumed k value(256).
https://github.com/privacy-scaling-explorations/halo2curves/blob/main/src/hash_to_curve.rs#L10-L25

I see, that's unfortunate

So, I think we need to update the hash_to_field implementation if we want to add the tests using test vectors.
Do you think it is necessary to update the hash_to_field impl so that it can be generic over hash func & constants?

Yes, because Ethereum and Bitcoin uses SHA256 for BLS12-381 and secp256k1. But not pressing

If so, I think it could be a bit much for this PR.
We can work on another issue/PR for hash_to_field func update. Plus, enhance the tests by adding test vectors.

Agree that it's too much, but maybe you can fork the hash-to-curve repo, modify k to 256, the DST and the hash function so that it generates Blake2b vectors with k = 256?

@duguorong009
Copy link
Contributor Author

@mratsim
I've checked the hash_to_field func of both impls - halo2curves & sage version.
I think we cannot compare the result by simply replacing the hash function and constants.
In the sage version, they use the standard impl of hash_to_field which uses the expand_message.
On the other hand, the halo2curves uses the different algorithm.
Do you know the that algorithm, by any chance?
https://github.com/privacy-scaling-explorations/halo2curves/blob/main/src/hash_to_curve.rs#L12-L86

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've left a couple of comments, but other than that LGTM. Good job! :)

src/hash_to_curve.rs Outdated Show resolved Hide resolved
src/hash_to_curve.rs Outdated Show resolved Hide resolved
src/hash_to_curve.rs Outdated Show resolved Hide resolved
src/hash_to_curve.rs Outdated Show resolved Hide resolved
src/secp256k1/curve.rs Outdated Show resolved Hide resolved
@mratsim
Copy link
Contributor

mratsim commented Dec 18, 2023

@mratsim I've checked the hash_to_field func of both impls - halo2curves & sage version. I think we cannot compare the result by simply replacing the hash function and constants. In the sage version, they use the standard impl of hash_to_field which uses the expand_message. On the other hand, the halo2curves uses the different algorithm. Do you know the that algorithm, by any chance? https://github.com/privacy-scaling-explorations/halo2curves/blob/main/src/hash_to_curve.rs#L12-L86

It is expand_message_xmd, see zcash/pasta_curves@785ad53

@duguorong009
Copy link
Contributor Author

duguorong009 commented Dec 18, 2023

I've left a couple of comments, but other than that LGTM. Good job! :)

@davidnevadoc
I tried to take care of comments in here. 9ff891e

Do you have any comment about refactoring - creating utils module & moving the fe_from_str?

Pls give more comments.

@duguorong009
Copy link
Contributor Author

@mratsim
Thanks for help!

I've done the experiments to get the test vectors of secp256k1::hash_to_curve with k = 256 and blake2b hash used in hash_to_field.
Here is the result.
https://github.com/duguorong009/draft-irtf-cfrg-hash-to-curve/blob/secp256k1-blake2b-sswu-experiment/poc/vectors/secp256k1_XMD%3ABLAKE2b_SSWU_RO_.json
You can also check the experiment code here. duguorong009/draft-irtf-cfrg-hash-to-curve@0ce9706

Unfortunately, when I tried these test vectors in testing, the output results do not match.

Hence, I believe that there is diff in algorithms used in hash_to_field.

cc @davidnevadoc

src/secp256k1/curve.rs Outdated Show resolved Hide resolved
@davidnevadoc davidnevadoc added this pull request to the merge queue Dec 22, 2023
Merged via the queue into privacy-scaling-explorations:main with commit 534da5b Dec 22, 2023
7 checks passed
@CPerezz CPerezz mentioned this pull request Jan 10, 2024
huitseeker added a commit to huitseeker/Nova that referenced this pull request Jan 11, 2024
- Updated expected digest in supernova's pp digest,
- see privacy-scaling-explorations/halo2curves#110 for the upstream change
huitseeker added a commit to huitseeker/Nova that referenced this pull request Jan 11, 2024
- Updated expected digest in pp digest tests,
- see privacy-scaling-explorations/halo2curves#110 for the upstream change
github-merge-queue bot pushed a commit to lurk-lab/arecibo that referenced this pull request Jan 11, 2024
…250)

* chore: "Update halo2curves dependency and adjust grumpkin-msm source"

- Introduced `halo2curves` version `0.6.0` as a global dependency with additional features
- Eliminated specific target architecture dependency on `halo2curves`

- Testing with an updated `grumpkin-msm` source using a distinct git URL and branch: lurk-lab/grumpkin-msm#11

* fix: make parameters of supernova test use expect_test

* test: Update test expectations for test(_supernova)?_pp_digest

- Updated expected digest in pp digest tests,
- see privacy-scaling-explorations/halo2curves#110 for the upstream change

* chore: point the grumpkin-msm dependency back to lurk-lab
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.

Implement Simplified SWU hash to curve for Secp256k1
3 participants