expression: replace most uses of terminal

This is a big diff but it's almost entirely mechanical. Replaces the old
expression::terminal method with two new `verify_terminal_parent` and
`verify_terminal` methods, which do the appropriate checks and return a
strongly-typed error.

Does not directly eliminate any instances of errstr or Unexpected
(though the next commit will), but it does eliminate a large class of
them: now when you try to parse an expression with a bad number of
children, e.g. and_v with 3 children, you will get an error message that
says this rather than an opaque "Unexected(<<and_v>>)" or whatever you
get now.

It does reduce the semantic::PolicyError type to a single variant, which
has no information and is only used to indicate that the entailment
calculation failed. This can be replaced by an Option (and will be, in
the next commit). It also eliminates some uses of concrete::PolicyError,
but the variants are still used by the absurd Policy::is_valid method,
so we have to keep them for now.

Also, you may notice that this commit and others have a ton of calls to
.map_err. I apologize for this. But when we change error types so that
parsing returns a string-parsing-specific error rather than the giant
Error enum, these should mostly go away.

Author: apoelstra

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/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/expression/mod.rs

@@ -9,10 +9,11 @@ use core::str::FromStr;
 use core::{fmt, ops};
 
 pub use self::error::{ParseThresholdError, ParseTreeError};
+use crate::blanket_traits::StaticDebugAndDisplay;
 use crate::descriptor::checksum::verify_checksum;
 use crate::iter::{self, TreeLike};
 use crate::prelude::*;
-use crate::{errstr, Error, Threshold, MAX_RECURSION_DEPTH};
+use crate::{errstr, Error, ParseError, Threshold, MAX_RECURSION_DEPTH};
 
 /// Allowed characters are descriptor strings.
 pub const INPUT_CHARSET: &str = "0123456789()[],'/*abcdefgh@:$%{}IJKLMNOPQRSTUVWXYZ&+-.;<=>?!^_|~ijklmnopqrstuvwxyzABCDEFGH`#\"\\ ";
@@ -137,6 +138,42 @@ impl<'a> Tree<'a> {
         }
     }
 
