Merge rust-bitcoin/rust-bitcoin#3216: ecdsa: Improve error types

9fb5edb39e ecdsa: Improve error types (Tobin C. Harding)

Pull request description:

  There are a couple of issues around the ECDSA signature decoding / parsing code. We have duplicate code in `from_str` and `from_slice` and both use the same error type even though it is impossible to get a hex error in `from_slice`.

  Create two errors:

  - A `DecodeError` returned by `from_slice`
  - A `FromStrError` that has a decode variant and a hex variant

  Call through to `from_slice` after parsing hex into a byte vector.

  Removes an instance of `unreachable!`.

  Fix: #1193

ACKs for top commit:
  Kixunil:
    ACK 9fb5edb39e
  apoelstra:
    ACK 9fb5edb39e successfully ran local tests

Tree-SHA512: 3b3ae31887d603f1739d261b491b99f7847987f94dbbfefb9aa84d4250736eba2d007d28746bbb064946d3055e4cca01510677bf2cdbb11bbf83d7388dbd2620

bitcoin/src/crypto/ecdsa.rs

@@ -36,10 +36,11 @@ impl Signature {
     }
 
     /// Deserializes from slice following the standardness rules for [`EcdsaSighashType`].
-    pub fn from_slice(sl: &[u8]) -> Result<Self, Error> {
+    pub fn from_slice(sl: &[u8]) -> Result<Self, DecodeError> {
-        let (sighash_type, sig) = sl.split_last().ok_or(Error::EmptySignature)?;
+        let (sighash_type, sig) = sl.split_last().ok_or(DecodeError::EmptySignature)?;
         let sighash_type = EcdsaSighashType::from_standard(*sighash_type as u32)?;
-        let signature = secp256k1::ecdsa::Signature::from_der(sig).map_err(Error::Secp256k1)?;
+        let signature =
+            secp256k1::ecdsa::Signature::from_der(sig).map_err(DecodeError::Secp256k1)?;
         Ok(Signature { signature, sighash_type })
     }
 
@@ -87,15 +88,11 @@ impl fmt::Display for Signature {
 }
 
 impl FromStr for Signature {
-    type Err = Error;
+    type Err = ParseSignatureError;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let bytes = Vec::from_hex(s)?;
-        let (sighash_byte, signature) = bytes.split_last().ok_or(Error::EmptySignature)?;
+        Ok(Self::from_slice(&bytes)?)
-        Ok(Signature {
-            signature: secp256k1::ecdsa::Signature::from_der(signature)?,
-            sighash_type: EcdsaSighashType::from_standard(*sighash_byte as u32)?,
-        })
     }
 }
 
@@ -204,12 +201,10 @@ impl<'a> IntoIterator for &'a SerializedSignature {
     fn into_iter(self) -> Self::IntoIter { (*self).iter() }
 }
 
-/// An ECDSA signature-related error.
+/// Error encountered while parsing an ECDSA signature from a byte slice.
 #[derive(Debug, Clone, PartialEq, Eq)]
 #[non_exhaustive]
-pub enum Error {
+pub enum DecodeError {
-    /// Hex decoding error.
-    Hex(hex::HexToBytesError),
     /// Non-standard sighash type.
     SighashType(NonStandardSighashTypeError),
     /// Signature was empty.
@@ -218,14 +213,13 @@ pub enum Error {
     Secp256k1(secp256k1::Error),
 }
 
-internals::impl_from_infallible!(Error);
+internals::impl_from_infallible!(DecodeError);
 
-impl fmt::Display for Error {
+impl fmt::Display for DecodeError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        use Error::*;
+        use DecodeError::*;
 
         match *self {
-            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, "secp256k1"; e),
@@ -234,12 +228,11 @@ impl fmt::Display for Error {
 }
 
 #[cfg(feature = "std")]
-impl std::error::Error for Error {
+impl std::error::Error for DecodeError {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
-        use Error::*;
+        use DecodeError::*;
 
         match *self {
-            Hex(ref e) => Some(e),
             Secp256k1(ref e) => Some(e),
             SighashType(ref e) => Some(e),
             EmptySignature => None,
@@ -247,18 +240,57 @@ impl std::error::Error for Error {
     }
 }
 
-impl From<secp256k1::Error> for Error {
+impl From<secp256k1::Error> for DecodeError {
     fn from(e: secp256k1::Error) -> Self { Self::Secp256k1(e) }
 }
 
-impl From<NonStandardSighashTypeError> for Error {
+impl From<NonStandardSighashTypeError> for DecodeError {
     fn from(e: NonStandardSighashTypeError) -> Self { Self::SighashType(e) }
 }
 
-impl From<hex::HexToBytesError> for Error {
+/// Error encountered while parsing an ECDSA signature from a string.
+#[derive(Debug, Clone, PartialEq, Eq)]
+#[non_exhaustive]
+pub enum ParseSignatureError {
+    /// Hex string decoding error.
+    Hex(hex::HexToBytesError),
+    /// Signature byte slice decoding error.
+    Decode(DecodeError),
+}
+
+internals::impl_from_infallible!(ParseSignatureError);
+
+impl fmt::Display for ParseSignatureError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        use ParseSignatureError::*;
+
+        match *self {
+            Hex(ref e) => write_err!(f, "signature hex decoding error"; e),
+            Decode(ref e) => write_err!(f, "signature byte slice decoding error"; e),
+        }
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for ParseSignatureError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        use ParseSignatureError::*;
+
+        match *self {
+            Hex(ref e) => Some(e),
+            Decode(ref e) => Some(e),
+        }
+    }
+}
+
+impl From<hex::HexToBytesError> for ParseSignatureError {
     fn from(e: hex::HexToBytesError) -> Self { Self::Hex(e) }
 }
 
+impl From<DecodeError> for ParseSignatureError {
+    fn from(e: DecodeError) -> Self { Self::Decode(e) }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

bitcoin/src/psbt/error.rs

@@ -82,7 +82,7 @@ pub enum Error {
     /// Parsing error indicating invalid xonly public keys
     InvalidXOnlyPublicKey,
     /// Parsing error indicating invalid ECDSA signatures
-    InvalidEcdsaSignature(crate::crypto::ecdsa::Error),
+    InvalidEcdsaSignature(crate::crypto::ecdsa::DecodeError),
     /// Parsing error indicating invalid Taproot signatures
     InvalidTaprootSignature(crate::crypto::taproot::SigFromSliceError),
     /// Parsing error indicating invalid control block

bitcoin/src/psbt/serialize.rs

@@ -172,10 +172,9 @@ impl Deserialize for ecdsa::Signature {
         // 0x05, the sighash message would have the last field as 0x05u32 while, the verification
         // 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::DecodeError::EmptySignature => Error::InvalidEcdsaSignature(e),
-            ecdsa::Error::SighashType(err) => Error::NonStandardSighashType(err.0),
+            ecdsa::DecodeError::SighashType(err) => Error::NonStandardSighashType(err.0),
-            ecdsa::Error::Secp256k1(..) => Error::InvalidEcdsaSignature(e),
+            ecdsa::DecodeError::Secp256k1(..) => Error::InvalidEcdsaSignature(e),
-            ecdsa::Error::Hex(..) => unreachable!("decoding from slice, not hex"),
         })
     }
 }