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_pubkey_sort #1518

Merged
merged 1 commit into from
May 6, 2024
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

#### Added
- New function `secp256k1_ec_pubkey_sort` that sorts public keys using lexicographic (of compressed serialization) order.

#### Changed
- The implementation of the point multiplication algorithm used for signing and public key generation was changed, resulting in improved performance for those operations.
- The related configure option `--ecmult-gen-precision` was replaced with `--ecmult-gen-kb` (`ECMULT_GEN_KB` for CMake).
Expand Down
2 changes: 2 additions & 0 deletions Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ noinst_HEADERS += src/field.h
noinst_HEADERS += src/field_impl.h
noinst_HEADERS += src/bench.h
noinst_HEADERS += src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.h
noinst_HEADERS += src/hsort.h
noinst_HEADERS += src/hsort_impl.h
noinst_HEADERS += contrib/lax_der_parsing.h
noinst_HEADERS += contrib/lax_der_parsing.c
noinst_HEADERS += contrib/lax_der_privatekey_parsing.h
Expand Down
14 changes: 14 additions & 0 deletions include/secp256k1.h
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,20 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_cmp(
const secp256k1_pubkey *pubkey2
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);

/** Sort public keys using lexicographic (of compressed serialization) order
*
* Returns: 0 if the arguments are invalid. 1 otherwise.
*
* Args: ctx: pointer to a context object
* In: pubkeys: array of pointers to pubkeys to sort
* n_pubkeys: number of elements in the pubkeys array
*/
SECP256K1_API int secp256k1_ec_pubkey_sort(
const secp256k1_context *ctx,
const secp256k1_pubkey **pubkeys,
size_t n_pubkeys
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2);

/** Parse an ECDSA signature in compact (64 bytes) format.
*
* Returns: 1 when the signature could be parsed, 0 otherwise.
Expand Down
33 changes: 33 additions & 0 deletions src/hsort.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/***********************************************************************
* Copyright (c) 2021 Russell O'Connor, Jonas Nick *
* Distributed under the MIT software license, see the accompanying *
* file COPYING or https://www.opensource.org/licenses/mit-license.php.*
***********************************************************************/

#ifndef SECP256K1_HSORT_H
#define SECP256K1_HSORT_H

#include <stddef.h>
#include <string.h>

/* In-place, iterative heapsort with an interface matching glibc's qsort_r. This
* is preferred over standard library implementations because they generally
* make no guarantee about being fast for malicious inputs.
* Remember that heapsort is unstable.
*
* In/Out: ptr: pointer to the array to sort. The contents of the array are
* sorted in ascending order according to the comparison function.
* In: count: number of elements in the array.
* size: size in bytes of each element.
* cmp: pointer to a comparison function that is called with two
* arguments that point to the objects being compared. The cmp_data
* argument of secp256k1_hsort is passed as third argument. The
* function must return an integer less than, equal to, or greater
* than zero if the first argument is considered to be respectively
* less than, equal to, or greater than the second.
* cmp_data: pointer passed as third argument to cmp.
*/
static void secp256k1_hsort(void *ptr, size_t count, size_t size,
int (*cmp)(const void *, const void *, void *),
void *cmp_data);
#endif
125 changes: 125 additions & 0 deletions src/hsort_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
/***********************************************************************
* Copyright (c) 2021 Russell O'Connor, Jonas Nick *
* Distributed under the MIT software license, see the accompanying *
* file COPYING or https://www.opensource.org/licenses/mit-license.php.*
***********************************************************************/

#ifndef SECP256K1_HSORT_IMPL_H
#define SECP256K1_HSORT_IMPL_H

#include "hsort.h"

/* An array is a heap when, for all non-zero indexes i, the element at index i
* compares as less than or equal to the element at index parent(i) = (i-1)/2.
*/

static SECP256K1_INLINE size_t secp256k1_heap_child1(size_t i) {
VERIFY_CHECK(i <= (SIZE_MAX - 1)/2);
return 2*i + 1;
}

