fix: check y_parity value #1107
Conversation
| if tx.y_parity not in (U256(0), U256(1)): | ||
| raise InvalidSignatureError("bad y_parity") |
There was a problem hiding this comment.
Are there cases where we'd want secp256k1_recover to accept y_parity not in (0, 1)?
There was a problem hiding this comment.
No, it's a boolean. It gets encoded in strange ways in LegacyTransaction, but that is weirdness inherited from Bitcoin.
There was a problem hiding this comment.
From my understanding, no
Valid recovery id values are in (0, 1, 2, 3) (see coincurve) but ECDSA signatures are considered valid by the yellow paper only when v is in (0, 1).
This is confirmed by the fact that even for erecover, the input of secp256k1_recover will be 0 or 1 see ecrecover in exec spec and yellow paper
Example with go-ethereum ValidateSignatureValues
https://github.com/ethereum/go-ethereum/blob/e6f3ce7b168b8f346de621a8f60d2fa57c2ebfb0/crypto/crypto.go#L274
and Sign function
https://github.com/ethereum/go-ethereum/blob/e6f3ce7b168b8f346de621a8f60d2fa57c2ebfb0/crypto/secp256k1/secp256.go#L68
There was a problem hiding this comment.
So should we move the check to secp256k1_recover?
What was wrong?
Related to Issue #1106
How was it fixed?
Add a check to ensure y_parity is 0 or 1