Rust language bindings for Bitcoin secp256k1 library.

Describe the bug I have the error that i describe later with last release of rust-bitcoin ( no problem with previous versions).

Build environment

OS+version: 20.04.01 Rust/Cargo version: 1.61.0 Rust/Cargo target: wasm32-unknown-unknown

That's the error:

Uncaught (in promise) RuntimeError: unreachable
at __rust_start_panic (67d29ce8a8f7d806078e.module.wasm:0xc6d5d8)
at rust_panic (67d29ce8a8f7d806078e.module.wasm:0xc55f4c)
at std::panicking::rust_panic_with_hook::he2a025723e105e28 (67d29ce8a8f7d806078e.module.wasm:0x960d29)
at std::panicking::begin_panic_handler::{{closure}}::hd9f8c213ec91b9d5 (67d29ce8a8f7d806078e.module.wasm:0xa6ac56)
at std::sys_common::backtrace::__rust_end_short_backtrace::h6efd730283875809 (67d29ce8a8f7d806078e.module.wasm:0xc6c185)
at rust_begin_unwind (67d29ce8a8f7d806078e.module.wasm:0xc16d18)
at core::panicking::panic_fmt::hb02133958c1e7d35 (67d29ce8a8f7d806078e.module.wasm:0xc24531)
at rand::rngs:thread:THREAD_RNG_KEY::__init::{{closure}}::ha2ad45ff6630e75f (67d29ce8a8f7d806078e.module.wasm:0xa55aef)
at core::result::Result<T,E>::unwrap_or_else::hdc727ae752f97c76 (67d29ce8a8f7d806078e.module.wasm:0x73eae4)
at rand::rngs:thread:THREAD_RNG_KEY::__init::hfb75fbbba2c214fb (67d29ce8a8f7d806078e.module.wasm:0x80efe3)

If anyone wants to repro, check out this commit: https://github.com/diba-io/bitmask-core/commit/76e83c953b0c7ecd5006b1c9d32c01e4cb644c6d

Hello, I'm unable to create a PR against non-existing branch, so I'm opening this as an issue.

I managed to implement version 0.15.6 in terms of using secp256k1-sys. This is very useful because it avoids linking the C library twice, creating link errors. Such errors prevented me from building lnp-node. It compiles fine when my change is used (tested via patch in Cargo.toml). CC @dr-orlovsky, you probably want to know about this. :)

All tests (except fuzztarget) for secp256k1 are passing of course. I guess fuzztarget is something special used for fuzzing only, so it doesn't matter that it's not passing, right?

It'd be nice if you created a new branch (maybe without the -lazy-semver-trick suffix) in your repo with my changes and published the 0.15.6 crate to crates.io. I created two commits expecting that it'll make code review easier, but feel free to squash it, if you want. (I wrote the commit message to be easy to modify. :))

See the description in https://github.com/Kixunil/rust-secp256k1/commit/10a33cc140a6180f79bfb9b79f6de7fbee8b71d5 to understand what I mean by "lazy" :)

I hope you will find this helpful!

Hi, This make this library support no-std. first I replaced everything from std to get it from core. then I had 2 things that I had to change and would love you input:

1. the C types from std::os::raw, I see few options. one which I did is just declare it manually which I don't really love, another is to make a feature if it will be received from std or libc.

2. Replacing the Vec in the DER serialization. I talked with Maxwell and he told me that the signature can be anywhere between 8 and 72 bytes, so the only way I thought I can achieve this without breaking the API is using the ArrayVec crate which initializes the Vector on the stack, A different approach will be accepting &mut [u8; 72] and returning a slice to the correct signature that will be a part of that mutable.

Currently SharedSecret provides a way to get a shared secret using SHA256 as well as a way to use a custom hash function to get the shared secret. Internally SharedSecret uses a 256 byte buffer, this is a tad wasteful. We would like to keep the current functionality but reduce memory usage.

• Patch 1: Pulls the new_with_hash logic out into a standalone public function that just returns the 64 bytes representing the x,y co-ordinates of the computed shared secret point. Callers are then responsible for hashing this point to get the shared secret (idea by @Kixunil, thanks).
• Patch 2: Does trivial refactor
• Patch 3: Uses a 32 byte buffer internally for SharedSecret. This is basically a revert of the work @elichai did to add the custom hashing logic. @elichai please holla if you are not happy with me walking all over this code :)

