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
f08276adfc Add convenience methods for keys (Tobin Harding)
b4c7fa0d4e Let the compiler work out int size (Tobin Harding)
c612130864 Borrow secret key (Tobin Harding)
Pull request description:
We have a bunch of `from_<key>` methods for converting between key types. To make the API more ergonomic to use we can add methods that do the same but called on a variable e.g., once applied the following are equivalent:
- `let pk = PublicKey::from_keypair(kp)`
- `let pk = kp.public_key()`
Do this for `SecretKey`, `PublicKey`, `KeyPair`, and `XOnlyKeyPair`.
Fixes: #428
### Note to reviewers
- `XOnlyPublicKey` -> `PublicKey` logic is made up by me, I could not work out how to get `libsecp256k1` to do this.
- Please review the tests carefully, they include assumptions based on my current understanding of the cryptography :)
ACKs for top commit:
sanket1729:
ACK f08276adfc. Thanks for going through all the iterations.
apoelstra:
ACK f08276adfc
Tree-SHA512: 1503a6e570a3958110c6f24cd6d075fe5694b3b32b91a7a9d332c63aa0806198ff10bdd95e7f9de0cf73cbf4e3655c6826bd04e5044d1b019f551471b187c8ea
676a9800df Remove unnecessary panic message (sanket1729)
aa50cc6ced Remove Schnorr word from keypairs (sanket1729)
Pull request description:
Keypairs are pair of EC points that don't have anything to do with the
signature algorithm
ACKs for top commit:
apoelstra:
ACK 676a9800df
tcharding:
ACK 676a9800df
Tree-SHA512: ed3e6f5e821d18641234b308b130271dcd2ec0dd6519a0e9d91564ab8e902b82180d7df377f2bcf08cd3ca1df7ce775422e4a3c386637eaff348e58b033de3ea
We have a bunch of `from_<key>` methods for converting between key types.
To improve the API and make it more ergonomic to use we can add methods
that do the same but can be called on the initial key instead of on the
resulting key's type. E.g. once applied the following are equivalent:
- `let pk = PublicKey::from_keypair(kp)`
- `let pk = kp.public_key()`
Do this for `SecretKey`, `PublicKey`, `KeyPair`, and `XOnlyKeyPair`.
We have two places in the code where we pass a mutable parity integer
to ffi code. At one callsite we tell the compiler explicitly what type
it is (`::secp256k1_sys::types::c_int`) and at the other call site we
let the compiler figure out the type.
Is one way better than the other? I don't know. But letting the compiler
figure it out seems to make the code easier to read.
`SecretKey` implements `Copy` and it is fine to take owneship of it; we
have multiple methods called `from_secret_key` and they all borrow the
secret key parameter. Favour consistency over perfection.
Borrow secret key parameter as is done in other `from_secret_key`
methods.
As we do for other keys implement serde de/serialization for the
`SharedSecret`. Includes implementation of `from_slice` method that is
the borrowed version of `from_bytes` as well as a `FromStr`
implementation that parses a hex string.
Recently we moved from using the identifier 'schnorrsig' to 'schnorr',
we omitted to update the tests.
While we are at it use more idiomatic Rust unit test names (i.e., do not
start test name with `test_` because it stutters when the name is read
in output of `cargo test`).
Recently we moved from using the identifier 'schnorrsig' to 'schnorr',
we omitted to update the schnorr signature size constant.
Deprecate `SCHNORRSIG_SIGNATURE_SIZE` and add
`SCHONORR_SIGNATURE_SIZE`.
Recently we moved from using the identifier 'schnorrsig' to 'schnorr',
we omitted to update the schnorr public key size constant.
Deprecate `SCHNORRSIG_PUBLIC_KEY_SIZE` and add
`SCHONORR_PUBLIC_KEY_SIZE`.
5acf6d23d3 `Parity` conversion and error handling cleanup (Martin Habovstiak)
Pull request description:
This removes the deprecated `From` conversion, replaces it with
`TryFrom`, and adds more convenience conversions. A new error type is
created for the invalid parity error with conversion to catch-all
`Error`.
This is intended for an API-breaking version.
ACKs for top commit:
apoelstra:
ACK 5acf6d23d3
Tree-SHA512: 49b73fc90455c172012b46f36eafa7d256b940f4b431b4eedb577ab07d9402eae40af931e00b3c409bbe502dbcac064a742e874a5e8bedd8d0cbe92a468ae4f6
Tobin C. Harding
Andrew Poelstra
Dr Maxim Orlovsky
Martin Habovstiak
Elichai Turkel
Tibo-lg
junderw
sanket1729
Dominik Spicher