Fixes unsoundness in `preallocated_gen_new` which previously did not
properly constrain the lifetime of the buffer used to back the context
object. We introduce an unsafe marker trait, and impl it for our
existing preallocated-context markers.
Annoyingly the trait has to be public even though it should never be
used directly, and is only used alongside the sealed `Context` trait,
so it is de-facto sealed itself.
Fixes#543
Currently we expect non-null pointers when we take `*mut T` parameters,
however we do not check that the pointers are non-null because we never
set VERIFY in our C build. We can use the `NonNull` type to enforce
no-null-ness as long as we use `NonNull::new`. In a couple of instances
we manually check that a buffer is not empty and therefore that the
pointer to it is non-null so we can safely use `NonNull::new_unchecked`.
Replace mutable pointer parameters `*mut T` (e.g. `*mut c_void`) and
return types with `NonNull<T>`.
Fix#546
For raw pointers that can never be null Rust provides the
`core::ptr::NonNull` type. Our `Secp256k1` type has an inner field that
is a non-null pointer; use `NonNull` for it.
Fix: #534
129ba3cd58 Remove size field (Tobin C. Harding)
Pull request description:
The `Secp256k1` `size` field is a cached value that we get using `ffi::secp256k1_context_preallocated_size` or
`ffi::secp256k1_context_preallocated_clone_size`, both of which just return the result of `sizeof(rustsecp256k1_v0_6_1_context)`. Instead of caching this value we can just call
`ffi::secp256k1_context_preallocated_clone_size` in the `Drop` implementation.
Fix#537
ACKs for top commit:
apoelstra:
ACK 129ba3cd58
Tree-SHA512: 3fce7863065e4b485fd2d1fdbbfe7002fa6188f1a703d89fdda570a1a32471d298e2e33fb8c5951a56a79facb5d2b427d58e473b5cb1d68eb02ffed728392b97
The `Secp256k1` `size` field is a cached value that we get using
`ffi::secp256k1_context_preallocated_size` or
`ffi::secp256k1_context_preallocated_clone_size`, both of which just
return the result of `sizeof(rustsecp256k1_v0_6_1_context)`. Instead of
caching this value we can just call
`ffi::secp256k1_context_preallocated_clone_size` in the `Drop`
implementation.
ecdad39ef4 context: Improve rustdocs (Tobin C. Harding)
e945751d85 schnorr: Improve rustdocs (Tobin C. Harding)
47f19a78ef Use lowercase for schnorr (Tobin C. Harding)
27b3e92889 Do trivial cleanup to module level docs (Tobin C. Harding)
Pull request description:
Audit of docs in `rust-secp256k1` and do a few trivial fixes. The docs are in pretty good condition, they just need more content as described in #128 if that issue is still valid.
ACKs for top commit:
apoelstra:
ACK ecdad39ef4
Tree-SHA512: 7466090325e02331f11e34cd38625541fbe8e642882afa6ddf2cf5d11ed669c7b2b48fd5b819915392760f4c6ef4ee460c2e622b3af648f99906c3ac408045d4
513144c923 Remove serde_keypair module (Tobin C. Harding)
Pull request description:
Done while unsuccessfully trying to solve https://github.com/rust-bitcoin/rust-secp256k1/issues/514#
The `serde_keypair` module appears to be only used for testing, however it is part of the public API for the `key` module?
serde de/serialization is already implemented on `KeyPair` by way of the normal `serde` traits, there is no obvious reason for the `serde_keypair` and the `KeyPairWrapper`.
Remove the `KeyPairWrapper` and test `KeyPair` serde impls directly.
ACKs for top commit:
apoelstra:
ACK 513144c923
Tree-SHA512: 23891217f3afc7cb3bb03431946e9866ee6ae611153fca8d93fe393b5a4abbd41d4713c6aa5ab24eb2734d8c8d94a9f6aed47316284b4097aa40f49f055f36b6
6d747301e8 secp256k1: Document safety constraints (Tobin C. Harding)
85681cece7 secp256k1-sys: Document safety constraints (Tobin C. Harding)
Pull request description:
Functions that are `unsafe` should include a `# Safety` section.
- Patch 1: Documents `unsafe` methods in `secp256k1-sys`. Please not this includes a minor refactor.
- Patch 2: Documents the `unsafe` `Context` trait
Together these two patches remove `#![allow(clippy::missing_safety_doc)]` from the repo.
Fix: #447
## Note to reviewers
The only function that was curly to understand the safety of was `secp256k1_context_create`, all the other stuff should be trivial to review.
ACKs for top commit:
apoelstra:
ACK 6d747301e8
Tree-SHA512: 247216c3f9e655fe8c2854b71613b31b6241318e877e83a1e4873ce84e481975a832d05cd748577f437f88b166ff287a537d26c012568e7378caed458ec55867
The `serde_keypair` module appears to be only used for testing, however
it is part of the public API for the `key` module?
serde de/serialization is already implemented on `KeyPair` by way of the normal
`serde` traits, there is no obvious reason for the `serde_keypair`
and the `KeyPairWrapper`.
Remove the `KeyPairWrapper` and test `KeyPair` serde impls directly.
We can help readers of the HTML docs by using a link to the constant.
While we are at it shorten the doc comment so it fits on one line (in
under 100 chars).
b9eefea092 Add documentation on side channel attacks to SecretKey (Tobin C. Harding)
7cf3c6c8a4 Implement constant time comparison for SecretKey (Tobin C. Harding)
19039d9281 Remove `Ord`/`Hash` traits from SecretKey (Tobin C. Harding)
4a0c7fca6a Do not use impl_array_newtype for SecretKey (Tobin C. Harding)
Pull request description:
Add constant time comparison implementation to `SecretKey`.
This PR does as suggested [here](https://github.com/rust-bitcoin/rust-secp256k1/issues/471#issuecomment-1179783309) at the end of the issue discussion thread.
Fix: #471
ACKs for top commit:
apoelstra:
ACK b9eefea092
Tree-SHA512: 217ed101b967cc048954547bcc0b3ab09e5ccf7c58e5dcb488370caf3f5567871152505a3bfb4558e59eea4849addaf1f11e1881b6744b0c90c633fa0157a5ae
We recently added a constant time `eq` implementation however library
users can inadvertently bypass this protection if they use `AsRef`. To
help prevent this add documentation to the `SecretKey` and also to the
`AsRef` implementation.
The current trait implementations of `Ord` and `PartialOrd` for
`SecretKey` leak data when doing the comparison i.e., they are not
constant time. Since there is no real usecase for ordering secret keys
remove the trait implementations all together.
Remove `Hash` at the same time because it does not make sense to
implement it if `Ord`/`PartialOrd` are not implemented.
In preparation for changing the logic of comparison trait (Ord, Eq)
implementations on the `SecretKey` copy all the code out of
`impl_array_newtype` and implement it directly in `key.rs`.
Refactor only, no logic changes (although I removed a few unneeded
references).
Currently we are not running the formatter in CI, in preparation for
doing so run `cargo +nightly fmt` to clear all current formatting
issues.
No manual changes.
9850550734 Move AsRef impl block next to Index (Tobin C. Harding)
4d42e8e906 Derive Copy and Clone (Tobin C. Harding)
b38ae97eaf Implement stable comparison functionality (Tobin C. Harding)
630fc1fcb6 Remove len and is_empty from impl_array_newtype macros (Tobin C. Harding)
9788b6df88 Remove leading colons from impl_array_newtype methods (Tobin C. Harding)
2bb08c21e5 Remove as_[mut_]ptr from impl_array_newtype macros (Tobin C. Harding)
3e28070187 Duplicate impl_array_newtype (Tobin C. Harding)
635890322a Add newline to end of file (Tobin C. Harding)
Pull request description:
Supersedes #515
The primary aim of this PR is to fix the fact that we currently implement various comparison traits (`Ord`, `PartialEq`) by comparing the inner byte arrays. These bytes are meant to be opaque and are not guaranteed across versions of `libsecp256k1`.
This PR is a bit involved because I had to detangle all the various types (across both `secp256k1` and `secp256k1-sys`) that use the `impl_array_newtype` macro.
- Patch 1: is trivial cleanup
- Patch 2: Step one of the PR is duplicating the macro into both crates so we can tease them apart.
- Patch 3-5: Are cleanup of the now duplicated `impl_array_newtype` macros
- Patch 6: Is the meat and potatoes
- Patch 7,8: Further minor clean ups to the macro
I had a lot of fun with this PR, I hope you enjoy it too.
Fix: #463
ACKs for top commit:
apoelstra:
ACK 9850550734
Tree-SHA512: 160381e53972ff882ceb1d2d47bac56a7301a2d13bfe75d3f6ff658ab2c6fbe516ad856587c4d23f98524205918ca1a5f9b737e35c23c7a01b476c92d8d1792f
There is no obvious reason why not to derive `Copy` and `Clone` for
types that use the `impl_newtype_macro`. Derives are less surprising so
deriving makes the code marginally easier to read.
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
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
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.
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".
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
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.
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
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.
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
Clippy default settings seemed to have changed introducing a few new
warnings.
warning: variable does not need to be mutable
warning: deref on an immutable reference
warning: returning the result of a `let` binding from a block
Fix them all in a single patch because CI has to pass for each patch.
a431edb86a Create configuration conditional bench (Tobin C. Harding)
2a1c9ab4b8 Remove rand-std feature from unstable (Tobin C. Harding)
ddc108c117 Increase heading size (Tobin C. Harding)
596adff8ba Remove unneeded whitespace (Tobin C. Harding)
Pull request description:
As we did in rust-bitcoin [0] create a configuration conditional `bench`
that we can use to guard bench mark code. This has the benefit of
making our features additive i.e., we can now test with `--all-features`
with a stable toolchain (currently this fails because of our use of the
`test` crate).
Please note, this patch maintains the current behaviour of turning on
the `recovery` and `rand-std` features when benching although I was
unable to ascertain why this is needed.
[0] - https://github.com/rust-bitcoin/rust-bitcoin/pull/1092
ACKs for top commit:
sanket1729:
ACK a431edb86a.
apoelstra:
ACK a431edb86a
Tree-SHA512: 913f5fbe0da08ec649081bf237c1d31cee58dacdac251d6030afabb99d455286c6d1dbdb6b2ac892b5d3c24584933254d1cfeec8e12f531cc420bd9d455a6531
This causes panics. We can't add catch the panic, we can't change its output, we
can't detect if it'll happen, etc. Rather than dealing with confused bug reports
let's just drop this.
If users want to rerandomize their contexts they can do so manually.
There is probably a better solution to this but it is still under debate, even
upstream in the C library, what this should look like. Meanwhile we have bug
reports now.
As we did in rust-bitcoin [0] create a configuration conditional `bench`
that we can use to guard bench mark code. This has the benefit of
making our features additive i.e., we can now test with `--all-features`
with a stable toolchain (currently this fails because of our use of the
`test` crate).
[0] - https://github.com/rust-bitcoin/rust-bitcoin/pull/1092
56f18430ff Add must_use for mut self key manipulation methods (Tobin C. Harding)
5b86e38aea Put compiler attributes below rustdocs (Tobin C. Harding)
Pull request description:
We recently added a bunch of key tweaking methods that take `mut self`
and return the tweaked/negated keys. These functions are pure and as
such the returned result is expected to be used. To help downstream
users use the API correctly add `must_use` attributes with a descriptive
error string for each of the methods that takes `mut self`.
Patch 1 is preparatory cleanup.
ACKs for top commit:
apoelstra:
ACK 56f18430ff
Tree-SHA512: 95ee63d5d0a34a9915551471d2f71de1963875eda04bf4217544076be0ed2836dcdee1875432dba5e02678556af86d7487e39daac6e928083807661430ddbcd6
Currently the following command fails
`RUSTFLAGS='--cfg=fuzzing' RUSTDOCFLAGS='--cfg=fuzzing' cargo test --all --all-features`
This is because `fuzzing` is not a feature, we should be using `fuzzing`
directly not `feature = "fuzzing"`.
I have no idea how this got past CI.
We recently added a bunch of key tweaking methods that take `mut self`
and return the tweaked/negated keys. These functions are pure and as
such the returned result is expected to be used. To help downstream
users use the API correctly add `must_use` attributes with a descriptive
error string for each of the methods that takes `mut self`.
e275166652 derive Hash for RecoverableSignature (NicolaLS)
Pull request description:
It would be nice to also derive Hash for `RecoverableSignature` so data structures containing it don't have to implement it themself if they need to derive Hash
ACKs for top commit:
apoelstra:
ACK e275166652
Tree-SHA512: 418337e16e82a5e736c54d123450fdb164f4776db68952cf8095b36c501436446542821d554fa781dffa0f9067fc2464833a6c461897e655ff4449018da12ca2
`Default` was pointless, so it was replaced with internal
`from_raw_parts` method which also checks the length.
This commit also documents changes to `SerializedSignature`.
Closes#454
This de-clutters the code and prepares for the next step of adding
`IntoIterator`. The type is still re-exported so the change is neither
breaking nor inconvenient.
This also adds more datialed explanation of `SerializedSignature` and
why it's needed.
65186e732a Add githooks (Tobin C. Harding)
6d76bd4a89 Add clippy to CI (Tobin C. Harding)
9f1ebb93cb Allow nonminimal_bool in unit test (Tobin C. Harding)
685444c342 Use "a".repeats() instead of manual implementation (Tobin C. Harding)
42de876e01 Allow let_and_return for feature guarded code (Tobin C. Harding)
d64132cd4b Allow missing_safety_doc (Tobin C. Harding)
2cb687fc69 Use to_le_bytes instead of mem::transmute (Tobin C. Harding)
c15b9d2699 Remove unneeded explicit reference (Tobin C. Harding)
35d59e7cc6 Remove explicit 'static lifetime (Tobin C. Harding)
1a582db160 Remove redundant import (Tobin C. Harding)
Pull request description:
The first 8 patches clear clippy warnings. Next we add a CI job to run clippy. Finally we add a `githooks` directory that includes running clippy, also adds a section to the README on how to use the githooks. This is identical to the text in the [open PR](https://github.com/rust-bitcoin/rust-bitcoin/pull/1044) on `rust-bitcoin` that adds githooks _without_ yet adding clippy.
**Note**: The new clippy CI job runs and is green :)
ACKs for top commit:
Kixunil:
ACK 65186e732a
apoelstra:
ACK 65186e732a
Tree-SHA512: f70a157896ce2a83af8cfc10f2fbacc8f68256ac96ef7dec4d190aa72324b568d2267418eb4fe99099aeda5486957c31070943d7c209973859b7b9290676ccd7
Clippy emits:
warning: returning the result of a `let` binding from a block
This is due to feature guarded code, add 'allow' attribute. Use
`cfg_attr` to restrict the allow to when it is needed. Add the already
present `unused_mut` inside the `cfg_attr` guard also.
We have a whole bunch of unsafe code that calls down to the FFI layer.
It would be nice to have clippy running on CI, these safety docs
warnings are prohibiting that. Until we can add the docs add a compiler
attribute to allow the lint.
Since we bumped the MSRV we have `to_le_bytes` available now, use it
instead of `mem::transmute`.
Remove code comment suggesting to do exactly this and clear clippy
warning.
While we are at it, use `cast` instead of `as`.
13af51926a Make key comparison non-fuzzable (Dr Maxim Orlovsky)
739660499b Implement PublicKey ordering using FFI (Dr Maxim Orlovsky)
0faf404f0e Benchmark for key ordering (Dr Maxim Orlovsky)
999d165c68 FFI for pubkey comparison ops (Dr Maxim Orlovsky)
Pull request description:
Re-base #309 for @dr-orlovsky on request by @Kixunil.
To do the rebase I just had to change instances of cfg_attr to use `v0_5_0` instead of `v0_4_1` e.g.,
```
#[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_5_0_xonly_pubkey_cmp")]
```
And drop the changes to `src/schnorrsig.rs`, all these changes are covered by the changes in `key.rs` I believe.
ACKs for top commit:
Kixunil:
ACK 13af51926a
apoelstra:
ACK 13af51926a
Tree-SHA512: 3054fcbc1707679f54466cdc91162c286394ad691e4f5c8ee18635a22b0854a4e60f1186ef3ca1532aacd8a637d0a153601ec203947e9e58dfcebf1bcb619955
12d4583638 Implement negate that consumes self (Tobin Harding)
5eb2d745b7 Rename tweak_add_assign -> add_tweak (Tobin Harding)
b9d08db8eb Replace _assign with _tweak (Tobin Harding)
Pull request description:
The various `_assign` methods (`add_assign`, `add_expr_assign`, `mul_assign`, `tweak_add_assign`) are cumbersome to use because a local variable that uses these methods changes meaning but keeps the same identifier. It would be more useful if we had methods that consumed `self` and returned the newly modified type.
We notice also that this API is for adding/multiplying tweaks not arbitraryly adding keys.
- Patch 1: Changes add/mul_assign -> add/mul_tweak for `PublicKey` and `SecretKey` (incl. re-working unit tests)
- Patch 2: Changes `tweak_add_assign` -> `add_tweak` for `KeyPair` and `XOnlyPublicKey`
- Patch 3: Changes `negate_assign` -> `negate`
All methods changed include:
- New method consumes self and returns the tweaked key
- Original method remains with a `deprecated` attribute, however I've left a TODO in there for adding the `since` field.
Close: #415
ACKs for top commit:
apoelstra:
ACK 12d4583638
Tree-SHA512: 026e8722892f3a0f18956281e4d2356d2789ef535a7ab71a375758201b180663d068397cde2dca5f60858ab7158069e53d7096326bfbd5a364269b0be680940c
3ca7f499e0 Add fixed-width-serde integration tests (Tobin Harding)
bf9f556225 Add rustdocs describing fixed width serde (Tobin Harding)
c28808c5a4 Improve rustdocs for KeyPair (Tobin Harding)
6842383161 Use fixed width serde impls for keys (Tobin Harding)
Pull request description:
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.](https://docs.rs/bincode/latest/bincode/index.html). 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?
ACKs for top commit:
apoelstra:
ACK 3ca7f499e0
Tree-SHA512: 77babce74fa9f0981bb3b869c4e77a68a4d1ec28d22d2c3be4305e27ef01d4828dac210e20b968cbbe5de8a0563cd985d7969bccf75cfe627a34a116fed1a5df
Feature guard the custom implementations of `Ord` and `PartialOrd` on
`cfg(not(fuzzing))`. When fuzzing, auto-derive implementations.
Co-authored-by: Tobin C. Harding <me@tobin.cc>
The method `negate_assign` (on pub/sec key) is cumbersome to use because
a local variable that uses these methods changes meaning but keeps the
same identifier. It would be more useful if we had methods that consumed
`self` and returned a new key.
Add method `negate` that consumes self and returns the negated key.
Deprecated the `negate_assign` methods.
Andrew Poelstra
Tobin C. Harding
Elichai Turkel
elsirion
NicolaLS
Martin Habovstiak
Tim Ruffing
Dr Maxim Orlovsky