Merge rust-bitcoin/rust-bitcoin#1862: crypto: Overhaul the errors

3c0bb63423 Do trivial rustdoc improvements (Tobin C. Harding)
3225aa9556 Use defensive documentation (Tobin C. Harding)
80d5d6665a crypto: key: Move error code to the bottom of the file (Tobin C. Harding)
fe3b1e1140 Move From for Error impl (Tobin C. Harding)
5f8e0ad67e Fix docs on error type (Tobin C. Harding)
f23155aa16 Do not capitalize error messages (Tobin C. Harding)
ae07786c27 Add InvalidSighashTypeError (Tobin C. Harding)
baba0fde57 Put NonStandardSighashTypeError inside ecdsa::Error variant (Tobin C. Harding)
6c9d9d9c36 Improve error display imlps (Tobin C. Harding)
22c7aa8808 Rename non standard sighash error type (Tobin C. Harding)

Pull request description:

  EDIT: The commit hashes below are stale but the text is valid still.

  In an effort to "perfect" our error handling, overhaul the error handling in the `crypto` module.

  The aim is to do a small chunk so we can bikeshed on it then I can apply the learnings to the rest of the codebase.

  Its all pretty trivial except:

  - commit `4c180277 Put NonStandardSighashTypeError inside ecdsa::Error variant`
  - comimt `5a196535 Add InvalidSighashTypeError`
  - commit `05772ade Use defensive documentation`

  Particularly the last one might be incorrect/controversial.

  Also, please take the time to check the overall state of error code in the `crypto` module on this branch in case there is anything else we want to do.

  Thanks

ACKs for top commit:
  apoelstra:
    ACK 3c0bb63423

Tree-SHA512: 7e5f8590aec5826098d4d8d33351a41b10c42b6379ff86e5b889e73271b71921fc3ca9525baa5da53e07fa2e961e710393694e04658a8243799950b4604caf43

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)
             );
         }
     }

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;
 
@@ -37,8 +37,7 @@ impl Signature {
     /// Deserializes from slice following the standardness rules for [`EcdsaSighashType`].
     pub fn from_slice(sl: &[u8]) -> Result<Self, Error> {
         let (hash_ty, sig) = sl.split_last().ok_or(Error::EmptySignature)?;
-        let hash_ty = EcdsaSighashType::from_standard(*hash_ty as u32)
+        let hash_ty = EcdsaSighashType::from_standard(*hash_ty as u32)?;
-            .map_err(|_| Error::NonStandardSighashType(*hash_ty as u32))?;
         let sig = secp256k1::ecdsa::Signature::from_der(sig).map_err(Error::Secp256k1)?;
         Ok(Signature { sig, hash_ty })
     }
@@ -183,28 +182,29 @@ 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 {
-    /// Hex decoding error
+    /// Hex decoding error.
     Hex(hex::HexToBytesError),
-    /// Base58 encoding error
+    /// Non-standard sighash type.
-    NonStandardSighashType(u32),
+    SighashType(NonStandardSighashTypeError),
-    /// Empty Signature
+    /// Signature was empty.
     EmptySignature,
-    /// secp256k1-related error
+    /// A secp256k1 error.
     Secp256k1(secp256k1::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),
+            Hex(ref e) => write_err!(f, "signature hex decoding error"; e),
-            Error::NonStandardSighashType(hash_ty) =>
+            SighashType(ref e) => write_err!(f, "non-standard signature hash type"; e),
-                write!(f, "Non standard signature hash type {}", hash_ty),
+            EmptySignature => write!(f, "empty ECDSA signature"),
-            Error::EmptySignature => write!(f, "Empty ECDSA signature"),
+            Secp256k1(ref e) => write_err!(f, "secp256k1"; e),
-            Error::Secp256k1(ref e) => write_err!(f, "invalid ECDSA signature"; e),
         }
     }
 }
@@ -217,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,
         }
     }
 }
@@ -226,8 +227,8 @@ impl From<secp256k1::Error> for Error {
     fn from(e: secp256k1::Error) -> Error { Error::Secp256k1(e) }
 }
 
