impl PartialOrd/Ord for PublicKey #524

sanket1729 · 2020-11-23T21:30:22Z

No description provided.

dpc · 2020-11-23T21:32:07Z

src/util/key.rs

 pub struct PublicKey {
    /// Whether this public key should be serialized as compressed
    pub compressed: bool,
    /// The actual ECDSA key
    pub key: secp256k1::PublicKey,
 }

+impl Ord for PublicKey{


cargo fmt 👍

dpc · 2020-11-23T21:36:22Z

src/util/key.rs

+        match (self.compressed, other.compressed){
+            (true, false) => cmp::Ordering::Less,
+            (false, true) => cmp::Ordering::Greater,
+            (true, true) => self.key.serialize().cmp(&other.key.serialize()),


serialize is expensive. If someone tries to run sorting on a vector of PublicKey s that might be a problem, as these will be run over and over.

That might be a fundamental problem and why cmp should not be directly implemented on PublicKey

Good point. Maybe, what we need a method for sorting instead of cmp on PublicKey

I think we can also add that as an efficient method for sorting. I am not sure whether we should impl Ord on PublicKey or not. But I do think we should remove the default derive impl because that is incorrect.

Seems like https://docs.rs/secp256k1/0.19.0/secp256k1/key/struct.PublicKey.html has Ord and under the hood it's all just a pub struct PublicKey([c_uchar; 64]); so you should be able to use raw ordering of the bytes, instead of trying to serialize anything first.

But, bip67(sortedmulti) requires us to convert that 33-byte representation for lexical comparison which requires seralize. My guess is that 64 bytes in ffi::PublicKey have the full x and y co-ordinate.

@dpc, this is not calling serialize on bitcoin::PublicKey, it is calling it on underlying secp key.

@apoelstra, the serialize function does do the conversion from 64-byte secp key to 33 byte one. I think that involves minimal crypto operation of checking the sign of y-coordinate. In theory, we do that log(n) extra times whenever we call sort operation using the default PartialOrd.

On the other hand, having a custom sort function should allow do the seralize only once(instead of log(n)) in sorting. I do get that there is some performance benefit to this, but don't have a strong intuition to quantify it.

It does two field normalizations, which are 15nsec each on my system, and then checks the least significant bit of y.

Oh, OK. No allocation is better, thought it is still not ideal. Judgment call, but seems reasonable.

Just make sure there potentially won't be any trouble if PublicKey has to change internally or anything like that. The fact that this order is a bit arbitrary and doesn't feel "obvious and natural" still worries me a little.

It's unfortunately "obvious and natural" in Bitcoin Core, and it is Core's ordering that we are trying to match.

apoelstra

utACK

stevenroose · 2020-11-26T16:15:23Z

Hmm, just to give my 2 cents. When there is not obviously just one way to order objects (like numbers etc have some natural order to them), I think Ord should have as objective to "just specify any ordering that is fast and cheap to execute".

It doesn't matter if this ordering is consistent with any external ordering and I also don't like to make the ordering part of the type's definition if it's not a natural order. That's why I think deriving Ord is almost always the best option if it's possible, it will almost always result is the cheapest ordering.

If there are some BIPs or externally specified orderings, I think it's better to specify those external to the type itself.

Most sorting- and order-related functions provide ways to define custom orderings. So creating methods that easily plug into those APIs is IMO a better way to add support for special external orderings.

So if this PR's main intention is to implement an order defined in BIP67, I'd suggest having a module bip67 in utils with some methods like

use std::cmp;

pub struct SortKey( ... );

impl cmp::Ord for SortKey { ... }

pub fn sort_key(pk: &PublicKey) -> SortKey { .. }

pub fn cmp(pk1: &PublicKey, pk2: &PublicKey) -> cmp::Ordering {
    cmp::Ord::cmp(sort_key(pk1), sort_key(pk2))
}

#[cfg(test)]
mod test {
    #[test]
    fn test_bip67_order() {
        let mut slice: [PublicKey; 3] = ["xx".parse().unwrap(), "yy".parse().unwrap(), "zz".parse().unwrap()];
        slice[..].sort_by_key(super::sort_key);
        assert_eq!(slice, [ ... ]);
    }
}

Though in BIP67's case, I might not be objected too much against using Ord itself. Thought in that case, I'd definitely want to have that documented on the PublicKey type and put in a regression test.

dpc · 2020-11-26T17:41:55Z

@stevenroose Perfectly explained. In this case it's not too bad, but still doesn't sit well with me.

apoelstra · 2020-11-27T16:36:09Z

To be clear, BIP67 forbids uncompressed keys, and pretty-much any sane ordering would do the right thing with compressed keys. The unnaturalness comes from Bitcoin Core doing unnatural things with uncompressed keys and then exposing this in the sortedmulti descriptor.

sanket1729 · 2020-11-28T01:29:51Z

I will add the methods as @stevenroose suggested in bip67 module in utils.

I think Ord should have as objective to "just specify any ordering that is fast and cheap to execute".

But I think we should not derive Ord/PartialOrd for these types in order to avoid downstream assuming these have meaning according to a popular BIP/or as it is implemented in core.

RCasatta · 2020-11-28T19:25:43Z

FWIW sort_by_cached_key maybe could save some re-serialization

apoelstra · 2020-12-03T14:42:09Z

sort_by_cached_key is not available in Rust 1.29

sanket1729 · 2020-12-21T15:43:42Z

Something feels off in assuming bitcoin-core as externally specified ordering when IMO rust-bitcoin should try to match bitcoin core.

Overall, I don't feel so strongly about this so I have implemented changes as requested by @stevenroose. I have also added a warning to warn users they might not get the same results as in bitcoin core when using Ord/PartialOrd from rust-bitcoin.

stevenroose

LGTM. I realize the API is not the most ideal. (Made a suggestion, but still.) Any specific reason to not call the module bip67? Because it is more broad and supports uncompressed keys?

stevenroose · 2020-12-27T23:32:39Z

src/util/sort_key.rs

+/// instead of naturally derived order which compares underlying
+/// 64 byte secp key.
+#[derive(Debug, Eq, PartialEq, Clone)]
+pub struct SortKey<'pk>(&'pk PublicKey);


In fact to make this a bit more ergonomic, this could be a Cow<PublicKey> so that this can also be used in owned form (like to build a map structure without having to keep the keys separately).

dr-orlovsky

I think sorting according to BIP-67 and Bitcoin Core rules should be clearly separated; with the first one being PartialOrd only. See my suggestions in comments

dr-orlovsky · 2021-01-30T14:34:29Z

src/util/sort_key.rs

+/// bytes when comparing the two PublicKeys in compressed form
+/// instead of naturally derived order which compares underlying
+/// 64 byte secp key.
+#[derive(Debug, Eq, PartialEq, Clone)]


This also should be Copy and Hash

src/util/sort_key.rs

dr-orlovsky · 2021-01-30T14:37:56Z

src/util/sort_key.rs

+}
+
+#[cfg(test)]
+mod test {


It will be really great to have all test vectors from https://github.com/bitcoin/bips/blob/master/bip-0067.mediawiki#test-vectors

Will add along with bip67Key type

dr-orlovsky · 2021-01-30T14:40:20Z

src/util/sort_key.rs

+    }
+}
+
+impl<'pk> PartialOrd for SortKey<'pk> {


BTW, according to BIP-67, "Uncompressed keys are incompatible with this specificiation. A compatible implementation should not automatically compress keys. Receiving an uncompressed key from a multisig participant should be interpreted as a sign that the user has an incompatible implementation."

So my proposal here is to make PartialOrd returning None if any of the keys is uncompressed (and leave Ord as is)

Or, alternatively, two types should be used: Bip67Key, which will be PartialOrd only; and SortedKey for Bitcoin Core-style sorting with the current code unchainged. Top-namespace cmp function should be split into two, bip67_partial_cmp -> Option<cmp::Ordering> and cmp, left as is.

I like the second idea with two types. In fact, my primary motivation was to have only the SortKey type to emulate core behavior with no focus on bip67 all. This is because in sortedmulti() descriptor, bitcoin core does allow uncompressed keys and sorts them with this logic.

Then, it also makes sense to add testcases from bip67.

I will amend the PR with two separate types.

dr-orlovsky · 2021-01-30T14:45:21Z

Using rust fmt on the files which were never formatted before makes this PR hardly compatible with other PRs, constantly requiring rebase

dr-orlovsky · 2021-04-28T18:27:55Z

I also assume that we probably should use bitcoin-core/secp256k1#850

sgeisler · 2021-05-23T22:16:58Z

Any updates? Needs a rebase, but apart from that looks good with the newtype for ordering. One question: should it maybe support both owned and borrowed keys? Maybe via struct SortKey<K: AsRef<PublicKey>>(K)?

sanket1729 · 2021-08-01T20:34:46Z

I will work on this today.

Kixunil · 2021-08-02T19:51:19Z

Perhaps wait for #635 so that sort_by_cached_key could be used?

tcharding · 2022-05-25T04:01:17Z

This PR could be revived if you have time @sanket1729.

sanket1729 · 2022-06-01T19:25:05Z

Thanks for the ping, @tcharding. I will work this one this week

tcharding · 2022-06-28T04:35:11Z

This PR is currently marked as part of the 0.29 milestone, are you still hoping to get this in before the 0.29 release?

sanket1729 · 2022-06-29T03:50:58Z

@tcharding, sorry for the delay. I am caught with a bunch of things on my plate right now. I might come back to attend this sometime later. This is open for grabs meanwhile. We can remove the 0.29 milestone from this.

tcharding · 2022-06-29T05:09:41Z

No worries, cheers man.

junderw · 2022-06-30T01:29:37Z

I can pick this up.

Let me know if this TODO list is accurate:

add Bip67Key with PartialOrd only (otherwise it is basically the same)
use Cow<PublicKey> instead of &PublicKey
Also, don't mean to bike-shed, but maybe we should remove all the cargo fmt changes?

Kixunil · 2022-06-30T15:53:43Z

Looking at this now, I think with sort_by_cached_key we can have a better design:

// Not sure if another type is very useful since the representation will not change anyway
// Maybe we could just return `[u8; 33]` or `type Bip67SortKey = [u8; 33];`
#[derive(Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
pub struct Bip67SortKey([u8; 33]);

pub struct CompressedKey(ffi::PublicKey);

impl CompressedKey {
    pub fn bip67_sort_key(&self) -> Bip67SortKey {
        Bip67SortKey(self.serialize())
    }
}

// in consumer code:
keys.sort_by_cached_key(CompressedKey::bip67_sort_key);

I find the usage elegant. :) Note that this incorporates #826 per BIP67:

