fix: check y_parity value

[obatirou]

What was wrong?

Related to Issue #1106

How was it fixed?

Add a check to ensure y_parity is 0 or 1

[SamWilsn]

Are there cases where we'd want secp256k1_recover to accept y_parity not in (0, 1)?

[petertdavies]

No, it's a boolean. It gets encoded in strange ways in LegacyTransaction, but that is weirdness inherited from Bitcoin.

[obatirou]

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

[SamWilsn]

So should we move the check to secp256k1_recover?

[obatirou]

It is possible in this context if not mistaken but I think it would introduce a business need in secp256k1_recover which goal should only be to recover the public key from a given valid signature imo.
Shouldn't the validation of the signature values happen before ?
The checks for r and s range are already outside the function

execution-specs/src/ethereum/cancun/transactions.py

Lines 260 to 265 in 9923823

	r, s = tx.r, tx.s
	if U256(0) >= r or r >= SECP256K1N:
	raise InvalidSignatureError("bad r")
	if U256(0) >= s or s > SECP256K1N // U256(2):
	raise InvalidSignatureError("bad s")

and

execution-specs/src/ethereum/cancun/vm/precompiled_contracts/ecrecover.py

Lines 47 to 52 in 9923823

	if v != U256(27) and v != U256(28):
	return
	if U256(0) >= r or r >= SECP256K1N:
	return
	if U256(0) >= s or s >= SECP256K1N:
	return

Let me know what you think. Open to make the changes if you prefer of course.

[SamWilsn]

I can certainly see the argument for keeping secp256k1_recover pure. That said, the s validation is not uniform across forks, so it cannot be included in secp256k1_recover directly. See

execution-specs/src/ethereum/frontier/transactions.py

Lines 134 to 135 in 9923823

	if U256(0) >= s or s >= SECP256K1N:
	raise InvalidSignatureError("bad s")

vs.

execution-specs/src/ethereum/cancun/transactions.py

Lines 263 to 264 in 9923823

	if U256(0) >= s or s > SECP256K1N // U256(2):
	raise InvalidSignatureError("bad s")

I suppose I'm happy either way!

[obatirou]

Thank you for the concrete example!
I think it conforts my opinion that if secp256k1_recover cannot fully validate the signature values it should be kept pure. Else there could be confusion on where the validation of signature values is done.
So the PR should stay as is if that is ok for you. Let me know if you prefer any changes.

[petertdavies]

LGTM