Implementation of new (draft) hash-to-G2 #898
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benjaminion
force-pushed
the
bls-standard
branch
2 times, most recently
from
1b5fdda
to
598dc8a
Compare
benjaminion
force-pushed
the
bls-standard
branch
2 times, most recently
from
a4e9cb1
to
9229da0
Compare
benjaminion
force-pushed
the
bls-standard
branch
4 times, most recently
from
1694bf9
to
ea6ed07
Compare
benjaminion
force-pushed
the
bls-standard
branch
from
abc1358
to
273265e
Compare
mkalinin
reviewed
Nov 12, 2019
benjaminion
force-pushed
the
bls-standard
branch
from
273265e
to
97c7c4a
Compare
|
Have re-organised things a little after @mkalinin's comment. There is now an |
benjaminion
force-pushed
the
bls-standard
branch
from
87bfbf5
to
c64b028
Compare
|
Nice, clean, with lots of comments. LGTM. |
mbaxter
approved these changes
Jan 6, 2020
| * purposes of hashing to G2 within Ethereum I believe that this is of no consequence, since all the | ||
| * input information is publicly known. | ||
| */ | ||
| public class hashToCurve { |
There was a problem hiding this comment.
Suggested change
| public class hashToCurve { | |
| public class HashToCurve { |
| JacobianPoint p = iso3(q0.add(q1)); | ||
|
|
||
| if (!onCurveG2(p)) { | ||
| throw new RuntimeException("hashToCurve failed for unknown reasons."); |
There was a problem hiding this comment.
Is this a sanity-check type exception - i.e. we don't expect this to ever get thrown? If so, might be worth adding a comment or updating the message to indicate there's a bug.
There was a problem hiding this comment.
Yes, making it an assert is perfect. It's an expensive check that we expect never to fail, so better to avoid it if we can. If it does fail then there's a bug.
benjaminion
force-pushed
the
bls-standard
branch
from
3fa7149
to
670b282
Compare
Spotless is terrible at handling structured commenting. For this reason we have changed from trailing comments to separate line comments, and we use C-style /* ... */ in order to keep Spotless from mangling things further.
Using field points (FP types) rather than BIGs means that Montgomery reduction is used for the modulo operation which leads to a significant speedup.
benjaminion
force-pushed
the
bls-standard
branch
from
670b282
to
b5d256d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description
This is a Java implementation of the new IETF (draft) standard hash-to-curve method for Ethereum 2.0. The underlying library is Milagro AMCL 1.4, though there is lots that I ended up adding to this.
References:
This code is based on the reference implementations in Python and C, and test vectors are taken from there. My implementation is not constant-time, but that should not be a problem for Eth2 use cases (all hashed messages are public information).
Status
A complete implementation of the new hash-to-curve standard as of 31/12/2019
Performance
(Based on an outdated version)
On my Linux VM
Here's a flame graph (it's interactive - click on the bars to zoom in). I'm struggling to see much room for further improvement.
TODO
Nothing more here. Note that a wider BLS refactor is underway: #1080