The package metatadata never changes and is not necessary to look at
basically ever, put it down the bottom of the manifest out of the way.
Helps to keep features and dependencies closer together.
Refactor only, no logic changes.
We manually implement these methods (and the GeneralHash trait) on newtypes
around sha256t::Hash, because tagged hashes require a bit more work. In
the next commit (API diff) you will see that this affects two hashes,
which are the only things that appear green in the diff.
Users who want to implement their own engine/from_engine types now need
to do it on their own. We do this for the non-Taproot sighash types in
`bitcoin` (though only privately) to demonstrate that it's possible.
This commit illustrates the transformation I intend to make everywhere
we use newtyped hashes as "general hashes". *Within the module that the
newtype is defined* I encapsulate engine calls, which I do by calling
engine methods on the underlying general hash function. So within the
module there is a slight reduction in type safety, in the sense that I
need to make sure that I'm wrapping stuff properly.
But outside of the module, there will be no difference except that I
will no longer export engine/from_engine/hash/etc on newtyped hashes.
Instead callers will need to compute the newtyped hash only in ways
supported by the API.
In theory we could have a macro to produce engine/from_engine/etc for
newtypes that want to act as general hashes. But AFAICT there is no use
case for this.
Alternately, we could have a macro that produces *private* Engine types
and private engine/from_engine/etc methods for the hashes, which could
be used within the module and would provide stronger type safety within
the module. But in practice, raw hashing is usually only used within a
couple of methods, so all this infrastructure is way overkill and will
just make maintenance harder for everybody.
Manually implement it for Wtxid, Txid and BlockHash, where the all-zero
"hash" has a consensus meaning. But in general we should not be
implementing this method unless we have a good reason to do so. It can
be emulated or implemeted in terms of from_byte_array.
The use of Wtxid::all_zeros is obscure and specific enough that I am
tempted to drop it. But for txid and blockhash, the 0 hash appears in
actual blockdata and we should keep it.
All other uses of all_zeros were either in test code or in places where
the specific hash was not important and [u8; 32] was a more appropriate
type.
As you can see from the - lines in the API diff, there is no reduction
in API surface (we just remove the T:Tag bound from the sha256t::Tag
type, which is not strictly necessary but maybe we would prefer to keep).
1f58476cb4 Run schemars test from extra_tests (Tobin C. Harding)
Pull request description:
We have a mechanism to run additional custom tests by way of the `extra_tests.sh` script in each crate.
Remove the CI job and run the schemars test using `extra_tests.sh`. This patch changes the test coverage because currently the schemars test is only run with a stable toolchain but with this patch applied it runs with stable, MSRV, and nightly.
Fix: #2787
ACKs for top commit:
apoelstra:
ACK 1f58476cb4
Tree-SHA512: 607132890ed08bf75fb544a0e10aeeda5f9c137eb04349f8af5ab28866408d4208cae4688c08645ffca95e7d9568562dbbbfa992382b5d2cb3efeba583d78b1f
We have a mechanism to run additional custom tests by way of the
`extra_tests.sh` script in each crate.
Remove the CI job and run the schemars test using `extra_tests.sh`. This
patch changes the test coverage because currently the schemars test is
only run with a stable toolchain but with this patch applied it runs
with stable, MSRV, and nightly.
Fix: #2787
Currently we have a trait `Hash` that is required for `Hmac`, `Hkdf`,
and other use cases. However, it is unegonomic for users who just want
to do a simple hash to have to import the trait.
Add inherent functions to all hash types including those created with
the new wrapper type macros.
This patch introduces some duplicate code but we are trying to make
progress in the hashes API re-write. We can come back and de-dublicate
later.
Includes making `to_byte_array`,`from_byte_array`, `as_byte_array`, and
`all_zeros` const where easily possible.
8aa893ebd0 Remove repetition from sha256t_hash_newtype macro (Tobin C. Harding)
Pull request description:
The `sha256t_hash_newtype` macro is hard to reason about because we allow repetition so which tag goes with which type is slightly obscure.
Remove repetition and call the macro three times.
Internal change in `bitcoin`, API change in `hashes`.
Fix#2811
ACKs for top commit:
apoelstra:
ACK 8aa893ebd0 nice, small diff
Tree-SHA512: b38e7c307ac7288b4a5c1c3170ad6aa54c62bd3198922ec8bb091867b230bb9149f7dc996766fc8fa20a1af18b318c475b3e83e2689d322b7f4af0d5cb588e50
The `sha256t_hash_newtype` macro is hard to reason about because we
allow repetition so which tag goes with which type is slightly obscure.
Remove repetition and call the macro three times.
Internal change in `bitcoin`, API change in `hashes`.
The `hash_trait_impls` macro currently adds an impl block for `Hash` -
this is not what the docs say since and `impl Hash` block is nothing
to do with traits.
Move the impl block and add a duplicate of the functions to the
`sha256t::Hash` type.
This is a refactor, no API or logic changes. Note that wrapper types
currently do net get these functions - that will be
discussed/implemented separately.
c9d1ff7037 Update hashes API changes (Nick Johnson)
878ab924d1 Add HMAC Extract-and-Expand Key Derivation Function (Nick Johnson)
Pull request description:
rustaceanrob and I have been working on a Rust-based BIP324 implementation over at https://github.com/rustaceanrob/bip324. We have been attempting to keep the code pretty clean in hopes of a future "soft landing" in rust-bitcoin. I figured the HKDF implementation is a small, self-contained chunk that might allow us to learn the ropes here first.
There was a mention in the [discussion thread on BIP324](https://github.com/rust-bitcoin/rust-bitcoin/discussions/1691) that the hashes interface may be changing in the near future. I am not sure the effect that would have on this implementation, but happy to work through any issues.
Closes#2551
ACKs for top commit:
tcharding:
ACK c9d1ff7037
apoelstra:
ACK c9d1ff7037
Tree-SHA512: 404d51ca055db4366ec57f1503fcf350aebcd181f36a20a17763ea8c47ade851213fc882acd2785313953a3e768d588c230f737ff93f88121b97c34b37c65127
Some of our CI shell scripts are meant only to be sourced and not
run directly however they include an initial shebang line, implying that
they should be run.
Remove the shebang line from `crates.sh` and the various `test_vars.sh`
scripts. Add a `shellcheck` directive to inhibit the no-shebang warning.
Fix: #2764
BIP324's peer to peer encryption protocol requires an HMAC-based extract
and expand key derivation function (HKDF). HKDFs were not part of many
bitcoin protocols before BIP324, but the hope is that the encrypted
protocol becomes the dominant standard justifying this implementation.
30a482504b bump nightly-version (Andrew Poelstra)
5ad7c245e3 cargo: whitelist all cfgs used in this repo (Andrew Poelstra)
814786b0a6 crypto: enable and fix accidentally disabled unit test (Andrew Poelstra)
Pull request description:
https://github.com/rust-lang/rust/issues/124800 has been fixed and we can update our nightly version by whitelisting all cfgs that are used.
There was one place where we had an old `cfg(feature = "no-std")` despite having removed the feature. By removing that cfg check we re-enabled a previously disabled test.
ACKs for top commit:
tcharding:
ACK 30a482504b
Tree-SHA512: d25bed819091db74b9d47cb2c23caa3ceb0d7be323b37831326e2ec1608cb1577d41aad2e1cdf59d66df69397537bc3e17a3c2872935d5a4f46f4dc55b5e613c
4446be6fc8 hashes: Add regression tests (Tobin C. Harding)
Pull request description:
We have regression tests spread out throughout the `hashes` module but they are not labelled as such. To give us more confidence and help debug when patching the `hashes` crate we can add a bunch of regression tests in a single place.
Add a module that does a single regression test for each type, simply hash some arbitrary data and check the hex display against a hard coded hex string.
ACKs for top commit:
apoelstra:
ACK 4446be6fc8 nice :)
Tree-SHA512: db643fdf799d23735934ad966ee66b40b3adb72db8f777112005f94d61ab4e382399a66e48d9fc9fdb0cd0abe17b3d4087fe6e6c494836d0c6fc713f4efc6413
We have regression tests spread out throughout the `hashes` module but
they are not labelled as such. To give us more confidence and help
debug when patching the `hashes` crate we can add a bunch of regression
tests in a single place.
Add a module that does a single regression test for each type, simply
hash some arbitrary data and check the hex display against a hard coded
hex string.
021bea89bb ci: shellcheck checks (Jose Storopoli)
Pull request description:
Closes#2739.
I am proposing that we use this GitHub Shellcheck action:
[`ludeeus/action-shellcheck`](https://github.com/ludeeus/action-shellcheck)
since it has most stars (and eyes on it).
I also did all fixes that I could find with
```bash
shellcheck **/*.sh
```
If I've missed any please let me know.
ACKs for top commit:
apoelstra:
ACK 021bea89bb
tcharding:
ACK 021bea89bb
Tree-SHA512: 67e37da9ae3ea0c5551af57b928016a2d9e76761af5558b3057ac47e773189629dd20eea9e659b4323c8568fb48dcdbe9ebd5c730f2c6266fb0db52886c9835f
6def5bc974 CI: Use run_task from maintainer tools (Tobin C. Harding)
c5af52847b CI: Add docs and document start (Tobin C. Harding)
62ba10503a CI: Use correct spacing (Tobin C. Harding)
0c0e88165e CI: Add README file (Tobin C. Harding)
44cb2255d3 CI: Add sanitizer script (Tobin C. Harding)
3407257936 CI: Add WASM script (Tobin C. Harding)
cc14edf63f CI: Run the schemars job directly using cargo (Tobin C. Harding)
1fb12e1917 CI: Remove recent from schemars job (Tobin C. Harding)
8d7117bb0e CI: Use original name (Tobin C. Harding)
Pull request description:
First we do some clean up, then we pull the stuff that is specific to this repo out into separate tests, then as the last patch we switch over to use the new script for `rust-bitcoin-maintainer-tools` that was introduced in https://github.com/rust-bitcoin/rust-bitcoin-maintainer-tools/pull/4.
ACKs for top commit:
apoelstra:
ACK 6def5bc974
Tree-SHA512: 70105d455294d49deb43461958435096d074d19ea8d9adc7036e15d74dfdab466f04b1d9e934574077ca0c32a6fe77d0e5aa11068d8c4d51acef634591227d33
As we did for the wasm job.
In preparation for using the `run_task` script from maintainer tools we
want to have all the things that are particular to `rust-bitcoin` out of
the current `run_task` script.
The address/memory sanitizer test is specific to `hashes`. Add a script
in `hashes/contrib` and call it from the ASAN job.
No test coverage change.
In preparation for using the `run_task` script from maintainer tools we
want to have all the things that are particular to `rust-bitcoin` out of
the current `run_task` script.
The wasm test is specific to `hashes`. Add a script in `hashes/contrib`
and call it from the wasm job.
No test coverage change.
7685461e62 Document the sha256t_hash_newtype direction (Tobin C. Harding)
30e91cc766 Default to forward for tagged hashes (Tobin C. Harding)
5ecc69cd28 Add forward/backward unit test (Tobin C. Harding)
9aee65d1ba Refactor tagged hash tests (Tobin C. Harding)
216422dffc Remove schemars impl for test type (Tobin C. Harding)
Pull request description:
First three patches are preparation, improvements to the units tests in `sha256t`.
From the final patch:
Displaying backward is an anomaly of Bitcoin Core's early days and the
double SHA256 hash type. We should not let this unfortunate beast leak
out into other places.
Default to displaying forward when creating a new tagged hash and remove
all the explicit attributes from `bitcoin` that just clutter the code.
This is an API break and may quietly break some users downstream - eventually we should stop doing that sort of thing.
ACKs for top commit:
apoelstra:
ACK 7685461e62
Tree-SHA512: cb8a41b207aa68ecf63cb7af7f39f7d7c8a3a27f38595867949b288a81a20bff0c17aa4c17bb099e2ecf85194d83bad23c9c9792f511b6c4cd625ff27c1affaa
Since the default display direction is now forward, use
`#[hash_newtype(backward)]`
in the rustdocs on the macro. Also add an example usage to the changelog
in case someone downstream is relying on the old default behaviour of
displaying backwards (unlikely).
Currently we require indexing trait bounds as well as `Borrow` on the
`Hash` trait. We also already implement `AsRef`.
It was observed that `Borrow<[u8]>` does not best describe what we want
from the `Hash` trait implementor but rather `AsRef<[u8]>` does.
Remove all the inexing trait bounds. Remove the `borrow::Borrow<[u8]>`
trait bound. Add a `convert::AsRef<[u8]>` trait bound.
This leaves the `Borrow<[u8]>` implementation for hashes created with
`hash_newtype`, I'm not sure if this should be removed or not.
71bb86232b hashes: Do not import str (Tobin C. Harding)
Pull request description:
Depending on things being in scope for macros to use is bad form, using the fully qualified path is the correct way.
Do not import `str` instead use the fully qualified path to the `core` re-export.
Use fully qualified path instead.
ACKs for top commit:
apoelstra:
ACK 71bb86232b trivial rebase
sanket1729:
ACK 71bb86232b
Tree-SHA512: 401520a5876b83ad4053bbe9b1e8cd9ff2e723cf86f95e47891a6411ad5e9af4f904e19ccaaab80d342dfe4745753c24af168dcbc8170fb6b39da08e577d30ae
26b9782d8b CI: Re-write run_task.sh (Tobin C. Harding)
Pull request description:
Recently we re-wrote CI to increase VM level parallelism, in hindsite this has proved to be not that great because:
- It resulted in approx 180 jobs
- We are on free tier so only get 20 jobs (VMs) at a time so its slow to run
- The UI is annoying to dig through the long job list to find failures
Have another go at organising the jobs with the main aim of shortening total run time and making it easier to quickly see fails.
Re-write the `run_task.sh` script, notable moving manifest handling to the workflow. Also don't bother testing with beta toolchain.
### Note on review
The diff is hard to read for `rust.yml`, I tried splitting out a bunch of separate patches but it resulted in the same thing (because there are so many identical lines in the yaml file). I suggest just looking at the yaml file and not the diff.
ACKs for top commit:
apoelstra:
ACK 26b9782d8b
sanket1729:
ACK 26b9782d8b.
Tree-SHA512: 1b0a0bab5cf729c5890f7150953499b42aebd3b1c31a1b0d3dfa5b5e78fda11e17a62a2df6b610ab4a950d5709f3af6fff1ae64d9e67379338903497ab77ae0e
Recently we re-wrote CI to increase VM level parallelism, in hindsite
this has proved to be not that great because:
- It resulted in approx 180 jobs
- We are on free tier so only get 20 jobs (VMs) at a time so its slow to run
- The UI is annoying to dig through the long job list to find failures
Have another go at organising the jobs with the main aim of shortening
total run time and making it easier to quickly see fails.
Re-write the `run_task.sh` script, notable moving manifest handling
to the workflow. Also don't bother testing with beta toolchain.
WASM Note
Removes the `cdylib` and `rlib` from the manifest patching during wasm
build - I do not know the following:
- Why this breaks on this PR but not on other PRs
- Why I can't get wasm test to run locally on master but PRs are passing
- What the `cdylib` and `rlib` were meant to be doing
This is the docs from: https://doc.rust-lang.org/reference/linkage.html
* --crate-type=cdylib, #![crate_type = "cdylib"] - A dynamic system
library will be produced. This is used when compiling a dynamic library
to be loaded from another language. This output type will create *.so
files on Linux, *.dylib files on macOS, and *.dll files on Windows.
* --crate-type=rlib, #![crate_type = "rlib"] - A "Rust library" file
will be produced. This is used as an intermediate artifact and can be
thought of as a "static Rust library". These rlib files, unlike
staticlib files, are interpreted by the compiler in future linkage. This
essentially means that rustc will look for metadata in rlib files like
it looks for metadata in dynamic libraries. This form of output is used
to produce statically linked executables as well as staticlib outputs.
Displaying backward is an anomaly of Bitcoin Core's early days and the
double SHA256 hash type. We should not let this unfortunate beast leak
out into other places.
Default to displaying forward when creating a new tagged hash and remove
all the explicit attributes from `bitcoin` that just clutter the code.
In the tagged hash unit tests we are testing two separate things in a
single test. To improve maintainability separate the test into two.
Refactor only, no test coverage change.
We do not test the schemars stuff in `hashes`, instead we do it in a
separate crate `extended_tests/schemars`. There is therefore no reason
to implement `schemars::JsonSchema` for the `TestHashTag`.
Depending on things being in scope for macros to use is bad form,
using the fully qualified path is the correct way.
Do not import `str` instead use the fully qualified path to the `core`
re-export.
Use fully qualified instead.
This lint triggers when parsing a reference to a large struct as a
generic argument, which is wrong.
Allow it crate wide because [subjectively] this lint never warns for
anything useful.
In preparation for release add a changlelog entry and bump the version.
I'm not 100% sure that this release is API breaking, dependencies
definitely changed. The rest might be only additives but I didn't bother
looking exactly because I think its better to bump the minor version and
err on the side of caution.
Note the hashes 0.13.0 dependency stays in the dependency graph because
of secp, we can update secp after releasing `hashes` then update the
secp dependency in `rust-bitcoin` thereby removing the `hashes v0.13.0`
dependency - phew.
56132f59d5 Remove the `:#` formatting for `hex_fmt_impl` macro (448 OG)
Pull request description:
This commit attempts to solve #2505 by ensuring that formatting is not forced using the `:#` in the hex macro code generating in macro rule `hex_fmt_impl` in the hashes/utils.rs file.
The write! macro forces all formatting to add the prefix `0x` by adding an alternate by (#) default
```rust
impl<$($gen: $gent),*> $crate::_export::_core::fmt::Debug for $ty<$($gen),*> {
#[inline]
fn fmt(&self, f: &mut $crate::_export::_core::fmt::Formatter) -> $crate::_export::_core::fmt::Result {
write!(f, "{:#}", self) // <-- This is where the formatting is being forced.
}
}
```
By removing this formatting, the `:#` must be specified by the user in order for a prefix to be added.
```rust
let outpoint = bitcoin::OutPoint::default();
println!("{:?}", &outpoint);
println!("{:#?}", &outpoint);
println!("{:#}", &outpoint);
println!("{:x}", &outpoint.txid);
// `{:#}` must be specified to pretty print with a prefix
println!("{:#}", &outpoint.txid);
dbg!(&outpoint);
dbg!(&outpoint.txid);
```
The PR also adds testcase for this when running `cargo test` .
ACKs for top commit:
tcharding:
ACK 56132f59d5
apoelstra:
ACK 56132f59d5
Tree-SHA512: 9e4fc9f30ab0b3cf2651d3c09f7f01d8245ac8ea7ae3a82bb4efd19f25c77662bf279020a31fa61b37587cc0c74284696c56045c59f1ba63b2dd42a210d98ebc
4e3bb7350a Add support for SHA-384 (Matt Corallo)
Pull request description:
Based on #2473 as we need support for 48-byte arrays <-> hex conversions.
Closes#2483
ACKs for top commit:
Kixunil:
ACK 4e3bb7350a
sanket1729:
ACK 4e3bb7350a
Tree-SHA512: e78d97f80ab8afda8a3ea240023338f17f7e95604a879b38fc9bde057fbb45b74b1f3fb3bd2b17af89682b79dda42bf114989e7c63066b3029451ef07894e82f
f8de7954b2 Remove unused pow::TryFromError type (Tobin C. Harding)
43c5eb765c Fix witness_version leaf error type (Tobin C. Harding)
2af764e859 hashes: Fix leaf error type (Tobin C. Harding)
Pull request description:
In light of recent discussion go over the codebase and look for some places that the leaf errors are wrong. Does not do the whole code base, excludes `p2p` and a couple of other places.
ACKs for top commit:
apoelstra:
ACK f8de7954b2
Kixunil:
ACK f8de7954b2
Tree-SHA512: 2905878363869ee205cce49c58c060c712c9b7b55965ee60bb856128842968a4be86c93a194ffffdb35e215b2bea8ad33b04ee47e8e17cc784b0641ea48518e5
This fixes the issue where pretty debug like `dbg` or `{:#}` introduce the use of
`0x` prefix to hex encoded transaction ID.
The transaction id is being forced to pretty print inside the `hex_fmt_impl` macro
using `{:#}` in the line `write!(f, "{:#}", self)` debug formatter.
Resolves: #2505
Currently we have two functions for displaying forwards and backwards and
we also have `fmt_hex_exact`. I do not know why we added the functions.
In the latest version of hex we do not have the ability to construct a
`DisplayArray` type so we have to use `fmt_hex_exact`.
Make the change now, separate from the `hex` upgrade, to assist review.
Different hashes output to hex strings differently depending on whether
they display backward or not but we are not currently testing that our
parsing and formatting impls both correctly handle backwards/forwards.
Add unit tests to roundtrip through a hex string, do so for one forwards
printing hash (sha256), on backwards printing hash (sha256d), and also
test that the `hash_newtype!` macro correctly passes on display backward.
We attempted to release with the current 0.1.0 version forgetting that
we had previously released an empty crate with that version to reserve
the name on crates.io.
Bump the version to 0.1.1 and release the actual code.
Use `bash` instead of `sh` to run shell scripts.
We would like to support Nix users who do not typically have any shell
other than `sh` at a known path, therefore use `/usr/bin/env bash`.
Make the trait level attributes uniform across all released crates in
the repo. Excludes things that are obviously not needed, eg, bench stuff
if there is not bench code.
- Remove `uninhabited_references` - this is allow by default now.
- Remove `unconditional_recursion` and mark the single false positive we
have with an `allow`.
Note, this does not add `missing_docs` to the `io` crate. There is an
open PR at the moment to add that along with the required docs.
94a6caf204 hashes: Add Hash::from_byte_chunks to construct hashes from iterators (Steven Roose)
Pull request description:
Is there any interest in a method like this? I'm not necessarily a fan myself, was just in a situation where I was doing this a lot and it would save the me the "engine, from_engine dance" each time.
ACKs for top commit:
Kixunil:
ACK 94a6caf204
apoelstra:
ACK 94a6caf204
Tree-SHA512: 96e74366dbf64d50fb6642024d18eeeee5b78154fa152c845a1073c904599af69328a141f0a7e2bd1a7b43a091c886da57dfe6e87bab5a5e01b0f167019caae0
Its not immediately obvious why we nest the whole `io` code in an `io`
submodule within `lib.rs`. As far as I can tell we can inline it and
re-export from `rust-bitcoin` same as we do for our other dependencies.
This change would effect other users of the crate but since the `io`
crate is unreleased this effects no-one except us.
Rust version 1.56.0 introduced edition 2021. Shortly afterwards, on
October 21 2021 Rust version 1.56.1 was released.
Debian stable is currently shipping `rustc 1.63.0`.
Our stated MSRV policy is: In Debian stable and at least 2 years old.
Therefore our MSRV policy is met by Rust version 1.56.1 and we can strat
to bump our MSRV org wide.
Start by bumping the `rust-bitcoin` and `hashes` MSRV to Rust 1.56.1,
includes:
- Update docs.
- Update CI and remove pinning.
- Update the build files and remove now stale cfg attributes rust_v_1_x
for values less than the new MSRV.
- Use new `IntoIterator` for arrays so we no longer need to allocate a
vector to iterate.
Links:
- https://blog.rust-lang.org/2021/11/01/Rust-1.56.1.html
- https://blog.rust-lang.org/2021/10/21/Rust-1.56.0.html
- https://packages.debian.org/stable/rust/rustc
We are trying to get rid of the `serde_derive` dependency from our
dependency graph.
Stop using default features for the `schemars` dependency which includes
`schemars_derive` which depends on `serder_derive`.
Manually implement `schemars::JsonSchema` instead of deriving it.
With the new `bitcoin_io` library, implementing `io::Write`
manually is somewhat tricky - for `std` users we really want to
provide an `std::io::Write` implementation, however for `no-std`
users we want to implement against our internal trait.
Sadly we cannot provide a blanket implementation of
`std::io::Write` for all types whcih implement our `io::Write`
trait as its an out-of-crate impl.
Instead, we provide a macro which will either implement
`std::io::Write` or our `io::Write` depending on the feature flags
set on `bitcoin_io`.
In order to support standard (de)serialization of structs, the
`rust-bitcoin` ecosystem uses the standard `std::io::{Read,Write}`
traits. This works great for environments with `std`, however sadly
the `std::io` module has not yet been added to the `core` crate.
Thus, in `no-std`, the `rust-bitcoin` ecosystem has historically
used the `core2` crate to provide copies of the `std::io` module
without any major dependencies. Sadly, its one dependency,
`memchr`, recently broke our MSRV.
Worse, because we didn't want to take on any excess dependencies
for `std` builds, `rust-bitcoin` has had to have
mutually-exclusive `std` and `no-std` builds. This breaks general
assumptions about how features work in Rust, causing substantial
pain for applications far downstream of `rust-bitcoin` crates.
Here, we add a new `bitcoin_io` crate, making it an unconditional
dependency and using its `io` module in the in-repository crates
in place of `std::io` and `core2::io`. As it is not substantial
additional code, the `hashes` io implementations are no longer
feature-gated.
This doesn't actually accomplish anything on its own, only adding
the new crate which still depends on `core2`.
71454d438a Use hash_type macro for sha512::Hash (Tobin C. Harding)
d51c1857fd sha512: Move from_engine functions (Tobin C. Harding)
Pull request description:
This PR does not introduce any logic changes.
- Patch 1 is a code move.
- Patch 2 uses the `hash_type!` macro to define the `sha512::Hash` type.
ACKs for top commit:
Kixunil:
ACK 71454d438a
apoelstra:
ACK 71454d438a
Tree-SHA512: c0148391fbea3602f0c52e5e08883faac6b5cb3f75ae62bfb53b1d2762e28871d349b3d3fb589f59d8bf2912dd63934f8f9a6fe6390afd378e5391eb2c91c237
Make the `sha512_256` module use similar code layout to the other hash
modules by putting the call to `hash_type` at the top followed by the
`from_engine` function.
Code move only, no other changes.
Currently we are defining the `sha512::Hash` type manually instead of
using the `hash_type` macro.
The generated code using the macro is identical to that without
it (although I didn't actually look at the generated code :)
685e5101ce Add wasm dev-deps using CI script (Tobin C. Harding)
Pull request description:
We only test WASM in CI using a stable toolchain however because we have a target specific dev-dependencies section the wasm deps get pulled in during MSRV builds - this breaks the MSRV build.
Instead of including WASM dev-dependencies in the manifest we can dynamically modify the manifest when running the WASM tests. We do this already to add the `crate-type` section so this is not really that surprising to see in the CI script.
Doing so allows us to stop pinning the transitive `syn` dependency also which is included in the dependency graph because of `wasm-bingen-test`.
Originally part of #2093
ACKs for top commit:
apoelstra:
ACK 685e5101ce
sanket1729:
ACK 685e5101ce. Removing wasm from lock-files is a win
Tree-SHA512: ad607a8a8af329e9830d9284a95a2540bf15a2ceda826b8d1c0312ee094f1f5f54599ee2faa6b611439997c349bdd9a29e6d5ae447bff96a9a538753434e2248
We only test WASM in CI using a stable toolchain however because we have
a target specific dev-dependencies section the wasm deps get pulled in
during MSRV builds - this breaks the MSRV build.
Instead of including WASM dev-dependencies in the manifest we can
dynamically modify the manifest when running the WASM tests. We do this
already to add the `crate-type` section so this is not really that
surprising to see in the CI script.
Doing so allows us to stop pinning the transitive `syn` dependency also
which is included in the dependency graph because of `wasm-bingen-test`.
On our way to v1.0.0 we are defining a standard for our error types,
this includes:
- Uses the following derives (unless not possible, usually because of `io::Error`)
`#[derive(Debug, Clone, PartialEq, Eq)]`
- Has `non_exhaustive` unless we really know we can commit to not adding
anything.
Furthermore, we are trying to make the codebase easy to read. Error code
is write-once-read-many (well it should be) so if we make all the error
code super uniform the users can flick to an error and quickly see what
it includes. In an effort to achieve this I have made up a style and
over recent times have change much of the error code to that new style,
this PR audits _all_ error types in the code base and enforces the
style, specifically:
- Is layed out: definition, [impl block], Display impl, error::Error impl, From impls
- `error::Error` impl matches on enum even if it returns `None` for all variants
- Display/Error impls import enum variants locally
- match uses *self and `ref e`
- error::Error variants that return `Some` come first, `None` after
Re: non_exhaustive
To make dev and review easier I have added `non_exhaustive` to _every_
error type. We can then remove it error by error as we see fit. This is
because it takes a bit of thinking to do and review where as this patch
should not take much brain power to review.
f2c5f19557 Introduce the `small-hash` feature for `bitcoin_hashes` (Alekos Filini)
Pull request description:
When enabled this feature swaps the hash implementation of sha512, sha256 and ripemd160 for a smaller (but also slower) one.
On embedded processors (Cortex-M4) it can lead to up to a 52% size reduction, from around 37KiB for just the `process_block` methods of the three hash functions to 17.8KiB.
The following numbers were collected on `aarch64-unknown-linux-gnu` with `cargo 1.72.0-nightly`.
## Original
```
RUSTFLAGS='--cfg=bench -C opt-level=z' cargo bench
```
```
test hash160::benches::hash160_10 ... bench: 33 ns/iter (+/- 1) = 303 MB/s
test hash160::benches::hash160_1k ... bench: 2,953 ns/iter (+/- 187) = 346 MB/s
test hash160::benches::hash160_64k ... bench: 188,480 ns/iter (+/- 11,595) = 347 MB/s
test hmac::benches::hmac_sha256_10 ... bench: 33 ns/iter (+/- 2) = 303 MB/s
test hmac::benches::hmac_sha256_1k ... bench: 2,957 ns/iter (+/- 104) = 346 MB/s
test hmac::benches::hmac_sha256_64k ... bench: 192,022 ns/iter (+/- 6,407) = 341 MB/s
test ripemd160::benches::ripemd160_10 ... bench: 25 ns/iter (+/- 1) = 400 MB/s
test ripemd160::benches::ripemd160_1k ... bench: 2,288 ns/iter (+/- 93) = 447 MB/s
test ripemd160::benches::ripemd160_64k ... bench: 146,823 ns/iter (+/- 1,102) = 446 MB/s
test sha1::benches::sha1_10 ... bench: 41 ns/iter (+/- 0) = 243 MB/s
test sha1::benches::sha1_1k ... bench: 3,844 ns/iter (+/- 70) = 266 MB/s
test sha1::benches::sha1_64k ... bench: 245,854 ns/iter (+/- 10,158) = 266 MB/s
test sha256::benches::sha256_10 ... bench: 35 ns/iter (+/- 0) = 285 MB/s
test sha256::benches::sha256_1k ... bench: 3,063 ns/iter (+/- 15) = 334 MB/s
test sha256::benches::sha256_64k ... bench: 195,729 ns/iter (+/- 2,880) = 334 MB/s
test sha256d::benches::sha256d_10 ... bench: 34 ns/iter (+/- 1) = 294 MB/s
test sha256d::benches::sha256d_1k ... bench: 3,071 ns/iter (+/- 107) = 333 MB/s
test sha256d::benches::sha256d_64k ... bench: 188,614 ns/iter (+/- 8,101) = 347 MB/s
test sha512::benches::sha512_10 ... bench: 21 ns/iter (+/- 0) = 476 MB/s
test sha512::benches::sha512_1k ... bench: 1,714 ns/iter (+/- 36) = 597 MB/s
test sha512::benches::sha512_64k ... bench: 110,084 ns/iter (+/- 3,637) = 595 MB/s
test sha512_256::benches::sha512_256_10 ... bench: 22 ns/iter (+/- 1) = 454 MB/s
test sha512_256::benches::sha512_256_1k ... bench: 1,822 ns/iter (+/- 70) = 562 MB/s
test sha512_256::benches::sha512_256_64k ... bench: 116,231 ns/iter (+/- 4,745) = 563 MB/s
test siphash24::benches::siphash24_1ki ... bench: 1,072 ns/iter (+/- 41) = 955 MB/s
test siphash24::benches::siphash24_1ki_hash ... bench: 1,102 ns/iter (+/- 42) = 929 MB/s
test siphash24::benches::siphash24_1ki_hash_u64 ... bench: 1,064 ns/iter (+/- 41) = 962 MB/s
test siphash24::benches::siphash24_64ki ... bench: 69,957 ns/iter (+/- 2,712) = 936 MB/
```
```
0000000000005872 t _ZN84_$LT$bitcoin_hashes..ripemd160..HashEngine$u20$as$u20$bitcoin_hashes..HashEngine$GT$5input17hc4800746a9da7ff4E
0000000000007956 t _ZN81_$LT$bitcoin_hashes..sha256..HashEngine$u20$as$u20$bitcoin_hashes..HashEngine$GT$5input17hf49345f65130ce9bE
0000000000008024 t _ZN14bitcoin_hashes6sha2568Midstate10const_hash17h57317bc8012004b4E.llvm.441255102889972912
0000000000010528 t _ZN81_$LT$bitcoin_hashes..sha512..HashEngine$u20$as$u20$bitcoin_hashes..HashEngine$GT$5input17h9bc868d4392bd9acE
```
Total size: 32380 bytes
## With `small-hash` enabled
```
RUSTFLAGS='--cfg=bench -C opt-level=z' cargo bench --features small-hash
```
```
test hash160::benches::hash160_10 ... bench: 52 ns/iter (+/- 3) = 192 MB/s
test hash160::benches::hash160_1k ... bench: 4,817 ns/iter (+/- 286) = 212 MB/s
test hash160::benches::hash160_64k ... bench: 319,572 ns/iter (+/- 11,031) = 205 MB/s
test hmac::benches::hmac_sha256_10 ... bench: 54 ns/iter (+/- 2) = 185 MB/s
test hmac::benches::hmac_sha256_1k ... bench: 4,846 ns/iter (+/- 204) = 211 MB/s
test hmac::benches::hmac_sha256_64k ... bench: 319,114 ns/iter (+/- 4,451) = 205 MB/s
test ripemd160::benches::ripemd160_10 ... bench: 27 ns/iter (+/- 0) = 370 MB/s
test ripemd160::benches::ripemd160_1k ... bench: 2,358 ns/iter (+/- 150) = 434 MB/s
test ripemd160::benches::ripemd160_64k ... bench: 154,573 ns/iter (+/- 3,954) = 423 MB/s
test sha1::benches::sha1_10 ... bench: 41 ns/iter (+/- 1) = 243 MB/s
test sha1::benches::sha1_1k ... bench: 3,700 ns/iter (+/- 243) = 276 MB/s
test sha1::benches::sha1_64k ... bench: 231,039 ns/iter (+/- 13,989) = 283 MB/s
test sha256::benches::sha256_10 ... bench: 51 ns/iter (+/- 3) = 196 MB/s
test sha256::benches::sha256_1k ... bench: 4,823 ns/iter (+/- 182) = 212 MB/s
test sha256::benches::sha256_64k ... bench: 299,960 ns/iter (+/- 17,545) = 218 MB/s
test sha256d::benches::sha256d_10 ... bench: 52 ns/iter (+/- 2) = 192 MB/s
test sha256d::benches::sha256d_1k ... bench: 4,827 ns/iter (+/- 323) = 212 MB/s
test sha256d::benches::sha256d_64k ... bench: 302,844 ns/iter (+/- 15,796) = 216 MB/s
test sha512::benches::sha512_10 ... bench: 34 ns/iter (+/- 1) = 294 MB/s
test sha512::benches::sha512_1k ... bench: 3,002 ns/iter (+/- 123) = 341 MB/s
test sha512::benches::sha512_64k ... bench: 189,767 ns/iter (+/- 10,396) = 345 MB/s
test sha512_256::benches::sha512_256_10 ... bench: 34 ns/iter (+/- 1) = 294 MB/s
test sha512_256::benches::sha512_256_1k ... bench: 2,996 ns/iter (+/- 198) = 341 MB/s
test sha512_256::benches::sha512_256_64k ... bench: 192,024 ns/iter (+/- 8,181) = 341 MB/s
test siphash24::benches::siphash24_1ki ... bench: 1,081 ns/iter (+/- 65) = 947 MB/s
test siphash24::benches::siphash24_1ki_hash ... bench: 1,083 ns/iter (+/- 63) = 945 MB/s
test siphash24::benches::siphash24_1ki_hash_u64 ... bench: 1,084 ns/iter (+/- 63) = 944 MB/s
test siphash24::benches::siphash24_64ki ... bench: 67,237 ns/iter (+/- 4,185) = 974 MB/s
```
```
0000000000005384 t _ZN81_$LT$bitcoin_hashes..sha256..HashEngine$u20$as$u20$bitcoin_hashes..HashEngine$GT$5input17hae341658cf9b880bE
0000000000005608 t _ZN14bitcoin_hashes9ripemd16010HashEngine13process_block17h3276b13f1e9feef8E.llvm.13618235596061801146
0000000000005616 t _ZN14bitcoin_hashes6sha2568Midstate10const_hash17h3e6fbef64c15ee00E.llvm.7326223909590351031
0000000000005944 t _ZN81_$LT$bitcoin_hashes..sha512..HashEngine$u20$as$u20$bitcoin_hashes..HashEngine$GT$5input17h321a237bfbe5c0bbE
```
Total size: 22552 bytes
## Conclusion
On `aarch64` there's overall a ~30% improvement in size, although ripemd160 doesn't really shrink that much (and its performance also aren't impacted much with only a 6% slowdown). sha512 and sha256 instead are almost 40% slower with `small-hash` enabled.
I don't have performance numbers for other architectures, but in terms of size there was an even larger improvements on `thumbv7em-none-eabihf`, with a 52% size reduction overall:
```
Size Crate Name
25.3KiB bitcoin_hashes <bitcoin_hashes[fe467ef2aa3a1470]::sha512::HashEngine as bitcoin_hashes[fe467ef2aa3a1470]::HashEngine>::input
6.9KiB bitcoin_hashes <bitcoin_hashes[fe467ef2aa3a1470]::sha256::HashEngine as bitcoin_hashes[fe467ef2aa3a1470]::HashEngine>::input
4.8KiB bitcoin_hashes <bitcoin_hashes[fe467ef2aa3a1470]::ripemd160::HashEngine as bitcoin_hashes[fe467ef2aa3a1470]::HashEngine>::input
```
vs
```
Size Crate Name
9.5KiB bitcoin_hashes <bitcoin_hashes[974bb476ef905797]::sha512::HashEngine as bitcoin_hashes[974bb476ef905797]::HashEngine>::input
4.5KiB bitcoin_hashes <bitcoin_hashes[974bb476ef905797]::ripemd160::HashEngine>::process_block
3.8KiB bitcoin_hashes <bitcoin_hashes[974bb476ef905797]::sha256::HashEngine as bitcoin_hashes[974bb476ef905797]::HashEngine>::input
```
I'm assuming this is because on more limited architectures the compiler needs to use more instructions to move data in and out of registers (especially for sha512 which ideally would benefit from 64-bit registers), so reusing the code by moving it into functions saves a lot of those instructions.
Also note that the `const_hash` method on `sha256` causes the compiler to emit two independent implementations. I haven't looked into the code yet, maybe there's a way to merge them so that the non-const `process_block` calls into the const fn.
-----
Note: commits are unverified right now because I don't have the keys available, I will sign them after addressing the review comments.
ACKs for top commit:
apoelstra:
ACK f2c5f19557
tcharding:
ACK f2c5f19557
Tree-SHA512: 1d5eb56324c458660e2571e8cf59895dc31dae9c5427c7ed36f8a0e81ca2e9a0f39026f56b6803df03635cc8b66aee3bf5182d51ab8972d169d56bcfec33771c
546c0122d7 Add simd sha256 intrinsics for x86 machines (sanket1729)
Pull request description:
This is my first time dabbling into architecture specific code and simd. The algorithm is a word to word translation of the C code from 4899efc81d/sha256-x86.c .
Some benchmarks:
With simd
```
test sha256::benches::sha256_10 ... bench: 11 ns/iter (+/- 0) = 909 MB/s
test sha256::benches::sha256_1k ... bench: 712 ns/iter (+/- 2) = 1438 MB/s
test sha256::benches::sha256_64k ... bench: 45,597 ns/iter (+/- 189) = 1437 MB/s
```
Without simd
```
test sha256::benches::sha256_10 ... bench: 47 ns/iter (+/- 0) = 212 MB/s
test sha256::benches::sha256_1k ... bench: 4,243 ns/iter (+/- 17) = 241 MB/s
test sha256::benches::sha256_64k ... bench: 271,263 ns/iter (+/- 1,610) = 241 MB/s
```
ACKs for top commit:
apoelstra:
ACK 546c0122d7
tcharding:
ACK 546c0122d7
Tree-SHA512: 7167c900b77e63cf38135a3960cf9ac2615f73b2ef7020a12b5cc3f4c047910063ba9045217b9ecfa70f7de1eb0f02f2674f291bd023a853bad2b9162fae831e
When enabled this feature swaps the hash implementation of sha512,
sha256 and ripemd160 for a smaller (but also slower) one.
On embedded processors (Cortex-M4) it can lead to up to a 52% size
reduction, from around 37KiB for just the `process_block` methods of the
three hash functions to 17.8KiB.
154552e334 docs: Do not link to std::option::Option (Tobin C. Harding)
24843468c3 Remove rustdocs links to serde (Tobin C. Harding)
Pull request description:
Two minor patches to fix up docs links. These were originally done as part of #1880 but are unrelated so pushing them up separately.
ACKs for top commit:
apoelstra:
ACK 154552e334
RCasatta:
utACK 154552e334
Tree-SHA512: e45e1538c66b59d63a66898896927bb6c1336fb4c8515bb9e2204c8035870ef8e4a6fd32dfc83db2938afda67feb27c48989e382410f9e7ea7a967132941c720
We have just released the `hex-conservative` crate, we can now use it.
Do the following:
- Depend on `hex-conservative` in `bitcoin` and `hashes`
- Re-export `hex-conservative` as `hex` from both crate roots.
- Remove all the old hex code from `hashes`
- Fix all the import statements (makes up the bulk of the lines changed
in this patch)
Not totally necessary but since I went to the trouble of working out the
last working version add it to the docs so the next guy can grep for
`cargo update` to find them.
To help folk work out how to run the `hashes/embedded` test crate copy
over the `script` directory and an updated version of the `README` from
`embedded/bitcoin`.
In order to keep the embedded and schemacs test crates building when we
update their local transient dependencies we need to use a `patch`
section.
- For `bitcoin/embedded` add `patch` section for `internals`, `hashes`
already has an entry.
- For `hashes/embedded` add `patch` section for `internals`.
- For `hashes/extendend_tests/schemars` add `patch` section for
`internals`.
FTR for direct local dependencies we use a `path` field when specifying
the dependency.
We use two different methods for specifying local dependencies, `patch`
and also `path`. There does not seem to be a reason why we use both,
lets be uniform. Elect to use `patch` for all local crates.
8813a63ec9 internals: Bump version to 0.2.0 (Tobin C. Harding)
Pull request description:
In preparation for release bump the version and add a changelog entry. Includes updating the dependency in `bitcoin` and `hashes`.
ACKs for top commit:
apoelstra:
ACK 8813a63ec9
sanket1729:
utACK 8813a63ec9
Tree-SHA512: a9bd9d4d69cba21329f3f63a9948afe566bb97c8c65f5d46c329a696a814e9eb31372d378de1ecf0f43f0cb42f11d53dc51bc467223b34629e61315d48b39a29
In preparation for release bump the version and add a changelog entry.
Includes updating the dependency in `bitcoin` and `hashes` as well as
the minimal/recent lock files.
Recently we made the hash engine fields pub crate so that `sha512_256`
could construct a hash engine with different constants. We can make the
code slightly cleaner by adding a pub crate constructor and making the
fields private again.
Idea from Kixunil:
https://github.com/rust-bitcoin/rust-bitcoin/pull/1413#pullrequestreview-1197207593
aa7b3a7d0c ci: Remove stal DO_ALLOC_TESTS variable (Tobin C. Harding)
Pull request description:
We never enable the `DO_ALLOC_TESTS` variable, and hence never test the "alloc" feature in `hashes`.
Remove the `DO_ALLOC_TESTS` variable and add "alloc" to the `FEATURES` array.
ACKs for top commit:
yancyribbens:
ACK aa7b3a7d0c
apoelstra:
ACK aa7b3a7d0c
sanket1729:
utACK aa7b3a7d0c
Tree-SHA512: 461735242ec94786624a3653c3ea108d1a8712d5a77943f2ce54b44298624f6d4a43c98a0079782211e9a4b61ac87fbadd9c98608ca0ad82574c4a63b921776f
4dfaec952c hashes: Remove stale status badge (Tobin C. Harding)
Pull request description:
We do not use travis in our CI pipeline anymore, remove the stale badge.
ACKs for top commit:
yancyribbens:
ACK 4dfaec952c
apoelstra:
ACK 4dfaec952c
sanket1729:
utACK 4dfaec952c
Tree-SHA512: 6865c8cc51f90161ea643922df3bbf890811777e3dcbff9e464d6092f0d79a6ae2798d593f6a27e7f18b05ba66a765c871d4d01086e18b8480916573288afc0e
06afd52a12 Improve hashes::Error (Tobin C. Harding)
Pull request description:
We are trying to make error types stable on the way to v1.0
The current `hashes::Error` is a "general" enum error type with a single variant, better to use a struct and make the error usecase specific.
Improve the `hashes::Error` by doing:
- Make it a struct
- Rename to `FromSliceError`
- Move it to the crate root (remove `error` module)
Includes usage in `bitcoin`.
ACKs for top commit:
apoelstra:
ACK 06afd52a12
Kixunil:
ACK 06afd52a12
Tree-SHA512: 20a517daaf3e9e09744e2a65cde6e238c8f2d1224899a6c04142a3a4e635d54112b0a2e846d25256071bb27cb70f7482380f98e9a535a5498aa4c7dc0d52cc54
3717a549f9 hashes: Fix stale repository name (Tobin C. Harding)
Pull request description:
The repository name is stale since we moved the `hashes` crate into the `rust-bitcoin` repo a while ago.
ACKs for top commit:
apoelstra:
ACK 3717a549f9
Kixunil:
ACK 3717a549f9
Tree-SHA512: ac4553547f1912c8242e3b87d3cc8951598999b5512ad1b49494b3c504449939e0e60905d96464b22c71b67ca975d18814a92af6e0aa66ff4f46effb97ac0733
We never enable the `DO_ALLOC_TESTS` variable, and hence never test the
"alloc" feature in `hashes`.
Remove the `DO_ALLOC_TESTS` variable and add "alloc" to the `FEATURES`
array.
We are trying to make error types stable on the way to v1.0
The current `hashes::Error` is a "general" enum error type with a single
variant, better to use a struct and make the error usecase specific.
Improve the `hashes::Error` by doing:
- Make it a struct
- Rename to `FromSliceError`
- Move it to the crate root (remove `error` module)
Includes usage in `bitcoin`.
During the last round of releases (bitcoin 0.30, hashes 0.12) we removed
the `FromHex` implementation from all types except vecs and arrays. We
added `FromStr` impls for types that roundtrip with `Display`.
We never added a changelog mention to either `bitcoin` or `hashes`, lets
retroactively add an entry.
Fix: #1747
c4c64c0dc5 Test with minimal dependency versions (Martin Habovstiak)
d5655d503a Bump core2 dependency from 0.3.0 -> 0.3.2 (Tobin C. Harding)
Pull request description:
This is work originally done by Kixunil in #1272, I picked it up to help out. The only changes I made were rebasingg, updating the recent lock file, adding `--locked` to hashes contrib file, and adding a co-developed-by tag for accountability.
It could happen that we unknowingly depend on a new version of a crate without updating `Cargo.toml`. This could cause resolution issues for downstream users. It's also unclear for outsiders to know with which dependencies did we test the crate.
This change commits two lock files: `minimal` and `recent`. `minimal` contains minimal dependency versions, while `recent` contains dependency versions at the time of making the change.
Further, this adds CI jobs to test with both lock files, CI job for `internals` crate, removes old `serde` pinning and prints a warning if `recent` is no longer up to date. (We may have to override it somehow if any crate breaks MSRV.)
The documentation is also updated accordingly.
Closes#1230
ACKs for top commit:
apoelstra:
ACK c4c64c0dc5
Kixunil:
ACK c4c64c0dc5
Tree-SHA512: 7d386e96ab747f6a6bafeea828ac65bd8bb11975eaa3408acecac369cd2f235f6e9d4c57202be18a3dc2eeb2a2df532d73e4d35cd1f3fbf092eb6414c55b1524
It could happen that we unknowingly depend on a new version of a crate
without updating `Cargo.toml`. This could cause resolution issues for
downstream users. It's also unclear for outsiders to see which
dependencies we tested the crate with.
This change commits two lock files: `minimal` and `recent`. `minimal`
contains minimal depdendency versions, while `recent` contains
dependency versions at the time of making the change.
Further, this adds CI jobs to test with both lock files, CI job for
`internals` crate, removes old `serde` pinning and prints a warning if
`recent` is no longer up to date. (We may have to override it somehow if
any crate breaks MSRV.)
The documentation is also updated accordingly.
Co-developed-by: Tobin C. Harding <me@tobin.cc>
Closes#1230
Whether or not every file needs an explicit license comment is out of
scope for this patch; in the `bitcoin` crate we use SPDX identifiers
because they are a single line with no loss of "benefit" over any longer
form.
Use SPDX identifiers in `hashes`. Drop the mention of re-licensing code
from Apache to CC0-1 (because the original code was written by Andrew
as well as the copied code then if the argument ever comes up it can be
easily countered).
Seems we no longer need an explicit error handler, remove it.
I did not grok the reason (long thread link below) but just removed it
and checked that the embedded crates still ran correctly.
https://github.com/rust-lang/rust/issues/51540)
6c61e1019e Fix pinning (schemars and MSRV) (Tobin C. Harding)
c8e38d6a5a hashes: Implement JsonSchema for sha256t::Hash<T> (Tobin C. Harding)
Pull request description:
This has grown due to now including pinning work also done in https://github.com/rust-bitcoin/rust-bitcoin/pull/1736, I decided to do this because the PRs conflict and doing it all here saves accidentally getting out of sync. And https://github.com/rust-bitcoin/rust-bitcoin/pull/1764 requires this PR.
- Patch 1 is unchanged
- Patch 2 now fixes pinning in bitcoin and hashes CI scripts and in the docs of both as well as the manifest stuff relating to `schemars` - phew.
Fix: #1687
ACKs for top commit:
Kixunil:
ACK 6c61e1019e
apoelstra:
ACK 6c61e1019e
Tree-SHA512: eae4aa9700817bab6ad444e07709e8b1a4ffb1625e08be6ba399abde38bf6f8e5ea216a0836e2e26dfaddc76c392802cd016738ea6e753a1bca2584d9d2a9796
Done as is single patch to make sure all the docs and CI are in sync and
correct.
We currently pin the `schemars` dependency using `<=0.8.3` as well as a
the `dyn-clone` transient dependency in the manifest (`hashes` and the
extended test crate). This is incorrect because it makes usage of the
crate klunky (or possibly impossible) if downstream users wish to use a
later version of `schemars`.
Observe also that we do not have to pin `schemars`, we do however have to pin
the `serde` crate if either `serde` or `schemars` features are enabled.
Do so in CI and document in the readme file within hashes.
Currently we have a pin remaining from the old MSRV (`syn` due to use
of `matches!`).
Fix pinning by:
- Remove pin in manifest for `schemars`
- Fix pinning for MSRV in CI and docs (this includes documenting pinning
requirements for `schemars` feature because it is related to the other
pin of `serde`) in both `hashes` readme and main repo readme.
The Rust API guidelines state that macros should be evocative of the
output, which is a sensible recommendation. We already had this for
`hash_newtype!` macro but didn't for sha256t version.
This changes the macro to have this syntax:
```rust
sha256t_hash_newtype! {
// Order of these structs is fixed.
/// Optional documentation details here. Summary is auto-generated.
/*pub*/ struct Tag = raw(MIDSTATE_BYTES, LEN);
/// Documentation here
#[hash_newtype(forward)] // optional, default is backward
/*pub*/ struct HashType(/* attributes allowed here */ _);
}
```
Closes#1427
Computing hashes in const fn is useful for esily creating tags for
`sha256t`. This adds `const fn` implementation for `sha256::Hash` and
the algorithm for computing midstate of tagged hash in `const` context.
Part of #1427
a189942c64 Use doc_auto_cfg (Tobin C. Harding)
Pull request description:
If we use `#![cfg_attr(docsrs, feature(doc_auto_cfg))]` instead of `#![cfg_attr(docsrs, feature(doc_cfg))]` we no longer need to manually mark types with `#[cfg_attr(docsrs, doc(cfg(feature = "std")))]`.
Sweeeeeet.
Props to pezcore for the lesson :)
ACKs for top commit:
apoelstra:
ACK a189942c64
Kixunil:
ACK a189942c64
Tree-SHA512: 1ced1e09f5d1733b362b83ca650d3f52c89eb57e78e8437f74c496d89776548f8c50feab6750352342e2abe680434681de2c126ce36a81dda21397b9695d4d4e
If we use `#![cfg_attr(docsrs, feature(doc_auto_cfg))]` instead of
`#![cfg_attr(docsrs, feature(doc_cfg))]` we no longer need to manually
mark types with `#[cfg_attr(docsrs, doc(cfg(feature = "std")))]`.
Sweeeeeet.
The `hashes` module contains a bunch of arrays, mostly formatted with 8
hex bytes on a line; add `rustfmt::skip` to keep the formatting of
arrays as is.
Remove the exclude for the `hashes` crate. Do not run the formatter,
that will be done as a separate patch to aid review.
bfd401c96e bitcoin_hashes: add CHANgELOG (Andrew Poelstra)
d1b7b54e3a bump bitcoin-hashes version to 0.12 (Andrew Poelstra)
Pull request description:
It was a little tricky to bump the version number because of https://github.com/rust-bitcoin/rust-bitcoin/issues/1553. There are a couple other things I considered trying, which maybe we'll do for future releases, but I believe this works for now.
Maybe should wait for #1111.
ACKs for top commit:
tcharding:
ACK bfd401c96e
sanket1729:
utACK bfd401c96e. ChangeLog looks good to me, did not review whether all noteworthy changes were included.
Tree-SHA512: d2104fc93e364415ae955e8123e6087c1eaa4c955aeaf4647ead051a223563326f66c0e278d68f64335e22c9d0af9b296dc3b744cd9d82d206844461fe7bf9c9
Enable formatting in CI by doing:
- Add a section to the `test.sh` scripts to run the formatter (guarded by
the env variable `DO_FMT`) for all crates (bitcoin, hashes, internals).
- Add `DO_FMT` to the nightly `Tests` CI job.
Because we have rust-secp in the loop, we need to update rust-secp, push
a new tag, and use that here, to ensure that the direct dependency on
bitcoin_hashes, and the rust-secp version, are compatible.
8ccfb412c1 Improve documentation of `hash_newtype!` (Martin Habovstiak)
58876e2be9 Remove unused macro (Martin Habovstiak)
Pull request description:
Removed unused macro and improved documentation to address review of #1659 - see commits. I also added a note about recursion.
ACKs for top commit:
apoelstra:
ACK 8ccfb412c1
tcharding:
ACK 8ccfb412c1
Tree-SHA512: 3b4b0c4ffc8a5166619110d9dcb51affd5cafbb2af84a55dd540a815e4702514d99c71dc1c54aca27fb91970e7e7189d1dffb4f7da7951b0f71336ef6f32d30b
The macro is non-trivial, so documenting it well is very useful. This
change improves both user-facing and developer-facing code with
appropriate warnings about the limitations of the code and Rust macro
system.
Currently we have an associated type on hash types `Inner` with
accompanying methods `into_inner`, `from_inner`, `as_inner`. Also, we
provide a way to create new wrapped hash types. The use of 'inner'
becomes ambiguous with the addition of wrapped types because the inner
could be the inner hash type or the `Inner` byte array of the inner
wrapped hash type.
In an effort to make the API more clear and uniform do the following:
- Rename `Inner` -> `Bytes`
- Rename `*_inner` -> `*_byte_array`
- Rename the inner hash to/from methods to `*_raw_hash`
Correct method prefix `into_` -> `to_` because theses methods convert
owned `Copy` types.
Add the trait Bound `Copy` to the `Bytes` type because we rely on this
trait bound for the conversion methods to be correctly named according
to convention.
Because of the dependency hole created by `secp256k1` this patch changes
the secp dependency to a git tag dependency that includes changes to the
hashes calls required so that we can get green lights on CI in this
repo.
The API guidelines say macro input should be evocative of the output.
`hash_newtype` didn't have this property.
This change makes it look exactly like the resulting struct, `$len`
parameter was removed since it's not needed, reversing is controlled
using an attribute. The macro is also better documented and ready to be
extended in the future.
The tagged SHA256 newtype is not yet modified because it has a more
complicated input parameters.
Closes#1648
70bff0de8d ci: Remove incorrect code comment and cargo clean (Tobin C. Harding)
Pull request description:
Recently while trying to fix CI I (Tobin) added a call to `cargo clean` and a code comment justifying it. This was merged because while not incorrect it is redundant since calling `cargo` with different `RUSTFLAGS` triggers a rebuild.
ACKs for top commit:
apoelstra:
ACK 70bff0de8d
Kixunil:
ACK 70bff0de8d
Tree-SHA512: 5bed107aedf9d6d240fa885335bfe04099436c0d30c045554644b43a06900c415708dfcedbd27037564d296c0ab12e4c4d4dc1e25ab64b4cc07ca7b46c46628a
a121e19e94 hashes: Implement AsRef for fixed size arrays (Tobin C. Harding)
Pull request description:
Implement `AsRef<[u8; X]>` for hash types including wrapped hash types. Doing so means at times the compiler can no longer infer the type because we have `AsRef<[u8]` implemented also but we can use `into_inner` and `as_inner` to get the inner array if needed.
Fix: #1462
## Note
This touches code that will likely be changed by #1577 and when we do #1491 but I believe its a step forward.
ACKs for top commit:
arturomf94:
ACK [`a121e19`](a121e19e94)
apoelstra:
ACK a121e19e94
Kixunil:
ACK a121e19e94
Tree-SHA512: 257c44826c7649db25bb3a6f023f68b2f17b70c546a056afad044bc8a16bf61f654c3846222505aaf5e6f9a0ad1d2113272d61317b407d0ac83702e41060a1ee
Currently we have a few things mixed up in the feature gating of
`hashes`.
Observe that:
- `io::Write` is not related to allocation.
- "std" should be able to enable "alloc".
Improve feature gating by doing:
- Enable "alloc" from "std" and simplify `cfg` codebase wide.
- Enable "internals/alloc" from "alloc".
- Fix feature gating to use the minimal requirement i.e., "alloc".
41f2dcf6ae Improve test coverage for docs build (Tobin C. Harding)
b4c14a4b7c hashes: Use automatic link (Tobin C. Harding)
96e8a080d1 ci: Remove redundant || exit (Tobin C. Harding)
Pull request description:
Currently the docs build commands in `hashes` and `bitcoin` differ, they should be the same.
Add a command `cargo doc` to improve coverage e.g., recently we botched the feature guarding but since CI only runs `cargo rustdoc` with custom compiler conditional set we didn't catch it.
Done after seeing: https://github.com/rust-bitcoin/rust-bitcoin/pull/1504 and CI should fail on this PR until 1504 is in.
ACKs for top commit:
apoelstra:
ACK 41f2dcf6ae
Kixunil:
ACK 41f2dcf6ae
Tree-SHA512: 7cde68292cfc6f32b75d066e188e7c418ee251f9a5abc57fbd642ba33e9cd5bd8ef7c5ba7cffd206acae6ddec2f8c3db38c8c911a4319e979158666b8225953d
Clean up the optional dependencies by doing:
- Put the optional dependencies below the non-optional dependencies as
is customary, separated by a line of whitespace.
- Put `optional = true` as the last item so as to be uniform
- Put `core2` at the top, aids reading because of how the comments are
distributed (`core2` does not have a comment on it).
Improve std/alloc features by doing:
- Fix incorrect feature enabling: "std" should enable "internals/std"
not "internals/alloc".
- Put the "alloc" feature under the "std" feature, same as in other
crates.
- Remove the comment, devs should understand what dependencies are for.
We don't have comments everywhere else so we don't really need this one.
`core2` is for Read/Write, nothing to do with allocation and we do not
use the "alloc" feature of `core2` in `hashes`.
Fix core2 dependency/features by doing:
- Explicitly enable "bitcoin_hashes/core2" in `bitcoin`.
- Do not enable "core2/alloc" in `hashes`
Improve all manifest package sections by doing:
- Order the list of options uniformly
- Remove unnecessary homepage option (currently same as repo)
- Add categories section
We can check which files are included in the packaged release with
`cargo package --list `.
Add an `exclude` section to each manifest that excludes `tests/` and
`contrib/`. Not all crates have a `tests/` directory yet but they should
so add the exclude anyway to future proof the crates.
Remove `FromHex` from hash and script types
- Remove the `FromHex` implementation from hash types and `ScriptBuf`
- Remove the `FromStr` implementation from `ScriptBuf` because it does not
roundtrip with `Display`.
- Implement a method `from_hex` on `ScriptBuf`.
- Implement `FromStr` on hash types using a fixed size array.
This leaves `FromHex` implementations only on `Vec` and fixed size arrays.
It is easier to maintain code if macros use the fully qualified path to
types and functions instead of relying on code that uses the macro to
import said type or function.
We have old Rust 1.29 error handling code still in `hashes`. Implement
`std::error::Error` for the `hex::Error` and `error::Error` types in
line with "modern" Rust 1.41.1 error handling.
Implement `AsRef<[u8; X]>` for hash types including wrapped hash types.
Doing so means at times the compiler can no longer infer the type because we have
`AsRef<[u8]` implemented also but we can use `into_inner` and `as_inner`
to get the inner array if needed.
The `HexWriter` is not used any more since we added the new hex code in
internals for fast hex encoding.
While we are removing the benches for `HexWriter` also remove the last
remaining bench for writing using `Display` because this is not the
correct place for that code - its trivial to re add later in the correct
module.
Recently while trying to fix CI I (Tobin) added a call to `cargo clean`
and a code comment justifying it. This was merged because while not
incorrect it is redundant since calling `cargo` with different
`RUSTFLAGS` triggers a rebuild.
When we merged `hashes` into `rust-bitcoin` we ran the test script a
couple of times using `./hashes/contrib/test.sh` this causes the `cargo`
commands in the CI script to be run from the crate root which is not
what we want.
This showed up recently because `cargo test` was getting run in
`bitcoin` after building with address/memory sanitazation configured
into the build.
While we are at it run `cargo clean` after the last sanitizer build just
to future proof the CI script in case we later accidentally re-use the same
build with plain old `cargo` (without `-Zbuild-std etc.`).
Currently the docs build commands in `hashes` and `bitcoin` differ, they
should be the same.
Add a command `cargo doc` to improve coverage e.g., recently we botched
the feature guarding but since CI only runs `cargo rustdoc` with custom
compiler conditional set we didn't catch it.
Run docs in CI using nightly and stable toolchains as required.
The `ToHex` trait was replaced by either simple `Display`/`LowerHex`
where appropriate or `DisplayHex` from `bitcoin_internals` which is
faster.
This change replaces the usages and removes the trait.
411174c391 Add fuzz target for sha512_256 (Calvin Kim)
31fc1f8638 Add support for sha512/256 (Calvin Kim)
15b5af1117 Export sha512::HashEngine fields/function within the crate (Calvin Kim)
Pull request description:
Adds a new file named `sha512_256.rs` that implements the `sha512/256` hash. This was needed as a part of https://github.com/rust-bitcoin/rust-bitcoin/discussions/1318 to drop the `sha2` dependency.
All the actual hashing code is exactly the same as `sha512.rs`, minus the initial constants and the use of `hash_type!` macro. Some unit tests were added from wikipedia (for the "" input) and the rest were from the Go standard library's tests for sha512_256.
Benchmarks on my Ryzen 3600 machine show that it is faster than sha256.
```
test sha256::benches::sha256_10 ... bench: 37 ns/iter (+/- 0) = 270 MB/s
test sha256::benches::sha256_1k ... bench: 3,338 ns/iter (+/- 24) = 306 MB/s
test sha256::benches::sha256_64k ... bench: 213,605 ns/iter (+/- 1,806) = 306 MB/s
test sha512_256::benches::sha512_256_10 ... bench: 27 ns/iter (+/- 1) = 370 MB/s
test sha512_256::benches::sha512_256_1k ... bench: 2,196 ns/iter (+/- 12) = 466 MB/s
test sha512_256::benches::sha512_256_64k ... bench: 140,552 ns/iter (+/- 777) = 466 MB/s
```
One caveat is that I could not get hongfuzz to build locally so I couldn't test the fuzz on my machine. I ended up only testing through the CI for the fuzz tests.
I thought adding a completely separate file was the easiest and the most straightforward way of implementing it. I'm very much open to changing the implementation if you guys don't think this is the right direction.
ACKs for top commit:
sanket1729:
ACK 411174c391. Reviwed range diff from 43feb9ea7b282d9119708a27fa7a1c7412d1386a that I had ACked
apoelstra:
ACK 411174c391
Tree-SHA512: 98298a7c177cbb616bfbc02cec5c5860f10204df8275cc9f1e4ea07333b901095e574fbc3fe0a03375e0d321a1579e2c2023a5c14addd863e10cc927f155710c
3e520f9094 Use hex from internals rather than hashes (Martin Habovstiak)
Pull request description:
`bitcoin-internals` contains a more performant implementation of hex encoding than what `bitcoin_hashes` uses internally. This switches the implementations for formatting trait implementations as a step towards moving over completely.
The public macros are also changed to delegate to inner type which is technically a breaking change but we will break the API anyway and the consuers should only call the macro on the actual hash newtypes where the inner types already have the appropriate implementations.
Apart from removing reliance on internal hex from public API this reduces duplicated code generated and compiled. E.g. if you created 10 hash newtypes of SHA256 the formatting implementation would be instantiated 11 times despite being the same.
To do all this some other changes were required to the hex infrastructure. Mainly modifying `put_bytes` to accept iterator (so that `iter().rev()` can be used) and adding a new `DisplayArray` type. The iterator idea was invented by Tobin C. Harding, this commit just adds a bound check and generalizes over `u8` and `&u8` returning iterators.
While it may seem that `DisplayByteSlice` would suffice it'd create and initialize a large array even for small arrays wasting performance. Knowing the exact length `DisplayArray` fixes this.
Another part of refactoring is changing from returning `impl Display` to return `impl LowerHex + UpperHex`. This makes selecting casing less annoying since the consumer no longer needs to import `Case` without cluttering the API with convenience methods.
ACKs for top commit:
tcharding:
ACK 3e520f9094
apoelstra:
ACK 3e520f9094
Tree-SHA512: 62988cec17550ed35990386e572c0d32dc7107e1c36b7c9099080747e15167e6d66497fb300178afbd22481c0360a6b7a1228fd09402d4ce5d295a8594c02aa6
56e4e53357 hashes: ci: Remove --all (Tobin C. Harding)
Pull request description:
Currently we are using the `--all` flag in `cargo` commands in the `hashes` CI script. This flag (the deprecated version of `--workspace`) causes cargo to run the command for the whole workspace, this is not what we want because we run test individually for each crate using a ci script per crate.
The effect of this patch is to reduce re-runs of tests i.e., reduce machine usage during CI runs with no reduction of coverage - PROFIT!
ACKs for top commit:
apoelstra:
ACK 56e4e53357
sanket1729:
utACK 56e4e53357
Tree-SHA512: 13134ba2f6806a705f99af5b8d66b051e1e58da177a02ee46880f494e37c380fc4c28731cd42eabbd69ae884763dbc360902e0e8afa7f88e78483e8a37f614f5
5a2a37d4be Allow dead_code/unused_imports when fuzzing (Tobin C. Harding)
Pull request description:
Littering the codebase with `#[cfg(not(fuzzing))]` is a bit messy just to quieten the linter during fuzzing. Instead just globally allow.
Done while debugging #1409
ACKs for top commit:
sanket1729:
ACK 5a2a37d4be
apoelstra:
ACK 5a2a37d4be
Tree-SHA512: fb84215a2b00ad6d3321b2781ba285af513ff8fd413c0997045a41c4f23028d2ef0fdf083839289d0c5108c990aa66bdae4430ad3ef32881eac5324b2e881b3b
`bitcoin-internals` contains a more performant implementation of hex
encoding than what `bitcoin_hashes` uses internally. This switches the
implementations for formatting trait implementations as a step towards
moving over completely.
The public macros are also changed to delegate to inner type which is
technically a breaking change but we will break the API anyway and the
consuers should only call the macro on the actual hash newtypes where
the inner types already have the appropriate implementations.
Apart from removing reliance on internal hex from public API this
reduces duplicated code generated and compiled. E.g. if you created 10
hash newtypes of SHA256 the formatting implementation would be
instantiated 11 times despite being the same.
To do all this some other changes were required to the hex
infrastructure. Mainly modifying `put_bytes` to accept iterator (so that
`iter().rev()` can be used) and adding a new `DisplayArray` type. The
iterator idea was invented by Tobin C. Harding, this commit just adds a
bound check and generalizes over `u8` and `&u8` returning iterators.
While it may seem that `DisplayByteSlice` would suffice it'd create and
initialize a large array even for small arrays wasting performance.
Knowing the exact length `DisplayArray` fixes this.
Another part of refactoring is changing from returning `impl Display` to
return `impl LowerHex + UpperHex`. This makes selecting casing less
annoying since the consumer no longer needs to import `Case` without
cluttering the API with convenience methods.
b78ba730f2 hashes: Run clippy in ci (Tobin C. Harding)
5e67f7a7cb Remove the unnecessary explicit reference (Tobin C. Harding)
Pull request description:
Currently we only run the linter in `bitcoin/contrib/test.sh`, we should do the same in the `hashes` ci script.
- Patch 1: Fix current clippy issues in `hashes` crate
- Patch 2: Run clippy in CI for `hashes` crate
ACKs for top commit:
apoelstra:
ACK b78ba730f2
Kixunil:
ACK b78ba730f2
Tree-SHA512: f9fedcd8c1a06c715396cf6c7b752e2c9e32dbfde48c8b4bcb9ac5e701abe109ddeadc2e7466f6926f7c3ab74fa26e68b70473b4a5b5009cb4644d634707d4ea
Currently we are using the `--all` flag in `cargo` commands in the
`hashes` CI script. This flag (the deprecated version of `--workspace`)
causes cargo to run the command for the whole workspace, this is not
what we want because we run test individually for each crate using a ci
script per crate.
The effect of this patch is to reduce re-runs of tests i.e., reduce
machine usage during CI runs with no reduction of coverage - PROFIT!
Currently we implement `Deref` for hashes. From the docs [0]
> Deref should only be implemented for smart pointers to avoid confusion
Furthermore because we implement `Deref` as well as implement
`internals::hex::display::DisplayHex` for slices hashes get coerced into
slices and `to_lower_hex_string` can be called on them, this is
incorrect because `DisplayHex` does not account for hashes that display
backwards so we end up with the wrong string.
[0] https://doc.rust-lang.org/std/ops/trait.Deref.html
Clippy emits various warnings of form:
warning: this expression creates a reference which is immediately
dereferenced by the compiler
As suggested, remove the unnecessary explicit reference.
Recently clippy was updated and now new warnings are generated for the
`hashes` crate.
Clippy emits 3 warnings of form:
warning: this expression borrows a value the compiler would automatically borrow
As suggested, remove the explicit borrow.
We would like to bring the `bitcoin_hashes` crate into the
`rust-bitcoin` repository.
Import `bitcoin_hashes` into `rust-bitocin/hashes`, doing so looses all
the commit history from the original crate but if we archive the
original repository then the history will be preserved. We maintain the
same version number obviously and in the changelog we note the change of
repository.
Commit hash that was tip of `bitcoin_hashes` at time of import:
commit 54c16249e06cc6b7870c7fc07d90f489d82647c7
Includes making `embedded` and `fuzzing` per-crate i.e., move them into
`bitcoin` as hashes includes these also.
NOTE: Does _not_ enable fuzzing for `hashes` in CI.
Notes on CI:
Attempts to merge in the github actions from the hashes crate however reduces
coverage by not running hashes tests for beta toolchain. Some additional
work could be done to improve the CI to increase efficiency without
reducing coverage. Leaving for another day.
Tobin C. Harding
Andrew Poelstra
Fmt Bot
Jamil Lambert, PhD
Nick Johnson
Jose Storopoli
Matt Corallo
448 OG
Martin Habovstiak
josibake
Steven Roose
Thabokani
shuoer86
Alekos Filini
Riccardo Casatta
sanket1729
yancy
junderw
Harshil Jani
Calvin Kim