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.
We now have a method `add_tweak` on the `SecretKey` and `PublicKey`. We
can add similar methods that consumes self and return the tweaked key
for the `KeyPair` and `XOnlyPublicKey` types.
The justification for doing so is that a local variable that calls
`tweak_add_assign` changes in meaning but the identifier remains the
same, this leads to cumbersome renaming of the local variable.
The tweaking done to the `KeyPair` is actually done via the xonly public
key not the public key. To reflect this call the method
`add_xonly_tweak`, this is similar to how it is named in secp
`secp256k1_keypair_xonly_tweak_add`.
The key methods `add_assign`, `add_expr_assign`, and `mul_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 a new key.
Observe also that these to methods are for adding/multiplying a key by a
tweak, rename the methods appropriately.
Add methods `add_tweak`, `add_expr_tweak`, and `mul_tweak` to the
`SecretKey` and `PublicKey` type. Deprecate `add_assign`,
`add_expr_assign`, and `mul_assign`.
The current display code for `Error` is a little unusual. We typically
just implement `Display` and if a `str` is needed use `format!`.
Improve the `Error` type by doing
- Remove the `as_str` function and implement `Display` directly.
- Remove the 'secp: ' prefix of all the error messages.
- Use a newly defined macro `write_err` that writes the error if `std`
feature is not enabled so that no-std builds do not loose error info.
Note: The `write_err` macro is currently being introduced in
`rust-bitcoin` also. Elect to just duplicate it here and not share it
between the crates.
This adds `Scalar` newtype to better represent values accepted by
tweaking functions. This type is always 32-bytes and guarantees being
within curve order.
We recently added fixed width serialization for some types however
serialization is only fixed width when data is serialized with the
`bincode` crate.
Add rustdocs describing fixed width serde to `SecretKey`, `PublicKey`,
and `XOnlyPublicKey` (`KeyPair` is already done).
Currently the rustdocs for `KeyPair` are stale in regards to serde, we
_do_ implement `Serialize` and `Deserialize` for `KeyPair`.
Improve the rustdocs for `KeyPair` by removing stale docs and adding
docs on fixed width binary serialization.
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 the `bincode` crate. This extra data
is unnecessary since we know in advance the length of these types.
It would be useful for users of the lib to be able to get a fixed width
binary serialization, this can be done but it depends on the crate used
to do the serialization. We elect to optimise for `bincode` and add docs
noting that other binary serialization crates may differ (rustdocs added
in separate patches).
Implement a tuple based visitor that encodes the keys as fixed width
data.
Do fixed width serde implementations for:
- `SecretKey`
- `PublicKey`
- `KeyPair`
- `XOnlyPublicKey`
5d2f1ceb64 Fix WASM build (Elichai Turkel)
39aaac6834 Use new trait TryFrom and do small refactoring (Elichai Turkel)
7d3a149ca5 Move more things from the std feature to the alloc feature (Elichai Turkel)
bc8c713631 Replace c_void with core::ffi::c_void (Elichai Turkel)
26a52bc8c8 Update secp256k1-sys to edition 2018 and fix imports (Elichai Turkel)
ebe46a4d4e Update rand to 0.8 and replace CounterRng with mock::StepRng (Elichai Turkel)
626835f540 Update secp256k1 to edition 2018 and fix imports (Elichai Turkel)
67c0922a46 Update MSRV in CI and Readme from 1.29 to 1.41 (Elichai Turkel)
Pull request description:
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
ACKs for top commit:
tcharding:
ACK 5d2f1ceb64
apoelstra:
ACK 5d2f1ceb64
Tree-SHA512: 5bf84e7ebb6286d59f8cada0bb712c46336f0dd6c35b67e6f4ba323b5484ad925b99b73e778ae4608f123938e7ee8705a0aec576cd9c065072c4ecf1248e3470
ef7f1972a7 Derive Hash for Signature (Tobin C. Harding)
Pull request description:
In preparation for deriving `Hash` in miniscript, derive `Hash` on the `ecdsa::Signature`.
ref: https://github.com/rust-bitcoin/rust-miniscript/issues/226
ACKs for top commit:
apoelstra:
ACK ef7f1972a7
elichai:
ACK ef7f1972a7
Tree-SHA512: 7313f59971444ae18611adbafe86a09478eddd7357f2b7f3ad3bb1761609b6358b156975086f6c318eb2777018b7b2f44386321108939acbcf2d0a522e7e208e
Tobin C. Harding
Andrew Poelstra
elsirion
NicolaLS
Martin Habovstiak
Tim Ruffing
Dr Maxim Orlovsky
Elichai Turkel
Tibo-lg