From b873a3cd44a34bc24b2971cb97bff6a3a8060ebb Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Tue, 27 Feb 2024 10:03:58 +1100 Subject: [PATCH] Do infallible int from hex conversions We have three integer wrapping types that can be created from hex strings where the conversion from an integer is infallible: - `absolute::LockTime` - `Sequence` - `CompactTarget` We would like to improve our handling of the two prefix characters (eg 0x) by making it explicit. - Modify the inherent `from_hex` method on each type to error if the input string does not contain a prefix. - Add an additional inherent method on each type `from_unprefixed_hex` that errors if the input string does contain a prefix. This patch does not touch the wrapper types that cannot be infallibly constructed from an integer (i.e. absolute `Height` and `Time`). --- bitcoin/src/blockdata/locktime/absolute.rs | 76 ++++++++----- bitcoin/src/blockdata/transaction.rs | 46 ++++++-- bitcoin/src/error.rs | 125 +++++++++++++++++++++ bitcoin/src/pow.rs | 53 ++++++--- 4 files changed, 252 insertions(+), 48 deletions(-) diff --git a/bitcoin/src/blockdata/locktime/absolute.rs b/bitcoin/src/blockdata/locktime/absolute.rs index fa7be4ef..ec1451a0 100644 --- a/bitcoin/src/blockdata/locktime/absolute.rs +++ b/bitcoin/src/blockdata/locktime/absolute.rs @@ -17,7 +17,7 @@ use mutagen::mutate; #[cfg(doc)] use crate::absolute; use crate::consensus::encode::{self, Decodable, Encodable}; -use crate::error::ParseIntError; +use crate::error::{ParseIntError, PrefixedHexError, UnprefixedHexError, ContainsPrefixError, MissingPrefixError}; use crate::parse::{self, impl_parse_str, impl_parse_str_from_int_infallible}; use crate::prelude::*; @@ -101,10 +101,25 @@ impl LockTime { /// The number of bytes that the locktime contributes to the size of a transaction. pub const SIZE: usize = 4; // Serialized length of a u32. - /// Creates a `LockTime` from a hex string. - /// - /// The input string is may or may not contain a typical hex prefix e.g., `0x`. - pub fn from_hex(s: &str) -> Result { + /// Creates a `LockTime` from an prefixed hex string. + pub fn from_hex(s: &str) -> Result { + let stripped = if let Some(stripped) = s.strip_prefix("0x") { + stripped + } else if let Some(stripped) = s.strip_prefix("0X") { + stripped + } else { + return Err(MissingPrefixError::new(s).into()); + }; + + let lock_time = parse::hex_u32(stripped)?; + Ok(Self::from_consensus(lock_time)) + } + + /// Creates a `LockTime` from an unprefixed hex string. + pub fn from_unprefixed_hex(s: &str) -> Result { + if s.starts_with("0x") || s.starts_with("0X") { + return Err(ContainsPrefixError::new(s).into()); + } let lock_time = parse::hex_u32(s)?; Ok(Self::from_consensus(lock_time)) } @@ -808,6 +823,37 @@ mod tests { assert_eq!(got, "block-height 741521"); } + #[test] + fn lock_time_from_hex_lower() { + let lock = LockTime::from_hex("0x6289c350").unwrap(); + assert_eq!(lock, LockTime::from_consensus(0x6289C350)); + } + + #[test] + fn lock_time_from_hex_upper() { + let lock = LockTime::from_hex("0X6289C350").unwrap(); + assert_eq!(lock, LockTime::from_consensus(0x6289C350)); + } + + #[test] + fn lock_time_from_unprefixed_hex_lower() { + let lock = LockTime::from_unprefixed_hex("6289c350").unwrap(); + assert_eq!(lock, LockTime::from_consensus(0x6289C350)); + } + + #[test] + fn lock_time_from_unprefixed_hex_upper() { + let lock = LockTime::from_unprefixed_hex("6289C350").unwrap(); + assert_eq!(lock, LockTime::from_consensus(0x6289C350)); + } + + #[test] + fn lock_time_from_invalid_hex_should_err() { + let hex = "0xzb93"; + let result = LockTime::from_hex(hex); + assert!(result.is_err()); + } + #[test] fn time_from_str_hex_happy_path() { let actual = Time::from_hex("0x6289C350").unwrap(); @@ -828,26 +874,6 @@ mod tests { assert!(result.is_err()); } - #[test] - fn lock_time_from_str_hex_happy_path() { - let actual = LockTime::from_hex("0xBA70D").unwrap(); - let expected = LockTime::from_consensus(0xBA70D); - assert_eq!(actual, expected); - } - - #[test] - fn lock_time_from_str_hex_no_prefix_happy_path() { - let lock_time = LockTime::from_hex("BA70D").unwrap(); - assert_eq!(lock_time, LockTime::from_consensus(0xBA70D)); - } - - #[test] - fn lock_time_from_str_hex_invalid_hex_should_ergr() { - let hex = "0xzb93"; - let result = LockTime::from_hex(hex); - assert!(result.is_err()); - } - #[test] fn height_from_str_hex_happy_path() { let actual = Height::from_hex("0xBA70D").unwrap(); diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index c0a7c023..94e3131a 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -24,8 +24,9 @@ use crate::blockdata::script::{Script, ScriptBuf}; use crate::blockdata::witness::Witness; use crate::blockdata::FeeRate; use crate::consensus::{encode, Decodable, Encodable}; +use crate::error::{PrefixedHexError, UnprefixedHexError, ContainsPrefixError, MissingPrefixError}; use crate::internal_macros::{impl_consensus_encoding, impl_hashencode}; -use crate::parse::{self, impl_parse_str_from_int_infallible, ParseIntError}; +use crate::parse::{self, impl_parse_str_from_int_infallible}; #[cfg(doc)] use crate::sighash::{EcdsaSighashType, TapSighashType}; use crate::prelude::*; @@ -401,10 +402,25 @@ impl Sequence { self.is_relative_lock_time() & (self.0 & Sequence::LOCK_TYPE_MASK > 0) } - /// Creates a `Sequence` number from a hex string. - /// - /// The input string may or may not contain a typical hex prefix e.g., `0x`. - pub fn from_hex(s: &str) -> Result { + /// Creates a `Sequence` from an prefixed hex string. + pub fn from_hex(s: &str) -> Result { + let stripped = if let Some(stripped) = s.strip_prefix("0x") { + stripped + } else if let Some(stripped) = s.strip_prefix("0X") { + stripped + } else { + return Err(MissingPrefixError::new(s).into()); + }; + + let sequence = parse::hex_u32(stripped)?; + Ok(Self::from_consensus(sequence)) + } + + /// Creates a `Sequence` from an unprefixed hex string. + pub fn from_unprefixed_hex(s: &str) -> Result { + if s.starts_with("0x") || s.starts_with("0X") { + return Err(ContainsPrefixError::new(s).into()); + } let lock_time = parse::hex_u32(s)?; Ok(Self::from_consensus(lock_time)) } @@ -2117,14 +2133,26 @@ mod tests { } #[test] - fn sequence_from_str_hex_happy_path() { - let sequence = Sequence::from_hex("0xFFFFFFFF").unwrap(); + fn sequence_from_hex_lower() { + let sequence = Sequence::from_hex("0xffffffff").unwrap(); assert_eq!(sequence, Sequence::MAX); } #[test] - fn sequence_from_str_hex_no_prefix_happy_path() { - let sequence = Sequence::from_hex("FFFFFFFF").unwrap(); + fn sequence_from_hex_upper() { + let sequence = Sequence::from_hex("0XFFFFFFFF").unwrap(); + assert_eq!(sequence, Sequence::MAX); + } + + #[test] + fn sequence_from_unprefixed_hex_lower() { + let sequence = Sequence::from_unprefixed_hex("ffffffff").unwrap(); + assert_eq!(sequence, Sequence::MAX); + } + + #[test] + fn sequence_from_unprefixed_hex_upper() { + let sequence = Sequence::from_unprefixed_hex("FFFFFFFF").unwrap(); assert_eq!(sequence, Sequence::MAX); } diff --git a/bitcoin/src/error.rs b/bitcoin/src/error.rs index 6adcbbb0..da9d6074 100644 --- a/bitcoin/src/error.rs +++ b/bitcoin/src/error.rs @@ -2,6 +2,131 @@ //! Contains error types and other error handling tools. +use core::fmt; + +use internals::write_err; + +use crate::prelude::*; + #[rustfmt::skip] // Keep public re-exports separate. #[doc(inline)] pub use crate::parse::ParseIntError; + +/// Error returned when parsing integer from an supposedly prefixed hex string for +/// a type that can be created infallibly from an integer. +#[derive(Debug, Clone, Eq, PartialEq)] +pub enum PrefixedHexError { + /// Hex string is missing prefix. + MissingPrefix(MissingPrefixError), + /// Error parsing integer from hex string. + ParseInt(ParseIntError), +} + +impl fmt::Display for PrefixedHexError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use PrefixedHexError::*; + + match *self { + MissingPrefix(ref e) => write_err!(f, "hex string is missing prefix"; e), + ParseInt(ref e) => write_err!(f, "prefixed hex string invalid int"; e), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for PrefixedHexError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use PrefixedHexError::*; + + match *self { + MissingPrefix(ref e) => Some(e), + ParseInt(ref e) => Some(e), + } + } +} + +impl From for PrefixedHexError { + fn from(e: MissingPrefixError) -> Self { Self::MissingPrefix(e) } +} + +impl From for PrefixedHexError { + fn from(e: ParseIntError) -> Self { Self::ParseInt(e) } +} + +/// Error returned when parsing integer from an supposedly un-prefixed hex string. +#[derive(Debug, Clone, Eq, PartialEq)] +pub enum UnprefixedHexError { + /// Hex string contains prefix. + ContainsPrefix(ContainsPrefixError), + /// Error parsing integer from string. + ParseInt(ParseIntError), +} + +impl fmt::Display for UnprefixedHexError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use UnprefixedHexError::*; + + match *self { + ContainsPrefix(ref e) => write_err!(f, "hex string is contains prefix"; e), + ParseInt(ref e) => write_err!(f, "hex string parse int"; e), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for UnprefixedHexError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use UnprefixedHexError::*; + + match *self { + ContainsPrefix(ref e) => Some(e), + ParseInt(ref e) => Some(e), + } + } +} + +impl From for UnprefixedHexError { + fn from(e: ContainsPrefixError) -> Self { Self::ContainsPrefix(e) } +} + +impl From for UnprefixedHexError { + fn from(e: ParseIntError) -> Self { Self::ParseInt(e) } +} + +/// Error when hex string is missing a prefix (e.g. 0x). +#[derive(Debug, Clone, Eq, PartialEq)] +pub struct MissingPrefixError { + hex: String, +} + +impl MissingPrefixError { + pub(crate) fn new(s: &str) -> Self { Self { hex: s.into() } } +} + +impl fmt::Display for MissingPrefixError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "hex string is missing a prefix (e.g. 0x): {}", self.hex) + } +} + +#[cfg(feature = "std")] +impl std::error::Error for MissingPrefixError {} + +/// Error when hex string contains a prefix (e.g. 0x). +#[derive(Debug, Clone, Eq, PartialEq)] +pub struct ContainsPrefixError { + hex: String, +} + +impl ContainsPrefixError { + pub(crate) fn new(s: &str) -> Self { Self { hex: s.into() } } +} + +impl fmt::Display for ContainsPrefixError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "hex string contains a prefix: {}", self.hex) + } +} + +#[cfg(feature = "std")] +impl std::error::Error for ContainsPrefixError {} diff --git a/bitcoin/src/pow.rs b/bitcoin/src/pow.rs index e098cbb7..3053d077 100644 --- a/bitcoin/src/pow.rs +++ b/bitcoin/src/pow.rs @@ -17,8 +17,8 @@ use crate::blockdata::block::BlockHash; use crate::consensus::encode::{self, Decodable, Encodable}; #[cfg(doc)] use crate::consensus::Params; -use crate::parse::{self, ParseIntError}; -use crate::Network; +use crate::error::{PrefixedHexError, UnprefixedHexError, ContainsPrefixError, MissingPrefixError}; +use crate::{parse, Network}; /// Implement traits and methods shared by `Target` and `Work`. macro_rules! do_impl { @@ -271,10 +271,25 @@ do_impl!(Target); pub struct CompactTarget(u32); impl CompactTarget { - /// Creates a [`CompactTarget`] from a hex string. - /// - /// The input string is may or may not contain a typical hex prefix e.g., `0x`. - pub fn from_hex(s: &str) -> Result { + /// Creates a `CompactTarget` from an prefixed hex string. + pub fn from_hex(s: &str) -> Result { + let stripped = if let Some(stripped) = s.strip_prefix("0x") { + stripped + } else if let Some(stripped) = s.strip_prefix("0X") { + stripped + } else { + return Err(MissingPrefixError::new(s).into()); + }; + + let target = parse::hex_u32(stripped)?; + Ok(Self::from_consensus(target)) + } + + /// Creates a `CompactTarget` from an unprefixed hex string. + pub fn from_unprefixed_hex(s: &str) -> Result { + if s.starts_with("0x") || s.starts_with("0X") { + return Err(ContainsPrefixError::new(s).into()); + } let lock_time = parse::hex_u32(s)?; Ok(Self::from_consensus(lock_time)) } @@ -1522,17 +1537,27 @@ mod tests { } #[test] - fn compact_target_from_hex_str_happy_path() { - let actual = CompactTarget::from_hex("0x01003456").unwrap(); - let expected = CompactTarget(0x01003456); - assert_eq!(actual, expected); + fn compact_target_from_hex_lower() { + let target = CompactTarget::from_hex("0x010034ab").unwrap(); + assert_eq!(target, CompactTarget(0x010034ab)); } #[test] - fn compact_target_from_hex_str_no_prefix_happy_path() { - let actual = CompactTarget::from_hex("01003456").unwrap(); - let expected = CompactTarget(0x01003456); - assert_eq!(actual, expected); + fn compact_target_from_hex_upper() { + let target = CompactTarget::from_hex("0X010034AB").unwrap(); + assert_eq!(target, CompactTarget(0x010034ab)); + } + + #[test] + fn compact_target_from_unprefixed_hex_lower() { + let target = CompactTarget::from_unprefixed_hex("010034ab").unwrap(); + assert_eq!(target, CompactTarget(0x010034ab)); + } + + #[test] + fn compact_target_from_unprefixed_hex_upper() { + let target = CompactTarget::from_unprefixed_hex("010034AB").unwrap(); + assert_eq!(target, CompactTarget(0x010034ab)); } #[test]