milksad
/
rust-bitcoin-unsafe-fast
14
0
Fork 0
Code Issues Pull Requests Activity

Merge rust-bitcoin/rust-bitcoin#2417: Improve lock time errors 

93dba898c2 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 93dba898c2
  tcharding:
    ACK 93dba898c2

Tree-SHA512: 68b60b413b1a1a0fc3648970d37f43e8b1b79f197ded053d83cfc1cf4fab4bed77d77841c2ae4d066b6436ee7187723c5d8cf934193c04c03520e797b7f7e82d
This commit is contained in:
Andrew Poelstra 2024-01-31 20:52:00 +00:00
parent 34c4039963 93dba898c2
commit 8efaf4a300
No known key found for this signature in database
GPG Key ID: C588D63CE41B97C1
2 changed files with 160 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

162
bitcoin/src/blockdata/locktime/absolute.rs
View File

@ -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::*;

40
bitcoin/src/parse.rs
View File

@ -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;