Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementing pre allocation context creation #116

Closed
wants to merge 2 commits into from
Closed

Implementing pre allocation context creation #116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e782eb
Added the preallocated FFI
elichai May 28, 2019
776da69
Implementing the pre allocated context
elichai May 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/ffi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,16 @@ extern "C" {
// Contexts
pub fn secp256k1_context_create(flags: c_uint) -> *mut Context;

pub fn secp256k1_context_preallocated_size(flags: c_uint) -> usize;

pub fn secp256k1_context_preallocated_create(prealloc: *mut c_void, flags: c_uint) -> *mut Context;

pub fn secp256k1_context_clone(cx: *mut Context) -> *mut Context;

pub fn secp256k1_context_destroy(cx: *mut Context);

pub fn secp256k1_context_preallocated_destroy(cx: *mut Context);

pub fn secp256k1_context_randomize(cx: *mut Context,
seed32: *const c_uchar)
-> c_int;
Expand Down
126 changes: 99 additions & 27 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,8 @@ pub use key::SecretKey;
pub use key::PublicKey;
use core::marker::PhantomData;
use core::ops::Deref;
use self::types::c_uint;
use types::c_void;

/// An ECDSA signature
#[derive(Copy, Clone, PartialEq, Eq)]
Expand Down Expand Up @@ -463,6 +465,8 @@ pub enum Error {
InvalidRecoveryId,
/// Invalid tweak for add_*_assign or mul_*_assign
InvalidTweak,
/// Didn't pass enough memory to `new_preallocated_internal`
NotEnoughMemory,
}

impl Error {
Expand All @@ -475,6 +479,7 @@ impl Error {
Error::InvalidSecretKey => "secp: malformed or out-of-range secret key",
Error::InvalidRecoveryId => "secp: bad recovery id",
Error::InvalidTweak => "secp: bad tweak",
Error::NotEnoughMemory => "secp: not enough memory allocated",
}
}
}
Expand Down Expand Up @@ -513,26 +518,29 @@ impl Verification for VerifyOnly {}
impl Verification for All {}