static SECP256K1_INLINE size_t secp256k1_heap_child2(size_t i) {
VERIFY_CHECK(i <= SIZE_MAX/2 - 1);
return secp256k1_heap_child1(i)+1;
}

static SECP256K1_INLINE void secp256k1_heap_swap64(unsigned char *a, unsigned char *b, size_t len) {
unsigned char tmp[64];
VERIFY_CHECK(len <= 64);
memcpy(tmp, a, len);
memmove(a, b, len);
memcpy(b, tmp, len);
}

static SECP256K1_INLINE void secp256k1_heap_swap(unsigned char *arr, size_t i, size_t j, size_t stride) {
unsigned char *a = arr + i*stride;
unsigned char *b = arr + j*stride;
size_t len = stride;
while (64 < len) {
secp256k1_heap_swap64(a + (len - 64), b + (len - 64), 64);
len -= 64;
}
secp256k1_heap_swap64(a, b, len);
}

/* This function accepts an array arr containing heap_size elements, each of
* size stride. The elements in the array at indices >i satisfy the max-heap
* property, i.e., for any element at index j (where j > i), all of its children
* are smaller than the element itself. The purpose of the function is to update
* the array so that all elements at indices >=i satisfy the max-heap
* property. */
static SECP256K1_INLINE void secp256k1_heap_down(unsigned char *arr, size_t i, size_t heap_size, size_t stride,
sipa marked this conversation as resolved.
Show resolved Hide resolved
int (*cmp)(const void *, const void *, void *), void *cmp_data) {
while (i < heap_size/2) {
VERIFY_CHECK(i <= SIZE_MAX/2 - 1);
/* Proof:
* i < heap_size/2
* i + 1 <= heap_size/2
* 2*i + 2 <= heap_size <= SIZE_MAX
* 2*i <= SIZE_MAX - 2
*/

VERIFY_CHECK(secp256k1_heap_child1(i) < heap_size);
/* Proof:
* i < heap_size/2
* i + 1 <= heap_size/2
* 2*i + 2 <= heap_size
* 2*i + 1 < heap_size
* child1(i) < heap_size
*/

/* Let [x] be notation for the contents at arr[x*stride].
*
* If [child1(i)] > [i] and [child2(i)] > [i],
* swap [i] with the larger child to ensure the new parent is larger
* than both children. When [child1(i)] == [child2(i)], swap [i] with
* [child2(i)].
* Else if [child1(i)] > [i], swap [i] with [child1(i)].
* Else if [child2(i)] > [i], swap [i] with [child2(i)].
*/
if (secp256k1_heap_child2(i) < heap_size
&& 0 <= cmp(arr + secp256k1_heap_child2(i)*stride, arr + secp256k1_heap_child1(i)*stride, cmp_data)) {
if (0 < cmp(arr + secp256k1_heap_child2(i)*stride, arr + i*stride, cmp_data)) {
secp256k1_heap_swap(arr, i, secp256k1_heap_child2(i), stride);
i = secp256k1_heap_child2(i);
} else {
/* At this point we have [child2(i)] >= [child1(i)] and we have
* [child2(i)] <= [i], and thus [child1(i)] <= [i] which means
* that the next comparison can be skipped. */
return;
}
} else if (0 < cmp(arr + secp256k1_heap_child1(i)*stride, arr + i*stride, cmp_data)) {
secp256k1_heap_swap(arr, i, secp256k1_heap_child1(i), stride);
i = secp256k1_heap_child1(i);
} else {
return;
}
}
/* heap_size/2 <= i
* heap_size/2 < i + 1
* heap_size < 2*i + 2
* heap_size <= 2*i + 1
* heap_size <= child1(i)
* Thus child1(i) and child2(i) are now out of bounds and we are at a leaf.
*/
}

