Add convenience methods for keys #430

tcharding · 2022-03-24T04:15:24Z

We have a bunch of from_<key> methods for converting between key types. To make the API more ergonomic to use we can add methods that do the same but called on a variable e.g., once applied the following are equivalent:

let pk = PublicKey::from_keypair(kp)
let pk = kp.public_key()

Do this for SecretKey, PublicKey, KeyPair, and XOnlyKeyPair.

Fixes: #428

Note to reviewers

XOnlyPublicKey -> PublicKey logic is made up by me, I could not work out how to get libsecp256k1 to do this.
Please review the tests carefully, they include assumptions based on my current understanding of the cryptography :)

apoelstra · 2022-03-26T00:44:57Z

It looks like we borrow SecretKey in other places, so good to be consistent. But it's a Copy type and it doesn't matter if we borrow it or not.

Regarding your questions,

You need the parity to do XOnlyPublicKey -> PublicKey.
I agree that we should return the parity when converting keypairs or pubkeys to XOnlyPublicKeys.
Patch 2 looks fine to me.

sanket1729 · 2022-03-27T19:34:37Z

I'm at the edge of my knowledge here but my thoughts are that this should be possible since Taproot pubkeys only use x co-ordinates and the x/y co-ordinate information is what is in the 33rd byte of PublicKey, right?

The 33rd byte is 0x02. XOnlyPublicKey is PublicKey with first byte Even. I hope that also answers the Parity question.

sanket1729 · 2022-03-27T19:38:05Z

@apoelstra

You need the parity to do XOnlyPublicKey -> PublicKey.

I don't think we need it. I was under the impression that XOnlyPublicKey is PublicKey with even Y

tcharding · 2022-03-27T23:05:50Z

I'm glad I'm not the only one who gets confused by this :)

I thought that too @sanket1729 but if that was the case we would never need parity when using XOnlyPublicKey because it would always be even, right? I.e., a tweaked XOnlyPublicKey would not need parity if XOnlyPublicKeys were always even.

sanket1729 · 2022-03-27T23:21:28Z

a tweaked XOnlyPublicKey would not need parity if XOnlyPublicKeys were always even.

Yes, the tweaked output key is XOnlyPublicKey. But we need a parity bit for checking the taptweak equation.
In Q = P + H(S||P)*G

Q = Output Key (XonlyPublicKey)
P =Internal key (XonlyPublicKey)
S = Script
G = generator

The output of P + H(S||P)*G is a regular EC point additional and does not necessarily have even Y. But Q must be a XOnlyPublicKey. Therefore, we need to supply the extra parity information of the output key. This is done in the control block for taproot script spend. When using key spend, we sign with keypair corresponding to Q, so there is no question of the parity bit.

I hope this answers the question.

tcharding · 2022-03-28T05:34:44Z

Legend, thanks.

apoelstra · 2022-03-28T13:49:09Z

@apoelstra

You need the parity to do XOnlyPublicKey -> PublicKey.

I don't think we need it. I was under the impression that XOnlyPublicKey is PublicKey with even Y

When it appears as the external key (and in pretty-much every other context), yes. But specifically when it is used for internal keys, no (as you note -- you need the parity of the internal key to check the taptweak equation).

Having said this, I wouldn't mind implementing From<PublicKey> for XOnlyPublicKey which assumes Evenness, and adding a separate from_key_with_parity method.

sanket1729 · 2022-03-28T16:46:06Z

you need the parity of the internal key

the parity bit of output key. The internal key is always x-only with even Y.

Having said this, I wouldn't mind implementing From for XOnlyPublicKey which assumes Evenness, and adding a separate from_key_with_parity method.

You mean From<XOnlyPublicKey> for PublicKey?

apoelstra · 2022-03-28T16:56:26Z

the parity bit of output key. The internal key is always x-only with even Y.

Ah you're right, I was confused.

You mean From for PublicKey?

Yes, sorry, that's what I meant.

tcharding · 2022-03-29T01:32:50Z

