Merge #1129: Parse int error

071a1c0 Implement string parsing for `Sequence` (Martin Habovstiak)
c39bc39 Extend `ParseIntError` to carry more context (Martin Habovstiak)

Pull request description:

  When debugging parsing errors it's very useful to know some context:
  what the input was and what integer type was parsed. `ParseIntError`
  from `core` doesn't contain this information.

  In this commit a custom `ParseIntError` type is crated that carries the
  one from `core` as well as additional information. Its `Display`
  implementation displays this additional information as a well-formed
  English sentence to aid users with understanding the problem. A helper
  function parses any integer type from common string types returning the
  new `ParseIntError` type on error.

  To clean up the error code a bit some new macros are added and used.
  New modules are added to organize the types, functions and macros.

  Closes #1113

  Depends on #994

ACKs for top commit:
  apoelstra:
    ACK 071a1c0
  tcharding:
    ACK 071a1c0

Tree-SHA512: 31cb84b9e4d5fe3bdeb1cd48b85da2cbe9b9d17d93d029c2f95e0eed5b8842d7a553afafcf8b4a87c075aa53cf0274776e893bed6dca37e7dbc2e1ee1d602b8e

src/blockdata/locktime.rs

@@ -21,12 +21,14 @@ use core::{mem, fmt};
 use core::cmp::{PartialOrd, Ordering};
 use core::convert::TryFrom;
 use core::str::FromStr;
-use core::num::ParseIntError;
+use crate::error::ParseIntError;
+use crate::parse;
 
 use crate::consensus::encode::{self, Decodable, Encodable};
 use crate::io::{self, Read, Write};
 use crate::prelude::*;
 use crate::internal_macros::write_err;
+use crate::impl_parse_str_through_int;
 
 /// The Threshold for deciding whether a lock time value is a height or a time (see [Bitcoin Core]).
 ///
@@ -134,29 +136,7 @@ impl From<PackedLockTime> for u32 {
     }
 }
 
-impl FromStr for PackedLockTime {
-    type Err = ParseIntError;
-
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        s.parse().map(PackedLockTime)
-    }
-}
-
-impl TryFrom<&str> for PackedLockTime {
-    type Error = ParseIntError;
-
-    fn try_from(s: &str) -> Result<Self, Self::Error> {
-        PackedLockTime::from_str(s)
-    }
-}
-
-impl TryFrom<String> for PackedLockTime {
-    type Error = ParseIntError;
-
-    fn try_from(s: String) -> Result<Self, Self::Error> {
-        PackedLockTime::from_str(&s)
-    }
-}
+impl_parse_str_through_int!(PackedLockTime);
 
 impl fmt::LowerHex for PackedLockTime {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -366,29 +346,7 @@ impl LockTime {
     }
 }
 
-impl FromStr for LockTime {
-    type Err = ParseIntError;
-
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        s.parse().map(LockTime::from_consensus)
-    }
-}
-
-impl TryFrom<&str> for LockTime {
-    type Error = ParseIntError;
-
-    fn try_from(s: &str) -> Result<Self, Self::Error> {
-        LockTime::from_str(s)
-    }
-}
-
-impl TryFrom<String> for LockTime {
-    type Error = ParseIntError;
-
-    fn try_from(s: String) -> Result<Self, Self::Error> {
-        LockTime::from_str(&s)
-    }
-}
+impl_parse_str_through_int!(LockTime, from_consensus);
 
 impl From<Height> for LockTime {
     fn from(h: Height) -> Self {
@@ -503,7 +461,7 @@ impl FromStr for Height {
     type Err = Error;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        let n = s.parse::<u32>().map_err(|e| Error::Parse(e, s.to_owned()))?;
+        let n = parse::int(s)?;
         Height::from_consensus(n)
     }
 }
@@ -512,15 +470,16 @@ impl TryFrom<&str> for Height {
     type Error = Error;
 
     fn try_from(s: &str) -> Result<Self, Self::Error> {
-        Height::from_str(s)
+        let n = parse::int(s)?;
+        Height::from_consensus(n)
     }
 }
 
 impl TryFrom<String> for Height {
     type Error = Error;
 
     fn try_from(s: String) -> Result<Self, Self::Error> {
-        let n = s.parse::<u32>().map_err(|e| Error::Parse(e, s))?;
+        let n = parse::int(s)?;
         Height::from_consensus(n)
     }
 }
@@ -585,7 +544,7 @@ impl FromStr for Time {
     type Err = Error;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        let n = s.parse::<u32>().map_err(|e| Error::Parse(e, s.to_owned()))?;
+        let n = parse::int(s)?;
         Time::from_consensus(n)
     }
 }
@@ -594,15 +553,16 @@ impl TryFrom<&str> for Time {
     type Error = Error;
 
     fn try_from(s: &str) -> Result<Self, Self::Error> {
-        Time::from_str(s)
+        let n = parse::int(s)?;
+        Time::from_consensus(n)
     }
 }
 
 impl TryFrom<String> for Time {
     type Error = Error;
 
     fn try_from(s: String) -> Result<Self, Self::Error> {
-        let n = s.parse::<u32>().map_err(|e| Error::Parse(e, s))?;
+        let n = parse::int(s)?;
         Time::from_consensus(n)
     }
 }
