expression: replace methods for parsing numbers and locktimes

This eliminates another instance of `errstr` and provides well-typed
errors when parsing locktimes and threshold values.

It copies the AbsoluteLockTime and RelativeLockTime error variants from
the main Error enum into ParseError. The old copies are now used only in
script decoding, and will be removed in a later PR when we give script
decoding its own error.

Author: apoelstra

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::*;
@@ -168,6 +168,36 @@ impl std::error::Error for ParseTreeError {
     }
 }
 
+/// 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,
+        }
+    }
+}
+
 /// Error parsing a threshold expression.
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum ParseThresholdError {
@@ -176,9 +206,7 @@ pub enum ParseThresholdError {
     /// The threshold value appeared to be a sub-expression rather than a number.
     KNotTerminal,
     /// 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 +218,7 @@ 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),
+            ParseK(ref x) => write!(f, "failed to parse threshold value: {}", x),
             Threshold(ref e) => e.fmt(f),
         }
     }
@@ -204,7 +232,7 @@ impl std::error::Error for ParseThresholdError {
         match *self {
             NoChildren => None,
             KNotTerminal => None,
-            ParseK(..) => None,
+            ParseK(ref e) => Some(e),
             Threshold(ref e) => Some(e),
         }
     }

src/expression/mod.rs

@@ -5,15 +5,15 @@
 
 mod error;
 
+use core::ops;
 use core::str::FromStr;
-use core::{fmt, ops};
 
-pub use self::error::{ParseThresholdError, ParseTreeError};
+pub use self::error::{ParseNumError, ParseThresholdError, ParseTreeError};
 use crate::blanket_traits::StaticDebugAndDisplay;
 use crate::descriptor::checksum::verify_checksum;
 use crate::iter::{self, TreeLike};
 use crate::prelude::*;
-use crate::{errstr, Error, ParseError, Threshold, MAX_RECURSION_DEPTH};
+use crate::{AbsLockTime, Error, ParseError, RelLockTime, Threshold, MAX_RECURSION_DEPTH};
 
 /// Allowed characters are descriptor strings.
 pub const INPUT_CHARSET: &str = "0123456789()[],'/*abcdefgh@:$%{}IJKLMNOPQRSTUVWXYZ&+-.;<=>?!^_|~ijklmnopqrstuvwxyzABCDEFGH`#\"\\ ";
@@ -138,6 +138,38 @@ impl<'a> Tree<'a> {
         }
     }
 
+    /// Check that a tree node has a single terminal child which is an absolute locktime.
+    ///
+    /// Returns an error assuming that the node is named "after".
+    ///
+    /// If so, parse the locktime from a string and return it.
+    pub fn verify_after(&self) -> Result<AbsLockTime, ParseError> {
+        self.verify_n_children("after", 1..=1)
+            .map_err(ParseError::Tree)?;
+        self.args[0]
+            .verify_n_children("absolute locktime", 0..=0)
+            .map_err(ParseError::Tree)?;
+        parse_num(self.args[0].name)
+            .map_err(ParseError::Num)
+            .and_then(|n| AbsLockTime::from_consensus(n).map_err(ParseError::AbsoluteLockTime))
+    }
+
+    /// Check that a tree node has a single terminal child which is a relative locktime.
+    ///
+    /// Returns an error assuming that the node is named "older".
+    ///
+    /// If so, parse the locktime from a string and return it.
+    pub fn verify_older(&self) -> Result<RelLockTime, ParseError> {
+        self.verify_n_children("older", 1..=1)
+            .map_err(ParseError::Tree)?;
+        self.args[0]
+            .verify_n_children("relative locktime", 0..=0)
+            .map_err(ParseError::Tree)?;
+        parse_num(self.args[0].name)
+            .map_err(ParseError::Num)
+            .and_then(|n| RelLockTime::from_consensus(n).map_err(ParseError::RelativeLockTime))
+    }
+
     /// Check that a tree node is a terminal (has no children).
     ///
     /// If so, parse the terminal from a string and return it.
@@ -377,34 +409,23 @@ impl<'a> Tree<'a> {
             return Err(ParseThresholdError::KNotTerminal);
         }
 
