expression: move some ad-hoc validation from descriptor module

In the next commit we will change the expression parser to parse both
{}s and ()s as parentheses, no longer distinguishing between a "taproot"
and "non-taproot" mode. This means that all non-Taproot descriptors need
to check that curly-brace {} expressions do not appear.

While we are adding this check, we can replace the existing checks for
things like "does this start with wsh and have exactly one child" with
an encapsulated function with strongly-typed errors. This gets rid of a
couple of Error::Unexpected instances.

We change one error output (if you pass no children to a sortedmulti).
The old text is nonsensical and the new text is explicit about what is
wrong.

This change is pretty-much mechanical, though unfortunately these are
all "manual" calls to validation functions, and if I missed any, the
compiler won't give us any help in noticing. But there aren't too many.
The only subtle/nonobvious thing maybe is that in Miniscript::from_tree
I added a call to verify_no_curly_braces which does a recursive check
for curly-braces. None of the other checks are recursive (nor do they
need to be).

Anyway, later on I will write a fuzz test which checks that we have not
changed the set of parseable descriptors (using normal keys, not Strings
or anything that might have braces in them, which we know we broke) and
that should catch any mistakes.

Also, similar to the last commit, this one doesn't really "do" anything
because it's still impossible to parse trees with mixed brace styles.
But in the next one, it will be possible, and we will be glad to have
moved a bunch of the diff into these prepatory commits.

src/descriptor/bare.rs

@@ -370,15 +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> {
-        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",
-                top.name,
-                top.args.len(),
-            )))
-        }
+        let top = top
+            .verify_toplevel("pkh", 1..=1)
+            .map_err(Error::ParseTree)?;
+        Ok(Pkh::new(expression::terminal(top, |pk| Pk::from_str(pk))?)?)
     }
 }
 

src/descriptor/mod.rs

