From 7b3b3a8f953c9013e7ba9ae735b24e6345f6df53 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sat, 5 Jul 2025 17:52:34 +0000 Subject: [PATCH 1/8] key: fix DefiniteDescriptorKey construction from key with hardened step Our current API allows constructing a DefiniteDescriptorKey with hardened steps, which means that you can't actually derive a public key. However, the point of this type is to provide a DescriptorPublicKey which implements the ToPublicKey trait. The next commits will change the API to reflect this, but this commit is on its own to make it easier for me to backport it. --- src/descriptor/key.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/descriptor/key.rs b/src/descriptor/key.rs index 30fba53c0..af1ad2e34 100644 --- a/src/descriptor/key.rs +++ b/src/descriptor/key.rs @@ -334,6 +334,7 @@ impl DescriptorMultiXKey { pub enum NonDefiniteKeyError { Wildcard, Multipath, + HardenedStep, } impl fmt::Display for NonDefiniteKeyError { @@ -341,6 +342,9 @@ impl fmt::Display for NonDefiniteKeyError { match *self { Self::Wildcard => f.write_str("key with a wildcard cannot be a DerivedDescriptorKey"), Self::Multipath => f.write_str("multipath key cannot be a DerivedDescriptorKey"), + Self::HardenedStep => { + f.write_str("key with hardened derivation steps cannot be a DerivedDescriptorKey") + } } } } @@ -819,6 +823,23 @@ impl DescriptorPublicKey { } } + /// Whether or not the key has a wildcard + pub fn has_hardened_step(&self) -> bool { + let paths = match self { + DescriptorPublicKey::Single(..) => &[], + DescriptorPublicKey::XPub(xpub) => core::slice::from_ref(&xpub.derivation_path), + DescriptorPublicKey::MultiXPub(xpub) => &xpub.derivation_paths.paths()[..], + }; + for p in paths { + for step in p.into_iter() { + if step.is_hardened() { + return true; + } + } + } + false + } + #[deprecated(note = "use at_derivation_index instead")] /// Deprecated name for [`Self::at_derivation_index`]. pub fn derive(self, index: u32) -> Result { @@ -1276,6 +1297,8 @@ impl DefiniteDescriptorKey { pub fn new(key: DescriptorPublicKey) -> Result { if key.has_wildcard() { Err(NonDefiniteKeyError::Wildcard) + } else if key.has_hardened_step() { + Err(NonDefiniteKeyError::HardenedStep) } else if key.is_multipath() { Err(NonDefiniteKeyError::Multipath) } else { @@ -1785,5 +1808,13 @@ mod test { .parse::() .unwrap(); assert!(matches!(DefiniteDescriptorKey::new(desc), Err(NonDefiniteKeyError::Multipath))); + // xpub with hardened path + let desc = "xpub661MyMwAqRbcFtXgS5sYJABqqG9YLmC4Q1Rdap9gSE8NqtwybGhePY2gZ29ESFjqJoCu1Rupje8YtGqsefD265TMg7usUDFdp6W1EGMcet8/1'/2" + .parse::() + .unwrap(); + assert!(matches!( + DefiniteDescriptorKey::new(desc), + Err(NonDefiniteKeyError::HardenedStep) + )); } } From 77ed78b86941426d2854a9994d64eeaf158f25e8 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sat, 5 Jul 2025 19:13:49 +0000 Subject: [PATCH 2/8] key.rs: remove some deprecated methods These have been deprecated since 2ad655518a1899c402d75f34cfc51cdd13cc851d which made it into the 9.x release. Fine to delete them. --- src/descriptor/key.rs | 10 ---------- src/descriptor/mod.rs | 10 ---------- 2 files changed, 20 deletions(-) diff --git a/src/descriptor/key.rs b/src/descriptor/key.rs index af1ad2e34..43701d419 100644 --- a/src/descriptor/key.rs +++ b/src/descriptor/key.rs @@ -810,10 +810,6 @@ impl DescriptorPublicKey { } } - /// Whether or not the key has a wildcard - #[deprecated(note = "use has_wildcard instead")] - pub fn is_deriveable(&self) -> bool { self.has_wildcard() } - /// Whether or not the key has a wildcard pub fn has_wildcard(&self) -> bool { match *self { @@ -840,12 +836,6 @@ impl DescriptorPublicKey { false } - #[deprecated(note = "use at_derivation_index instead")] - /// Deprecated name for [`Self::at_derivation_index`]. - pub fn derive(self, index: u32) -> Result { - self.at_derivation_index(index) - } - /// Replaces any wildcard (i.e. `/*`) in the key with a particular derivation index, turning it into a /// *definite* key (i.e. one where all the derivation paths are set). /// diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index 4c4b8c48e..93754f182 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -648,10 +648,6 @@ impl ForEachKey for Descriptor { } impl Descriptor { - /// Whether or not the descriptor has any wildcards - #[deprecated(note = "use has_wildcards instead")] - pub fn is_deriveable(&self) -> bool { self.has_wildcard() } - /// Whether or not the descriptor has any wildcards i.e. `/*`. pub fn has_wildcard(&self) -> bool { self.for_any_key(|key| key.has_wildcard()) } @@ -684,12 +680,6 @@ impl Descriptor { .map_err(|e| e.expect_translator_err("No Context errors while translating")) } - #[deprecated(note = "use at_derivation_index instead")] - /// Deprecated name for [`Self::at_derivation_index`]. - pub fn derive(&self, index: u32) -> Result, ConversionError> { - self.at_derivation_index(index) - } - /// Convert all the public keys in the descriptor to [`bitcoin::PublicKey`] by deriving them or /// otherwise converting them. All [`bitcoin::secp256k1::XOnlyPublicKey`]s are converted to by adding a /// default(0x02) y-coordinate. From e1f08025be9d4fd721c735a57644614406a57089 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Wed, 21 May 2025 12:46:44 +0000 Subject: [PATCH 3/8] macros: make target pk and error type optional for translate_* macros There is no reason callers need to provide these types explicitly. Allow them to do so anyway for backward compatibility. (This commit I pulled out of my mega-refactor branch, but it seemed useful here, so I'm pulling it in now.) --- src/descriptor/mod.rs | 6 +++--- src/interpreter/inner.rs | 4 ++-- src/policy/semantic.rs | 2 +- src/psbt/mod.rs | 2 +- src/pub_macros.rs | 12 ++++++++++++ 5 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index 93754f182..2150289c0 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -674,7 +674,7 @@ impl Descriptor { pk.clone().at_derivation_index(self.0) } - translate_hash_clone!(DescriptorPublicKey, DescriptorPublicKey, ConversionError); + translate_hash_clone!(DescriptorPublicKey); } self.translate_pk(&mut Derivator(index)) .map_err(|e| e.expect_translator_err("No Context errors while translating")) @@ -918,7 +918,7 @@ impl Descriptor { .ok_or(Error::MultipathDescLenMismatch), } } - translate_hash_clone!(DescriptorPublicKey, DescriptorPublicKey, Error); + translate_hash_clone!(DescriptorPublicKey); } for (i, desc) in descriptors.iter_mut().enumerate() { @@ -972,7 +972,7 @@ impl Descriptor { pk.derive_public_key(self.0) } - translate_hash_clone!(DefiniteDescriptorKey, bitcoin::PublicKey, ConversionError); + translate_hash_clone!(DefiniteDescriptorKey); } let derived = self.translate_pk(&mut Derivator(secp)); diff --git a/src/interpreter/inner.rs b/src/interpreter/inner.rs index 71097da4f..f3b7ea099 100644 --- a/src/interpreter/inner.rs +++ b/src/interpreter/inner.rs @@ -364,7 +364,7 @@ impl ToNoChecks for Miniscript { Ok(BitcoinKey::Fullkey(*pk)) } - translate_hash_clone!(bitcoin::PublicKey, BitcoinKey, Self::Error); + translate_hash_clone!(bitcoin::PublicKey); } self.translate_pk_ctx(&mut TranslateFullPk) @@ -384,7 +384,7 @@ impl ToNoChecks for Miniscript Policy { /// /// // Handy macro for failing if we encounter any other fragment. /// // See also [`translate_hash_clone!`] for cloning instead of failing. - /// translate_hash_fail!(String, bitcoin::PublicKey, ()); + /// translate_hash_fail!(String); /// } /// /// let mut pk_map = HashMap::new(); diff --git a/src/psbt/mod.rs b/src/psbt/mod.rs index 6e7aff05e..63ea4bd7e 100644 --- a/src/psbt/mod.rs +++ b/src/psbt/mod.rs @@ -999,7 +999,7 @@ impl Translator for KeySourceLookUp { Ok(derived) } - translate_hash_clone!(DescriptorPublicKey, bitcoin::PublicKey, descriptor::ConversionError); + translate_hash_clone!(DescriptorPublicKey); } // Provides generalized access to PSBT fields common to inputs and outputs diff --git a/src/pub_macros.rs b/src/pub_macros.rs index 538d37f3c..f0d80e873 100644 --- a/src/pub_macros.rs +++ b/src/pub_macros.rs @@ -47,6 +47,12 @@ /// ``` #[macro_export] macro_rules! translate_hash_fail { + ($source: ty) => { + // The target and error type must always be these values; but we let + // the user specify them explicitly for compatibility with the version + // of this macro that existed before 13.x. + translate_hash_fail!($source, Self::TargetPk, Self::Error); + }; ($source: ty, $target:ty, $error_ty: ty) => { fn sha256( &mut self, @@ -88,6 +94,12 @@ macro_rules! translate_hash_fail { /// See also [`crate::translate_hash_fail`] #[macro_export] macro_rules! translate_hash_clone { + ($source: ty) => { + // The target and error type must always be these values; but we let + // the user specify them explicitly for compatibility with the version + // of this macro that existed before 13.x. + translate_hash_clone!($source, Self::TargetPk, Self::Error); + }; ($source: ty, $target:ty, $error_ty: ty) => { fn sha256( &mut self, From 6267818faae8975a587fbf571e96bfa05b1b6c27 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sat, 5 Jul 2025 19:31:54 +0000 Subject: [PATCH 4/8] TranslateErr: add helper function to unwrap impossible case --- src/lib.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index 8cecd0592..9dbdbae6d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -365,6 +365,20 @@ impl TranslateErr { } } +impl TranslateErr { + /// Remove the impossible "translator error" case and return a context error. + /// + /// When the translator error type is [`core::convert::Infallible`], which is + /// impossible to construct, allows unwrapping the outer error without any + /// panic paths. + pub fn into_outer_err(self) -> Error { + match self { + Self::TranslatorErr(impossible) => match impossible {}, + Self::OuterError(e) => e, + } + } +} + impl TranslateErr { /// If we are doing a translation where our "outer error" is the generic /// Miniscript error, eliminate the `TranslateErr` type which is just noise. From c169af02362df4fc87cd61fd4dd12b95f121009c Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sat, 5 Jul 2025 19:16:20 +0000 Subject: [PATCH 5/8] key.rs: remove a bunch of unwrap()s which are impossible Now that we are consistent about the rules that a DerivedDescriptor must obey, we no longer return a Result from derived_key(), which in turn eliminates a bunch of other unwraps. --- bitcoind-tests/tests/test_cpp.rs | 2 +- bitcoind-tests/tests/test_desc.rs | 2 +- examples/xpub_descriptors.rs | 1 - src/descriptor/key.rs | 19 ++++++++----------- src/descriptor/mod.rs | 25 +++++++++++-------------- src/psbt/mod.rs | 14 ++++++-------- 6 files changed, 27 insertions(+), 36 deletions(-) diff --git a/bitcoind-tests/tests/test_cpp.rs b/bitcoind-tests/tests/test_cpp.rs index 2836212e2..cdc336b2f 100644 --- a/bitcoind-tests/tests/test_cpp.rs +++ b/bitcoind-tests/tests/test_cpp.rs @@ -145,7 +145,7 @@ pub fn test_from_cpp_ms(cl: &Client, testdata: &TestData) { // Sign the transactions with all keys // AKA the signer role of psbt for i in 0..psbts.len() { - let wsh_derived = desc_vec[i].derived_descriptor(&secp).unwrap(); + let wsh_derived = desc_vec[i].derived_descriptor(&secp); let ms = if let Descriptor::Wsh(wsh) = &wsh_derived { match wsh.as_inner() { miniscript::descriptor::WshInner::Ms(ms) => ms, diff --git a/bitcoind-tests/tests/test_desc.rs b/bitcoind-tests/tests/test_desc.rs index 083ae44ec..e09260097 100644 --- a/bitcoind-tests/tests/test_desc.rs +++ b/bitcoind-tests/tests/test_desc.rs @@ -89,7 +89,7 @@ pub fn test_desc_satisfy( .at_derivation_index(0) .unwrap(); - let derived_desc = definite_desc.derived_descriptor(&secp).unwrap(); + let derived_desc = definite_desc.derived_descriptor(&secp); let desc_address = derived_desc.address(bitcoin::Network::Regtest); let desc_address = desc_address.map_err(|_x| DescError::AddressComputationError)?; diff --git a/examples/xpub_descriptors.rs b/examples/xpub_descriptors.rs index 90b0458f3..2f9478ff8 100644 --- a/examples/xpub_descriptors.rs +++ b/examples/xpub_descriptors.rs @@ -31,7 +31,6 @@ fn p2wsh(secp: &Secp256k1) -> Address { let address = Descriptor::::from_str(&s) .unwrap() .derived_descriptor(secp) - .unwrap() .address(Network::Bitcoin) .unwrap(); diff --git a/src/descriptor/key.rs b/src/descriptor/key.rs index 43701d419..40c4d50b4 100644 --- a/src/descriptor/key.rs +++ b/src/descriptor/key.rs @@ -1254,29 +1254,26 @@ impl DefiniteDescriptorKey { /// /// Will return an error if the descriptor key has any hardened derivation steps in its path. To /// avoid this error you should replace any such public keys first with [`crate::Descriptor::translate_pk`]. - pub fn derive_public_key( - &self, - secp: &Secp256k1, - ) -> Result { + pub fn derive_public_key(&self, secp: &Secp256k1) -> bitcoin::PublicKey { match self.0 { DescriptorPublicKey::Single(ref pk) => match pk.key { - SinglePubKey::FullKey(pk) => Ok(pk), - SinglePubKey::XOnly(xpk) => Ok(xpk.to_public_key()), + SinglePubKey::FullKey(pk) => pk, + SinglePubKey::XOnly(xpk) => xpk.to_public_key(), }, DescriptorPublicKey::XPub(ref xpk) => match xpk.wildcard { Wildcard::Unhardened | Wildcard::Hardened => { - unreachable!("we've excluded this error case") + unreachable!("impossible by construction of DefiniteDescriptorKey") } Wildcard::None => match xpk.xkey.derive_pub(secp, &xpk.derivation_path.as_ref()) { - Ok(xpub) => Ok(bitcoin::PublicKey::new(xpub.public_key)), + Ok(xpub) => bitcoin::PublicKey::new(xpub.public_key), Err(bip32::Error::CannotDeriveFromHardenedKey) => { - Err(ConversionError::HardenedChild) + unreachable!("impossible by construction of DefiniteDescriptorKey") } Err(e) => unreachable!("cryptographically unreachable: {}", e), }, }, DescriptorPublicKey::MultiXPub(_) => { - unreachable!("A definite key cannot contain a multipath key.") + unreachable!("impossible by construction of DefiniteDescriptorKey") } } } @@ -1346,7 +1343,7 @@ impl MiniscriptKey for DefiniteDescriptorKey { impl ToPublicKey for DefiniteDescriptorKey { fn to_public_key(&self) -> bitcoin::PublicKey { let secp = Secp256k1::verification_only(); - self.derive_public_key(&secp).unwrap() + self.derive_public_key(&secp) } fn to_sha256(hash: &sha256::Hash) -> sha256::Hash { *hash } diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index 2150289c0..aae995ccd 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -693,7 +693,7 @@ impl Descriptor { /// .expect("Valid ranged descriptor"); /// # let index = 42; /// # let secp = Secp256k1::verification_only(); - /// let derived_descriptor = descriptor.at_derivation_index(index).unwrap().derived_descriptor(&secp).unwrap(); + /// let derived_descriptor = descriptor.at_derivation_index(index).unwrap().derived_descriptor(&secp); /// # assert_eq!(descriptor.derived_descriptor(&secp, index).unwrap(), derived_descriptor); /// ``` /// @@ -712,7 +712,7 @@ impl Descriptor { secp: &secp256k1::Secp256k1, index: u32, ) -> Result, ConversionError> { - self.at_derivation_index(index)?.derived_descriptor(secp) + Ok(self.at_derivation_index(index)?.derived_descriptor(secp)) } /// Parse a descriptor that may contain secret keys @@ -948,7 +948,7 @@ impl Descriptor { /// let secp = secp256k1::Secp256k1::verification_only(); /// let descriptor = Descriptor::::from_str("tr(xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ/0/*)") /// .expect("Valid ranged descriptor"); - /// let result = descriptor.at_derivation_index(0).unwrap().derived_descriptor(&secp).expect("Non-hardened derivation"); + /// let result = descriptor.at_derivation_index(0).unwrap().derived_descriptor(&secp); /// assert_eq!(result.to_string(), "tr(03cc8a4bc64d897bddc5fbc2f670f7a8ba0b386779106cf1223c6fc5d7cd6fc115)#6qm9h8ym"); /// ``` /// @@ -958,18 +958,15 @@ impl Descriptor { pub fn derived_descriptor( &self, secp: &secp256k1::Secp256k1, - ) -> Result, ConversionError> { + ) -> Descriptor { struct Derivator<'a, C: secp256k1::Verification>(&'a secp256k1::Secp256k1); impl Translator for Derivator<'_, C> { type TargetPk = bitcoin::PublicKey; - type Error = ConversionError; + type Error = core::convert::Infallible; - fn pk( - &mut self, - pk: &DefiniteDescriptorKey, - ) -> Result { - pk.derive_public_key(self.0) + fn pk(&mut self, pk: &DefiniteDescriptorKey) -> Result { + Ok(pk.derive_public_key(self.0)) } translate_hash_clone!(DefiniteDescriptorKey); @@ -977,8 +974,10 @@ impl Descriptor { let derived = self.translate_pk(&mut Derivator(secp)); match derived { - Ok(derived) => Ok(derived), - Err(e) => Err(e.expect_translator_err("No Context errors when deriving keys")), + Ok(derived) => derived, + // Impossible to hit, since deriving keys does not change any Miniscript-relevant + // properties of the key. + Err(e) => panic!("Context errors when deriving keys: {}", e.into_outer_err()), } } } @@ -1825,14 +1824,12 @@ mod tests { .at_derivation_index(index) .unwrap() .derived_descriptor(&secp_ctx) - .unwrap() .address(bitcoin::Network::Bitcoin) .unwrap(); let addr_two = desc_two .at_derivation_index(index) .unwrap() .derived_descriptor(&secp_ctx) - .unwrap() .address(bitcoin::Network::Bitcoin) .unwrap(); let addr_expected = bitcoin::Address::from_str(raw_addr_expected) diff --git a/src/psbt/mod.rs b/src/psbt/mod.rs index 63ea4bd7e..bccdeeb6d 100644 --- a/src/psbt/mod.rs +++ b/src/psbt/mod.rs @@ -981,19 +981,16 @@ struct KeySourceLookUp( impl Translator for KeySourceLookUp { type TargetPk = bitcoin::PublicKey; - type Error = descriptor::ConversionError; + type Error = core::convert::Infallible; - fn pk( - &mut self, - xpk: &DefiniteDescriptorKey, - ) -> Result { - let derived = xpk.derive_public_key(&self.1)?; + fn pk(&mut self, xpk: &DefiniteDescriptorKey) -> Result { + let derived = xpk.derive_public_key(&self.1); self.0.insert( derived.to_public_key().inner, ( xpk.master_fingerprint(), xpk.full_derivation_path() - .ok_or(descriptor::ConversionError::MultiKey)?, + .expect("definite keys cannot be multikeys"), ), ); Ok(derived) @@ -1097,7 +1094,8 @@ fn update_item_with_descriptor_helper( let mut bip32_derivation = KeySourceLookUp(BTreeMap::new(), secp); let derived = descriptor .translate_pk(&mut bip32_derivation) - .map_err(|e| e.expect_translator_err("No Outer Context errors in translations"))?; + .map_err(crate::TranslateErr::into_outer_err) + .expect("No Context errors while translating"); // 2. If we have a specific scriptpubkey we are targeting, bail out. if let Some(check_script) = check_script { From 627d963e01e9e41e7686e5c4b1805a559776c8c5 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sat, 5 Jul 2025 20:02:51 +0000 Subject: [PATCH 6/8] psbt: remove derives from UtxoUpdateError and OutputUpdateError Going forward, we should only derive Debug and whichever of Clone, PartialEq and Eq are possible. PartialEq is very useful for doing assertions, and Clone is often useful because it allows Results to be cloned. But the other traits are uncommon to implement on error types, have an unclear meaning, and limit our ability to compose types without breaking changes. Also adds a bunch of Eq bounds to error types that implement PartialEq. There is almost never a reason to implement PartialEq but not Eq. In the next commit I'm gonna replace ConversionError, an old error which derives all these things, with the new NonDefiniteKeyError, which will prevent these derives from working on UtxoUpdateError. --- src/descriptor/key.rs | 6 +++--- src/psbt/mod.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/descriptor/key.rs b/src/descriptor/key.rs index 40c4d50b4..4f85bd083 100644 --- a/src/descriptor/key.rs +++ b/src/descriptor/key.rs @@ -328,7 +328,7 @@ impl DescriptorMultiXKey { } /// Kinds of malformed key data -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, PartialEq, Eq, Clone)] #[non_exhaustive] #[allow(missing_docs)] pub enum NonDefiniteKeyError { @@ -353,7 +353,7 @@ impl fmt::Display for NonDefiniteKeyError { impl error::Error for NonDefiniteKeyError {} /// Kinds of malformed key data -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, PartialEq, Eq, Clone)] #[non_exhaustive] #[allow(missing_docs)] pub enum MalformedKeyDataKind { @@ -397,7 +397,7 @@ impl fmt::Display for MalformedKeyDataKind { } /// Descriptor Key parsing errors -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, PartialEq, Eq, Clone)] #[non_exhaustive] pub enum DescriptorKeyParseError { /// Error while parsing a BIP32 extended private key diff --git a/src/psbt/mod.rs b/src/psbt/mod.rs index bccdeeb6d..e3db0bd03 100644 --- a/src/psbt/mod.rs +++ b/src/psbt/mod.rs @@ -1175,7 +1175,7 @@ fn update_item_with_descriptor_helper( } /// Return error type for [`PsbtExt::update_input_with_descriptor`] -#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy)] +#[derive(Debug, PartialEq, Eq, Clone)] pub enum UtxoUpdateError { /// Index out of bounds IndexOutOfBounds(usize, usize), @@ -1224,7 +1224,7 @@ impl error::Error for UtxoUpdateError { } /// Return error type for [`PsbtExt::update_output_with_descriptor`] -#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy)] +#[derive(Debug, PartialEq, Eq, Clone)] pub enum OutputUpdateError { /// Index out of bounds IndexOutOfBounds(usize, usize), From 4930ccb76fb2ae0cc1d073fbde3e096d5ba37be8 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sat, 5 Jul 2025 19:59:11 +0000 Subject: [PATCH 7/8] key: replace ConversionError with NonDefiniteKeyError The new error type is now more descriptive and a strict superset of the old one. Furthermore, the new name is more appropriate since this error typically arises during construction of a DefiniteDescriptorKey rather than during conversion between different key types. --- src/descriptor/key.rs | 38 +++++++------------------------------- src/descriptor/mod.rs | 19 ++++++++----------- src/psbt/mod.rs | 16 ++++++++-------- 3 files changed, 23 insertions(+), 50 deletions(-) diff --git a/src/descriptor/key.rs b/src/descriptor/key.rs index 4f85bd083..91200c6d1 100644 --- a/src/descriptor/key.rs +++ b/src/descriptor/key.rs @@ -689,33 +689,6 @@ impl From for DescriptorPublicKey { } } -/// Descriptor key conversion error -#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy)] -pub enum ConversionError { - /// Attempted to convert a key with hardened derivations to a bitcoin public key - HardenedChild, - /// Attempted to convert a key with multiple derivation paths to a bitcoin public key - MultiKey, -} - -impl fmt::Display for ConversionError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.write_str(match *self { - Self::HardenedChild => "hardened child step in bip32 path", - Self::MultiKey => "multiple existing keys", - }) - } -} - -#[cfg(feature = "std")] -impl error::Error for ConversionError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - match self { - Self::HardenedChild | Self::MultiKey => None, - } - } -} - impl DescriptorPublicKey { /// The fingerprint of the master key associated with this key, `0x00000000` if none. pub fn master_fingerprint(&self) -> bip32::Fingerprint { @@ -849,7 +822,10 @@ impl DescriptorPublicKey { /// /// - If `index` is hardened. /// - If the key contains multi-path derivations - pub fn at_derivation_index(self, index: u32) -> Result { + pub fn at_derivation_index( + self, + index: u32, + ) -> Result { let definite = match self { DescriptorPublicKey::Single(_) => self, DescriptorPublicKey::XPub(xpub) => { @@ -858,12 +834,12 @@ impl DescriptorPublicKey { Wildcard::Unhardened => xpub.derivation_path.into_child( bip32::ChildNumber::from_normal_idx(index) .ok() - .ok_or(ConversionError::HardenedChild)?, + .ok_or(NonDefiniteKeyError::HardenedStep)?, ), Wildcard::Hardened => xpub.derivation_path.into_child( bip32::ChildNumber::from_hardened_idx(index) .ok() - .ok_or(ConversionError::HardenedChild)?, + .ok_or(NonDefiniteKeyError::HardenedStep)?, ), }; DescriptorPublicKey::XPub(DescriptorXKey { @@ -873,7 +849,7 @@ impl DescriptorPublicKey { wildcard: Wildcard::None, }) } - DescriptorPublicKey::MultiXPub(_) => return Err(ConversionError::MultiKey), + DescriptorPublicKey::MultiXPub(_) => return Err(NonDefiniteKeyError::Multipath), }; Ok(DefiniteDescriptorKey::new(definite) diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index aae995ccd..371c7fa35 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -53,9 +53,9 @@ pub mod checksum; mod key; pub use self::key::{ - ConversionError, DefiniteDescriptorKey, DerivPaths, DescriptorKeyParseError, - DescriptorMultiXKey, DescriptorPublicKey, DescriptorSecretKey, DescriptorXKey, InnerXKey, - MalformedKeyDataKind, SinglePriv, SinglePub, SinglePubKey, Wildcard, + DefiniteDescriptorKey, DerivPaths, DescriptorKeyParseError, DescriptorMultiXKey, + DescriptorPublicKey, DescriptorSecretKey, DescriptorXKey, InnerXKey, MalformedKeyDataKind, + NonDefiniteKeyError, SinglePriv, SinglePub, SinglePubKey, Wildcard, }; /// Alias type for a map of public key to secret key @@ -660,17 +660,14 @@ impl Descriptor { pub fn at_derivation_index( &self, index: u32, - ) -> Result, ConversionError> { + ) -> Result, NonDefiniteKeyError> { struct Derivator(u32); impl Translator for Derivator { type TargetPk = DefiniteDescriptorKey; - type Error = ConversionError; + type Error = NonDefiniteKeyError; - fn pk( - &mut self, - pk: &DescriptorPublicKey, - ) -> Result { + fn pk(&mut self, pk: &DescriptorPublicKey) -> Result { pk.clone().at_derivation_index(self.0) } @@ -711,7 +708,7 @@ impl Descriptor { &self, secp: &secp256k1::Secp256k1, index: u32, - ) -> Result, ConversionError> { + ) -> Result, NonDefiniteKeyError> { Ok(self.at_derivation_index(index)?.derived_descriptor(secp)) } @@ -852,7 +849,7 @@ impl Descriptor { secp: &secp256k1::Secp256k1, script_pubkey: &Script, range: Range, - ) -> Result)>, ConversionError> { + ) -> Result)>, NonDefiniteKeyError> { let range = if self.has_wildcard() { range } else { 0..1 }; for i in range { diff --git a/src/psbt/mod.rs b/src/psbt/mod.rs index e3db0bd03..dcf0e7690 100644 --- a/src/psbt/mod.rs +++ b/src/psbt/mod.rs @@ -925,14 +925,14 @@ pub trait PsbtInputExt { fn update_with_descriptor_unchecked( &mut self, descriptor: &Descriptor, - ) -> Result, descriptor::ConversionError>; + ) -> Result, descriptor::NonDefiniteKeyError>; } impl PsbtInputExt for psbt::Input { fn update_with_descriptor_unchecked( &mut self, descriptor: &Descriptor, - ) -> Result, descriptor::ConversionError> { + ) -> Result, descriptor::NonDefiniteKeyError> { let (derived, _) = update_item_with_descriptor_helper(self, descriptor, None)?; Ok(derived) } @@ -959,14 +959,14 @@ pub trait PsbtOutputExt { fn update_with_descriptor_unchecked( &mut self, descriptor: &Descriptor, - ) -> Result, descriptor::ConversionError>; + ) -> Result, descriptor::NonDefiniteKeyError>; } impl PsbtOutputExt for psbt::Output { fn update_with_descriptor_unchecked( &mut self, descriptor: &Descriptor, - ) -> Result, descriptor::ConversionError> { + ) -> Result, descriptor::NonDefiniteKeyError> { let (derived, _) = update_item_with_descriptor_helper(self, descriptor, None)?; Ok(derived) } @@ -1083,10 +1083,10 @@ fn update_item_with_descriptor_helper( check_script: Option<&Script>, // We return an extra boolean here to indicate an error with `check_script`. We do this // because the error is "morally" a UtxoUpdateError::MismatchedScriptPubkey, but some - // callers expect a `descriptor::ConversionError`, which cannot be produced from a + // callers expect a `descriptor::NonDefiniteKeyError`, which cannot be produced from a // `UtxoUpdateError`, and those callers can't get this error anyway because they pass // `None` for `check_script`. -) -> Result<(Descriptor, bool), descriptor::ConversionError> { +) -> Result<(Descriptor, bool), descriptor::NonDefiniteKeyError> { let secp = Secp256k1::verification_only(); // 1. Derive the descriptor, recording each key derivation in a map from xpubs @@ -1182,7 +1182,7 @@ pub enum UtxoUpdateError { /// The unsigned transaction didn't have an input at that index MissingInputUtxo, /// Derivation error - DerivationError(descriptor::ConversionError), + DerivationError(descriptor::NonDefiniteKeyError), /// The PSBT's `witness_utxo` and/or `non_witness_utxo` were invalid or missing UtxoCheck, /// The PSBT's `witness_utxo` and/or `non_witness_utxo` had a script_pubkey that did not match @@ -1231,7 +1231,7 @@ pub enum OutputUpdateError { /// The raw unsigned transaction didn't have an output at that index MissingTxOut, /// Derivation error - DerivationError(descriptor::ConversionError), + DerivationError(descriptor::NonDefiniteKeyError), /// The output's script_pubkey did not match the descriptor MismatchedScriptPubkey, } From f09bf48597ce58c16a2eaf52d455fb6749843691 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sat, 5 Jul 2025 20:52:52 +0000 Subject: [PATCH 8/8] fuzz: add fuzz test for parsing definite descriptor keys --- .github/workflows/cron-daily-fuzz.yml | 1 + fuzz/Cargo.toml | 4 ++++ .../fuzz_targets/parse_descriptor_definite.rs | 21 +++++++++++++++++++ 3 files changed, 26 insertions(+) create mode 100644 fuzz/fuzz_targets/parse_descriptor_definite.rs diff --git a/.github/workflows/cron-daily-fuzz.yml b/.github/workflows/cron-daily-fuzz.yml index dd7ab5da5..a65af2ef5 100644 --- a/.github/workflows/cron-daily-fuzz.yml +++ b/.github/workflows/cron-daily-fuzz.yml @@ -22,6 +22,7 @@ compile_descriptor, compile_taproot, miniscript_satisfy, parse_descriptor, +parse_descriptor_definite, parse_descriptor_priv, parse_descriptor_secret, regression_descriptor_parse, diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index b1722c5df..d1df87ca3 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -34,6 +34,10 @@ path = "fuzz_targets/miniscript_satisfy.rs" name = "parse_descriptor" path = "fuzz_targets/parse_descriptor.rs" +[[bin]] +name = "parse_descriptor_definite" +path = "fuzz_targets/parse_descriptor_definite.rs" + [[bin]] name = "parse_descriptor_priv" path = "fuzz_targets/parse_descriptor_priv.rs" diff --git a/fuzz/fuzz_targets/parse_descriptor_definite.rs b/fuzz/fuzz_targets/parse_descriptor_definite.rs new file mode 100644 index 000000000..f6a43476c --- /dev/null +++ b/fuzz/fuzz_targets/parse_descriptor_definite.rs @@ -0,0 +1,21 @@ +#![allow(unexpected_cfgs)] + +use honggfuzz::fuzz; +use miniscript::{DefiniteDescriptorKey, Descriptor}; + +fn do_test(data: &[u8]) { + let data_str = String::from_utf8_lossy(data); + + if let Ok(desc) = data_str.parse::>() { + let _ = desc.to_string(); + let _ = desc.address(miniscript::bitcoin::Network::Bitcoin); + } +} + +fn main() { + loop { + fuzz!(|data| { + do_test(data); + }); + } +}