-        let k = parse_num(self.args[0].name)
-            .map_err(|e| ParseThresholdError::ParseK(e.to_string()))? as usize;
+        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
-pub fn parse_num(s: &str) -> Result<u32, Error> {
-    if s.len() > 1 {
-        let ch = s.chars().next().unwrap();
+pub fn parse_num(s: &str) -> Result<u32, ParseNumError> {
+    if s == "0" {
+        // Special-case 0 since it is the only number which may start with a leading zero.
+        return Ok(0);
+    }
+    if let Some(ch) = s.chars().next() {
         if !('1'..='9').contains(&ch) {
-            return Err(Error::Unexpected("Number must start with a digit 1-9".to_string()));
+            return Err(ParseNumError::InvalidLeadingDigit(ch));
         }
     }
-    u32::from_str(s).map_err(|_| errstr(s))
-}
-
-/// Attempts to parse a terminal expression
-pub fn terminal<T, F, Err>(term: &Tree, convert: F) -> Result<T, Error>
-where
-    F: FnOnce(&str) -> Result<T, Err>,
-    Err: fmt::Display,
-{
-    if term.args.is_empty() {
-        convert(term.name).map_err(|e| Error::Unexpected(e.to_string()))
-    } else {
-        Err(errstr(term.name))
-    }
+    u32::from_str(s).map_err(ParseNumError::StdParse)
 }
 
 #[cfg(test)]

src/lib.rs

@@ -137,7 +137,7 @@ use bitcoin::{script, Opcode};
 pub use crate::blanket_traits::FromStrKey;
 pub use crate::descriptor::{DefiniteDescriptorKey, Descriptor, DescriptorPublicKey};
 pub use crate::error::ParseError;
-pub use crate::expression::{ParseThresholdError, ParseTreeError};
+pub use crate::expression::{ParseNumError, ParseThresholdError, ParseTreeError};
 pub use crate::interpreter::Interpreter;
 pub use crate::miniscript::analyzable::{AnalysisError, ExtParams};
 pub use crate::miniscript::context::{BareCtx, Legacy, ScriptContext, Segwitv0, SigType, Tap};

src/miniscript/astelem.rs

@@ -15,10 +15,7 @@ use crate::miniscript::context::SigType;
 use crate::miniscript::ScriptContext;
 use crate::prelude::*;
 use crate::util::MsKeyBuilder;
-use crate::{
-    expression, AbsLockTime, Error, FromStrKey, Miniscript, MiniscriptKey, RelLockTime, Terminal,
-    ToPublicKey,
-};
+use crate::{expression, Error, FromStrKey, Miniscript, MiniscriptKey, Terminal, ToPublicKey};
 
 impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Arc<Terminal<Pk, Ctx>> {
     fn from_tree(top: &expression::Tree) -> Result<Arc<Terminal<Pk, Ctx>>, Error> {
@@ -54,16 +51,14 @@ impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Termina
                 .verify_terminal_parent("pk_h", "public key")
                 .map(Terminal::PkH)
                 .map_err(Error::Parse),
-            "after" => expression::terminal(&top.args[0], |x| {
-                expression::parse_num(x)
-                    .and_then(|x| AbsLockTime::from_consensus(x).map_err(Error::AbsoluteLockTime))
-                    .map(Terminal::After)
-            }),
-            "older" => expression::terminal(&top.args[0], |x| {
-                expression::parse_num(x)
-                    .and_then(|x| RelLockTime::from_consensus(x).map_err(Error::RelativeLockTime))
-                    .map(Terminal::Older)
-            }),
+            "after" => top
+                .verify_after()
+                .map_err(Error::Parse)
+                .map(Terminal::After),
+            "older" => top
+                .verify_older()
+                .map_err(Error::Parse)
+                .map(Terminal::Older),
             "sha256" => top
                 .verify_terminal_parent("sha256", "hash")
                 .map(Terminal::Sha256)

src/policy/concrete.rs

@@ -867,7 +867,9 @@ impl<Pk: FromStrKey> Policy<Pk> {
                 if !allow_prob {
                     return Err(Error::AtOutsideOr(top.name.to_owned()));
                 }
-                frag_prob = expression::parse_num(prob)? as usize;
+                frag_prob = expression::parse_num(prob)
+                    .map_err(crate::ParseError::Num)
+                    .map_err(Error::Parse)? as usize;
                 frag_name = name;
             }
             (Some(_), Some(_), Some(_)) => {
@@ -891,16 +893,8 @@ impl<Pk: FromStrKey> Policy<Pk> {
                 .verify_terminal_parent("pk", "public key")
                 .map(Policy::Key)
                 .map_err(Error::Parse),
-            "after" => expression::terminal(&top.args[0], |x| {
-                expression::parse_num(x)
-                    .and_then(|x| AbsLockTime::from_consensus(x).map_err(Error::AbsoluteLockTime))
-                    .map(Policy::After)
-            }),
-            "older" => expression::terminal(&top.args[0], |x| {
-                expression::parse_num(x)
-                    .and_then(|x| RelLockTime::from_consensus(x).map_err(Error::RelativeLockTime))
-                    .map(Policy::Older)
-            }),
+            "after" => top.verify_after().map_err(Error::Parse).map(Policy::After),
+            "older" => top.verify_older().map_err(Error::Parse).map(Policy::Older),
             "sha256" => top
                 .verify_terminal_parent("sha256", "hash")
                 .map(Policy::Sha256)

src/policy/mod.rs

@@ -320,14 +320,14 @@ mod tests {
             ConcretePol::from_str("thresh(3,after(0),pk(),pk())")
                 .unwrap_err()
                 .to_string(),
-            "unexpected «absolute locktimes in Miniscript have a minimum value of 1»",
+            "absolute locktimes in Miniscript have a minimum value of 1",
         );
 
         assert_eq!(
             ConcretePol::from_str("thresh(2,older(2147483650),pk(),pk())")
                 .unwrap_err()
                 .to_string(),
-            "unexpected «locktime value 2147483650 is not a valid BIP68 relative locktime»"
+            "locktime value 2147483650 is not a valid BIP68 relative locktime"
         );
     }
 

src/policy/semantic.rs

@@ -302,16 +302,8 @@ impl<Pk: FromStrKey> expression::FromTree for Policy<Pk> {
                 .verify_terminal_parent("pk", "public key")
                 .map(Policy::Key)
                 .map_err(Error::Parse),
-            "after" => expression::terminal(&top.args[0], |x| {
-                expression::parse_num(x)
-                    .and_then(|x| AbsLockTime::from_consensus(x).map_err(Error::AbsoluteLockTime))
-                    .map(Policy::After)
-            }),
-            "older" => expression::terminal(&top.args[0], |x| {
-                expression::parse_num(x)
-                    .and_then(|x| RelLockTime::from_consensus(x).map_err(Error::RelativeLockTime))
-                    .map(Policy::Older)
-            }),
+            "after" => top.verify_after().map_err(Error::Parse).map(Policy::After),
+            "older" => top.verify_older().map_err(Error::Parse).map(Policy::Older),
             "sha256" => top
                 .verify_terminal_parent("sha256", "hash")
                 .map(Policy::Sha256)