From 730c492b95ebcd865ea52e6184805416e953cd9a Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 18 Oct 2023 10:30:45 +1100 Subject: [PATCH 1/8] Manually implement is_uncompressed for BitcoinKey The `interpreter::BitcoinKey` uses the default implementation of `MiniscriptKey`, which means `is_uncompressed` returns `false`. However if the full key is a `bitcoin::PublicKey` it may be compressed. Manually implement `MiniscriptKey::is_uncompressed` for `BitcoinKey` and return the compressedness of the inner full key. --- src/interpreter/mod.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index 55ec97ed4..af405bda9 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -123,6 +123,13 @@ impl MiniscriptKey for BitcoinKey { type Hash256 = hash256::Hash; type Ripemd160 = ripemd160::Hash; type Hash160 = hash160::Hash; + + fn is_uncompressed(&self) -> bool { + match *self { + BitcoinKey::Fullkey(pk) => !pk.compressed, + BitcoinKey::XOnlyPublicKey(_) => false, + } + } } impl<'txin> Interpreter<'txin> { From f250438d29f2fe3654398e58bf982183da6f3722 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 18 Oct 2023 09:54:18 +1100 Subject: [PATCH 2/8] Improve rustdoc on the MiniscriptKey trait We want to support private keys in descriptors, in preparation improve the rustdocs on the `MiniscriptKey` trait by doing: - Use "key" instead of "pukbey". - Fix the links - Improve spacing, use header body foramt --- src/lib.rs | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 277e50662..f049c80cc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -153,34 +153,39 @@ use crate::prelude::*; pub use crate::primitives::absolute_locktime::{AbsLockTime, AbsLockTimeError}; pub use crate::primitives::relative_locktime::{RelLockTime, RelLockTimeError}; -/// Public key trait which can be converted to Hash type +/// Trait representing a key which can be converted to a hash type. pub trait MiniscriptKey: Clone + Eq + Ord + fmt::Debug + fmt::Display + hash::Hash { - /// Returns true if the pubkey is uncompressed. Defaults to `false`. + /// Returns true if the key is serialized uncompressed, defaults to `false`. fn is_uncompressed(&self) -> bool { false } - /// Returns true if the pubkey is an x-only pubkey. Defaults to `false`. + /// Returns true if the key is an x-only pubkey, defaults to `false`. // This is required to know what in DescriptorPublicKey to know whether the inner // key in allowed in descriptor context fn is_x_only_key(&self) -> bool { false } - /// Returns the number of different derivation paths in this key. Only >1 for keys - /// in BIP389 multipath descriptors. + /// Returns the number of different derivation paths in this key, defaults to `0`. + /// + /// Only >1 for keys in BIP389 multipath descriptors. fn num_der_paths(&self) -> usize { 0 } - /// The associated [`bitcoin::hashes::sha256::Hash`] for this [`MiniscriptKey`], used in the - /// sha256 fragment. + /// The associated [`sha256::Hash`] for this `MiniscriptKey`, used in the sha256 fragment. + /// + /// [`sha256::Hash`]: bitcoin::hashes::sha256::Hash type Sha256: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash; - /// The associated [`miniscript::hash256::Hash`] for this [`MiniscriptKey`], used in the - /// hash256 fragment. + /// The associated [`hash256::Hash`] for this `MiniscriptKey`, used in the hash256 fragment. + /// + /// [`hash256::Hash`]: crate::hash256::Hash type Hash256: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash; - /// The associated [`bitcoin::hashes::ripemd160::Hash`] for this [`MiniscriptKey`] type, used - /// in the ripemd160 fragment. + /// The associated [`ripemd160::Hash`] for this `MiniscriptKey` type, used in the ripemd160 fragment. + /// + /// [`ripemd160::Hash`]: bitcoin::hashes::ripemd160::Hash type Ripemd160: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash; - /// The associated [`bitcoin::hashes::hash160::Hash`] for this [`MiniscriptKey`] type, used in - /// the hash160 fragment. + /// The associated [`hash160::Hash`] for this `MiniscriptKey` type, used in the hash160 fragment. + /// + /// [`hash160::Hash`]: bitcoin::hashes::hash160::Hash type Hash160: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash; } From 94dbd84810c4bdfb46ca1de45ab39f367f6aa197 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 18 Oct 2023 09:56:57 +1100 Subject: [PATCH 3/8] Move trait method Clean up `MiniscriptKey` implementation for `bitcoin::PublicKey`. - Be uniform, put the trait method implementation below the associated types. - Trait methods have documentation on the trait, remove the unnecessary rustdoc on the implementation. --- src/lib.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index f049c80cc..1d3748bb6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -197,13 +197,12 @@ impl MiniscriptKey for bitcoin::secp256k1::PublicKey { } impl MiniscriptKey for bitcoin::PublicKey { - /// Returns the compressed-ness of the underlying secp256k1 key. - fn is_uncompressed(&self) -> bool { !self.compressed } - type Sha256 = sha256::Hash; type Hash256 = hash256::Hash; type Ripemd160 = ripemd160::Hash; type Hash160 = hash160::Hash; + + fn is_uncompressed(&self) -> bool { !self.compressed } } impl MiniscriptKey for bitcoin::secp256k1::XOnlyPublicKey { From 068ee83096743ae7ff59cd6d85c03b096fadf192 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 18 Oct 2023 09:59:18 +1100 Subject: [PATCH 4/8] Remove "what" comment We can see that the hashes are specified as strings, no need to comment this. --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 1d3748bb6..2c9da4a28 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -215,7 +215,7 @@ impl MiniscriptKey for bitcoin::secp256k1::XOnlyPublicKey { } impl MiniscriptKey for String { - type Sha256 = String; // specify hashes as string + type Sha256 = String; type Hash256 = String; type Ripemd160 = String; type Hash160 = String; From 166c09afc1183cc970e4eb8cc7e0bf3a076a44c0 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 18 Oct 2023 10:17:19 +1100 Subject: [PATCH 5/8] Improve rustdocs on primary traits The `MiniscriptKey` and `ToPublicKey` traits are more-or-less the first point of call for users of this library, lets clean them up. --- src/lib.rs | 46 +++++++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 2c9da4a28..517620d51 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -168,24 +168,16 @@ pub trait MiniscriptKey: Clone + Eq + Ord + fmt::Debug + fmt::Display + hash::Ha /// Only >1 for keys in BIP389 multipath descriptors. fn num_der_paths(&self) -> usize { 0 } - /// The associated [`sha256::Hash`] for this `MiniscriptKey`, used in the sha256 fragment. - /// - /// [`sha256::Hash`]: bitcoin::hashes::sha256::Hash + /// The type used in the sha256 fragment. type Sha256: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash; - /// The associated [`hash256::Hash`] for this `MiniscriptKey`, used in the hash256 fragment. - /// - /// [`hash256::Hash`]: crate::hash256::Hash + /// The type used in the hash256 fragment. type Hash256: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash; - /// The associated [`ripemd160::Hash`] for this `MiniscriptKey` type, used in the ripemd160 fragment. - /// - /// [`ripemd160::Hash`]: bitcoin::hashes::ripemd160::Hash + /// The type used in the ripemd160 fragment. type Ripemd160: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash; - /// The associated [`hash160::Hash`] for this `MiniscriptKey` type, used in the hash160 fragment. - /// - /// [`hash160::Hash`]: bitcoin::hashes::hash160::Hash + /// The type used in the hash160 fragment. type Hash160: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash; } @@ -221,21 +213,21 @@ impl MiniscriptKey for String { type Hash160 = String; } -/// Trait describing public key types which can be converted to bitcoin pubkeys +/// Trait describing key types that can be converted to bitcoin public keys. pub trait ToPublicKey: MiniscriptKey { - /// Converts an object to a public key + /// Converts key to a public key. fn to_public_key(&self) -> bitcoin::PublicKey; - /// Convert an object to x-only pubkey + /// Converts key to an x-only public key. fn to_x_only_pubkey(&self) -> bitcoin::secp256k1::XOnlyPublicKey { let pk = self.to_public_key(); bitcoin::secp256k1::XOnlyPublicKey::from(pk.inner) } - /// Obtain the public key hash for this MiniscriptKey - /// Expects an argument to specify the signature type. - /// This would determine whether to serialize the key as 32 byte x-only pubkey - /// or regular public key when computing the hash160 + /// Obtains the pubkey hash for this key (as a `MiniscriptKey`). + /// + /// Expects an argument to specify the signature type. This determines whether to serialize + /// the key as 32 byte x-only pubkey or regular public key when computing the hash160. fn to_pubkeyhash(&self, sig_type: SigType) -> hash160::Hash { match sig_type { SigType::Ecdsa => hash160::Hash::hash(&self.to_public_key().to_bytes()), @@ -243,16 +235,24 @@ pub trait ToPublicKey: MiniscriptKey { } } - /// Converts the generic associated [`MiniscriptKey::Sha256`] to [`sha256::Hash`] + /// Converts the associated [`MiniscriptKey::Sha256`] type to a [`sha256::Hash`]. + /// + /// [`sha256::Hash`]: bitcoin::hashes::sha256::Hash fn to_sha256(hash: &::Sha256) -> sha256::Hash; - /// Converts the generic associated [`MiniscriptKey::Hash256`] to [`hash256::Hash`] + /// Converts the associated [`MiniscriptKey::Hash256`] type to a [`hash256::Hash`]. + /// + /// [`hash256::Hash`]: crate::hash256::Hash fn to_hash256(hash: &::Hash256) -> hash256::Hash; - /// Converts the generic associated [`MiniscriptKey::Ripemd160`] to [`ripemd160::Hash`] + /// Converts the associated [`MiniscriptKey::Ripemd160`] type to a [`ripemd160::Hash`]. + /// + /// [`ripemd160::Hash`]: bitcoin::hashes::ripemd160::Hash fn to_ripemd160(hash: &::Ripemd160) -> ripemd160::Hash; - /// Converts the generic associated [`MiniscriptKey::Hash160`] to [`hash160::Hash`] + /// Converts the associated [`MiniscriptKey::Hash160`] type to a [`hash160::Hash`]. + /// + /// [`hash160::Hash`]: bitcoin::hashes::hash160::Hash fn to_hash160(hash: &::Hash160) -> hash160::Hash; } From ef1d28d25e4dfb3b4441af5df132172f4b882afd Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 18 Oct 2023 10:18:40 +1100 Subject: [PATCH 6/8] Remove code comment on is_x_only_key The `is_x_only_key` trait method is used for more than one thing, the code comment is either stale, not exhaustive, or wrong. Let's just remove it. --- src/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 517620d51..01472a2df 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -159,8 +159,6 @@ pub trait MiniscriptKey: Clone + Eq + Ord + fmt::Debug + fmt::Display + hash::Ha fn is_uncompressed(&self) -> bool { false } /// Returns true if the key is an x-only pubkey, defaults to `false`. - // This is required to know what in DescriptorPublicKey to know whether the inner - // key in allowed in descriptor context fn is_x_only_key(&self) -> bool { false } /// Returns the number of different derivation paths in this key, defaults to `0`. From bdddc391fa4e70a1a8792c951b149717cba00569 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 18 Oct 2023 10:25:48 +1100 Subject: [PATCH 7/8] Remove default trait method implementations Implementors of `MiniscriptKey` must reason about the three trait methods, this implies the trait methods are required, not provided. We are using the default impls to remove code duplication, this is an abuse of the default impls. It makes the docs read incorrectly, by implying that implementors _do not_ need to think reason about these trait methods. Remove default trait method implementations and manually implement the trait methods for all current implementors. --- src/interpreter/mod.rs | 2 ++ src/lib.rs | 24 ++++++++++++++++++------ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index af405bda9..462d9049e 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -130,6 +130,8 @@ impl MiniscriptKey for BitcoinKey { BitcoinKey::XOnlyPublicKey(_) => false, } } + fn is_x_only_key(&self) -> bool { false } + fn num_der_paths(&self) -> usize { 0 } } impl<'txin> Interpreter<'txin> { diff --git a/src/lib.rs b/src/lib.rs index 01472a2df..0a67f7e85 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -155,16 +155,16 @@ pub use crate::primitives::relative_locktime::{RelLockTime, RelLockTimeError}; /// Trait representing a key which can be converted to a hash type. pub trait MiniscriptKey: Clone + Eq + Ord + fmt::Debug + fmt::Display + hash::Hash { - /// Returns true if the key is serialized uncompressed, defaults to `false`. - fn is_uncompressed(&self) -> bool { false } + /// Returns true if the key is serialized uncompressed. + fn is_uncompressed(&self) -> bool; - /// Returns true if the key is an x-only pubkey, defaults to `false`. - fn is_x_only_key(&self) -> bool { false } + /// Returns true if the key is an x-only pubkey. + fn is_x_only_key(&self) -> bool; - /// Returns the number of different derivation paths in this key, defaults to `0`. + /// Returns the number of different derivation paths in this key. /// /// Only >1 for keys in BIP389 multipath descriptors. - fn num_der_paths(&self) -> usize { 0 } + fn num_der_paths(&self) -> usize; /// The type used in the sha256 fragment. type Sha256: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash; @@ -184,6 +184,10 @@ impl MiniscriptKey for bitcoin::secp256k1::PublicKey { type Hash256 = hash256::Hash; type Ripemd160 = ripemd160::Hash; type Hash160 = hash160::Hash; + + fn is_uncompressed(&self) -> bool { false } + fn is_x_only_key(&self) -> bool { false } + fn num_der_paths(&self) -> usize { 0 } } impl MiniscriptKey for bitcoin::PublicKey { @@ -193,6 +197,8 @@ impl MiniscriptKey for bitcoin::PublicKey { type Hash160 = hash160::Hash; fn is_uncompressed(&self) -> bool { !self.compressed } + fn is_x_only_key(&self) -> bool { false } + fn num_der_paths(&self) -> usize { 0 } } impl MiniscriptKey for bitcoin::secp256k1::XOnlyPublicKey { @@ -201,7 +207,9 @@ impl MiniscriptKey for bitcoin::secp256k1::XOnlyPublicKey { type Ripemd160 = ripemd160::Hash; type Hash160 = hash160::Hash; + fn is_uncompressed(&self) -> bool { false } fn is_x_only_key(&self) -> bool { true } + fn num_der_paths(&self) -> usize { 0 } } impl MiniscriptKey for String { @@ -209,6 +217,10 @@ impl MiniscriptKey for String { type Hash256 = String; type Ripemd160 = String; type Hash160 = String; + + fn is_uncompressed(&self) -> bool { false } + fn is_x_only_key(&self) -> bool { false } + fn num_der_paths(&self) -> usize { 0 } } /// Trait describing key types that can be converted to bitcoin public keys. From 133569a5131aa13ff9382ff855b318e97af7d4bf Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 28 Mar 2024 11:56:45 +1100 Subject: [PATCH 8/8] Move associated types to top of struct There is no obvious reason why the associated types for `MiniscriptKey` are below the trait methods. Move them to the top. Note, the diff shows moving functions not associated types - same thing. Refactor only, no logic changes. --- src/lib.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 0a67f7e85..90ef32e17 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -155,17 +155,6 @@ pub use crate::primitives::relative_locktime::{RelLockTime, RelLockTimeError}; /// Trait representing a key which can be converted to a hash type. pub trait MiniscriptKey: Clone + Eq + Ord + fmt::Debug + fmt::Display + hash::Hash { - /// Returns true if the key is serialized uncompressed. - fn is_uncompressed(&self) -> bool; - - /// Returns true if the key is an x-only pubkey. - fn is_x_only_key(&self) -> bool; - - /// Returns the number of different derivation paths in this key. - /// - /// Only >1 for keys in BIP389 multipath descriptors. - fn num_der_paths(&self) -> usize; - /// The type used in the sha256 fragment. type Sha256: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash; @@ -177,6 +166,17 @@ pub trait MiniscriptKey: Clone + Eq + Ord + fmt::Debug + fmt::Display + hash::Ha /// The type used in the hash160 fragment. type Hash160: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash; + + /// Returns true if the key is serialized uncompressed. + fn is_uncompressed(&self) -> bool; + + /// Returns true if the key is an x-only pubkey. + fn is_x_only_key(&self) -> bool; + + /// Returns the number of different derivation paths in this key. + /// + /// Only >1 for keys in BIP389 multipath descriptors. + fn num_der_paths(&self) -> usize; } impl MiniscriptKey for bitcoin::secp256k1::PublicKey {