Note to reviewers

Secret obfuscation is done on top of this in https://github.com/rust-bitcoin/rust-secp256k1/pull/396, they could be reviewed in order if this work is of interest to you.

As BIP 340 support was merged in bitcoin core and that I had a branch of rust-secp256k1 with BIP 340 support I thought maybe now was a good time to try to get it merged.

Changes:

• Update the depend folder to the latest master
• Update secp256k1-sys to add Schnorr and extra keys modules, as well as the bindings for the related functions
• Add schnorrsig module in the rust-secp256k1

One thing I am unsure is whether types from the extra keys module should be used into the public interfaces (x-only pubkeys and keypairs). For now I implemented to just use PublicKey and SecretKey in public interface, but I can change it.

Currently we serialize keys using the BytesVisitor, this causes the serialized data to contain additional metadata encoding the length (an extra 8 bytes) when serialized with bincode.. This extra data is unnecessary since we know in advance the length of these two types.

We do not control the data output by serialization of our types because it depends on which crate is used to do the serialization. This PR improves the situation for serialization using the bincode crate, and this PR introduces mentions of bincode in the rustdocs, is this acceptable? See below for a table that describes binary serialization by other crates.

Implement a sequence based visitor that encodes the keys as fixed width data for:

• SecretKey
• PublicKey
• KeyPair
• XOnlyPublicKey

Fixes: #295

Question: PR only does keys, do we want to do signatures as well?

Behold, use-after-free!

use secp256k1::Secp256k1;

fn make_bad_secp() -> Secp256k1::<secp256k1::AllPreallocated<'static>> {
    // in principle `Box` is not needed but if it's not here it won't show up as a crash.
    let mut array = Box::new([secp256k1::ffi::types::AlignedType::ZERO; 1024]);
    secp256k1::Secp256k1::<secp256k1::AllPreallocated<'static>>::preallocated_gen_new(&mut *array).unwrap()
}

fn main() {
    let secp = make_bad_secp();
    let secret = secp256k1::SecretKey::from_slice(b"release the nasal daemons!!!!!!!").unwrap();
    let pubkey = secp256k1::PublicKey::from_secret_key(&secp, &secret);
    println!("Dear compiler, do not optimize this computation {}", pubkey);
}

Yep, no unsafe. Curiously, this seems to corrupt the heap on my machine or something because it crashes after printing the key. Anyway, it's still UB so I'm lucky I didn't get nasal daemons. :man_shrugging:

But, but, but there's C: 'buf bound!

The problem is C: 'buf means C outlives 'buf, so in our case 'static 'buf which is trivially true for all lifetimes 'buf. In theory the bound should've been inverted: 'buf: C. But sadly, that's not a valid Rust syntax.

The bad news is I see no way of avoiding putting lifetimes everywhere with the current design.

Possibilities I can think of:

• Make Secp256k1 unsized type (to prevent mem::swap which is curiously forbidden by secp). This will needlessly add size back as slice metadata but we can't do anything about it until extern types are stabilized and in our MSRV. I think Box<Secp256k1> could be valid, if not we need to have our own BoxedSecp256k1.
• Add a lifetime parameter to Context, Signing, and Verification traits contaminating everything.
• GAT can probably help but I can't think of how from top off my head and this would unreasonably bump MSRV so I don't want to spending much time on thinking about this one.

Maybe there's some ridiculous hack I didn't think about (my mind is not fresh rn), so if anyone knows that'd be great.

@apoelstra found another problem with the API: it's possible to cause invalid deallocation. Demo for reference:

fn main() {
    let mut array = [secp256k1::ffi::types::AlignedType::ZERO; 1024];
    let secp = secp256k1::Secp256k1::<secp256k1::All>::preallocated_gen_new(&mut array).unwrap();
    let secret = secp256k1::SecretKey::from_slice(b"release the nasal daemons!!!!!!!").unwrap();
    let pubkey = secp256k1::PublicKey::from_secret_key(&secp, &secret);
    println!("Dear compiler, do not optimize this computation {}", pubkey);
}

Notice that the array lives for the duration of the whole function so wrong lifetimes do not cause UB but invalid deallocation does.