For a set of public keys, ensure that they have been received in compressed form

I strongly believe that having a newtype for compressed keys is better than silent conversion or wrong order.

Of course there may still be the need for use in BTree{Map,Set}. If we're going to make a newtype for that I suggest making it generic with Borrow bound instead of Cow:

use core::borrow::Borrow;

#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
pub struct Bip67SortedPublicKey<K = CompressedPublicKey>(pub K) where K: Borrow<CompressedPublicKey>;

// manually impl Ord, Partial ord

Note that while it may be tempting to impl Borrow<CompressedPublicKey> for Bip67SortedPublicKey<K> it'd be wrong because orderings don't match. Sadly, this leads to a bit worse ergonomics around using Btree{Map,Set} but I don't think we can do anything about it. Implementing the reverse doesn't help either and would require unsafe or a suspicious hack anyway.

Edit: added a default for K since I think it may be handy.

junderw · 2022-06-30T23:31:40Z

What about:

Add a new util module called sort
Add a module inside that called serialized
Add a module inside that called by_cached_key
Include fn for_public_key(self: &PublicKey) -> [u8]
After CompressedKey is merged, add fn for_compressed_key(self: &CompressedKey) -> [u8; 33]

Or

Create a new util module called sort
Create a new trait called SerializedSort
Include method fn cached_sort(&self) -> [u8]
impl SerializedSort for PublicKey, and CompressedKey once merged.
Mention BIP67 in the docs for the CompressedKey impl.

