Currently when debug printing the `RecoverableSignature` we do so byte
by byte, this means that the output differs depending on the endianess
of the machine. If instead we serialize the signature in compact form
then the output is the same irrespective of the endianess.
With this applied the following two commands now pass:
```
cargo test test_debug_output --features=recovery
```
cross test --target powerpc-unknown-linux-gnu test_debug_output --features=recovery
```
Fixes: #375
b3503ba148 Add example to SharedSecret (Tobin Harding)
Pull request description:
Currently the rustdoc on `SharedSecret` is wildly incorrect (possibly a cut'n'pasta error).
Fix the rustdoc for `SharedSecret` and add an examples section to assist testing the public API.
Fixes: #249
ACKs for top commit:
apoelstra:
ACK b3503ba148
Tree-SHA512: 650092388099bb415c11ea335ca6b64c90094f1a51ceecc403911316ee62da0279488af6fa66e00ee5269c129f06d4641085f8ab9be91c98d24a7a4449d235c2
8339ca5706 Add documentation guiding users towards randomization (Tobin Harding)
cf1496b64e Add documentation about rand-std feature (Tobin Harding)
1693d51ce7 Randomize context on creation (Tobin Harding)
a0465ea279 Remove feature global-context-less-secure (Tobin Harding)
Pull request description:
Currently it is easy for users to mis-use our API because they may not know that `randomize()` should be called after context creation for maximum defence against side channel attacks.
This PR entails the first two parts of the plan outlined in #388. The commit messages are a bit light of information as to _why_ we are doing this so please see #388 for more context.
In light of @real-or-random's [comment](https://github.com/rust-bitcoin/rust-secp256k1/issues/388#issuecomment-1026613592) about verification contexts the randomization is done in `gen_new` i.e., for _all_ contexts not just signing ones.
Also, I think we should add some docs about exactly _what_ randomization buys the user and what it costs. I do not know exactly what this is, can someone please write a sentence or two that we can include in the docs to `gen_new`?
@TheBlueMatt please review patch 4.
Resolves: #225
**Note**: This is a total re-write of the original PR, most of the discussion below is stale. Of note, the additional API that takes a seed during construction is not implemented here.
ACKs for top commit:
apoelstra:
ACK 8339ca5706
Tree-SHA512: e74fe9a6eaf8ac40e4e06997602006eb8ca95216b5bc6dca3f5f96b5b4d3bf8610d851d8f1ef5c199ab7fbe85b34d162f2ee0073647f45105a486d20d8c0722a
Currently the rustdoc on `SharedSecret` is wildly incorrect (possibly a
cut'n'pasta error).
Fix the rustdoc for `SharedSecret` and add an examples section to assist
testing the public API.
Fixes: 249
Now that we opportunistically randomize the context on creation if
`rand-std` is enabled it would be nice to encourage users who do not
wish to use `rand-std` to randomize the context. We already have an API
to do this but it requires a separate call to do so. Instead of adding a
bunch of additional constructors elect to add documentation to the
current constructors guiding users towards randomization.
We recently implemented opportunistic randomization of the context
object if the the `rand-std` feature is enabled. Both for the global
context and also for signing context constructors.
Add documentation about `rand-std` feature in relation to the context
object.
Instead of providing a mechanism for users to opt out of randomization
we can just feature gate the call site i.e., opportunistically randomize
the global context on creation if `rand-std` feature is enabled.
2732891359 Change rand to rand-std in lib.rs documentation (Vincent Liao)
Pull request description:
I copy-pasted the key-generation example written on the documentation, but it didn't work. It only worked when I used the feature `rand-std` instead of `rand`.
To reproduce, boot up a new Rust project, and add this to main.rs:
```
use secp256k1::rand::rngs::OsRng;
use secp256k1::{Secp256k1, Message};
use secp256k1::hashes::sha256;
let secp = Secp256k1::new();
let mut rng = OsRng::new().expect("OsRng");
let (secret_key, public_key) = secp.generate_keypair(&mut rng);
let message = Message::from_hashed_data::<sha256::Hash>("Hello World!".as_bytes());
let sig = secp.sign_ecdsa(&message, &secret_key);
assert!(secp.verify_ecdsa(&message, &sig, &public_key).is_ok());
```
Using this dependencies causes error: `secp256k1 = {version="0.21.2", features=["rand", "bitcoin_hashes"]}`. After replacing `rand` with `rand-std`, it works.
ACKs for top commit:
apoelstra:
ACK 2732891
tcharding:
tACK 2732891359
Tree-SHA512: 6b5436bc71bab7535e432e119679bc6bcb11d2575b609e039cc25c122ae92b528f95a673e9c643a6cfa2ee3a663f7efdd61731b6084261c52a220448b6f72d12
c30026d08b Fix typo 'epected' -> 'expected' (Tobin Harding)
f3688ecf56 Use rand-std in key rustdoc examples (Tobin Harding)
ae3e06f95b Fix lint warnings in test code (Tobin Harding)
c01cd8f1f3 Enable running tests without default features (Tobin Harding)
a79840eca2 Be explicit about example feature requirements (Tobin Harding)
433c350424 Add multiple implementations of Debug for secrets (Tobin Harding)
632ecc4530 Use fully qualified path for mem (Tobin Harding)
Pull request description:
As indicated by the comment in `contrib/test.sh` we should be able to test with `--no-default-features`.
- Patch 1 uses fully qualified path to remove a build warning.
- Patch 2 adds additional `Debug` implementations for secrets, uses `bitcoin_hashes` if available, please review carefully.
- Patch 3 adds `std` as an explicit requirement for the three examples
- Patch 4 enables `cargo test --no-default-features, fixes all the feature gating in unit tests.
- Patch 5 fixes lint warnings generated while running the feature matrix in `contrib/test.sh`.
**Please Note**: Currently the `alloc` feature cannot be built with Rust 1.29, this made it into master because we don't build ever with the `alloc` feature enabled in CI. This PR _should_ add `alloc` to the features matrix but it does not. Adds a TODO comment to `contrib/test.sh` to add it once we bump MSRV.
ACKs for top commit:
apoelstra:
ACK c30026d08b
Tree-SHA512: 3bbdda332ab1e04eaa3479d9e9c7463a54347f56019ce5366bb36eb8d5ccaced32539e2c58454a7714d76b7bab9f1ab56accb04de67c826165dd104ac0b3b893
Seems there is a bug in cargo, the tests in `key.rs` run successfully
but AFAICT they should fail. Here is an example, running `cargo test
--features=rand` should make this test fail but it doesn't?
```
/// Secret 256-bit key used as `x` in an ECDSA signature.
///
/// # Examples
///
/// Basic usage:
///
/// ```
/// # #[cfg(all(feature = "rand", any(feature = "alloc", feature = "std")))] {
/// use secp256k1::{rand, Secp256k1, SecretKey};
///
/// let secp = Secp256k1::new();
/// let secret_key = SecretKey::new(&mut rand::thread_rng());
/// # }
/// ```
Anywho, use the correct feature gate: `rand-std`.
Various combinations of features trigger lint warnings for unused code,
all warnings are caused by incorrect feature gating.
Correct feature gating to remove Clippy warnings during testing.
Currently various features fail to build when enabled without default
features. This is because many tests need feature gating.
Feature gating the import statements quickly turns into spaghetti when
trying to cover all combinations of two features correctly, instead just
allow unused imports on `tests` modules where needed.
Add correct feature requirements to the examples so they also can be run
without default features.
Improve the CI script by doing:
- Add `std` to the feature matrix.
- Add `--no-default-features` to test runs in the CI script.
The examples depend on having the "std" feature [1]. In preparation for
being able to run tests with `--no-default-features` add the "std"
feature as a requirement for all three examples. While we are at it use
the correct rand feature requirement: `rand-std`.
[1] Technically we only need "alloc" but "alloc" is not working with
Rust 1.29 currently so just use "std".
The `Debug` implementation for secrets is feature gated on `std` because
it uses a hasher from `std`. If `bitcoin_hashes` is enabled we can use
it for hashing. If neither `std` nor `bitcoin_hashes` is enabled fall
back to outputting:
<secret requires std or bitcoin_hashes feature to display>
Remove the docs conditional since we now implement `Debug` always.
When building with --no-default-features the compiler emits:
warning: unused import: `mem`
The call site is feature gated so we either need to feature gate the
import or use a fully qualified path. Since 'core' is quite short elect
to use the fully qualified path.
aa828f01a5 Improve documentation in the key module (Tobin Harding)
9e46d6f122 Add examples to types and methods in key module (Tobin Harding)
a7f3d9bcfd Improve key module docs (Tobin Harding)
6d23614467 Improve lib.rs rustdocs (Tobin Harding)
4c4268f1ad Improve docs on method generate_keypair (Tobin Harding)
Pull request description:
This PR is an initial attempt to more thoroughly test our public API.
Add examples to various types/methods/functions in the key module.
I'm not entirely sure when is enough, do we want an example on every single public method, function, and type or is this overkill. In this PR I tried to find a balance by doing ever method/function that took an argument that is a custom type from this lib. I think this should be extended to include return values too though ...
Thanks to @thomaseizinger for the idea!
First 2 patches are docs improvements to `lib.rs`.
ACKs for top commit:
apoelstra:
ACK aa828f01a5
Tree-SHA512: 9383ad263469f98ce7e988d47edc1482a09a0ce82f43d3991bd80aabdf621430f4a3c86be4debf33232dcb1d60d3e81f2c6d930ea7de7aa0e34b037accd7bc98
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
We recently patched much of the docs in the `key` module, lets attempt
to attain perfection.
Improve docs by doing:
- Use full stops
- Use 100 character column width
- Use plural third person tense
- Use plural for section headings
- Fix any grammar mistakes
- Use code ticks and links as appropriate
Done in an effort to better test our public API.
Add tests in the `Examples` section as is idiomatic in the Rust
ecosystem.
Make other minor improvements to any rusdocs we touch:
- Use full stops
- Use 100 character column width
- Use plural third person tense
- Use plural for section headings
Improve method docs by doing:
- Remove 'batch' comment
- Remove mention of required features, docs already show this
- Use links to types as well as ticks
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.
Tobin Harding
Andrew Poelstra
Vincent Liao
elsirion
Martin Habovstiak