expression: encapsulate threshold parsing

Our current expression API has a `to_null_threshold` method, which works
reasonably well but requires the caller translate the returned threshold
by accessing individual children.

We will later want to change the Tree represntation to make individual
child access inefficient. To do this, encapsulate the threshold
construction into a new verify_threshold.

This previously was not done because our messy error structures made it
very difficult to manage. But our error structures are less messy so
it's possible now, though a bit of a hack. (We map everything to the
global Error structure. Later we will do pretty-much the same thing
but with ParseError in place of Error, which will be more elegant.)

Author: apoelstra

src/descriptor/sortedmulti.rs

@@ -69,10 +69,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
 
         let ret = Self {
             inner: tree
-                .to_null_threshold()
-                .map_err(Error::ParseThreshold)?
-                .translate_by_index(|i| tree.args[i + 1].verify_terminal("public key"))
-                .map_err(Error::Parse)?,
+                .verify_threshold(|sub| sub.verify_terminal("public_key").map_err(Error::Parse))?,
             phantom: PhantomData,
         };
         ret.constructor_check()

src/expression/mod.rs

@@ -239,6 +239,43 @@ impl<'a> Tree<'a> {
         Ok((&self.args[0], &self.args[1]))
     }
 
+    /// Parses an expression tree as a threshold (a term with at least one child,
+    /// the first of which is a positive integer k).
+    ///
+    /// This sanity-checks that the threshold is well-formed (begins with a valid
+    /// threshold value, etc.) but does not parse the children of the threshold.
+    /// Instead it returns a threshold holding the empty type `()`, which is
+    /// constructed without any allocations, and expects the caller to convert
+    /// this to the "real" threshold type by calling [`Threshold::translate`].
+    ///
+    /// (An alternate API which does the conversion inline turned out to be
+    /// too messy; it needs to take a closure, have multiple generic parameters,
+    /// and be able to return multiple error types.)
+    pub fn verify_threshold<
+        const MAX: usize,
+        F: FnMut(&Self) -> Result<T, E>,
+        T,
+        E: From<ParseThresholdError>,
+    >(
+        &self,
+        mut map_child: F,
+    ) -> Result<Threshold<T, MAX>, E> {
+        // First, special case "no arguments" so we can index the first argument without panics.
+        if self.args.is_empty() {
+            return Err(ParseThresholdError::NoChildren.into());
+        }
+
+        if !self.args[0].args.is_empty() {
+            return Err(ParseThresholdError::KNotTerminal.into());
+        }
+
+        let k = parse_num(self.args[0].name).map_err(ParseThresholdError::ParseK)? as usize;
+        Threshold::new(k, vec![(); self.args.len() - 1])
+            .map_err(ParseThresholdError::Threshold)
+            .map_err(From::from)
+            .and_then(|thresh| thresh.translate_by_index(|i| map_child(&self.args[1 + i])))
+    }
+
     /// 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() {
@@ -403,34 +440,6 @@ impl<'a> Tree<'a> {
         args.reverse();
         Ok(Tree { name: &s[..node_name_end], children_pos, parens, args })
     }
-
-    /// Parses an expression tree as a threshold (a term with at least one child,
-    /// the first of which is a positive integer k).
-    ///
-    /// This sanity-checks that the threshold is well-formed (begins with a valid
-    /// threshold value, etc.) but does not parse the children of the threshold.
-    /// Instead it returns a threshold holding the empty type `()`, which is
-    /// constructed without any allocations, and expects the caller to convert
-    /// this to the "real" threshold type by calling [`Threshold::translate`].
-    ///
-    /// (An alternate API which does the conversion inline turned out to be
-    /// too messy; it needs to take a closure, have multiple generic parameters,
-    /// and be able to return multiple error types.)
-    pub fn to_null_threshold<const MAX: usize>(
-        &self,
-    ) -> Result<Threshold<(), MAX>, ParseThresholdError> {
-        // First, special case "no arguments" so we can index the first argument without panics.
-        if self.args.is_empty() {
-            return Err(ParseThresholdError::NoChildren);
-        }
-
-        if !self.args[0].args.is_empty() {
-            return Err(ParseThresholdError::KNotTerminal);
-        }
-
-        let k = parse_num(self.args[0].name).map_err(ParseThresholdError::ParseK)? as usize;
-        Threshold::new(k, vec![(); self.args.len() - 1]).map_err(ParseThresholdError::Threshold)
-    }
 }
 
 /// Parse a string as a u32, for timelocks or thresholds

