Introduce specific errors for individual use cases

This PR attempts to introduce a new convention for error handling. The
aim of which 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 bitcoin_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

@@ -8,7 +8,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 +320,25 @@ 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(Clone, Copy, Debug, PartialEq, Eq)]
+pub struct NotEnoughMemoryError;
+
+crate::error::impl_simple_struct_error!(
+    NotEnoughMemoryError,
+    "not enough preallocated memory for the requested buffer size"
+);
+
 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 +354,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 +389,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 +413,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

@@ -8,9 +8,9 @@ use core::{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,12 @@ impl<'de> ::serde::Deserialize<'de> for SharedSecret {
     }
 }
 
+/// Share secret is invalid.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub struct SharedSecretError;
+
+crate::error::impl_simple_struct_error!(SharedSecretError, "shared secret is invalid");
+
 #[cfg(test)]
 #[allow(unused_imports)]
 mod tests {

src/ecdsa/mod.rs

@@ -15,9 +15,7 @@ pub use self::serialized_signature::SerializedSignature;
 use crate::ffi::CPtr;
 #[cfg(feature = "global-context")]
 use crate::SECP256K1;
-use crate::{
-    ffi, from_hex, Error, Message, PublicKey, Secp256k1, SecretKey, Signing, Verification,
-};
+use crate::{ffi, from_hex, Message, PublicKey, Secp256k1, SecretKey, Signing, Verification};
 
 /// An ECDSA signature
 #[derive(Copy, Clone, PartialOrd, Ord, PartialEq, Eq, Hash)]
@@ -36,22 +34,22 @@ impl fmt::Display for Signature {
 }
 
 impl str::FromStr for Signature {
-    type Err = Error;
+    type Err = SignatureError;
     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),
+            _ => Err(SignatureError),
         }
     }
 }
 
 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);
         }
 
         unsafe {
@@ -65,15 +63,15 @@ impl Signature {
             {
                 Ok(Signature(ret))
             } else {
-                Err(Error::InvalidSignature)
+                Err(SignatureError)
             }
         }
     }
 
     /// 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);
         }
 
         unsafe {
@@ -86,7 +84,7 @@ impl Signature {
             {
                 Ok(Signature(ret))
             } else {
-                Err(Error::InvalidSignature)
+                Err(SignatureError)
             }
         }
     }
@@ -95,9 +93,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);
         }
 
         unsafe {
@@ -111,7 +109,7 @@ impl Signature {
             {
                 Ok(Signature(ret))
             } else {
-                Err(Error::InvalidSignature)
+                Err(SignatureError)
             }
         }
     }
@@ -194,7 +192,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<(), SignatureError> {
         SECP256K1.verify_ecdsa(msg, self, pk)
     }
 }