-impl From<NonStandardSighashType> for Error {
+impl From<NonStandardSighashTypeError> for Error {
-    fn from(err: NonStandardSighashType) -> Self { Error::NonStandardSighashType(err.0) }
+    fn from(err: NonStandardSighashTypeError) -> Self { Error::SighashType(err) }
 }
 
 impl From<hex::HexToBytesError> for Error {

bitcoin/src/crypto/key.rs

@@ -23,61 +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 {
-        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) =>
-                write!(f, "PublicKey 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<base58::Error> for Error {
-    fn from(e: base58::Error) -> Error { Error::Base58(e) }
-}
-
-impl From<secp256k1::Error> for Error {
-    fn from(e: secp256k1::Error) -> Error { Error::Secp256k1(e) }
-}
-
-impl From<hex::HexToArrayError> 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 {
@@ -730,6 +675,62 @@ impl From<TweakedKeyPair> 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 {
+    /// A base58 error.
+    Base58(base58::Error),
+    /// A secp256k1 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, "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, "hex"; 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<base58::Error> for Error {
+    fn from(e: base58::Error) -> Error { Error::Base58(e) }
+}
+
+impl From<secp256k1::Error> for Error {
+    fn from(e: secp256k1::Error) -> Error { Error::Secp256k1(e) }
+}
+
+impl From<hex::HexToArrayError> for Error {
+    fn from(e: hex::HexToArrayError) -> Self { Error::Hex(e) }
+}
 
 #[cfg(test)]
 mod tests {

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"),
+            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"),
+            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`"),
+            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`"),
+            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),
         }
     }
@@ -277,6 +277,10 @@ impl std::error::Error for Error {
     }
 }
 
+impl From<io::Error> for Error {
+    fn from(e: io::Error) -> Self { Error::Io(e.kind()) }
+}
+
 impl<'u, T> Prevouts<'u, T>
 where
     T: Borrow<TxOut>,
@@ -325,8 +329,8 @@ impl<'s> ScriptPath<'s> {
         self.leaf_version
             .to_consensus()
             .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");
+        self.script.consensus_encode(&mut enc).expect("writing to hash enging should never fail");
 
         TapLeafHash::from_engine(enc)
     }
@@ -446,7 +450,7 @@ impl EcdsaSighashType {
     /// # Errors
     ///
     /// If `n` is a non-standard sighash value.
-    pub fn from_standard(n: u32) -> Result<EcdsaSighashType, NonStandardSighashType> {
+    pub fn from_standard(n: u32) -> Result<EcdsaSighashType, NonStandardSighashTypeError> {
         use EcdsaSighashType::*;
 
         match n {
@@ -457,7 +461,7 @@ impl EcdsaSighashType {
             0x81 => Ok(AllPlusAnyoneCanPay),
             0x82 => Ok(NonePlusAnyoneCanPay),
             0x83 => Ok(SinglePlusAnyoneCanPay),
-            non_standard => Err(NonStandardSighashType(non_standard)),
+            non_standard => Err(NonStandardSighashTypeError(non_standard)),
         }
     }
 
@@ -499,7 +503,7 @@ impl TapSighashType {
     }
 
     /// Constructs a [`TapSighashType`] from a raw `u8`.
-    pub fn from_consensus_u8(hash_ty: u8) -> Result<Self, Error> {
+    pub fn from_consensus_u8(hash_ty: u8) -> Result<Self, InvalidSighashTypeError> {
         use TapSighashType::*;
 
         Ok(match hash_ty {
@@ -510,23 +514,35 @@ 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)]
-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)
+        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.
 ///
@@ -539,7 +555,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)
     }
 }
 
@@ -1073,10 +1089,6 @@ impl<R: BorrowMut<Transaction>> SighashCache<R> {
     }
 }
 
-impl From<io::Error> 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]);
@@ -1654,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)
             );
         }
     }

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)
+                let hash_ty = TapSighashType::from_consensus_u8(*hash_ty)?;
-                    .map_err(|_| SigFromSliceError::InvalidSighashType(*hash_ty))?;
                 let sig = secp256k1::schnorr::Signature::from_slice(sig)?;
                 Ok(Signature { sig, hash_ty })
             }
