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

Switches hash-to-field to new v06 hash system #87

Merged
merged 12 commits into from
May 5, 2020

Conversation

CarlBeek
Copy link
Collaborator

@CarlBeek CarlBeek commented Mar 18, 2020

What was wrong?

The BLS hash-to-curve spec is being changed from v05 -> v06. BLS hash-to-curve It is basically a change from repeated HKDF calls to direct SHA256 calls. This has the advantage of halving the number of SHA256 calls (and allows for other improvements for other curves in the hash-to-curve suite). See here for the full discussion: cfrg/draft-irtf-cfrg-hash-to-curve#202

The changes made are summarised in PRs 212 (specs update), 214 (proof of concept implementation) & 217 (test vectors).

How was it fixed?

The new specs & tests were implemented.

Please do not merge. This PR is for visibility for now until V06 is officially released.

To do:

  • Update to v06 when released (No changes expected)
  • Update hyperlinks to v06 spec when released

Cute Animal Picture

Baby Pangolin

py_ecc/bls/hash.py Outdated Show resolved Hide resolved
@CarlBeek CarlBeek changed the title Switches hash-to-field to new v06 hash system (Do Not Merge) Switches hash-to-field to new v06 hash system Mar 26, 2020
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

Thank you for keeping following the standardization updates, looks good 👍
Just some improvement suggestions.

py_ecc/bls/hash_to_curve.py Outdated Show resolved Hide resolved
py_ecc/bls/hash_to_curve.py Outdated Show resolved Hide resolved
py_ecc/bls/constants.py Outdated Show resolved Hide resolved
py_ecc/bls/hash.py Outdated Show resolved Hide resolved
py_ecc/bls/hash.py Outdated Show resolved Hide resolved
py_ecc/bls/hash.py Outdated Show resolved Hide resolved
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

LGTM!
Let me know if you think it's ready to merge.

tests/bls/test_hash_to_curve.py Outdated Show resolved Hide resolved
py_ecc/bls/hash.py Outdated Show resolved Hide resolved
@hwwhww
Copy link
Contributor

hwwhww commented May 5, 2020

@CarlBeek I pushed the fix into this branch, all the tests pass now.
Let me know if you think it's ready to merge. :)

@CarlBeek
Copy link
Collaborator Author

CarlBeek commented May 5, 2020

Thanks for pushing the changes. I'll check my local flake8/mypy config. Good to merge!

@hwwhww hwwhww merged commit 8981f54 into ethereum:master May 5, 2020
Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

Evidently I didn't submit this comment... Probably a nice improvement to exception message if you find yourself back in this file.

raise ValueError('DST must be <= 255 bytes')
ell = math.ceil(len_in_bytes / b_in_bytes)
if ell > 255:
raise ValueError('invalid len in bytes for hash function')
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to include both the value of ell and the limit value in this message.

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