Improve lock time errors

The errors returned from various lock time functions had several issues.
Among the obvious - `Error` being returned from all operations even when
some of its variants were unreachable, there were subtle issues around
error messages:

* `ParseIntError` didn't contain information whether the parsed object
   is `Height` or `Time`.
* Logically overflow and out-of-bounds should be the same thing but
  produced different error messages.
* Mentioning integers is too technical for a user, talking about upper
  and lower bound is easier to understand.
* When minus sign is present `std` reports it as invalid digit which is
  less helpful than saying negative numbers are not allowed.

It is also possible that `ParseIntError` will need to be removed from
public API during crate smashing or stabilization, so avoiding it may be
better.

This commit significantly refactors the errors. It adds separate types
for parsing `Height` and `Time`. Notice that we don't compose them from
`ParseIntError` and `ConversionError` - that's not helpful because they
carry information that wouldn't be used when displaying which is
wasteful. Keeping errors small can be important.

It's also worth noting that exposing the inner representation could
cause confusion since the same thing: out of bounds can be represented
as an overflow or as a conversion error. So for now we conservatively
hide the details and even pretend there's no `source` in case of
overflow. This can be expanded in the future if needed.

The returned errors are now minimal. `LockTime` parsing errors are
currentlly unchanged.

bitcoin/src/blockdata/locktime/absolute.rs

@@ -18,7 +18,7 @@ use mutagen::mutate;
 use crate::absolute;
 use crate::consensus::encode::{self, Decodable, Encodable};
 use crate::error::ParseIntError;
-use crate::parse::{impl_parse_str_from_int_fallible, impl_parse_str_from_int_infallible};
+use crate::parse::{impl_parse_str, impl_parse_str_from_int_infallible};
 use crate::prelude::*;
 use crate::string::FromHexStr;
 
