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.

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

context: introduce alternate global context scheme based on #346 #539

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
37 changes: 33 additions & 4 deletions secp256k1-sys/depend/secp256k1.c.patch
Original file line number Diff line number Diff line change
@@ -1,4 +1,33 @@
139,149d138
61a62,89
> /* rust-secp: add two static contexts which are initialized for signing. We
> * need two so that we can effect updates (i.e. rerandomization) by atomic
> * pointer assignment */
> static secp256k1_context secp256k1_context_signing_1_ = {
> {
> 1,
> SECP256K1_SCALAR_CONST(0, 0, 0, 0, 0, 0, 0, 0),
> SECP256K1_GEJ_CONST_INFINITY
> },
> { secp256k1_default_illegal_callback_fn, 0 },
> { secp256k1_default_error_callback_fn, 0 },
> 0
> };
> secp256k1_context *secp256k1_context_signing_1 = &secp256k1_context_signing_1_;
>
> static secp256k1_context secp256k1_context_signing_2_ = {
> {
> 1,
> SECP256K1_SCALAR_CONST(0, 0, 0, 0, 0, 0, 0, 0),
> SECP256K1_GEJ_CONST_INFINITY
> },
> { secp256k1_default_illegal_callback_fn, 0 },
> { secp256k1_default_error_callback_fn, 0 },
> 0
> };
> secp256k1_context *secp256k1_context_signing_2 = &secp256k1_context_signing_2_;
> /* end rust-secp */
>
107,117d125
< secp256k1_context* secp256k1_context_create(unsigned int flags) {
< size_t const prealloc_size = secp256k1_context_preallocated_size(flags);
< secp256k1_context* ctx = (secp256k1_context*)checked_malloc(&default_error_callback, prealloc_size);
Expand All @@ -10,7 +39,7 @@
< return ctx;
< }
<
164,174d152
128,138d135
< secp256k1_context* secp256k1_context_clone(const secp256k1_context* ctx) {
< secp256k1_context* ret;
< size_t prealloc_size;
Expand All @@ -22,15 +51,15 @@
< return ret;
< }
<
183,189d160
146,152d142
< void secp256k1_context_destroy(secp256k1_context* ctx) {
< if (ctx != NULL) {
< secp256k1_context_preallocated_destroy(ctx);
< free(ctx);
< }
< }
<
206,215d176
169,178d158
< }
<
< secp256k1_scratch_space* secp256k1_scratch_space_create(const secp256k1_context* ctx, size_t max_size) {
Expand Down
14 changes: 9 additions & 5 deletions secp256k1-sys/depend/secp256k1.h.patch
Original file line number Diff line number Diff line change
@@ -1,21 +1,25 @@
226,228d225
206a207,209
> SECP256K1_API extern secp256k1_context *secp256k1_context_signing_1;
> SECP256K1_API extern secp256k1_context *secp256k1_context_signing_2;
>
218,220d220
< SECP256K1_API secp256k1_context* secp256k1_context_create(
< unsigned int flags
< ) SECP256K1_WARN_UNUSED_RESULT;
231,233d227
231,233d230
< SECP256K1_API secp256k1_context* secp256k1_context_clone(
< const secp256k1_context* ctx
< ) SECP256K1_ARG_NONNULL(1) SECP256K1_WARN_UNUSED_RESULT;
248,250d241
248,250d244
< SECP256K1_API void secp256k1_context_destroy(
< secp256k1_context* ctx
< ) SECP256K1_ARG_NONNULL(1);
327,330d317
327,330d320
< SECP256K1_API SECP256K1_WARN_UNUSED_RESULT secp256k1_scratch_space* secp256k1_scratch_space_create(
< const secp256k1_context* ctx,
< size_t size
< ) SECP256K1_ARG_NONNULL(1);
338,341d324
338,341d327
< SECP256K1_API void secp256k1_scratch_space_destroy(
< const secp256k1_context* ctx,
< secp256k1_scratch_space* scratch
Expand Down
3 changes: 3 additions & 0 deletions secp256k1-sys/depend/secp256k1/include/secp256k1.h
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,9 @@ typedef int (*rustsecp256k1_v0_6_1_nonce_function)(
*/
SECP256K1_API extern const rustsecp256k1_v0_6_1_context *rustsecp256k1_v0_6_1_context_no_precomp;

SECP256K1_API extern rustsecp256k1_v0_6_1_context *rustsecp256k1_v0_6_1_context_signing_1;
SECP256K1_API extern rustsecp256k1_v0_6_1_context *rustsecp256k1_v0_6_1_context_signing_2;

/** Create a secp256k1 context object (in dynamically allocated memory).
*
* This function uses malloc to allocate memory. It is guaranteed that malloc is
Expand Down
28 changes: 28 additions & 0 deletions secp256k1-sys/depend/secp256k1/src/secp256k1.c
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,34 @@ static const rustsecp256k1_v0_6_1_context rustsecp256k1_v0_6_1_context_no_precom
};
const rustsecp256k1_v0_6_1_context *rustsecp256k1_v0_6_1_context_no_precomp = &rustsecp256k1_v0_6_1_context_no_precomp_;

/* rust-secp: add two static contexts which are initialized for signing. We
* need two so that we can effect updates (i.e. rerandomization) by atomic
* pointer assignment */
static rustsecp256k1_v0_6_1_context rustsecp256k1_v0_6_1_context_signing_1_ = {
{
1,
SECP256K1_SCALAR_CONST(0, 0, 0, 0, 0, 0, 0, 0),
SECP256K1_GEJ_CONST_INFINITY
},
{ rustsecp256k1_v0_6_1_default_illegal_callback_fn, 0 },
{ rustsecp256k1_v0_6_1_default_error_callback_fn, 0 },
0
};
rustsecp256k1_v0_6_1_context *rustsecp256k1_v0_6_1_context_signing_1 = &rustsecp256k1_v0_6_1_context_signing_1_;

static rustsecp256k1_v0_6_1_context rustsecp256k1_v0_6_1_context_signing_2_ = {
{
1,
SECP256K1_SCALAR_CONST(0, 0, 0, 0, 0, 0, 0, 0),
SECP256K1_GEJ_CONST_INFINITY
},
{ rustsecp256k1_v0_6_1_default_illegal_callback_fn, 0 },
{ rustsecp256k1_v0_6_1_default_error_callback_fn, 0 },
0
};
rustsecp256k1_v0_6_1_context *rustsecp256k1_v0_6_1_context_signing_2 = &rustsecp256k1_v0_6_1_context_signing_2_;
/* end rust-secp */

size_t rustsecp256k1_v0_6_1_context_preallocated_size(unsigned int flags) {
size_t ret = sizeof(rustsecp256k1_v0_6_1_context);
/* A return value of 0 is reserved as an indicator for errors when we call this function internally. */
Expand Down
7 changes: 7 additions & 0 deletions secp256k1-sys/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ pub mod types;
pub mod recovery;

use core::{slice, ptr};
use core::sync::atomic::AtomicPtr;
use types::*;

/// Flag for context to enable no precomputation
Expand Down Expand Up @@ -510,6 +511,12 @@ extern "C" {
#[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_context_no_precomp")]
pub static secp256k1_context_no_precomp: *const Context;

#[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_context_signing_1")]
pub static secp256k1_context_signing_1: AtomicPtr<Context>;

#[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_context_signing_2")]
pub static secp256k1_context_signing_2: AtomicPtr<Context>;

// Contexts
#[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_context_preallocated_destroy")]
pub fn secp256k1_context_preallocated_destroy(cx: *mut Context);
Expand Down
94 changes: 94 additions & 0 deletions src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,100 @@ use crate::ffi::types::{c_uint, c_void, AlignedType};
use crate::ffi::{self, CPtr};
use crate::{Error, Secp256k1};

// This module will be renamed to `global` in later commit which eliminates the other `global` module
#[cfg(feature = "std")]
pub mod global_ {
use core::cell::RefCell;

use crate::{ffi, All, Secp256k1};

thread_local! {
pub static CONTEXT: RefCell<Secp256k1<All>> = RefCell::new(Secp256k1::new());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could actually use pure UnsafeCell and just make sure we don't access it twice from our methods nor call any user-provided callbacks. Maybe not worth optimizing though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should do this, but not on the first iteration. RefCell will let us do development "in debug mode" :)


/// Do some operation using an immutable reference to the global signing context
///
/// Safety: you are being given a raw pointer. Do not mutate through it. Do not
/// move it, or any reference to it, across thread boundaries.
pub unsafe fn with_global_context<F: FnOnce(*const ffi::Context) -> R, R>(f: F) -> R {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use &ffi::Context instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, neat! I spent so long going back and forth on ffi::Context vs Secp256k1<All> vs some custom type that I didn't even consider the pointer type.

This is a good idea.

CONTEXT.with(|refcell| f(refcell.borrow().ctx as *const _))
}

/// Rerandomize the global signing context
///
/// # Panics
///
/// Will panic if called from a closure passed to `with_global_ctx`.
pub fn rerandomize_context(seed: &[u8; 32]) {
CONTEXT.with(|refcell| {
let borrow = refcell.borrow_mut();
let bare = borrow.ctx;
unsafe {
let ret = ffi::secp256k1_context_randomize(bare, seed.as_ptr());
assert_eq!(ret, 1); // sanity check: context_randomize cannot fail
}
});
}
}

#[cfg(not(feature = "std"))]
pub mod global_ {
use core::sync::atomic::{AtomicBool, Ordering};

use crate::ffi;

static LOCK: AtomicBool = AtomicBool::new(false);

// Safety: all of the unsafe blocks in this module are due to our accessing
// extern statics (the ffi::* context objects). Rust cannot control access
// to these, so we need unsafe.
//
// In particular, we are promising here that nothing else will concurrently
// access ffi::secp256k1_context_signing_{1, 2} ... though in principle,
// any code that can see the `ffi` module will be able to do so. We can only
// make promises about our own code.
//
// Other accessors, who to be clear SHOULD NOT EXIST, will need their own
// unsafe block. As far as I know this is the best we can do.

/// Do some operation using an immutable reference to the global signing context
pub unsafe fn with_global_context<F: FnOnce(*const ffi::Context) -> R, R>(f: F) -> R {
let ctx = unsafe { ffi::secp256k1_context_signing_1.load(Ordering::Acquire) };
f(ctx as *const _)
}

/// Do some operation using a mutable reference to the global signing context
pub fn rerandomize_context(seed: &[u8; 32]) {
if LOCK.swap(true, Ordering::Acquire) {
// Concurrent operation in progress; give up
return;
}

unsafe {
// Obtain a pointer to the alternate context.
let alt_ctx = ffi::secp256k1_context_signing_2.load(Ordering::Acquire);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is never modified so we shouldn't need atomic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, right. It was modified in a first iteration of this, and my on-paper design (when I was trying to use AtomicPtr::swap on two AtomicPtrs directly, which annoyingly you cannot do).

But as you noticed in your above comment, this whole scheme doesn't seem to work, so it's a moot point.

// Obtain a pointer to the signing context, atomically replacing it with
// the alternate signing context (which never gets modified). Any concurrent
// callers to `with_global_ctx` will see the alternate context, not any
// inconsistent state of this one.
//
// (Concurrent callers to `with_global_ctx_mut` will fail after checking `LOCK`.).
let ctx = ffi::secp256k1_context_signing_1.swap(alt_ctx, Ordering::Acquire);
// Do the rerandomization. Note that unlike in the `std` case, we don't
// panic if this function fails (which again, it is guaranteed not to).
// The reason is that a panic at this point in the code would leave
// `LOCK` at true (as well as both `signing_1` and `signing_2` pointing
// at the same, never-rerandomized, place) and therefore prevent any
// future rerandomizations from succeeding.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could still panic after unlocking 🤷‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

Hah, good idea! Again, I was so worked up over an unrelated thing (whether to call randomize or let the user pass in an arbitrary-maybe-panicking closure) that I didn't consider this.

Code review ftw.

ffi::secp256k1_context_randomize(ctx, seed.as_ptr());
// Atomically swap completely-modified context into place
ffi::secp256k1_context_signing_1.swap(ctx, Ordering::Release);
};

LOCK.store(false, Ordering::Release);
}
}

#[cfg(all(feature = "global-context", feature = "std"))]
#[cfg_attr(docsrs, doc(cfg(all(feature = "global-context", feature = "std"))))]
/// Module implementing a singleton pattern for a global `Secp256k1` context.
Expand Down
19 changes: 19 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1054,6 +1054,25 @@ mod tests {
assert_eq!(msg.0, hash.into_inner());
assert_eq!(msg, Message::from_hashed_data::<hashes::sha256d::Hash>(test_bytes));
}

#[test]
fn test_sign_verify_nostd() {
let mut buf_ful = vec![AlignedType::zeroed(); Secp256k1::preallocate_size()];
let full = Secp256k1::preallocated_new(&mut buf_ful).unwrap();

let sk = SecretKey::from_slice(&[3u8; 32]).unwrap();
let pk = PublicKey::from_secret_key(&full, &sk);
let msg = Message::from_slice(&[2u8; 32]).unwrap();

// ECDSA
let sig = full.sign_ecdsa(&msg, &sk);
assert!(full.verify_ecdsa(&msg, &sig, &pk).is_ok());

// Schnorr
let kp = KeyPair::from_secret_key(&full, &sk);
let sig = full.sign_schnorr_no_aux_rand(&msg, &kp);
assert!(full.verify_schnorr(&sig, &msg, &kp.x_only_public_key().0).is_ok());
}
}

#[cfg(bench)]
Expand Down
29 changes: 16 additions & 13 deletions src/schnorr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,19 +103,22 @@ impl<C: Signing> Secp256k1<C> {
nonce_data: *const ffi::types::c_uchar,
) -> Signature {
unsafe {
let mut sig = [0u8; constants::SCHNORR_SIGNATURE_SIZE];
assert_eq!(
1,
ffi::secp256k1_schnorrsig_sign(
self.ctx,
sig.as_mut_c_ptr(),
msg.as_c_ptr(),
keypair.as_c_ptr(),
nonce_data,
)
);

Signature(sig)
crate::context::global_::rerandomize_context(b"this is a 32-byte random string!");
crate::context::global_::with_global_context(|ctx| {
let mut sig = [0u8; constants::SCHNORR_SIGNATURE_SIZE];
assert_eq!(
1,
ffi::secp256k1_schnorrsig_sign(
ctx,
sig.as_mut_c_ptr(),
msg.as_c_ptr(),
keypair.as_c_ptr(),
nonce_data,
)
);

Signature(sig)
})
}
}

Expand Down