Update TranslatePk again

This fixes of the long last standing bug in rust-miniscript that allowed
creating unsound miniscript using the translate APIs

Author: sanket1729

bitcoind-tests/tests/setup/test_util.rs

@@ -279,8 +279,10 @@ pub fn parse_test_desc(
     let desc = subs_hash_frag(desc, pubdata);
     let desc = Descriptor::<String>::from_str(&desc)?;
     let mut translator = StrDescPubKeyTranslator(0, pubdata);
-    let desc: Result<_, ()> = desc.translate_pk(&mut translator);
-    Ok(desc.expect("Translate must succeed"))
+    let desc = desc
+        .translate_pk(&mut translator)
+        .expect("Translation failed");
+    Ok(desc)
 }
 
 // substitute hash fragments in the string as the per rules

src/descriptor/bare.rs

@@ -20,8 +20,8 @@ use crate::policy::{semantic, Liftable};
 use crate::prelude::*;
 use crate::util::{varint_len, witness_to_scriptsig};
 use crate::{
-    BareCtx, Error, ForEachKey, Miniscript, MiniscriptKey, Satisfier, ToPublicKey, TranslatePk,
-    Translator,
+    BareCtx, Error, ForEachKey, Miniscript, MiniscriptKey, Satisfier, ToPublicKey, TranslateErr,
+    TranslatePk, Translator,
 };
 
 /// Create a Bare Descriptor. That is descriptor that is
@@ -188,11 +188,11 @@ where
 {
     type Output = Bare<Q>;
 
-    fn translate_pk<T, E>(&self, t: &mut T) -> Result<Self::Output, E>
+    fn translate_pk<T, E>(&self, t: &mut T) -> Result<Bare<Q>, TranslateErr<E>>
     where
         T: Translator<P, Q, E>,
     {
-        Ok(Bare::new(self.ms.translate_pk(t)?).expect("Translation cannot fail inside Bare"))
+        Ok(Bare::new(self.ms.translate_pk(t)?).map_err(TranslateErr::OuterError)?)
     }
 }
 
@@ -373,11 +373,14 @@ where
 {
     type Output = Pkh<Q>;
 
-    fn translate_pk<T, E>(&self, t: &mut T) -> Result<Self::Output, E>
+    fn translate_pk<T, E>(&self, t: &mut T) -> Result<Self::Output, TranslateErr<E>>
     where
         T: Translator<P, Q, E>,
     {
         let res = Pkh::new(t.pk(&self.pk)?);
-        Ok(res.expect("Expect will be fixed in next commit"))
+        match res {
+            Ok(pk) => Ok(pk),
+            Err(e) => Err(TranslateErr::OuterError(Error::from(e))),
+        }
     }
 }

src/descriptor/mod.rs

@@ -26,7 +26,7 @@ use crate::miniscript::{Legacy, Miniscript, Segwitv0};
 use crate::prelude::*;
 use crate::{
     expression, hash256, miniscript, BareCtx, Error, ForEachKey, MiniscriptKey, Satisfier,
-    ToPublicKey, TranslatePk, Translator,
+    ToPublicKey, TranslateErr, TranslatePk, Translator,
 };
 
 mod bare;
@@ -519,7 +519,7 @@ where
     type Output = Descriptor<Q>;
 
     /// Converts a descriptor using abstract keys to one using specific keys.
-    fn translate_pk<T, E>(&self, t: &mut T) -> Result<Self::Output, E>
+    fn translate_pk<T, E>(&self, t: &mut T) -> Result<Self::Output, TranslateErr<E>>
     where
         T: Translator<P, Q, E>,
     {
@@ -581,7 +581,10 @@ impl Descriptor<DescriptorPublicKey> {
 
             translate_hash_clone!(DescriptorPublicKey, DescriptorPublicKey, ConversionError);
         }
-        self.translate_pk(&mut Derivator(index))
+        self.translate_pk(&mut Derivator(index)).map_err(|e| {
+            e.try_into_translator_err()
+                .expect("No Context errors while translating")
+        })
     }
 
     #[deprecated(note = "use at_derivation_index instead")]
@@ -694,9 +697,13 @@ impl Descriptor<DescriptorPublicKey> {
         }
 
         let descriptor = Descriptor::<String>::from_str(s)?;