But it's a Copy type and it doesn't matter if we borrow it or not.

That's right, we've had this discussion before - whether to borrow a 32 byte array or not :) I've favored consistency but updated the commit message.

sanket1729 · 2022-03-29T01:49:45Z

For reference: rust-bitcoin/rust-bitcoin#708. It seems that there was some consensus that we should take 32-byte arrays by value as they already implement Copy.

However, that is not related to this PR, and we can address that in another breaking release.

dr-orlovsky

Some non-critical naming suggestions

dr-orlovsky · 2022-03-30T08:46:36Z

src/key.rs

+    ///
+    /// This is equivalent to using [`KeyPair::from_secret_key`].
+    #[inline]
+    pub fn keypair<C: Signing>(&self, secp: &Secp256k1<C>) -> KeyPair {


If we use KeyPair, according to rust naming guidelines this should be key_pair, like in PublicKey/public_key

Lol, similar to sighash. I have no preference on this one but we should be consistent, yes.

Double lol, I noticed this while writing this PR too and hoped to no one else would notice because KeyPair/keypair has to be addressed in rust-secp256k1 as well, its an invasive change.

I think the problem is that capitalization adds less visual noise than underscoring. Thus, the same API guidelines to add capital letters and underscores in practice do not work well for human eyes.

Personally I do not care if we will use KeyPair and keypair at the same time - we already doing that all around the library.

dr-orlovsky · 2022-03-30T08:48:06Z

src/key.rs

@@ -418,6 +443,19 @@ impl PublicKey {
        }
    }