@@ -64,8 +63,8 @@ impl Signature {
 #[non_exhaustive]
 pub enum SigFromSliceError {
     /// Invalid signature hash type.
-    InvalidSighashType(u8),
+    SighashType(InvalidSighashTypeError),
-    /// Signature has valid size but does not parse correctly
+    /// A secp256k1 error.
     Secp256k1(secp256k1::Error),
     /// Invalid taproot signature size
     InvalidSignatureSize(usize),
@@ -76,9 +75,8 @@ 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, "sighash"; e),
-            Secp256k1(ref e) =>
+            Secp256k1(ref e) => write_err!(f, "secp256k1"; e),
-                write_err!(f, "taproot signature has correct len but is malformed"; e),
             InvalidSignatureSize(sz) => write!(f, "invalid taproot signature size: {}", sz),
         }
     }
@@ -91,11 +89,16 @@ impl std::error::Error for SigFromSliceError {
 
         match self {
             Secp256k1(e) => Some(e),
-            InvalidSighashType(_) | InvalidSignatureSize(_) => None,
+            SighashType(e) => Some(e),
+            InvalidSignatureSize(_) => None,
         }
     }
 }
 
 impl From<secp256k1::Error> for SigFromSliceError {
-    fn from(e: secp256k1::Error) -> Self { SigFromSliceError::Secp256k1(e) }
+    fn from(e: secp256k1::Error) -> Self { Self::Secp256k1(e) }
+}
+
+impl From<InvalidSighashTypeError> for SigFromSliceError {
+    fn from(err: InvalidSighashTypeError) -> Self { Self::SighashType(err) }
 }

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, NonStandardSighashType, SighashTypeParseError, TapSighashType,
+    EcdsaSighashType, InvalidSighashTypeError, NonStandardSighashTypeError, SighashTypeParseError,
+    TapSighashType,
 };
 use crate::taproot::{ControlBlock, LeafVersion, TapLeafHash, TapNodeHash};
 
@@ -190,15 +191,15 @@ impl From<TapSighashType> for PsbtSighashType {
 impl PsbtSighashType {
     /// Returns the [`EcdsaSighashType`] if the [`PsbtSighashType`] can be
     /// converted to one.
-    pub fn ecdsa_hash_ty(self) -> Result<EcdsaSighashType, NonStandardSighashType> {
+    pub fn ecdsa_hash_ty(self) -> Result<EcdsaSighashType, NonStandardSighashTypeError> {
         EcdsaSighashType::from_standard(self.inner)
     }
 
     /// Returns the [`TapSighashType`] if the [`PsbtSighashType`] can be
     /// converted to one.
-    pub fn taproot_hash_ty(self) -> Result<TapSighashType, sighash::Error> {
+    pub fn taproot_hash_ty(self) -> Result<TapSighashType, InvalidSighashTypeError> {
         if self.inner > 0xffu32 {
-            Err(sighash::Error::InvalidSighashType(self.inner))
+            Err(InvalidSighashTypeError(self.inner))
         } else {
             TapSighashType::from_consensus_u8(self.inner as u8)
         }
@@ -223,7 +224,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<EcdsaSighashType, NonStandardSighashType> {
+    pub fn ecdsa_hash_ty(&self) -> Result<EcdsaSighashType, NonStandardSighashTypeError> {
         self.sighash_type
             .map(|sighash_type| sighash_type.ecdsa_hash_ty())
             .unwrap_or(Ok(EcdsaSighashType::All))
@@ -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<TapSighashType, sighash::Error> {
+    pub fn taproot_hash_ty(&self) -> Result<TapSighashType, InvalidSighashTypeError> {
         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(NonStandardSighashType(nonstd)));
+        assert_eq!(back.ecdsa_hash_ty(), Err(NonStandardSighashTypeError(nonstd)));
-        assert_eq!(back.taproot_hash_ty(), Err(sighash::Error::InvalidSighashType(nonstd)));
+        assert_eq!(back.taproot_hash_ty(), Err(InvalidSighashTypeError(nonstd)));
     }
 }

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"),
         })
@@ -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),
         })