Merge #778: cleanups: Eliminate errstr and (nearly) eliminate Unexpected

33a60e2 expression: pull MultiColon error into parsing logic, drop AtOutsideOr (Andrew Poelstra)
f7cb701 expression: add "illegal and/or" for thresholds; drop errstr (Andrew Poelstra)
b3d1b17 descriptor: eliminate several instances of Unexpected (Andrew Poelstra)
1467453 compiler: refactor out a call to errstr (Andrew Poelstra)
dd19874 expression: introduce "unknown name" error variant (Andrew Poelstra)
4ae5079 expression: replace methods for parsing numbers and locktimes (Andrew Poelstra)
ee15056 policy: remove now-unused semantic::PolicyError type (Andrew Poelstra)
ce3d3e8 expression: replace most uses of `terminal` (Andrew Poelstra)
8423557 expression: replace poorly-typed `binary` function with new one (Andrew Poelstra)

Pull request description:

  This PR is a series of commits which cleans up the expression parsing module. After the last couple PRs, which substantially rewrote the parser and introduce a new parsing-error module, we can get rid of many uses of the `Error::Unexpected` variant and its constructor, the `errstr` function.

  This PR should have no visible effects, and does not even change any algorithms. The next one will return to the process of rewriting the expression parser, by replacing the recursive `Tree` type with a non-recursive one.

  Will post benchmarks once they are done.

ACKs for top commit:
  sanket1729:
    ACK 33a60e2

Tree-SHA512: 09927b83f84baa3593af22e95b84aa38f65851e6e005fb53f02cbb23edc7e2275345727399d70a252ccd1f8462707666b91fc043f613d2998fa8a51204525a9e

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

@@ -27,8 +27,8 @@ use crate::miniscript::{satisfy, Legacy, Miniscript, Segwitv0};
 use crate::plan::{AssetProvider, Plan};
 use crate::prelude::*;
 use crate::{
-    expression, hash256, BareCtx, Error, ForEachKey, FromStrKey, MiniscriptKey, Satisfier,
-    ToPublicKey, TranslateErr, Translator,
+    expression, hash256, BareCtx, Error, ForEachKey, FromStrKey, MiniscriptKey, ParseError,
+    Satisfier, ToPublicKey, TranslateErr, Translator,
 };
 
 mod bare;
@@ -715,8 +715,9 @@ impl Descriptor<DescriptorPublicKey> {
                     Some(sk),
                 ),
                 Err(_) => (
-                    DescriptorPublicKey::from_str(s)
-                        .map_err(|e| Error::Unexpected(e.to_string()))?,
+                    // try to parse as a public key if parsing as a secret key failed
+                    s.parse()
+                        .map_err(|e| Error::Parse(ParseError::box_from_str(e)))?,
                     None,
                 ),
             };
@@ -741,37 +742,34 @@ impl Descriptor<DescriptorPublicKey> {
             }
 
             fn sha256(&mut self, sha256: &String) -> Result<sha256::Hash, Error> {
-                let hash =
-                    sha256::Hash::from_str(sha256).map_err(|e| Error::Unexpected(e.to_string()))?;
-                Ok(hash)
+                sha256
+                    .parse()
+                    .map_err(|e| Error::Parse(ParseError::box_from_str(e)))
             }
 
             fn hash256(&mut self, hash256: &String) -> Result<hash256::Hash, Error> {
-                let hash = hash256::Hash::from_str(hash256)
-                    .map_err(|e| Error::Unexpected(e.to_string()))?;
-                Ok(hash)
+                hash256
+                    .parse()
+                    .map_err(|e| Error::Parse(ParseError::box_from_str(e)))
             }
 
             fn ripemd160(&mut self, ripemd160: &String) -> Result<ripemd160::Hash, Error> {
-                let hash = ripemd160::Hash::from_str(ripemd160)
-                    .map_err(|e| Error::Unexpected(e.to_string()))?;
-                Ok(hash)
+                ripemd160
+                    .parse()
+                    .map_err(|e| Error::Parse(ParseError::box_from_str(e)))
             }
 
             fn hash160(&mut self, hash160: &String) -> Result<hash160::Hash, Error> {
-                let hash = hash160::Hash::from_str(hash160)
-                    .map_err(|e| Error::Unexpected(e.to_string()))?;
-                Ok(hash)
+                hash160
+                    .parse()
+                    .map_err(|e| Error::Parse(ParseError::box_from_str(e)))
             }
         }
 
         let descriptor = Descriptor::<String>::from_str(s)?;
-        let descriptor = descriptor.translate_pk(&mut keymap_pk).map_err(|e| {
-            Error::Unexpected(
-                e.expect_translator_err("No Outer context errors")
-                    .to_string(),
-            )
-        })?;
+        let descriptor = descriptor
+            .translate_pk(&mut keymap_pk)
+            .map_err(|e| e.expect_translator_err("No Outer context errors"))?;
 
         Ok((descriptor, keymap_pk.0))
     }

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/error.rs

@@ -8,16 +8,25 @@ use core::fmt;
 use std::error;
 
 use crate::blanket_traits::StaticDebugAndDisplay;
+use crate::primitives::absolute_locktime::AbsLockTimeError;
+use crate::primitives::relative_locktime::RelLockTimeError;
 use crate::Box;
+
 /// An error parsing a Miniscript object (policy, descriptor or miniscript)
 /// from a string.
 #[derive(Debug)]
 pub enum ParseError {
+    /// Invalid absolute locktime
+    AbsoluteLockTime(AbsLockTimeError),
+    /// Invalid absolute locktime
+    RelativeLockTime(RelLockTimeError),
     /// 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>),
+    /// Failed to parse a number.
+    Num(crate::ParseNumError),
     /// Error parsing a string into an expression tree.
     Tree(crate::ParseTreeError),
 }
