From 765df8944dd22b5bfe1ddc5a56e5b7bcb1e3f3c7 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 18 Oct 2023 09:54:18 +1100 Subject: [PATCH 1/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 format - Don't link to hashes types --- src/lib.rs | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index eac67c5d1..c6d9bdbc0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -147,34 +147,31 @@ pub use crate::primitives::absolute_locktime::{AbsLockTime, AbsLockTimeError}; pub use crate::primitives::relative_locktime::{RelLockTime, RelLockTimeError}; pub use crate::primitives::threshold::{Threshold, ThresholdError}; -/// 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 SHA256 hash type used in the `sha256` fragment. 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 HASH256 hash type used in the `hash256` fragment. 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 RIPEMD256 hash type used in the `ripemd256` fragment. 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 HASH160 hash type used in the `hash160` fragment. type Hash160: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash; } From f93eca815c68dd6766b9842c7a4ae1369d9aeedc Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 18 Oct 2023 09:56:57 +1100 Subject: [PATCH 2/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 c6d9bdbc0..a121573b7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -183,13 +183,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 02cd5adf92325c45116f99dc55b6024e5f72e077 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 18 Oct 2023 09:59:18 +1100 Subject: [PATCH 3/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 a121573b7..693a33f8a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -201,7 +201,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 2d8c06fe0e0931ece4d0aac7b04606862ef19c51 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 18 Oct 2023 10:17:19 +1100 Subject: [PATCH 4/8] Improve rustdocs on ToPublicKey Clean up the `ToPublicKey` trait docs. Note this leaves the links to the hash types in there since they are the return type. --- src/lib.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 693a33f8a..a0a0c6b96 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -207,21 +207,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()), @@ -229,16 +229,16 @@ pub trait ToPublicKey: MiniscriptKey { } } - /// Converts the generic associated [`MiniscriptKey::Sha256`] to [`sha256::Hash`] + /// Converts the associated [`MiniscriptKey::Sha256`] type to a [`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`]. 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`]. 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`]. fn to_hash160(hash: &::Hash160) -> hash160::Hash; } From 438bbc5d80867a0b6b542f4b6ed5b723e951a7c9 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 18 Oct 2023 10:18:40 +1100 Subject: [PATCH 5/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 a0a0c6b96..1919d0cd5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -153,8 +153,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 8dc371b2a8836c64a9d7fb6fba41f89a90dddbe2 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Wed, 18 Oct 2023 10:25:48 +1100 Subject: [PATCH 6/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 reason about these trait methods. Remove default trait method implementations and manually implement the trait methods for all current implementors. --- fuzz/src/lib.rs | 3 +++ src/interpreter/mod.rs | 2 ++ src/lib.rs | 13 +++++++++++-- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/fuzz/src/lib.rs b/fuzz/src/lib.rs index f9cae61df..616dc2254 100644 --- a/fuzz/src/lib.rs +++ b/fuzz/src/lib.rs @@ -40,6 +40,9 @@ impl MiniscriptKey for FuzzPk { type Ripemd160 = u8; type Hash160 = u8; type Hash256 = u8; + + fn is_x_only_key(&self) -> bool { false } + fn num_der_paths(&self) -> usize { 0 } } impl ToPublicKey for FuzzPk { diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index 77ad18136..1d6ed9695 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 1919d0cd5..0c45363bc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -153,12 +153,12 @@ 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`). - fn is_x_only_key(&self) -> bool { false } + fn is_x_only_key(&self) -> bool; /// 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 } + fn num_der_paths(&self) -> usize; /// The SHA256 hash type used in the `sha256` fragment. type Sha256: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash; @@ -178,6 +178,9 @@ impl MiniscriptKey for bitcoin::secp256k1::PublicKey { type Hash256 = hash256::Hash; type Ripemd160 = ripemd160::Hash; type Hash160 = hash160::Hash; + + fn is_x_only_key(&self) -> bool { false } + fn num_der_paths(&self) -> usize { 0 } } impl MiniscriptKey for bitcoin::PublicKey { @@ -187,6 +190,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 { @@ -196,6 +201,7 @@ impl MiniscriptKey for bitcoin::secp256k1::XOnlyPublicKey { type Hash160 = hash160::Hash; fn is_x_only_key(&self) -> bool { true } + fn num_der_paths(&self) -> usize { 0 } } impl MiniscriptKey for String { @@ -203,6 +209,9 @@ impl MiniscriptKey for String { type Hash256 = String; type Ripemd160 = String; type Hash160 = String; + + 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 2968a102f6e8e2c7a7afc23fb83e22a8b45fa4bf Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 28 Mar 2024 11:56:45 +1100 Subject: [PATCH 7/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 0c45363bc..9f9715aa4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -149,17 +149,6 @@ pub use crate::primitives::threshold::{Threshold, ThresholdError}; /// 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 an x-only pubkey (defaults to `false`). - fn is_x_only_key(&self) -> bool; - - /// 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; - /// The SHA256 hash type used in the `sha256` fragment. type Sha256: Clone + Eq + Ord + fmt::Display + fmt::Debug + hash::Hash; @@ -171,6 +160,17 @@ pub trait MiniscriptKey: Clone + Eq + Ord + fmt::Debug + fmt::Display + hash::Ha /// The HASH160 hash 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 (defaults to `false`). + fn is_uncompressed(&self) -> bool { false } + + /// Returns true if the key is an x-only pubkey (defaults to `false`). + fn is_x_only_key(&self) -> bool; + + /// 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; } impl MiniscriptKey for bitcoin::secp256k1::PublicKey { From c3a02d25a9227f012de7ca77889abfad9497115e Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 17 Nov 2025 13:39:03 +1100 Subject: [PATCH 8/8] Remove whitespace Remove the whitespace between trivial trait method impls. This reduces line count without making the code any harder to read, arguably easier. --- src/lib.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 9f9715aa4..87714e103 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -253,11 +253,8 @@ impl ToPublicKey for bitcoin::PublicKey { fn to_public_key(&self) -> bitcoin::PublicKey { *self } fn to_sha256(hash: &sha256::Hash) -> sha256::Hash { *hash } - fn to_hash256(hash: &hash256::Hash) -> hash256::Hash { *hash } - fn to_ripemd160(hash: &ripemd160::Hash) -> ripemd160::Hash { *hash } - fn to_hash160(hash: &hash160::Hash) -> hash160::Hash { *hash } } @@ -265,11 +262,8 @@ impl ToPublicKey for bitcoin::secp256k1::PublicKey { fn to_public_key(&self) -> bitcoin::PublicKey { bitcoin::PublicKey::new(*self) } fn to_sha256(hash: &sha256::Hash) -> sha256::Hash { *hash } - fn to_hash256(hash: &hash256::Hash) -> hash256::Hash { *hash } - fn to_ripemd160(hash: &ripemd160::Hash) -> ripemd160::Hash { *hash } - fn to_hash160(hash: &hash160::Hash) -> hash160::Hash { *hash } } @@ -286,11 +280,8 @@ impl ToPublicKey for bitcoin::secp256k1::XOnlyPublicKey { fn to_x_only_pubkey(&self) -> bitcoin::secp256k1::XOnlyPublicKey { *self } fn to_sha256(hash: &sha256::Hash) -> sha256::Hash { *hash } - fn to_hash256(hash: &hash256::Hash) -> hash256::Hash { *hash } - fn to_ripemd160(hash: &ripemd160::Hash) -> ripemd160::Hash { *hash } - fn to_hash160(hash: &hash160::Hash) -> hash160::Hash { *hash } }