-
Notifications
You must be signed in to change notification settings - Fork 64
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
Ecrecover point at infinity #76
Conversation
5bb6cdf
to
56dbd42
Compare
👋 @ChihChengLiang! I tagged you because Piper said you know the math behind this really well. If you could please take a look to make sure these changes make sense, that would be great! |
56dbd42
to
86e07d0
Compare
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, I definitely don't understand what's going on here well enough to give a 👍🏻 . The thing that stood out the most to me was that this only seems to fix the problem if we are using the native backend, but we should definitely make sure this fixes the issue for all backends.
# This test does introduce some flakiness on the small chance | ||
# that hypothesis generates another message hash that corresponds to the | ||
# Jacobian point at infinity |
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.
Hm, that seems important. I wonder if we can track down another message hash that also corresponds to the Jacobian point at infinity.
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, probably a good idea to track down another one for some more testing. I'll see if I can find one!
I think two bugs are introduced with this PR, below I put the rationale. TLDR
(Note: I use the term point at infinity, quite loosely below and it should instead be distinguished point since before this PR, those points were not the points at infinity) Assumptions
Evidence for second assumption See:
If you check the jacobian_add method (2nd link), where we are adding two points
So both if statements check the second component of the jacobian point, and if it is zero, it says "this is the point at infinity/identity point" judging by what it does on those conditions. Pre PR Before the PR was merged, since the points at infinity were represented using either (0,0,0) or (0,0,1), the check above would work. Note this check implicitly assumes that any point whose second component is 0, is the identity point, I haven't checked any further into assumption, I just assumed it held true. Bug If you agree with the above rationale, then I think the first bug was introduced because you now represent the point at infinity using the constant (1,1,0) . See here: https://github.com/kclowes/eth-keys/blob/ecrecover-point-at-infinity/eth_keys/constants.py#L20 But the logic to check for the point at infinity is still to check for the second component, which is not zero using that constant. The second bug is because using the representation that you chose, the points at infinity is now a set of points (t^2, t^3, 0) where t !=0. This is in the stack overflow post accompanied with this PR above. This means that the Testing for bug
Something like the below code result = POINT_AT_INFINITY
for i in range(0,100):
result = jacobian_add(result, POINT_AT_INFINITY)
assert result == POINT_AT_INFINITY
If I understand your changes, then the two tests should fail. Suggestion The code was failing because the ecdsa recover was not checking for the points at infinity IIUC. If we assume that the library is correct in just checking for the second component, then you would only need to check this too in the ecdsa_recover method. You could also explicitly check for the two internal representations being used. I'd probably add a method called |
@kclowes Sorry I didn't finish my review in time. I think @kevaundray summarized the issue thoroughly. |
42e8d2e
to
e51a520
Compare
Thanks for the review @kevaundray! I think I fixed the bugs you pointed out, but would appreciate if you could take another look.
Is there a way that seems more correct? If the y-coordinate of point Q from here is 0, does that always correspond to an identity point? @carver - would appreciate another look as well when you have a few! |
e834f30
to
4c8cbf7
Compare
4c8cbf7
to
84f82d7
Compare
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.
Cool, I'm mostly piggy-backing on the expertise of other folks here when I say
6a138e1
to
4f7e094
Compare
Ecrecover point at infinity
* remove gitter, testing setup, and pandoc sections, add quotes to dev install
What was wrong?
ecdsa_recover
returned a public address even if the Jacobian transformation returned an invalid point (the point at infinity). This behavior was inconsistent with other clients. See: ethereum/tests#941Once this is merged and released, we'll need to update
eth-account
andpy-evm
.How was it fixed?
Replaced points in the Jacobian files that were meant to represent the points at infinity with the more commonly used representations. And now the
ecdsa_recovery
method checks to see ifQ
is a point at infinity and raises an error to be more consistent with Geth's ecrecover.I went back and forth deciding whether to tack on an
is_infinity
flag to the point at infinity representation, but ultimately decided to go with the simple solution for now. I'm open to adding anis_infinity
flag though if anyone else feels strongly.Cute Animal Picture