Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add secp256k1_ec_pubkey_cmp method #850

Merged
merged 2 commits into from
May 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions include/secp256k1.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,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];
Expand Down Expand Up @@ -370,6 +371,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
Comment on lines +379 to +381
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add "(cannot be NULL)" here (and nit: full stops), same below

Suggested change
* Args: ctx: a secp256k1 context object.
* In: pubkey1: first public key to compare
* pubkey2: second public key to compare
* Args: ctx: a secp256k1 context object (cannot be NULL).
* In: pubkey1: first public key to compare (cannot be NULL).
* pubkey2: second public key to compare (cannot be NULL).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is implicit as per the discussion in #783 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, yes. Strictly speaking, we first need to add the line Unless explicitly stated all pointer arguments must not be NULL. in the header file (as done in #783) because currently we promise that the illegal_callback is only called for violations explicitly mentioned in the header.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added that line in #926

*/
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.
Expand Down
21 changes: 18 additions & 3 deletions include/secp256k1_extrakeys.h
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-null comments here too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary if #926 gets merged (needs more ACKs :] )

* 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
Expand Down
26 changes: 26 additions & 0 deletions src/modules/extrakeys/main_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down
38 changes: 38 additions & 0 deletions src/modules/extrakeys/tests_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down Expand Up @@ -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();
Expand Down
26 changes: 26 additions & 0 deletions src/secp256k1.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
50 changes: 50 additions & 0 deletions src/tests.c
Original file line number Diff line number Diff line change
Expand Up @@ -4809,6 +4809,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++) {
Expand Down Expand Up @@ -5860,6 +5909,7 @@ int main(int argc, char **argv) {
#endif

/* ecdsa tests */
run_pubkey_comparison();
run_random_pubkeys();
run_ecdsa_der_parse();
run_ecdsa_sign_verify();
Expand Down