-        let descriptor = descriptor
-            .translate_pk(&mut keymap_pk)
-            .map_err(|e| Error::Unexpected(e.to_string()))?;
+        let descriptor = descriptor.translate_pk(&mut keymap_pk).map_err(|e| {
+            Error::Unexpected(
+                e.try_into_translator_err()
+                    .expect("No Outer context errors")
+                    .to_string(),
+            )
+        })?;
 
         Ok((descriptor, keymap_pk.0))
     }
@@ -823,49 +830,16 @@ impl Descriptor<DescriptorPublicKey> {
 
         for (i, desc) in descriptors.iter_mut().enumerate() {
             let mut index_choser = IndexChoser(i);
-            *desc = desc.translate_pk(&mut index_choser)?;
+            *desc = desc.translate_pk(&mut index_choser).map_err(|e| {
+                e.try_into_translator_err()
+                    .expect("No Context errors possible")
+            })?;
         }
 
         Ok(descriptors)
     }
 }
 
-impl<Pk: MiniscriptKey> Descriptor<Pk> {
-    /// Whether this descriptor is a multipath descriptor that contains any 2 multipath keys
-    /// with a different number of derivation paths.
-    /// Such a descriptor is invalid according to BIP389.
-    pub fn multipath_length_mismatch(&self) -> bool {
-        // (Ab)use `for_each_key` to record the number of derivation paths a multipath key has.
-        #[derive(PartialEq)]
-        enum MultipathLenChecker {
-            SinglePath,
-            MultipathLen(usize),
-            LenMismatch,
-        }
-
-        let mut checker = MultipathLenChecker::SinglePath;
-        self.for_each_key(|key| {
-            match key.num_der_paths() {
-                0 | 1 => {}
-                n => match checker {
-                    MultipathLenChecker::SinglePath => {
-                        checker = MultipathLenChecker::MultipathLen(n);
-                    }
-                    MultipathLenChecker::MultipathLen(len) => {
-                        if len != n {
-                            checker = MultipathLenChecker::LenMismatch;
-                        }
-                    }
-                    MultipathLenChecker::LenMismatch => {}
-                },
-            }
-            true
-        });
-
-        checker == MultipathLenChecker::LenMismatch
-    }
-}
-
 impl Descriptor<DefiniteDescriptorKey> {
     /// 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
@@ -909,8 +883,14 @@ impl Descriptor<DefiniteDescriptorKey> {
             translate_hash_clone!(DefiniteDescriptorKey, bitcoin::PublicKey, ConversionError);
         }
 
-        let derived = self.translate_pk(&mut Derivator(secp))?;
-        Ok(derived)
+        let derived = self.translate_pk(&mut Derivator(secp));
+        match derived {
+            Ok(derived) => Ok(derived),
+            Err(e) => match e.try_into_translator_err() {
+                Ok(e) => Err(e),
+                Err(_) => unreachable!("No Context errors when deriving keys"),
+            },
+        }
     }
 }
 
@@ -944,10 +924,6 @@ impl_from_str!(
             expression::FromTree::from_tree(&top)
         }?;
 
-        if desc.multipath_length_mismatch() {
-            return Err(Error::MultipathDescLenMismatch);
-        }
-
         Ok(desc)
     }
 );
@@ -1992,7 +1968,6 @@ pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))";
         // We can parse a multipath descriptors, and make it into separate single-path descriptors.
         let desc = Descriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/<7';8h;20>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/4567/<0;1;987>/*)))").unwrap();
         assert!(desc.is_multipath());