/// The secp256k1 engine, used to execute all signature operations
pub struct Secp256k1<C> {
pub struct Secp256k1<'buf, C> {
ctx: *mut ffi::Context,
phantom: PhantomData<C>
phantom: PhantomData<C>,
_buf: Option<&'buf [u8]>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think unconditionally adding 'buf to the public API will make things too unergonomic for users who aren't providing their own buffer.

I think it'd be better to add a new set of context types SignOnlyBuffer<'buf> VerifyOnlyBuffer<'buf> AllBuffer<'buf> which tie the lifetime of the passed-in buffer to the context. Then let phantom deal with the borrowck and change the _buf field into buf: Option<*mut u8>.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm you prefer that no-std users will have a different Context struct? (If you provide no buffer you shouldn't feel the new lifetime at all(as a user, for the library maintainers you're right that it's unergonomic))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do go in that direction we'll need to abstract out the Context, so that sign/verify functions won't be implemented on the struct itself but on a generic of the trait (adding something like get_ptr() to the traits)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the nice thing about having &[u8] is that it's a fat pointer which makes it easier to clone (no need to recheck the size)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least having 'buf in the API makes the documentation confusing, since users will see a lifetime but have no idea what it's for or how to use it.

I'm pretty sure sign/verify are already abstracted over the context, since e.g. both SignOnly and All have signing functionality.

}

// The underlying secp context does not contain any references to memory it does not own
unsafe impl<C> Send for Secp256k1<C> {}
unsafe impl<'buf, C> Send for Secp256k1<'buf, C> {}
// The API does not permit any mutation of `Secp256k1` objects except through `&mut` references
unsafe impl<C> Sync for Secp256k1<C> {}
unsafe impl<'buf, C> Sync for Secp256k1<'buf, C> {}

impl<C> Clone for Secp256k1<C> {
fn clone(&self) -> Secp256k1<C> {
#[cfg(feature = "std")]
impl<'buf, C> Clone for Secp256k1<'buf, C> {
fn clone(&self) -> Secp256k1<'static, C> {
Secp256k1 {
ctx: unsafe { ffi::secp256k1_context_clone(self.ctx) },
phantom: self.phantom
phantom: self.phantom,
_buf: None,
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...then I'd implement Clone only for the non-Buffer versions of Secp256k1.


impl<C> PartialEq for Secp256k1<C> {
impl<'buf, C> PartialEq for Secp256k1<'buf, C> {
fn eq(&self, _other: &Secp256k1<C>) -> bool { true }
}

Expand Down Expand Up @@ -566,60 +574,90 @@ impl Deref for SerializedSignature {

impl Eq for SerializedSignature {}

impl<C> Eq for Secp256k1<C> { }
impl<'buf, C> Eq for Secp256k1<'buf, C> { }

impl<C> Drop for Secp256k1<C> {
impl<'buf, C> Drop for Secp256k1<'buf, C> {
fn drop(&mut self) {
unsafe { ffi::secp256k1_context_destroy(self.ctx); }
match self._buf {
None => unsafe { ffi::secp256k1_context_destroy(self.ctx) },
Some(_) => unsafe { ffi::secp256k1_context_preallocated_destroy(self.ctx) },
}
}
}

impl fmt::Debug for Secp256k1<SignOnly> {
impl<'buf> fmt::Debug for Secp256k1<'buf, SignOnly> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "<secp256k1 context {:?}, signing only>", self.ctx)
}
}

impl fmt::Debug for Secp256k1<VerifyOnly> {
impl<'buf> fmt::Debug for Secp256k1<'buf, VerifyOnly> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "<secp256k1 context {:?}, verification only>", self.ctx)
}
}

impl fmt::Debug for Secp256k1<All> {
impl<'buf> fmt::Debug for Secp256k1<'buf, All> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "<secp256k1 context {:?}, all capabilities>", self.ctx)
}
}

impl Secp256k1<All> {
impl<'buf> Secp256k1<'buf, All> {
const FLAGS: c_uint = ffi::SECP256K1_START_SIGN | ffi::SECP256K1_START_VERIFY;
/// Creates a new Secp256k1 context with all capabilities
pub fn new() -> Secp256k1<All> {
Secp256k1 { ctx: unsafe { ffi::secp256k1_context_create(ffi::SECP256K1_START_SIGN | ffi::SECP256K1_START_VERIFY) }, phantom: PhantomData }
pub fn new() -> Secp256k1<'buf, All> {
Secp256k1 { ctx: unsafe { ffi::secp256k1_context_create(Self::FLAGS) }, phantom: PhantomData, _buf: None }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed on IRC, I'd like to allocate our own memory in Rust and then call the prealloc functions.

}

pub fn get_preallocated_size() -> usize {
Self::get_preallocated_size_internal(Self::FLAGS)
}

pub fn new_preallocated(buf: &'buf mut [u8]) -> Result<Secp256k1<'buf, All>, Error> {
Self::new_preallocated_internal(buf, Self::FLAGS)
}
}

impl Default for Secp256k1<All> {
impl<'buf> Default for Secp256k1<'buf, All> {
fn default() -> Self {
Self::new()
}
}

impl Secp256k1<SignOnly> {
impl<'buf> Secp256k1<'buf, SignOnly> {
const FLAGS: c_uint = ffi::SECP256K1_START_SIGN;
/// Creates a new Secp256k1 context that can only be used for signing
pub fn signing_only() -> Secp256k1<SignOnly> {
Secp256k1 { ctx: unsafe { ffi::secp256k1_context_create(ffi::SECP256K1_START_SIGN) }, phantom: PhantomData }
pub fn signing_only() -> Secp256k1<'buf, SignOnly> {
Secp256k1 { ctx: unsafe { ffi::secp256k1_context_create(Self::FLAGS) }, phantom: PhantomData, _buf: None }
}

pub fn get_preallocated_signing_only_size() -> usize {
Self::get_preallocated_size_internal(Self::FLAGS)
}

pub fn new_preallocated_signing_only(buf: &'buf mut [u8]) -> Result<Secp256k1<'buf, SignOnly>, Error>{
Self::new_preallocated_internal(buf, Self::FLAGS)
}
}

impl Secp256k1<VerifyOnly> {
impl<'buf> Secp256k1<'buf, VerifyOnly> {
const FLAGS: c_uint = ffi::SECP256K1_START_VERIFY;
/// Creates a new Secp256k1 context that can only be used for verification
pub fn verification_only() -> Secp256k1<VerifyOnly> {
Secp256k1 { ctx: unsafe { ffi::secp256k1_context_create(ffi::SECP256K1_START_VERIFY) }, phantom: PhantomData }
pub fn verification_only() -> Secp256k1<'buf, VerifyOnly> {
Secp256k1 { ctx: unsafe { ffi::secp256k1_context_create(Self::FLAGS) }, phantom: PhantomData, _buf: None }
}

pub fn get_preallocated_verification_only_size() -> usize {
Self::get_preallocated_size_internal(Self::FLAGS)
}

pub fn new_preallocated_verification_only(buf: &'buf mut [u8]) -> Result<Secp256k1<'buf, VerifyOnly>, Error>{
Self::new_preallocated_internal(buf, Self::FLAGS)
}
}

impl<C> Secp256k1<C> {
impl<'buf, C> Secp256k1<'buf, C> {

/// Getter for the raw pointer to the underlying secp256k1 context. This
/// shouldn't be needed with normal usage of the library. It enables
Expand Down Expand Up @@ -650,9 +688,25 @@ impl<C> Secp256k1<C> {
}
}

pub(crate) fn get_preallocated_size_internal(flags: c_uint) -> usize {
unsafe { ffi::secp256k1_context_preallocated_size(flags) }
}

pub(crate) fn new_preallocated_internal(buf: &'buf mut [u8], flags: c_uint) -> Result<Secp256k1<'buf, C>, Error> {
if buf.len() < Self::get_preallocated_size_internal(flags) {
return Err(Error::NotEnoughMemory);
}

Ok(Secp256k1 {
ctx: unsafe { ffi::secp256k1_context_preallocated_create(buf.as_mut_ptr() as *mut c_void, flags) },
phantom: PhantomData,
_buf: Some(buf)
})
}

}

impl<C: Signing> Secp256k1<C> {
impl<'buf, C: Signing> Secp256k1<'buf, C> {

/// Constructs a signature for `msg` using the secret key `sk` and RFC6979 nonce
/// Requires a signing-capable context.
Expand Down Expand Up @@ -685,7 +739,7 @@ impl<C: Signing> Secp256k1<C> {
}
}

impl<C: Verification> Secp256k1<C> {
impl<'buf, C: Verification> Secp256k1<'buf, C> {
/// Checks that `sig` is a valid ECDSA signature for `msg` using the public
/// key `pubkey`. Returns `Ok(true)` on success. Note that this function cannot
/// be used for Bitcoin consensus checking since there may exist signatures
Expand Down Expand Up @@ -741,6 +795,7 @@ mod tests {
use super::constants;
use super::{Secp256k1, Signature, Message};
use super::Error::{InvalidMessage, IncorrectSignature, InvalidSignature};
use ::{SignOnly, VerifyOnly, All};

macro_rules! hex {
($hex:expr) => ({
Expand All @@ -756,6 +811,23 @@ mod tests {
let vrfy = Secp256k1::verification_only();
let full = Secp256k1::new();

capabilities_internal(sign, vrfy, full);

}

#[test]
fn preallocated_capabilities() {
let mut sign_buf = vec![0u8; Secp256k1::get_preallocated_signing_only_size()];
let mut verify_buf = vec![0u8; Secp256k1::get_preallocated_verification_only_size()];
let mut all_buf = vec![0u8; Secp256k1::get_preallocated_size()];
let sign: Secp256k1<SignOnly> = Secp256k1::new_preallocated_signing_only(&mut sign_buf).unwrap();
let vrfy: Secp256k1<VerifyOnly> = Secp256k1::new_preallocated_verification_only(&mut verify_buf).unwrap();
let full: Secp256k1<All> = Secp256k1::new_preallocated(&mut all_buf).unwrap();

capabilities_internal(sign, vrfy, full);
}

fn capabilities_internal(sign: Secp256k1<SignOnly>, vrfy: Secp256k1<VerifyOnly>, full: Secp256k1<All>) {
let mut msg = [0u8; 32];
thread_rng().fill_bytes(&mut msg);
let msg = Message::from_slice(&msg).unwrap();
Expand Down