As proposed in https://github.com/rust-bitcoin/rust-bitcoin/issues/510#issuecomment-881686342 this PR raises the MSRV to 1.41.1 it also changes the code to be Edition 2018.

The PR contains a few things:

• Moving to edition 2018 and fixing the imports
• Sorting and combining imports to make them more concise
• Replacing our c_void with core::ffi::c_void
• Bumping the rand version to latest and modifying our RngCore implementations accordingly
• Doing some small refactoring and using the new TryInto trait where it makes the code nicer

If people prefer I can split this PR into multiple and/or drop some commits

~We just did an MSRV bump, this typically would require bumping our major version number but since we are pre 1.0 we bump the minor version number.~

In preparation for release, write CHANGELOG release notes and bump the crate version to 0.23.0

Thanks to Kixunil for sifting through the PRs to make the changelog list, I added links to the relevant PRs.

Add an optional global, static context to avoid creating new ones over and over again when there's no option to pass a context. According to my understanding the context is immutable, doesn't do any locking internally and is basically just a bunch of lookup-tables.

As this might not be useful for everyone, introduces a dependency and would bump the minimum rust version to 1.27.2 I hid the functionality behind the new global-context feature. While these limitations are a bit annoying I still think this crate is the right place for something like that (instead of a bunch of other crates doing the same, leading to a bunch of redundant, static contexts).

Secp256k1::preallocated_*(buf) takes an arbitrary mutable slice of u8 as buffer; however it seems that, as it will be used to store pointers, it needs to be aligned to at least the pointer size of the platform.

I'm using a static buffer in my embedded project, and I've had it crash on ret->illegal_callback = default_illegal_callback in secp256k1_context_preallocated_create for no apparent reason until I discovered this requirement.

• A possible solution would be to take a *mut [u64] or *mut [usize] instead. This at least makes sure it's aligned enough to accept pointers
• Another one would be to accept buffers with arbitrary alignments, but make manual_alloc align the pointer (currently, it only aligns the size)
• Alternatively, let's add an assert! and at least document this. I now see it's documented at the C side:

/** Create a secp256k1 context object in caller-provided memory.
 *
 *  The caller must provide a pointer to a rewritable contiguous block of memory
 *  of size at least secp256k1_context_preallocated_size(flags) bytes, suitably
 *  aligned to hold an object of any type.

We can statically rule out invalid tweaks by having NonZeroScalar and have it constructible from ThirtyTwoBitHash. Unfortunately we can't do layout optimizations but even if we could we would have to drop ThirtyTwoBitHash or make it unsafe.

See also https://github.com/LLFourn/secp256kfun/issues/144

Improve the README files

Improve the secp256k1 readme by:

• Use a top level markdown header (level 1)
• Add a link to the SECG's website (www.secg.org)
• Add a link for secp256k1 to bitcoin.it explaining the curve

Improve the secp256k1-sys readme by:

• Mirror secp256k1 readme badges, heading, docs link
• Basic cleanup
  • Use 100 column width
  • Use backticks
  • Use capitals

Currently we have a wildcard on safety requirements, saying more or less "plus a bunch of other stuff we don't mention". This is not helpful.

Attempt to fully describe the safety requirements of creating a context from a raw context (all, signing only, and verification only).

Fix: #544

Note

This is best effort only, will require some thought to review. To do this I read https://doc.rust-lang.org/reference/behavior-considered-undefined.html and then I flicked through depend/secp256k1/src/secp256k1.c and util.h to look for things that could cause things in the linked to list of UB.

The code had a hard-coded maximum alignment of 16 (128 bits) which may have potentially wasted memory and cause memory unsafety if it was ever compiled with larger alignment (unlikely).

This change implements a trick to retrieve the actual alignment from C. To achieve this a newly added C file defines a symbol initialized to _Alignof(max_align_t). It is then compiled (as a separate object), the contents of the symbol is extracted and converted to decimal string which is injected into the AlignedType definition. The definition is generated as a separate .rs file in OUT_DIR and included into types.rs.

This POC currently doesn't support cross-compilation which I intend to fix, I just wanted to show that it's possible to do this. Do we want to?

I think that either AlignedType should contain MaybeUninit inside so that people can avoid zeroing or preallocated_gen_new should take something like [MaybeUninit<AlignedType>]. Zeroing can still be available by keeping ZERO associated const. We would just add UNINIT const.