Bitcoin keys initial implementation #1

Kixunil · 2022-07-04T16:44:29Z

This makes whole code to be added in a "PR" to enable code review by @dr-orlovsky

This reverts commit d8ab011.

dr-orlovsky

I think this is very elegant work!

I left a lot of comments, many of them are questions or stylistic improvements. There are however some suggestions on a better API and also on implementing more key arithmetics. I haven't mentioned in the comments also that I propose to add core::ops::Neg for all key types (doing modulo inversion for private keys and parity inversion for public`).

dr-orlovsky · 2022-07-06T09:07:12Z

bitcoin_keys/src/ecdsa/mod.rs

+/// Distinguishes compressed keys from uncompressed ones (runtime).
+///
+/// This is a more readable alternative to `bool`.
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]


Usual derives discussion :)

Why not Ord and Hash? Ord allows putting types into BTreeSet and as keys into BTreeMap; so it is not that much about types having "ordering" property but rather about types which can be deterministically serialized as a collection members.

Also I think it is worth having manual Default impl with defaulting to Compressed.

bitcoin_keys/src/ecdsa/mod.rs

dr-orlovsky · 2022-07-06T09:18:26Z

bitcoin_keys/Cargo.toml

+alloc = []
+
+[dependencies]
+secp256k1 = "0.23.0"


shouldn't we make it optional for apps which do not require key validity (like in Lightning where node keys are not always checked due to a performance considerations)? As we discussed in rust-bitcoin/rust-bitcoin#919

If you agree, probably it will be nice to have it as a follow-up PR, not to re-do this one

I believe the idea was to make secp256k1-sys optional? I'd absolutely make it optional here if that attempt fails.

Not sure I understand. Secp256k1 provides no functionality on top of secp256k1-sys and basically an API wrapper. Sys crate would never be optional inside secp256k1...

Hmm, I guess we got confused but TBH I really like the idea of trying it out here and if sys becomes optional we can move it.

bitcoin_keys/src/ecdsa/mod.rs

dr-orlovsky · 2022-07-06T10:12:22Z

bitcoin_keys/src/ecdsa/mod.rs