-        assert!(!desc.multipath_length_mismatch());
         assert_eq!(desc.into_single_descriptors().unwrap(), vec![
             Descriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/7'/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/4567/0/*)))").unwrap(),
             Descriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/8h/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/4567/1/*)))").unwrap(),
@@ -2002,7 +1977,6 @@ pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))";
         // Even if only one of the keys is multipath.
         let desc = Descriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/<0;1>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/4567/*)))").unwrap();
         assert!(desc.is_multipath());
-        assert!(!desc.multipath_length_mismatch());
         assert_eq!(desc.into_single_descriptors().unwrap(), vec![
             Descriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/0/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/4567/*)))").unwrap(),
             Descriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/1/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/4567/*)))").unwrap(),
@@ -2011,7 +1985,6 @@ pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))";
         // We can detect regular single-path descriptors.
         let notmulti_desc = Descriptor::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/4567/*)))").unwrap();
         assert!(!notmulti_desc.is_multipath());
-        assert!(!notmulti_desc.multipath_length_mismatch());
         assert_eq!(
             notmulti_desc.clone().into_single_descriptors().unwrap(),
             vec![notmulti_desc]

src/descriptor/segwitv0.rs

@@ -18,8 +18,8 @@ use crate::policy::{semantic, Liftable};
 use crate::prelude::*;
 use crate::util::varint_len;
 use crate::{
-    Error, ForEachKey, Miniscript, MiniscriptKey, Satisfier, Segwitv0, ToPublicKey, TranslatePk,
-    Translator,
+    Error, ForEachKey, Miniscript, MiniscriptKey, Satisfier, Segwitv0, ToPublicKey, TranslateErr,
+    TranslatePk, Translator,
 };
 /// A Segwitv0 wsh descriptor
 #[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
@@ -282,7 +282,7 @@ where
 {
     type Output = Wsh<Q>;
 
-    fn translate_pk<T, E>(&self, t: &mut T) -> Result<Self::Output, E>
+    fn translate_pk<T, E>(&self, t: &mut T) -> Result<Self::Output, TranslateErr<E>>
     where
         T: Translator<P, Q, E>,
     {
@@ -480,10 +480,14 @@ where
 {
     type Output = Wpkh<Q>;
 
-    fn translate_pk<T, E>(&self, t: &mut T) -> Result<Self::Output, E>
+    fn translate_pk<T, E>(&self, t: &mut T) -> Result<Self::Output, TranslateErr<E>>
     where
         T: Translator<P, Q, E>,
     {
-        Ok(Wpkh::new(t.pk(&self.pk)?).expect("Uncompressed keys in Wpkh"))
+        let res = Wpkh::new(t.pk(&self.pk)?);
+        match res {
+            Ok(pk) => Ok(pk),
+            Err(e) => Err(TranslateErr::OuterError(Error::from(e))),
+        }
     }
 }

src/descriptor/sh.rs

@@ -23,7 +23,7 @@ use crate::prelude::*;
 use crate::util::{varint_len, witness_to_scriptsig};
 use crate::{
     push_opcode_size, Error, ForEachKey, Legacy, Miniscript, MiniscriptKey, Satisfier, Segwitv0,
-    ToPublicKey, TranslatePk, Translator,
+    ToPublicKey, TranslateErr, TranslatePk, Translator,
 };
 
 /// A Legacy p2sh Descriptor
@@ -437,7 +437,7 @@ where
 {
     type Output = Sh<Q>;
 
-    fn translate_pk<T, E>(&self, t: &mut T) -> Result<Self::Output, E>
+    fn translate_pk<T, E>(&self, t: &mut T) -> Result<Self::Output, TranslateErr<E>>
     where
         T: Translator<P, Q, E>,
     {

src/descriptor/sortedmulti.rs

@@ -18,7 +18,7 @@ use crate::miniscript::limits::MAX_PUBKEYS_PER_MULTISIG;
 use crate::prelude::*;
 use crate::{
     errstr, expression, miniscript, policy, script_num_size, Error, ForEachKey, Miniscript,
-    MiniscriptKey, Satisfier, ToPublicKey, Translator,
+    MiniscriptKey, Satisfier, ToPublicKey, TranslateErr, Translator,
 };
 
 /// Contents of a "sortedmulti" descriptor
@@ -87,17 +87,14 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
     pub fn translate_pk<T, Q, FuncError>(
         &self,
         t: &mut T,
-    ) -> Result<SortedMultiVec<Q, Ctx>, FuncError>
+    ) -> Result<SortedMultiVec<Q, Ctx>, TranslateErr<FuncError>>
     where
         T: Translator<Pk, Q, FuncError>,
         Q: MiniscriptKey,
     {
         let pks: Result<Vec<Q>, _> = self.pks.iter().map(|pk| t.pk(pk)).collect();
-        Ok(SortedMultiVec {
-            k: self.k,
-            pks: pks?,
-            phantom: PhantomData,
-        })
+        let res = SortedMultiVec::new(self.k, pks?).map_err(TranslateErr::OuterError)?;
+        Ok(res)
     }
 }
 

src/descriptor/tr.rs

@@ -20,7 +20,7 @@ use crate::prelude::*;
 use crate::util::{varint_len, witness_size};
 use crate::{
     errstr, Error, ForEachKey, MiniscriptKey, Satisfier, ScriptContext, Tap, ToPublicKey,
-    TranslatePk, Translator,
+    TranslateErr, TranslatePk, Translator,
 };
 
 /// A Taproot Tree representation.
@@ -129,9 +129,9 @@ impl<Pk: MiniscriptKey> TapTree<Pk> {
     }
 
     // Helper function to translate keys
-    fn translate_helper<T, Q, Error>(&self, t: &mut T) -> Result<TapTree<Q>, Error>
+    fn translate_helper<T, Q, E>(&self, t: &mut T) -> Result<TapTree<Q>, TranslateErr<E>>
     where
-        T: Translator<Pk, Q, Error>,
+        T: Translator<Pk, Q, E>,
         Q: MiniscriptKey,
     {
         let frag = match self {
@@ -629,7 +629,7 @@ where
 {
     type Output = Tr<Q>;
 
-    fn translate_pk<T, E>(&self, translate: &mut T) -> Result<Self::Output, E>
+    fn translate_pk<T, E>(&self, translate: &mut T) -> Result<Self::Output, TranslateErr<E>>
     where
         T: Translator<P, Q, E>,
     {
@@ -638,7 +638,7 @@ where
             None => None,
         };
         let translate_desc = Tr::new(translate.pk(&self.internal_key)?, tree)
-            .expect("This will be removed in future");
+            .map_err(|e| TranslateErr::OuterError(e))?;
         Ok(translate_desc)
     }
 }

src/lib.rs

@@ -368,6 +368,57 @@ where
     fn hash160(&mut self, hash160: &P::Hash160) -> Result<Q::Hash160, E>;
 }
 
+/// An enum for representing translation errors
+pub enum TranslateErr<E> {
+    /// Error inside in the underlying key translation
+    TranslatorErr(E),
+    /// Error in the final translated structure. In some cases, the translated
+    /// structure might not be valid under the given context. For example, translating
+    /// from string keys to x-only keys in wsh descriptors.
+    OuterError(Error),
+}
+
+impl<E> TranslateErr<E> {
+    /// Enum used to capture errors from the [`Translator`] trait as well as
+    /// context errors from the translated structure.
+    /// The errors occurred in translation are captured in the [`TranslateErr::TranslatorErr`]
+    /// while the errors in the translated structure are captured in the [`TranslateErr::OuterError`]
+    ///
+    /// As of taproot upgrade: The following rules apply to the translation of descriptors:
+    /// - Legacy/Bare does not allow x_only keys
+    /// - SegwitV0 does not allow uncompressed keys and x_only keys
+    /// - Tapscript does not allow uncompressed keys
+    /// - Translating into multi-path descriptors should have same number of path
+    /// for all the keys in the descriptor
+    ///
+    /// # Errors
+    ///
+    /// This function will return an error if the Error is OutError.
+    pub fn try_into_translator_err(self) -> Result<E, Self> {
+        if let Self::TranslatorErr(v) = self {
+            Ok(v)
+        } else {
+            Err(self)
+        }
+    }
+}
+
+impl<E> From<E> for TranslateErr<E> {
+    fn from(v: E) -> Self {
+        Self::TranslatorErr(v)
+    }
+}
+
+// Required for unwrap
+impl<E: fmt::Debug> fmt::Debug for TranslateErr<E> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            Self::TranslatorErr(e) => write!(f, "TranslatorErr({:?})", e),
+            Self::OuterError(e) => write!(f, "OuterError({:?})", e),
+        }
+    }
+}
+
 /// Converts a descriptor using abstract keys to one using specific keys. Uses translator `t` to do
 /// the actual translation function calls.
 pub trait TranslatePk<P, Q>
@@ -380,7 +431,7 @@ where
 
     /// Translates a struct from one generic to another where the translations
     /// for Pk are provided by the given [`Translator`].
-    fn translate_pk<T, E>(&self, translator: &mut T) -> Result<Self::Output, E>
+    fn translate_pk<T, E>(&self, translator: &mut T) -> Result<Self::Output, TranslateErr<E>>
     where
         T: Translator<P, Q, E>;
 }