@@ -626,7 +586,7 @@ pub enum Error {
     /// An error occurred while operating on lock times.
     Operation(OperationError),
     /// An error occurred while parsing a string into an `u32`.
-    Parse(ParseIntError, String),
+    Parse(ParseIntError),
 }
 
 impl fmt::Display for Error {
@@ -636,7 +596,7 @@ impl fmt::Display for Error {
         match *self {
             Conversion(ref e) => write_err!(f, "error converting lock time value"; e),
             Operation(ref e) => write_err!(f, "error during lock time operation"; e),
-            Parse(ref e, ref s) => write_err!(f, "error parsing string: {}", s; e),
+            Parse(ref e) => write_err!(f, "failed to parse lock time from string"; e),
         }
     }
 }
@@ -650,7 +610,7 @@ impl std::error::Error for Error {
         match *self {
             Conversion(ref e) => Some(e),
             Operation(ref e) => Some(e),
-            Parse(ref e, _) => Some(e),
+            Parse(ref e) => Some(e),
         }
     }
 }
@@ -667,6 +627,12 @@ impl From<OperationError> for Error {
     }
 }
 
+impl From<ParseIntError> for Error {
+    fn from(e: ParseIntError) -> Self {
+        Error::Parse(e)
+    }
+}
+
 /// An error that occurs when converting a `u32` to a lock time variant.
 #[derive(Debug, Clone, Eq, PartialEq, Hash)]
 pub struct ConversionError {

src/blockdata/transaction.rs

@@ -32,6 +32,7 @@ use crate::hash_types::{Sighash, Txid, Wtxid};
 use crate::VarInt;
 use crate::util::sighash::UINT256_ONE;
 use crate::internal_macros::{impl_consensus_encoding, serde_string_impl, serde_struct_human_string_impl, write_err};
+use crate::impl_parse_str_through_int;
 
 #[cfg(doc)]
 use crate::util::sighash::SchnorrSighashType;
@@ -107,7 +108,7 @@ pub enum ParseOutPointError {
     /// Error in TXID part.
     Txid(hashes::hex::Error),
     /// Error in vout part.
-    Vout(::core::num::ParseIntError),
+    Vout(crate::error::ParseIntError),
     /// Error in general format.
     Format,
     /// Size exceeds max.
@@ -151,7 +152,7 @@ fn parse_vout(s: &str) -> Result<u32, ParseOutPointError> {
             return Err(ParseOutPointError::VoutNotCanonical);
         }
     }
-    s.parse().map_err(ParseOutPointError::Vout)
+    crate::parse::int(s).map_err(ParseOutPointError::Vout)
 }
 
 impl core::str::FromStr for OutPoint {
@@ -422,6 +423,8 @@ impl fmt::Display for RelativeLockTimeError {
     }
 }
 
