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
Andrew Poelstra
Fmt Bot
Tobin C. Harding
Jamil Lambert, PhD
Nick Johnson
Jose Storopoli
Matt Corallo
448 OG