Taproot: BIP32 extended keys using Scep256k1 keys instead of bitcoin ECDSA #590

dr-orlovsky · 2021-04-12T11:59:07Z

This is third step required to introduce Schnorr key support according to #588. This PR starts API-breaking changes and is follow-up to non-API breaking #589, which is already merged.

PR rationale: BIP32 does not support uncompressed keys and using type with compression flag was a mistake

thomaseizinger

ACK d155115

src/util/bip32.rs

dr-orlovsky · 2021-06-27T17:15:38Z

Rebased on master, addressed comments, now ready for review

sanket1729

Thanks for this. The code looks great. I have a discussion point about API naming which might use more feedback from other contributors.

sanket1729 · 2021-06-28T00:45:02Z

src/util/bip32.rs

+
+    /// Constructs BIP340 keypair for Schnorr signatures and Taproot use matching the internal
+    /// secret key representation.
+    pub fn to_schnorr<C: secp256k1::Signing>(&self, secp: &Secp256k1<C>) -> schnorr::KeyPair {


I have a slight preference in favor of to_schnorr_keypair.

I also like to_ecdsa_privkey, to_ecdsa_pubkey, and to_schnorr_pubkey over the current ones. But this one is more debatable since the user has the context of what return type to expect.

Changed to to_keypair since there is no other keypair (and @apoelstra does not like "Schnorr" surname :D because of him patenting signature algo)

I would prefer to_schnorr_keypair. It's sad that this has patent history, but IMO the convenience and usability triumphs this.

Would like to get input from other contributors as well, but IMO to_keypair is just confusing. It could main ecdsa_keypair as well as schnorr_keypair.

Do we have a keypair type for non-BIP-340 keys?

I really do not like inconsistency in function names for extended keys that we have today in the API: derive_priv, derive_pub, new_master, from_private - and now to_ecdsa_privkey.... :(

Did a temporal version with priv/pub instead of privkey for more consisted naming for now; will wait for others to comment

Reviewing the discussion, after some thinking I think my vote is for to_ec_pubkey, to_xonly_keypair for rationale mentioned here:
#588 (comment)

src/util/bip32.rs

sgeisler

Looks pretty good. I just now saw @sanket1729's comment 😆 so take my comments with a grain of salt. As explained in the first comment my aversion to the more descriptive names comes from the idea of unifying the interface of x-keys via a common trait. For that the different names would be very annoying (but no blocker here, won't cut a release too soon anyway). I'll try to build it in a separate PR, the basic idea is to have:

pub trait XKey: Sized {
    type EcdsaKey;
    type SchnorrKey;

    fn derive<P: IntoIterator<Item=ChildNumber>>(&self, path: P) -> Self;
    fn to_ecdsa(&self) -> Self::EcdsaKey;
    fn to_schnorr(&self) -> Self::SchnorrKey;
}

sgeisler · 2021-06-29T09:33:16Z

src/util/bip32.rs

            chain_code: ChainCode::from(&hmac_result[32..]),
        })
    }

+    /// Constructs ECDSA compressed private key matching internal secret key representation.
+    pub fn to_ecdsa_priv(&self) -> ecdsa::PrivateKey {


I wonder if the _priv part is really useful. You know you are operating on an xpriv and the return type is a private key. Kinda related: I'd like to explore the possibility of putting the derive and conversion functions into a trait (not here though) to allow generic code over x-keys.

After #755, I think this should be just to_priv. As mentioned and discussed in several places, keys are not related to signature algorithms.

sgeisler · 2021-06-29T09:33:43Z

src/util/bip32.rs

+
+    /// Constructs BIP340 keypair for Schnorr signatures and Taproot use matching the internal
+    /// secret key representation.
+    pub fn to_schnorr_keypair<C: secp256k1::Signing>(&self, secp: &Secp256k1<C>) -> schnorr::KeyPair {


Same here _keypair seems redundant.

I missed this. This should also be to_keypair

sgeisler · 2021-06-29T09:35:06Z

src/util/bip32.rs

            chain_code: sk.chain_code
        }
    }

