ECRECOVER precompiled contract should have stayed unaffected by EIP155 changes #305
Conversation
pirapira
changed the title
|
Maybe this should be merged into |
Paper.tex
Outdated
| @@ -1418,6 +1418,13 @@ \section{Signing Transactions}\label{app:signing} | |||
| %\mathtt{\tiny secp256k1p} &= 2^{256} - 2^{32} - 977\\ | |||
| \end{align} | |||
|
|
|||
| Note that $\mathtt{\small ECDSARECOVER}$ function does not follow the distinction of valid or invalid transactions defined here. $\mathtt{\small ECDSARECOVER}$ returns non-empty results if the following conditions are met (this is relevant in the definition of $\Xi_{\mathtt{ECREC}}$). | |||
There was a problem hiding this comment.
Note that the ECDSARECOVER function... defined above ... definition of the precompile ...
There was a problem hiding this comment.
These are proposed edits.
Note that the
There was a problem hiding this comment.
That looks better. I'll modify my branch.
Paper.tex
Outdated
| Note that $\mathtt{\small ECDSARECOVER}$ function does not follow the distinction of valid or invalid transactions defined here. $\mathtt{\small ECDSARECOVER}$ returns non-empty results if the following conditions are met (this is relevant in the definition of $\Xi_{\mathtt{ECREC}}$). | ||
| \begin{align} | ||
| 0 < r &< \mathtt{\tiny secp256k1n} \\ | ||
| 0 < s &< \mathtt{\tiny secp256k1n} \\ |
Paper.tex
Outdated
| \begin{align} | ||
| 0 < r &< \mathtt{\tiny secp256k1n} \\ | ||
| 0 < s &< \mathtt{\tiny secp256k1n} \\ | ||
| v &\in \{27,28\} |
There was a problem hiding this comment.
At first impression this seems to conflict with EIP-155 and https://ethereum.github.io/yellowpaper/paper.pdf#v. Wait a minute, I'm still checking.
There was a problem hiding this comment.
On an unrelated note to this PR but a correction in the section that I just linked to, you may also notice "These are formally defined in the literature, e.g. by ?." #630 should fix that.
There was a problem hiding this comment.
Would be good to add to the build script to pick up Latex errors.
There was a problem hiding this comment.
I think I understand the conflict. Following are the two definitions of an invalid signature.
There was a problem hiding this comment.
Wouldn't it also be good to update the implementations and the first of the above?
There was a problem hiding this comment.
Noticed something else: #632.
There was a problem hiding this comment.
Wouldn't it be better to update the EIP draft, implementations and then this paper for the first screenshot with the ECREC precompile so that an invalid signature is defined in accordance with the second screenshot in Appendix F?
There was a problem hiding this comment.
That will be a hardfork, so, there needs to be a separate EIP proposing the fix.
pirapira
force-pushed
the
precompiled-ecrecover-unchanged
branch
from
156c96e
to
dd5edda
Compare
|
LGTM. It is unfortunate that 2 different conditions for valid ECDSA verification need to be used. |
pirapira
force-pushed
the
precompiled-ecrecover-unchanged
branch
from
dd5edda
to
601202f
Compare
|
@jamesray1 fixed my wording. |
|
@nicksavers pointed out that, this PR explains ECRECOVER precompiled as some modification of transaction signing, but it should be the other way around. The transaction signature checking (considering replay attack prevention) should be a more complicated variant of ECRECOVER operation (that's used in the ECRECOVER precompiled). I agree with this pereference. |
|
Since YP delegates the definition of ECRECOVER to a paper by Johnson, Menezes and Vanstone, I have to see first which |
|
But this document explains signature generation and verification, not the recovery of the public key. I created an issue about that #688 . For this PR I assume that the original ECDSA doesn't care about the chain IDs. |
|
Some sentences on https://github.com/ethereum/yellowpaper/pull/305/files#diff-9f702e1491c55da9d76a68d651278764R1703 talk about chain_ids, these should be moved downwards to around https://github.com/ethereum/yellowpaper/pull/305/files#diff-9f702e1491c55da9d76a68d651278764R1761 |
pirapira
changed the title
|
And also, the values except 27 and 28 should not appear https://github.com/ethereum/yellowpaper/pull/305/files#diff-9f702e1491c55da9d76a68d651278764R1711. They have to wait until we see signed transactions. |
Before this PR, the definition of ECREC precompiled contract seemed to have changed during Homestead changes. The Homestead changes involved stricter checks on the signature, and on the Yellow Paper, that change also affected the recovering precompiled contract. However, the precompiled contract did not change in the implementations.
pirapira
force-pushed
the
precompiled-ecrecover-unchanged
branch
from
601202f
to
f2fd7a6
Compare
pirapira
changed the title
|
@nicksavers this should be better now. |
|
Ah wait, I think this is still somehow wrong. So the precompiled contract should not check the value of |
pirapira
changed the title
pirapira
changed the title
pirapira
changed the title
…nchanged ECRECOVER precompiled contract should have stayed unaffected by EIP155 changes
Before this PR, the definition of ECREC precompiled contract seemed to have changed during Homestead changes. The Homestead changes involved stricter checks on the signature, and on the Yellow Paper, that change also affected the recovering precompiled contract. However, the precompiled contract did not change in the implementations.I think it's about EIP155 replay protection. EIP155 should not change the ECDSARECOVER precompiled contract.