+    /// Check that a tree node is a terminal (has no children).
+    ///
+    /// If so, parse the terminal from a string and return it.
+    ///
+    /// The `description` and `inner_description` arguments are only used to
+    /// populate the error return, and is not validated in any way.
+    pub fn verify_terminal<T>(&self, description: &'static str) -> Result<T, ParseError>
+    where
+        T: FromStr,
+        T::Err: StaticDebugAndDisplay,
+    {
+        self.verify_n_children(description, 0..=0)
+            .map_err(ParseError::Tree)?;
+        T::from_str(self.name).map_err(ParseError::box_from_str)
+    }
+
+    /// Check that a tree node has exactly one child, which is a terminal.
+    ///
+    /// If so, parse the terminal child from a string and return it.
+    ///
+    /// The `description` and `inner_description` arguments are only used to
+    /// populate the error return, and is not validated in any way.
+    pub fn verify_terminal_parent<T>(
+        &self,
+        description: &'static str,
+        inner_description: &'static str,
+    ) -> Result<T, ParseError>
+    where
+        T: FromStr,
+        T::Err: StaticDebugAndDisplay,
+    {
+        self.verify_n_children(description, 1..=1)
+            .map_err(ParseError::Tree)?;
+        self.args[0].verify_terminal(inner_description)
+    }
+
     /// Check that a tree node has exactly two children.
     ///
     /// If so, return them.

src/lib.rs

@@ -449,8 +449,6 @@ pub enum Error {
     /// Compiler related errors
     CompilerError(crate::policy::compiler::CompilerError),
     /// Errors related to policy
-    SemanticPolicy(policy::semantic::PolicyError),
-    /// Errors related to policy
     ConcretePolicy(policy::concrete::PolicyError),
     /// Errors related to lifting
     LiftError(policy::LiftError),
@@ -514,7 +512,6 @@ impl fmt::Display for Error {
             Error::ContextError(ref e) => fmt::Display::fmt(e, f),
             #[cfg(feature = "compiler")]
             Error::CompilerError(ref e) => fmt::Display::fmt(e, f),
-            Error::SemanticPolicy(ref e) => fmt::Display::fmt(e, f),
             Error::ConcretePolicy(ref e) => fmt::Display::fmt(e, f),
             Error::LiftError(ref e) => fmt::Display::fmt(e, f),
             Error::MaxRecursiveDepthExceeded => write!(
@@ -577,7 +574,6 @@ impl std::error::Error for Error {
             #[cfg(feature = "compiler")]
             CompilerError(e) => Some(e),
             ConcretePolicy(e) => Some(e),
-            SemanticPolicy(e) => Some(e),
             LiftError(e) => Some(e),
             ContextError(e) => Some(e),
             AnalysisError(e) => Some(e),

src/miniscript/astelem.rs

@@ -7,9 +7,7 @@
 //! encoding in Bitcoin script, as well as a datatype. Full details
 //! are given on the Miniscript website.
 
-use core::str::FromStr;
-
-use bitcoin::hashes::{hash160, Hash};
+use bitcoin::hashes::Hash;
 use bitcoin::{absolute, opcodes, script};
 use sync::Arc;
 
@@ -33,7 +31,7 @@ impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Termina
         let binary =
             |node: &expression::Tree, name, termfn: fn(_, _) -> Self| -> Result<Self, Error> {
                 node.verify_binary(name)
-                    .map_err(crate::ParseError::Tree)
+                    .map_err(From::from)
                     .map_err(Error::Parse)
                     .and_then(|(x, y)| {
                         let x = Arc::<Miniscript<Pk, Ctx>>::from_tree(x)?;
@@ -43,70 +41,97 @@ impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Termina
             };
 
         let (frag_name, frag_wrap) = super::split_expression_name(top.name)?;
-        let unwrapped = match (frag_name, top.args.len()) {
-            ("expr_raw_pkh", 1) => expression::terminal(&top.args[0], |x| {
-                hash160::Hash::from_str(x).map(Terminal::RawPkH)
-            }),
-            ("pk_k", 1) => {
-                expression::terminal(&top.args[0], |x| Pk::from_str(x).map(Terminal::PkK))
-            }
-            ("pk_h", 1) => {
-                expression::terminal(&top.args[0], |x| Pk::from_str(x).map(Terminal::PkH))
-            }
-            ("after", 1) => expression::terminal(&top.args[0], |x| {
+        let unwrapped = match frag_name {
+            "expr_raw_pkh" => top
+                .verify_terminal_parent("expr_raw_pkh", "public key hash")
+                .map(Terminal::RawPkH)
+                .map_err(Error::Parse),
+            "pk_k" => top
+                .verify_terminal_parent("pk_k", "public key")
+                .map(Terminal::PkK)
+                .map_err(Error::Parse),
+            "pk_h" => top
+                .verify_terminal_parent("pk_h", "public key")
+                .map(Terminal::PkH)
+                .map_err(Error::Parse),
+            "after" => expression::terminal(&top.args[0], |x| {
                 expression::parse_num(x)
                     .and_then(|x| AbsLockTime::from_consensus(x).map_err(Error::AbsoluteLockTime))
                     .map(Terminal::After)
             }),
-            ("older", 1) => expression::terminal(&top.args[0], |x| {
+            "older" => expression::terminal(&top.args[0], |x| {
                 expression::parse_num(x)
                     .and_then(|x| RelLockTime::from_consensus(x).map_err(Error::RelativeLockTime))
                     .map(Terminal::Older)
             }),
-            ("sha256", 1) => expression::terminal(&top.args[0], |x| {
-                Pk::Sha256::from_str(x).map(Terminal::Sha256)
-            }),
-            ("hash256", 1) => expression::terminal(&top.args[0], |x| {
-                Pk::Hash256::from_str(x).map(Terminal::Hash256)
-            }),
-            ("ripemd160", 1) => expression::terminal(&top.args[0], |x| {
-                Pk::Ripemd160::from_str(x).map(Terminal::Ripemd160)
-            }),
-            ("hash160", 1) => expression::terminal(&top.args[0], |x| {
-                Pk::Hash160::from_str(x).map(Terminal::Hash160)
-            }),
-            ("1", 0) => Ok(Terminal::True),
-            ("0", 0) => Ok(Terminal::False),
-            ("and_v", _) => binary(top, "and_v", Terminal::AndV),
-            ("and_b", _) => binary(top, "and_b", Terminal::AndB),
-            ("and_n", 2) => Ok(Terminal::AndOr(
-                expression::FromTree::from_tree(&top.args[0])?,
-                expression::FromTree::from_tree(&top.args[1])?,
-                Arc::new(Miniscript::FALSE),
-            )),
-            ("andor", 3) => Ok(Terminal::AndOr(
-                expression::FromTree::from_tree(&top.args[0])?,
-                expression::FromTree::from_tree(&top.args[1])?,
-                expression::FromTree::from_tree(&top.args[2])?,
-            )),
-            ("or_b", _) => binary(top, "or_b", Terminal::OrB),
-            ("or_d", _) => binary(top, "or_d", Terminal::OrD),
-            ("or_c", _) => binary(top, "or_c", Terminal::OrC),
-            ("or_i", _) => binary(top, "or_i", Terminal::OrI),
-            ("thresh", _) => top
+            "sha256" => top
+                .verify_terminal_parent("sha256", "hash")
+                .map(Terminal::Sha256)
+                .map_err(Error::Parse),
+            "hash256" => top
+                .verify_terminal_parent("hash256", "hash")
+                .map(Terminal::Hash256)
+                .map_err(Error::Parse),
+            "ripemd160" => top
+                .verify_terminal_parent("ripemd160", "hash")
+                .map(Terminal::Ripemd160)
+                .map_err(Error::Parse),
+            "hash160" => top
+                .verify_terminal_parent("hash160", "hash")
+                .map(Terminal::Hash160)
+                .map_err(Error::Parse),
+            "1" => {
+                top.verify_n_children("1", 0..=0)
+                    .map_err(From::from)
+                    .map_err(Error::Parse)?;
+                Ok(Terminal::True)
+            }
+            "0" => {
+                top.verify_n_children("0", 0..=0)
+                    .map_err(From::from)
+                    .map_err(Error::Parse)?;
+                Ok(Terminal::False)
+            }
+            "and_v" => binary(top, "and_v", Terminal::AndV),
+            "and_b" => binary(top, "and_b", Terminal::AndB),
+            "and_n" => {
+                binary(top, "and_n", |x, y| Terminal::AndOr(x, y, Arc::new(Miniscript::FALSE)))
+            }
+            "andor" => {
+                top.verify_n_children("andor", 3..=3)
+                    .map_err(From::from)
+                    .map_err(Error::Parse)?;
+                let x = Arc::<Miniscript<Pk, Ctx>>::from_tree(&top.args[0])?;
+                let y = Arc::<Miniscript<Pk, Ctx>>::from_tree(&top.args[1])?;
+                let z = Arc::<Miniscript<Pk, Ctx>>::from_tree(&top.args[2])?;
+                Ok(Terminal::AndOr(x, y, z))
+            }
+            "or_b" => binary(top, "or_b", Terminal::OrB),
+            "or_d" => binary(top, "or_d", Terminal::OrD),
+            "or_c" => binary(top, "or_c", Terminal::OrC),
+            "or_i" => binary(top, "or_i", Terminal::OrI),
+            "thresh" => top
                 .to_null_threshold()
                 .map_err(Error::ParseThreshold)?
                 .translate_by_index(|i| Miniscript::from_tree(&top.args[1 + i]).map(Arc::new))
                 .map(Terminal::Thresh),
-            ("multi", _) => top
+            "multi" => top
                 .to_null_threshold()
                 .map_err(Error::ParseThreshold)?
-                .translate_by_index(|i| expression::terminal(&top.args[1 + i], Pk::from_str))
+                .translate_by_index(|i| {
+                    top.args[1 + i]
+                        .verify_terminal("public key")
+                        .map_err(Error::Parse)
+                })
                 .map(Terminal::Multi),
-            ("multi_a", _) => top
+            "multi_a" => top
                 .to_null_threshold()
                 .map_err(Error::ParseThreshold)?
-                .translate_by_index(|i| expression::terminal(&top.args[1 + i], Pk::from_str))
+                .translate_by_index(|i| {
+                    top.args[1 + i]
+                        .verify_terminal("public key")
+                        .map_err(Error::Parse)
+                })
                 .map(Terminal::MultiA),
             _ => Err(Error::Unexpected(format!(
                 "{}({} args) while parsing Miniscript",