866cf8c732 Fix rustdoc link (Thomas DuBuisson)
Pull request description:
Not familiar with rustdoc, let's see what CI says to make sure I got it right.
ACKs for top commit:
apoelstra:
ACK 866cf8c732
Tree-SHA512: 47aaf5932d1622be071a189f15f93cc206e6aae53ee771a14bf18b6a0acecf057f15b69c3d2460b39d15ff2bb3f34984a544574de0bd279bbcaef04eb2077c42
6e0ae2a7bb Document sig verify's intentional limitation (Thomas M. DuBuisson)
Pull request description:
This bit of documentation is similar to the secp256k1 C lib comment:
```
* To avoid accepting malleable signatures, only ECDSA signatures in lower-S
* form are accepted.
```
ACKs for top commit:
apoelstra:
ACK 6e0ae2a7bb
Tree-SHA512: 3259898c497b33cb967eac910ce746d6ccf2706adb0563ce862737156ef82e65d486d1b83c62dd474350a1fce4a2f9f5da20509ed85af2c138f4ea3a29cc240c
This is only needed for the serde test, so don't bother putting it in
the README. Downstream users won't encounter this dependency and don't
need to care about it.
The `cbor` crate has been unmaintained for several years, and depends on
the ancient `rustc_serialize` crate which (a) doesn't build on WASM, and
(b) doesn't build when we use a minimal-dep Cargo.lock. (The latter is
because cbor specifies rustc_serialize 0.3.0 when it should specify 0.3.1,
but there is nothing we can do to fix that when cbor is unmaintained.)
This changes a hardcoded value in a regression test, but it's because
we're replacing the serialization engine rather than changing our code,
so this is not actually a change.
5ae136d7bd Bump secp256k1-sys version to 0.8.1 (Tobin C. Harding)
Pull request description:
We just bumped the version of `secp256k1`, since we recently added a new public function to `secp256k1-sys` we need to bump the minor version number there too.
Should have been done as part of #588, its hard to get good help :)
ACKs for top commit:
apoelstra:
ACK 5ae136d7bd
Tree-SHA512: e763257ede269544f4fd21fd76cf4279dff2dcb4835933652a796b0ad54f364f9a893c13c85b5d05acd6805bc51d98b639fa9c1330fad5fa2313d28aafc2bb60
We are ready to release a new minor version of `secp256k1-sys`, in order
to do so we must make change the symbol names to reflect the new version
as well as the usual changelog and version bump.
In preparation for releasing `secp256k1-sys` v0.8.1 do:
- Rename symbols to from `0_8_0` -> `0_8_1`, done mechanically (search
and replace)
- Add changes log notes (includes changelog entry for 0.8.0)
- Bump `secp256k1-sys` crate version 0.8.0 -> 0.8.1, justified because
we have added a new public function.
45395190c2 Bump version to 0.27.0 (Tobin C. Harding)
8e772493dc Depend on bitcoin_hashes v0.12 (Tobin C. Harding)
Pull request description:
Depend on the newly released `bitcoin_hashes` version 0.12, add changelog, and bump to v0.27.0
ACKs for top commit:
Kixunil:
ACK 45395190c2
apoelstra:
ACK 45395190c2
Tree-SHA512: 9ea99c88a90d0d34dfbbd3e467ea77a2981a7eae75c52163eed805381683e7555ea841d9c953787ab878ce8848d26fd9a593bb2eb52b5be28cee3930a373434c
8fffbeab13 implement "non_secure_erase" methods (kwantam)
Pull request description:
This PR adds [`Zeroize`](https://docs.rs/zeroize) derivations for the following structs:
- `SecretKey`
- `KeyPair`
- `SharedSecret`
- `Scalar`
- `DisplaySecret`
This is *only* a Zeroize impl, and does not make Zeroize happen automatically on drop (doing that would be a breaking change because it would preclude deriving `Copy`). But this is still useful, because it allows downstream libraries to implement `ZeroizeOnDrop` for structs that contain such secrets and/or simply to use the `Zeroizing` container struct.
Because these new impls are never invoked automatically, performance impact should be zero. Safety-wise, the `Zeroize` library appears to be widely used in cryptographic code. For example, Supranational's [blst](https://github.com/supranational/blst) Rust bindings use it, and in turn are used in one of the most popular eth2 validator implementations.
Thanks for maintaining a really great library!
ACKs for top commit:
tcharding:
FWIW ACK 8fffbeab13
apoelstra:
ACK 8fffbeab13
Tree-SHA512: 28d2cdcc6bd2d2d6330b67ae8635561882e869199d8fef9a3ebc3f368a7a0c2c00b818281190133f551b099e9c5226f104a56edc14c9b6f699ceba49e4b24563
This PR implements a `non_secure_erase()` method on SecretKey,
KeyPair, SharedSecret, Scalar, and DisplaySecret. The purpose
of this method is to (attempt to) overwrite secret data with
valid default values. This method can be used by libraries
to implement Zeroize on structs containing secret values.
`non_secure_erase()` attempts to avoid being optimized away or
reordered using the same mechanism as the zeroize crate: first,
using `std::ptr::write_volatile` (which will not be optimized
away) to overwrite the memory, then using a memory fence to
prevent subtle issues due to load or store reordering.
Note, however, that this method is *very unlikely* to do anything
useful on its own. Effective use involves carefully placing these
values inside non-Copy structs and pinning those structs in place.
See the [`zeroize`](https://docs.rs/zeroize) documentation for tips
and tricks, and for further discussion.
[this commit includes a squashed-in commit from tcharding to fix docs
and helpful suggestions from apoelstra and Kixunil]
e705bcffb5 Fully describe safety requirements (Tobin C. Harding)
Pull request description:
Currently we have a wildcard on safety requirements, saying more or less "plus a bunch of other stuff we don't mention". This is not helpful.
Attempt to fully describe the safety requirements of creating a context from a raw context (all, signing only, and verification only).
Fix: #544
## Note
This is best effort only, will require some thought to review. To do this I read https://doc.rust-lang.org/reference/behavior-considered-undefined.html and then I flicked through `depend/secp256k1/src/secp256k1.c` and `util.h` to look for things that could cause things in the linked to list of UB.
ACKs for top commit:
apoelstra:
ACK e705bcffb5
Kixunil:
ACK e705bcffb5
Tree-SHA512: 0180d196f6d528e3c7a06da54ef58d015df19c351d98030453ae5c5e62e0565797b06169f27f5d8b40ea0b9adba377cadd45dd306c8213d0bdc98b20651766c7
e597860a64 Followup: Disallow missing `Debug` implementations for `Scalar` type. (Arik Sosman)
Pull request description:
Because `Scalar` now implements it, that carveout is no longer necessary.
ACKs for top commit:
tcharding:
ACK e597860a64
apoelstra:
ACK e597860a64
Tree-SHA512: fd9682550cc6bd2d3d59d067d3a0c7faf5767b4c127d86f95c7355ff795189272f399ce2df7d870f85fa3a3d6727fa6debc058171aab965a8f0aa5b5aecff581
8ed8cac2fe Implement `Debug` trait for `Scalar` type. (Arik Sosman)
Pull request description:
Currently, `Scalar` types do not implement the `Debug` trait, whereas most other types in the library do. Besides that being an upstream requirement for us, I believe it would also be quite useful for users of that type.
Also implements the `Index` traits for `Scalar`.
ACKs for top commit:
apoelstra:
ACK 8ed8cac2fe
Tree-SHA512: f254859144850e40badf6ace2b2a1b231e5ed224ec60861586cd5f2042167d89c759dc16a1075702bce90d810ac60db924ea8cb20d82099a42fddb2718da12db
Currently we have a wildcard on safety requirements, saying more or less
"plus a bunch of other stuff we don't mention". This is not helpful.
Attempt to fully describe the safety requirements of creating a context
from a raw context (all, signing only, and verification only).
Fix: #544
d1184156c6 Fix CI (Tobin C. Harding)
Pull request description:
Currently CI is broken because we use the latest version of `rustfmt` and `clippy` in CI. We can resolve the `rustfmt` issue permanently by removing the `required_version` config option. We also need to fix the latest clippy warnings.
Done as a single patch so that all patches pass CI.
ACKs for top commit:
apoelstra:
ACK d1184156c6
Tree-SHA512: 846d78d974f40f63ee605faf095f000b14057eb04450c3612054673594ea6ef3a110033d20bc57d3a943b3c8853fbad3102e2fdc6863227cb684c22f7fa6ffc7
Currently CI is broken because we use the latest version of `rustfmt`
and `clippy` in CI. We can resolve the `rustfmt` issue permanently by
removing the `required_version` config option. We also need to fix the
latest clippy warnings.
Done as a single patch so that all patches pass CI.
bdfa0ffcd0 Use library to_hex function (Tobin C. Harding)
Pull request description:
We do not need to use the `hex` module from `bitcoin_hashes` to encode into hex, we have a function in this library.
Use library hex encoding logic, removes dependency on the `hex` module of `bitcoin_hashes` entirely from this crate.
ACKs for top commit:
apoelstra:
ACK bdfa0ffcd0
Tree-SHA512: 0923939cfeb3cb4f8a3c2fad3961f2b17d3083d85232b3992d9efef59e622fa18f6ecf3c93b064518a7cb6ac4b704480a59ecdc3bcc016811758b4a13b00d31f
We do not need to use the `hex` module from `bitcoin_hashes` to encode
into hex, we have a function in this library.
Use library hex encoding logic, removes dependency on the `hex` module
of `bitcoin_hashes` entirely from this crate.
5a3f13eecf Overcome ASAN false positive regression (Tobin C. Harding)
Pull request description:
The Memory Sanitizer is giving a false positive at the moment in `nightly`. Adding compiler flags resolves the issue. I didn't grok the exact root cause but this fixes it (cut'n'pasta from the issue [0]).
Props to elichai for working this out: https://github.com/rust-bitcoin/rust-secp256k1/pull/573#issuecomment-1399465995
[0] https://github.com/rust-lang/rust/issues/107149
ACKs for top commit:
apoelstra:
ACK 5a3f13eecf
Tree-SHA512: 873145b732f7574c93ecc1bbabd9d82a1e501a39d1e2184770f71a07ffb72468783ab1b3fbfef8ef377c7e7a4b8c45253da1fce11660152d3369902136f1c049
The Memory Sanitizer is giving a false positive at the moment in
`nightly`. Adding compiler flags resolves the issue. I didn't grok the
exact root cause but this fixes it (cut'n'pasta from the issue [0]).
[0] https://github.com/rust-lang/rust/issues/107149
fe828d040d add redundant features for cargo 1.41 bug (Andrew Poelstra)
Pull request description:
It looks like cargo versions up to 1.45 have a bug that sometimes causes dependency resolution to fail. There is a straightforward, somewhat ugly fix, which this PR implements.
In #571 we implemented this and rush-merged it into 0.24.x because (a) it was breaking our CI, (b) it was clearly harmless, even if it turn out not to be the best decision. For this PR I'd like somebody other than me and Sanket to take a look and sanity check us.
If we merged this, it will also need a backport to 0.25.x.
For more details see #571 and the linked issues there.
ACKs for top commit:
Kixunil:
ACK fe828d040d
Tree-SHA512: 1e771137088036ae7331d42c86955a2a8c73c22f2850d03b8a9e9b7aa21315d558cbfe6cb1f4c839fa8df15b24756bc26eda25b1214d27e719abd10af2cef5fc
43370d8128 Add secp256k1_schnorrsig_sign_custom in fuzzing config (Tibo-lg)
Pull request description:
Trying to do some fuzz testing I noticed that I had omitted to add `secp256k1_schnorrsig_sign_custom` to the `fuzz_dummy` module of `secp256k1sys` crate in #440. This PR adds it. I just forwarded the call to `secp256k1_schnorrsig_sign` as I didn't have any better idea but open to suggestions.
ACKs for top commit:
apoelstra:
ACK 43370d8128
Tree-SHA512: 44789fbf7c0186a7e0c0a445efd48c32e5a23169cd5d723aa19a04c5d0cb1bf6eeefbd2d153e5cb58f25eb823b5ee41a35411af3996722ed389ab18a741b388e
2dad589394 Upgrade the vendored libsecp256k1 code (Tobin C. Harding)
2d4aacc4ad Update scratch_impl.h patch file (Tobin C. Harding)
Pull request description:
This bumps `secp256k1` to v0.26.0 and `secp256k1-sys` to v0.8.0
`libsecp256k1` v0.2.0 was just released.
Update the vendored code using
`./vendor-libsecp.sh depend 0_8_0 21ffe4b`
```
git show 21ffe4b
commit 21ffe4b22a9683cf24ae0763359e401d1284cc7a (tag: v0.2.0)
Merge: 8c949f5 e025ccd
Author: Pieter Wuille <pieter@wuille.net>
Date: Mon Dec 12 17:00:52 2022 -0500
Merge bitcoin-core/secp256k1#1055: Prepare initial release
e025ccdf7473702a76bb13d763dc096548ffefba release: prepare for initial release 0.2.0 (Jonas Nick)
6d1784a2e2c1c5a8d89ffb08a7f76fa15e84fff5 build: add missing files to EXTRA_DIST (Jonas Nick)
13bf1b6b324f2ed1c1fb4c8d17a4febd3556839e changelog: make order of change types match keepachangelog.com (Jonas Nick)
b1f992a552785395d2e60b10862626fd11f66f84 doc: improve release process (Jonas Nick)
ad39e2dc417f85c1577a6a6a9c519f5c60453def build: change package version to 0.1.0-dev (Jonas Nick)
90618e9263ebc2a0d73d487d6d94fd3af96b973c doc: move CHANGELOG from doc/ to root directory (Jonas Nick)
Pull request description:
Based on #964
ACKs for top commit:
sipa:
ACK e025ccdf7473702a76bb13d763dc096548ffefba
Tree-SHA512: b9ab71d7362537d383a32b5e321ef44069f00e3e92340375bcd662267bc5a60c2bad60222998e6602cfac24ad65efb23d772eac37c86065036b90ef090b54c49
```
Requires a new version of `secp256k1-sys`, use v0.8.0
- Update the `secp256k1-sys` manifest (including links field)
- Update symbols to use 0_8_0
- Add a changelog entry
- depend on the new version in `secp256k1`
Which in turn requires a new version of `secp256k1`, use v0.26.0
ACKs for top commit:
apoelstra:
ACK 2dad589394
Tree-SHA512: 58eb5a276a78336e45b1473970f2d68dc2249b4a751deae44d70c2453cf5798b0edc0fdee2eabfb5707053e76e3a49849009b0c2f9dce08bd4bb5bb8d3549a62
`libsecp256k1` v0.2.0 was just released.
Update the vendored code using
`./vendor-libsecp.sh depend 0_8_0 21ffe4b`
```
git show 21ffe4b
commit 21ffe4b22a9683cf24ae0763359e401d1284cc7a (tag: v0.2.0)
Merge: 8c949f5 e025ccd
Author: Pieter Wuille <pieter@wuille.net>
Date: Mon Dec 12 17:00:52 2022 -0500
Merge bitcoin-core/secp256k1#1055: Prepare initial release
e025ccdf7473702a76bb13d763dc096548ffefba release: prepare for initial release 0.2.0 (Jonas Nick)
6d1784a2e2c1c5a8d89ffb08a7f76fa15e84fff5 build: add missing files to EXTRA_DIST (Jonas Nick)
13bf1b6b324f2ed1c1fb4c8d17a4febd3556839e changelog: make order of change types match keepachangelog.com (Jonas Nick)
b1f992a552785395d2e60b10862626fd11f66f84 doc: improve release process (Jonas Nick)
ad39e2dc417f85c1577a6a6a9c519f5c60453def build: change package version to 0.1.0-dev (Jonas Nick)
90618e9263ebc2a0d73d487d6d94fd3af96b973c doc: move CHANGELOG from doc/ to root directory (Jonas Nick)
Pull request description:
Based on #964
ACKs for top commit:
sipa:
ACK e025ccdf7473702a76bb13d763dc096548ffefba
Tree-SHA512: b9ab71d7362537d383a32b5e321ef44069f00e3e92340375bcd662267bc5a60c2bad60222998e6602cfac24ad65efb23d772eac37c86065036b90ef090b54c49
```
Requires a new version of `secp256k1-sys`, use v0.8.0
- Update the `secp256k1-sys` manifest (including links field)
- Update symbols to use 0_8_0
- Add a changelog entry
- depend on the new version in `secp256k1`
Which in turn requires a new version of `secp256k1`, use v0.26.0
A recent update to clippy introduced a new class of warning.
Clippy emits:
warning: casting to the same type is unnecessary (`usize` -> `usize`)
As suggested remove the unnecessary cast.
01b1fbcccb Update CHANGELOG for release of v0.25.0 (Tobin C. Harding)
Pull request description:
We just did a new release of `secp256k1-sys` needed for release of v0.25.0
Update the CHANGELOG to include secp-sys version bump.
ACKs for top commit:
apoelstra:
ACK 01b1fbcccb
Tree-SHA512: 87ffc0f4ce468975b99ea3a0bbc97f8d0d048ecaee9982550bad13f564e0ea0093ce7dfdb7af0b5de8f14ff00eafd29bdb23dcf15ad52e93dc194459de0e15ab
3fa2436272 Bump secp256k1-sys version to 0.7.0 (Tobin C. Harding)
Pull request description:
We are ready to release a new minor version of `secp256k1-sys`, in order to do so we must make change the symbol names to reflect the new version as well as the usual changelog and version bump.
In preparation for releasing `secp256k1-sys` v0.7.0 do:
- Rename symbols to from `0_6_1` -> `0_7_0`, done mechanically (search and replace)
- Add changes log notes
- Bump `secp256k1-sys` crate version 0.6.1 -> 0.7.0, justified because we have added new public methods to various types (e.g., `PublicKey::cmp_fast_unstable`)
### Notes
I based this PR on:
- https://github.com/rust-bitcoin/rust-secp256k1/pull/490/files
- https://github.com/rust-bitcoin/rust-secp256k1/pull/457/files
ACKs for top commit:
apoelstra:
ACK 3fa2436272
Tree-SHA512: cb16de633865f26613aa29479ac6a6299b1790a00372cca61173f09a753179fa1d619b91ca25ba5872f571d3d9372b46731f9d4b3e8050077ec3c73d583f54ce
We are ready to release a new minor version of `secp256k1-sys`, in order
to do so we must make change the symbol names to reflect the new version
as well as the usual changelog and version bump.
In preparation for releasing `secp256k1-sys` v0.7.0 do:
- Rename symbols to from `0_6_1` -> `0_7_0`, done mechanically (search
and replace)
- Add changes log notes
- Bump `secp256k1-sys` crate version 0.6.1 -> 0.7.0, justified because
we have added new public methods to various types (e.g.,
`PublicKey::cmp_fast_unstable`)
494b07a415 Add changelog entry (Tobin C. Harding)
d0c4af0e26 Add newline after docs heading (Tobin C. Harding)
Pull request description:
~Bump version to 0.25.1 ready to release~ Add changelog entry for the recently fixed unsoundness issue.
Patch 1 is an annoyingly trivial fix to docs.
ACKs for top commit:
apoelstra:
ACK 494b07a415
Tree-SHA512: 8de8d735d3dce06683ec8e66b78b966406f42ea0a8e679e8e82143a984251addd74bea3658cc63ba9d9eada3517e461e9c28085d5261d9c0db2dceb15a8cbcc2
1d6a46eb6d change bitcoin-hashes feature gates to bitcoin_hashes (Andrew Poelstra)
Pull request description:
This leaves `bitcoin-hashes` in place everywhere in the documentation, but changes it to `bitcoin_hashes` on the actual feature gates, to ensure that both flags work.
It's unfortunately a bit hard to test this because the library will be self-consistent pretty-much no matter what we do ... the issue is if the user enables the wrong flag we want to make sure that all the APIs we intend to be visible are actually visible.
Fixes#562.
ACKs for top commit:
tcharding:
ACK 1d6a46eb6d
Tree-SHA512: 1e060aeb2810ef1e23cf5c956023aca586550ba0287bbf7bc1108dc14091e17d7601aac3f057d0313fafd21351cdda1b08b4250f34ecde917be686d0b739e65a
Recently we found and fixed an unsoundness issue in the
`preallocated_gen_new` function. As we have yet to release 0.25.0 we can
just update the changelog to reflect the newly merged fix.
As is customary add a newline between rustdoc heading and content. Done
so that the code is identical to other released code (during backport
the space was added).
1e6eb6cb4d shut clippy up (Andrew Poelstra)
f961497e69 context: introduce unsafe `PreallocatedContext` trait (Andrew Poelstra)
Pull request description:
Stop this from being a generic function over all contexts, to only a function generic over contexts where we can bound the lifetime precisely. Introduces a new unsafe trait. I *believe* the only code this breaks was already unsound:
* code that tried to use one of the `*Preallocated` context markers with an incorrect lifetime
* code that tried to use `preallocated_gen_new` with a non-`*Preallocated` marker, which I believe we allowed before (I just noticed this now) and almost certainly would've led to UB on drop
Fixes#543
ACKs for top commit:
Kixunil:
ACK 1e6eb6cb4d
tcharding:
ACK 1e6eb6cb4d
Tree-SHA512: 44eb4637a2f86d5b16d40174cb9e27f37cf8eb4f29546159dbbdcd3326d01f9de2f500ba732376dd84e67ebc3528c709d2d4e2aceb8a329bcb9fb9d25c9b89cb
9b07e8e8c5 secp-sys: Use NonNull in API instead of *mut T (Tobin C. Harding)
Pull request description:
Currently we expect non-null pointers when we take `*mut T` parameters, however we do not check that the pointers are non-null because we never set VERIFY in our C build. We can use the `NonNull` type to enforce no-null-ness as long as we use `NonNull::new`. In a couple of instances we manually check that a buffer is not empty and therefore that the pointer to it is non-null so we can safely use `NonNull::new_unchecked`.
Replace mutable pointer parameters `*mut T` (e.g. `*mut c_void`) and return types with `NonNull<T>`.
Fix#546
### Note
The description above fully explains the issue to the best of my knowledge, if the description is not spot on then I'm not fully understanding the issue. Please correct me if this is the case.
> One unfortunate thing is that this means that we wouldn't be able to implement CPtr for secp256k1::Context, which is our designated "expose types from secp256k1-sys which is not stable" semver escape hatch.
You've lost me here? `secp256k1::Context` is a trait did you mean `secp256k1::Secp256k1` or `secp256k1_sys::Context`?
ACKs for top commit:
apoelstra:
ACK 9b07e8e8c5
Tree-SHA512: 37aceebfa62e590ce8cc282c35b014ad018e5cfbea99402ed3aa1fcbaa69e01a01c1c1f32351f5f15a7d270e31da5b239ee5bc11d2343cf866082ad85df6a622
Fixes unsoundness in `preallocated_gen_new` which previously did not
properly constrain the lifetime of the buffer used to back the context
object. We introduce an unsafe marker trait, and impl it for our
existing preallocated-context markers.
Annoyingly the trait has to be public even though it should never be
used directly, and is only used alongside the sealed `Context` trait,
so it is de-facto sealed itself.
Fixes#543
Andrew Poelstra
Thomas DuBuisson
Tobin C. Harding
kwantam
Arik Sosman
Tibo-lg
Elichai Turkel