expression: unify Taproot and non-Taproot parsing

The `expression::Tree` type now parses expression trees containing both
{} and () as children. It marks which is which, and it is the
responsibility of FromTree implementors to make sure that the correct
style is used.

This eliminates a ton of ad-hoc string parsing code from
descriptor/tr.rs, including 8 instances of Error::Unexpected. It is also
the first change that preserves (sorta) a pubkey parsing error rather
than converting it to a string.

Because of rust-bitcoin#734
this inserts a call to `Descriptor::sanity_check` when parsing a string,
specifically for Taproot descriptors. This is weird and wrong but we
want to address it separately from this PR, whose goal is to preserve
all behavior.

This also introduces the new `error` module and `ParseError` enum, which
we will move all string-parsing errors into. For now we just stick it in
the big global enum, but eventually it will live on its own.

src/descriptor/mod.rs

@@ -21,6 +21,7 @@ use bitcoin::{
 };
 use sync::Arc;
 
+use crate::expression::FromTree as _;
 use crate::miniscript::decode::Terminal;
 use crate::miniscript::{satisfy, Legacy, Miniscript, Segwitv0};
 use crate::plan::{AssetProvider, Plan};
@@ -945,17 +946,17 @@ impl<Pk: FromStrKey> crate::expression::FromTree for Descriptor<Pk> {
 impl<Pk: FromStrKey> FromStr for Descriptor<Pk> {
     type Err = Error;
     fn from_str(s: &str) -> Result<Descriptor<Pk>, Error> {
-        // tr tree parsing has special code
-        // Tr::from_str will check the checksum
-        // match "tr(" to handle more extensibly
-        let desc = if s.starts_with("tr(") {
-            Ok(Descriptor::Tr(Tr::from_str(s)?))
-        } else {
-            let top = expression::Tree::from_str(s)?;
-            expression::FromTree::from_tree(&top)
-        }?;
-
-        Ok(desc)
+        let top = expression::Tree::from_str(s)?;
+        let ret = Self::from_tree(&top)?;
+        if let Descriptor::Tr(ref inner) = ret {
+            // FIXME preserve weird/broken behavior from 12.x.
+            // See https://github.com/rust-bitcoin/rust-miniscript/issues/734
+            ret.sanity_check()?;
+            for (_, ms) in inner.iter_scripts() {
+                ms.ext_check(&crate::miniscript::analyzable::ExtParams::sane())?;
+            }
+        }
+        Ok(ret)
     }
 }
 
@@ -1509,6 +1510,21 @@ mod tests {
         )
     }
 
+    #[test]
+    fn tr_named_branch() {
+        use crate::{ParseError, ParseTreeError};
+
+        assert!(matches!(
+            StdDescriptor::from_str(
+                "tr(0202d44008000010100000000084F0000000dd0dd00000000000201dceddd00d00,abc{0,0})"
+            ),
+            Err(Error::Parse(ParseError::Tree(ParseTreeError::IncorrectName {
+                expected: "",
+                ..
+            }))),
+        ));
+    }
+
     #[test]
     fn roundtrip_tests() {
         let descriptor = Descriptor::<bitcoin::PublicKey>::from_str("multi");

src/descriptor/tr.rs

@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: CC0-1.0
 
-use core::str::FromStr;
 use core::{cmp, fmt, hash};
 
 #[cfg(not(test))] // https://github.com/rust-lang/rust/issues/121684
@@ -12,9 +11,10 @@ use bitcoin::taproot::{
 use bitcoin::{opcodes, Address, Network, ScriptBuf, Weight};
 use sync::Arc;
 
-use super::checksum::{self, verify_checksum};
+use super::checksum;
 use crate::descriptor::DefiniteDescriptorKey;
 use crate::expression::{self, FromTree};
+use crate::iter::TreeLike as _;
 use crate::miniscript::satisfy::{Placeholder, Satisfaction, SchnorrSigType, Witness};
 use crate::miniscript::Miniscript;
 use crate::plan::AssetProvider;
@@ -23,8 +23,8 @@ use crate::policy::Liftable;
 use crate::prelude::*;
 use crate::util::{varint_len, witness_size};
 use crate::{
-    Error, ForEachKey, FromStrKey, MiniscriptKey, Satisfier, ScriptContext, Tap, Threshold,
-    ToPublicKey, TranslateErr, Translator,
+    Error, ForEachKey, FromStrKey, MiniscriptKey, ParseError, Satisfier, ScriptContext, Tap,
+    Threshold, ToPublicKey, TranslateErr, Translator,
 };
 
 /// A Taproot Tree representation.
@@ -485,79 +485,114 @@ where
     }
 }
 