Kixunil · 2022-07-01T08:26:46Z

Adding a new module just for a function looks too much to me. Adding a global module for various sorting utilities is even worse because it goes against the idea of splitting up the crate.

junderw · 2022-07-01T12:08:30Z

Yeah, I guess so...

Perhaps this could just be an associated function that takes &self on PublicKey and returns [u8] for now.

Then if/when CompressedKey lands it can also have a similar associated function.

I am personally against naming the API itself after BIP67... since the bip is not widely known by number (in the sense that someone thinking "I want to sort these keys" won't think "hmm, I wonder if there's a BIP67 fn or mod somewhere..." even though they might search for bip39 when thinking about mnemonics.

In the docs for the PublicKey fn we can say that it is BIP67 compliant under the condition that all PublicKeys in the slice are compressed. Then in the docs for CompressedKey we can say it's always BIP67 compliant.

Like OP said, Bitcoin Core sorts any mixture of un/compressed pubkeys, so this is what I think would be best.

If there are no objections I will work on it this weekend.

Kixunil · 2022-07-01T19:21:40Z

If we provide it for uncompressed keys then it must not be named BIP67. :)

What's the correct sort order of uncompressed keys?

apoelstra · 2022-07-01T19:35:06Z

What's the correct sort order of uncompressed keys?

