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 7, 2023
1 parent c06263d commit f626506
Show file tree
Hide file tree
Showing 12 changed files with 746 additions and 268 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
28 changes: 22 additions & 6 deletions src/context.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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.
Expand Down Expand Up @@ -320,14 +321,29 @@ 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;

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) };
Expand All @@ -343,7 +359,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 +394,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 +418,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
23 changes: 17 additions & 6 deletions src/ecdh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
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,17 @@ impl<'de> ::serde::Deserialize<'de> for SharedSecret {
}
}

/// Share secret is invalid.
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
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 {
Expand Down
114 changes: 94 additions & 20 deletions src/ecdsa/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,14 @@ use core::{fmt, ptr, str};
#[cfg(feature = "recovery")]
pub use self::recovery::{RecoverableSignature, RecoveryId};
pub use self::serialized_signature::SerializedSignature;
#[cfg(feature = "recovery")]
pub use crate::ecdsa::recovery::InvalidRecoveryIdError;
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
Expand All @@ -36,22 +39,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 {
Expand All @@ -65,15 +67,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 {
Expand All @@ -86,7 +88,7 @@ impl Signature {
{
Ok(Signature(ret))
} else {
Err(Error::InvalidSignature)
Err(SignatureError::Sys(SysError))
}
}
}
Expand All @@ -95,9 +97,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 {
Expand All @@ -111,7 +113,7 @@ impl Signature {
{
Ok(Signature(ret))
} else {
Err(Error::InvalidSignature)
Err(SignatureError::Sys(SysError))
}
}
}
Expand Down Expand Up @@ -194,7 +196,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)
}
}
Expand Down Expand Up @@ -366,7 +368,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());
Expand All @@ -376,7 +378,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(SysError));
/// # }
/// ```
#[inline]
Expand All @@ -385,7 +387,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(),
Expand All @@ -394,7 +396,7 @@ impl<C: Verification> Secp256k1<C> {
pk.as_c_ptr(),
) == 0
{
Err(Error::IncorrectSignature)
Err(SysError)
} else {
Ok(())
}
Expand Down Expand Up @@ -429,3 +431,75 @@ 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 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(Clone, Copy, Debug, 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) }
}
Loading

0 comments on commit f626506

Please sign in to comment.