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.
97dc0ea9ac Run correct clang --version (Tobin Harding)
a3582ff77d test.sh: Use set -e to exit on failure (Tobin Harding)
7bec31c3a6 test.sh: explicitly return 0 (Tobin Harding)
Pull request description:
Change the test script to exit with non-zero status code if any command fails.
The `test.sh` script is silently failing, that means changes causing failures are slipping through our CI pipeline and being merged.
Resolves: #419
## Note
Just the last 3 patches, the first 6 are from #420. re-base just shows it works on top of 420, it is going to have to be rebased again when 420 merges.
ACKs for top commit:
apoelstra:
ACK 97dc0ea9ac
Tree-SHA512: b86a6876d8c45a2b90b7b3c8adbc08ad6f49b430b1cfaec31cd2de8441cb96af39c63da02b98d6ed71dfab045d466d71d3757297886b5e44ebb6cbaeb4ed32dd
58db1b6753 Run WASM for multiple toolchains (Tobin Harding)
946ac3b51e Do docs build in Nightly job (Tobin Harding)
f7bc7d3728 Install clang to run adress sanitizer (Tobin Harding)
96685c571d Remove unnecessary matrix (Tobin Harding)
a8a679ed7d Re-name nightly CI job to Nightly (Tobin Harding)
9c9d622b0e Remove trailing whitespace (Tobin Harding)
Pull request description:
Improve the CI pipeline. Done while investigating #419. This PR now only includes the GitHub actions changes, I'm moving the `test.sh` changes to a https://github.com/rust-bitcoin/rust-secp256k1/pull/422 because they cannot be merged yet.
(Please note, this PR has been heavily re-worked so discussion below may be confusing to reviewers new to the PR.)
ACKs for top commit:
apoelstra:
ACK 58db1b6753
thomaseizinger:
ACK 58db1b6753
Tree-SHA512: 5520cf7a7ea0ba701aeaf6b97150416192c0629df8b65545a20d8937a4d76bd323a0d7a875deccb7ce9adc4f3a423e6cd27b300682f206f79407f5ab4eaa5ddb
For the test that uses `clang-9` do the sanity call using `clang-9`
instead of `clang`.
For the test that uses `clang` add a sanity call to `clang --version`.
Currently the `test.sh` script is silently failing because we do not
exit if a command fails. We can achieve this by using the Bash builtin
`set -e`.
For some reason I cannot explain a chain of commands that fails does not
fail the script. Instead of working out _why_ just remove the chain and
run each command on its own. This is functionally the same and, I hazard
a guess, is what the original author hoped to achieve with the chaining.
bfd88dbd6c Move WASM const definitions to a source file (Tobin Harding)
Pull request description:
Total re-write ... again :)
Currently we are defining the WASM integer size and alignments in the `stdio.h` header file, this is wrong because this file is included in the build by way of `build.rs` as well as by upstream `libsecp256k1`.
Move WASM integer definitions to a `C` source file and build the file into the binary if target is WASM.
Fixes the first part of #419 (#422 does the second part).
### Note to reviewers
I'm not exactly sure why the directory `was-sysroot` is named as it is or if the name is significant to `cargo` , please review carefully the directory tree changes.
```
cd secp256k1-sys
tree wasm
wasm
├── wasm.c
└── wasm-sysroot
├── stdio.h
├── stdlib.h
└── string.h
```
ACKs for top commit:
thomaseizinger:
ACK bfd88dbd6c
apoelstra:
ACK bfd88dbd6c
Tree-SHA512: ba822b764fb5f74dfd22cc797f7e3f70440dbaabfe34e0475c796e0e5d88f2086bedb00a1ec765cce91bde6bb45130b9abe5d9289317d6c20f692c6ed711969e
d2e1f8cc95 Move panic test to top of script (Tobin Harding)
Pull request description:
In `test.sh` we have a test that checks for a panic by greping the output of `cargo test --exact 'tests::test_panic_raw_ctx_should_terminate_abnormally'`. If we put this test at the top of the script right after we run `cargo test` we are guaranteed to not trigger a re-build.
### Note to reviewers
I just noticed this patch somehow snuck into #420, all other changes in that PR are to `.github/workflows/rust.yml` so this change does not fit in there. Hence raising it as a separate PR.
ACKs for top commit:
apoelstra:
ACK d2e1f8cc95
Tree-SHA512: 90ad7a8762a6fd977345f347f0aa8b0979a7576585000b6d80624c0672b7de457dec471dc63b2e7fa4c3f52143d0f6fd1f4031a70f85c9fab4b7c22a787c438b
Currently we are defining the WASM integer size and alignments in the
`stdio.h` header file, this is wrong because this file is included in
the build by way of `build.rs` as well as by upstream `libsecp256k1`.
Move WASM integer definitions to a `C` source file and build the file
into the binary if target is WASM.
WASM is supported by Rust 1.30. We can therefore run the WASM tests on
any all the toolchains except MSRV (1.29.0). This has benefit of
catching nightly/beta issues before they get to stable.
Done as a separate CI job since it is conceptually different to the
`Tests` job.
Run WASM for nightly, beta, and stable toolchains.
We have a separate CI job for things that require a nightly toolchain.
Building the docs requires a nightly toolchain (because of `--cfg
docsrc` flag). It makes more sense to run the docs build in the
`Nightly` job instead of hidden in the `Tests` job.
Do the docs build in the `Nightly` job instead of in the `Tests` job.
The address sanitizer job is silently failing at the moment because we
do not install clang.
Install clang so the address sanitizer job can run. Do not fix the
silent failure, that will be done later on.
In line with the `Tests` job and for the fact that this job does stuff
with the nightly toolchain other than bench.
Re-name nightly CI job from `bench_nightly`to `Nightly`.
The test that checks for a panic uses `cargo test --exact`, it makes
sense to put it at the top of the script right after we run `cargo test`
so we can run the test without triggering a re-build.
c1bb316675 Make global-context-less-secure actually enable the global context (Elichai Turkel)
Pull request description:
In #407 we restored the `global-context-less-secure` feature, but it didn't actually do anything because #385 changed all the cfg checks on the whole module to depend on `global-context`, so we need to enable `global-context` in order to make that module compile.
so before all this, users could enable *just* `global-context-less-secure` without enabling the `global-context`, and after this PR it will behave the same.
(this will not enable the randomization because of: 1cf2429b12/src/context.rs (L51))
ACKs for top commit:
apoelstra:
ACK c1bb316675
Tree-SHA512: edc7b4916b359a0696cc25f498bc52ad340f981ad6b01b83b68966d6179200bac6acb96f5480157e24c605b5552bdd7b6eb8770bc9a2c5734da3df11c021fb5b
f93ca81348 Add sign_ecdsa_with_noncedata and sign_ecdsa_recoverable_with_noncedata (junderw)
Pull request description:
Fixes#424
As discussed on [IRC](https://gnusha.org/bitcoin-rust/2022-03-19.log) (starts at 09:14).
These methods will allow for users to generate multiple signatures with the same private key and message by utilizing one of the `Variants` mention in RFC6979 which is exposed by libsecp256k1 via the `noncedata` argument.
The reasoning behind adding this is to allow our library to migrate from using the -sys crate. Currently we support using this noncedata argument, and would like to continue doing so while at the same time migrating away from -sys crate.
ACKs for top commit:
apoelstra:
ACK f93ca81348
Tree-SHA512: 494d4f9046960779e199b18ff908fe74feda66a5cfc066c9ae6f3836fcaabd56defaa2138a913b25f1af3aa7dd48986e058804223224b76b303837c0c7adbaed
de65fb2f1e Implement de/serialization for SharedSecret (Tobin Harding)
Pull request description:
As we do for other keys implement serde de/serialization for the `SharedSecret`.
Please note, this adds `from_slice` and `from_bytes` (borrowed and owner respectively) because I needed to use them. Doing so treads on @dspicher's toes because he is in the process of implementing an owned conversion method for `SharedSecret`. The fair thing to do would be let https://github.com/rust-bitcoin/rust-secp256k1/pull/417 resolve and merge before merging this one (I can rebase).
~Side note, its kind of rubbish that `BytesVisitor` deserializes into a buffer (presumably) then we reallocate and copy the buffer to use the borrowed conversion method due to the serde function signature `fn visit_bytes<E: de::Error>(self, v: &[u8]) -> Result<Self::Value, E>`~ (I was bumbling nonsense.)
Closes: #416
ACKs for top commit:
apoelstra:
ACK de65fb2f1e
Tree-SHA512: 3d484f160d8a459a867f645736713984bad982429236ac5351c20b6c21b42cec86e68009fe7adf062912037cf7e747e5b15357a5fd7900e52169f208a4e56710
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.
463148f9a0 bump version to 0.22.1 (Dominik Spicher)
9be8e74107 Allow SharedSecret to be created from byte array (Dominik Spicher)
Pull request description:
This was accidentally removed in 8b2edad. See also the discussion
on https://github.com/rust-bitcoin/rust-secp256k1/pull/402Closes#416.
ACKs for top commit:
apoelstra:
ACK 463148f9a0
Tree-SHA512: 04e16226efa2cf6fd461eabb0c78e5b00f347c78e20c1c7561591ffa74a7259fb3265b49a9d7326caf70e4d5ce32a620485f1bd5538c292654f91eb68c2a57dc
0fd07ad059 Improve CI pipeline (Tobin Harding)
Pull request description:
We have unnecessary runs of the `test.sh` script. We can simplify the CI pipeline and at the same time improve the docs build by using `--cfg docsrs`.
- Remove the `wasm` job, replace it by enabling the `DO_WASM` env var for the stable toolchain run in the `Tests` job.
- Add `--cfg docrs` flag to the docs build and set the `DO_DOCS` env var as part of the nightly toolchain run in `Tests` job.
The end result is one less run of the `test.sh` script and better test coverage.
Idea came from @Kixunil when reviewing https://github.com/rust-bitcoin/rust-bitcoin/pull/858, thanks.
ACKs for top commit:
apoelstra:
ACK 0fd07ad059
Tree-SHA512: 063493ce03aa8cef5d7fc7636f3bfaaeff5c918d7076473bac23313082e8357d5282fcaf4d76a3dc5b0650e7ee43fa9d2b738f79863be7f24f2acf32f99da4b1
aa516384df update changelog for 0.22.0 (Andrew Poelstra)
d06dd2023b update fuzzdummy API to match normal API (Andrew Poelstra)
f3d48a298e update "should terminate abnormally" test to trigger a different ARG_CHECK (Andrew Poelstra)
8294ea3f50 secp256k1-sys: update upstream library (Andrew Poelstra)
2932179bd6 secp256k1-sys: update secp256k1.h.patch (Andrew Poelstra)
Pull request description:
Should wait on merging until we get a minor release out with #382 and #376.
May also want to bundle #380 with this?
ACKs for top commit:
real-or-random:
ACK aa516384df I can't judge if the feature set is meaningful but this release PR is fine
Tree-SHA512: e7f48b402378e280a034127f2de58d3127e04303a114f07f294fa3d00c0a083ae0d43375a8a74d226b13ea45fb3fde07d8450790e602bbf9581adc5fd8bc7d29
We have unnecessary runs of the `test.sh` script. We can simplify the CI
pipeline and at the same time improve the docs build by using `--cfg
docsrs`.
- Remove the `wasm` job, replace it by enabling the `DO_WASM` env var for
the stable toolchain run in the `Tests` job.
- Add `--cfg docrs` flag to the docs build and set the `DO_DOCS` env var
as part of the nightly toolchain run in `Tests` job.
The end result is one less run of the `test.sh` script and better test
coverage.
faa153988f Remove call to deprecated methods (Tobin Harding)
Pull request description:
We recently added `sign_ecdsa` and `verify_ecdsa` and deprecated `sign`
and `verify`. The `no_std_test` crate got missed during the upgrade.
Remove call to deprecated methods `sign` and `verify` in `no_std_test`
crate.
ACKs for top commit:
apoelstra:
ACK faa153988f
Tree-SHA512: 27a66e3e254744dfeae46ecc846e1c3229277254d9847f87de3167704d3425a504f8bee22be859f4e119672b1b18b98c3b31d84149d68b5f9c5c1c580662f989
6bcf3ea0d0 Add bitcoin-hashes-std features (Tobin Harding)
555833b70f Disable bitcoin_hashes default features (Tobin Harding)
b6f169f083 Improve manifest whitespace (Tobin Harding)
Pull request description:
Currently we use default features for the `bitcoin_hashes` dependency, doing so breaks the `no-std` feature in `rust-bitcoin` because `std` is part of `bitcoin_hashes` default feature set.
Disable `bitcoin_hashes` default features, no changes to `rust-bitcoin` are require after this change since we manually turn on `std` and `alloc` as part of the `std`/`no-std` features of `rust-bitcoin`.
For other users of `rust-secp256k1` this is a breaking change but is unlikely to cause too much bother because `std` is so commonly used.
This PR resolves an open [issue](https://github.com/rust-bitcoin/rust-secp256k1/pull/384) in `rust-bitcoin`, see issue for discussion.
ACKs for top commit:
apoelstra:
ACK 6bcf3ea0d0
Tree-SHA512: 3cb83b67ba73b096f05cb5c98e1057c34cbf75208c626830a9c5050d3927c7dc6c13109e43c01701b1dfa7adfcfb6745bae6501f903be5976f6d1534fa9b3598
Currently we use 'no default features' for the `bitcoin_hashes`
dependency. Doing so means that if users want the `std` feature they
need to explicitly add a `bitcoin_hashes` dependency even though we
re-export `bitcoin_hashes` as `hashes`. This means that in the common
case the re-export is pointless. As an example, `rust-bitcoin`
unnecessarily requires an explicit dependency on `bitcoin_hashes`.
Add `bitcoin-hashes-std` feature so that users do not need an explicit
dependency in the common use case.
Change the test matrix to only test '*-std' features when 'std' is
enabled since enabling one without the other is illogical. Please note,
this replaces the test run of feature 'std'+'rand'+'rand-std' with just
'std'+'rand-std' because enabling 'rand-std' enables 'rand' so the
explicit additional feature is redundant.
7b91f9d8ef Remove schnorrsig from test names (Tobin Harding)
4b840ffe87 Remove schnorrsig from helper method (Tobin Harding)
79770e17f3 Deprecate SCHNORRSIG_SIGNATURE_SIZE (Tobin Harding)
7a417fd1c5 Deprecate SCHNORRSIG_PUBLIC_KEY_SIZE (Tobin Harding)
Pull request description:
Recently we moved from using the identifier 'schnorrsig' to 'schnorr' but we missed a few places.
Change identifiers to use 'schnorr' instead of 'schnorrsig', deprecate if necessary.
Please note, does not touch `secp256k1-sys`. Use of 'schnorrsig' remains in `secp256k1-sys`,
ACKs for top commit:
apoelstra:
ACK 7b91f9d8ef
Tree-SHA512: 709594f444b778b521e653822241b41df370a8cb1da802844d19ce12d01edb84bd69453df8bc57ba757b5b8d15cc71b04d787093403d04a436debeaa477f139c
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`.
We recently added `sign_ecdsa` and `verify_ecdsa` and deprecated `sign`
and `verify`. The `no_std_test` crate got missed during the upgrade.
Remove call to deprecated methods `sign` and `verify` in `no_std_test`
crate.
Currently we use default features for the `bitcoin_hashes` dependency,
doing so breaks the `no-std` feature in `rust-bitcoin` because `std` is
part of `bitcoin_hashes` default feature set.
Disable `bitcoin_hashes` default features, no changes to `rust-bitcoin`
are require after this change since we manually turn on `std` and
`alloc` as part of the `std`/`no-std` features of `rust-bitcoin`.
For other users of `rust-secp256k1` this is a breaking change but is
unlikely to cause too much bother because `std` is so commonly used.
Mirror the whitespacing in `rust-bitcoin` by doing:
- Only use single line of whitespace between sections
- Separate optional dependencies from non-optional ones
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
cf6badf96a Obfuscate SharedSecret when printing (Tobin Harding)
e4be664d97 Improve rustdocs for displaying secrets (Tobin Harding)
5c7c76eb74 Rename serialize_secret -> secret_bytes (Tobin Harding)
4ded2c0478 Use byte instead of i (Tobin Harding)
91106f5685 Remove magic number (Tobin Harding)
6dca99631f Mention bitcoin_hashes in obfuscated secret msg (Tobin Harding)
Pull request description:
Currently printing the `SharedSecret` using `Display` or `Debug` prints the real secret, this is sub-optimal. We have a solution for other secrets in the project where printing is obfuscated and we provide a `display_secret` method for explicitly printing.
Mirror the logic for other secrets and obfuscate the `SharedSecret` when printing.
- Patches 1 - 5: Clean up.
- Patch 6: The meat and potatoes.
This is the final change needed to:
Resolve: #226
ACKs for top commit:
apoelstra:
ACK cf6badf96a
Tree-SHA512: df14e8c5f5815bd76c585a1cd1db42fab6858004ca2cafa9a158b8b04a44c4a11b1260374a6ff82fee540ca955f262b28efae023012de5ac3832e4f5d1d1815e
Currently printing the `SharedSecret` using `Display` or `Debug` prints
the real secret, this is sub-optimal. We have a solution for other
secrets in the project where printing is obfuscated and we provide a
`display_secret` method for explicitly printing.
Mirror the logic for other secrets and obfuscate the `SharedSecret` when printing.
Improve rustdocs on `display_secret` by doing:
- Minor improvements to the rustdocs to aid readability in the editor.
- Do not guarantee (`assert_eq!`) debug output
Tobin Harding
Andrew Poelstra
Elichai Turkel
junderw
Dominik Spicher
Martin Habovstiak