Merge #493: update TranslatePk again

b90cdda Change try_into_translator_err to expect_translator_err (sanket1729)
be07a9c Update TranslatePk again (sanket1729)
ee8de9a Cleanly check validity rules for DescriptorKeys (sanket1729)
fa2e8b4 Cleanup Miniscript constructors (sanket1729)

Pull request description:

  This PR has lot of cleanups that I had planned but did not have time for a long time.

  - All places now use constructors that do invariant checking on the struct being created.
  - Translate* APIs had a soundness bug you could create uncompressed keys in segwit descriptors etc.

ACKs for top commit:
  apoelstra:
    ACK b90cdda

Tree-SHA512: d1c7001650b55c018d6e395628cb3cb8e5d15540c24b696946d370202e6ccc82b376b7f18c1138fdbd83a550fef0e39dc8b44fd670019dc4854b87a67cdc41c7

Author: apoelstra

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

@@ -15,13 +15,13 @@ use bitcoin::{Address, Network, ScriptBuf};
 
 use super::checksum::{self, verify_checksum};
 use crate::expression::{self, FromTree};
-use crate::miniscript::context::ScriptContext;
+use crate::miniscript::context::{ScriptContext, ScriptContextError};
 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)?)
     }
 }
 
@@ -205,9 +205,12 @@ pub struct Pkh<Pk: MiniscriptKey> {
 
 impl<Pk: MiniscriptKey> Pkh<Pk> {
     /// Create a new Pkh descriptor
-    pub fn new(pk: Pk) -> Self {
+    pub fn new(pk: Pk) -> Result<Self, ScriptContextError> {
         // do the top-level checks
-        Self { pk }
+        match BareCtx::check_pk(&pk) {
+            Ok(()) => Ok(Pkh { pk }),
+            Err(e) => Err(e),
+        }
     }
 
     /// Get a reference to the inner key
@@ -336,7 +339,7 @@ impl_from_tree!(
         if top.name == "pkh" && top.args.len() == 1 {
             Ok(Pkh::new(expression::terminal(&top.args[0], |pk| {
                 Pk::from_str(pk)
-            })?))
+            })?)?)
         } else {
             Err(Error::Unexpected(format!(
                 "{}({} args) while parsing pkh descriptor",
@@ -370,10 +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>,
     {
-        Ok(Pkh::new(t.pk(&self.pk)?))
+        let res = Pkh::new(t.pk(&self.pk)?);
+        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;
@@ -177,8 +177,8 @@ impl<Pk: MiniscriptKey> Descriptor<Pk> {
     }
 
     /// Create a new PkH descriptor
-    pub fn new_pkh(pk: Pk) -> Self {
-        Descriptor::Pkh(Pkh::new(pk))
+    pub fn new_pkh(pk: Pk) -> Result<Self, Error> {
+        Ok(Descriptor::Pkh(Pkh::new(pk)?))
     }
 
     /// Create a new Wpkh descriptor
@@ -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>,
     {
@@ -582,6 +582,7 @@ impl Descriptor<DescriptorPublicKey> {
             translate_hash_clone!(DescriptorPublicKey, DescriptorPublicKey, ConversionError);
         }
         self.translate_pk(&mut Derivator(index))
+            .map_err(|e| e.expect_translator_err("No Context errors while translating"))
     }
 
     #[deprecated(note = "use at_derivation_index instead")]
@@ -694,9 +695,12 @@ 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.expect_translator_err("No Outer context errors")
+                    .to_string(),
+            )
+        })?;
 
         Ok((descriptor, keymap_pk.0))
     }
@@ -823,49 +827,15 @@ 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.expect_translator_err("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 +879,11 @@ 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) => Err(e.expect_translator_err("No Context errors when deriving keys")),
+        }
     }
 }
 
@@ -944,10 +917,6 @@ impl_from_str!(
             expression::FromTree::from_tree(&top)
         }?;
 
-        if desc.multipath_length_mismatch() {
-            return Err(Error::MultipathDescLenMismatch);
-        }
-
         Ok(desc)
     }
 );
@@ -1297,7 +1266,7 @@ mod tests {
         );
         assert_eq!(bare.unsigned_script_sig(), bitcoin::ScriptBuf::new());
 