+    /// Constructs ECDSA compressed public key matching internal public key representation.
+    pub fn to_ecdsa_pub(&self) -> ecdsa::PublicKey {


redundant _pub

Same, this should be to_pub

sgeisler · 2021-06-29T09:35:14Z

src/util/bip32.rs

+
+    /// Constructs BIP340 x-only public key for BIP-340 signatures and Taproot use matching
+    /// the internal public key representation.
+    pub fn to_schnorr_pub(&self) -> schnorr::PublicKey {


redundant _pub

And this should be to_x_only_pub

apoelstra · 2021-07-09T16:36:55Z

What is the status of this? Does it need a rebase and/or the PR description updated to have different commit IDs?

dr-orlovsky · 2021-08-01T19:05:34Z

@apoelstra rebased, description update, ready for (re)reviews

sanket1729

I think we should resolve our discussion #588 (comment) before proceeding with this.

dr-orlovsky · 2021-08-09T16:19:39Z

@sanket1729 that discussion much more relates to previous PR #589 which is already merged (and a bitcoin-rust version with it is already released). If we need to change that approach, that should be a matter of a new PR.

This PR is very specifically removes unnecessary non-Secp256k1 pubkeys from BIP32/PSBT related data structures and does not directly related to the key type naming discussion within rust-bitcoin

sanket1729 · 2021-08-09T16:55:22Z

@dr-orlovsky, there is a discussion to possibly undo things in #589 (in a non-breaking way). If there is a consensus for that, this PR would be updated, namely, the function names that use ecdsa_ and schnorr_ should be ec_ and xonly_(x_only_)

dr-orlovsky · 2021-08-09T16:59:32Z

Why we can't do that alltogether after merging this PR, independently from it?

sanket1729 · 2021-08-13T01:32:44Z

I don't have a strong opinion but appears that there is consensus for removing signature names from functions names using x_only, ec.

This PR is not dependant on it and can get in without the other one, but there is no reason to have poor API names here just so we can change them later.

dr-orlovsky · 2021-08-22T09:12:57Z

@sanket1729 can you pls explain what exactly you propose to update in this PR?

sanket1729 · 2021-09-07T08:54:14Z

I was about to suggest APIs like

- pub fn to_ecdsa_priv(&self) -> ecdsa::PrivateKey
+ pub fn to_ec_priv(&self) -> ecdsa::PrivateKey

but reading this seems odd that API because it currently does not match the return type

ACK 7c565ec.

We can change this later when we address #588

sanket1729

ACK 7c565ec . Once we rename ecdsa and schnorr keys to ec and xonly, we should update the API names in this PR too.

examples/bip32.rs

apoelstra · 2021-09-20T14:53:12Z

src/util/bip32.rs

+    /// Constructs BIP340 keypair for Schnorr signatures and Taproot use matching the internal
+    /// secret key representation.
+    pub fn to_schnorr_keypair<C: secp256k1::Signing>(&self, secp: &Secp256k1<C>) -> schnorr::KeyPair {
+        schnorr::KeyPair::from_seckey_slice(secp, &self.private_key[..]).expect("BIP32 internal private key representation is broken")


Can you add a unit test which tries to parse an xpriv with a secret key of all zeros and one where the secret key is all ffs? I'm worried that this panic can be triggered.

Well, you can't construct xpriv with such secret keys - it errors during the construction on incorrect secret key. However I added general test case for this method.

GeneFerneau · 2021-09-22T23:06:51Z

utACK 7c565ec

Kixunil

ACK 1ff5730

sanket1729

Left two comments. I don't think we should wait for a point release of secp for a purpose of a test (which I think is not testing anything concrete). We can add the test later after a point release of secp.

sanket1729 · 2022-01-08T01:50:47Z

src/util/bip32.rs

+
+    /// Constructs BIP340 keypair for Schnorr signatures and Taproot use matching the internal
+    /// secret key representation.
+    pub fn to_schnorr_keypair<C: secp256k1::Signing>(&self, secp: &Secp256k1<C>) -> schnorr::KeyPair {


I missed this. This should also be to_keypair

sanket1729 · 2022-01-08T02:05:08Z

src/util/bip32.rs

+        let secp = Secp256k1::new();
+        let xpriv = ExtendedPrivKey::from_str("xprv9s21ZrQH143K3QTDL4LXw2F7HEK3wJUD2nW2nRk4stbPy6cq3jPPqjiChkVvvNKmPGJxWUtg6LnF5kejMRNNU3TGtRBeJgk33yuGBxrMPHi").unwrap();
+        let pair = KeyPair::from_seckey_slice(&secp, &xpriv.private_key[..]).unwrap();
+        assert_eq!(xpriv.to_schnorr_keypair(&secp), pair);


I don't see any value in this test? What is this test testing? It is a one-line function code that is repeated in the test.

If you feel there is value in this, we can check that the x-only public keys from the keypairs are the same for now. After another point release, we can re-add this test. I don't think we are losing any features for the next release because of this.

dr-orlovsky · 2022-01-08T11:58:17Z

@sanket1729 addressed issues and removed test.

@Kixunil pls re-ACK

Kixunil · 2022-01-08T14:18:21Z

Would've reacked but it's uncompilable...

dr-orlovsky · 2022-01-08T15:15:34Z

@Kixunil sorry, my bad: introduced last minute "improvement" with additional commit and didn't properly checked it. Should be fixed now

Kixunil · 2022-01-08T16:52:11Z

Still some issue.

dr-orlovsky · 2022-01-08T22:30:52Z

@Kixunil I do not know what the ... is happening with CI, but it perfectly builds locally while it fails with exactly the same config on GitHub:

orlovsky@alienware:~/repos/rust-bitcoin$ rustup update
info: syncing channel updates for 'stable-x86_64-unknown-linux-gnu'
info: syncing channel updates for 'nightly-x86_64-unknown-linux-gnu'
info: cleaning up downloads & tmp directories
orlovsky@alienware:~/repos/rust-bitcoin$ cargo +stable build --features=no-std --no-default-features
    Finished dev [unoptimized + debuginfo] target(s) in 0.27s
orlovsky@alienware:~/repos/rust-bitcoin$ cargo +nightly build --features=no-std --no-default-features
    Finished dev [unoptimized + debuginfo] target(s) in 0.26s

sanket1729 · 2022-01-09T01:26:24Z

@dr-orlovsky , the tests are not building. Not the binary.

sanket1729 · 2022-01-09T01:31:58Z

Looks like we are not deriving Debug in nostd. I was not involved in no-std and I do not understand embedded development. But what was the rationale for this?

#[derive(Copy, Clone, PartialEq, Eq)]
#[cfg_attr(feature = "std", derive(Debug))]
pub struct ExtendedPrivKey {

sanket1729 · 2022-01-09T01:39:27Z

src/util/bip32.rs

@@ -44,7 +45,8 @@ impl_array_newtype!(Fingerprint, u8, 4);
 impl_bytes_newtype!(Fingerprint, 4);

 /// Extended private key
-#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+#[derive(Copy, Clone, PartialEq, Eq)]
+#[cfg_attr(feature = "std", derive(Debug))]


Why are you doing this? I think this is causing the issue? Why cannot use derive debug in no-std? And how is that related to this PR.

We need this since SecretKey in the upstream does custom Debug which works only on std. Without this change it foes not compile on no_std

sanket1729 · 2022-01-09T01:42:36Z

I think we should leave all the secret display stuff out of this PR and this should build.

According to #588, BIP32 does not support uncompressed keys and using type with compression flag is a mistake

dr-orlovsky · 2022-01-09T06:18:25Z

@Kixunil @sanket1729 sorry it was my bad: probably was too tired yesterday and thought that I had no-std Debug impl for ExtendedPrivateKey. Fixed

apoelstra

ACK cf0c48c

It's a little eyebrow-raising that the nostd Debug impl just silently omits the secret key, but whatever. I have no opinions about the renaming, though I think after we get all the key-changing PRs in (but before we do the Taproot release) we should go through everything and possibly do a rename cleanup.

dr-orlovsky · 2022-01-09T16:53:00Z

@apoelstra there is not a lot of options for no_std Debug: in secp256k1 secret key do not have any Debug in no_std and is hashed in std. If we just plainly print secret key here we will abandon "never display secrets in logs" policy we introduced in secp256k1 with that hashing.

apoelstra · 2022-01-09T18:27:55Z

Right, I understand that we have no good options here. I think I might've stuck the literal string [secret] in there or something, but just eliding the field is fine.

sanket1729

ACK cf0c48c. #757 might need rework after this

sanket1729 · 2022-01-09T22:11:19Z

src/util/bip32.rs

@@ -638,6 +659,20 @@ impl ExtendedPubKey {
        }
    }

+    /// Constructs ECDSA compressed public key matching internal public key representation.
+    pub fn to_pub(&self) -> ecdsa::PublicKey {
+        ecdsa::PublicKey {


Note that there would be some conflicts with #757

…n ECDSA a6e8f58 PSBT BIP32 keys moved to Secp256k1 from bitcoin ECDSA (Dr Maxim Orlovsky) Pull request description: Fourth step in implementation of Schnorr key support after #588. This PR is a follow-up to non-API breaking #589 and API-breaking #590, which must be reviewed and merged first. ~~(The current PR includes all commits from #589 and #590, which should be reviewed there. The only commit specific to this PR is b8105e9)~~ UPDATE: All related PRs are merged now and this PR is ready for the review PR description: While PSBT BIP174 does not specify whether uncompressed keys are supported in BIP32-related fields, from BIP32 it follows that it is impossible to use uncompressed keys within the extended keys. This PR fixes this situation and is a companion to BIP174 PR clarifying key serialization: bitcoin/bips#1100 ACKs for top commit: apoelstra: ACK a6e8f58 sanket1729: ACK a6e8f58. Not sure which order to merge since there are many ready PRs which that would break each other. Tree-SHA512: 198ba646bbce1949b255a54a97957d952acdad8b7f9580be123116c0f44d773e6d90e0cac0d5993ec9a6b3328aa43aced0908522817861585877c50008fec835

dr-orlovsky added the API break This PR requires a version bump for the next release label Apr 12, 2021

dr-orlovsky added this to the 0.27.0 milestone Apr 12, 2021

dr-orlovsky added this to Ready for Review in Taproot Apr 12, 2021

dr-orlovsky mentioned this pull request Apr 12, 2021

PSBT BIP32 keys using to Secp256k1 keys instead of bitcoin ECDSA #591

Merged

dr-orlovsky changed the title ~~Taproot: switch BIP32 extended keys to Scep256k1 keys instead of bitcoin ECDSA~~ Taproot: BIP32 extended keys using Scep256k1 keys instead of bitcoin ECDSA Apr 12, 2021

dr-orlovsky mentioned this pull request Apr 12, 2021

Taproot keys implementation strategy #588

Closed

4 tasks

thomaseizinger reviewed Apr 13, 2021

View reviewed changes

src/util/bip32.rs Outdated Show resolved Hide resolved

src/util/bip32.rs Outdated Show resolved Hide resolved

dr-orlovsky mentioned this pull request May 14, 2021

Tracking issue for Taproot #503

Closed

20 tasks

sanket1729 reviewed Jun 28, 2021

View reviewed changes

src/util/bip32.rs Outdated Show resolved Hide resolved

sgeisler previously approved these changes Jun 29, 2021

View reviewed changes

dr-orlovsky dismissed sgeisler’s stale review via ba31bf0 August 1, 2021 18:58

Taproot automation moved this from Ready for Review to In review Aug 1, 2021

dr-orlovsky requested review from sanket1729, sgeisler and apoelstra August 1, 2021 19:05

sanket1729 reviewed Aug 1, 2021

View reviewed changes

sanket1729 previously approved these changes Sep 13, 2021

View reviewed changes

apoelstra reviewed Sep 20, 2021

View reviewed changes

examples/bip32.rs Outdated Show resolved Hide resolved

apoelstra reviewed Sep 20, 2021

View reviewed changes

Kixunil previously approved these changes Jan 7, 2022

View reviewed changes

Kixunil added the one ack PRs that have one ACK, so one more can progress them label Jan 7, 2022

sanket1729 reviewed Jan 8, 2022

View reviewed changes

dr-orlovsky dismissed Kixunil’s stale review via cb06042 January 8, 2022 11:57

sanket1729 reviewed Jan 9, 2022

View reviewed changes

dr-orlovsky added 4 commits January 9, 2022 07:16

BIP32 extended keys are using Scep256k1 keys instead of bitcoin ECDSA

b72f56c

According to #588, BIP32 does not support uncompressed keys and using type with compression flag is a mistake

BIP32 extended key to_ecdsa() and to_schnorr() methods

e6a3d60

Test for extended private key keypair generation f5875a

b65a6ae

Improve Debug for PrivateKey

cf0c48c

apoelstra approved these changes Jan 9, 2022

View reviewed changes

dr-orlovsky requested review from sanket1729 and Kixunil January 9, 2022 19:30

sanket1729 approved these changes Jan 9, 2022

View reviewed changes

Taproot automation moved this from In review to Ready for merge Jan 9, 2022

sanket1729 merged commit 476eed7 into rust-bitcoin:master Jan 9, 2022

Taproot automation moved this from Ready for merge to Done Jan 9, 2022

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

apoelstra mentioned this pull request Feb 24, 2022

BUG: Does not work with no_std under 1.29 (MSRV) #690

Merged

Taproot: BIP32 extended keys using Scep256k1 keys instead of bitcoin ECDSA #590

Taproot: BIP32 extended keys using Scep256k1 keys instead of bitcoin ECDSA #590

Conversation

dr-orlovsky commented Apr 12, 2021 • edited

thomaseizinger left a comment

Choose a reason for hiding this comment

dr-orlovsky commented Jun 27, 2021

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

sanket1729 Jun 28, 2021 • 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

sgeisler 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Jul 9, 2021

dr-orlovsky commented Aug 1, 2021

sanket1729 left a comment

Choose a reason for hiding this comment

dr-orlovsky commented Aug 9, 2021

sanket1729 commented Aug 9, 2021 • edited

dr-orlovsky commented Aug 9, 2021

sanket1729 commented Aug 13, 2021

dr-orlovsky commented Aug 22, 2021

sanket1729 commented Sep 7, 2021 • edited

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

GeneFerneau commented Sep 22, 2021

Kixunil left a comment

Choose a reason for hiding this comment

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

dr-orlovsky commented Jan 8, 2022

Kixunil commented Jan 8, 2022

dr-orlovsky commented Jan 8, 2022

Kixunil commented Jan 8, 2022

dr-orlovsky commented Jan 8, 2022

sanket1729 commented Jan 9, 2022

sanket1729 commented Jan 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 commented Jan 9, 2022

dr-orlovsky commented Jan 9, 2022

apoelstra left a comment

Choose a reason for hiding this comment

dr-orlovsky commented Jan 9, 2022

apoelstra commented Jan 9, 2022

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-orlovsky commented Apr 12, 2021 •

edited

sanket1729 Jun 28, 2021 •

edited

sanket1729 commented Aug 9, 2021 •

edited

sanket1729 commented Sep 7, 2021 •

edited

sanket1729 commented Jan 9, 2022 •

edited