Add combine_keys function to PublicKey #291

Tibo-lg · 2021-03-31T01:09:16Z

Added a static method to PublicKey to enable computing the sum of a slice of public keys, as at the moment it looked like it was only possible to compute the sum of two public keys.

Tibo-lg · 2021-03-31T04:36:33Z

Looking at the CI failure and the fact that there is no use of Vec in the entire code base makes me think that there might have been a reason for not having this type of function in the first place, but I'd appreciate if someone can tell me if it's worth trying to find an alternative or if I should just give up.

elichai · 2021-03-31T07:31:33Z

Hmmm if we make PublicKey #[repr(transparent)] we should be able to strictly transmute a &[&PublicKey] to a &[*const ffi::PublicKey] and skip the heap allocation

Tibo-lg · 2021-03-31T09:08:01Z

@elichai Thanks for the suggestions, the transmutation trick seems indeed to work! Or at least it works for me locally. Is it safe to assume that the SIGILL on the CI is not caused by my change? (I can see it also happened on master)

elichai · 2021-03-31T09:10:49Z

We can see that miri doesn't complain here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=bb329246b3e8ad06a104d06d2aba429e

elichai · 2021-03-31T09:13:47Z

maybe also assert that keys.len() < i32::MAX

Tibo-lg · 2021-03-31T09:22:35Z

maybe also assert that keys.len() < i32::MAX

Added a debug_assert I hope that's what you had in mind.

elichai · 2021-03-31T09:38:08Z

src/key.rs