-        let pkh = Descriptor::new_pkh(pk);
+        let pkh = Descriptor::new_pkh(pk).unwrap();
         pkh.satisfy(&mut txin, &satisfier).expect("satisfaction");
         assert_eq!(
             txin,
@@ -1992,7 +1961,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 +1970,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 +1978,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]
@@ -2021,4 +1987,54 @@ pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))";
         Descriptor::<DescriptorPublicKey>::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/<0;1>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/<0;1;2;3;4>/*)))").unwrap_err();
         Descriptor::<DescriptorPublicKey>::from_str("wsh(andor(pk(tpubDEN9WSToTyy9ZQfaYqSKfmVqmq1VVLNtYfj3Vkqh67et57eJ5sTKZQBkHqSwPUsoSskJeaYnPttHe2VrkCsKA27kUaN9SDc5zhqeLzKa1rr/0'/<0;1;2;3>/*),older(10000),pk(tpubD8LYfn6njiA2inCoxwM7EuN3cuLVcaHAwLYeups13dpevd3nHLRdK9NdQksWXrhLQVxcUZRpnp5CkJ1FhE61WRAsHxDNAkvGkoQkAeWDYjV/8/<0;1;2>/*)))").unwrap_err();
     }
+
+    #[test]
+    fn test_context_pks() {
+        let comp_key = bitcoin::PublicKey::from_str(
+            "02015e4cb53458bf813db8c79968e76e10d13ed6426a23fa71c2f41ba021c2a7ab",
+        )
+        .unwrap();
+        let x_only_key = bitcoin::key::XOnlyPublicKey::from_str(
+            "015e4cb53458bf813db8c79968e76e10d13ed6426a23fa71c2f41ba021c2a7ab",
+        )
+        .unwrap();
+        let uncomp_key = bitcoin::PublicKey::from_str("04015e4cb53458bf813db8c79968e76e10d13ed6426a23fa71c2f41ba021c2a7ab0d46021e9e69ef061eb25eab41ae206187b2b05e829559df59d78319bd9267b4").unwrap();
+
+        type Desc = Descriptor<DescriptorPublicKey>;
+
+        // Legacy tests, x-only keys are not supported
+        Desc::from_str(&format!("sh(pk({}))", comp_key)).unwrap();
+        Desc::from_str(&format!("sh(pk({}))", uncomp_key)).unwrap();
+        Desc::from_str(&format!("sh(pk({}))", x_only_key)).unwrap_err();
+
+        // bare tests, x-only keys not supported
+        Desc::from_str(&format!("pk({})", comp_key)).unwrap();
+        Desc::from_str(&format!("pk({})", uncomp_key)).unwrap();
+        Desc::from_str(&format!("pk({})", x_only_key)).unwrap_err();
+
+        // pkh tests, x-only keys not supported
+        Desc::from_str(&format!("pkh({})", comp_key)).unwrap();
+        Desc::from_str(&format!("pkh({})", uncomp_key)).unwrap();
+        Desc::from_str(&format!("pkh({})", x_only_key)).unwrap_err();
+
+        // wpkh tests, uncompressed and x-only keys not supported
+        Desc::from_str(&format!("wpkh({})", comp_key)).unwrap();
+        Desc::from_str(&format!("wpkh({})", uncomp_key)).unwrap_err();
+        Desc::from_str(&format!("wpkh({})", x_only_key)).unwrap_err();
+
+        // Segwitv0 tests, uncompressed and x-only keys not supported
+        Desc::from_str(&format!("wsh(pk({}))", comp_key)).unwrap();
+        Desc::from_str(&format!("wsh(pk({}))", uncomp_key)).unwrap_err();
+        Desc::from_str(&format!("wsh(pk({}))", x_only_key)).unwrap_err();
+
+        // Tap tests, key path
+        Desc::from_str(&format!("tr({})", comp_key)).unwrap();
+        Desc::from_str(&format!("tr({})", uncomp_key)).unwrap_err();
+        Desc::from_str(&format!("tr({})", x_only_key)).unwrap();
+
+        // Tap tests, script path
+        Desc::from_str(&format!("tr({},pk({}))", x_only_key, comp_key)).unwrap();
+        Desc::from_str(&format!("tr({},pk({}))", x_only_key, uncomp_key)).unwrap_err();
+        Desc::from_str(&format!("tr({},pk({}))", x_only_key, x_only_key)).unwrap();
+    }
 }

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>,
     {
@@ -303,14 +303,11 @@ pub struct Wpkh<Pk: MiniscriptKey> {
 
 impl<Pk: MiniscriptKey> Wpkh<Pk> {
     /// Create a new Wpkh descriptor
-    pub fn new(pk: Pk) -> Result<Self, Error> {
+    pub fn new(pk: Pk) -> Result<Self, ScriptContextError> {
         // do the top-level checks
-        if pk.is_uncompressed() {
-            Err(Error::ContextError(ScriptContextError::CompressedOnly(
-                pk.to_string(),
-            )))
-        } else {
-            Ok(Self { pk })
+        match Segwitv0::check_pk(&pk) {
+            Ok(_) => Ok(Wpkh { pk }),
+            Err(e) => Err(e),
         }
     }
 
@@ -483,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>,
     {