6fad20ef0c Fix the mess around Parity (Tobin Harding)
Pull request description:
Recently we made a wee mess with the `Parity` opaque type. Let's fix it
up by doing:
- Use an enum with variants `Even` and `Odd`.
- Add explicit conversion methods to/from u8 and i32
- Implement `BitXor`
This PR attempts to follow the plan laid out in the issue but:
- I don't get why`to_u8` is requested, I added it anyways.
- `to_i32` is not mentioned but we need it if we are going to pass the parity back to FFI code after receiving it _without_ having to care about the value. And that is the whole point of this `Parity` type in the first place.
- I don't get why `from_xxx` is fallible, when is an integer not even or odd?
Please note: This patch is an API breaking change that does _not_ follow the deprecation guidelines. Rust does not allow deprecating `From` impl blocks AFAICT.
Fixes: #370
ACKs for top commit:
apoelstra:
ACK 6fad20ef0c
Tree-SHA512: 810d5028f02fa608eab48721d32dca58be472080fcbac7a0ee79b5211b229112a756c48233e8597fb6c137690b2565431f410e9e97d6940ed389a4501bb015a0
Recently we made a wee mess with the `Parity` opaque type. Let's fix it
up by doing:
- Use an enum with variants `Even` and `Odd`.
- Add explicit conversion methods to/from u8 and i32
- Implement `BitXor`
Note: This patch is an API breaking change that does _not_ follow the
deprecation guidelines. Rust does not allow deprecating `From` impl
blocks AFAICT.
20fe3a14dc Add a disabled rustfmt.toml (Tobin Harding)
Pull request description:
We do not currently use `rustfmt`, this is a nuisance for devs who routinely work on code bases that do use rustfmt who often have their editors set to format on save. We can make the life of such devs much better by explicitly disabling formatting using `rustfmt.toml`.
ref: https://rust-lang.github.io/rustfmt/?version=v1.4.38&search=#disable_all_formatting
ACKs for top commit:
apoelstra:
ACK 20fe3a14dc
Tree-SHA512: 63a7f9c64886cf85e15748c28137a9e642de74890c0b5194fafe3b6c0f9243662b9b7c19c04fbf56e9dd4b14449b07cefd163f69ccfa7e4ffe7dbc3df60b34ff
We do not currently use `rustfmt`, this is a nuisance for devs who
routinely work on code bases that do use rustfmt who often have their
editors set to format on save. We can make the life of such devs much
better by explicitly disabling formatting using `rustfmt.toml`.
ref: https://rust-lang.github.io/rustfmt/?version=v1.4.38&search=#disable_all_formatting
47411ce73d Fix typo in documentation (Tobin Harding)
Pull request description:
Docs reference a function name but there is a typo.
'grund' -> 'grind'.
ACKs for top commit:
apoelstra:
ACK 47411ce73d
Tree-SHA512: e6724f1c7972625d59be0ae9de358295c9280e2e126e7322d706cbdca342c1189552b2fdeef9370ff4f85ea8ef185ef4447f6693979d5e8548fcfa2df41a491e
1877e4db33 Add serde impl for KeyPair (elsirion)
Pull request description:
The impl is added as a module instead of being a direct implementation since it uses the global context and users should be aware that.
ACKs for top commit:
apoelstra:
ACK 1877e4db33
elichai:
ACK 1877e4db33
Tree-SHA512: decb593a3b047631d08763a13ae10979d07c73bc2547d8f7ea541287f162461e0608992f43a81d819aaf201fc9feed7edc2ef918bdb2d82a39205cb2c77852f3
c7a8bbb772 Use the new recover_ecdsa in bench function (Tobin Harding)
Pull request description:
We recently deprecated `recover` in favour of `recover_ecdsa` but missed
one call site in benches.
ACKs for top commit:
apoelstra:
ACK c7a8bbb772
Tree-SHA512: c57625ff50aea6c3985f7c7d52bf47ba6a48e93d3c60408edb6ab9ddc6e014171f8691dab954472b02ac745fcefa95c9206000560ef1ba64308b521b36563bfa
97524b2da7 Deprecate generate_schnorrsig_keypair (Tobin Harding)
389abddcc7 Add method KeyPair::public_key (Tobin Harding)
Pull request description:
Recently we deprecated a bunch of functions/methods that used the term `schnorrsig`. Seems we left `generate_schnorrsig_keypair` in there, along with some stale docs on it.
- Patch 1: Adds method `KeyPair::public_key` that calls through to `XOnlyPublicKey::from_keypair`.
- Patch 2: Deprecates `generate_schnorrsig_keypair` and uses the newly defined `pk.public_key()` everywhere.
### Note to reviewers
Please note, this PR has been totally re-written using the suggestions below by @apoelstra.
ACKs for top commit:
apoelstra:
ACK 97524b2da7
Tree-SHA512: a10255d04b86c0031d5cfe4b6357224bc7bcd31c7e278d28af414a34ba4f158dd05d712c4878dfdc327ff8cb42b4421cc0a4b2605c6781691a3158b237fda2d3
Recently we deprecated a bunch of methods/functions. We are still
calling them in test code. Found by Clippy.
Use the shiny new methods/functions instead of the deprecated ones.
We have deprecated all other functions that use the identifier
'schnorrsig' but we missed `generate_schnorrsig_keypair`.
This function is purely a helper function and serves no real purpose
other than to reduce two lines of code to a single line. Downstream
users can write this function themselves if they need it.
Also, we recently added a new public method to `KeyPair` to get the
public key in a slightly more ergonomic fashion. Use `kp.public_key()`
when replacing usage of now deprecated `generate_schnorrsig_keypair`
function.
Currently to get the `XOnlyPublicKey` from a `KeyPair` users must do
`XOnlyPublicKey::from_keypair(&kp)`. While this does the job we can make
the lib more ergonomic by providing a method directly on `KeyPair` that
calls through to `XOnlyPublicKey::from_keypair`.
Add method `KeyPair::public_key(&self)`.
f6a19290fc Use hyperlinks (Tobin Harding)
Pull request description:
Clippy emits two warnings of type:
warning: this URL is not a hyperlink
As suggested, add pointy brackets to the links.
Docs built and links verified to be:
a) Not links as Clippy said before this patch is applied
b) Both are valid links with this patch applied
ACKs for top commit:
apoelstra:
ACK f6a19290fc
Tree-SHA512: 702807fee7922ff7755a43fc84333b8fb477c9700199eee2c2ee7c8c8238fcd3d2915ef256efd933f8934600c85bd64914dfe0c092d0bb356ce12a53b4469637
314e8755df Clarify `global-context` feature (Martin Habovstiak)
d52ab85dd5 Added missing features to docs.rs config (Martin Habovstiak)
Pull request description:
Sadly, I missed two details in #353: features missing in docs.rs configuration and `global-context` being a bit confusing.
This PR fixes those, see commit messages for details.
ACKs for top commit:
apoelstra:
ACK 314e8755df
Tree-SHA512: 01bed8ae2f30adcbdd436b514f08a084492d7f4e1a739ca62e6d8b8547e379c01faeda3522733c27ab615acbb4c6cff60e13906cc88a0d2b90e439e7da517466
656f19407b Remove capital letter in middle of docs sentence (Tobin Harding)
Pull request description:
(Candidate for most trivial patch of all time.)
Seems to be a typo, change the 'L' to an 'l'.
ACKs for top commit:
real-or-random:
ACK 656f19407b
Tree-SHA512: 06a4712868c3195a8465b9cf7bd39e55a30e37574086ca27cb032e0109a8fe053411426a15bcb354642bf78e6420b6fa2789ca487c6cc499f741a11220d5dc22
69f44d9301 Manually implement Debug for SerializedSignature (Tobin Harding)
26921a31b8 Add lints to catch missing traits (Tobin Harding)
35556e22f2 Remove useless call to format (Tobin Harding)
0ad414a982 Remove unneeded return statements (Tobin Harding)
Pull request description:
We can use the linters to help us catch type definitions that are missing 'standard' derives. 'standard' is project defined to be
- Copy
- Clone
- Debug
- PartialEq and Eq
- PartialOrd and Ord
- Hash
(I've assumed this to be true based on the code and an open [PR](https://github.com/rust-bitcoin/rust-bitcoin/pull/587) in rust-bitcoin.)
While neither Rustc nor Clippy can find all of these, Rustc can warn for missing `Copy` and `Debug` implementations and these warnings can assist us find types that may need additional derives.
First two patches are trivial Clippy fixes in preparation for using the linter to improve type definitions crate wide.
Patch 3 adds
```
#![warn(missing_copy_implementations)]
#![warn(missing_debug_implementations)]
```
and fixes newly emitted warnings.
ACKs for top commit:
thomaseizinger:
ACK 69f44d9301
apoelstra:
ACK 69f44d9301
Tree-SHA512: 18f2c52d207f962ef7d6749a57a35e48eb18a18fac82d4df4ff3dce549b69661cb27f66c4cae516ae5477f5b919d9197f70a5c924955605c73f8545f430c3b42
Previously only `global-context-less-secure` was shown in the doc even
though `global-context` may also work. This was strictly correct because
`global-context` implies `global-context-less-secure` which is also
documented but people could miss it or forget about it and then worry
about security or worse, enable less secure feature.
Calling out both fetures seems useful, even important and thankfully
doesn't seem to cause too much noise in the docs.
Currently we have an implementation of `Debug` (also used by `Display`)
for `Signature` that first converts the sig to a `SerializedSignature`
then prints it as hex.
We would like to have an implementation of `Debug` for
`SerializedSignature`, this cannot be derived because of the `data: [u8;
field]`. We can manually implement `Debug` for `SerializedSignature`
exactly as it is currently done for `Signature` and call this new
implementation from `Signature::fmt()`.
This code path is already tested in `lib.rs` in the test function
`signature_display`.
Rustc can warn us when we forget to add `Copy` and `Deubg` trait
implementations to types.
Add lint directives to enable warnings for missing `Copy` and `Debug`
implementations. Use the newly emitted warnings to find types that do
not implement our 'standard' traits. These 'standard' traits are defined
as the set of attributes that it has been found beneficial to
opportunistically add to all types, these are
- Copy
- Clone
- Debug
- PartialEq and Eq
- PartialOrd and Ord
- Hash
1671dfc2ed Release 0.21.2 (sanket1729)
837be22e09 Basic derives for Parity (sanket1729)
7059192de9 Wildcard export from key module (sanket1729)
Pull request description:
Sorry for getting another point release. This time I have tested against this branch for rust-bitcoin https://github.com/rust-bitcoin/rust-bitcoin/pull/755. Hopefully, this is the last release.
Next release, we should have a Release Candidate for a couple of days before publishing a release.
ACKs for top commit:
apoelstra:
ACK 1671dfc2ed
Tree-SHA512: 263ad027da3da764bd76f719200382c47ba21a976caefc23ebef45d1c4be35ddfc80ce619b57326310aaab22bbf75ca7f1db80b45e95ec076584805efb791f3f
e595b39510 Re-export Parity struct (sanket1729)
Pull request description:
pub struct Parity is under a private module key and not re-exported in lib.rs . It is therefore not
possible to use it downstream.
ACKs for top commit:
elichai:
ACK e595b39510
apoelstra:
ACK e595b39510
Tree-SHA512: 2573689f9a08505c8dfe8f79cd921d5a2742a2a2f4f92cf4066fe6557c765c756531d13560fa4fe6461f094b0c11a52aca30b44542eb77eda7dd1ebd24d3b155
18f74d5242 Clarify what does "less security" mean (Martin Habovstiak)
94c55b4d09 Fixed typos/grammar mistakes (Martin Habovštiak)
1bf05523f0 Documented features (Martin Habovstiak)
Pull request description:
This documents the Cargo features making sure docs.rs shows warning for
feature-gated items. They are also explicitly spelled out in the crate
documentation.
The PR is similar in spirit to https://github.com/rust-bitcoin/rust-bitcoin/pull/633
ACKs for top commit:
apoelstra:
ACK 18f74d5242
Tree-SHA512: 8aac3fc5fd8ee887d6b13606d66b3d11ce44662afb92228c4f8da6169e3f70ac6a005b328f427a91d307f8d36d091dcf24bfe4d17dfc034d02b578258719a90a
c50411f798 release secp256k1-sys 0.4.2; make new `ZERO` type publically accessible (Andrew Poelstra)
Pull request description:
Exposes the new const object provided by #345
ACKs for top commit:
elichai:
ACK c50411f798
Tree-SHA512: 42fce191b68a88811c339ff267dafbb616e765108f5b2e70514b5153f64ef5152f5704982ddc0b20ece5ad15da23927e18f9c78af2763ef971c0e3b9bbf490a5
This documents the Cargo features making sure docs.rs shows warning for
feature-gated items. They are also explicitly spelled out in the crate
documentation.
fafc141782 Removed useless Makefile (Martin Habovstiak)
Pull request description:
This Makefile did nothing interesting and could confuse people.
ACKs for top commit:
apoelstra:
ACK fafc141782
Tree-SHA512: 00337677787f98c4c4f1014f5cb4205b5e4057eaa2a1d512f44280d9e7952219b8ef3804e64dca35cc19856bbe780069ce3ab072a082023c72143542aaaaacaa
ede114fb1a Improve docs on tweak_add_check method (Tobin Harding)
fbc64c7725 Add opaque parity type (Tobin Harding)
1b768b2749 Make tweak_add_assign return statements uniform (Tobin Harding)
edafb88f8c Move key unit tests to key module (Tobin Harding)
e3d21a3d87 Clean up test imports with key module (Tobin Harding)
Pull request description:
Two functions in the FFI secp code return and accept a parity integer.
Currently we are manually converting this to a bool. Doing so forces readers of the code to think what the bool means even though understanding this value is not needed since in is just passed back down to the FFI code.
We initially tried to solve this issue by adding an enum, discussion below refers to that version. Instead of an enum we can solve this issue by adding an opaque type that holds the parity value returned by the FFI function call and then just pass it back down to FFI code without devs needing to know what the value should be. This fully abstracts the value away and removes the boolean conversion code which must otherwise be read by each dev.
- Patch 1 and 2 improve unit tests that test the code path modified by this PR
- Patch 3 trivially changes code to be uniform between two similar methods (`tweak_add_assign`)
- Patch 4 is the meat and potatoes (main part of PR :)
- Patch 5 is docs improvements to code in the area of this PR
ACKs for top commit:
apoelstra:
ACK ede114fb1a
Tree-SHA512: 37843e066d9006c5daa30dece9f7eb7a802864b85606e43ed2651c6d55938c4f884cc4abab81eccb69685f6eda918a9b9ba57bf1a4efec41e89239b99ae2b726
It is not immediately apparent what 'err == 1' means, one must determine
that the FFI function call returns 1 for success. We can help readers of
the code by adding a 'Return' section to the method documentation.
Add trailing full stop to method docs initial line also.
Two functions in the FFI secp code return and accept a parity int.
Currently we are manually converting this to a bool. Doing so forces
readers of the code to think what the bool means even though
understanding this bool is not needed since in is just passed back down
to the FFI code. We can abstract this away by using an opaque type to
hold the original int and not converting it to a boolean value.
Add 'Return' and 'Error' sections to `tweak_add_assign` while fixing the
docs to describe the new opaque parity type.
Andrew Poelstra
Tobin Harding
elsirion
Martin Habovstiak
sanket1729