+impl_parse_str_through_int!(Sequence);
+
 #[cfg(feature = "std")]
 #[cfg_attr(docsrs, doc(cfg(feature = "std")))]
 impl std::error::Error for RelativeLockTimeError {
@@ -1308,7 +1311,7 @@ mod tests {
         assert_eq!(OutPoint::from_str("5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c945X:1"),
                    Err(ParseOutPointError::Txid(Txid::from_hex("5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c945X").unwrap_err())));
         assert_eq!(OutPoint::from_str("5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456:lol"),
-                   Err(ParseOutPointError::Vout(u32::from_str("lol").unwrap_err())));
+                   Err(ParseOutPointError::Vout(crate::parse::int::<u32, _>("lol").unwrap_err())));
 
         assert_eq!(OutPoint::from_str("5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456:42"),
                    Ok(OutPoint{

src/error.rs

@@ -0,0 +1,25 @@
+//! Contains error types and other error handling tools.
+
+pub use crate::parse::ParseIntError;
+
+/// Impls std::error::Error for the specified type with appropriate attributes, possibly returning
+/// source.
+#[macro_export]
+macro_rules! impl_std_error {
+    // No source available
+    ($type:ty) => {
+        #[cfg(feature = "std")]
+        #[cfg_attr(docsrs, doc(cfg(feature = "std")))]
+        impl std::error::Error for $type {}
+    };
+    // Struct with $field as source
+    ($type:ty, $field:ident) => {
+        #[cfg(feature = "std")]
+        #[cfg_attr(docsrs, doc(cfg(feature = "std")))]
+        impl std::error::Error for $type {
+            fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+                Some(&self.$field)
+            }
+        }
+    };
+}

src/internal_macros.rs

@@ -581,7 +581,7 @@ pub(crate) use user_enum;
 /// because `e.source()` is only available in std builds, without this macro the error source is
 /// lost for no-std builds.
 macro_rules! write_err {
-    ($writer:expr, $string:literal $(, $args:expr),*; $source:expr) => {
+    ($writer:expr, $string:literal $(, $args:expr)*; $source:expr) => {
         {
             #[cfg(feature = "std")]
             {

src/lib.rs

@@ -78,11 +78,13 @@ mod test_macros;
 mod internal_macros;
 #[cfg(feature = "serde")]
 mod serde_utils;
+mod parse;
 
 #[macro_use]
 pub mod network;
 pub mod blockdata;
 pub mod consensus;
+pub mod error;
 pub mod hash_types;
 pub mod policy;
 pub mod util;

src/parse.rs

@@ -0,0 +1,123 @@
+use crate::internal_macros::write_err;
+use crate::impl_std_error;
+use core::fmt;
+use core::str::FromStr;
+use core::convert::TryFrom;
+use crate::prelude::*;
+
+/// Error with rich context returned when a string can't be parsed as an integer.
+///
+/// This is an extension of [`core::num::ParseIntError`], which carries the input that failed to
+/// parse as well as type information. As a result it provides very informative error messages that
+/// make it easier to understand the problem and correct mistakes.
+///
+/// Note that this is larger than the type from `core` so if it's passed through a deep call stack
+/// in a performance-critical application you may want to box it or throw away the context by
+/// converting to `core` type.
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub struct ParseIntError {
+    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,
+}
+
+impl ParseIntError {
+    /// Returns the input that was attempted to be parsed.
+    pub fn input(&self) -> &str {
+        &self.input
+    }
+}
+
+impl From<ParseIntError> for core::num::ParseIntError {
+    fn from(value: ParseIntError) -> Self {
+        value.source
+    }
+}
+
+impl AsRef<core::num::ParseIntError> for ParseIntError {
+    fn as_ref(&self) -> &core::num::ParseIntError {
+        &self.source
+    }
+}
+
+impl fmt::Display for ParseIntError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        let signed = if self.is_signed { "signed" } else { "unsigned" };
+        let n = if self.bits == 8 { "n" } else { "" };
+        write_err!(f, "failed to parse '{}' as a{} {}-bit {} integer", self.input, n, self.bits, signed; self.source)
+    }
+}
+
+/// Not strictly neccessary but serves as a lint - avoids weird behavior if someone accidentally
+/// passes non-integer to the `parse()` function.
+pub(crate) trait Integer: FromStr<Err=core::num::ParseIntError> + TryFrom<i8> + Sized {}
+
+macro_rules! impl_integer {
+    ($($type:ty),* $(,)?) => {
+        $(
+        impl Integer for $type {}
+        )*
+    }
+}
+
+impl_integer!(u8, i8, u16, i16, u32, i32, u64, i64, u128, i128);
+
+/// Parses the input string as an integer returning an error carrying rich context.
+///
+/// If the caller owns `String` or `Box<str>` which is not used later it's better to pass it as
+/// owned since it avoids allocation in error case.
+pub(crate) fn int<T: Integer, S: AsRef<str> + Into<String>>(s: S) -> Result<T, ParseIntError> {
+    s.as_ref().parse().map_err(|error| {
+        ParseIntError {
+            input: s.into(),
+            bits: u8::try_from(core::mem::size_of::<T>() * 8).expect("max is 128 bits for u128"),
+            // We detect if the type is signed by checking if -1 can be represented by it
+            // this way we don't have to implement special traits and optimizer will get rid of the
+            // computation.
+            is_signed: T::try_from(-1i8).is_ok(),
+            source: error,
+        }
+    })
+}
+
+impl_std_error!(ParseIntError, source);
+
+/// Implements `TryFrom<$from> for $to` using `parse::int`, mapping the output using `fn`
+#[macro_export]
+macro_rules! impl_tryfrom_str_through_int_single {
+    ($($from:ty, $to:ident $(, $fn:ident)?);*) => {
+        $(
+        impl core::convert::TryFrom<$from> for $to {
+            type Error = $crate::error::ParseIntError;
+
+            fn try_from(s: $from) -> Result<Self, Self::Error> {
+                $crate::parse::int(s).map($to $(:: $fn)?)
+            }
+        }
+        )*
+    }
+}
+
+/// Implements `FromStr` and `TryFrom<{&str, String, Box<str>}> for $to` using `parse::int`, mapping the output using `fn`
+///
+/// The `Error` type is `ParseIntError`
+#[macro_export]
+macro_rules! impl_parse_str_through_int {
+    ($to:ident $(, $fn:ident)?) => {
+        $crate::impl_tryfrom_str_through_int_single!(&str, $to $(, $fn)?; String, $to $(, $fn)?; Box<str>, $to $(, $fn)?);
+
+        impl core::str::FromStr for $to {
+            type Err = $crate::error::ParseIntError;
+
+            fn from_str(s: &str) -> Result<Self, Self::Err> {
+                $crate::parse::int(s).map($to $(:: $fn)?)
+            }
+        }
+
+    }
+}