Bitcoin Core uses lexicographic order of serialized keys. This is the closest thing to "correct" that we have.

I agree that we shouldn't call this BIP67 if it supports uncompressed keys. Also I don't really like BIP67 :). But we could comment that, if you want a BIP67 sort, you can filter out uncompressed/hybrid keys and then use this logic.

junderw · 2022-07-02T02:15:29Z

Ok. So pretty much the implementation is straight forward, basically just like to_bytes but without all the Vec allocation.

Perhaps to_slice is a good name.

Then we can mention sort_by_cached_key in the docs as a concrete example, and mention BIP67 (for searchability) and how to use to_slice to achieve it.

junderw · 2022-07-03T11:46:39Z

See #1084

Kixunil · 2022-07-04T08:40:11Z

@junderw it can not be to_slice since you can't borrow a slice from PublicKey and you can't make it 'static without leaking memory. If we want to avoid allocation we need a special type. I actually wrote it experimentally for @dr-orlovsky

I suggest to copy the code from there and change to_bytes() to return SerializedPublicKey instead of Vec<u8>. The APIs are very similar so while it's semver-breaking it shouldn't be too annoying and at worst people will have to call .into()

junderw · 2022-07-04T08:44:47Z

Please check out my PR and give any feedback.

I return a [u8; 65] for explicit use with the *_by_key sort methods on slice.

Zero allocations.

…y key 24f0441 Add PublicKey::to_sort_key method for use with sorting by key (junderw) Pull request description: Replaces #524 See previous PR for reasoning. This solution is a little more straightforward. The name and documentation should be enough to prevent misuse. We can also impl a to_sort_key for any CompressedKey added later. (or just impl Ord in a BIP67 compliant way) TODO: - [x] Add more sorting test vectors. Ideas of edge cases to test are welcome. ACKs for top commit: apoelstra: ACK 24f0441 tcharding: ACK 24f0441 Kixunil: ACK 24f0441 Tree-SHA512: 92d68cccaf32e224dd7328edeb3481dd7dcefb2f9090b7381e135e897c21f79ade922815cc766c5adb7ba751b71b51a773d103af6ba13fc081e1f5bfb846b201

