Merge #839: more DefiniteDescriptorKey fixes and cleanups

f09bf48 fuzz: add fuzz test for parsing definite descriptor keys (Andrew Poelstra)
4930ccb key: replace ConversionError with NonDefiniteKeyError (Andrew Poelstra)
627d963 psbt: remove derives from UtxoUpdateError and OutputUpdateError (Andrew Poelstra)
c169af0 key.rs: remove a bunch of unwrap()s which are impossible (Andrew Poelstra)
6267818 TranslateErr: add helper function to unwrap impossible case (Andrew Poelstra)
e1f0802 macros: make target pk and error type optional for translate_* macros (Andrew Poelstra)
77ed78b key.rs: remove some deprecated methods (Andrew Poelstra)
7b3b3a8 key: fix DefiniteDescriptorKey construction from key with hardened step (Andrew Poelstra)

Pull request description:

  Again this one changes logic in a way that I need to backport, but the backport PRs will be much simpler since they are constrained not to change the API.
  
  This one makes it impossible to construct a `DefiniteDescriptorKey` with hardened derivation steps. Previously this was usually impossible, but not always, which led to inconsistent error checks and a proliferance of `unwrap`s and generally hard-to-reason-about behavior.


ACKs for top commit:
  sanket1729:
    ACK f09bf48.


Tree-SHA512: b57016592bc1b41c1ebe9ceba9c68f3feec432f79782aedbb2af0c817648d5af476e28a03968243fcf0f3ecde92bf28ee10414dd6e39a4d7d4119c4f68d1a1fe

Author: apoelstra

.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,

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,

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)?;
 

examples/xpub_descriptors.rs

@@ -31,7 +31,6 @@ fn p2wsh<C: Verification>(secp: &Secp256k1<C>) -> Address {
     let address = Descriptor::<DefiniteDescriptorKey>::from_str(&s)
         .unwrap()
         .derived_descriptor(secp)
-        .unwrap()
         .address(Network::Bitcoin)
         .unwrap();
 

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"

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::<Descriptor<DefiniteDescriptorKey>>() {
+        let _ = desc.to_string();
+        let _ = desc.address(miniscript::bitcoin::Network::Bitcoin);
+    }
+}
+
+fn main() {
+    loop {
+        fuzz!(|data| {
+            do_test(data);
+        });
+    }
+}

src/descriptor/key.rs

@@ -328,19 +328,23 @@ impl DescriptorMultiXKey<bip32::Xpriv> {
 }
 
 /// Kinds of malformed key data
-#[derive(Debug, PartialEq, Clone)]
+#[derive(Debug, PartialEq, Eq, Clone)]
 #[non_exhaustive]
 #[allow(missing_docs)]
 pub enum NonDefiniteKeyError {
     Wildcard,
     Multipath,
+    HardenedStep,
 }
 
 impl fmt::Display for NonDefiniteKeyError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         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")
+            }
         }
     }
 }
@@ -349,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 {
@@ -393,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
@@ -685,33 +689,6 @@ impl From<PublicKey> 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 {
@@ -806,10 +783,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 {
@@ -819,10 +792,21 @@ impl DescriptorPublicKey {
         }
     }
 
-    #[deprecated(note = "use at_derivation_index instead")]
-    /// Deprecated name for [`Self::at_derivation_index`].
-    pub fn derive(self, index: u32) -> Result<DefiniteDescriptorKey, ConversionError> {
-        self.at_derivation_index(index)
+    /// 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
     }
 
     /// Replaces any wildcard (i.e. `/*`) in the key with a particular derivation index, turning it into a
@@ -838,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<DefiniteDescriptorKey, ConversionError> {
+    pub fn at_derivation_index(
+        self,
+        index: u32,
+    ) -> Result<DefiniteDescriptorKey, NonDefiniteKeyError> {
         let definite = match self {
             DescriptorPublicKey::Single(_) => self,
             DescriptorPublicKey::XPub(xpub) => {
@@ -847,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 {
@@ -862,7 +849,7 @@ impl DescriptorPublicKey {
                     wildcard: Wildcard::None,
                 })
             }
-            DescriptorPublicKey::MultiXPub(_) => return Err(ConversionError::MultiKey),
+            DescriptorPublicKey::MultiXPub(_) => return Err(NonDefiniteKeyError::Multipath),
         };
 
         Ok(DefiniteDescriptorKey::new(definite)
@@ -1243,29 +1230,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<C: Verification>(
-        &self,
-        secp: &Secp256k1<C>,
-    ) -> Result<bitcoin::PublicKey, ConversionError> {
+    pub fn derive_public_key<C: Verification>(&self, secp: &Secp256k1<C>) -> 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")
             }
         }
     }
@@ -1276,6 +1260,8 @@ impl DefiniteDescriptorKey {
     pub fn new(key: DescriptorPublicKey) -> Result<Self, NonDefiniteKeyError> {
         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 {
@@ -1333,7 +1319,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 }
@@ -1785,5 +1771,13 @@ mod test {
             .parse::<DescriptorPublicKey>()
             .unwrap();
         assert!(matches!(DefiniteDescriptorKey::new(desc), Err(NonDefiniteKeyError::Multipath)));
+        // xpub with hardened path
+        let desc = "xpub661MyMwAqRbcFtXgS5sYJABqqG9YLmC4Q1Rdap9gSE8NqtwybGhePY2gZ29ESFjqJoCu1Rupje8YtGqsefD265TMg7usUDFdp6W1EGMcet8/1'/2"
+            .parse::<DescriptorPublicKey>()
+            .unwrap();
+        assert!(matches!(
+            DefiniteDescriptorKey::new(desc),
+            Err(NonDefiniteKeyError::HardenedStep)
+        ));
     }
 }