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
The `serialize_secret` method is a getter method, it does not do any
serialisation. However we use the method on secret keys and key types so
in order for the name to be uniform use the descriptive name
`secret_bytes`.
Rename `serialize_secret` to be `secret_bytes`.
The identifier `i` is predominantly used for indexing an array but we
are using it as a place holder for the iterated value of an array that
is then printed. The identifier `byte` is more descriptive.
Done in preparation for adding similar code to the `ecdh` module.
In array initialisation we use magic number 64, this is the secret bytes
length multiplied by 2.
Please note; we still use the magic number 32, left as such because it
is used in various ways and its not immediately clear that using a
single const would be any more descriptive.
Use `SECRET_KEY_SIZE * 2` instead of magic number 64.
Hashing the debug output for secrets can be done with `bitcoin_hashes`
not just `std`. Mention this in the obfuscated string output when
neither are available.
5603d71ad3 Limit SharedSecret to 32 byte buffer (Tobin Harding)
d5eeb099ad Use more intuitive local var numbering (Tobin Harding)
834f63c26c Separate new_with_hash into public function (Tobin Harding)
Pull request description:
Currently `SharedSecret` provides a way to get a shared secret using SHA256 _as well as_ a way to use a custom hash function to get the shared secret. Internally `SharedSecret` uses a 256 byte buffer, this is a tad wasteful. We would like to keep the current functionality but reduce memory usage.
- Patch 1: Pulls the `new_with_hash` logic out into a standalone public function that just returns the 64 bytes representing the x,y co-ordinates of the computed shared secret point. Callers are then responsible for hashing this point to get the shared secret (idea by @Kixunil, thanks).
- Patch 2: Does trivial refactor
- Patch 3: Uses a 32 byte buffer internally for `SharedSecret`. This is basically a revert of the work @elichai did to add the custom hashing logic. @elichai please holla if you are not happy with me walking all over this code :)
### Note to reviewers
Secret obfuscation is done on top of this in https://github.com/rust-bitcoin/rust-secp256k1/pull/396, they could be reviewed in order if this work is of interest to you.
ACKs for top commit:
apoelstra:
ACK 5603d71ad3
Tree-SHA512: 48982a4a6a700a111e4c1d5d21d62503d34f433d8cb303d11ff018d2f2be2467fa806107018db16b6d0fcc5ff1a0325dd5790c62c47831c7cd2141a1b6f9467d
Andrew Poelstra
Tobin Harding
Elichai Turkel
junderw
sanket1729
Dominik Spicher
Martin Habovstiak