From f5e54cd88989dd5c3944ec645960cc2861b66b72 Mon Sep 17 00:00:00 2001 From: Tobias Ruck Date: Tue, 22 Oct 2024 19:25:52 +0200 Subject: [PATCH] [secp256k1] Add `secp256k1_ec_pubkey_cmp` and `secp256k1_xonly_pubkey_cmp` Summary: Allows comparing `secp256k1_pubkey` and `secp256k1_xonly_pubkey` respectively, to establish an order between them. This is a backport of [[ https://github.com/bitcoin-core/secp256k1/pull/850 | secp256k1#850 ]] and [[ https://github.com/bitcoin-core/secp256k1/pull/926 | secp256k1#926 ]]. We need this for porting `rust-secp256k1` to the secp256k1 library of this repo. Test Plan: `ninja check-secp256k1` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D16957 --- src/secp256k1/include/secp256k1.h | 24 +++++++-- src/secp256k1/include/secp256k1_extrakeys.h | 21 ++++++-- .../src/modules/extrakeys/main_impl.h | 26 ++++++++++ .../src/modules/extrakeys/tests_impl.h | 38 ++++++++++++++ src/secp256k1/src/secp256k1.c | 26 ++++++++++ src/secp256k1/src/tests.c | 50 +++++++++++++++++++ 6 files changed, 179 insertions(+), 6 deletions(-) diff --git a/src/secp256k1/include/secp256k1.h b/src/secp256k1/include/secp256k1.h index 3fee9c68ba..41b96eca9c 100644 --- a/src/secp256k1/include/secp256k1.h +++ b/src/secp256k1/include/secp256k1.h @@ -7,7 +7,9 @@ extern "C" { #include -/* These rules specify the order of arguments in API calls: +/* Unless explicitly stated all pointer arguments must not be NULL. + * + * The following rules specify the order of arguments in API calls: * * 1. Context pointers go first, followed by output arguments, combined * output/input arguments, and finally input-only arguments. @@ -61,8 +63,9 @@ typedef struct secp256k1_scratch_space_struct secp256k1_scratch_space; * The exact representation of data inside is implementation defined and not * guaranteed to be portable between different platforms or versions. It is * however guaranteed to be 64 bytes in size, and can be safely copied/moved. - * If you need to convert to a format suitable for storage, transmission, or - * comparison, use secp256k1_ec_pubkey_serialize and secp256k1_ec_pubkey_parse. + * If you need to convert to a format suitable for storage or transmission, + * use secp256k1_ec_pubkey_serialize and secp256k1_ec_pubkey_parse. To + * compare keys, use secp256k1_ec_pubkey_cmp. */ typedef struct { unsigned char data[64]; @@ -370,6 +373,21 @@ SECP256K1_API int secp256k1_ec_pubkey_serialize( unsigned int flags ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4); +/** Compare two public keys using lexicographic (of compressed serialization) order + * + * Returns: <0 if the first public key is less than the second + * >0 if the first public key is greater than the second + * 0 if the two public keys are equal + * Args: ctx: a secp256k1 context object. + * In: pubkey1: first public key to compare + * pubkey2: second public key to compare + */ +SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_cmp( + const secp256k1_context* ctx, + const secp256k1_pubkey* pubkey1, + const secp256k1_pubkey* pubkey2 +) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3); + /** Parse an ECDSA signature in compact (64 bytes) format. * * Returns: 1 when the signature could be parsed, 0 otherwise. diff --git a/src/secp256k1/include/secp256k1_extrakeys.h b/src/secp256k1/include/secp256k1_extrakeys.h index 6fc7b290f8..0a37fb6b9d 100644 --- a/src/secp256k1/include/secp256k1_extrakeys.h +++ b/src/secp256k1/include/secp256k1_extrakeys.h @@ -15,9 +15,9 @@ extern "C" { * The exact representation of data inside is implementation defined and not * guaranteed to be portable between different platforms or versions. It is * however guaranteed to be 64 bytes in size, and can be safely copied/moved. - * If you need to convert to a format suitable for storage, transmission, or - * comparison, use secp256k1_xonly_pubkey_serialize and - * secp256k1_xonly_pubkey_parse. + * If you need to convert to a format suitable for storage, transmission, use + * use secp256k1_xonly_pubkey_serialize and secp256k1_xonly_pubkey_parse. To + * compare keys, use secp256k1_xonly_pubkey_cmp. */ typedef struct { unsigned char data[64]; @@ -67,6 +67,21 @@ SECP256K1_API int secp256k1_xonly_pubkey_serialize( const secp256k1_xonly_pubkey* pubkey ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3); +/** Compare two x-only public keys using lexicographic order + * + * Returns: <0 if the first public key is less than the second + * >0 if the first public key is greater than the second + * 0 if the two public keys are equal + * Args: ctx: a secp256k1 context object. + * In: pubkey1: first public key to compare + * pubkey2: second public key to compare + */ +SECP256K1_API int secp256k1_xonly_pubkey_cmp( + const secp256k1_context* ctx, + const secp256k1_xonly_pubkey* pk1, + const secp256k1_xonly_pubkey* pk2 +) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3); + /** Converts a secp256k1_pubkey into a secp256k1_xonly_pubkey. * * Returns: 1 if the public key was successfully converted diff --git a/src/secp256k1/src/modules/extrakeys/main_impl.h b/src/secp256k1/src/modules/extrakeys/main_impl.h index 7390b22718..c052e29a85 100644 --- a/src/secp256k1/src/modules/extrakeys/main_impl.h +++ b/src/secp256k1/src/modules/extrakeys/main_impl.h @@ -55,6 +55,32 @@ int secp256k1_xonly_pubkey_serialize(const secp256k1_context* ctx, unsigned char return 1; } +int secp256k1_xonly_pubkey_cmp(const secp256k1_context* ctx, const secp256k1_xonly_pubkey* pk0, const secp256k1_xonly_pubkey* pk1) { + unsigned char out[2][32]; + const secp256k1_xonly_pubkey* pk[2]; + int i; + + VERIFY_CHECK(ctx != NULL); + pk[0] = pk0; pk[1] = pk1; + for (i = 0; i < 2; i++) { + /* If the public key is NULL or invalid, xonly_pubkey_serialize will + * call the illegal_callback and return 0. In that case we will + * serialize the key as all zeros which is less than any valid public + * key. This results in consistent comparisons even if NULL or invalid + * pubkeys are involved and prevents edge cases such as sorting + * algorithms that use this function and do not terminate as a + * result. */ + if (!secp256k1_xonly_pubkey_serialize(ctx, out[i], pk[i])) { + /* Note that xonly_pubkey_serialize should already set the output to + * zero in that case, but it's not guaranteed by the API, we can't + * test it and writing a VERIFY_CHECK is more complex than + * explicitly memsetting (again). */ + memset(out[i], 0, sizeof(out[i])); + } + } + return secp256k1_memcmp_var(out[0], out[1], sizeof(out[1])); +} + /** Keeps a group element as is if it has an even Y and otherwise negates it. * y_parity is set to 0 in the former case and to 1 in the latter case. * Requires that the coordinates of r are normalized. */ diff --git a/src/secp256k1/src/modules/extrakeys/tests_impl.h b/src/secp256k1/src/modules/extrakeys/tests_impl.h index 9473a7dd48..25e42f68e1 100644 --- a/src/secp256k1/src/modules/extrakeys/tests_impl.h +++ b/src/secp256k1/src/modules/extrakeys/tests_impl.h @@ -137,6 +137,43 @@ void test_xonly_pubkey(void) { secp256k1_context_destroy(verify); } +void test_xonly_pubkey_comparison(void) { + unsigned char pk1_ser[32] = { + 0x58, 0x84, 0xb3, 0xa2, 0x4b, 0x97, 0x37, 0x88, 0x92, 0x38, 0xa6, 0x26, 0x62, 0x52, 0x35, 0x11, + 0xd0, 0x9a, 0xa1, 0x1b, 0x80, 0x0b, 0x5e, 0x93, 0x80, 0x26, 0x11, 0xef, 0x67, 0x4b, 0xd9, 0x23 + }; + const unsigned char pk2_ser[32] = { + 0xde, 0x36, 0x0e, 0x87, 0x59, 0x8f, 0x3c, 0x01, 0x36, 0x2a, 0x2a, 0xb8, 0xc6, 0xf4, 0x5e, 0x4d, + 0xb2, 0xc2, 0xd5, 0x03, 0xa7, 0xf9, 0xf1, 0x4f, 0xa8, 0xfa, 0x95, 0xa8, 0xe9, 0x69, 0x76, 0x1c + }; + secp256k1_xonly_pubkey pk1; + secp256k1_xonly_pubkey pk2; + int ecount = 0; + secp256k1_context *none = api_test_context(SECP256K1_CONTEXT_NONE, &ecount); + + CHECK(secp256k1_xonly_pubkey_parse(none, &pk1, pk1_ser) == 1); + CHECK(secp256k1_xonly_pubkey_parse(none, &pk2, pk2_ser) == 1); + + CHECK(secp256k1_xonly_pubkey_cmp(none, NULL, &pk2) < 0); + CHECK(ecount == 1); + CHECK(secp256k1_xonly_pubkey_cmp(none, &pk1, NULL) > 0); + CHECK(ecount == 2); + CHECK(secp256k1_xonly_pubkey_cmp(none, &pk1, &pk2) < 0); + CHECK(secp256k1_xonly_pubkey_cmp(none, &pk2, &pk1) > 0); + CHECK(secp256k1_xonly_pubkey_cmp(none, &pk1, &pk1) == 0); + CHECK(secp256k1_xonly_pubkey_cmp(none, &pk2, &pk2) == 0); + CHECK(ecount == 2); + memset(&pk1, 0, sizeof(pk1)); /* illegal pubkey */ + CHECK(secp256k1_xonly_pubkey_cmp(none, &pk1, &pk2) < 0); + CHECK(ecount == 3); + CHECK(secp256k1_xonly_pubkey_cmp(none, &pk1, &pk1) == 0); + CHECK(ecount == 5); + CHECK(secp256k1_xonly_pubkey_cmp(none, &pk2, &pk1) > 0); + CHECK(ecount == 6); + + secp256k1_context_destroy(none); +} + void test_xonly_pubkey_tweak(void) { unsigned char zeros64[64] = { 0 }; unsigned char overflows[32]; @@ -540,6 +577,7 @@ void run_extrakeys_tests(void) { test_xonly_pubkey_tweak(); test_xonly_pubkey_tweak_check(); test_xonly_pubkey_tweak_recursive(); + test_xonly_pubkey_comparison(); /* keypair tests */ test_keypair(); diff --git a/src/secp256k1/src/secp256k1.c b/src/secp256k1/src/secp256k1.c index 8a9ec49ecf..3d6d8b49df 100644 --- a/src/secp256k1/src/secp256k1.c +++ b/src/secp256k1/src/secp256k1.c @@ -316,6 +316,32 @@ int secp256k1_ec_pubkey_serialize(const secp256k1_context* ctx, unsigned char *o return ret; } +int secp256k1_ec_pubkey_cmp(const secp256k1_context* ctx, const secp256k1_pubkey* pubkey0, const secp256k1_pubkey* pubkey1) { + unsigned char out[2][33]; + const secp256k1_pubkey* pk[2]; + int i; + + VERIFY_CHECK(ctx != NULL); + pk[0] = pubkey0; pk[1] = pubkey1; + for (i = 0; i < 2; i++) { + size_t out_size = sizeof(out[i]); + /* If the public key is NULL or invalid, ec_pubkey_serialize will call + * the illegal_callback and return 0. In that case we will serialize the + * key as all zeros which is less than any valid public key. This + * results in consistent comparisons even if NULL or invalid pubkeys are + * involved and prevents edge cases such as sorting algorithms that use + * this function and do not terminate as a result. */ + if (!secp256k1_ec_pubkey_serialize(ctx, out[i], &out_size, pk[i], SECP256K1_EC_COMPRESSED)) { + /* Note that ec_pubkey_serialize should already set the output to + * zero in that case, but it's not guaranteed by the API, we can't + * test it and writing a VERIFY_CHECK is more complex than + * explicitly memsetting (again). */ + memset(out[i], 0, sizeof(out[i])); + } + } + return secp256k1_memcmp_var(out[0], out[1], sizeof(out[0])); +} + static void secp256k1_ecdsa_signature_load(const secp256k1_context* ctx, secp256k1_scalar* r, secp256k1_scalar* s, const secp256k1_ecdsa_signature* sig) { (void)ctx; if (sizeof(secp256k1_scalar) == 32) { diff --git a/src/secp256k1/src/tests.c b/src/secp256k1/src/tests.c index fc8db5e73e..a6b7db749f 100644 --- a/src/secp256k1/src/tests.c +++ b/src/secp256k1/src/tests.c @@ -4829,6 +4829,55 @@ void test_random_pubkeys(void) { } } +void run_pubkey_comparison(void) { + unsigned char pk1_ser[33] = { + 0x02, + 0x58, 0x84, 0xb3, 0xa2, 0x4b, 0x97, 0x37, 0x88, 0x92, 0x38, 0xa6, 0x26, 0x62, 0x52, 0x35, 0x11, + 0xd0, 0x9a, 0xa1, 0x1b, 0x80, 0x0b, 0x5e, 0x93, 0x80, 0x26, 0x11, 0xef, 0x67, 0x4b, 0xd9, 0x23 + }; + const unsigned char pk2_ser[33] = { + 0x02, + 0xde, 0x36, 0x0e, 0x87, 0x59, 0x8f, 0x3c, 0x01, 0x36, 0x2a, 0x2a, 0xb8, 0xc6, 0xf4, 0x5e, 0x4d, + 0xb2, 0xc2, 0xd5, 0x03, 0xa7, 0xf9, 0xf1, 0x4f, 0xa8, 0xfa, 0x95, 0xa8, 0xe9, 0x69, 0x76, 0x1c + }; + secp256k1_pubkey pk1; + secp256k1_pubkey pk2; + int32_t ecount = 0; + + CHECK(secp256k1_ec_pubkey_parse(ctx, &pk1, pk1_ser, sizeof(pk1_ser)) == 1); + CHECK(secp256k1_ec_pubkey_parse(ctx, &pk2, pk2_ser, sizeof(pk2_ser)) == 1); + + secp256k1_context_set_illegal_callback(ctx, counting_illegal_callback_fn, &ecount); + CHECK(secp256k1_ec_pubkey_cmp(ctx, NULL, &pk2) < 0); + CHECK(ecount == 1); + CHECK(secp256k1_ec_pubkey_cmp(ctx, &pk1, NULL) > 0); + CHECK(ecount == 2); + CHECK(secp256k1_ec_pubkey_cmp(ctx, &pk1, &pk2) < 0); + CHECK(secp256k1_ec_pubkey_cmp(ctx, &pk2, &pk1) > 0); + CHECK(secp256k1_ec_pubkey_cmp(ctx, &pk1, &pk1) == 0); + CHECK(secp256k1_ec_pubkey_cmp(ctx, &pk2, &pk2) == 0); + CHECK(ecount == 2); + { + secp256k1_pubkey pk_tmp; + memset(&pk_tmp, 0, sizeof(pk_tmp)); /* illegal pubkey */ + CHECK(secp256k1_ec_pubkey_cmp(ctx, &pk_tmp, &pk2) < 0); + CHECK(ecount == 3); + CHECK(secp256k1_ec_pubkey_cmp(ctx, &pk_tmp, &pk_tmp) == 0); + CHECK(ecount == 5); + CHECK(secp256k1_ec_pubkey_cmp(ctx, &pk2, &pk_tmp) > 0); + CHECK(ecount == 6); + } + + secp256k1_context_set_illegal_callback(ctx, NULL, NULL); + + /* Make pk2 the same as pk1 but with 3 rather than 2. Note that in + * an uncompressed encoding, these would have the opposite ordering */ + pk1_ser[0] = 3; + CHECK(secp256k1_ec_pubkey_parse(ctx, &pk2, pk1_ser, sizeof(pk1_ser)) == 1); + CHECK(secp256k1_ec_pubkey_cmp(ctx, &pk1, &pk2) < 0); + CHECK(secp256k1_ec_pubkey_cmp(ctx, &pk2, &pk1) > 0); +} + void run_random_pubkeys(void) { int i; for (i = 0; i < 10*count; i++) { @@ -5778,6 +5827,7 @@ int main(int argc, char **argv) { #endif /* ecdsa tests */ + run_pubkey_comparison(); run_random_pubkeys(); run_ecdsa_der_parse(); run_ecdsa_sign_verify();