Re-implementing public key ordering using underlying FFI functions

[dr-orlovsky]

No description provided.

[dr-orlovsky]

Not sure I understand why fuzzification destroys comparison operation in tests. Should I move ffi:*_cmp functions to #[cfg(not(fuzzify))] section and mark all Ord/PartialOrd impl and corresponding tests as #[cfg(not(fuzzify))]?

[TheBlueMatt]

Can you write up a benchmark for this in some context (eg inserting public keys into a BtreeMap)? I'm somewhat worried the lack of ability to inline here may hurt some common use-cases.

[dr-orlovsky]

Added tests in commit 76cfe2b before the rest of commits so benchmarking with previous ordering version can be measured by checking it out.

Before (with autoderived cmp):

test key::benches::bench_pk_ordering  ... bench:       5,608 ns/iter (+/- 771)

After (with libsecp256k1 backed cmp):

test key::benches::bench_pk_ordering  ... bench:       4,107 ns/iter (+/- 855)

So with C function it is actually faster :)

[dr-orlovsky]

@TheBlueMatt any ideas on this? #309 (comment)

[TheBlueMatt]

So with C function it is actually faster :)

Huh. That sounds like a rather significant bug in LLVM or at least rustc, then, actually.

As for fuzz, I think best to re-implement it as a extern C function in fuzz_dummy module in secp256k1-sys if you need it to behave differently.

[real-or-random]

Oh, so this means that we the current derived implementation of Ord just compares the internal FFI bytes, which is platform-dependent...?

Concept ACK then ;) But this is an API change then.

[dr-orlovsky]

@TheBlueMatt I was finally able to make this PR to pass CI :) Pls review

[apoelstra]

Since there is no way to access a "real" representation of the keys without going through the C API, there is no way we can get the kind of inlining behavior that Matt wants. (Ok, we could make assumptions about the internal/opaque representation of the pubkey types, which we could get away with since we vendor specific versions of the library, but this representation is not supposed to be user-visible so we should really not do this.)

I am also very surprised by @dr-orlovsky benchmark results. They do not replicate for me. With auto-derive I get

test key::benches::bench_pk_ordering  ... bench:       3,344 ns/iter (+/- 38)

and with the FFI version I get

test key::benches::bench_pk_ordering  ... bench:       5,057 ns/iter (+/- 284)

But anyway, there is no non-FFI way to implement this so I think we are stuck.

[apoelstra]

With cfg(fuzzing) now there is no implementation of Ord for PublicKey, which will cause downstream compliation failures during fuzzing.

@dr-orlovsky can you implement some sort of ordering for fuzzable public keys? You can even just auto-derive one.

[TheBlueMatt]

My understanding of the libsecp API is that it *does* guarantee that the public key is a byte array of a certain number of bytes. Given we’re not seeking any specific order, but only *some* order, I don’t see why we’re not allowed to simply memcmp? It seems that would be API guaranteed to not be defined, though the specific order may vary between versions or be random.

…

On Sep 8, 2021, at 11:46, Andrew Poelstra ***@***.***> wrote:  Since there is no way to access a "real" representation of the keys without going through the C API, there is no way we can get the kind of inlining behavior that Matt wants. (Ok, we could make assumptions about the internal/opaque representation of the pubkey types, which we could get away with since we vendor specific versions of the library, but this representation is not supposed to be user-visible so we should really not do this.) I am also very surprised by @dr-orlovsky benchmark results. They do not replicate for me. With auto-derive I get test key::benches::bench_pk_ordering ... bench: 3,344 ns/iter (+/- 38) and with the FFI version I get test key::benches::bench_pk_ordering ... bench: 5,057 ns/iter (+/- 284) But anyway, there is no non-FFI way to implement this so I think we are stuck. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[apoelstra]

Hmm, yeah, that makes sense to me. cc @real-or-random what do you think? If we're comparing public keys from exactly the same build of libsecp, and we know that they are fixed-size byte arrays, and we don't care about the specific order, surely we can just use memcmp?