@@ -1102,7 +1102,7 @@ mod tests {
             StdDescriptor::from_str("sh(sortedmulti)")
                 .unwrap_err()
                 .to_string(),
-            "expected threshold, found terminal",
+            "sortedmulti must have at least 1 children, but found 0"
         ); //issue 202
         assert_eq!(
             StdDescriptor::from_str(&format!("sh(sortedmulti(2,{}))", &TEST_PK[3..69]))

src/descriptor/segwitv0.rs

@@ -248,21 +248,16 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for Wsh<Pk> {
 
 impl<Pk: FromStrKey> crate::expression::FromTree for Wsh<Pk> {
     fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
-        if top.name == "wsh" && top.args.len() == 1 {
-            let top = &top.args[0];
-            if top.name == "sortedmulti" {
-                return Ok(Wsh { inner: WshInner::SortedMulti(SortedMultiVec::from_tree(top)?) });
-            }
-            let sub = Miniscript::from_tree(top)?;
-            Segwitv0::top_level_checks(&sub)?;
-            Ok(Wsh { inner: WshInner::Ms(sub) })
-        } else {
-            Err(Error::Unexpected(format!(
-                "{}({} args) while parsing wsh descriptor",
-                top.name,
-                top.args.len(),
-            )))
+        let top = top
+            .verify_toplevel("wsh", 1..=1)
+            .map_err(Error::ParseTree)?;
+
+        if top.name == "sortedmulti" {
+            return Ok(Wsh { inner: WshInner::SortedMulti(SortedMultiVec::from_tree(top)?) });
         }
+        let sub = Miniscript::from_tree(top)?;
+        Segwitv0::top_level_checks(&sub)?;
+        Ok(Wsh { inner: WshInner::Ms(sub) })
     }
 }
 
@@ -488,15 +483,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> {
-        if top.name == "wpkh" && top.args.len() == 1 {
-            Ok(Wpkh::new(expression::terminal(&top.args[0], |pk| Pk::from_str(pk))?)?)
-        } else {
-            Err(Error::Unexpected(format!(
-                "{}({} args) while parsing wpkh descriptor",
-                top.name,
-                top.args.len(),
-            )))
-        }
+        let top = top
+            .verify_toplevel("wpkh", 1..=1)
+            .map_err(Error::ParseTree)?;
+        Ok(Wpkh::new(expression::terminal(top, |pk| Pk::from_str(pk))?)?)
     }
 }
 

src/descriptor/sh.rs

@@ -82,26 +82,19 @@ impl<Pk: MiniscriptKey> fmt::Display for Sh<Pk> {
 
 impl<Pk: FromStrKey> crate::expression::FromTree for Sh<Pk> {
     fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
-        if top.name == "sh" && top.args.len() == 1 {
-            let top = &top.args[0];
-            let inner = match top.name {
-                "wsh" => ShInner::Wsh(Wsh::from_tree(top)?),
-                "wpkh" => ShInner::Wpkh(Wpkh::from_tree(top)?),
-                "sortedmulti" => ShInner::SortedMulti(SortedMultiVec::from_tree(top)?),
-                _ => {
-                    let sub = Miniscript::from_tree(top)?;
-                    Legacy::top_level_checks(&sub)?;
-                    ShInner::Ms(sub)
-                }
-            };
-            Ok(Sh { inner })
-        } else {
-            Err(Error::Unexpected(format!(
-                "{}({} args) while parsing sh descriptor",
-                top.name,
-                top.args.len(),
-            )))
-        }
+        let top = top.verify_toplevel("sh", 1..=1).map_err(Error::ParseTree)?;
+
+        let inner = match top.name {
+            "wsh" => ShInner::Wsh(Wsh::from_tree(top)?),
+            "wpkh" => ShInner::Wpkh(Wpkh::from_tree(top)?),
+            "sortedmulti" => ShInner::SortedMulti(SortedMultiVec::from_tree(top)?),
+            _ => {
+                let sub = Miniscript::from_tree(top)?;
+                Legacy::top_level_checks(&sub)?;
+                ShInner::Ms(sub)
+            }
+        };
+        Ok(Sh { inner })
     }
 }
 

src/descriptor/sortedmulti.rs

@@ -64,6 +64,9 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
         Pk: FromStr,
         <Pk as FromStr>::Err: fmt::Display,
     {
+        tree.verify_toplevel("sortedmulti", 1..)
+            .map_err(Error::ParseTree)?;
+
         let ret = Self {
             inner: tree
                 .to_null_threshold()

src/expression/error.rs

@@ -53,6 +53,29 @@ pub enum ParseTreeError {
         /// The position of the closing parethesis.
         close_pos: usize,
     },
+    /// A node had the wrong name.
+    IncorrectName {
+        /// The name that was found.
+        actual: String,
+        /// The name that was expected.
+        expected: &'static str,
+    },
+    /// A node had the wrong number of children.
+    IncorrectNumberOfChildren {
+        /// A description of the node in question.
+        description: &'static str,
+        /// The number of children the node had.
+        n_children: usize,
+        /// The minimum of children the node should have had.
+        minimum: Option<usize>,
+        /// The minimum of children the node should have had.
+        maximum: Option<usize>,
+    },
+    /// A Taproot child occurred somewhere it was not allowed.
+    IllegalCurlyBrace {
+        /// The position of the opening curly brace.
+        pos: usize,
+    },
     /// Data occurred after the final ).
     TrailingCharacter {
         /// The first trailing character.
@@ -93,6 +116,34 @@ impl fmt::Display for ParseTreeError {
                     open_ch, open_pos, close_ch, close_pos
                 )
             }
+            ParseTreeError::IllegalCurlyBrace { pos } => {
+                write!(f, "illegal `{{` at position {} (Taproot branches not allowed here)", pos)
+            }
+            ParseTreeError::IncorrectName { actual, expected } => {
+                if expected.is_empty() {
+                    write!(f, "found node '{}', expected nameless node", actual)
+                } else {
+                    write!(f, "expected node '{}', found '{}'", expected, actual)
+                }
+            }
+            ParseTreeError::IncorrectNumberOfChildren {
+                description,
+                n_children,
+                minimum,
+                maximum,
+            } => {
+                write!(f, "{} must have ", description)?;
+                match (minimum, maximum) {
+                    (_, Some(0)) => f.write_str("no children"),
+                    (Some(min), Some(max)) if min == max => write!(f, "{} children", min),
+                    (Some(min), None) if n_children < min => write!(f, "at least {} children", min),
+                    (Some(min), Some(max)) if n_children < min => write!(f, "at least {} children (maximum {})", min, max),
+                    (None, Some(max)) if n_children > max => write!(f, "at most {} children", max),
+                    (Some(min), Some(max)) if n_children > max => write!(f, "at most {} children (minimum {})", max, min),
+                    (x, y) => panic!("IncorrectNumberOfChildren error was constructed inconsistently (min {:?} max {:?})", x, y),
+                }?;
+                write!(f, ", but found {}", n_children)
+            }
             ParseTreeError::TrailingCharacter { ch, pos } => {
                 write!(f, "trailing data `{}...` (position {})", ch, pos)
             }
@@ -109,6 +160,9 @@ impl std::error::Error for ParseTreeError {
             | ParseTreeError::UnmatchedOpenParen { .. }
             | ParseTreeError::UnmatchedCloseParen { .. }
             | ParseTreeError::MismatchedParens { .. }
+            | ParseTreeError::IllegalCurlyBrace { .. }
+            | ParseTreeError::IncorrectName { .. }
+            | ParseTreeError::IncorrectNumberOfChildren { .. }
             | ParseTreeError::TrailingCharacter { .. } => None,
         }
     }

src/expression/mod.rs

@@ -5,11 +5,12 @@
 
 mod error;
 
-use core::fmt;
 use core::str::FromStr;
+use core::{fmt, ops};
 
 pub use self::error::{ParseThresholdError, ParseTreeError};
 use crate::descriptor::checksum::verify_checksum;
+use crate::iter::{self, TreeLike};
 use crate::prelude::*;
 use crate::{errstr, Error, Threshold, MAX_RECURSION_DEPTH};
 
@@ -44,6 +45,21 @@ impl PartialEq for Tree<'_> {
 }
 impl Eq for Tree<'_> {}
 
+impl<'a, 't> TreeLike for &'t Tree<'a> {
+    type NaryChildren = &'t [Tree<'a>];
+
+    fn nary_len(tc: &Self::NaryChildren) -> usize { tc.len() }
+    fn nary_index(tc: Self::NaryChildren, idx: usize) -> Self { &tc[idx] }
+
+    fn as_node(&self) -> iter::Tree<Self, Self::NaryChildren> {
+        if self.args.is_empty() {
+            iter::Tree::Nullary
+        } else {
+            iter::Tree::Nary(&self.args)
+        }
+    }
+}
+
 /// The type of parentheses surrounding a node's children.
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
 pub enum Parens {
@@ -71,6 +87,75 @@ pub trait FromTree: Sized {
 }
 
 impl<'a> Tree<'a> {
+    /// Check that a tree node has the given number of children.
+    ///
+    /// The `description` argument is only used to populate the error return,
+    /// and is not validated in any way.
+    pub fn verify_n_children(
+        &self,
+        description: &'static str,
+        n_children: impl ops::RangeBounds<usize>,
+    ) -> Result<(), ParseTreeError> {
+        if n_children.contains(&self.n_children()) {
+            Ok(())
+        } else {
+            let minimum = match n_children.start_bound() {
+                ops::Bound::Included(n) => Some(*n),
+                ops::Bound::Excluded(n) => Some(*n + 1),
+                ops::Bound::Unbounded => None,
+            };
+            let maximum = match n_children.end_bound() {
+                ops::Bound::Included(n) => Some(*n),
+                ops::Bound::Excluded(n) => Some(*n - 1),
+                ops::Bound::Unbounded => None,
+            };
+            Err(ParseTreeError::IncorrectNumberOfChildren {
+                description,
+                n_children: self.n_children(),
+                minimum,
+                maximum,
+            })
+        }
+    }
+
+    /// Check that a tree node has the given name, one child, and round braces.
+    ///
+    /// Returns the first child.
+    ///
+    /// # Panics
+    ///
+    /// Panics if zero is in bounds for `n_children` (since then there may be
+    /// no sensible value to return).
+    pub fn verify_toplevel(
+        &self,
+        name: &'static str,
+        n_children: impl ops::RangeBounds<usize>,
+    ) -> Result<&Self, ParseTreeError> {
+        assert!(
+            !n_children.contains(&0),
+            "verify_toplevel is intended for nodes with >= 1 child"
+        );
+
+        if self.name != name {
+            Err(ParseTreeError::IncorrectName { actual: self.name.to_owned(), expected: name })
+        } else if self.parens == Parens::Curly {
+            Err(ParseTreeError::IllegalCurlyBrace { pos: self.children_pos })
+        } else {
+            self.verify_n_children(name, n_children)?;
+            Ok(&self.args[0])
+        }
+    }
+
+    /// Check that a tree has no curly-brace children in it.
+    pub fn verify_no_curly_braces(&self) -> Result<(), ParseTreeError> {
+        for tree in self.pre_order_iter() {
+            if tree.parens == Parens::Curly {
+                return Err(ParseTreeError::IllegalCurlyBrace { pos: tree.children_pos });
+            }
+        }
+        Ok(())
+    }
+
     /// Parse an expression with round brackets
     pub fn from_slice(sl: &'a str) -> Result<Tree<'a>, ParseTreeError> {
         Self::from_slice_delim(sl, Deliminator::NonTaproot)

src/miniscript/mod.rs

@@ -802,6 +802,7 @@ impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Miniscr
     /// Parse an expression tree into a Miniscript. As a general rule, this
     /// should not be called directly; rather go through the descriptor API.
     fn from_tree(top: &expression::Tree) -> Result<Miniscript<Pk, Ctx>, Error> {
+        top.verify_no_curly_braces().map_err(Error::ParseTree)?;
         let inner: Terminal<Pk, Ctx> = expression::FromTree::from_tree(top)?;
         Miniscript::from_ast(inner)
     }