Add secp256k1_pubkey_sort

[jonasnick]

This PR adds a secp256k1_pubkey_sort function the the public API which was originally part of the musig PR (#1479). However, I opened a separate PR because it adds internal functions that are also used by the WIP silent payments module.

[real-or-random]

Concept ACK. This is useful independently of MuSig and Silent Payments. Also for users with no standard library, e.g., on hardware wallets.

cc @roconnor-blockstream who wrote most of this code

---

Just out of curiosity, I checked what glibc actually does. They have Quicksort, Insertionsort, Mergesort, and Heapsort in their code base. If malloc fails, they'll always fall back to Heapsort. I think they had been using Quicksort and Mergesort for decades (perhaps selected depending on the platform?), but they changed their mind quite often lately, and this was indeed related to degenerate and adversarially constructed inputs:

• Oct 2023: They added Heapsort and used it to implement Introsort. Introsort is basically Quicksort two fallbacks: 1) a fallback on some O(n log n) sorting algorithm in degenerate cases, and 2) a fallback on Insertion sort for small arrays (bminor/glibc@274a46c). They had also removed Mergesort, making Introsort the algorithm used everywhere. (bminor/glibc@03bf835)
• Nov 2023: The logic for the fallback to Heapsort was tightened, and a test with a degerate array. (bminor/glibc@64e4acf: "To avoid a denial-of-service condition with adversarial input, it is necessary to fall over to heapsort if tail-recursing deeply, too [...]")
• Jan 2024: They reverted to Mergesort because it turns out that people rely on the sorting being stable, even though this is not guaranteed by the standard, and even though their Heapsort fallback won't guarantee it anyway. (bminor/glibc@709fbd3)

This means that the current libc is probably fine. But old versions may be problematic, and other implementations of the standard library may also be problematic. If anything, all of this confirms that Heapsort is a good choice, and that we should not rely on the standard library here.

[josibake]

Concept ACK. This is particularly useful for the silent payments module in that we are able to hide a lot of complexity from the caller. In addition, I agree with Tim's assessment that this is a generally useful thing to have (not specific to Musig2/Silent payments).

Left a small nit regarding documentation but overall I found this pretty easy to work with as implemented when creating a custom cmp for silent payments.

[theStack]

Concept ACK

[elichai]

A wild idea:
What if we used something simple(like shellsort or even bubblesort) that is very easy to verify, and has good performance in the best case (when it's already sorted) and then we recommend users that use huge list of pub keys to sort them before passing them in with the stdlib sort function of the platform their using.

Needing to reimplement and maintaining highly efficient in place sorting every time is very sad

About it being useful, I'll say that embedded rust still gives you sorting via libcore, and I think every embedded C toolkit has some sort of sorting utility, but I might be wrong on this

[real-or-random]

A wild idea: What if we used something simple(like shellsort or even bubblesort) that is very easy to verify, and has good performance in the best case (when it's already sorted) and then we recommend users that use huge list of pub keys to sort them before passing them in with the stdlib sort function of the platform their using.

Hm, I think this gives us the worst of both worlds in the end:

• We have to maintain the implementation of sorting algorithm.
• We still tell users to rely on the stdlib.

Also, I doubt that Shellsort or Bubblesort are much easier to implement than Heapsort. (And look at the nature of the bug I found: The swap was wrong, not the Heapsort itself. And you'll need a swap for every algorithm.)

Needing to reimplement and maintaining highly efficient in place sorting every time is very sad

Yeah, but that's because C is sad (sometimes).

[josibake]

I'll echo @real-or-random 's point and add:

and then we recommend users that use huge list of pub keys to sort them before passing them in with the stdlib sort function of the platform their using.

I think this is fine if the sorting step is a convenience, but in the case of the silent payments module sorting is a required step for generating the outputs correctly. For this, I think it's far better to have sorting handled inside the module vs requiring the user to sort before passing the public keys. Doing the sorting for the users eliminates a difficult to catch error and we'd need to sort anyways to be able to detect and alert the user that they did not sort (or sorted incorrectly).

[real-or-random]

ACK mod nits, I've inspected the code and the proofs again.

New improved tests are nice.

[real-or-random]

Not exactly this PR, but do you think it's worth noting in the API docs that callbacks may be triggered more than once per API call? This may be interesting in the case of the illegal callback because you wouldn't need to crash necessarily for this type of callback (e.g., thinking of python bindings that could choose to raise). If yes, we could track this separately.

[jonasnick]

Hm good point. But if we think that calling the callback multiple times causes problems, it may makes more sense to fix/change that because it's unlikely someone will read this and take it into account.

For the error callback the documentation says "After this callback returns, anything may happen, including crashing." and for the illegal callback "When this callback is triggered, the API function called is guaranteed not to cause a crash, though its return value and output arguments are undefined."

[real-or-random]

But if we think that calling the callback multiple times causes problems, it may makes more sense to fix/change that because it's unlikely someone will read this and take it into account.

Let's maybe track this in a separate issue. It's certainly not great to call the callback twice, but last time when I tried to "fix" it, I wasn't happy with the added code complexity.

[jonasnick]

I added a description of the interface to the hsort doc. Apparently mac OS uses a different order of arguments.

[roconnor-blockstream]

Not entirely sure what we are trying to do with this check, but if we are thinking of hsort as a black box sorting function, the minimum number of comparisons is NUM-1 (min 0).

If we are thinking of hsort as a white box heap sort, then the minimum number of comparisons is larger, on the order of (3/2 n) or so. We can probably figure it out if that's your intention.

[jonasnick]

Changed to NUM-1.

[roconnor-blockstream]

Doing the sorting for the users eliminates a difficult to catch error and we'd need to sort anyways to be able to detect and alert the user that they did not sort (or sorted incorrectly).

You can test that a list is sorted in O(n) time, but sorting (by comparisons) take O(n log n) time

That said, I do agree that we should just sort ourselves, in O(n log n) worst case time, with constant memory overhead.

[jonasnick]

Rebased on master and added change log entry.

[josibake]

ACK 7d2591c

Don't have much to offer on the hsort side, but did review the secp256k1 specific parts and have been using this commit in #1519 without any issues.

[josibake]

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.

[jonasnick]

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 **).

[real-or-random]

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.

[roconnor-blockstream]

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

[sipa]

ACK 7d2591c

[real-or-random]

ACK 7d2591c

[real-or-random]

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