-#[rustfmt::skip]
-impl<Pk: FromStrKey> Tr<Pk> {
-    // Helper function to parse taproot script path
-    fn parse_tr_script_spend(tree: &expression::Tree,) -> Result<TapTree<Pk>, Error> {
-        match tree {
-            expression::Tree { name, args, .. } if !name.is_empty() && args.is_empty() => {
-                let script = Miniscript::<Pk, Tap>::from_str(name)?;
-                Ok(TapTree::Leaf(Arc::new(script)))
-            }
-            expression::Tree { name, args, .. } if name.is_empty() && args.len() == 2 => {
-                let left = Self::parse_tr_script_spend(&args[0])?;
-                let right = Self::parse_tr_script_spend(&args[1])?;
-                Ok(TapTree::combine(left, right))
-            }
-            _ =>  {
-                Err(Error::Unexpected(
-                "unknown format for script spending paths while parsing taproot descriptor"
-                    .to_string(),
-            ))},
-        }
+impl<Pk: FromStrKey> core::str::FromStr for Tr<Pk> {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let expr_tree = expression::Tree::from_str(s)?;
+        Self::from_tree(&expr_tree)
     }
 }
 
 impl<Pk: FromStrKey> crate::expression::FromTree for Tr<Pk> {
-    fn from_tree(top: &expression::Tree) -> Result<Self, Error> {
-        if top.name == "tr" {
-            match top.args.len() {
-                1 => {
-                    let key = &top.args[0];
-                    if !key.args.is_empty() {
-                        return Err(Error::Unexpected(format!(
-                            "#{} script associated with `key-path` while parsing taproot descriptor",
-                            key.args.len()
-                        )));
+    fn from_tree(expr_tree: &expression::Tree) -> Result<Self, Error> {
+        use crate::expression::{Parens, ParseTreeError};
+
+        expr_tree
+            .verify_toplevel("tr", 1..=2)
+            .map_err(From::from)
+            .map_err(Error::Parse)?;
+
+        let mut round_paren_depth = 0;
+
+        let mut internal_key = None;
+        let mut tree_stack = vec![];
+
+        for item in expr_tree.verbose_pre_order_iter() {
+            // Top-level "tr" node.
+            if item.index == 0 {
+                if item.is_complete {
+                    debug_assert!(
+                        internal_key.is_some(),
+                        "checked above that top-level 'tr' has children"
+                    );
+                }
+            } 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)?,
+                );
+            } else {
+                // From here on we are into the taptree.
+                if item.n_children_yielded == 0 {
+                    match item.node.parens {
+                        Parens::Curly => {
+                            if !item.node.name.is_empty() {
+                                return Err(Error::Parse(ParseError::Tree(
+                                    ParseTreeError::IncorrectName {
+                                        actual: item.node.name.to_owned(),
+                                        expected: "",
+                                    },
+                                )));
+                            }
+                            if round_paren_depth > 0 {
+                                return Err(Error::Parse(ParseError::Tree(
+                                    ParseTreeError::IllegalCurlyBrace {
+                                        pos: item.node.children_pos,
+                                    },
+                                )));
+                            }
+                        }
+                        Parens::Round => round_paren_depth += 1,
+                        _ => {}
                     }
-                    Tr::new(expression::terminal(key, Pk::from_str)?, None)
                 }
-                2 => {
-                    let key = &top.args[0];
-                    if !key.args.is_empty() {
-                        return Err(Error::Unexpected(format!(
-                            "#{} script associated with `key-path` while parsing taproot descriptor",
-                            key.args.len()
-                        )));
+                if item.is_complete {
+                    if item.node.parens == Parens::Curly {
+                        if item.n_children_yielded == 2 {
+                            let rchild = tree_stack.pop().unwrap();
+                            let lchild = tree_stack.pop().unwrap();
+                            tree_stack.push(TapTree::combine(lchild, rchild));
+                        } else {
+                            return Err(Error::Parse(ParseError::Tree(
+                                ParseTreeError::IncorrectNumberOfChildren {
+                                    description: "Taptree node",
+                                    n_children: item.n_children_yielded,
+                                    minimum: Some(2),
+                                    maximum: Some(2),
+                                },
+                            )));
+                        }
+                    } else {
+                        if item.node.parens == Parens::Round {
+                            round_paren_depth -= 1;
+                        }
+                        if round_paren_depth == 0 {
+                            let script = Miniscript::from_tree(item.node)?;
+                            // FIXME hack for https://github.com/rust-bitcoin/rust-miniscript/issues/734
+                            if script.ty.corr.base != crate::miniscript::types::Base::B {
+                                return Err(Error::NonTopLevel(format!("{:?}", script)));
+                            };
+                            tree_stack.push(TapTree::Leaf(Arc::new(script)));
+                        }
                     }
-                    let tree = &top.args[1];
-                    let ret = Self::parse_tr_script_spend(tree)?;
-                    Tr::new(expression::terminal(key, Pk::from_str)?, Some(ret))
                 }
-                _ => Err(Error::Unexpected(format!(
-                    "{}[#{} args] while parsing taproot descriptor",
-                    top.name,
-                    top.args.len()
-                ))),
             }
-        } else {
-            Err(Error::Unexpected(format!(
-                "{}[#{} args] while parsing taproot descriptor",
-                top.name,
-                top.args.len()
-            )))
         }
-    }
-}
-
-impl<Pk: FromStrKey> core::str::FromStr for Tr<Pk> {
-    type Err = Error;
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        let desc_str = verify_checksum(s)
-            .map_err(From::from)
-            .map_err(Error::ParseTree)?;
-        let top = parse_tr_tree(desc_str)?;
-        Self::from_tree(&top)
+        assert!(tree_stack.len() <= 1);
+        Tr::new(internal_key.unwrap(), tree_stack.pop())
     }
 }
 
@@ -583,69 +618,6 @@ impl<Pk: MiniscriptKey> fmt::Display for Tr<Pk> {
     }
 }
 
-// Helper function to parse string into miniscript tree form
-fn parse_tr_tree(s: &str) -> Result<expression::Tree, Error> {
-    if s.len() > 3 && &s[..3] == "tr(" && s.as_bytes()[s.len() - 1] == b')' {
-        let rest = &s[3..s.len() - 1];
-        if !rest.contains(',') {
-            let key = expression::Tree::from_str(rest)?;
-            if !key.args.is_empty() {
-                return Err(Error::Unexpected("invalid taproot internal key".to_string()));
-            }
-            let internal_key = expression::Tree {
-                name: key.name,
-                parens: expression::Parens::None,
-                children_pos: 0,
-                args: vec![],
-            };
-            return Ok(expression::Tree {
-                name: "tr",
-                parens: expression::Parens::Round,
-                children_pos: 0,
-                args: vec![internal_key],
-            });
-        }
-        // use str::split_once() method to refactor this when compiler version bumps up
-        let (key, script) = split_once(rest, ',')
-            .ok_or_else(|| Error::BadDescriptor("invalid taproot descriptor".to_string()))?;
-
-        let key = expression::Tree::from_str(key)?;
-        if !key.args.is_empty() {
-            return Err(Error::Unexpected("invalid taproot internal key".to_string()));
-        }
-        let internal_key = expression::Tree {
-            name: key.name,
-            parens: expression::Parens::None,
-            children_pos: 0,
-            args: vec![],
-        };
-        let tree = expression::Tree::from_slice_delim(script, expression::Deliminator::Taproot)
-            .map_err(Error::ParseTree)?;
-        Ok(expression::Tree {
-            name: "tr",
-            parens: expression::Parens::Round,
-            children_pos: 0,
-            args: vec![internal_key, tree],
-        })
-    } else {
-        Err(Error::Unexpected("invalid taproot descriptor".to_string()))
-    }
-}
-
-fn split_once(inp: &str, delim: char) -> Option<(&str, &str)> {
-    if inp.is_empty() {
-        None
-    } else {
-        // find the first character that matches delim
-        let res = inp
-            .chars()
-            .position(|ch| ch == delim)
-            .map(|idx| (&inp[..idx], &inp[idx + 1..]))
-            .unwrap_or((inp, ""));
-        Some(res)
-    }
-}
-
 impl<Pk: MiniscriptKey> Liftable<Pk> for TapTree<Pk> {
     fn lift(&self) -> Result<Policy<Pk>, Error> {
         fn lift_helper<Pk: MiniscriptKey>(s: &TapTree<Pk>) -> Result<Policy<Pk>, Error> {
@@ -762,6 +734,8 @@ where
 
 #[cfg(test)]
 mod tests {
+    use core::str::FromStr;
+
     use super::*;
 
     fn descriptor() -> String {

src/error.rs

@@ -0,0 +1,52 @@
+// Written in 2019 by Andrew Poelstra <[email protected]>
+// SPDX-License-Identifier: CC0-1.0
+
+//! Errors
+
+use core::fmt;
+#[cfg(feature = "std")]
+use std::error;
+
+use crate::blanket_traits::StaticDebugAndDisplay;
+/// An error parsing a Miniscript object (policy, descriptor or miniscript)
+/// from a string.
+#[derive(Debug)]
+pub enum ParseError {
+    /// 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>),
+    /// Error parsing a string into an expression tree.
+    Tree(crate::ParseTreeError),
+}
+
+impl ParseError {
+    /// Boxes a `FromStr` error for a `Pk` (or associated types) into a `ParseError`
+    pub(crate) fn box_from_str<E: StaticDebugAndDisplay>(e: E) -> Self {
+        ParseError::FromStr(Box::new(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::FromStr(ref e) => e.fmt(f),
+            ParseError::Tree(ref e) => e.fmt(f),
+        }
+    }
+}
+
+#[cfg(feature = "std")]
+impl error::Error for ParseError {
+    fn source(&self) -> Option<&(dyn error::Error + 'static)> {
+        match self {
+            ParseError::FromStr(..) => None,
+            ParseError::Tree(ref e) => Some(e),
+        }
+    }
+}