+    /// Creates a [`PublicKey`] using the key material from `xonly` combined with `parity`.
+    pub fn from_x_only_public_key_and_parity(xonly: XOnlyPublicKey, parity: Parity) -> Result<PublicKey, Error> {


Rust API guidelines recommend to name parameters taking many args to use with_* pattern. I propose here with_xpubkey_parity

I think from is clearer because we're literally transforming the inputs, the parameters are not modifiers on a default PublicKey.

I went looking and couldn't find the API docs for with vs for, if you disagree with @apoelstra could you please post a link @dr-orlovsky?

Re the ridiculously long name; I was trying to be uniform with the other functions that use x_only_public_key in the function name. However x_only_public_key functions also return the parity now so possibly we could shorten them all to xonly, users would know that 'xonly' means XOnlyPublicKey and Parity?

As an example

/// Returns the [`XOnlyPublicKey`] (and it's [`Parity`]) for this [`SecretKey`]. /// /// This is equivalent to `XOnlyPublicKey::from_keypair(self.keypair(secp))`. #[inline] pub fn xonly<C: Signing>(&self, secp: &Secp256k1<C>) -> Result<(XOnlyPublicKey, Parity), Error> { let kp = self.keypair(secp); XOnlyPublicKey::from_keypair(&kp) }

@tcharding https://github.com/rust-lang/rfcs/blob/master/text/0430-finalizing-naming-conventions.md#general-naming-conventions

I had read in some previous docs on rust naming in APIs that with_* stands for when we have more than a one parameter, while from are the conversions from a specific single type.

I will agree with any naming: all my naming comments are just nits; in fact with modern IDE I do not really care about function names since I always check the function content when do not remember what it does.

Thanks for the link, I ended up using from_xonly - this may violate the naming convention since xonly is not referring to a single type but to the key and parity?

from_xonly is good by me. Have a mild preference for from_xonly_parity but it's fine, the fact that you need the parity is clear from the function signature (and will be clear from the compiler error if you use it wrong).

dr-orlovsky · 2022-03-30T08:48:53Z

src/key.rs

+
+    /// Returns the [`XOnlyPublicKey`] (and it's [`Parity`]) for this [`PublicKey`].
+    #[inline]
+    pub fn x_only_public_key<C: Signing>(&self, secp: &Secp256k1<C>) -> Result<(XOnlyPublicKey, Parity), Error> {


split_xpubkey_parity?

I think that name is confusing and hard to remember.

dr-orlovsky · 2022-03-30T08:49:07Z

src/key.rs

    #[inline]
-    pub fn public_key(&self) -> XOnlyPublicKey {
+    pub fn x_only_public_key(&self) -> Result<(XOnlyPublicKey, Parity), Error> {


sanket1729

Partial review

src/key.rs

tcharding · 2022-03-30T23:39:27Z

Changes in force push:

Rename x_only_public_key functions to xonly
Rename from_x_only_public_key_an_parity -> from_xonly
rebase on master
Remove errors from newly name xonly functions (i.e., use expect for parity conversion)

sanket1729 · 2022-03-31T05:29:29Z

Rename x_only_public_key functions to xonly

I am not a big fan of this. Just feels incomplete, maybe I like the completeness of xonly_public_key (or x_only_public_key). I think we can overboard with this logic. Why name functions public_key instead of public? Users would know what it means public key. I think having the function name the same as the output type has immense value in terms of code readability.
Code reviewers using this function don't have to ask the extra question "What's xonly here? Oh, it must mean that XOnlyPublicKey". I don't think xonly_public_key is a ridiculously long name for the consistency and readability benefit it offers.

It is good to be consistent with the naming convention to convert to a type MyNewType, we call the method .my_new_type(). I think it's okay to have xonly_public_key instead of x_only_public_key. I kind of feel a bit strongly about it, but willing to change my mind about this.

tcharding · 2022-03-31T20:51:30Z

Thanks for you input @sanket1729, I totally agree with you when the return type was just the XOnlylPublicKey but its the Parity as well now, does that effect your view on the name? Totally open to something better than xonly though.

dr-orlovsky · 2022-04-01T10:58:04Z

This is why I am a big fan using Pubkey all around instead of PublicKey - to make function names like that shorter. XOnlyPubkey, xonly_pubkey, InternalPubkey, internal_pubkey etc.

sanket1729

Looks good. Some final comments. Maybe @apoelstra has opinions about xonly public key parities.

sanket1729 · 2022-04-01T18:05:53Z

src/key.rs

@@ -418,6 +443,20 @@ impl PublicKey {
        }
    }

+    /// Creates a [`PublicKey`] using the key material from `pk` combined with the `parity`.
+    pub fn from_xonly(pk: XOnlyPublicKey, parity: Parity) -> Result<PublicKey, Error> {


I am sorry to bring this up again. But I think this should add 0x02 by default. XOnlyPublicKey is a full public key with 0x02 y co-ordinate.

I am unable to make forward progress on this PR, @sanket1729 and @apoelstra currently have opposing views, how do we want to move forward, with or without parity parameter?

I concede, let's go with @sanket1729's suggestion.

sanket1729 · 2022-04-01T18:07:46Z

src/key.rs

+        };
+        buf[1..].clone_from_slice(&pk.serialize());
+
+        PublicKey::from_slice(&buf)


You can safely unwrap/expect here.

Cool, thanks. Error return removed as suggested.

sanket1729 · 2022-04-01T18:08:53Z

src/key.rs

+    /// This is equivalent to using [`PublicKey::from_xonly_and_parity(self, parity)`].
+    #[inline]
+    pub fn public_key(&self, parity: Parity) -> Result<PublicKey, Error> {
+        PublicKey::from_xonly(*self, parity)


After the abox fix, this should not return Result. And I think we should have not a parity parameter here.

`SecretKey` implements `Copy` and it is fine to take owneship of it; we have multiple methods called `from_secret_key` and they all borrow the secret key parameter. Favour consistency over perfection. Borrow secret key parameter as is done in other `from_secret_key` methods.

We have two places in the code where we pass a mutable parity integer to ffi code. At one callsite we tell the compiler explicitly what type it is (`::secp256k1_sys::types::c_int`) and at the other call site we let the compiler figure out the type. Is one way better than the other? I don't know. But letting the compiler figure it out seems to make the code easier to read.

We have a bunch of `from_<key>` methods for converting between key types. To improve the API and make it more ergonomic to use we can add methods that do the same but can be called on the initial key instead of on the resulting key's type. E.g. once applied the following are equivalent: - `let pk = PublicKey::from_keypair(kp)` - `let pk = kp.public_key()` Do this for `SecretKey`, `PublicKey`, `KeyPair`, and `XOnlyKeyPair`.

tcharding · 2022-04-04T03:01:09Z

Changes in force push:

Revert to using x_only_public_key and from_x_only_public_key
Remove error returns and use expect as suggested
Rebase on master

The parity parameter is still present but if it should be or not is still unresolved.

sanket1729

ACK f08276a. Thanks for going through all the iterations.

apoelstra

ACK f08276a

Yes, thanks for iterating!

tcharding force-pushed the 428-key-util-methods branch 3 times, most recently from 0be9a1f to 3e63c6d Compare March 29, 2022 05:17

tcharding marked this pull request as ready for review March 29, 2022 05:27

dr-orlovsky reviewed Mar 30, 2022

View reviewed changes

sanket1729 reviewed Mar 30, 2022

View reviewed changes

src/key.rs Outdated Show resolved Hide resolved

src/key.rs Outdated Show resolved Hide resolved

src/key.rs Outdated Show resolved Hide resolved

tcharding force-pushed the 428-key-util-methods branch from 3e63c6d to 927839b Compare March 30, 2022 23:31

tcharding mentioned this pull request Mar 31, 2022

Push key xonly rust-bitcoin/rust-bitcoin#920

Merged

dr-orlovsky added this to In review in Taproot Apr 1, 2022

sanket1729 reviewed Apr 1, 2022

View reviewed changes

tcharding added 3 commits April 4, 2022 12:50

tcharding force-pushed the 428-key-util-methods branch from 927839b to f08276a Compare April 4, 2022 02:59

sanket1729 approved these changes Apr 28, 2022

View reviewed changes

apoelstra approved these changes Apr 30, 2022

View reviewed changes

Taproot automation moved this from In review to Ready for merge Apr 30, 2022

apoelstra merged commit a30e9bb into rust-bitcoin:master Apr 30, 2022

Taproot automation moved this from Ready for merge to Done Apr 30, 2022

tcharding deleted the 428-key-util-methods branch May 3, 2022 21:52

Kixunil mentioned this pull request Jun 20, 2022

Bump crate version to 0.23.0 #444

Merged

sanket1729 mentioned this pull request Jun 21, 2022

Audit FFI methods return values #455

Open

Add convenience methods for keys #430

Add convenience methods for keys #430

Conversation

tcharding commented Mar 24, 2022 • edited

Note to reviewers

apoelstra commented Mar 26, 2022

sanket1729 commented Mar 27, 2022

sanket1729 commented Mar 27, 2022 • edited

tcharding commented Mar 27, 2022

sanket1729 commented Mar 27, 2022

tcharding commented Mar 28, 2022

apoelstra commented Mar 28, 2022

sanket1729 commented Mar 28, 2022 • edited

apoelstra commented Mar 28, 2022

tcharding commented Mar 29, 2022 • edited

sanket1729 commented Mar 29, 2022

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

tcharding Mar 30, 2022 • edited

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

sanket1729 left a comment

Choose a reason for hiding this comment

tcharding commented Mar 30, 2022 • edited

sanket1729 commented Mar 31, 2022

tcharding commented Mar 31, 2022

dr-orlovsky commented Apr 1, 2022

sanket1729 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

Choose a reason for hiding this comment

tcharding commented Apr 4, 2022 • edited

sanket1729 left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Mar 24, 2022 •

edited

sanket1729 commented Mar 27, 2022 •

edited

sanket1729 commented Mar 28, 2022 •

edited

tcharding commented Mar 29, 2022 •

edited

tcharding Mar 30, 2022 •

edited

tcharding commented Mar 30, 2022 •

edited

tcharding commented Apr 4, 2022 •

edited