@@ -385,7 +383,7 @@ impl<C: Verification> Secp256k1<C> {
         msg: &Message,
         sig: &Signature,
         pk: &PublicKey,
-    ) -> Result<(), Error> {
+    ) -> Result<(), SignatureError> {
         unsafe {
             if ffi::secp256k1_ecdsa_verify(
                 self.ctx.as_ptr(),
@@ -394,7 +392,7 @@ impl<C: Verification> Secp256k1<C> {
                 pk.as_c_ptr(),
             ) == 0
             {
-                Err(Error::IncorrectSignature)
+                Err(SignatureError)
             } else {
                 Ok(())
             }
@@ -429,3 +427,15 @@ pub(crate) fn der_length_check(sig: &ffi::Signature, max_len: usize) -> bool {
     }
     len <= max_len
 }
+
+/// Signature is invalid.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub struct SignatureError;
+
+crate::error::impl_simple_struct_error!(SignatureError, "signature is invalid");
+
+/// Recovery ID is invalid.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub struct RecoveryIdError;
+
+crate::error::impl_simple_struct_error!(RecoveryIdError, "recover ID is invalid");

src/ecdsa/recovery.rs

@@ -7,10 +7,10 @@
 use core::ptr;
 
 use self::super_ffi::CPtr;
-use super::ffi as super_ffi;
+use super::{ffi as super_ffi, RecoveryIdError, SignatureError};
 use crate::ecdsa::Signature;
 use crate::ffi::recovery as ffi;
-use crate::{key, Error, Message, Secp256k1, Signing, Verification};
+use crate::{key, Message, Secp256k1, Signing, Verification};
 
 /// A tag used for recovering the public key from a compact signature.
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
@@ -23,10 +23,10 @@ pub struct RecoverableSignature(ffi::RecoverableSignature);
 impl RecoveryId {
     #[inline]
     /// Allows library users to create valid recovery IDs from i32.
-    pub fn from_i32(id: i32) -> Result<RecoveryId, Error> {
+    pub fn from_i32(id: i32) -> Result<RecoveryId, RecoveryIdError> {
         match id {
             0..=3 => Ok(RecoveryId(id)),
-            _ => Err(Error::InvalidRecoveryId),
+            _ => Err(RecoveryIdError),
         }
     }
 
@@ -39,16 +39,19 @@ impl RecoverableSignature {
     #[inline]
     /// Converts a compact-encoded byte slice to a signature. This
     /// representation is nonstandard and defined by the libsecp256k1 library.
-    pub fn from_compact(data: &[u8], recid: RecoveryId) -> Result<RecoverableSignature, Error> {
+    pub fn from_compact(
+        data: &[u8],
+        recid: RecoveryId,
+    ) -> Result<RecoverableSignature, SignatureError> {
         if data.is_empty() {
-            return Err(Error::InvalidSignature);
+            return Err(SignatureError);
         }
 
         let mut ret = ffi::RecoverableSignature::new();
 
         unsafe {
             if data.len() != 64 {
-                Err(Error::InvalidSignature)
+                Err(SignatureError)
             } else if ffi::secp256k1_ecdsa_recoverable_signature_parse_compact(
                 super_ffi::secp256k1_context_no_precomp,
                 &mut ret,
@@ -58,7 +61,7 @@ impl RecoverableSignature {
             {
                 Ok(RecoverableSignature(ret))
             } else {
-                Err(Error::InvalidSignature)
+                Err(SignatureError)
             }
         }
     }
@@ -113,7 +116,7 @@ impl RecoverableSignature {
     /// verify-capable context.
     #[inline]
     #[cfg(feature = "global-context")]
-    pub fn recover(&self, msg: &Message) -> Result<key::PublicKey, Error> {
+    pub fn recover(&self, msg: &Message) -> Result<key::PublicKey, SignatureError> {
         crate::SECP256K1.recover_ecdsa(msg, self)
     }
 }
@@ -191,7 +194,7 @@ impl<C: Verification> Secp256k1<C> {
         &self,
         msg: &Message,
         sig: &RecoverableSignature,
-    ) -> Result<key::PublicKey, Error> {
+    ) -> Result<key::PublicKey, SignatureError> {
         unsafe {
             let mut pk = super_ffi::PublicKey::new();
             if ffi::secp256k1_ecdsa_recover(
@@ -201,7 +204,7 @@ impl<C: Verification> Secp256k1<C> {
                 msg.as_c_ptr(),
             ) != 1
             {
-                return Err(Error::InvalidSignature);
+                return Err(SignatureError);
             }
             Ok(key::PublicKey::from(pk))
         }
@@ -214,9 +217,9 @@ mod tests {
     #[cfg(target_arch = "wasm32")]
     use wasm_bindgen_test::wasm_bindgen_test as test;
 
-    use super::{RecoverableSignature, RecoveryId};
+    use super::*;
     use crate::constants::ONE;
-    use crate::{Error, Message, Secp256k1, SecretKey};
+    use crate::{Message, Secp256k1, SecretKey};
 
     #[test]
     #[cfg(feature = "rand-std")]
@@ -316,7 +319,8 @@ mod tests {
 
         let msg = crate::random_32_bytes(&mut rand::thread_rng());
         let msg = Message::from_slice(&msg).unwrap();
-        assert_eq!(s.verify_ecdsa(&msg, &sig, &pk), Err(Error::IncorrectSignature));
+        // TODO: Is this ok, or do we want IncorrectSignatureError as well?
+        assert_eq!(s.verify_ecdsa(&msg, &sig, &pk), Err(SignatureError));
 
         let recovered_key = s.recover_ecdsa(&msg, &sigr).unwrap();
         assert!(recovered_key != pk);
@@ -366,7 +370,7 @@ mod tests {
 
         // Zero is not a valid sig
         let sig = RecoverableSignature::from_compact(&[0; 64], RecoveryId(0)).unwrap();
-        assert_eq!(s.recover_ecdsa(&msg, &sig), Err(Error::InvalidSignature));
+        assert_eq!(s.recover_ecdsa(&msg, &sig), Err(SignatureError));
         // ...but 111..111 is
         let sig = RecoverableSignature::from_compact(&[1; 64], RecoveryId(0)).unwrap();
         assert!(s.recover_ecdsa(&msg, &sig).is_ok());