Skip to content

Commit

Permalink
Introduce specific errors for individual use cases
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
tcharding committed Aug 2, 2023
1 parent c06263d commit eced889
Show file tree
Hide file tree
Showing 12 changed files with 571 additions and 260 deletions.
4 changes: 3 additions & 1 deletion examples/sign_verify_recovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand All @@ -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>(
Expand Down
23 changes: 17 additions & 6 deletions src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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) };
Expand All @@ -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.
Expand Down Expand Up @@ -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)
}

Expand All @@ -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)
}

Expand Down
16 changes: 11 additions & 5 deletions src/ecdh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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),
}
}
}
Expand Down Expand Up @@ -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 {
Expand Down
48 changes: 29 additions & 19 deletions src/ecdsa/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -86,7 +84,7 @@ impl Signature {
{
Ok(Signature(ret))
} else {
Err(Error::InvalidSignature)
Err(SignatureError)
}
}
}
Expand All @@ -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 {
Expand All @@ -111,7 +109,7 @@ impl Signature {
{
Ok(Signature(ret))
} else {
Err(Error::InvalidSignature)
Err(SignatureError)
}
}
}
Expand Down Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -366,7 +364,7 @@ impl<C: Verification> Secp256k1<C> {
///
/// ```rust
/// # #[cfg(feature = "rand-std")] {
/// # use secp256k1::{rand, Secp256k1, Message, Error};
/// # use secp256k1::{ecdsa, rand, Secp256k1, Message};
/// #
/// # let secp = Secp256k1::new();
/// # let (secret_key, public_key) = secp.generate_keypair(&mut rand::thread_rng());
Expand All @@ -376,7 +374,7 @@ impl<C: Verification> Secp256k1<C> {
/// assert_eq!(secp.verify_ecdsa(&message, &sig, &public_key), Ok(()));
///
/// let message = Message::from_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(ecdsa::SignatureError));
/// # }
/// ```
#[inline]
Expand All @@ -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(),
Expand All @@ -394,7 +392,7 @@ impl<C: Verification> Secp256k1<C> {
pk.as_c_ptr(),
) == 0
{
Err(Error::IncorrectSignature)
Err(SignatureError)
} else {
Ok(())
}
Expand Down Expand Up @@ -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");
34 changes: 19 additions & 15 deletions src/ecdsa/recovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand All @@ -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),
}
}

Expand All @@ -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,
Expand All @@ -58,7 +61,7 @@ impl RecoverableSignature {
{
Ok(RecoverableSignature(ret))
} else {
Err(Error::InvalidSignature)
Err(SignatureError)
}
}
}
Expand Down Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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(
Expand All @@ -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))
}
Expand All @@ -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")]
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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());
Expand Down
Loading

0 comments on commit eced889

Please sign in to comment.