cleanups: Eliminate errstr and (nearly) eliminate Unexpected

src/descriptor/bare.rs

@@ -370,11 +370,10 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Pkh<Pk> {
 
 impl<Pk: FromStrKey> FromTree for Pkh<Pk> {
     fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
-        let top = top
-            .verify_toplevel("pkh", 1..=1)
-            .map_err(From::from)
+        let pk = top
+            .verify_terminal_parent("pkh", "public key")
             .map_err(Error::Parse)?;
-        Ok(Pkh::new(expression::terminal(top, |pk| Pk::from_str(pk))?)?)
+        Pkh::new(pk).map_err(Error::ContextError)
     }
 }
 

src/descriptor/mod.rs

@@ -27,8 +27,8 @@ use crate::miniscript::{satisfy, Legacy, Miniscript, Segwitv0};
 use crate::plan::{AssetProvider, Plan};
 use crate::prelude::*;
 use crate::{
-    expression, hash256, BareCtx, Error, ForEachKey, FromStrKey, MiniscriptKey, Satisfier,
-    ToPublicKey, TranslateErr, Translator,
+    expression, hash256, BareCtx, Error, ForEachKey, FromStrKey, MiniscriptKey, ParseError,
+    Satisfier, ToPublicKey, TranslateErr, Translator,
 };
 
 mod bare;
@@ -715,8 +715,9 @@ impl Descriptor<DescriptorPublicKey> {
                     Some(sk),
                 ),
                 Err(_) => (
-                    DescriptorPublicKey::from_str(s)
-                        .map_err(|e| Error::Unexpected(e.to_string()))?,
+                    // try to parse as a public key if parsing as a secret key failed
+                    s.parse()
+                        .map_err(|e| Error::Parse(ParseError::box_from_str(e)))?,
                     None,
                 ),
             };
@@ -741,37 +742,34 @@ impl Descriptor<DescriptorPublicKey> {
             }
 
             fn sha256(&mut self, sha256: &String) -> Result<sha256::Hash, Error> {
-                let hash =
-                    sha256::Hash::from_str(sha256).map_err(|e| Error::Unexpected(e.to_string()))?;
-                Ok(hash)
+                sha256
+                    .parse()
+                    .map_err(|e| Error::Parse(ParseError::box_from_str(e)))
             }
 
             fn hash256(&mut self, hash256: &String) -> Result<hash256::Hash, Error> {
-                let hash = hash256::Hash::from_str(hash256)
-                    .map_err(|e| Error::Unexpected(e.to_string()))?;
-                Ok(hash)
+                hash256
+                    .parse()
+                    .map_err(|e| Error::Parse(ParseError::box_from_str(e)))
             }
 
             fn ripemd160(&mut self, ripemd160: &String) -> Result<ripemd160::Hash, Error> {
-                let hash = ripemd160::Hash::from_str(ripemd160)
-                    .map_err(|e| Error::Unexpected(e.to_string()))?;
-                Ok(hash)
+                ripemd160
+                    .parse()
+                    .map_err(|e| Error::Parse(ParseError::box_from_str(e)))
             }
 
             fn hash160(&mut self, hash160: &String) -> Result<hash160::Hash, Error> {
-                let hash = hash160::Hash::from_str(hash160)
-                    .map_err(|e| Error::Unexpected(e.to_string()))?;
-                Ok(hash)
+                hash160
+                    .parse()
+                    .map_err(|e| Error::Parse(ParseError::box_from_str(e)))
             }
         }
 
         let descriptor = Descriptor::<String>::from_str(s)?;
-        let descriptor = descriptor.translate_pk(&mut keymap_pk).map_err(|e| {
-            Error::Unexpected(
-                e.expect_translator_err("No Outer context errors")
-                    .to_string(),
-            )
-        })?;
+        let descriptor = descriptor
+            .translate_pk(&mut keymap_pk)
+            .map_err(|e| e.expect_translator_err("No Outer context errors"))?;
 
         Ok((descriptor, keymap_pk.0))
     }

src/descriptor/segwitv0.rs

@@ -484,11 +484,10 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Wpkh<Pk> {
 
 impl<Pk: FromStrKey> crate::expression::FromTree for Wpkh<Pk> {
     fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
-        let top = top
-            .verify_toplevel("wpkh", 1..=1)
-            .map_err(From::from)
+        let pk = top
+            .verify_terminal_parent("wpkh", "public key")
             .map_err(Error::Parse)?;
-        Ok(Wpkh::new(expression::terminal(top, |pk| Pk::from_str(pk))?)?)
+        Wpkh::new(pk).map_err(Error::ContextError)
     }
 }
 

src/descriptor/sortedmulti.rs

@@ -7,10 +7,10 @@
 
 use core::fmt;
 use core::marker::PhantomData;
