Merge rust-bitcoin/rust-miniscript#775: Rewrite expression parser to be non-recursive

75f400f86579b6c6047822fac1de5fccfb25fa67 error: remove two more variants which were redundant with threshold errors (Andrew Poelstra)
cdac32e22dd443ef7ae8671652c4b0cea5164515 error: remove 3 now-unused variants from the global Error enum (Andrew Poelstra)
ccb9c7089e2e7d125cf16a00fd2e5a4b8ec97990 expression: drop Error::ParseTree variant (Andrew Poelstra)
0b6fcf4833996c88b4989ab8acbbb6c721d0f5bd expression: unify Taproot and non-Taproot parsing (Andrew Poelstra)
360ed59411a58349b890cca8b59556ea2e574581 Introduce `error` module with single `ParseError` enum in it. (Andrew Poelstra)
179dc87cf8249d9fc9e18ac778e988f579ea5657 expression: move some ad-hoc validation from descriptor module (Andrew Poelstra)
f5dcafd36e9834055e012b5e1b34a5a2d38b29b2 expression: start tracking parenthesis-type and string position (Andrew Poelstra)
853a01af9515466b5a99edca711b8007e68a0d1c expression: rewrite parser to be non-recursive (Andrew Poelstra)
20066bdb185a90d2e470413d9b229b35a348ab0f checksum: use array::from_fn (Andrew Poelstra)
df75cbc362165fe8d139e2b7b48e15f5f9feac76 update fuzz/README.md to use hex crate (Andrew Poelstra)

Pull request description:

  This PR does several things but the only big commit is "unify Taproot and non-Taproot parsing" which replaces the ad-hoc logic in `Tr::from_str` to handle Taproot parsing with unified expression-parsing logic.

  In addition to this, we also:
  * rewrite the expression parser to be non-recursive, which is a reduction in LOC thanks to our prepatory work
  * introduces an `error` module whose types will eventually replace the top-level `Error` enum
  * relatedly, drops several error variants including the stringly-typed `BadDescriptor` (it does not get rid of `Unexpected` which is *also* a stringly-typed catch-all error, but we will..)

ACKs for top commit:
  sanket1729:
    ACK 75f400f86579b6c6047822fac1de5fccfb25fa67

Tree-SHA512: 8c535f420f39cf565900adb938f7a30c24cfc93fe30f09268f1b2d70fff5d7dc1f06ca0a3972c118f4e6889b107040feeab849b22e74bd8355f816a9dd3d4448

Author: heap-coder

fuzz/README.md

@@ -118,28 +118,12 @@ as follows:
 ```
 #[cfg(test)]
 mod tests {
-    fn extend_vec_from_hex(hex: &str, out: &mut Vec<u8>) {
-        let mut b = 0;
-        for (idx, c) in hex.as_bytes().iter().enumerate() {
-            b <<= 4;
-            match *c {
-                b'A'...b'F' => b |= c - b'A' + 10,
-                b'a'...b'f' => b |= c - b'a' + 10,
-                b'0'...b'9' => b |= c - b'0',
-                _ => panic!("Bad hex"),
-            }
-            if (idx & 1) == 1 {
-                out.push(b);
-                b = 0;
-            }
-        }
-    }
+    use miniscript::bitcoin::hex::FromHex;
 
     #[test]
     fn duplicate_crash() {
-        let mut a = Vec::new();
-        extend_vec_from_hex("048531e80700ae6400670000af5168", &mut a);
-        super::do_test(&a);
+        let v = Vec::from_hex("abcd").unwrap();
+        super::do_test(&v);
     }
 }
 ```

src/descriptor/bare.rs

@@ -370,15 +370,11 @@ 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(From::from)
+            .map_err(Error::Parse)?;
+        Ok(Pkh::new(expression::terminal(top, |pk| Pk::from_str(pk))?)?)
     }
 }
 

src/descriptor/checksum.rs

@@ -8,8 +8,8 @@
 //! [BIP-380]: <https://github.com/bitcoin/bips/blob/master/bip-0380.mediawiki>
 
 use core::convert::TryFrom;
-use core::fmt;
 use core::iter::FromIterator;
+use core::{array, fmt};
 
 use bech32::primitives::checksum::PackedFe32;
 use bech32::{Checksum, Fe32};
@@ -115,10 +115,10 @@ pub fn verify_checksum(s: &str) -> Result<&str, Error> {
         eng.input_unchecked(s[..last_hash_pos].as_bytes());
 
         let expected = eng.checksum_chars();
-        let mut actual = ['_'; CHECKSUM_LENGTH];
-        for (act, ch) in actual.iter_mut().zip(checksum_str.chars()) {
-            *act = ch;
-        }
+
+        let mut iter = checksum_str.chars();
+        let actual: [char; CHECKSUM_LENGTH] =
+            array::from_fn(|_| iter.next().expect("length checked above"));
 
         if expected != actual {
             return Err(Error::InvalidChecksum { actual, expected });

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};
@@ -981,17 +982,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)
     }
 }
 
@@ -1102,7 +1103,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]))
@@ -1545,6 +1546,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");
@@ -1836,9 +1852,10 @@ mod tests {
         // https://github.com/bitcoin/bitcoin/blob/7ae86b3c6845873ca96650fc69beb4ae5285c801/src/test/descriptor_tests.cpp#L355-L360
         macro_rules! check_invalid_checksum {
             ($secp: ident,$($desc: expr),*) => {
+                use crate::{ParseError, ParseTreeError};
                 $(
                     match Descriptor::parse_descriptor($secp, $desc) {
-                        Err(Error::ParseTree(crate::ParseTreeError::Checksum(_))) => {},
+                        Err(Error::Parse(ParseError::Tree(ParseTreeError::Checksum(_)))) => {},
                         Err(e) => panic!("Expected bad checksum for {}, got '{}'", $desc, e),
                         _ => panic!("Invalid checksum treated as valid: {}", $desc),
                     };

src/descriptor/segwitv0.rs

@@ -248,21 +248,17 @@ 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(From::from)
+            .map_err(Error::Parse)?;
+
+        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 +484,11 @@ 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(From::from)
+            .map_err(Error::Parse)?;
+        Ok(Wpkh::new(expression::terminal(top, |pk| Pk::from_str(pk))?)?)
     }
 }
 

src/descriptor/sh.rs

@@ -82,26 +82,22 @@ 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(From::from)
+            .map_err(Error::Parse)?;
+
+        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,10 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
         Pk: FromStr,
         <Pk as FromStr>::Err: fmt::Display,
     {
+        tree.verify_toplevel("sortedmulti", 1..)
+            .map_err(From::from)
+            .map_err(Error::Parse)?;
+
         let ret = Self {
             inner: tree
                 .to_null_threshold()