The implementations of `ThirtyTwoByteHash` for types from the `hashes`
crate are problematic during upgrades because both `bitcoin` and
`secp256k1` depend on `hashes` and when the versions of `hashes` get
out of sync usage of the trait breaks.
Deprecate the `ThirtyTwoByteHash` trait and remove the impls for types
from `bitcoin_hashes`.
Add an explanation in the changelog because its too long to go in the
deprecation message.
Wildcards make it hard to grep for where stuff comes from, explicit
imports and re-exports are ... more explicit.
Re-export the `key` types explicitly.
Wildcards make it hard to grep for where stuff comes from, explicit
imports and re-exports are ... more explicit.
Import and re-export explicitly instead of by using wildcards.
The inner bytes of `ElligatorSwiftSharedSecret` were almost inaccessible
making the type almost useless, so this commit adds methods to access
inner bytes.
Closes#675
dd6bf7c10d Fix unit test import statements (Tobin C. Harding)
Pull request description:
In `lib.rs` unit tests we are getting build warnings because of how we are importing things, just import with `super::*` unconditionally and be done with it.
This patch is the only good one out of #661.
ACKs for top commit:
apoelstra:
ACK dd6bf7c10d
Kixunil:
ACK dd6bf7c10d
Tree-SHA512: 3970f4c1374ec6de4798bfb52b561e9ac4611ec3a3885edc79639566f777e1fbb502cb36fa7abd015f3fd4a9ca4b6a4931b4ecb2e629e967b4e49391db97a97f
In `lib.rs` unit tests we are getting build warnings because of how we
are importing things, just import with `super::*` unconditionally and be
done with it.
Create bindings for all methods and static types in ellswift.h in
secp256k1-sys and their respective safe-rust types.
All methods are extensively commented and tested using BIP324's
test vectors
This is just a bad test. It constructs a preallocated context object by
starting from a non-preallocated context object, in a way that can't be
done by users (since it directly constructs a `Secp256k1` struct) and a
way that is very difficult to unwind, because you wind up with two
pointers to the same underlying context object, one a "preallocated" one
and one a normal one.
If you then drop the preallocated one, it will call
`secp256k1_context_destroy`, forcing you to manually deallocate the
other one. If you drop the normally-allocated one, you need to
mem::forget the preallocated one to avoid calling
`secp256k1_context_destroy` twice. The whole thing is pretty fragile.
There is another unit test, `test_raw_ctx`, which gets into the same
situation but using the public API, and demonstrates a few ways to get
out of it.
aa4489c71b key: Improve docs (Tobin C. Harding)
Pull request description:
`PublicKey` types are for verifying ECDSA signatures, when these docs where written there were no other types of signatures. With the addition of taproot these docs have become stale.
ACKs for top commit:
apoelstra:
ACK aa4489c71b
Tree-SHA512: bb24d82f2bf316f8907b1bf02132d454d21f0b13d57f06f09f9985bc7fbf7b36e6972a0fdaf3a68967577dbe1995f2a14fd06fddd38eb46718f04bca1c50a445
We use "keypair" in identifiers (local vars and function names) but
`KeyPair` - one of them is wrong.
Elect to follow upstream and define keypair as a single word i.e., use
`Keypair` for type name and `keypair` in identifiers.
This patch can be reproduced mechanically by doing two
search-and-replace operations on all files excluding the CHANGELOG
- Replace "KeyPair" with "Keypair"
- Replace "key_pair" with "keypair"
Now that we have `hashes` as the crate name of `bitcoin_hashes` we can
slightly clean up the import statements.
This is based on the convention we have to import things directly from
the crate if we depend on it and not from the crate level re-export.
Use the more terse `hashes` by way of the `package` field in the
manifest.
Allows us to remove the ugly feature alias "bitcoin-hashes" ->
"bitcoin_hashes" and removes all the bother with the underscore.
Why did we not think of this 2 years ago?
cd40ae7f19 Improve Message constructors (Tobin C. Harding)
Pull request description:
Observe:
- The word "hash" can be a verb or a noun, its usage in function names is therefore at times ambiguous.
- The function name `from_slice` gives no indication as to what the slice input is.
Improve Message constructors by doing:
- Add a constructor `Message::from_digest` that takes a 32 byte array as input.
- Rename `Message::from_slice` to `Message::from_digest_slice` (deprecate `from_slice` and add `from_digest_slice`)
- Improve the docs while we are at it.
### Note
The original PR conflate 2 separate issues, the `Message` constructor naming clarity issue and the upgrade difficulty issue, PR is now only a solution to the first. The second will be done as a separate PR.
ACKs for top commit:
apoelstra:
ACK cd40ae7f19
Tree-SHA512: 4e5aeccf15cca95073f4c3a518b9e1f54f0e33c92c45dfecd1daa31d052022cd28c71bb6df6cff8a6548993e3e22788f11cd2633214ab5a580c753e66d2ea749
Currently the panic message refers to stuff related to development of
the library, this is meaningless for users of the lib. Target panic
message at secp users instead.
We have an unconditional panic for some combination of features, this
causes clippy to give a bunch of useless warnings.
Add allow attributes to quieten down clippy.
Observe:
- The word "hash" can be a verb or a noun, its usage in function names
is therefore at times ambiguous.
- The function name `from_slice` gives no indication as to what the
slice input is.
Improve Message constructors by doing:
- Add a constructor `Message::from_digest` that takes a 32 byte array as
input.
- Rename `Message::from_slice` to `Message::from_digest_slice`
(deprecate `from_slice` and add `from_digest_slice`)
- Improve the docs while we are at it.
8af2cf12da add .serialize() function to schnorr signature (isaac-asdf)
Pull request description:
convert from Signature to a byte_array
ACKs for top commit:
Kixunil:
ACK 8af2cf12da
tcharding:
ACK 8af2cf12da
apoelstra:
ACK 8af2cf12da
Tree-SHA512: b69d58646cdba4d83a79189f18628590970f471771feef0e11e089d73bd934777e3554a448b88a3643203522fde98084fd7570a5cec400516166583a3433c000
896e6c7f2d Introduce SPDX license identifiers (Tobin C. Harding)
Pull request description:
Licenses are boring as hell, so is are all the comments at the top of each file. This patch makes no comment on the merit of license comments in each file, rather this patch reduces the license comment to the minimum possible with no loss of meaning - an SPDX license identifier.
Note also please that we remove the "written by" comments as well for the following reasons (discussed recently on rust-bitcoin repo):
- they are not descriptive because many devs contributed
- they have a tendency to include the wrong date because of cut'n'pasta
- all this info is in the git history
ref: https://spdx.dev/ids/#how
cc elichai because this PR removes your name but you were not explicitly part of the conversation on `rust-bitcoin` about this topic. Here is the issue: https://github.com/rust-bitcoin/rust-bitcoin/issues/1816 also for more on SPDX see https://github.com/rust-bitcoin/rust-bitcoin/pull/1076
ACKs for top commit:
Kixunil:
ACK 896e6c7f2d
apoelstra:
ACK 896e6c7f2d
Tree-SHA512: 6f0ff7ec2632aed510df362e2fb9cf25fe02cae347bdd4a481804a3ea2b9e060c4ec2c85de3e9d1d40920e4b9c4eecfab127e61f3d076886fe8f2fb4bff9f5a7
Licenses are boring as hell, so is are all the comments at the top of
each file. This patch makes no comment on the merit of license comments
in each file, rather this patch reduces the license comment to the
minimum possible with no loss of meaning - an SPDX license identifier.
Note also please that we remove the "written by" comments as well for
the following reasons (discussed recently on rust-bitcoin repo):
- they are not descriptive because many devs contributed
- they have a tendency to include the wrong date because of cut'n'pasta
- all this info is in the git history
ref: https://spdx.dev/ids/#how
I was reading the docs for `normalize_s` and got confused what the
point was - it says that libsecp "will only accept" signatures that
are normalized, which led me to believe it would refuse to
deserialize such signatures. This is untrue, it only refuses to
*validate* such signatures.
Now that we have Rust 1.48 as the MSRV we no longer need the custom
implementations of `PartialEq`, `Eq`, `PartialOrd`, `Ord`, and `Hash`.
We can just let users of the `impl_array_newtype` macro derive these
traits if they want them.
Remove the custom implementations and add derives to our two users of
the macro.
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
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
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.
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.
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.
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
Tobin C. Harding
Steven Roose
Martin Habovstiak
benthecarman
Jiri Jakes
Andrew Poelstra
Davidson Souza
isaac-asdf
Matt Corallo
Thomas DuBuisson
kwantam
Arik Sosman