-use core::str::FromStr;
 
 use bitcoin::script;
 
+use crate::blanket_traits::FromStrKey;
 use crate::miniscript::context::ScriptContext;
 use crate::miniscript::decode::Terminal;
 use crate::miniscript::limits::MAX_PUBKEYS_PER_MULTISIG;
@@ -61,8 +61,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
     /// Parse an expression tree into a SortedMultiVec
     pub fn from_tree(tree: &expression::Tree) -> Result<Self, Error>
     where
-        Pk: FromStr,
-        <Pk as FromStr>::Err: fmt::Display,
+        Pk: FromStrKey,
     {
         tree.verify_toplevel("sortedmulti", 1..)
             .map_err(From::from)
@@ -72,7 +71,8 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
             inner: tree
                 .to_null_threshold()
                 .map_err(Error::ParseThreshold)?
-                .translate_by_index(|i| expression::terminal(&tree.args[i + 1], Pk::from_str))?,
+                .translate_by_index(|i| tree.args[i + 1].verify_terminal("public key"))
+                .map_err(Error::Parse)?,
             phantom: PhantomData,
         };
         ret.constructor_check()
@@ -230,6 +230,8 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> fmt::Display for SortedMultiVec<Pk,
 
 #[cfg(test)]
 mod tests {
+    use core::str::FromStr as _;
+
     use bitcoin::secp256k1::PublicKey;
 
     use super::*;

src/descriptor/tr.rs

@@ -524,21 +524,11 @@ impl<Pk: FromStrKey> crate::expression::FromTree for Tr<Pk> {
                 }
             } else if item.index == 1 {
                 // First child of tr, which must be the internal key
-                if !item.node.args.is_empty() {
-                    return Err(Error::Parse(ParseError::Tree(
-                        ParseTreeError::IncorrectNumberOfChildren {
-                            description: "internal key",
-                            n_children: item.node.args.len(),
-                            minimum: Some(0),
-                            maximum: Some(0),
-                        },
-                    )));
-                }
-                internal_key = Some(
-                    Pk::from_str(item.node.name)
-                        .map_err(ParseError::box_from_str)
-                        .map_err(Error::Parse)?,
-                );
+                internal_key = item
+                    .node
+                    .verify_terminal("internal key")
+                    .map_err(Error::Parse)
+                    .map(Some)?;
             } else {
                 // From here on we are into the taptree.
                 if item.n_children_yielded == 0 {

src/error.rs

@@ -8,16 +8,25 @@ use core::fmt;
 use std::error;
 
 use crate::blanket_traits::StaticDebugAndDisplay;
+use crate::primitives::absolute_locktime::AbsLockTimeError;
+use crate::primitives::relative_locktime::RelLockTimeError;
 use crate::Box;
+
 /// An error parsing a Miniscript object (policy, descriptor or miniscript)
 /// from a string.
 #[derive(Debug)]
 pub enum ParseError {
+    /// Invalid absolute locktime
+    AbsoluteLockTime(AbsLockTimeError),
+    /// Invalid absolute locktime
+    RelativeLockTime(RelLockTimeError),
     /// Failed to parse a public key or hash.
     ///
     /// Note that the error information is lost for nostd compatibility reasons. See
     /// <https://users.rust-lang.org/t/how-to-box-an-error-type-retaining-std-error-only-when-std-is-enabled/>.
     FromStr(Box<dyn StaticDebugAndDisplay>),
+    /// Failed to parse a number.
+    Num(crate::ParseNumError),
     /// Error parsing a string into an expression tree.
     Tree(crate::ParseTreeError),
 }
@@ -29,14 +38,21 @@ impl ParseError {
     }
 }
 
+impl From<crate::ParseNumError> for ParseError {
+    fn from(e: crate::ParseNumError) -> Self { Self::Num(e) }
+}
+
 impl From<crate::ParseTreeError> for ParseError {
     fn from(e: crate::ParseTreeError) -> Self { Self::Tree(e) }
 }
 
 impl fmt::Display for ParseError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
+            ParseError::AbsoluteLockTime(ref e) => e.fmt(f),
+            ParseError::RelativeLockTime(ref e) => e.fmt(f),
             ParseError::FromStr(ref e) => e.fmt(f),
+            ParseError::Num(ref e) => e.fmt(f),
             ParseError::Tree(ref e) => e.fmt(f),
         }
     }
