Implement lexigraphic ordering for PubKey

[justinmoon]

While implementing a feature in rust-miniscript I ran into surprising behavior with public key ordering in rust-bitcoin. I would expect pubkeys to be lexigraphically ordered by compressed serialization (used by BIP 67), but instead it was based on the internal 64-byte libsecp256k1 representation.

@apoelstra mentioned he was in favor of changing this so I attempted to implement it.

[apoelstra]

I expect this won't compile on Rust 1.29. I think using &self.serialize()[..] will work because slices are comparable even though [u8; 33]s are not.

[justinmoon]

Tests pass on 1.29 on my machine:

$ cargo --version
cargo 1.29.0 (524a578d7 2018-08-05)

$ cargo test
running 33 tests
...
test result: ok. 33 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

   Doc-tests secp256k1

running 5 tests
...
test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

I see a Travis check here that's not "completed yet". Is this broken?

[apoelstra]

Yeah, CI seems pretty broken :(

[apoelstra]

Travis says "Abuse detected" https://travis-ci.org/github/rust-bitcoin/rust-secp256k1/requests

This is pretty stupid, let's just switch to github actions.

[apoelstra]

Once we get #250 in you'll need to rebase and then we'll be rid of Travis once and for all.

[real-or-random]

I think this could break existing code. I don't think it will but we can't know for sure.

[elichai]

Should we also implement PartialEq in terms of the serialized key? (I remember talking on #secp256k1 if we allow users to assume secp256k1_* objects are comparable or not, I don't remember the answer though hehe)

[apoelstra]

@elichai any such existing code was already buggy because it was violating the contract for the upstream public key type

[apoelstra]

@elichai as for PartialEq ... it is true that the contract says we're not allowed to compare based on the underlying, so morally we should fix this. But it's also true that in our particular vendored version, comparing equality based or serialize will give identical results to comparing based on the underlying representation. So I guess, I'm indifferent.

[real-or-random]

@elichai as for PartialEq ... it is true that the contract says we're not allowed to compare based on the underlying, so morally we should fix this. But it's also true that in our particular vendored version, comparing equality based or serialize will give identical results to comparing based on the underlying representation. So I guess, I'm indifferent.

We could ask upstream to give us an equality function.

[apoelstra]

@sipa informs me in bitcoin-core/secp256k1#850 (comment) that this PR is incorrect for sortedmulti in descriptors.

[sipa]

See https://github.com/bitcoin/bitcoin/blob/v0.20.1/src/script/descriptor.cpp#L679. CPubKey's ordering is lexicographic (i.e., compressed keys will sort before uncompressed ones).

[apoelstra]

@justinmoon can you try force-pushing this and see if that makes CI move?

[apoelstra]

I think we should get this PR in even if it doesn't match Core. Our current Ord impl sorts on the internal representation of the keys which is definitely wrong, and it's useful to be able to sort keys without compressedness flag somehow..

[justinmoon]

@apoelstra Done

[apoelstra]

lets do it

[elichai]

@apoelstra We can implement Ord differently here, and in rust-bitcoin where we can take the compressed flag into account

[apoelstra]

Yep. We should be sure to do both, because it's a footgun to use this Ord with Bitcoin keys in contexts where you may have uncompressed keys and you care about Core compat.