@@ -29,14 +38,21 @@ impl ParseError {
     }
 }
 
+impl From<crate::ParseNumError> for ParseError {
+    fn from(e: crate::ParseNumError) -> Self { Self::Num(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::AbsoluteLockTime(ref e) => e.fmt(f),
+            ParseError::RelativeLockTime(ref e) => e.fmt(f),
             ParseError::FromStr(ref e) => e.fmt(f),
+            ParseError::Num(ref e) => e.fmt(f),
             ParseError::Tree(ref e) => e.fmt(f),
         }
     }
@@ -46,7 +62,10 @@ impl fmt::Display for ParseError {
 impl error::Error for ParseError {
     fn source(&self) -> Option<&(dyn error::Error + 'static)> {
         match self {
+            ParseError::AbsoluteLockTime(ref e) => Some(e),
+            ParseError::RelativeLockTime(ref e) => Some(e),
             ParseError::FromStr(..) => None,
+            ParseError::Num(ref e) => Some(e),
             ParseError::Tree(ref e) => Some(e),
         }
     }

src/expression/error.rs

@@ -2,7 +2,7 @@
 
 //! Expression-related errors
 
-use core::fmt;
+use core::{fmt, num};
 
 use crate::descriptor::checksum;
 use crate::prelude::*;
@@ -76,13 +76,25 @@ pub enum ParseTreeError {
         /// The position of the opening curly brace.
         pos: usize,
     },
+    ///  Multiple separators (':' or '@') appeared in a node name.
+    MultipleSeparators {
+        /// The separator in question.
+        separator: char,
+        /// The position of the second separator.
+        pos: usize,
+    },
     /// Data occurred after the final ).
     TrailingCharacter {
         /// The first trailing character.
         ch: char,
         /// Its byte-index into the string.
         pos: usize,
     },
+    /// A node's name was not recognized.
+    UnknownName {
+        /// The name that was not recognized.
+        name: String,
+    },
 }
 
 impl From<checksum::Error> for ParseTreeError {
@@ -144,9 +156,17 @@ impl fmt::Display for ParseTreeError {
                 }?;
                 write!(f, ", but found {}", n_children)
             }
+            ParseTreeError::MultipleSeparators { separator, pos } => {
+                write!(
+                    f,
+                    "separator '{}' occured multiple times (second time at position {})",
+                    separator, pos
+                )
+            }
             ParseTreeError::TrailingCharacter { ch, pos } => {
                 write!(f, "trailing data `{}...` (position {})", ch, pos)
             }
+            ParseTreeError::UnknownName { name } => write!(f, "unrecognized name '{}'", name),
         }
     }
 }
@@ -163,7 +183,39 @@ impl std::error::Error for ParseTreeError {
             | ParseTreeError::IllegalCurlyBrace { .. }
             | ParseTreeError::IncorrectName { .. }
             | ParseTreeError::IncorrectNumberOfChildren { .. }
-            | ParseTreeError::TrailingCharacter { .. } => None,
+            | ParseTreeError::MultipleSeparators { .. }
+            | ParseTreeError::TrailingCharacter { .. }
+            | ParseTreeError::UnknownName { .. } => None,
+        }
+    }
+}
+
+/// Error parsing a number.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum ParseNumError {
+    /// Failed to parse the number at all.
+    StdParse(num::ParseIntError),
+    /// Number had a leading zero, + or -.
+    InvalidLeadingDigit(char),
+}
+
+impl fmt::Display for ParseNumError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            ParseNumError::StdParse(ref e) => e.fmt(f),
+            ParseNumError::InvalidLeadingDigit(ch) => {
+                write!(f, "numbers must start with 1-9, not {}", ch)
+            }
+        }
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for ParseNumError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        match self {
+            ParseNumError::StdParse(ref e) => Some(e),
+            ParseNumError::InvalidLeadingDigit(..) => None,
         }
     }
 }
@@ -175,10 +227,12 @@ pub enum ParseThresholdError {
     NoChildren,
     /// The threshold value appeared to be a sub-expression rather than a number.
     KNotTerminal,
+    /// A 1-of-n threshold was used in a context it was not allowed.
+    IllegalOr,
+    /// A n-of-n threshold was used in a context it was not allowed.
+    IllegalAnd,
     /// Failed to parse the threshold value.
-    // FIXME this should be a more specific type. Will be handled in a later PR
-    // that rewrites the expression parsing logic.
-    ParseK(String),
+    ParseK(ParseNumError),
     /// Threshold parameters were invalid.
     Threshold(ThresholdError),
 }
@@ -190,7 +244,13 @@ impl fmt::Display for ParseThresholdError {
         match *self {
             NoChildren => f.write_str("expected threshold, found terminal"),
             KNotTerminal => f.write_str("expected positive integer, found expression"),
-            ParseK(ref x) => write!(f, "failed to parse threshold value {}", x),
+            IllegalOr => f.write_str(
+                "1-of-n thresholds not allowed here; please use an 'or' fragment instead",
+            ),
+            IllegalAnd => f.write_str(
+                "n-of-n thresholds not allowed here; please use an 'and' fragment instead",
+            ),
+            ParseK(ref x) => write!(f, "failed to parse threshold value: {}", x),
             Threshold(ref e) => e.fmt(f),
         }
     }
@@ -202,9 +262,8 @@ impl std::error::Error for ParseThresholdError {
         use ParseThresholdError::*;
 
         match *self {
-            NoChildren => None,
-            KNotTerminal => None,
-            ParseK(..) => None,
+            NoChildren | KNotTerminal | IllegalOr | IllegalAnd => None,
+            ParseK(ref e) => Some(e),
             Threshold(ref e) => Some(e),
         }
     }