Merge #2417: Improve lock time errors

93dba89 Improve lock time errors (Martin Habovstiak)

Pull request description:

  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.

  I can add `LockTime` changes in the same commit or separate within this PR if you want. Just wanted to push something for review before I go to sleep.

ACKs for top commit:
  apoelstra:
    ACK 93dba89
  tcharding:
    ACK 93dba89

Tree-SHA512: 68b60b413b1a1a0fc3648970d37f43e8b1b79f197ded053d83cfc1cf4fab4bed77d77841c2ae4d066b6436ee7187723c5d8cf934193c04c03520e797b7f7e82d

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))
         }
     }
 
@@ -456,22 +456,41 @@ impl Height {
     pub fn to_consensus_u32(self) -> u32 { self.0 }
 }
 
-impl_parse_str_from_int_fallible!(Height, u32, from_consensus, Error);
-
 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 {
+    // To be consistent with `write_err` we need to **not** return source in case of overflow
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { self.0.source() }
+}
+
+impl From<ParseError> for ParseHeightError {
+    fn from(value: ParseError) -> Self { Self(value) }
+}
+
 /// A UNIX timestamp, seconds since epoch, guaranteed to always contain a valid time value.
 ///
 /// Note that there is no manipulation of the inner value during construction or when using
@@ -504,11 +523,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))
         }
     }
 
@@ -526,22 +545,65 @@ impl Time {
     pub fn to_consensus_u32(self) -> u32 { self.0 }
 }
 
-impl_parse_str_from_int_fallible!(Time, u32, from_consensus, Error);
-
 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)
+        parse_hex(s, Self::from_consensus)
     }
 }
 
+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 {
+    // To be consistent with `write_err` we need to **not** return source in case of overflow
+    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, S, F>(f: F) -> impl FnOnce(S) -> Result<T, E>
+where
+    E: From<ParseError>,
+    S: AsRef<str> + Into<String>,
+    F: FnOnce(u32) -> Result<T, ConversionError>,
+{
+    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, S, F>(s: S, f: F) -> Result<T, E>
+where
+    E: From<ParseError>,
+    S: AsRef<str> + Into<String>,
+    F: FnOnce(u32) -> Result<T, ConversionError>,
+{
+    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 +741,70 @@ 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;
+
+        use ParseError::*;
+
+        match self {
+            InvalidInteger { source, input } if *source.kind() == IntErrorKind::PosOverflow => {
+                write!(f, "{} {} is above limit {}", subject, input, upper_bound)
+            }
+            InvalidInteger { source, input } if *source.kind() == IntErrorKind::NegOverflow => {
+                write!(f, "{} {} is below limit {}", subject, input, lower_bound)
+            }
+            InvalidInteger { source, input } => {
+                write_err!(f, "failed to parse {} as {}", input, subject; source)
+            }
+            Conversion(value) if *value < i64::from(lower_bound) => {
+                write!(f, "{} {} is below limit {}", subject, value, lower_bound)
+            }
+            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;
+
+        use ParseError::*;
+
+        match self {
+            InvalidInteger { source, .. } if *source.kind() == IntErrorKind::PosOverflow => None,
+            InvalidInteger { source, .. } if *source.kind() == IntErrorKind::NegOverflow => None,
+            InvalidInteger { source, .. } => Some(source),
+            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;