Currently we rely on the inner bytes with types that are passed across
the FFI boundry when implementing comparison functions (e.g. `Ord`,
`PartialEq`), this is incorrect because the bytes are opaque, meaning
the byte layout is not guaranteed across versions of `libsecp26k1`.
Implement stable comparison functionality by doing:
- Implement `core::cmp` traits by first coercing the data into a stable
form e.g., by serializing it.
- Add fast comparison methods to `secp256k1-sys` types that wrap types
from libsecp, add similar methods to types in `secp256k1` that wrap
`secp256k1-sys` types (just call through to inner type).
- In `secp256k1-sys` feature gate the new `core::cmp` impls on
`not(fuzzing)`, when fuzzing just derive the impls instead.
Any additional methods added to `secp256k1-sys` types are private,
justified by the fact the -sys is meant to be just a thin wrapper around
libsecp256k1, we don't want to commit to supporting additional API
functions.
Please note, the solution presented in this patch is already present for
`secp256k1::PublicKey`, this PR removes that code in favour of deriving
traits that then call down to the same logic in `secp256k1-sys`.
Currently we have a few problems with our feature gating, attempt to
audit all feature gating and fix it by doing:
1. Do not enable features on optional dependencies (`rand` and
`bitcoin-hashes`) in dev-dependencies, doing so hides broken feature
gating in unit tests.
2. Do not use unnecessary feature combinations when one feature enables
another e.g. `any(feature = "std", feature = "alloc")`.
3. Enable "std" from "rand-std" and "bitcoin-std" (and fix features
gating as for point 2).
4. Clean up code around `rand::thread_rng`, this is part of this patch
because `thread_rng` requires the "rand-std" feature.
5. Clean up CI test script to test each feature individually now that
"rand-std" and "bitcoin-hashes-std" enable "std".
An array in Rust has no concept of length, it is a fixed size data type.
Equally an array cannot be "empty", again since it is a fixed size data
type. These are methods/concepts seen in slices and vectors.
Remove the `len` and `is_empty` methods.
The two crates `secp256k1` and `secp256k1-sys` serve very different
purposes, having a macro defined in one that is used in both makes it
hard to get nuanced things correct in the macro, for example the
comparison implementations (`Ord`, `PartialEq` etc.) are semantically
different in each crate.
In an effort to decouple `secp256k1` and `secp256k1-sys` duplicate the
`impl_array_newtype` macro.
e0e575dde7 Run cargo fmt (Tobin C. Harding)
41449e455d Prepare codebase for formatting (Tobin C. Harding)
7e3c8935b6 Introduce rustfmt config file (Tobin C. Harding)
Pull request description:
(Includes the patch from #504, I pulled it out of this to merge faster)
Introduce `rustfmt` by doing:
- Copy the `rustfmt` config file from `rust-bitcoin`
- Prepare the codebase by adding `#[rustfmt::skip]` as needed and doing some manual format improvements.
- Run the formatter: `cargo +nightly fmt`
- Add formatting checks to CI and the pre-commit hook
Thanks in advance for doing the painful review on patch 3.
ACKs for top commit:
apoelstra:
ACK e0e575dde7
Tree-SHA512: 1b6fdbaf81480c0446e660cc3f6ab7ac0697f272187f6fdfd6b95d894a418cde8cf1c423f1d18ebbe03ac5c43489630a35ad07912afaeb6107cfbe7338a9bed7
ade888e922 Check for broken links in CI (Tobin C. Harding)
e3f6d23b49 Fix incorrect method name in docs (Tobin C. Harding)
Pull request description:
- Patch 1: Fix broken link (links to recently removed deprecated function)
- Patch 2: Add `-- -D rustdoc::broken-intra-doc-links` to CI
cc dpc, wasn't on this repo but you brought this to my attention, thanks man!
ACKs for top commit:
apoelstra:
ACK ade888e922
Tree-SHA512: febe4dc3d8831d59edcc6ae1e6b31c48bc1ab8765a7c074573657350e906cd877ef2ed486adc656b09f3e2471d11cd3e57072a33f2f0279eb9cd13b2102f1cd7
Add `-- -D rustdoc::broken-intra-doc-links` to the docs build in CI to
check for broken links. In order to use this flag use `cargo rustdoc`
instead of `cargo doc`.
We are currently not checking for broken doc links in CI. Recently we
removed a bunch of deprecated functions, one of which was still referred
to in rustdocs.
Fix the docs to use the correct new method name.
cd7a6b316b Fix incorrect method call (Tobin C. Harding)
Pull request description:
We have the following method on `SecretKey`
```
pub fn sign_ecdsa(&self, msg: Message) -> ecdsa::Signature {
SECP256K1.sign_ecdsa(&msg, self)
}
```
But we have a method call in rustdocs
```
//! let (secret_key, public_key) = generate_keypair(&mut thread_rng());
//! let message = Message::from_hashed_data::<sha256::Hash>("Hello World!".as_bytes());
//!
//! let sig = secret_key.sign_ecdsa(&message, &secret_key);
```
This is incorrect, I have no idea why this code builds.
(Also see https://github.com/rust-bitcoin/rust-secp256k1/issues/508.)
ACKs for top commit:
apoelstra:
ACK cd7a6b316b
Tree-SHA512: 5f22157798a4e4a21fff946dd930c62c7d82100a8e729309f6383709cb3fefdd935df1b7954fc545f1c2283a46b8c22f61e1c6d4534cb750eb7ab3b5036eedf5
ec47198a17 Remove ONE_KEY (Tobin C. Harding)
d546c16134 Remove cfg docs feature requirements (Tobin C. Harding)
5a7cedef00 doc: Fix preallocated memory grammar (Tobin C. Harding)
Pull request description:
Read over and improve various parts of the crate documentation. Note this is an API breaking PR because it removes the public `ONE_KEY` type that we exposed when writing the docs for the `secret` module, exposing this type was, in my opinion, a mistake.
ACKs for top commit:
apoelstra:
ACK ec47198a17
Tree-SHA512: cf8573e58c9498093b0df3f240501d3ad0a9d65e07d2f7c3a9e4116bac6ba366d3d41ac695f4e79010597124512a43b32b4ecb02b08d81226c527d5f77a1a541
Run the command `cargo +nightly fmt` to fix formatting issues.
The formatter got confused in one place, adding an incorrect
indentation, this was manually fixed.
As we did in `rust-bitcoin` introduced a `rustfmt` configuration file
that is palatable to devs.
Do not run formatter, that is done as a separate patch to assist review.
We have the following method on `SecretKey`
```
pub fn sign_ecdsa(&self, msg: Message) -> ecdsa::Signature {
SECP256K1.sign_ecdsa(&msg, self)
}
```
But we have a method call in rustdocs
```
//! let (secret_key, public_key) = generate_keypair(&mut thread_rng());
//! let message = Message::from_hashed_data::<sha256::Hash>("Hello World!".as_bytes());
//!
//! let sig = secret_key.sign_ecdsa(&message, &secret_key);
```
This is incorrect and is currently not running because the feature guard
is incorrectly spelled, it contains the word "features" instead of
"feature".
The `ONE_KEY` is only used in two rustdoc examples, as such it
unnecessarily pollutes the crate root namespace. We can use
`SecretKey::from_str()` with no loss of clarity and remove the
`ONE_KEY`.
While we are touching the import statements in `secret.rs` elect to
remove the hide (use of `#`) for import statements relating to this
library. Doing so gives devs all the information they need in one place
if they are using the examples to copy code. It is also in line with the
rest of the codebase.
The `alloc_only` module already has a docs guard on the "alloc" feature,
using an additional docs guard on the `SignOnly`, `VerifyOnly`, `All`
enums leads to a redundant feature combination "alloc" and "alloc or
std" - we really only require "alloc".
92b733386f Support non-WASM platforms that are missing `string.h` (Matt Corallo)
Pull request description:
Dunno why we haven't seen this elsewhere, but when trying to build locally for an ARM embedded target `secp256k1-sys` failed to compile as it was missing `string.h`, just like WASM.
This patch adds a trivial fallback - if we fail to compile initially we unconditionally retry with the wasm-sysroot, giving us a valid `string.h`.
ACKs for top commit:
tcharding:
ACK 92b733386f
apoelstra:
ACK 92b733386f
Tree-SHA512: 81cbc5023f349681a3bef138506d9314be948b8b7b78bb2b2ffacf43b0c97d92ea67238105009a94b05a0a3adbd4113ed68f79a0a303708d95c6a7f520d5170e
b0d0b2afcb Improve feature usage bitcoin-hashes[-std] (Tobin C. Harding)
Pull request description:
Currently we have a feature `bitcoin-hashes-std` and a dependency `bitcoin_hashes`, this means one has to think about and change the `_` and `-` when coding. The underscore in `bitcoin_hashes` is an artifact of days gone by and we cannot fix it but we can cover it up and make our lives easier, especially now we have `bitcoin-hashes-std`.
Improve feature usage of the `bitcoin_hashes` library by:
- Add a feature `bitcoin-hashes` that enables `bitcoin_hashes`.
- Use the new feature in all feature gated code
- Use `bitcoin-hashes-std` in feature gated code that includes other `std` features (e.g. `rand-std`)
ACKs for top commit:
apoelstra:
ACK b0d0b2afcb
Tree-SHA512: e6a86fe2c5b249a6c32b0fdedaeb8e25c47a30a4709f4fc4020cc1762747fe5d25883e2340ff77698079c9ee397491984889d3c1aaf195ca27eec09a77f62978
68c73850d8 Minimise FFI in the public API (Tobin C. Harding)
Pull request description:
Normal users should never need to directly interact with the FFI layer.
Audit and reduce the use of `ffi` types in the public API of various types. Leave only the implementation of `CPtr`, and document this clearly as not required by normal users. Done for:
- PublicKey
- XOnlyPublicKey
- KeyPair
- ecdsa::Signature
- ecdsa::RecoverableSignature
ACKs for top commit:
apoelstra:
ACK 68c73850d8
Tree-SHA512: 8242527837872f9aba2aab19b02c2280ca1eb1dfd33c8ca619726d981811d72de3e5a57cbde2fbe621eb8e50e43f488804cd51d27949459da1c0ceb03fca35e3
8c7c5e7394 Remove deprecated code (Tobin C. Harding)
e779e5dc05 doc: Use add_tweak in example code (Tobin C. Harding)
eedbd0b7e4 secp256k1-sys: Remove deprecated code (Tobin C. Harding)
Pull request description:
Remove deprecated code from `secp256k1-sys` and `secp256k1`.
ACKs for top commit:
apoelstra:
ACK 8c7c5e7394
Tree-SHA512: 830d4459cf21fba98e75e1c099c96316c9db1c1fb87dd28343cea066544ac8568685ec9fc85969caee3d35014f64c3f42b5a5afbf4f4d16221a57a204e6a3524
5ccf0c8db7 Manually implement PartialEq, Eq, and Hash for PublicKey (Tobin C. Harding)
Pull request description:
`PartialEq` and `Eq` should agree with `PartialOrd` and `Ord` but we are deriving `PartialEq`/`Eq` and doing a custom implementation of `PartialOrd` and `Ord` (that calls down to ffi functions).
If two keys are equal their hashes should be equal so, we should add a custom implementation of `Hash` also. In order to guarantee the digest will be the same across library versions first serialize the key before hashing it.
Add custom implementation of `PartialEq`, `Eq`, and `Hash` when not fuzzing.
Please note, this is for the main `PublicKey` type, the patch does not effect the `ffi::PublicKey`, nor do we call methods on the `ffi::PublicKey`.
EDIT: Please note the comment below by apoelstra about the possible performance hit introduced by this PR.
ACKs for top commit:
apoelstra:
ACK 5ccf0c8db7
Tree-SHA512: 1464308238411d259bb0493dc1eca775ec235036eef10b91f70ef17816174f452d5911ecae3b40434b71f9866be1db54d69e8ed9475a4f2801c07a800aead2b2
Currently we have a feature `bitcoin-hashes-std` and a dependency
`bitcoin_hashes`, this means one has to think about and change the `_`
and `-` when coding. The underscore in `bitcoin_hashes` is an artifact
of days gone by and we cannot fix it but we can cover it up and make our
lives easier, especially now we have `bitcoin-hashes-std`.
Improve feature usage of the `bitcoin_hashes` library by:
- Add a feature `bitcoin-hashes` that enables `bitcoin_hashes`.
- Use the new feature in all feature gated code
- Use `bitcoin-hashes-std` in feature gated code that includes other
`std` features (e.g. `rand-std`)
`PartialEq` and `Eq` should agree with `PartialOrd` and `Ord` but we are
deriving `PartialEq`/`Eq` and doing a custom implementation of
`PartialOrd` and `Ord` (that calls down to ffi functions).
If two keys are equal their hashes should be equal so, we should add a
custom implementation of `Hash` also. In order to guarantee the digest
will be the same across library versions first serialize the key before
hashing it.
Add custom implementation of `PartialEq`, `Eq`, and `Hash` when not
fuzzing.
Please note, this is for the main `PublicKey` type, the patch does not
effect the `ffi::PublicKey`, nor do we call methods on the
`ffi::PublicKey`.
Normal users should never need to directly interact with the FFI layer.
Audit and reduce the use of `ffi` types in the public API of various
types. Leave only the implementation of `CPtr`, and document this
clearly as not required by normal users. Done for:
- PublicKey
- XOnlyPublicKey
- KeyPair
- ecdsa::Signature
- ecdsa::RecoverableSignature
603f441548 Add array constants (Tobin C. Harding)
Pull request description:
In multiple places we use array constants for zero and one. Add two constants and use them throughout the codebase. Note the endian-ness of `ONE` in the docs.
ACKs for top commit:
apoelstra:
ACK 603f441548
Tree-SHA512: 70c455ee42f8a04feec37c3963b030c0f2c07b83801caf818dbb1661b7a0f65c4b92ff6a5df496a4dd6a917d13af4d60624a072c6f8a083293db9cd80d194232
In multiple places we use array constants for zero and one. Add two
constants and use them throughout the codebase. Note the endian-ness of
`ONE` in the docs.
1c17d0f215 Improve docs on impl_array_newtype (Tobin C. Harding)
91ac518d17 Use generic implementation of Index (Tobin C. Harding)
Pull request description:
Instead of all the manual implementations of `Index` for ranged types we can just use a generic implementation as we do in `rust-bitcoin/internals/src/macros.rs`.
Patch 2 does some trivial docs improvements to the `impl_array_newtype` macro since we are touching it anyways.
ACKs for top commit:
apoelstra:
ACK 1c17d0f215
Tree-SHA512: 6b37933659841af51c8abed3caeca83e63972d82be0a6483d7cdb804242986075f3d93e72b73072d496097224ed8130b6eee6858bf9d76205df4016ff012fa00
a59028c965 Use clang instead of clang-9 (Tobin C. Harding)
Pull request description:
The current version of clang is 14, there is no obvious reason why we use clang-9 (as far as I can tell on my local machine).
Use `clang` instead of `clang-9` so that the latest version is used by default. This effects the version installed by CI as well as the version used to run commands in `test.sh`.
ACKs for top commit:
apoelstra:
ACK a59028c965
Tree-SHA512: b3c5a56d21dc0bc8cb80db9854917b39c86ba735434b3a644eb22608492b07558ddff4b2ee3ff5b14e066d31fbdcc890d4c9e3f44af7b1d62c5e8eab6d31b90e
76a0804ca5 Fix typo in public method (Tobin C. Harding)
Pull request description:
We have a method called `from_raw_signining_only`, I'm guessing this should be `from_raw_signing_only`.
ACKs for top commit:
apoelstra:
ACK 76a0804ca5
Tree-SHA512: ee03dbf3f69f0b348a483fa928fab9fba73bfdea383aee385a853b99998a882695c6839fff6433784ad097709ca31c67fc0a4e1a948caae7356be2eab7e332e5
The current version of clang is 14, there is no obvious reason why we
use clang-9 (as far as I can tell on my local machine).
Use `clang` instead of `clang-9` so that the latest version is used by
default. This effects the version installed by CI as well as the
version used to run commands in `test.sh`.
5417fad7cb Add method SecretKey::from_hashed_data (Tobin C. Harding)
Pull request description:
Analogous to the method on `Message`; add a constructor method on `SecretKey` that hashes the input data.
While we are at it improve the rustdocs on `Message::from_hashed_data` so docs on both methods are uniform.
Fix: #487
ACKs for top commit:
apoelstra:
ACK 5417fad7cb
Tree-SHA512: d321c1e8fddaf5ee692a7f119d86749ea4c8b4f3796f06e8c6145aa03bc22f5c88992e193dd34aa7ba3da8a45cf8f60e72f61e415a092ad16d2bd8c2b6c8fa23
Analogous to the method on `Message`; add a constructor method on
`SecretKey` that hashes the input data.
While we are at it improve the rustdocs on `Message::from_hashed_data`
so docs on both methods are uniform.
1f327b478a Bump version number to v0.24.1 (elsirion)
53c1354cc5 Fix broken `serde::Deserialize` and `FromStr` impl of `keyPair` (elsirion)
Pull request description:
Fixes#491
ACKs for top commit:
apoelstra:
ACK 1f327b478a
Tree-SHA512: 1af54667b7a1b310035fa35bd2aeb508e432d8c7f153ae1b9850431ba77dcc3e2194c1cda45a1ed5218d955d9284ba6512cf8ab6dafc673f23ccdad7c601b1b6
91f10965b3 secp-sys: change symbol names to `0_6_1` from `0_5_0` (Andrew Poelstra)
Pull request description:
Needed to build secp-sys 0.5 and secp-sys 0.6 in the same tree. Fixes#489.
This PR can be reproduced by running
./vendor-libsecp.sh depend/ 0_6_1 a1102b12196ea27f44d6201de4d25926a2ae9640
in the secp256k1-sys directory.
ACKs for top commit:
elichai:
tACK 91f10965b3
Tree-SHA512: 0ce5149c9c4b7b44592dec84f1a6348f62437e679c15300efe0e2cc55ced5746e6061c596c83e18428841efb7df07c5cb443a0fd81800dc2a05da9a4f7a07c1a
Needed to build secp-sys 0.5 and secp-sys 0.6 in the same tree. Fixes#489.
This PR can be reproduced by running
./vendor-libsecp.sh depend/ 0_6_1 a1102b12196ea27f44d6201de4d25926a2ae9640
in the secp256k1-sys directory.
Dunno why we haven't seen this elsewhere, but when trying to build
locally for an ARM embedded target `secp256k1-sys` failed to
compile as it was missing `string.h`, just like WASM.
This patch adds a trivial fallback - if we fail to compile
initially we unconditionally retry with the wasm-sysroot, giving us
a valid `string.h`.
0f29348b6c move some unsafe code inside an unsafe{} boundary (Andrew Poelstra)
Pull request description:
An internal function had a non-unsafe signature but could be called
with data that would cause it to exhibit UB. Move the unsafety inside
of the function so that the function signature now enforces soundness.
Fixes#481
Top commit has no ACKs.
Tree-SHA512: b1ffc643aa11e9c8d0b7a32965a1504da14f6ac3f9e0aa175d2c09d7d7b6bf84e228f64e1f57800d75500e2c65066a4991f0070a3a1d0a19c1bd84ca0dd44363
An internal function had a non-unsafe signature but could be called
with data that would cause it to exhibit UB. Move the unsafety inside
of the function so that the function signature now enforces soundness.
Fixes#481
Tobin C. Harding
Andrew Poelstra
elsirion
Matt Corallo