junderw · 2022-07-15T06:25:58Z

I think this can be closed now.

sanket1729 force-pushed the ord_pk branch from e5775cf to 0c7c863 Compare November 23, 2020 21:31

dpc reviewed Nov 23, 2020

View reviewed changes

sanket1729 force-pushed the ord_pk branch from 0c7c863 to 2bf1590 Compare November 23, 2020 21:35

dpc reviewed Nov 23, 2020

View reviewed changes

apoelstra previously approved these changes Nov 24, 2020

View reviewed changes

RCasatta mentioned this pull request Dec 3, 2020

Rust 2018 & MSRV bump 1.41.1 tracking issue #510

Closed

20 tasks

dr-orlovsky mentioned this pull request Dec 21, 2020

bump version to 0.26 #533

Merged

sanket1729 added 3 commits December 21, 2020 09:25

Add sortKey for bitcoin core sorting support

1a7321f

Add warning for using default implementation of Ord/partialOrd

2b55d0e

Make util files rustfmt friendly

a6f2629

sanket1729 dismissed apoelstra’s stale review via a6f2629 December 21, 2020 15:29

sanket1729 force-pushed the ord_pk branch from 2bf1590 to a6f2629 Compare December 21, 2020 15:29

stevenroose previously approved these changes Dec 27, 2020

View reviewed changes

dr-orlovsky suggested changes Jan 30, 2021

View reviewed changes

sanket1729 mentioned this pull request Apr 6, 2021

Improve the dynafed::Params fmt::Debug implementation ElementsProject/rust-elements#72

Merged

dr-orlovsky dismissed stevenroose’s stale review via a6f2629 August 1, 2021 16:30

tcharding added this to the 0.29.0 milestone Jun 24, 2022

sanket1729 modified the milestones: 0.29.0, 0.30.0 Jun 29, 2022

junderw mentioned this pull request Jul 3, 2022

Add PublicKey::to_sort_key method for use with sorting by key #1084

Merged

1 task

Kixunil closed this Jul 15, 2022

impl PartialOrd/Ord for PublicKey #524

impl PartialOrd/Ord for PublicKey #524

Conversation

sanket1729 commented Nov 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

stevenroose commented Nov 26, 2020

dpc commented Nov 26, 2020

apoelstra commented Nov 27, 2020

sanket1729 commented Nov 28, 2020

RCasatta commented Nov 28, 2020

apoelstra commented Dec 3, 2020

sanket1729 commented Dec 21, 2020

stevenroose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-orlovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 Jan 30, 2021 • edited

Choose a reason for hiding this comment

dr-orlovsky commented Jan 30, 2021

dr-orlovsky commented Apr 28, 2021

sgeisler commented May 23, 2021

sanket1729 commented Aug 1, 2021

Kixunil commented Aug 2, 2021

tcharding commented May 25, 2022

sanket1729 commented Jun 1, 2022 • edited

tcharding commented Jun 28, 2022

sanket1729 commented Jun 29, 2022 • edited

tcharding commented Jun 29, 2022

junderw commented Jun 30, 2022

Kixunil commented Jun 30, 2022 • edited

junderw commented Jun 30, 2022

Kixunil commented Jul 1, 2022

junderw commented Jul 1, 2022

Kixunil commented Jul 1, 2022

apoelstra commented Jul 1, 2022

junderw commented Jul 2, 2022

junderw commented Jul 3, 2022

Kixunil commented Jul 4, 2022

junderw commented Jul 4, 2022

junderw commented Jul 15, 2022

sanket1729 Jan 30, 2021 •

edited

sanket1729 commented Jun 1, 2022 •

edited

sanket1729 commented Jun 29, 2022 •

edited

Kixunil commented Jun 30, 2022 •

edited