From 4f68e79da0ceb40da2e369c65f1d2b197386f5cb Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 15 Feb 2024 16:26:55 +1100 Subject: [PATCH] bitcoin: Stop using base58 errors We are currently using the `base58::Error` type to create errors in `bitcoin`, these are bitcoin errors not `base58` errors. Note that we add what looks like duplicate `InvalidBase58PayloadLengthError` types but they are different because of the expected length. This could have been a field but I elected not to do so for two reasons: 1. We will need to do so anyways if we crate smash more 2. The `crypto::key` one can have one of two values 33 or 34. With this applied we can remove the now unused error variants from `base58::Error`. --- base58/src/error.rs | 17 ------- bitcoin/src/address/error.rs | 87 ++++++++++++++++++++++++++++++++++++ bitcoin/src/address/mod.rs | 11 +++-- bitcoin/src/bip32.rs | 33 +++++++++++++- bitcoin/src/crypto/key.rs | 76 +++++++++++++++++++++++++++---- 5 files changed, 193 insertions(+), 31 deletions(-) diff --git a/base58/src/error.rs b/base58/src/error.rs index 3e214a76..9f420f62 100644 --- a/base58/src/error.rs +++ b/base58/src/error.rs @@ -14,15 +14,6 @@ pub enum Error { Decode(InvalidCharacterError), /// Checksum was not correct (expected, actual). BadChecksum(u32, u32), - /// The length (in bytes) of the object was not correct. - /// - /// Note that if the length is excessively long the provided length may be an estimate (and the - /// checksum step may be skipped). - InvalidLength(usize), - /// Extended Key version byte(s) were not recognized. - InvalidExtendedKeyVersion([u8; 4]), - /// Address version byte were not recognized. - InvalidAddressVersion(u8), /// Checked data was less than 4 bytes. TooShort(usize), } @@ -37,11 +28,6 @@ impl fmt::Display for Error { Decode(ref e) => write_err!(f, "decode"; e), BadChecksum(exp, actual) => write!(f, "base58ck checksum {:#x} does not match expected {:#x}", actual, exp), - InvalidLength(ell) => write!(f, "length {} invalid for this base58 type", ell), - InvalidExtendedKeyVersion(ref v) => - write!(f, "extended key version {:#04x?} is invalid for this base58 type", v), - InvalidAddressVersion(ref v) => - write!(f, "address version {} is invalid for this base58 type", v), TooShort(_) => write!(f, "base58ck data not even long enough for a checksum"), } } @@ -55,9 +41,6 @@ impl std::error::Error for Error { match self { Decode(ref e) => Some(e), BadChecksum(_, _) - | InvalidLength(_) - | InvalidExtendedKeyVersion(_) - | InvalidAddressVersion(_) | TooShort(_) => None, } } diff --git a/bitcoin/src/address/error.rs b/bitcoin/src/address/error.rs index ac0073d5..b1757da6 100644 --- a/bitcoin/src/address/error.rs +++ b/bitcoin/src/address/error.rs @@ -137,6 +137,12 @@ pub enum ParseError { WitnessProgram(witness_program::Error), /// Tried to parse an unknown HRP. UnknownHrp(UnknownHrpError), + /// Legacy address is too long. + LegacyAddressTooLong(LegacyAddressTooLongError), + /// Invalid base58 payload data length for legacy address. + InvalidBase58PayloadLength(InvalidBase58PayloadLengthError), + /// Invalid legacy address prefix in base58 data payload. + InvalidLegacyPrefix(InvalidLegacyPrefixError), } internals::impl_from_infallible!(ParseError); @@ -151,6 +157,9 @@ impl fmt::Display for ParseError { WitnessVersion(ref e) => write_err!(f, "witness version conversion/parsing error"; e), WitnessProgram(ref e) => write_err!(f, "witness program error"; e), UnknownHrp(ref e) => write_err!(f, "tried to parse an unknown hrp"; e), + LegacyAddressTooLong(ref e) => write_err!(f, "legacy address base58 string"; e), + InvalidBase58PayloadLength(ref e) => write_err!(f, "legacy address base58 data"; e), + InvalidLegacyPrefix(ref e) => write_err!(f, "legacy address base58 prefix"; e), } } } @@ -166,6 +175,9 @@ impl std::error::Error for ParseError { WitnessVersion(ref e) => Some(e), WitnessProgram(ref e) => Some(e), UnknownHrp(ref e) => Some(e), + LegacyAddressTooLong(ref e) => Some(e), + InvalidBase58PayloadLength(ref e) => Some(e), + InvalidLegacyPrefix(ref e) => Some(e), } } } @@ -190,6 +202,18 @@ impl From for ParseError { fn from(e: UnknownHrpError) -> Self { Self::UnknownHrp(e) } } +impl From for ParseError { + fn from(e: LegacyAddressTooLongError) -> Self { Self::LegacyAddressTooLong(e) } +} + +impl From for ParseError { + fn from(e: InvalidBase58PayloadLengthError) -> Self { Self::InvalidBase58PayloadLength(e) } +} + +impl From for ParseError { + fn from(e: InvalidLegacyPrefixError) -> Self { Self::InvalidLegacyPrefix(e) } +} + /// Unknown HRP error. #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] @@ -203,3 +227,66 @@ impl fmt::Display for UnknownHrpError { impl std::error::Error for UnknownHrpError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None } } + +/// Decoded base58 data was an invalid length. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct InvalidBase58PayloadLengthError { + /// The base58 payload length we got after decoding address string. + pub(crate) length: usize, +} + +impl InvalidBase58PayloadLengthError { + /// Returns the invalid payload length. + pub fn invalid_base58_payload_length(&self) -> usize { self.length } +} + +impl fmt::Display for InvalidBase58PayloadLengthError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "decoded base58 data was an invalid length: {} (expected 21)", self.length) + } +} + +#[cfg(feature = "std")] +impl std::error::Error for InvalidBase58PayloadLengthError {} + +/// Legacy base58 address was too long, max 50 characters. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct LegacyAddressTooLongError { + /// The length of the legacy address. + pub(crate) length: usize, +} + +impl LegacyAddressTooLongError { + /// Returns the invalid legacy address length. + pub fn invalid_legcay_address_length(&self) -> usize { self.length } +} + +impl fmt::Display for LegacyAddressTooLongError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "legacy address is too long: {} (max 50 characters)", self.length) + } +} + +#[cfg(feature = "std")] +impl std::error::Error for LegacyAddressTooLongError {} + +/// Invalid legacy address prefix in decoded base58 data. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct InvalidLegacyPrefixError { + /// The invalid prefix byte. + pub(crate) invalid: u8, +} + +impl InvalidLegacyPrefixError { + /// Returns the invalid prefix. + pub fn invalid_legacy_address_prefix(&self) -> u8 { self.invalid } +} + +impl fmt::Display for InvalidLegacyPrefixError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "invalid legacy address prefix in decoded base58 data {}", self.invalid) + } +} + +#[cfg(feature = "std")] +impl std::error::Error for InvalidLegacyPrefixError {} diff --git a/bitcoin/src/address/mod.rs b/bitcoin/src/address/mod.rs index 64585902..df1c7906 100644 --- a/bitcoin/src/address/mod.rs +++ b/bitcoin/src/address/mod.rs @@ -53,7 +53,10 @@ use crate::taproot::TapNodeHash; #[rustfmt::skip] // Keep public re-exports separate. #[doc(inline)] pub use self::{ - error::{FromScriptError, NetworkValidationError, ParseError, P2shError, UnknownAddressTypeError, UnknownHrpError}, + error::{ + FromScriptError, InvalidBase58PayloadLengthError, InvalidLegacyPrefixError, LegacyAddressTooLongError, + NetworkValidationError, ParseError, P2shError, UnknownAddressTypeError, UnknownHrpError + }, }; /// The different types of addresses. @@ -732,11 +735,11 @@ impl FromStr for Address { // If segwit decoding fails, assume its a legacy address. if s.len() > 50 { - return Err(ParseError::Base58(base58::Error::InvalidLength(s.len() * 11 / 15))); + return Err(LegacyAddressTooLongError { length: s.len() }.into()); } let data = base58::decode_check(s)?; if data.len() != 21 { - return Err(ParseError::Base58(base58::Error::InvalidLength(data.len()))); + return Err(InvalidBase58PayloadLengthError { length: s.len() }.into()); } let (prefix, data) = data.split_first().expect("length checked above"); @@ -759,7 +762,7 @@ impl FromStr for Address { let hash = ScriptHash::from_byte_array(data); AddressInner::P2sh { hash, network: NetworkKind::Test } } - x => return Err(ParseError::Base58(base58::Error::InvalidAddressVersion(x))), + invalid => return Err(InvalidLegacyPrefixError { invalid }.into()), }; Ok(Address(inner, PhantomData)) diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index 22242f12..cb749a00 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -489,6 +489,8 @@ pub enum Error { Hex(hex::HexToArrayError), /// `PublicKey` hex should be 66 or 130 digits long. InvalidPublicKeyHexLength(usize), + /// Base58 decoded data was an invalid length. + InvalidBase58PayloadLength(InvalidBase58PayloadLengthError) } internals::impl_from_infallible!(Error); @@ -512,6 +514,7 @@ impl fmt::Display for Error { Hex(ref e) => write_err!(f, "Hexadecimal decoding error"; e), InvalidPublicKeyHexLength(got) => write!(f, "PublicKey hex should be 66 or 130 digits long, got: {}", got), + InvalidBase58PayloadLength(ref e) => write_err!(f, "base58 payload"; e), } } } @@ -525,6 +528,7 @@ impl std::error::Error for Error { Secp256k1(ref e) => Some(e), Base58(ref e) => Some(e), Hex(ref e) => Some(e), + InvalidBase58PayloadLength(ref e) => Some(e), CannotDeriveFromHardenedKey | InvalidChildNumber(_) | InvalidChildNumberFormat @@ -544,6 +548,10 @@ impl From for Error { fn from(err: base58::Error) -> Self { Error::Base58(err) } } +impl From for Error { + fn from(e: InvalidBase58PayloadLengthError) -> Error { Self::InvalidBase58PayloadLength(e) } +} + impl Xpriv { /// Construct a new master key from a seed value pub fn new_master(network: impl Into, seed: &[u8]) -> Result { @@ -828,7 +836,7 @@ impl FromStr for Xpriv { let data = base58::decode_check(inp)?; if data.len() != 78 { - return Err(base58::Error::InvalidLength(data.len()).into()); + return Err(InvalidBase58PayloadLengthError { length: data.len() }.into()); } Xpriv::decode(&data) @@ -848,7 +856,7 @@ impl FromStr for Xpub { let data = base58::decode_check(inp)?; if data.len() != 78 { - return Err(base58::Error::InvalidLength(data.len()).into()); + return Err(InvalidBase58PayloadLengthError { length: data.len() }.into()); } Xpub::decode(&data) @@ -863,6 +871,27 @@ impl From<&Xpub> for XKeyIdentifier { fn from(key: &Xpub) -> XKeyIdentifier { key.identifier() } } +/// Decoded base58 data was an invalid length. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct InvalidBase58PayloadLengthError { + /// The base58 payload length we got after decoding xpriv/xpub string. + pub(crate) length: usize, +} + +impl InvalidBase58PayloadLengthError { + /// Returns the invalid payload length. + pub fn invalid_base58_payload_length(&self) -> usize { self.length } +} + +impl fmt::Display for InvalidBase58PayloadLengthError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "decoded base58 xpriv/xpub data was an invalid length: {} (expected 78)", self.length) + } +} + +#[cfg(feature = "std")] +impl std::error::Error for InvalidBase58PayloadLengthError {} + #[cfg(test)] mod tests { use hex::test_hex_unwrap as hex; diff --git a/bitcoin/src/crypto/key.rs b/bitcoin/src/crypto/key.rs index a1f03480..23c2e20e 100644 --- a/bitcoin/src/crypto/key.rs +++ b/bitcoin/src/crypto/key.rs @@ -476,16 +476,16 @@ impl PrivateKey { let compressed = match data.len() { 33 => false, 34 => true, - _ => { - return Err(FromWifError::Base58(base58::Error::InvalidLength(data.len()))); + length => { + return Err(InvalidBase58PayloadLengthError { length }.into()); } }; let network = match data[0] { 128 => NetworkKind::Main, 239 => NetworkKind::Test, - x => { - return Err(FromWifError::Base58(base58::Error::InvalidAddressVersion(x))); + invalid => { + return Err(InvalidAddressVersionError { invalid }.into()); } }; @@ -930,8 +930,12 @@ impl From for FromSliceError { #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] pub enum FromWifError { - /// A base58 error. + /// A base58 decoding error. Base58(base58::Error), + /// Base58 decoded data was an invalid length. + InvalidBase58PayloadLength(InvalidBase58PayloadLengthError), + /// Base58 decoded data contained an invalid address version byte. + InvalidAddressVersion(InvalidAddressVersionError), /// A secp256k1 error. Secp256k1(secp256k1::Error), } @@ -941,8 +945,11 @@ internals::impl_from_infallible!(FromWifError); impl fmt::Display for FromWifError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { use FromWifError::*; + match *self { Base58(ref e) => write_err!(f, "invalid base58"; e), + InvalidBase58PayloadLength(ref e) => write_err!(f, "decoded base58 data was an invalid length"; e), + InvalidAddressVersion(ref e) => write_err!(f, "decoded base58 data contained an invalid address version btye"; e), Secp256k1(ref e) => write_err!(f, "private key validation failed"; e), } } @@ -952,9 +959,12 @@ impl fmt::Display for FromWifError { impl std::error::Error for FromWifError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { use FromWifError::*; - match self { - Base58(e) => Some(e), - Secp256k1(e)=> Some(e), + + match *self { + Base58(ref e) => Some(e), + InvalidBase58PayloadLength(ref e) => Some(e), + InvalidAddressVersion(ref e) => Some(e), + Secp256k1(ref e)=> Some(e), } } } @@ -967,6 +977,14 @@ impl From for FromWifError { fn from(e: secp256k1::Error) -> Self { Self::Secp256k1(e) } } +impl From for FromWifError { + fn from(e: InvalidBase58PayloadLengthError) -> FromWifError { Self::InvalidBase58PayloadLength(e) } +} + +impl From for FromWifError { + fn from(e: InvalidAddressVersionError) -> FromWifError { Self::InvalidAddressVersion(e) } +} + /// Error returned while constructing public key from string. #[derive(Debug, Clone, PartialEq, Eq)] pub enum ParsePublicKeyError { @@ -1064,6 +1082,48 @@ impl std::error::Error for UncompressedPublicKeyError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None } } +/// Decoded base58 data was an invalid length. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct InvalidBase58PayloadLengthError { + /// The base58 payload length we got after decoding WIF string. + pub(crate) length: usize, +} + +impl InvalidBase58PayloadLengthError { + /// Returns the invalid payload length. + pub fn invalid_base58_payload_length(&self) -> usize { self.length } +} + +impl fmt::Display for InvalidBase58PayloadLengthError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "decoded base58 data was an invalid length: {} (expected 33 or 34)", self.length) + } +} + +#[cfg(feature = "std")] +impl std::error::Error for InvalidBase58PayloadLengthError {} + +/// Invalid address version in decoded base58 data. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct InvalidAddressVersionError { + /// The invalid version. + pub(crate) invalid: u8, +} + +impl InvalidAddressVersionError { + /// Returns the invalid version. + pub fn invalid_address_version(&self) -> u8 { self.invalid } +} + +impl fmt::Display for InvalidAddressVersionError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "invalid address version in decoded base58 data {}", self.invalid) + } +} + +#[cfg(feature = "std")] +impl std::error::Error for InvalidAddressVersionError {} + #[cfg(test)] mod tests { use super::*;