/* In-place heap sort. */
static void secp256k1_hsort(void *ptr, size_t count, size_t size,
int (*cmp)(const void *, const void *, void *),
void *cmp_data) {
size_t i;

for (i = count/2; 0 < i; --i) {
secp256k1_heap_down(ptr, i-1, count, size, cmp, cmp_data);
}
for (i = count; 1 < i; --i) {
/* Extract the largest value from the heap */
secp256k1_heap_swap(ptr, 0, i-1, size);

/* Repair the heap condition */
secp256k1_heap_down(ptr, 0, i-1, size, cmp, cmp_data);
}
}

#endif
29 changes: 29 additions & 0 deletions src/secp256k1.c
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include "int128_impl.h"
#include "scratch_impl.h"
#include "selftest.h"
#include "hsort_impl.h"

#ifdef SECP256K1_NO_BUILD
# error "secp256k1.h processed without SECP256K1_BUILD defined while building secp256k1.c"
Expand Down Expand Up @@ -325,6 +326,34 @@ int secp256k1_ec_pubkey_cmp(const secp256k1_context* ctx, const secp256k1_pubkey
return secp256k1_memcmp_var(out[0], out[1], sizeof(out[0]));
}

static int secp256k1_ec_pubkey_sort_cmp(const void* pk1, const void* pk2, void *ctx) {
return secp256k1_ec_pubkey_cmp((secp256k1_context *)ctx,
*(secp256k1_pubkey **)pk1,
*(secp256k1_pubkey **)pk2);
Comment on lines +329 to +332
Copy link
Member

Choose a reason for hiding this comment

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

In #1519, I initially created a secp256k1_silentpayments_recipient_sort_cmp function using (secp256k1_silentpayments_recipient *)r->scan_pubkey instead of *(secp256k1_silentpayments_recipient **)r->scan_pubkey. Everything compiled fine, but the end result was my array was not sorted correctly. Took me awhile to figure out that it was due to using (typedef *) instead of *(typedef **).

This could just be a me problem in not having a strong C background, but wanted to mention it in case its worth documenting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, this is documented. The hsort doc says "comparison function is called with two arguments that point to the objects being compared" (which was copied from the qsort_r manpage). If the array you give to hsort consists of elements of type secp256k1_silentpayments_recipient *, the comparison function is called with pointers to the elements (secp256k1_silentpayments_recipient **).

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess seeing stuff like *(secp256k1_pubkey **)pk1 can be confusing even to seasoned C programmers at first glance, due to the asterisk being used for two related but different things here (first as a dereference operator and then in the cast for indicating a pointer type). But yeah, I don't think we can do anything to improve this.

Copy link
Contributor

Choose a reason for hiding this comment

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

should this be *(const secp256k1_pubkey * const *)pk1 or something?

}

int secp256k1_ec_pubkey_sort(const secp256k1_context* ctx, const secp256k1_pubkey **pubkeys, size_t n_pubkeys) {
VERIFY_CHECK(ctx != NULL);
ARG_CHECK(pubkeys != NULL);

/* Suppress wrong warning (fixed in MSVC 19.33) */
#if defined(_MSC_VER) && (_MSC_VER < 1933)
#pragma warning(push)
#pragma warning(disable: 4090)
#endif

/* Casting away const is fine because neither secp256k1_hsort nor
* secp256k1_ec_pubkey_sort_cmp modify the data pointed to by the cmp_data
* argument. */
secp256k1_hsort(pubkeys, n_pubkeys, sizeof(*pubkeys), secp256k1_ec_pubkey_sort_cmp, (void *)ctx);

#if defined(_MSC_VER) && (_MSC_VER < 1933)
#pragma warning(pop)
#endif
Comment on lines +339 to +352
Copy link
Contributor

Choose a reason for hiding this comment

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

I verified that the pragmas are still necessary now that the call to secp256k1_hsort was changed. See https://godbolt.org/z/13MY3acP1 .


return 1;
}

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
Loading