@@ -134,7 +134,7 @@ impl LockTime {
     /// assert!(LockTime::from_height(1653195600).is_err());
     /// ```
     #[inline]
-    pub fn from_height(n: u32) -> Result<Self, Error> {
+    pub fn from_height(n: u32) -> Result<Self, ConversionError> {
         let height = Height::from_consensus(n)?;
         Ok(LockTime::Blocks(height))
     }
@@ -150,7 +150,7 @@ impl LockTime {
     /// assert!(LockTime::from_time(741521).is_err());
     /// ```
     #[inline]
-    pub fn from_time(n: u32) -> Result<Self, Error> {
+    pub fn from_time(n: u32) -> Result<Self, ConversionError> {
         let time = Time::from_consensus(n)?;
         Ok(LockTime::Seconds(time))
     }
@@ -326,7 +326,7 @@ impl fmt::Display for LockTime {
 }
 
 impl FromHexStr for LockTime {
-    type Error = Error;
+    type Error = ParseIntError;
 
     #[inline]
     fn from_hex_str_no_prefix<S: AsRef<str> + Into<String>>(s: S) -> Result<Self, Self::Error> {
@@ -434,11 +434,11 @@ impl Height {
     /// assert_eq!(height.to_consensus_u32(), h);
     /// ```
     #[inline]
-    pub fn from_consensus(n: u32) -> Result<Height, Error> {
+    pub fn from_consensus(n: u32) -> Result<Height, ConversionError> {
         if is_block_height(n) {
             Ok(Self(n))
         } else {
-            Err(ConversionError::invalid_height(n).into())
+            Err(ConversionError::invalid_height(n))
         }
     }
 
@@ -454,21 +454,51 @@ impl Height {
     /// assert_eq!(lock_time.to_consensus_u32(), n_lock_time);
     #[inline]
     pub fn to_consensus_u32(self) -> u32 { self.0 }
-}
 
-impl_parse_str_from_int_fallible!(Height, u32, from_consensus, Error);
+    /*
+    fn parse<S: AsRef<str> + Into<String>>(s: S) -> Result<Self, ParseError> {
+        let n = s.as_ref().parse::<i64>().map_err(ParseError::invalid_int(s))?
+        let n = u32::try_from(n).map_err(|_| ParseError::Conversion(n))?;
+        Self::from_consensus(n).map_err(Into::into)
+    }
+    */
+}
 
 impl fmt::Display for Height {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(&self.0, f) }
 }
 
 impl FromHexStr for Height {
-    type Error = Error;
+    type Error = ParseHeightError;
 
     #[inline]
     fn from_hex_str_no_prefix<S: AsRef<str> + Into<String>>(s: S) -> Result<Self, Self::Error> {
-        let height = crate::parse::hex_u32(s)?;
-        Self::from_consensus(height)
+        parse_hex(s, Self::from_consensus)
+    }
+}
+
+impl_parse_str!(Height, ParseHeightError, parser(Height::from_consensus));
+
+/// Error returned when parsing block height fails.
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub struct ParseHeightError(ParseError);
+
+impl fmt::Display for ParseHeightError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        self.0.display(f, "block height", 0, LOCK_TIME_THRESHOLD - 1)
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for ParseHeightError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        self.0.source()
+    }
+}
+
+impl From<ParseError> for ParseHeightError {
+    fn from(value: ParseError) -> Self {
+        Self(value)
     }
 }
 
@@ -504,11 +534,11 @@ impl Time {
     /// assert_eq!(time.to_consensus_u32(), t);
     /// ```
     #[inline]
-    pub fn from_consensus(n: u32) -> Result<Time, Error> {
+    pub fn from_consensus(n: u32) -> Result<Time, ConversionError> {
         if is_block_time(n) {
             Ok(Self(n))
         } else {
-            Err(ConversionError::invalid_time(n).into())
+            Err(ConversionError::invalid_time(n))
         }
     }
 
@@ -524,24 +554,67 @@ impl Time {
     /// ```
     #[inline]
     pub fn to_consensus_u32(self) -> u32 { self.0 }
-}
 
-impl_parse_str_from_int_fallible!(Time, u32, from_consensus, Error);
+    fn from_consensus_i64(n: i64) -> Result<Self, ParseError> {
+        let n = u32::try_from(n).map_err(|_| ParseError::Conversion(n))?;
+        Self::from_consensus(n).map_err(Into::into)
+    }
+}
 
 impl fmt::Display for Time {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(&self.0, f) }
 }
 
 impl FromHexStr for Time {
-    type Error = Error;
+    type Error = ParseTimeError;
 
     #[inline]
     fn from_hex_str_no_prefix<S: AsRef<str> + Into<String>>(s: S) -> Result<Self, Self::Error> {
-        let time = crate::parse::hex_u32(s)?;
-        Time::from_consensus(time)
+        let height = i64::from_str_radix(s.as_ref(), 16)
+            .map_err(ParseError::invalid_int(s))?;
+        Self::from_consensus_i64(height).map_err(Into::into)
     }
 }
 
+impl_parse_str!(Time, ParseTimeError, parser(Time::from_consensus));
+
+/// Error returned when parsing block time fails.
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub struct ParseTimeError(ParseError);
+
+impl fmt::Display for ParseTimeError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        self.0.display(f, "block height", LOCK_TIME_THRESHOLD, u32::MAX)
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for ParseTimeError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        self.0.source()
+    }
+}
+
+impl From<ParseError> for ParseTimeError {
+    fn from(value: ParseError) -> Self {
+        Self(value)
+    }
+}
+
+fn parser<T, E: From<ParseError>, S: AsRef<str> + Into<String>, F: FnOnce(u32) -> Result<T, ConversionError>>(f: F) -> impl FnOnce(S) -> Result<T, E> {
+    move |s| {
+        let n = s.as_ref().parse::<i64>().map_err(ParseError::invalid_int(s))?;
+        let n = u32::try_from(n).map_err(|_| ParseError::Conversion(n))?;
+        f(n).map_err(ParseError::from).map_err(Into::into)
+    }
+}
+
+fn parse_hex<T, E: From<ParseError>, S: AsRef<str> + Into<String>, F: FnOnce(u32) -> Result<T, ConversionError>>(s: S, f: F) -> Result<T, E> {
+    let n = i64::from_str_radix(s.as_ref(), 16).map_err(ParseError::invalid_int(s))?;
+    let n = u32::try_from(n).map_err(|_| ParseError::Conversion(n))?;
+    f(n).map_err(ParseError::from).map_err(Into::into)
+}
+
 /// Returns true if `n` is a block height i.e., less than 500,000,000.
 fn is_block_height(n: u32) -> bool { n < LOCK_TIME_THRESHOLD }
 
@@ -679,6 +752,74 @@ impl std::error::Error for OperationError {
     }
 }
 
+/// Internal - common representation for height and time.
+#[derive(Debug, Clone, Eq, PartialEq)]
+enum ParseError {
+    InvalidInteger { source: core::num::ParseIntError, input: String },
+    // unit implied by outer type
+    // we use i64 to have nicer messages for negative values
+    Conversion(i64),
+}
+
+impl ParseError {
+    fn invalid_int<S: Into<String>>(s: S) -> impl FnOnce(core::num::ParseIntError) -> Self {
+        move |source| Self::InvalidInteger { source, input: s.into() }
+    }
+
+    fn display(&self, f: &mut fmt::Formatter<'_>, subject: &str, lower_bound: u32, upper_bound: u32) -> fmt::Result {
+        use core::num::IntErrorKind;
+        match self {
+            ParseError::InvalidInteger { source, input } if *source.kind() == IntErrorKind::PosOverflow => {
+                write!(f, "{} {} is above limit {}", subject, input, upper_bound)
+            },
+            ParseError::InvalidInteger { source, input } if *source.kind() == IntErrorKind::NegOverflow => {
+                write!(f, "{} {} is below limit {}", subject, input, lower_bound)
+            },
+            ParseError::InvalidInteger { source, input } => {
+                write_err!(f, "failed to parse {} as {}", input, subject; source)
+            },
+            ParseError::Conversion(value) if *value < i64::from(lower_bound) => {
+                write!(f, "{} {} is below limit {}", subject, value, lower_bound)
+            },
+            ParseError::Conversion(value) => {
+                write!(f, "{} {} is above limit {}", subject, value, upper_bound)
+            },
+        }
+    }
+
+    // To be consistent with `write_err` we need to **not** return source in case of overflow
+    #[cfg(feature = "std")]
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        use core::num::IntErrorKind;
+        match self {
+            ParseError::InvalidInteger { source, .. } if *source.kind() == IntErrorKind::PosOverflow => {
+                None
+            },
+            ParseError::InvalidInteger { source, .. } if *source.kind() == IntErrorKind::NegOverflow => {
+                None
+            },
+            ParseError::InvalidInteger { source, .. } => {
+                Some(source)
+            },
+            ParseError::Conversion(_) => {
+                None
+            },
+        }
+    }
+}
+
+impl From<ParseIntError> for ParseError {
+    fn from(value: ParseIntError) -> Self {
+        Self::InvalidInteger { source: value.source, input: value.input }
+    }
+}
+
+impl From<ConversionError> for ParseError {
+    fn from(value: ConversionError) -> Self {
+        Self::Conversion(value.input.into())
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

bitcoin/src/parse.rs

@@ -19,14 +19,14 @@ use crate::prelude::*;
 #[derive(Debug, Clone, PartialEq, Eq)]
 #[non_exhaustive]
 pub struct ParseIntError {
-    input: String,
+    pub(crate) input: String,
     // for displaying - see Display impl with nice error message below
     bits: u8,
     // We could represent this as a single bit but it wouldn't actually derease the cost of moving
     // the struct because String contains pointers so there will be padding of bits at least
     // pointer_size - 1 bytes: min 1B in practice.
     is_signed: bool,
-    source: core::num::ParseIntError,
+    pub(crate) source: core::num::ParseIntError,
 }
 
 impl ParseIntError {
@@ -136,41 +136,33 @@ macro_rules! impl_parse_str_from_int_infallible {
 }
 pub(crate) use impl_parse_str_from_int_infallible;
 
-/// Implements `TryFrom<$from> for $to` using `parse::int`, mapping the output using fallible
-/// conversion function `fn`.
-macro_rules! impl_tryfrom_str_from_int_fallible {
-    ($($from:ty, $to:ident, $inner:ident, $fn:ident, $err:ident);*) => {
+macro_rules! impl_tryfrom_str {
+    ($($from:ty, $to:ty, $err:ty, $inner_fn:expr);*) => {
         $(
-        impl core::convert::TryFrom<$from> for $to {
-            type Error = $err;
+            impl core::convert::TryFrom<$from> for $to {
+                type Error = $err;
 
-            fn try_from(s: $from) -> core::result::Result<Self, Self::Error> {
-                let u = $crate::parse::int::<$inner, $from>(s)?;
-                $to::$fn(u)
+                fn try_from(s: $from) -> core::result::Result<Self, Self::Error> {
+                    $inner_fn(s)
+                }
             }
-        }
         )*
     }
 }
-pub(crate) use impl_tryfrom_str_from_int_fallible;
+pub(crate) use impl_tryfrom_str;
 
-/// Implements `FromStr` and `TryFrom<{&str, String, Box<str>}> for $to` using `parse::int`, mapping
-/// the output using fallible conversion function `fn`.
-///
-/// The `Error` type is `ParseIntError`
-macro_rules! impl_parse_str_from_int_fallible {
-    ($to:ident, $inner:ident, $fn:ident, $err:ident) => {
-        $crate::parse::impl_tryfrom_str_from_int_fallible!(&str, $to, $inner, $fn, $err; String, $to, $inner, $fn, $err; Box<str>, $to, $inner, $fn, $err);
+/// Implements standard parsing traits for `$type` by calling into `$inner_fn`.
+macro_rules! impl_parse_str {
+    ($to:ty, $err:ty, $inner_fn:expr) => {
+        $crate::parse::impl_tryfrom_str!(&str, $to, $err, $inner_fn; String, $to, $err, $inner_fn; Box<str>, $to, $err, $inner_fn);
 
         impl core::str::FromStr for $to {
             type Err = $err;
 
             fn from_str(s: &str) -> core::result::Result<Self, Self::Err> {
-                let u = $crate::parse::int::<$inner, &str>(s)?;
-                $to::$fn(u)
+                $inner_fn(s)
             }
         }
-
     }
 }
-pub(crate) use impl_parse_str_from_int_fallible;
+pub(crate) use impl_parse_str;