+    }
+}
+
+impl<K: PublicKey> Legacy<K> {


add_scalar_exp and mul_scalar are missed

dr-orlovsky · 2022-07-06T10:12:38Z

bitcoin_keys/src/ecdsa/mod.rs

+}
+
+impl<K: PrivateKey> Legacy<K> {
+    /// Computes a public key from this private key


add_scalar and mul_scalar are missed

Oh now I realized, do they make sense for pre-taproot keys? I think that's why I originally didn't add them.

Yes, they are required in BIP32 derivations on extended priv keys + whenever P2C commitments are happening.

dr-orlovsky · 2022-07-06T10:13:21Z

bitcoin_keys/src/ecdsa/mod.rs

+    ///
+    /// This is generally **not** presented to the user, just used to generate Bitcoin script.
+    #[inline]
+    pub fn serialize_public_key(&self) -> [u8; 33] {


In uncompressed serialization we have a type guarantees on the immutability of the serialized data. Here we do not.

I didn't implement mut stuff on SerializedPublicKey because I find mutating it useless but at the same time preventing mutation (not really, just making it harder) doesn't seem to provide much value either. Do you have a particular reason to do one or the other beyond "consistency" or do you think conistency is a good enough reason for more code?

I think we do not need mutability and I'd like to be consistent. Mutating public key data directly is a bad idea, so it would be nice to have type guarantees about immutability of the serialzied data.

Hmm, that'll be a pretty significant boilerplate. I guess some part of it can be deduplicated using a macro but still...

You can have just a single generic wrapper type for both cases, like pub struct SerializedKey<const LEN: usize>([u8; LEN])

We can't use const generics in 1.41.1 and even if we could, as_slice() would be quite different. Also I don't like the idea of exposing such thing to consumers.

dr-orlovsky · 2022-07-06T10:15:05Z

bitcoin_keys/src/ecdsa/mod.rs

+}
+
+impl<K: PublicKey> Compressed<K> {
+    /// Serializes the public key into bytes in compressed format.


add_scalar_exp and mul_scalar are missed

dr-orlovsky · 2022-07-06T10:15:17Z

bitcoin_keys/src/ecdsa/mod.rs

+}
+
+impl<K: PrivateKey> Compressed<K> {
+    /// Computes a public key from this private key


add_scalar and mul_scalar are missed

dr-orlovsky · 2022-07-06T11:44:29Z

I also did several issues in this repo to summarize main todos which can be done as separate PRs

Kixunil

I'm got a bit cold so not in a very productive state but was able to at least respond.

Kixunil · 2022-07-07T14:24:06Z

bitcoin_keys/Cargo.toml

+alloc = []
+
+[dependencies]
+secp256k1 = "0.23.0"


I believe the idea was to make secp256k1-sys optional? I'd absolutely make it optional here if that attempt fails.

bitcoin_keys/README.md

Kixunil · 2022-07-07T14:35:49Z

bitcoin_keys/src/ecdsa/mod.rs

@@ -0,0 +1,340 @@
+//! Types handling ECDSA public keys.


My understanding is that on lowest level they don't but we can still have type-level markers for what the keys are intended for. The main idea is to avoid confusing situations like deriving a different address and then not seeing incoming coins even though strictly it's not a loss. I actually first hand witnessed numerous similar cases where people sent BTC to LTC address and the recovery was annoying.

Not sure if pre_tr/tr is better naming and not in good mental state to think deeply about it but will when I get better.

bitcoin_keys/src/ecdsa/mod.rs

Kixunil · 2022-07-07T14:39:56Z

bitcoin_keys/src/ecdsa/mod.rs

+mod sealed {
+    use secp256k1::Secp256k1;
+
+    pub trait Key: Copy + Eq {}


TBH, I don't remember why exactly I added those two, I think for some convenience. This is an internal trait anyway, so shouldn't matter much.

Kixunil · 2022-07-07T15:27:18Z

bitcoin_keys/src/lib.rs

+/// Key pair that may be serialized as uncompressed, used in legacy addresses only.
+///
+/// You probably want to use this alias instead of explicitly writing out the type.
+pub type LegacyKeyPair = ecdsa::Legacy<secp256k1::KeyPair>;


Maybe I've read the docs wrong but I think signing multiple messages with key pair is faster than signing them with just private key.

Kixunil · 2022-07-07T15:28:30Z

bitcoin_keys/src/schnorr.rs

@@ -0,0 +1,69 @@
+//! Keys intended to be used in Schnorr sinatures - in P2TR.


Oh, yeah, bip340 sounds like a better name than schnorr.

Kixunil · 2022-07-07T15:30:13Z

bitcoin_keys/src/schnorr.rs

+/// Private key intended for schnorr signatures.
+///
+/// This type wraps [`secp256k1::SecretKey`] to prevent accidental use in ECDSA signatures.
+/// It is mostly used to sign P2TR spends or derive P2TR addresses.


I think the API of ExtendedPrivKey should eventually change to directly support known standards including P2TR.

Kixunil · 2022-07-07T15:38:02Z

bitcoin_keys/src/schnorr.rs

+///
+/// This type wraps [`secp256k1::SecretKey`] to prevent accidental use in ECDSA signatures.
+/// It is mostly used to sign P2TR spends or derive P2TR addresses.
+#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]


Note: rust-bitcoin/rust-secp256k1#471

Kixunil · 2022-07-07T15:43:22Z

bitcoin_keys/src/schnorr.rs

+        secp256k1::PublicKey::from_secret_key(context, &self.key).into()
+    }
+
+    pub fn add_tweak(&self, tweak: &Scalar) -> Result<Self, secp256k1::Error> {


2 I would prefer to improve error upstream
4 Unfortunately it doesn't work that way :( We could #[must_use] whole type but it could be annoying if people compute it for side effects.

dr-orlovsky

I wish you get well soon! Do not hurry up, take your time. I have responded to your comments.

Regarding implementing all arithm functions, I wrote this: #3

dr-orlovsky · 2022-07-07T17:58:56Z

bitcoin_keys/Cargo.toml

+alloc = []
+
+[dependencies]
+secp256k1 = "0.23.0"


Not sure I understand. Secp256k1 provides no functionality on top of secp256k1-sys and basically an API wrapper. Sys crate would never be optional inside secp256k1...

bitcoin_keys/README.md

dr-orlovsky · 2022-07-07T18:01:25Z

bitcoin_keys/src/ecdsa/mod.rs

+mod sealed {
+    use secp256k1::Secp256k1;
+
+    pub trait Key: Copy + Eq {}


The trait is internal, but Copy+Eq requirement got externalized via outer pub trait Key: sealed::Key.

dr-orlovsky · 2022-07-07T18:02:44Z

bitcoin_keys/src/ecdsa/mod.rs

+        fn private_key(&self) -> secp256k1::SecretKey;
+
+        #[inline]
+        fn compute_public_key<C: secp256k1::Signing>(&self, context: &Secp256k1<C>) -> secp256k1::PublicKey {


I lost a track on the status of static context in secp lib. However indeed it is the stuff that hugely pollutes all APIs

dr-orlovsky · 2022-07-07T18:04:12Z

bitcoin_keys/src/ecdsa/mod.rs

+    /// **Warning:** make sure to supply the correct key format. Incorrect format may lead to a
+    /// different address making spending difficult or even impossible for non-technical people.
+    #[inline]
+    pub fn from_raw(key: K, format: KeyFormat) -> Self {


As I explained below, we should prevent constructing uncompressed keys as much as possible. So I rather will have a one-argument constructor for compressed key and some ugly-named function for uncompressed.

dr-orlovsky · 2022-07-07T18:14:26Z

bitcoin_keys/src/schnorr.rs

+/// Private key intended for schnorr signatures.
+///
+/// This type wraps [`secp256k1::SecretKey`] to prevent accidental use in ECDSA signatures.
+/// It is mostly used to sign P2TR spends or derive P2TR addresses.


Nope, I talked to Sipa on that and he confirmed multiple times that taproot must not affect BIP32-related APIs and they should remain working with full pub keys (not xonly)

dr-orlovsky · 2022-07-07T18:15:42Z

bitcoin_keys/src/schnorr.rs

+///
+/// This type wraps [`secp256k1::SecretKey`] to prevent accidental use in ECDSA signatures.
+/// It is mostly used to sign P2TR spends or derive P2TR addresses.
+#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]


not sure I understand this non-constant time story. But I clearly would like to have them in a BTreeSet in deterministic order, for instance for serialization

dr-orlovsky · 2022-07-07T18:19:06Z

bitcoin_keys/src/schnorr.rs

+        secp256k1::PublicKey::from_secret_key(context, &self.key).into()
+    }
+
+    pub fn add_tweak(&self, tweak: &Scalar) -> Result<Self, secp256k1::Error> {


2 I would prefer to improve error upstream
will take ages and we just have a very specific error conditions here (upstream the Error must be split into multiple types IMO)

Unfortunately it doesn't work that way :(

I see :(

dr-orlovsky · 2022-07-07T18:21:09Z

bitcoin_keys/src/ecdsa/mod.rs

@@ -0,0 +1,340 @@
+//! Types handling ECDSA public keys.


As we discussed below in https://github.com/BP-WG/bp/pull/1/files#r916009576, probably bip340 and legacy would be better than schnorr and ecdsa.

bitcoin_keys/src/ecdsa/mod.rs

Kixunil · 2022-07-07T20:39:31Z

bitcoin_keys/Cargo.toml

+alloc = []
+
+[dependencies]
+secp256k1 = "0.23.0"


Hmm, I guess we got confused but TBH I really like the idea of trying it out here and if sys becomes optional we can move it.

bitcoin_keys/src/ecdsa/mod.rs

Kixunil · 2022-07-07T20:43:03Z

bitcoin_keys/src/ecdsa/mod.rs

+    /// **Warning:** make sure to supply the correct key format. Incorrect format may lead to a
+    /// different address making spending difficult or even impossible for non-technical people.
+    #[inline]
+    pub fn from_raw(key: K, format: KeyFormat) -> Self {


Maybe from_raw(Key) and from_raw_with_format(Key, Format)?

Kixunil · 2022-07-07T20:45:48Z

bitcoin_keys/src/ecdsa/mod.rs

+    ///
+    /// This is generally **not** presented to the user, just used to generate Bitcoin script.
+    #[inline]
+    pub fn serialize_public_key(&self) -> [u8; 33] {


Hmm, that'll be a pretty significant boilerplate. I guess some part of it can be deduplicated using a macro but still...

bitcoin_keys/src/ecdsa/serialized_public_key.rs

Kixunil · 2022-07-07T20:50:05Z

bitcoin_keys/src/ecdsa/serialized_public_key.rs

+    /// Returns the serialized bytes as a slice.
+    ///
+    /// The length of the returned slice will be either 33 or 65, depending on the format of the
+    /// kye this was created from.


Note to self: s/kye/key/

Kixunil · 2022-07-07T20:56:19Z

bitcoin_keys/src/schnorr.rs

+/// Private key intended for schnorr signatures.
+///
+/// This type wraps [`secp256k1::SecretKey`] to prevent accidental use in ECDSA signatures.
+/// It is mostly used to sign P2TR spends or derive P2TR addresses.


There's still a separate purpose value defined for P2TR (don't remember the value). I was thinking something like:

impl ExtendedPrivKey { pub fn derive_raw(&self, path: Path) -> secp256k1::SecretKey; // AFAIR uncompressed aren't supported by BIP32 // purpose must be omitted from path pub fn derive_pre_segwit(&self, path: Path) -> bitcoin_keys:CompressedPrivateKey; pub fn derive_p2tr(&self, path: Path) -> bitcoin_keys::Bip340PrivateKey; }

Kixunil · 2022-07-07T20:57:51Z

bitcoin_keys/src/schnorr.rs

+///
+/// This type wraps [`secp256k1::SecretKey`] to prevent accidental use in ECDSA signatures.
+/// It is mostly used to sign P2TR spends or derive P2TR addresses.
+#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]


If you compare keys you may leak them by timing information. The code should be changed to avoid that.

Kixunil · 2022-07-07T20:58:47Z

bitcoin_keys/src/schnorr.rs

+        secp256k1::PublicKey::from_secret_key(context, &self.key).into()
+    }
+
+    pub fn add_tweak(&self, tweak: &Scalar) -> Result<Self, secp256k1::Error> {


Will try to send a PR.

Restore bitcoin_keys to enable code review"

998dc77

This reverts commit d8ab011.

dr-orlovsky reviewed Jul 6, 2022

View reviewed changes

dr-orlovsky assigned Kixunil Jul 6, 2022

dr-orlovsky changed the title ~~PR to enable whole-crate code review~~ Bitcoin keys initial implementation Jul 6, 2022

Kixunil commented Jul 7, 2022

View reviewed changes

dr-orlovsky reviewed Jul 7, 2022

View reviewed changes

Kixunil commented Jul 7, 2022

View reviewed changes

		@@ -0,0 +1,69 @@
		//! Keys intended to be used in Schnorr sinatures - in P2TR.

Bitcoin keys initial implementation #1

Are you sure you want to change the base?

Bitcoin keys initial implementation #1

Conversation

Kixunil commented Jul 4, 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

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

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

dr-orlovsky commented Jul 6, 2022

Kixunil 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment