Add a description of the overall background and high-level changes that this PR introduces
Previously, some tests were failing on the dev/ethsecp branch in cosmos-sdk/crypto/keys/secp256k1/secp256k1_nocgo_test.go due to issues validating GETH signatures' ecrecover byte $v$.
To see why, let's examine the following excerpt of libsecp256k1's secp256k1_ecdsa_sig_sign function, which is used by GETH to generate uncompressed signatures of form $r || s || v$:
secp256k1_ecmult_gen(ctx, &rp, nonce);
secp256k1_ge_set_gej(&r, &rp);
secp256k1_fe_normalize(&r.x);
secp256k1_fe_normalize(&r.y);
secp256k1_fe_get_b32(b, &r.x);
secp256k1_scalar_set_b32(sigr, b, &overflow);
if (recid) {
/* The overflow condition is cryptographically unreachable as hitting it requires finding the discrete log
* of some P where P.x >= order, and only 1 in about 2^127 points meet this criteria.
*/
*recid = (overflow << 1) | secp256k1_fe_is_odd(&r.y);
}
Referring to the ECDSA signature generation process, r in the code above corresponds to $(x_1, y_1) = kG$, and recid corresponds to the ecrecover byte $v$. Note that recid is set to the parity of $y_1$. However, the cosmos isECRecoverByteValid function is checking it against the $y$-coordinate parity of the public key $Q_A = d_AG$ (where $d_A$ is the signer's private key), and these two parities are WHP going to be different since $d_A$ and $k$ are randomly generated, so $d_AG$ and $kG$ are essentially two random points in the secp256k1 curve group.
To fix this, we instead call ecrecover on the supplied hashed message and signature and check to see if the recovered pubkey matches the supplied pubkey. The supplied pubkey is valid if and only if it matches with the recovered pubkey.
Brief Changelog
Renamed isECRecoverByteValid to checkPubkeyMatchesEcrecover and changed the logic to call GETH's Ecrecover function on the supplied message hash and uncompressed r || s || v signature.
Added GETH as an import to for Ethereum signature generation
Added a test TestGenerateRandomEIP191 that generates random SHA3(message) and signature pairs and verifies that the recovered and supplied pubkeys are the same.
Testing and Verifying
Added unit tests as described above
Documentation and Release Note
Does this pull request introduce a new feature or user-facing behavior changes? no
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Closes: #XXX
What is the purpose of the change
Previously, some tests were failing on the
dev/ethsecp
branch incosmos-sdk/crypto/keys/secp256k1/secp256k1_nocgo_test.go
due to issues validating GETH signatures' ecrecover byte $v$.To see why, let's examine the following excerpt of libsecp256k1's secp256k1_ecdsa_sig_sign function, which is used by GETH to generate uncompressed signatures of form $r || s || v$:
Referring to the ECDSA signature generation process,
r
in the code above corresponds to $(x_1, y_1) = kG$, andrecid
corresponds to the ecrecover byte $v$. Note thatrecid
is set to the parity of $y_1$. However, the cosmosisECRecoverByteValid
function is checking it against the $y$-coordinate parity of the public key $Q_A = d_AG$ (where $d_A$ is the signer's private key), and these two parities are WHP going to be different since $d_A$ and $k$ are randomly generated, so $d_AG$ and $kG$ are essentially two random points in the secp256k1 curve group.To fix this, we instead call ecrecover on the supplied hashed message and signature and check to see if the recovered pubkey matches the supplied pubkey. The supplied pubkey is valid if and only if it matches with the recovered pubkey.
Brief Changelog
isECRecoverByteValid
tocheckPubkeyMatchesEcrecover
and changed the logic to call GETH's Ecrecover function on the supplied message hash and uncompressed r || s || v signature.TestGenerateRandomEIP191
that generates random SHA3(message) and signature pairs and verifies that the recovered and supplied pubkeys are the same.Testing and Verifying
Added unit tests as described above
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? no