Introduce specific errors for individual use cases

This patch attempts to introduce a new convention for error handling.

The aim is to make errors more informative to help debugging, to make
the code easier to read, and to help stabalize the API.

Each function that returns a `Result` returns is made to return an error
that is either a struct or an enum in which _every_ variant is used. We
then provide a general purpose error for the crate and conversion
functions to it from all the other error types.

Author: tcharding

examples/sign_verify_recovery.rs

@@ -4,6 +4,8 @@ extern crate secp256k1;
 use hashes::{sha256, Hash};
 use secp256k1::{ecdsa, Error, Message, PublicKey, Secp256k1, SecretKey, Signing, Verification};
 
+// Notice that we provide a general error type for this crate and conversion
+// functions to it from all the other error types so `?` works as expected.
 fn recover<C: Verification>(
     secp: &Secp256k1<C>,
     msg: &[u8],
@@ -15,7 +17,7 @@ fn recover<C: Verification>(
     let id = ecdsa::RecoveryId::from_i32(recovery_id as i32)?;
     let sig = ecdsa::RecoverableSignature::from_compact(&sig, id)?;
 
-    secp.recover_ecdsa(&msg, &sig)
+    Ok(secp.recover_ecdsa(&msg, &sig)?)
 }
 
 fn sign_recovery<C: Signing>(

src/context.rs

@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: CC0-1.0
 
+use core::fmt;
 use core::marker::PhantomData;
 use core::mem::ManuallyDrop;
 use core::ptr::NonNull;
@@ -8,7 +9,7 @@ use core::ptr::NonNull;
 pub use self::alloc_only::*;
 use crate::ffi::types::{c_uint, c_void, AlignedType};
 use crate::ffi::{self, CPtr};
-use crate::{Error, Secp256k1};
+use crate::Secp256k1;
 
 #[cfg(all(feature = "global-context", feature = "std"))]
 /// Module implementing a singleton pattern for a global `Secp256k1` context.
@@ -320,14 +321,31 @@ unsafe impl<'buf> PreallocatedContext<'buf> for AllPreallocated<'buf> {}
 unsafe impl<'buf> PreallocatedContext<'buf> for SignOnlyPreallocated<'buf> {}
 unsafe impl<'buf> PreallocatedContext<'buf> for VerifyOnlyPreallocated<'buf> {}
 
+/// Not enough preallocated memory for the requested buffer size.
+#[derive(Debug, Clone, PartialEq, Eq)]
+#[non_exhaustive]
+#[allow(missing_copy_implementations)] // Don't implement Copy when we use non_exhaustive.
+pub struct NotEnoughMemoryError;
+
+impl fmt::Display for NotEnoughMemoryError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        f.write_str("not enough preallocated memory for the requested buffer size")
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for NotEnoughMemoryError {}
+
 impl<'buf, C: Context + PreallocatedContext<'buf>> Secp256k1<C> {
     /// Lets you create a context with a preallocated buffer in a generic manner (sign/verify/all).
-    pub fn preallocated_gen_new(buf: &'buf mut [AlignedType]) -> Result<Secp256k1<C>, Error> {
+    pub fn preallocated_gen_new(
+        buf: &'buf mut [AlignedType],
+    ) -> Result<Secp256k1<C>, NotEnoughMemoryError> {
         #[cfg(target_arch = "wasm32")]
         ffi::types::sanity_checks_for_wasm();
 
         if buf.len() < Self::preallocate_size_gen() {
-            return Err(Error::NotEnoughMemory);
+            return Err(NotEnoughMemoryError);
         }
         // Safe because buf is not null since it is not empty.
         let buf = unsafe { NonNull::new_unchecked(buf.as_mut_c_ptr() as *mut c_void) };
@@ -343,7 +361,7 @@ impl<'buf> Secp256k1<AllPreallocated<'buf>> {
     /// Creates a new Secp256k1 context with all capabilities.
     pub fn preallocated_new(
         buf: &'buf mut [AlignedType],
-    ) -> Result<Secp256k1<AllPreallocated<'buf>>, Error> {
+    ) -> Result<Secp256k1<AllPreallocated<'buf>>, NotEnoughMemoryError> {
         Secp256k1::preallocated_gen_new(buf)
     }
     /// Uses the ffi `secp256k1_context_preallocated_size` to check the memory size needed for a context.
@@ -378,7 +396,7 @@ impl<'buf> Secp256k1<SignOnlyPreallocated<'buf>> {
     /// Creates a new Secp256k1 context that can only be used for signing.
     pub fn preallocated_signing_only(
         buf: &'buf mut [AlignedType],
-    ) -> Result<Secp256k1<SignOnlyPreallocated<'buf>>, Error> {
+    ) -> Result<Secp256k1<SignOnlyPreallocated<'buf>>, NotEnoughMemoryError> {
         Secp256k1::preallocated_gen_new(buf)
     }
 
@@ -402,7 +420,7 @@ impl<'buf> Secp256k1<VerifyOnlyPreallocated<'buf>> {
     /// Creates a new Secp256k1 context that can only be used for verification
     pub fn preallocated_verification_only(
         buf: &'buf mut [AlignedType],
-    ) -> Result<Secp256k1<VerifyOnlyPreallocated<'buf>>, Error> {
+    ) -> Result<Secp256k1<VerifyOnlyPreallocated<'buf>>, NotEnoughMemoryError> {
         Secp256k1::preallocated_gen_new(buf)
     }
 

src/ecdh.rs

@@ -4,13 +4,13 @@
 //!
 
 use core::borrow::Borrow;
-use core::{ptr, str};
+use core::{fmt, ptr, str};
 
 use secp256k1_sys::types::{c_int, c_uchar, c_void};
 
+use crate::constants;
 use crate::ffi::{self, CPtr};
 use crate::key::{PublicKey, SecretKey};
-use crate::{constants, Error};
 
 // The logic for displaying shared secrets relies on this (see `secret.rs`).
 const SHARED_SECRET_SIZE: usize = constants::SECRET_KEY_SIZE;
@@ -65,25 +65,25 @@ impl SharedSecret {
 
     /// Creates a shared secret from `bytes` slice.
     #[inline]
-    pub fn from_slice(bytes: &[u8]) -> Result<SharedSecret, Error> {
+    pub fn from_slice(bytes: &[u8]) -> Result<SharedSecret, SharedSecretError> {
         match bytes.len() {
             SHARED_SECRET_SIZE => {
                 let mut ret = [0u8; SHARED_SECRET_SIZE];
                 ret[..].copy_from_slice(bytes);
                 Ok(SharedSecret(ret))
             }
-            _ => Err(Error::InvalidSharedSecret),
+            _ => Err(SharedSecretError),
         }
     }
 }
 
 impl str::FromStr for SharedSecret {
-    type Err = Error;
+    type Err = SharedSecretError;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let mut res = [0u8; SHARED_SECRET_SIZE];
         match crate::from_hex(s, &mut res) {
             Ok(SHARED_SECRET_SIZE) => Ok(SharedSecret::from_bytes(res)),
-            _ => Err(Error::InvalidSharedSecret),
+            _ => Err(SharedSecretError),
         }
     }
 }
@@ -183,6 +183,19 @@ impl<'de> ::serde::Deserialize<'de> for SharedSecret {
     }
 }
 
+/// Share secret is invalid.
+#[derive(Debug, Clone, PartialEq, Eq)]
+#[non_exhaustive]
+#[allow(missing_copy_implementations)] // Don't implement Copy when we use non_exhaustive.
+pub struct SharedSecretError;
+
+impl fmt::Display for SharedSecretError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { f.write_str("shared secret is invalid") }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for SharedSecretError {}
+
 #[cfg(test)]
 #[allow(unused_imports)]
 mod tests {

src/ecdsa/mod.rs

@@ -10,13 +10,14 @@ pub mod serialized_signature;
 use core::{fmt, ptr, str};
 
 #[cfg(feature = "recovery")]
-pub use self::recovery::{RecoverableSignature, RecoveryId};
+pub use self::recovery::{InvalidRecoveryIdError, RecoverableSignature, RecoveryId};
 pub use self::serialized_signature::SerializedSignature;
+use crate::error::{write_err, SysError};
 use crate::ffi::CPtr;
 #[cfg(feature = "global-context")]
 use crate::SECP256K1;
 use crate::{
-    ffi, from_hex, Error, Message, PublicKey, Secp256k1, SecretKey, Signing, Verification,
+    ffi, from_hex, FromHexError, Message, PublicKey, Secp256k1, SecretKey, Signing, Verification,
 };
 
 /// An ECDSA signature
@@ -36,22 +37,21 @@ impl fmt::Display for Signature {
 }
 
 impl str::FromStr for Signature {
-    type Err = Error;
+    type Err = SignatureFromStrError;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let mut res = [0u8; 72];
-        match from_hex(s, &mut res) {
-            Ok(x) => Signature::from_der(&res[0..x]),
-            _ => Err(Error::InvalidSignature),
-        }
+        let len = from_hex(s, &mut res)?;
+        let sig = Signature::from_der(&res[0..len])?;
+        Ok(sig)
     }
 }
 
 impl Signature {
     #[inline]
     /// Converts a DER-encoded byte slice to a signature
-    pub fn from_der(data: &[u8]) -> Result<Signature, Error> {
+    pub fn from_der(data: &[u8]) -> Result<Signature, SignatureError> {
         if data.is_empty() {
-            return Err(Error::InvalidSignature);
+            return Err(SignatureError::InvalidLength(0));
         }
 
         unsafe {
@@ -65,15 +65,15 @@ impl Signature {
             {
                 Ok(Signature(ret))
             } else {
-                Err(Error::InvalidSignature)
+                Err(SignatureError::Sys(SysError))
             }
         }
     }
 
     /// Converts a 64-byte compact-encoded byte slice to a signature
-    pub fn from_compact(data: &[u8]) -> Result<Signature, Error> {
+    pub fn from_compact(data: &[u8]) -> Result<Signature, SignatureError> {
         if data.len() != 64 {
-            return Err(Error::InvalidSignature);
+            return Err(SignatureError::InvalidLength(data.len()));
         }
 
         unsafe {
@@ -86,7 +86,7 @@ impl Signature {
             {
                 Ok(Signature(ret))
             } else {
-                Err(Error::InvalidSignature)
+                Err(SignatureError::Sys(SysError))
             }
         }
     }
@@ -95,9 +95,9 @@ impl Signature {
     /// only useful for validating signatures in the Bitcoin blockchain from before
     /// 2016. It should never be used in new applications. This library does not
     /// support serializing to this "format"
-    pub fn from_der_lax(data: &[u8]) -> Result<Signature, Error> {
+    pub fn from_der_lax(data: &[u8]) -> Result<Signature, SignatureError> {
         if data.is_empty() {
-            return Err(Error::InvalidSignature);
+            return Err(SignatureError::InvalidLength(0));
         }
 
         unsafe {
@@ -111,7 +111,7 @@ impl Signature {
             {
                 Ok(Signature(ret))
             } else {
-                Err(Error::InvalidSignature)
+                Err(SignatureError::Sys(SysError))
             }
         }
     }
@@ -194,7 +194,7 @@ impl Signature {
     /// The signature must be normalized or verification will fail (see [`Signature::normalize_s`]).
     #[inline]
     #[cfg(feature = "global-context")]
-    pub fn verify(&self, msg: &Message, pk: &PublicKey) -> Result<(), Error> {
+    pub fn verify(&self, msg: &Message, pk: &PublicKey) -> Result<(), SysError> {
         SECP256K1.verify_ecdsa(msg, self, pk)
     }
 }
@@ -366,7 +366,7 @@ impl<C: Verification> Secp256k1<C> {
     ///
     /// ```rust
     /// # #[cfg(feature = "rand-std")] {
-    /// # use secp256k1::{rand, Secp256k1, Message, Error};
+    /// # use secp256k1::{ecdsa, rand, Secp256k1, Message, SysError};
     /// #
     /// # let secp = Secp256k1::new();
     /// # let (secret_key, public_key) = secp.generate_keypair(&mut rand::thread_rng());
@@ -376,7 +376,7 @@ impl<C: Verification> Secp256k1<C> {
     /// assert_eq!(secp.verify_ecdsa(&message, &sig, &public_key), Ok(()));
     ///
     /// let message = Message::from_digest_slice(&[0xcd; 32]).expect("32 bytes");
-    /// assert_eq!(secp.verify_ecdsa(&message, &sig, &public_key), Err(Error::IncorrectSignature));
+    /// assert_eq!(secp.verify_ecdsa(&message, &sig, &public_key), Err(SysError));
     /// # }
     /// ```
     #[inline]
@@ -385,7 +385,7 @@ impl<C: Verification> Secp256k1<C> {
         msg: &Message,
         sig: &Signature,
         pk: &PublicKey,
-    ) -> Result<(), Error> {
+    ) -> Result<(), SysError> {
         unsafe {
             if ffi::secp256k1_ecdsa_verify(
                 self.ctx.as_ptr(),
@@ -394,7 +394,7 @@ impl<C: Verification> Secp256k1<C> {
                 pk.as_c_ptr(),
             ) == 0
             {
-                Err(Error::IncorrectSignature)
+                Err(SysError)
             } else {
                 Ok(())
             }
@@ -429,3 +429,76 @@ pub(crate) fn der_length_check(sig: &ffi::Signature, max_len: usize) -> bool {
     }
     len <= max_len
 }
+
+/// Signature is invalid.
+#[derive(Debug, Clone, PartialEq, Eq)]
+#[non_exhaustive]
+pub enum SignatureError {
+    /// Invalid signature length.
+    InvalidLength(usize),
+    /// FFI call failed.
+    Sys(SysError),
+}
+
+impl fmt::Display for SignatureError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        use SignatureError::*;
+
+        match *self {
+            InvalidLength(len) => write!(f, "invalid signature length: {}", len),
+            Sys(ref e) => write_err!(f, "sys error"; e),
+        }
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for SignatureError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        use SignatureError::*;
+
+        match *self {
+            InvalidLength(_) => None,
+            Sys(ref e) => Some(e),
+        }
+    }
+}
+
+/// Signature is invalid.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum SignatureFromStrError {
+    /// Invalid hex string.
+    Hex(FromHexError),
+    /// Invalid signature.
+    Sig(SignatureError),
+}
+
+impl fmt::Display for SignatureFromStrError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        use SignatureFromStrError::*;
+
+        match *self {
+            Hex(ref e) => write_err!(f, "error decoding hex"; e),
+            Sig(ref e) => write_err!(f, "invalid signature"; e),
+        }
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for SignatureFromStrError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        use SignatureFromStrError::*;
+
+        match *self {
+            Hex(ref e) => Some(e),
+            Sig(ref e) => Some(e),
+        }
+    }
+}
+
+impl From<FromHexError> for SignatureFromStrError {
+    fn from(e: FromHexError) -> Self { Self::Hex(e) }
+}
+
+impl From<SignatureError> for SignatureFromStrError {
+    fn from(e: SignatureError) -> Self { Self::Sig(e) }
+}