We are currently using the DO_BENCH variable as a proxy for whether or
not we are using a nightly toolchain, while this is technically correct
we use it from within an if guarded statement that is guarded by
DO_FEATURE_MATRIX and we never run the CI script with _both_ of these
variables set to true. This means that the all features test is never
being run.
Add a NIGHTLY variable and set it based on the output of `cargo
--version`.
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
0c15c01eb1 Use fuzzing not feature = "fuzzing" (Tobin C. Harding)
Pull request description:
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~, found while trying to [upgrade secp in bitcoin](https://github.com/rust-bitcoin/rust-bitcoin/pull/1066).
This got past CI because of the feature gate combination `#[cfg(all(test, feature = "unstable"))]`, we never run tests on CI with both DO_FEATURE_MATRIX and DO_BENCH.
```
if [ "$DO_FEATURE_MATRIX" = true ]; then
...
if [ "$DO_BENCH" = true ]; then # proxy for us having a nightly compiler
cargo test --all --all-features
RUSTFLAGS='--cfg=fuzzing' RUSTDOCFLAGS='--cfg=fuzzing' cargo test --all --all-features
fi
fi
```
ACKs for top commit:
apoelstra:
ACK 0c15c01eb1
Tree-SHA512: 08ada4eb20c3b7b128a225ed66cc621af097367f8ca19128b868d1b5de897f46d19f3a96a06ebd5dfaa288bc4477046f5d1214f0cdc33237b0ace079c539fc9e
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`.
141f2d1dbc Bump version to 0.23.2 (Artem Vorotnikov)
Pull request description:
ACKs for top commit:
apoelstra:
ACK 141f2d1dbc
Tree-SHA512: 931b4ad992f7f00640edae08fcb6ba7bacccf088f54adab7820207dd6d980788922b0596c45ef30e4b949a156f9e67b48cb20f79b033fda31dbde3c857d3bfd6
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
4e44abc2e0 Bump version to v0.23.1 (Tobin C. Harding)
c36b4375c0 Enable rand/std_rng feature (Tobin C. Harding)
Pull request description:
Hot fix because currently attempting to use secp256 v0.23.0 as a dependency throws a secp build error.
We recently upgraded the rand dependency and we use it behind code feature gated on "rand-std". In that code we use `thread_rng` but this is only available if the "std_rng" feature is turned on, however in non-dev builds we do not enable this feature, we have a "rand-std" feature that enables "rand/std", it should also enable "std_rng".
Enable "rand/std_rng" in the "rand-std" feature.
I threw a version bump patch on this too in case we want to merge and release this fix (assuming I'm not mistaken about the 0.23.0 release), its Friday afternoon right now for me so if you want to forge ahead feel free to do what ever needs doing, don't wait for me and this PR :)
ACKs for top commit:
apoelstra:
ACK 4e44abc2e0
Tree-SHA512: a76b4a94cea219f6cac3fe33efd6913b713f781917c9cdf6c5265e4021d57a91cae53ca4bb396deea654b976495843fdbad660959356669299b1c7c0b2371f80
We just applied a hot fix to the 0.23.0 released code to fix the
features enabled in `rand` when our "rand-std" feature is enabled. This
requires a bump of the patch version for release.
Bump the version and add a changelog entry.
We recently upgraded the rand dependency and we use it behind code
feature gated on "rand-std". In that code we use `thread_rng` but this
is only available if the "std_rng" feature is turned on, however in
non-dev builds we do not enable this feature, we have a "rand-std"
feature that enables "rand/std", it should also enable "std_rng".
Enable "rand/std_rng" in the "rand-std" feature.
79a4ee333b secp256k1-sys: bump version to 0.6.0 (Andrew Poelstra)
Pull request description:
Needed for release of secp256k1 0.23.0
ACKs for top commit:
tcharding:
ACK 79a4ee333b
sanket1729:
utACK 79a4ee333b
Tree-SHA512: 25c35145a1b4bc4bc997e136b727345b5f43921efdc983fb5866a717ee2b2bab501c0801fa3e613672027f5b35117890a0384a396770cb59467405ae374079ee
b18f5d0454 Removed `Default` from `SerializedSignature` (Martin Habovstiak)
Pull request description:
`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
ACKs for top commit:
tcharding:
utACK b18f5d0454
apoelstra:
ACK b18f5d0454
Tree-SHA512: 5ee32160721d4d22cfe7c5dcc433bf013fc78a350e86b3d8d42c207fec7f2bf11c47fce77269ae816567be77602fdc86231d86e2c62aa2d327540056ab445842
`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
c1d735802c Bump crate version to 0.23.0 (Tobin C. Harding)
Pull request description:
~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.
ACKs for top commit:
apoelstra:
ACK c1d735802c
Tree-SHA512: b320f061c78c9646a0c51a54b7fcdd676e7487b10a5a5323f20cf5ed11d3ea97a7a330764c934f3e4756679ac3daebbff2d725c9a15ec15216b60c7dfea9706b
e612458dc7 Remove mentions of 32-byte slice from tweak APIs (Martin Habovštiak)
Pull request description:
These methods accept `&Scalar`, not slice and `&Scalar` already guarantees 32-bytes, so this failure case is impossible.
ACKs for top commit:
sanket1729:
ACK e612458dc7.
apoelstra:
ACK e612458dc7
Tree-SHA512: f1c083756cb99b16b16764c4d603196a99b7bae864ca7f62908866667cf0218c459447b95298edc71de92c1abe1268a1c085495e1626bb0b9168e1af6aaf2af6
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
Add `githooks` directory and:
- Copy the default pre-commit hook into new githooks directory
- Add a call to clippy to the pre-commit hook
- Add a section to the README instructing devs how to use the new
githooks
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`.
f419fe884b Fix getting parity from keypair in fuzzing (Tim Ruffing)
Pull request description:
This also enables a test that was failung due to the parity bug.
ACKs for top commit:
tcharding:
ACK f419fe884b
elichai:
ACK f419fe884b
apoelstra:
ACK f419fe884b
Tree-SHA512: bec00d517495cf010c37079e3d71f1d9547a319aac50cecbca74e6d62eca7188aa79d61ee3d50ef81512c30a31b61b947e2f19e45a95b1b17a1f5cf7cf3f1019
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
946cd83106 Improve Error display (Tobin C. Harding)
Pull request description:
Introduce `write_err` macro and improve the `Display` implementation on `Error`.
This PR is re-written after review below. The `std::error::Error` implementation is removed in favour of #409. Now only includes the `Display` stuff.
ACKs for top commit:
apoelstra:
ACK 946cd83106 nice!
Tree-SHA512: a62e9593c2ed1ba6136136e6b575219d68c25736069f55448f8411048efae27f2467bcb65260a78ff065de9c8c2994873804c687fb37a55b705cddfe20544f37
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>
Tobin C. Harding
Andrew Poelstra
Artem Vorotnikov
NicolaLS
Martin Habovstiak
Tim Ruffing
Dr Maxim Orlovsky