[elichai]

I remember we had a discussion around that on IRC, can we even assume that equal public keys have the same byte representation? because even for "some order" that should be important

[real-or-random]

My understanding of the libsecp API is that it does guarantee that the public key is a byte array of a certain number of bytes. Given we’re not seeking any specific order, but only some order, I don’t see why we’re not allowed to simply memcmp? It seems that would be API guaranteed to not be defined, though the specific order may vary between versions or be random.

I don't understand, maybe I lack context.

Why aren't we seeking any specific order? I think we do. Wouldn't it be strange to expose different order on every platform? We're doing this currently but I think it's a bug.

If performance is really an issue (I can believe it is), wouldn't it be possible to offer a verbose wrapper type with a fast Ord?

But assuming we think memcmp is fine, then why can't we stick with currently autoderived Ord which I think is essentially memcmp?

And yeah, I think @elichai is right. The secp256k1 API does nowhere state the guarantee that equal keys have an equal byte representation. Although we may extend secp256k1 to guarantee this, or simply rely on it: If you look at the implementation, those 64 bytes will (of course) hold the x-coordinate and the y-coordinate. This is not guaranteed either but if we make this assumption, then this already enough to deduce that there are no padding bytes in the FFI type (since both values are 32-byte integers), and this implies it's safe to use memcmp.

[apoelstra]

@real-or-random we cannot fix the performance issues by using wrapper types. The problem is that to get from the platform-dependent representation to any other representation, we have to go through the C layer, which makes inlining impossible.

[TheBlueMatt]

Why aren't we seeking any specific order? I think we do. Wouldn't it be strange to expose different order on every platform? We're doing this currently but I think it's a bug.

