From 22c7aa880850d966d336193e9a4b91e1c3e05a9f Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 18 May 2023 15:11:26 +1000 Subject: [PATCH 01/10] Rename non standard sighash error type Error types conventionally include `Error` as a suffix. Rename `NonStandardSighashType` to `NonStandardSighashTypeError`. While we are at it make the inner type private to the crate, there is no need to leak the inner values type. --- bitcoin/src/crypto/ecdsa.rs | 6 +++--- bitcoin/src/crypto/sighash.rs | 10 +++++----- bitcoin/src/psbt/map/input.rs | 8 ++++---- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/bitcoin/src/crypto/ecdsa.rs b/bitcoin/src/crypto/ecdsa.rs index 8f295518..1bdb347d 100644 --- a/bitcoin/src/crypto/ecdsa.rs +++ b/bitcoin/src/crypto/ecdsa.rs @@ -13,7 +13,7 @@ use secp256k1; use crate::prelude::*; use crate::script::PushBytes; -use crate::sighash::{EcdsaSighashType, NonStandardSighashType}; +use crate::sighash::{EcdsaSighashType, NonStandardSighashTypeError}; const MAX_SIG_LEN: usize = 73; @@ -226,8 +226,8 @@ impl From for Error { fn from(e: secp256k1::Error) -> Error { Error::Secp256k1(e) } } -impl From for Error { - fn from(err: NonStandardSighashType) -> Self { Error::NonStandardSighashType(err.0) } +impl From for Error { + fn from(err: NonStandardSighashTypeError) -> Self { Error::NonStandardSighashType(err.0) } } impl From for Error { diff --git a/bitcoin/src/crypto/sighash.rs b/bitcoin/src/crypto/sighash.rs index ec4a2ad7..7e5b1e11 100644 --- a/bitcoin/src/crypto/sighash.rs +++ b/bitcoin/src/crypto/sighash.rs @@ -446,7 +446,7 @@ impl EcdsaSighashType { /// # Errors /// /// If `n` is a non-standard sighash value. - pub fn from_standard(n: u32) -> Result { + pub fn from_standard(n: u32) -> Result { use EcdsaSighashType::*; match n { @@ -457,7 +457,7 @@ impl EcdsaSighashType { 0x81 => Ok(AllPlusAnyoneCanPay), 0x82 => Ok(NonePlusAnyoneCanPay), 0x83 => Ok(SinglePlusAnyoneCanPay), - non_standard => Err(NonStandardSighashType(non_standard)), + non_standard => Err(NonStandardSighashTypeError(non_standard)), } } @@ -518,15 +518,15 @@ impl TapSighashType { /// This type is consensus valid but an input including it would prevent the transaction from /// being relayed on today's Bitcoin network. #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct NonStandardSighashType(pub u32); +pub struct NonStandardSighashTypeError(pub u32); -impl fmt::Display for NonStandardSighashType { +impl fmt::Display for NonStandardSighashTypeError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "Non standard sighash type {}", self.0) } } -impl_std_error!(NonStandardSighashType); +impl_std_error!(NonStandardSighashTypeError); /// Error returned for failure during parsing one of the sighash types. /// diff --git a/bitcoin/src/psbt/map/input.rs b/bitcoin/src/psbt/map/input.rs index 5e610ceb..91993e06 100644 --- a/bitcoin/src/psbt/map/input.rs +++ b/bitcoin/src/psbt/map/input.rs @@ -18,7 +18,7 @@ use crate::psbt::map::Map; use crate::psbt::serialize::Deserialize; use crate::psbt::{self, error, raw, Error}; use crate::sighash::{ - self, EcdsaSighashType, NonStandardSighashType, SighashTypeParseError, TapSighashType, + self, EcdsaSighashType, NonStandardSighashTypeError, SighashTypeParseError, TapSighashType, }; use crate::taproot::{ControlBlock, LeafVersion, TapLeafHash, TapNodeHash}; @@ -190,7 +190,7 @@ impl From for PsbtSighashType { impl PsbtSighashType { /// Returns the [`EcdsaSighashType`] if the [`PsbtSighashType`] can be /// converted to one. - pub fn ecdsa_hash_ty(self) -> Result { + pub fn ecdsa_hash_ty(self) -> Result { EcdsaSighashType::from_standard(self.inner) } @@ -223,7 +223,7 @@ impl Input { /// # Errors /// /// If the `sighash_type` field is set to a non-standard ECDSA sighash value. - pub fn ecdsa_hash_ty(&self) -> Result { + pub fn ecdsa_hash_ty(&self) -> Result { self.sighash_type .map(|sighash_type| sighash_type.ecdsa_hash_ty()) .unwrap_or(Ok(EcdsaSighashType::All)) @@ -575,7 +575,7 @@ mod test { let back = PsbtSighashType::from_str(&s).unwrap(); assert_eq!(back, sighash); - assert_eq!(back.ecdsa_hash_ty(), Err(NonStandardSighashType(nonstd))); + assert_eq!(back.ecdsa_hash_ty(), Err(sighash::NonStandardSighashTypeError(nonstd))); assert_eq!(back.taproot_hash_ty(), Err(sighash::Error::InvalidSighashType(nonstd))); } } From 6c9d9d9c36b5cc34b84e5472b0a30b6a6dba2597 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 18 May 2023 15:15:02 +1000 Subject: [PATCH 02/10] Improve error display imlps Improve the `Error` `Display` impls by doing: - Be more terse by importing the error enum's variants. - Do not use capital letters for error messages. --- bitcoin/src/crypto/ecdsa.rs | 12 +++++++----- bitcoin/src/crypto/key.rs | 12 +++++++----- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/bitcoin/src/crypto/ecdsa.rs b/bitcoin/src/crypto/ecdsa.rs index 1bdb347d..1b0c3c69 100644 --- a/bitcoin/src/crypto/ecdsa.rs +++ b/bitcoin/src/crypto/ecdsa.rs @@ -199,12 +199,14 @@ pub enum Error { impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use Error::*; + match *self { - Error::Hex(ref e) => write_err!(f, "Signature hex decoding error"; e), - Error::NonStandardSighashType(hash_ty) => - write!(f, "Non standard signature hash type {}", hash_ty), - Error::EmptySignature => write!(f, "Empty ECDSA signature"), - Error::Secp256k1(ref e) => write_err!(f, "invalid ECDSA signature"; e), + Hex(ref e) => write_err!(f, "signature hex decoding error"; e), + NonStandardSighashType(hash_ty) => + write!(f, "non-standard signature hash type {}", hash_ty), + EmptySignature => write!(f, "empty ECDSA signature"), + Secp256k1(ref e) => write_err!(f, "invalid ECDSA signature"; e), } } } diff --git a/bitcoin/src/crypto/key.rs b/bitcoin/src/crypto/key.rs index e870592c..dbe6e97d 100644 --- a/bitcoin/src/crypto/key.rs +++ b/bitcoin/src/crypto/key.rs @@ -41,12 +41,14 @@ pub enum Error { impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use Error::*; + match *self { - Error::Base58(ref e) => write_err!(f, "key base58 error"; e), - Error::Secp256k1(ref e) => write_err!(f, "key secp256k1 error"; e), - Error::InvalidKeyPrefix(ref b) => write!(f, "key prefix invalid: {}", b), - Error::Hex(ref e) => write_err!(f, "key hex decoding error"; e), - Error::InvalidHexLength(got) => + Base58(ref e) => write_err!(f, "key base58 error"; e), + Secp256k1(ref e) => write_err!(f, "key secp256k1 error"; e), + InvalidKeyPrefix(ref b) => write!(f, "key prefix invalid: {}", b), + Hex(ref e) => write_err!(f, "key hex decoding error"; e), + InvalidHexLength(got) => write!(f, "PublicKey hex should be 66 or 130 digits long, got: {}", got), } } From baba0fde571591eff59329b89a266b3f8afb4320 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 18 May 2023 15:28:31 +1000 Subject: [PATCH 03/10] Put NonStandardSighashTypeError inside ecdsa::Error variant As per convention; put the error type inside a variant and delegate to it instead of carrying an integer around. --- bitcoin/src/crypto/ecdsa.rs | 15 +++++++-------- bitcoin/src/psbt/serialize.rs | 2 +- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/bitcoin/src/crypto/ecdsa.rs b/bitcoin/src/crypto/ecdsa.rs index 1b0c3c69..d6981f47 100644 --- a/bitcoin/src/crypto/ecdsa.rs +++ b/bitcoin/src/crypto/ecdsa.rs @@ -37,8 +37,7 @@ impl Signature { /// Deserializes from slice following the standardness rules for [`EcdsaSighashType`]. pub fn from_slice(sl: &[u8]) -> Result { let (hash_ty, sig) = sl.split_last().ok_or(Error::EmptySignature)?; - let hash_ty = EcdsaSighashType::from_standard(*hash_ty as u32) - .map_err(|_| Error::NonStandardSighashType(*hash_ty as u32))?; + let hash_ty = EcdsaSighashType::from_standard(*hash_ty as u32)?; let sig = secp256k1::ecdsa::Signature::from_der(sig).map_err(Error::Secp256k1)?; Ok(Signature { sig, hash_ty }) } @@ -189,8 +188,8 @@ impl<'a> IntoIterator for &'a SerializedSignature { pub enum Error { /// Hex decoding error Hex(hex::HexToBytesError), - /// Base58 encoding error - NonStandardSighashType(u32), + /// Non-standard sighash type + SighashType(NonStandardSighashTypeError), /// Empty Signature EmptySignature, /// secp256k1-related error @@ -203,8 +202,7 @@ impl fmt::Display for Error { match *self { Hex(ref e) => write_err!(f, "signature hex decoding error"; e), - NonStandardSighashType(hash_ty) => - write!(f, "non-standard signature hash type {}", hash_ty), + SighashType(ref e) => write_err!(f, "non-standard signature hash type"; e), EmptySignature => write!(f, "empty ECDSA signature"), Secp256k1(ref e) => write_err!(f, "invalid ECDSA signature"; e), } @@ -219,7 +217,8 @@ impl std::error::Error for Error { match self { Hex(e) => Some(e), Secp256k1(e) => Some(e), - NonStandardSighashType(_) | EmptySignature => None, + SighashType(e) => Some(e), + EmptySignature => None, } } } @@ -229,7 +228,7 @@ impl From for Error { } impl From for Error { - fn from(err: NonStandardSighashTypeError) -> Self { Error::NonStandardSighashType(err.0) } + fn from(err: NonStandardSighashTypeError) -> Self { Error::SighashType(err) } } impl From for Error { diff --git a/bitcoin/src/psbt/serialize.rs b/bitcoin/src/psbt/serialize.rs index dfca1b6c..0d5deb61 100644 --- a/bitcoin/src/psbt/serialize.rs +++ b/bitcoin/src/psbt/serialize.rs @@ -176,7 +176,7 @@ impl Deserialize for ecdsa::Signature { // would use check the signature assuming sighash_u32 as `0x01`. ecdsa::Signature::from_slice(bytes).map_err(|e| match e { ecdsa::Error::EmptySignature => Error::InvalidEcdsaSignature(e), - ecdsa::Error::NonStandardSighashType(flag) => Error::NonStandardSighashType(flag), + ecdsa::Error::SighashType(err) => Error::NonStandardSighashType(err.0), ecdsa::Error::Secp256k1(..) => Error::InvalidEcdsaSignature(e), ecdsa::Error::Hex(..) => unreachable!("Decoding from slice, not hex"), }) From ae07786c2710daa136c35e4be49eaa1106cf085d Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 18 May 2023 15:45:50 +1000 Subject: [PATCH 04/10] Add InvalidSighashTypeError As we do for `NonStandardSighashErrorType` add an error struct for invalid sighash type, used by the `taproot` module instead of returning a generic error enum with loads of unused variants. --- bitcoin/src/crypto/sighash.rs | 16 ++++++++++++++-- bitcoin/src/crypto/taproot.rs | 18 +++++++++++------- bitcoin/src/psbt/map/input.rs | 13 +++++++------ bitcoin/src/psbt/serialize.rs | 2 +- 4 files changed, 33 insertions(+), 16 deletions(-) diff --git a/bitcoin/src/crypto/sighash.rs b/bitcoin/src/crypto/sighash.rs index 7e5b1e11..e4b391d4 100644 --- a/bitcoin/src/crypto/sighash.rs +++ b/bitcoin/src/crypto/sighash.rs @@ -499,7 +499,7 @@ impl TapSighashType { } /// Constructs a [`TapSighashType`] from a raw `u8`. - pub fn from_consensus_u8(hash_ty: u8) -> Result { + pub fn from_consensus_u8(hash_ty: u8) -> Result { use TapSighashType::*; Ok(match hash_ty { @@ -510,11 +510,23 @@ impl TapSighashType { 0x81 => AllPlusAnyoneCanPay, 0x82 => NonePlusAnyoneCanPay, 0x83 => SinglePlusAnyoneCanPay, - x => return Err(Error::InvalidSighashType(x as u32)), + x => return Err(InvalidSighashTypeError(x.into())), }) } } +/// Integer is not a consensus valid sighash type. +#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct InvalidSighashTypeError(pub u32); + +impl fmt::Display for InvalidSighashTypeError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "invalid sighash type {}", self.0) + } +} + +impl_std_error!(InvalidSighashTypeError); + /// This type is consensus valid but an input including it would prevent the transaction from /// being relayed on today's Bitcoin network. #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] diff --git a/bitcoin/src/crypto/taproot.rs b/bitcoin/src/crypto/taproot.rs index e6f20832..fa9bc1e1 100644 --- a/bitcoin/src/crypto/taproot.rs +++ b/bitcoin/src/crypto/taproot.rs @@ -11,7 +11,7 @@ use internals::write_err; pub use secp256k1::{self, constants, KeyPair, Parity, Secp256k1, Verification, XOnlyPublicKey}; use crate::prelude::*; -use crate::sighash::TapSighashType; +use crate::sighash::{InvalidSighashTypeError, TapSighashType}; /// A BIP340-341 serialized taproot signature with the corresponding hash type. #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] @@ -35,8 +35,7 @@ impl Signature { } 65 => { let (hash_ty, sig) = sl.split_last().expect("Slice len checked == 65"); - let hash_ty = TapSighashType::from_consensus_u8(*hash_ty) - .map_err(|_| SigFromSliceError::InvalidSighashType(*hash_ty))?; + let hash_ty = TapSighashType::from_consensus_u8(*hash_ty)?; let sig = secp256k1::schnorr::Signature::from_slice(sig)?; Ok(Signature { sig, hash_ty }) } @@ -64,7 +63,7 @@ impl Signature { #[non_exhaustive] pub enum SigFromSliceError { /// Invalid signature hash type. - InvalidSighashType(u8), + SighashType(InvalidSighashTypeError), /// Signature has valid size but does not parse correctly Secp256k1(secp256k1::Error), /// Invalid taproot signature size @@ -76,7 +75,7 @@ impl fmt::Display for SigFromSliceError { use SigFromSliceError::*; match *self { - InvalidSighashType(hash_ty) => write!(f, "invalid signature hash type {}", hash_ty), + SighashType(ref e) => write_err!(f, "invalid signature hash type"; e), Secp256k1(ref e) => write_err!(f, "taproot signature has correct len but is malformed"; e), InvalidSignatureSize(sz) => write!(f, "invalid taproot signature size: {}", sz), @@ -91,11 +90,16 @@ impl std::error::Error for SigFromSliceError { match self { Secp256k1(e) => Some(e), - InvalidSighashType(_) | InvalidSignatureSize(_) => None, + SighashType(e) => Some(e), + InvalidSignatureSize(_) => None, } } } impl From for SigFromSliceError { - fn from(e: secp256k1::Error) -> Self { SigFromSliceError::Secp256k1(e) } + fn from(e: secp256k1::Error) -> Self { Self::Secp256k1(e) } +} + +impl From for SigFromSliceError { + fn from(err: InvalidSighashTypeError) -> Self { Self::SighashType(err) } } diff --git a/bitcoin/src/psbt/map/input.rs b/bitcoin/src/psbt/map/input.rs index 91993e06..0be0d481 100644 --- a/bitcoin/src/psbt/map/input.rs +++ b/bitcoin/src/psbt/map/input.rs @@ -18,7 +18,8 @@ use crate::psbt::map::Map; use crate::psbt::serialize::Deserialize; use crate::psbt::{self, error, raw, Error}; use crate::sighash::{ - self, EcdsaSighashType, NonStandardSighashTypeError, SighashTypeParseError, TapSighashType, + EcdsaSighashType, InvalidSighashTypeError, NonStandardSighashTypeError, SighashTypeParseError, + TapSighashType, }; use crate::taproot::{ControlBlock, LeafVersion, TapLeafHash, TapNodeHash}; @@ -196,9 +197,9 @@ impl PsbtSighashType { /// Returns the [`TapSighashType`] if the [`PsbtSighashType`] can be /// converted to one. - pub fn taproot_hash_ty(self) -> Result { + pub fn taproot_hash_ty(self) -> Result { if self.inner > 0xffu32 { - Err(sighash::Error::InvalidSighashType(self.inner)) + Err(InvalidSighashTypeError(self.inner)) } else { TapSighashType::from_consensus_u8(self.inner as u8) } @@ -235,7 +236,7 @@ impl Input { /// # Errors /// /// If the `sighash_type` field is set to a invalid Taproot sighash value. - pub fn taproot_hash_ty(&self) -> Result { + pub fn taproot_hash_ty(&self) -> Result { self.sighash_type .map(|sighash_type| sighash_type.taproot_hash_ty()) .unwrap_or(Ok(TapSighashType::Default)) @@ -575,7 +576,7 @@ mod test { let back = PsbtSighashType::from_str(&s).unwrap(); assert_eq!(back, sighash); - assert_eq!(back.ecdsa_hash_ty(), Err(sighash::NonStandardSighashTypeError(nonstd))); - assert_eq!(back.taproot_hash_ty(), Err(sighash::Error::InvalidSighashType(nonstd))); + assert_eq!(back.ecdsa_hash_ty(), Err(NonStandardSighashTypeError(nonstd))); + assert_eq!(back.taproot_hash_ty(), Err(InvalidSighashTypeError(nonstd))); } } diff --git a/bitcoin/src/psbt/serialize.rs b/bitcoin/src/psbt/serialize.rs index 0d5deb61..3f9c57ad 100644 --- a/bitcoin/src/psbt/serialize.rs +++ b/bitcoin/src/psbt/serialize.rs @@ -258,7 +258,7 @@ impl Deserialize for taproot::Signature { use taproot::SigFromSliceError::*; taproot::Signature::from_slice(bytes).map_err(|e| match e { - InvalidSighashType(flag) => Error::NonStandardSighashType(flag as u32), + SighashType(err) => Error::NonStandardSighashType(err.0), InvalidSignatureSize(_) => Error::InvalidTaprootSignature(e), Secp256k1(..) => Error::InvalidTaprootSignature(e), }) From f23155aa167f9132b99461e47835ef6af14be6e0 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 18 May 2023 15:38:19 +1000 Subject: [PATCH 05/10] Do not capitalize error messages As per convention; do not capitalize error messages. --- bitcoin/src/blockdata/transaction.rs | 2 +- bitcoin/src/crypto/key.rs | 2 +- bitcoin/src/crypto/sighash.rs | 20 ++++++++++---------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index edd0c223..853aeddb 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -1792,7 +1792,7 @@ mod tests { for s in sht_mistakes { assert_eq!( EcdsaSighashType::from_str(s).unwrap_err().to_string(), - format!("Unrecognized SIGHASH string '{}'", s) + format!("unrecognized SIGHASH string '{}'", s) ); } } diff --git a/bitcoin/src/crypto/key.rs b/bitcoin/src/crypto/key.rs index dbe6e97d..7fe4f484 100644 --- a/bitcoin/src/crypto/key.rs +++ b/bitcoin/src/crypto/key.rs @@ -49,7 +49,7 @@ impl fmt::Display for Error { InvalidKeyPrefix(ref b) => write!(f, "key prefix invalid: {}", b), Hex(ref e) => write_err!(f, "key hex decoding error"; e), InvalidHexLength(got) => - write!(f, "PublicKey hex should be 66 or 130 digits long, got: {}", got), + write!(f, "pubkey hex should be 66 or 130 digits long, got: {}", got), } } } diff --git a/bitcoin/src/crypto/sighash.rs b/bitcoin/src/crypto/sighash.rs index e4b391d4..d5f67ed7 100644 --- a/bitcoin/src/crypto/sighash.rs +++ b/bitcoin/src/crypto/sighash.rs @@ -248,12 +248,12 @@ impl fmt::Display for Error { match self { Io(error_kind) => write!(f, "writer errored: {:?}", error_kind), - IndexOutOfInputsBounds { index, inputs_size } => write!(f, "Requested index ({}) is greater or equal than the number of transaction inputs ({})", index, inputs_size), + IndexOutOfInputsBounds { index, inputs_size } => write!(f, "requested index ({}) is greater or equal than the number of transaction inputs ({})", index, inputs_size), SingleWithoutCorrespondingOutput { index, outputs_size } => write!(f, "SIGHASH_SINGLE for input ({}) haven't a corresponding output (#outputs:{})", index, outputs_size), - PrevoutsSize => write!(f, "Number of supplied prevouts differs from the number of inputs in transaction"), - PrevoutIndex => write!(f, "The index requested is greater than available prevouts or different from the provided [Provided::Anyone] index"), - PrevoutKind => write!(f, "A single prevout has been provided but all prevouts are needed without `ANYONECANPAY`"), - WrongAnnex => write!(f, "Annex must be at least one byte long and the first bytes must be `0x50`"), + PrevoutsSize => write!(f, "number of supplied prevouts differs from the number of inputs in transaction"), + PrevoutIndex => write!(f, "the index requested is greater than available prevouts or different from the provided [Provided::Anyone] index"), + PrevoutKind => write!(f, "a single prevout has been provided but all prevouts are needed without `ANYONECANPAY`"), + WrongAnnex => write!(f, "annex must be at least one byte long and the first bytes must be `0x50`"), InvalidSighashType(hash_ty) => write!(f, "Invalid taproot signature hash type : {} ", hash_ty), } } @@ -325,8 +325,8 @@ impl<'s> ScriptPath<'s> { self.leaf_version .to_consensus() .consensus_encode(&mut enc) - .expect("Writing to hash enging should never fail"); - self.script.consensus_encode(&mut enc).expect("Writing to hash enging should never fail"); + .expect("writing to hash enging should never fail"); + self.script.consensus_encode(&mut enc).expect("writing to hash enging should never fail"); TapLeafHash::from_engine(enc) } @@ -534,7 +534,7 @@ pub struct NonStandardSighashTypeError(pub u32); impl fmt::Display for NonStandardSighashTypeError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "Non standard sighash type {}", self.0) + write!(f, "non-standard sighash type {}", self.0) } } @@ -551,7 +551,7 @@ pub struct SighashTypeParseError { impl fmt::Display for SighashTypeParseError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "Unrecognized SIGHASH string '{}'", self.unrecognized) + write!(f, "unrecognized SIGHASH string '{}'", self.unrecognized) } } @@ -1666,7 +1666,7 @@ mod tests { for s in sht_mistakes { assert_eq!( TapSighashType::from_str(s).unwrap_err().to_string(), - format!("Unrecognized SIGHASH string '{}'", s) + format!("unrecognized SIGHASH string '{}'", s) ); } } From 5f8e0ad67e7ce3f56e1ce16f4b3fcf3f5c954690 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 18 May 2023 16:00:13 +1000 Subject: [PATCH 06/10] Fix docs on error type Make the rustdoc arguable clearer, this is a sig error. --- bitcoin/src/crypto/ecdsa.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bitcoin/src/crypto/ecdsa.rs b/bitcoin/src/crypto/ecdsa.rs index d6981f47..54a9ec3d 100644 --- a/bitcoin/src/crypto/ecdsa.rs +++ b/bitcoin/src/crypto/ecdsa.rs @@ -182,7 +182,7 @@ impl<'a> IntoIterator for &'a SerializedSignature { fn into_iter(self) -> Self::IntoIter { (*self).iter() } } -/// A key-related error. +/// An ECDSA signature-related error. #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] pub enum Error { From fe3b1e1140f1daaeb37ef1c749036bbac00ac067 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 18 May 2023 16:10:33 +1000 Subject: [PATCH 07/10] Move From for Error impl Move the From impl to be below the other code for the error. Refactor only, no logic changes. --- bitcoin/src/crypto/sighash.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bitcoin/src/crypto/sighash.rs b/bitcoin/src/crypto/sighash.rs index d5f67ed7..56b5ab1c 100644 --- a/bitcoin/src/crypto/sighash.rs +++ b/bitcoin/src/crypto/sighash.rs @@ -277,6 +277,10 @@ impl std::error::Error for Error { } } +impl From for Error { + fn from(e: io::Error) -> Self { Error::Io(e.kind()) } +} + impl<'u, T> Prevouts<'u, T> where T: Borrow, @@ -1085,10 +1089,6 @@ impl> SighashCache { } } -impl From for Error { - fn from(e: io::Error) -> Self { Error::Io(e.kind()) } -} - /// The `Annex` struct is a slice wrapper enforcing first byte is `0x50`. #[derive(Clone, PartialEq, Eq, Hash, Debug)] pub struct Annex<'a>(&'a [u8]); From 80d5d6665a1ef1f6eb7fb0aafd6d9e820d65972f Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 18 May 2023 16:12:51 +1000 Subject: [PATCH 08/10] crypto: key: Move error code to the bottom of the file Error code is boring, put it at the bottom of the file. Refactor only, no logic changes. --- bitcoin/src/crypto/key.rs | 113 +++++++++++++++++++------------------- 1 file changed, 56 insertions(+), 57 deletions(-) diff --git a/bitcoin/src/crypto/key.rs b/bitcoin/src/crypto/key.rs index 7fe4f484..e4840ed2 100644 --- a/bitcoin/src/crypto/key.rs +++ b/bitcoin/src/crypto/key.rs @@ -23,63 +23,6 @@ use crate::prelude::*; use crate::taproot::{TapNodeHash, TapTweakHash}; use crate::{base58, io}; -/// A key-related error. -#[derive(Debug, Clone, PartialEq, Eq)] -#[non_exhaustive] -pub enum Error { - /// Base58 encoding error - Base58(base58::Error), - /// secp256k1-related error - Secp256k1(secp256k1::Error), - /// Invalid key prefix error - InvalidKeyPrefix(u8), - /// Hex decoding error - Hex(hex::HexToArrayError), - /// `PublicKey` hex should be 66 or 130 digits long. - InvalidHexLength(usize), -} - -impl fmt::Display for Error { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - use Error::*; - - match *self { - Base58(ref e) => write_err!(f, "key base58 error"; e), - Secp256k1(ref e) => write_err!(f, "key secp256k1 error"; e), - InvalidKeyPrefix(ref b) => write!(f, "key prefix invalid: {}", b), - Hex(ref e) => write_err!(f, "key hex decoding error"; e), - InvalidHexLength(got) => - write!(f, "pubkey hex should be 66 or 130 digits long, got: {}", got), - } - } -} - -#[cfg(feature = "std")] -impl std::error::Error for Error { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - use self::Error::*; - - match self { - Base58(e) => Some(e), - Secp256k1(e) => Some(e), - Hex(e) => Some(e), - InvalidKeyPrefix(_) | InvalidHexLength(_) => None, - } - } -} - -impl From for Error { - fn from(e: base58::Error) -> Error { Error::Base58(e) } -} - -impl From for Error { - fn from(e: secp256k1::Error) -> Error { Error::Secp256k1(e) } -} - -impl From for Error { - fn from(e: hex::HexToArrayError) -> Self { Error::Hex(e) } -} - /// A Bitcoin ECDSA public key #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct PublicKey { @@ -732,6 +675,62 @@ impl From for TweakedPublicKey { #[inline] fn from(pair: TweakedKeyPair) -> Self { TweakedPublicKey::from_keypair(pair) } } +/// A key-related error. +#[derive(Debug, Clone, PartialEq, Eq)] +#[non_exhaustive] +pub enum Error { + /// Base58 encoding error + Base58(base58::Error), + /// secp256k1-related error + Secp256k1(secp256k1::Error), + /// Invalid key prefix error + InvalidKeyPrefix(u8), + /// Hex decoding error + Hex(hex::HexToArrayError), + /// `PublicKey` hex should be 66 or 130 digits long. + InvalidHexLength(usize), +} + +impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use Error::*; + + match *self { + Base58(ref e) => write_err!(f, "key base58 error"; e), + Secp256k1(ref e) => write_err!(f, "key secp256k1 error"; e), + InvalidKeyPrefix(ref b) => write!(f, "key prefix invalid: {}", b), + Hex(ref e) => write_err!(f, "key hex decoding error"; e), + InvalidHexLength(got) => + write!(f, "pubkey hex should be 66 or 130 digits long, got: {}", got), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for Error { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use self::Error::*; + + match self { + Base58(e) => Some(e), + Secp256k1(e) => Some(e), + Hex(e) => Some(e), + InvalidKeyPrefix(_) | InvalidHexLength(_) => None, + } + } +} + +impl From for Error { + fn from(e: base58::Error) -> Error { Error::Base58(e) } +} + +impl From for Error { + fn from(e: secp256k1::Error) -> Error { Error::Secp256k1(e) } +} + +impl From for Error { + fn from(e: hex::HexToArrayError) -> Self { Error::Hex(e) } +} #[cfg(test)] mod tests { From 3225aa9556d5bae54a72c997adc7539f29bd11d3 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 18 May 2023 16:19:37 +1000 Subject: [PATCH 09/10] Use defensive documentation Only commit in the docs and error messages to what we _really_ know. In an attempt to reduce the likelyhood of the code going stale only commit to what is guaranteed - that we have an error from a module. This does arguably reduce the amount of context around the error. --- bitcoin/src/crypto/ecdsa.rs | 4 ++-- bitcoin/src/crypto/key.rs | 10 +++++----- bitcoin/src/crypto/taproot.rs | 7 +++---- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/bitcoin/src/crypto/ecdsa.rs b/bitcoin/src/crypto/ecdsa.rs index 54a9ec3d..6a3cf24c 100644 --- a/bitcoin/src/crypto/ecdsa.rs +++ b/bitcoin/src/crypto/ecdsa.rs @@ -192,7 +192,7 @@ pub enum Error { SighashType(NonStandardSighashTypeError), /// Empty Signature EmptySignature, - /// secp256k1-related error + /// A secp256k1 error Secp256k1(secp256k1::Error), } @@ -204,7 +204,7 @@ impl fmt::Display for Error { Hex(ref e) => write_err!(f, "signature hex decoding error"; e), SighashType(ref e) => write_err!(f, "non-standard signature hash type"; e), EmptySignature => write!(f, "empty ECDSA signature"), - Secp256k1(ref e) => write_err!(f, "invalid ECDSA signature"; e), + Secp256k1(ref e) => write_err!(f, "secp256k1"; e), } } } diff --git a/bitcoin/src/crypto/key.rs b/bitcoin/src/crypto/key.rs index e4840ed2..e239ea9f 100644 --- a/bitcoin/src/crypto/key.rs +++ b/bitcoin/src/crypto/key.rs @@ -679,9 +679,9 @@ impl From for TweakedPublicKey { #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] pub enum Error { - /// Base58 encoding error + /// A base58 error. Base58(base58::Error), - /// secp256k1-related error + /// A secp256k1 error. Secp256k1(secp256k1::Error), /// Invalid key prefix error InvalidKeyPrefix(u8), @@ -696,10 +696,10 @@ impl fmt::Display for Error { use Error::*; match *self { - Base58(ref e) => write_err!(f, "key base58 error"; e), - Secp256k1(ref e) => write_err!(f, "key secp256k1 error"; e), + Base58(ref e) => write_err!(f, "base58"; e), + Secp256k1(ref e) => write_err!(f, "secp256k1"; e), InvalidKeyPrefix(ref b) => write!(f, "key prefix invalid: {}", b), - Hex(ref e) => write_err!(f, "key hex decoding error"; e), + Hex(ref e) => write_err!(f, "hex"; e), InvalidHexLength(got) => write!(f, "pubkey hex should be 66 or 130 digits long, got: {}", got), } diff --git a/bitcoin/src/crypto/taproot.rs b/bitcoin/src/crypto/taproot.rs index fa9bc1e1..360b0e62 100644 --- a/bitcoin/src/crypto/taproot.rs +++ b/bitcoin/src/crypto/taproot.rs @@ -64,7 +64,7 @@ impl Signature { pub enum SigFromSliceError { /// Invalid signature hash type. SighashType(InvalidSighashTypeError), - /// Signature has valid size but does not parse correctly + /// A secp256k1 error. Secp256k1(secp256k1::Error), /// Invalid taproot signature size InvalidSignatureSize(usize), @@ -75,9 +75,8 @@ impl fmt::Display for SigFromSliceError { use SigFromSliceError::*; match *self { - SighashType(ref e) => write_err!(f, "invalid signature hash type"; e), - Secp256k1(ref e) => - write_err!(f, "taproot signature has correct len but is malformed"; e), + SighashType(ref e) => write_err!(f, "sighash"; e), + Secp256k1(ref e) => write_err!(f, "secp256k1"; e), InvalidSignatureSize(sz) => write!(f, "invalid taproot signature size: {}", sz), } } From 3c0bb6342322a99cb13ce6dafa9da99e07c6f694 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Tue, 1 Aug 2023 16:16:28 +1000 Subject: [PATCH 10/10] Do trivial rustdoc improvements --- bitcoin/src/crypto/ecdsa.rs | 8 ++++---- bitcoin/src/crypto/key.rs | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/bitcoin/src/crypto/ecdsa.rs b/bitcoin/src/crypto/ecdsa.rs index 6a3cf24c..c93fa33f 100644 --- a/bitcoin/src/crypto/ecdsa.rs +++ b/bitcoin/src/crypto/ecdsa.rs @@ -186,13 +186,13 @@ impl<'a> IntoIterator for &'a SerializedSignature { #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] pub enum Error { - /// Hex decoding error + /// Hex decoding error. Hex(hex::HexToBytesError), - /// Non-standard sighash type + /// Non-standard sighash type. SighashType(NonStandardSighashTypeError), - /// Empty Signature + /// Signature was empty. EmptySignature, - /// A secp256k1 error + /// A secp256k1 error. Secp256k1(secp256k1::Error), } diff --git a/bitcoin/src/crypto/key.rs b/bitcoin/src/crypto/key.rs index e239ea9f..e17d23b1 100644 --- a/bitcoin/src/crypto/key.rs +++ b/bitcoin/src/crypto/key.rs @@ -683,9 +683,9 @@ pub enum Error { Base58(base58::Error), /// A secp256k1 error. Secp256k1(secp256k1::Error), - /// Invalid key prefix error + /// Invalid key prefix error. InvalidKeyPrefix(u8), - /// Hex decoding error + /// Hex decoding error. Hex(hex::HexToArrayError), /// `PublicKey` hex should be 66 or 130 digits long. InvalidHexLength(usize),