@@ -46,7 +62,10 @@ impl fmt::Display for ParseError {
 impl error::Error for ParseError {
     fn source(&self) -> Option<&(dyn error::Error + 'static)> {
         match self {
+            ParseError::AbsoluteLockTime(ref e) => Some(e),
+            ParseError::RelativeLockTime(ref e) => Some(e),
             ParseError::FromStr(..) => None,
+            ParseError::Num(ref e) => Some(e),
             ParseError::Tree(ref e) => Some(e),
         }
     }

src/expression/error.rs

@@ -2,7 +2,7 @@
 
 //! Expression-related errors
 
-use core::fmt;
+use core::{fmt, num};
 
 use crate::descriptor::checksum;
 use crate::prelude::*;
@@ -76,13 +76,25 @@ pub enum ParseTreeError {
         /// The position of the opening curly brace.
         pos: usize,
     },
+    ///  Multiple separators (':' or '@') appeared in a node name.
+    MultipleSeparators {
+        /// The separator in question.
+        separator: char,
+        /// The position of the second separator.
+        pos: usize,
+    },
     /// Data occurred after the final ).
     TrailingCharacter {
         /// The first trailing character.
         ch: char,
         /// Its byte-index into the string.
         pos: usize,
     },
+    /// A node's name was not recognized.
+    UnknownName {
+        /// The name that was not recognized.
+        name: String,
+    },
 }
 
 impl From<checksum::Error> for ParseTreeError {
@@ -144,9 +156,17 @@ impl fmt::Display for ParseTreeError {
                 }?;
                 write!(f, ", but found {}", n_children)
             }
+            ParseTreeError::MultipleSeparators { separator, pos } => {
+                write!(
+                    f,
+                    "separator '{}' occured multiple times (second time at position {})",
+                    separator, pos
+                )
+            }
             ParseTreeError::TrailingCharacter { ch, pos } => {
                 write!(f, "trailing data `{}...` (position {})", ch, pos)
             }
+            ParseTreeError::UnknownName { name } => write!(f, "unrecognized name '{}'", name),
         }
     }
 }
@@ -163,7 +183,39 @@ impl std::error::Error for ParseTreeError {
             | ParseTreeError::IllegalCurlyBrace { .. }
             | ParseTreeError::IncorrectName { .. }
             | ParseTreeError::IncorrectNumberOfChildren { .. }
-            | ParseTreeError::TrailingCharacter { .. } => None,
+            | ParseTreeError::MultipleSeparators { .. }
+            | ParseTreeError::TrailingCharacter { .. }
+            | ParseTreeError::UnknownName { .. } => None,
+        }
+    }
+}
+
+/// Error parsing a number.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum ParseNumError {
+    /// Failed to parse the number at all.
+    StdParse(num::ParseIntError),
+    /// Number had a leading zero, + or -.
+    InvalidLeadingDigit(char),
+}
+
+impl fmt::Display for ParseNumError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            ParseNumError::StdParse(ref e) => e.fmt(f),
+            ParseNumError::InvalidLeadingDigit(ch) => {
+                write!(f, "numbers must start with 1-9, not {}", ch)
+            }
+        }
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for ParseNumError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        match self {
+            ParseNumError::StdParse(ref e) => Some(e),
+            ParseNumError::InvalidLeadingDigit(..) => None,
         }
     }
 }
@@ -175,10 +227,12 @@ pub enum ParseThresholdError {
     NoChildren,
     /// The threshold value appeared to be a sub-expression rather than a number.
     KNotTerminal,
+    /// A 1-of-n threshold was used in a context it was not allowed.
+    IllegalOr,
+    /// A n-of-n threshold was used in a context it was not allowed.
+    IllegalAnd,
     /// Failed to parse the threshold value.
-    // FIXME this should be a more specific type. Will be handled in a later PR
-    // that rewrites the expression parsing logic.
-    ParseK(String),
+    ParseK(ParseNumError),
     /// Threshold parameters were invalid.
     Threshold(ThresholdError),
 }
@@ -190,7 +244,13 @@ impl fmt::Display for ParseThresholdError {
         match *self {
             NoChildren => f.write_str("expected threshold, found terminal"),
             KNotTerminal => f.write_str("expected positive integer, found expression"),
-            ParseK(ref x) => write!(f, "failed to parse threshold value {}", x),
+            IllegalOr => f.write_str(
+                "1-of-n thresholds not allowed here; please use an 'or' fragment instead",
+            ),
+            IllegalAnd => f.write_str(
+                "n-of-n thresholds not allowed here; please use an 'and' fragment instead",
+            ),
+            ParseK(ref x) => write!(f, "failed to parse threshold value: {}", x),
             Threshold(ref e) => e.fmt(f),
         }
     }
@@ -202,9 +262,8 @@ impl std::error::Error for ParseThresholdError {
         use ParseThresholdError::*;
 
         match *self {
-            NoChildren => None,
-            KNotTerminal => None,
-            ParseK(..) => None,
+            NoChildren | KNotTerminal | IllegalOr | IllegalAnd => None,
+            ParseK(ref e) => Some(e),
             Threshold(ref e) => Some(e),
         }
     }