Merge rust-bitcoin/rust-miniscript#778: cleanups: Eliminate errstr and (nearly) eliminate Unexpected

33a60e29bd089cc14b9fd00f89e69903d2275a65 expression: pull MultiColon error into parsing logic, drop AtOutsideOr (Andrew Poelstra)
f7cb7013cc9bef027a853ffdb2c4744becdc1f07 expression: add "illegal and/or" for thresholds; drop errstr (Andrew Poelstra)
b3d1b171f9c3d13cf195dcea0c8a51545c4beff4 descriptor: eliminate several instances of Unexpected (Andrew Poelstra)
1467453d3999dcde2ac62ac82c61ca7fbc0b2a46 compiler: refactor out a call to errstr (Andrew Poelstra)
dd19874380e958132d541af4a74c5cca8f930cc2 expression: introduce "unknown name" error variant (Andrew Poelstra)
4ae5079ae3c9df0d80cabe6fc32fbaf0f9819cd7 expression: replace methods for parsing numbers and locktimes (Andrew Poelstra)
ee1505613693166beb8a11307d8fd1c3eca4fe48 policy: remove now-unused semantic::PolicyError type (Andrew Poelstra)
ce3d3e8b7b75a64df11700a13b1637670d0f70aa expression: replace most uses of `terminal` (Andrew Poelstra)
8423557514649c2403ffe5df18cf04d10ef6b32a 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 33a60e29bd089cc14b9fd00f89e69903d2275a65

Tree-SHA512: 09927b83f84baa3593af22e95b84aa38f65851e6e005fb53f02cbb23edc7e2275345727399d70a252ccd1f8462707666b91fc043f613d2998fa8a51204525a9e

Author: heap-coder

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),
         }
     }