src/lib.rs

@@ -480,6 +480,11 @@ pub enum Error {
     Parse(ParseError),
 }
 
+#[doc(hidden)] // will be removed when we remove Error
+impl From<ParseThresholdError> for Error {
+    fn from(e: ParseThresholdError) -> Self { Self::ParseThreshold(e) }
+}
+
 // https://github.com/sipa/miniscript/pull/5 for discussion on this number
 const MAX_RECURSION_DEPTH: u32 = 402;
 

src/miniscript/astelem.rs

@@ -122,27 +122,13 @@ impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Termina
             "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))
+                .verify_threshold(|sub| Miniscript::from_tree(sub).map(Arc::new))
                 .map(Terminal::Thresh),
             "multi" => top
-                .to_null_threshold()
-                .map_err(Error::ParseThreshold)?
-                .translate_by_index(|i| {
-                    top.args[1 + i]
-                        .verify_terminal("public key")
-                        .map_err(Error::Parse)
-                })
+                .verify_threshold(|sub| sub.verify_terminal("public_key").map_err(Error::Parse))
                 .map(Terminal::Multi),
             "multi_a" => top
-                .to_null_threshold()
-                .map_err(Error::ParseThreshold)?
-                .translate_by_index(|i| {
-                    top.args[1 + i]
-                        .verify_terminal("public key")
-                        .map_err(Error::Parse)
-                })
+                .verify_threshold(|sub| sub.verify_terminal("public_key").map_err(Error::Parse))
                 .map(Terminal::MultiA),
             x => Err(Error::Parse(crate::ParseError::Tree(crate::ParseTreeError::UnknownName {
                 name: x.to_owned(),

src/policy/concrete.rs

@@ -926,10 +926,8 @@ impl<Pk: FromStrKey> Policy<Pk> {
                 Ok(Policy::Or(subs))
             }
             "thresh" => top
-                .to_null_threshold()
-                .map_err(Error::ParseThreshold)?
-                .translate_by_index(|i| Policy::from_tree(&top.args[1 + i]).map(Arc::new))
-                .map(Policy::Thresh),
+                .verify_threshold(|sub| Self::from_tree(sub).map(Arc::new))
+                .map(Self::Thresh),
             x => Err(Error::Parse(crate::ParseError::Tree(crate::ParseTreeError::UnknownName {
                 name: x.to_owned(),
             }))),

src/policy/semantic.rs

@@ -343,7 +343,7 @@ impl<Pk: FromStrKey> expression::FromTree for Policy<Pk> {
                 Ok(Policy::Thresh(Threshold::new(1, subs).map_err(Error::Threshold)?))
             }
             "thresh" => {
-                let thresh = top.to_null_threshold().map_err(Error::ParseThreshold)?;
+                let thresh = top.verify_threshold(|sub| Self::from_tree(sub).map(Arc::new))?;
 
                 // thresh(1) and thresh(n) are disallowed in semantic policies
                 if thresh.is_or() {
@@ -353,9 +353,7 @@ impl<Pk: FromStrKey> expression::FromTree for Policy<Pk> {
                     return Err(Error::ParseThreshold(crate::ParseThresholdError::IllegalAnd));
                 }
 
-                thresh
-                    .translate_by_index(|i| Policy::from_tree(&top.args[1 + i]).map(Arc::new))
-                    .map(Policy::Thresh)
+                Ok(Policy::Thresh(thresh))
             }
             x => Err(Error::Parse(crate::ParseError::Tree(crate::ParseTreeError::UnknownName {
                 name: x.to_owned(),

src/primitives/threshold.rs

@@ -151,8 +151,8 @@ impl<T, const MAX: usize> Threshold<T, MAX> {
     /// Like [`Self::translate_ref`] but passes indices to the closure rather than internal data.
     ///
     /// This is useful in situations where the data to be translated exists outside of the
-    /// threshold itself, and the threshold data is irrelevant. In particular it is commonly
-    /// paired with [`crate::expression::Tree::to_null_threshold`].
+    /// threshold itself, and the threshold data is irrelevant. In particular it is used
+    /// within the `verify_threshold` method for expression trees.
     ///
     /// If the data to be translated comes from a post-order iterator, you may instead want
     /// [`Self::map_from_post_order_iter`].