Hmm, why is this a bug? I don't see why a user would ever materially care, unless they're trying to be very specific, in which case you probably want to do it manually (Eg lightning cares about the order, serialized as 33 bytes, but we're not gonna get that from libsecp).

[apoelstra]

Because it may make it hard to send sorted data structures, e.g. binary search trees, across the wire.

[TheBlueMatt]

Because it may make it hard to send sorted data structures, e.g. binary search trees, across the wire.

Hmm, so that would be a bug for any platform-specific behavior but not for a specific ordering? I suppose we're kinda screwed either way there, though.

Of course the real bug here is that rust encourages rustup so heavily instead of encouraging distro packages, where cross-language LTO is a totally reasonable thing to expect, and then we wouldn't care.

This is somewhat annoying either way - for reference, rust-lightning does have a large BTreeMap with a PublicKey key. We don't care about the specific order, but do need reliable ordering when walking the map, so we'd be impacted by any performance change here. https://github.com/rust-bitcoin/rust-lightning/blob/75f7af6/lightning/src/routing/network_graph.rs#L58

[real-or-random]

@real-or-random we cannot fix the performance issues by using wrapper types. The problem is that to get from the platform-dependent representation to any other representation, we have to go through the C layer, which makes inlining impossible.

What I meant is this:

We can have a PublicKey and implement Ord using the FFI functions as in this in PR.
And then we can have a new struct PublicKeyUnstableOrd(PublicKey) where we implement Ord by comparing bytes. Here we don't need to go through the FFI functions, so this should be fast.

It's certainly not elegant but it does the job. A more elegant option would be comparators but nothing like this is in std. I read somewhere that a single Ord was chosen over comparators (maybe with a default comparator) because it makes the syntax simpler in most cases, and one can still emulate comparators using newtypes.

I don't see why a user would ever materially care, unless they're trying to be very specific, in which case you probably want to do it manually (Eg lightning cares about the order, serialized as 33 bytes, but we're not gonna get that from libsecp).

It's simply what (I personally) would expect from an order as I expect -3 < 17 to be true on every platform. In general, I don't expect Rust code to behave differently depending on the platform. That's a major inconvenience when you try to write portable code.

[elichai]

We could also have a PublicKeyStableOrd([u8; 64]) that stores uncompressed bytes and de-serializes when converted back to PublicKey (which is very cheap)
(So here you pay a small price on converting and then very cheap comparing, can also go with compressed and pay bigger price on converting but even cheaper comparing)

[real-or-random]

We could also have a PublicKeyStableOrd([u8; 64]) that stores uncompressed bytes and de-serializes when converted back to PublicKey (which is very cheap)
(So here you pay a small price on converting and then very cheap comparing, can also go with compressed and pay bigger price on converting but even cheaper comparing)

That's also reasonable. When you store the keys in a collection, you probably want to compare each key at least once, so you anyway want to serialize.

[apoelstra]

If somebody implements, say, a BST which is serialized as a flat vector, then sends it across the wire to a machine with a different ordering, the receiving machine will get mysteriously incorrect behavior when trying to use the BST.

I'm not sure how to parse

Hmm, so that would be a bug for any platform-specific behavior but not for a specific ordering?

Yes, any platform-specific behavior would result in these sorts of bugs ... but AFAIK the pubkey ordering is the only such behavior in the library right now.

[dr-orlovsky]

Can we summarize somehow to which specific conclusion we have arrived on this matter, such that I can update the PR?

@apoelstra @TheBlueMatt @real-or-random @elichai

[real-or-random]

I don't think we have arrived at a conclusion yet. The three proposals are:

• Don't care that Ord will be different across machines: Re-implementing public key ordering using underlying FFI functions #309 (comment)
• Introduce a struct PublicKeyUnstableOrd(PublicKey) with faster Ord, keep the normal Ord of PublicKey slow but consistent (Re-implementing public key ordering using underlying FFI functions #309 (comment))
• Do it the other way around: introduce PublicKeyStableOrd([u8; 64]) and keep the normal Ord of PublicKey fast but inconsistent (Re-implementing public key ordering using underlying FFI functions #309 (comment))
• Make Ord consistent and do not produce extra types.

[dr-orlovsky]

Is the option "let's make ordering consistent and not produce extra types" off the table?

[real-or-random]

Is the option "let's make ordering consistent and not produce extra types" off the table?

I added that one to the list.

[dr-orlovsky]

How we can move forward with making a decision of which way to follow?

[real-or-random]

I don't think I'm in the position to take a decision but (unsurprisingly) I still favor my own solution. @apoelstra Do you want to have a look and take a decision?

[apoelstra]

My decision for now is to punt for the next major release (which implicitly means "don't care that Ord is inconsistent"). But I'd like to revisit this in the future.

[TheBlueMatt]

Following up on this - we moved to a NodeId([u8; 33]) and stopped using PublicKeys in places where these things matter. In part so this can move forward and in part because deserializing a million PublicKeys on startup is slow and in part because it cuts the size of key performance-critical maps down substantially. I'm fine with this moving forward either way now.

[apoelstra]

I think we should implement (my) slow solution, and try to document somewhere that if you need fast sorting and searching of keys then you should pre-serialize them like this.

[Kixunil]

I think that focusing on correctness (cross-platform, stable ordering) is better for Bitcoin stuff. :) To avoid too many types, we could add a method cmp_unstable(&self, other: &PublicKey) -> cmp::Ordering that can be used in sort functions and such and also document that pre-serializing may be better anyway.

Is there a chance to get this in the next release? From what I understand the only difference between @apoelstra and @real-or-random opinions is that @real-or-random wants to provide the unstable wrappers while @apoelstra doesn't want the burden with providing them (LMK if I misunderstood). Maybe my suggestion is an acceptable middle ground? I personally don't care which of the three options is chosen as long as the default ordering is the stable one.

Regarding cross-language LTO, unless we're making it cumbersome, it should be the responsibility of the application developers to setup their environment to allow it if their code is performance-sensitive.

[apoelstra]

Yeah, I think if this were rebased I'd be happy to take it.

[Kixunil]

To my knowledge Maxim is busy at least until June 24. If someone has the time to rebase and open a new PR I think it'd be great. I may be able to do it in upcoming days too but not sure.

[apoelstra]

Superceded by #449