+    #[cfg(all(feature = "std"))]
+    pub fn combine_keys(keys: &[&PublicKey]) -> Result<PublicKey, Error> {
+        use std::mem::transmute;
+        use std::i32::MAX;


Can you use core and drop the feature gate?

Done thanks that's much better indeed.

real-or-random · 2021-03-31T09:59:22Z

Is it safe to assume that the SIGILL on the CI is not caused by my change? (I can see it also happened on master)

Yes, this will be fixed by #290

elichai

LGTM (@apoelstra what's our policy about assert vs debug_assert?)

elichai · 2021-04-07T15:05:17Z

@Tibo-lg Can you rebase please? that will solve the CI failures

Tibo-lg · 2021-04-07T21:36:47Z

@Tibo-lg Can you rebase please? that will solve the CI failures

Done!

elichai

Re-ACK with the same wondering about debug_assert vs assert

apoelstra

ack 7d32182

Funny, I have no recollection of the C function taking an arbitrary number of pubkeys.

Re debug_assert vs assert, we don't really have a policy ... but I think here where it's (a) an overflow condition, and (b) a programmer error, it's okay to use debug_assert. I'm not thrilled with it, but this is what libstd does all over the place, so it's consistent with the Rust ecosystem at least.

dr-orlovsky · 2021-06-09T06:28:27Z

src/key.rs

+    /// Adds the keys in the provided slice together, returning the sum. Returns
+    /// an error if the result would be the point at infinity, i.e. we are adding
+    /// a point to its own negation
+    pub fn combine_keys(keys: &[&PublicKey]) -> Result<PublicKey, Error> {


Post-merge note: PublicKey is a copy type and I am not sure why we need to have it here by reference

IIRC the reason is because we need a &[&PublicKey] to transmute. It would also be possible to take a &[PublicKey] and do the conversion internally but I guess I preferred to keep the library code minimal. I don't have a strong opinion if people want to change it though.

Yeah the C code takes pointer to a list of pointers, so we'd have to allocate a vector of pointers and we try to avoid allocations where possible

real-or-random · 2021-06-09T13:20:57Z

Funny, I have no recollection of the C function taking an arbitrary number of pubkeys.

To be honest, maybe upstream should reconsider this. The function is mostly useless to end users.

real-or-random · 2021-06-09T13:28:20Z

Funny, I have no recollection of the C function taking an arbitrary number of pubkeys.

To be honest, maybe upstream should reconsider this. The function is mostly useless to end users.

It was added by the PR of the old Schnorr module to normal secp256k1.h (outside the module) https://github.com/bitcoin-core/secp256k1/pull/212/files#diff-4c52bd5cae65c9519af920228863ca34102abef6414e790498caa9fb05f03bbdR431

@Tibo-lg Do you need this function or did you simply create this PR because to make the bindings more complete?

apoelstra · 2021-06-09T13:40:13Z

@real-or-random it is used somewhere by the LN folks (which is why it still exists in upstream BTW), so it would not surprise me if it has some use in rust-lightning

Tibo-lg · 2021-06-10T00:35:03Z

@real-or-random

@Tibo-lg Do you need this function or did you simply create this PR because to make the bindings more complete?

I need it to efficiently compute adaptor points (for adaptor signatures) based on multiple signature points, and using this version over calling combine multiple times is really much faster. I had actually some clever algorithmic optimization to compute adaptor points but it ended up being slower than just using this combine_keys without optimizing because it required adding points 2 by 2. If someone knows a way to speed things up when calling just combine it'd be really useful to me and I'd be happy to drop the combine_keys, but my guess (without knowing much about the internals of secp256k1) is that going from external to internal representation many times is what causes the slow down and so I don't think it can be improved.

If someone is curious I had made some quick and dirty benchmarks to compare both versions when adding 1000 pubkeys here.
Results show that pairwise addition is roughly 10 times slower:

test benches::add_batch    ... bench:     273,601 ns/iter (+/- 57,851)
test benches::add_no_batch ... bench:   2,760,124 ns/iter (+/- 146,883)

For just 10 keys it's still about 5 times slower:

test benches::add_batch    ... bench:       4,987 ns/iter (+/- 409)
test benches::add_no_batch ... bench:      24,859 ns/iter (+/- 2,331)

real-or-random · 2021-06-10T07:50:33Z

Thanks. combine_keys is entirely fine if we have combine. Speaking as an upstream maintainer, I was rather curious why people need to add pubkeys in the first place. So this is a variant of adaptor signatures with more than two parties involved? Can you tell me more or is there some writeup? (Again, just for my curiosity.)

Post merge review:
Here we assert that there is at least one key (because upstream does the same). This should be documented, or we should wrap it into a nicer error in combine_keys. And there should be a test for the empty slice.

rust-secp256k1/secp256k1-sys/src/lib.rs

Line 894 in bb25ed4

assert!(n >= 1);

Tibo-lg · 2021-06-10T12:14:00Z

Speaking as an upstream maintainer, I was rather curious why people need to add pubkeys in the first place. So this is a variant of adaptor signatures with more than two parties involved? Can you tell me more or is there some writeup? (Again, just for my curiosity.)

It's for Discrete Log Contracts (DLC) with numerical decompositions (or multiple oracles). You can check this post for example for a small write up. Note that as I mentioned above the memoization optimization that I describe there is actually slower than just using combine_keys with the "simple pre-computation" version.

Post merge review:

Tried to address your comments here: #304

Tibo-lg force-pushed the add-combine-keys branch from f5eba4c to c7522b5 Compare March 31, 2021 04:16

Tibo-lg force-pushed the add-combine-keys branch 3 times, most recently from 21a0482 to 3db7e6b Compare March 31, 2021 08:57

Tibo-lg force-pushed the add-combine-keys branch from 3db7e6b to 0c0e61d Compare March 31, 2021 09:21

elichai reviewed Mar 31, 2021

View reviewed changes

Tibo-lg force-pushed the add-combine-keys branch from 0c0e61d to 71a1bbf Compare March 31, 2021 11:52

elichai previously approved these changes Mar 31, 2021

View reviewed changes

Add combine_keys function to PublicKey

7d32182

Tibo-lg dismissed elichai’s stale review via 7d32182 April 7, 2021 21:28

Tibo-lg force-pushed the add-combine-keys branch from 71a1bbf to 7d32182 Compare April 7, 2021 21:28

elichai approved these changes Apr 8, 2021

View reviewed changes

apoelstra approved these changes Jun 8, 2021

View reviewed changes

apoelstra merged commit e89fd5d into rust-bitcoin:master Jun 8, 2021

dr-orlovsky reviewed Jun 9, 2021

View reviewed changes

Tibo-lg mentioned this pull request Jun 10, 2021

Add error type for combine keys + test and doc #304

Merged

apoelstra mentioned this pull request Jul 3, 2024

PublicKey::combine_keys doesn't have to fail on long slices #704

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add combine_keys function to PublicKey #291

Add combine_keys function to PublicKey #291

Tibo-lg commented Mar 31, 2021

Tibo-lg commented Mar 31, 2021

elichai commented Mar 31, 2021 •

edited

Loading

Tibo-lg commented Mar 31, 2021

elichai commented Mar 31, 2021

elichai commented Mar 31, 2021

Tibo-lg commented Mar 31, 2021

elichai Mar 31, 2021

Tibo-lg Mar 31, 2021

real-or-random commented Mar 31, 2021

elichai left a comment

elichai commented Apr 7, 2021 •

edited

Loading

Tibo-lg commented Apr 7, 2021

elichai left a comment

apoelstra left a comment

dr-orlovsky Jun 9, 2021

Tibo-lg Jun 9, 2021

elichai Jun 9, 2021

real-or-random commented Jun 9, 2021

real-or-random commented Jun 9, 2021

apoelstra commented Jun 9, 2021

Tibo-lg commented Jun 10, 2021

real-or-random commented Jun 10, 2021 •

edited

Loading

Tibo-lg commented Jun 10, 2021

Add combine_keys function to PublicKey #291

Add combine_keys function to PublicKey #291

Conversation

Tibo-lg commented Mar 31, 2021

Tibo-lg commented Mar 31, 2021

elichai commented Mar 31, 2021 • edited Loading

Tibo-lg commented Mar 31, 2021

elichai commented Mar 31, 2021

elichai commented Mar 31, 2021

Tibo-lg commented Mar 31, 2021

elichai Mar 31, 2021

Choose a reason for hiding this comment

Tibo-lg Mar 31, 2021

Choose a reason for hiding this comment

real-or-random commented Mar 31, 2021

elichai left a comment

Choose a reason for hiding this comment

elichai commented Apr 7, 2021 • edited Loading

Tibo-lg commented Apr 7, 2021

elichai left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

dr-orlovsky Jun 9, 2021

Choose a reason for hiding this comment

Tibo-lg Jun 9, 2021

Choose a reason for hiding this comment

elichai Jun 9, 2021

Choose a reason for hiding this comment

real-or-random commented Jun 9, 2021

real-or-random commented Jun 9, 2021

apoelstra commented Jun 9, 2021

Tibo-lg commented Jun 10, 2021

real-or-random commented Jun 10, 2021 • edited Loading

Tibo-lg commented Jun 10, 2021

elichai commented Mar 31, 2021 •

edited

Loading

elichai commented Apr 7, 2021 •

edited

Loading

real-or-random commented Jun 10, 2021 •

edited

Loading