Our `release` job checks for 'TBD', I can't remember exactly why but I
thought we introduced `0.0.0-NEXT-RELEASE` because CI was failing when
we used TBD - clearly this is not the case now because we have a bunch
of `TBD`s in the code base.
Change all the instances of `0.0.0-NEXT-RELEASE` to be `TBD`.
c97389596b Remove stale docs from sha256t_hash_newtype (Tobin C. Harding)
39f7dcb816 Reduce API surface of tagged wrapped hash types (Tobin C. Harding)
Pull request description:
Recently we made it so that wrapper types created with `hash_newtype` were not general purpose hash types i.e., one could not easily hash arbitrary data into them. We would like to do the same for tagged wrapped hash types.
In `hashes` do:
- Create a new macro `sha256_tag` that does just the tag/engine stuff out of the `sha256t_hash_newtype` macro.
- Deprecate the `sha256t_hash_newtype` macro.
In `bitcoin` do:
- Use a combination of `sha256_tag` and `hash_newtype` to create tagged wrapped hash types.
Note that we do not add private helper functions `engine` and `from_engine` to the tagged wrapper types as we do for legacy/segwit in `sighash`. Can be done later if wanted/needed.
Fix: #3135
ACKs for top commit:
Kixunil:
ACK c97389596b
apoelstra:
ACK c97389596b successfully ran local tests
Tree-SHA512: d937a8eac1a77298231f946f9dfbc2f7739af8da00f2075b0b54803b4111c0cec810bc6564515153769193056cf102a9c954e216664f055b249d4a6153b14bca
Recently we made it so that wrapper types created with `hash_newtype`
were not general purpose hash types i.e., one could not easily hash
arbitrary data into them. We would like to do the same for tagged
wrapped hash types.
In `hashes` do:
- Create a new macro `sha256t_tag` that does just the tag/engine stuff
out of the `sha256t_hash_newtype` macro.
- Deprecate the `sha256t_hash_newtype` macro.
In `bitcoin` do:
- Use a combination of `sha256t_tag` and `hash_newtype` to create tagged
wrapped hash types.
Note that we do not add private helper functions `engine` and
`from_engine` to the tagged wrapper types as we do for legacy/segwit in
`sighash`. Can be done later if wanted/needed.
975f22f399 hashes: Call through to trait methods (Tobin C. Harding)
Pull request description:
Currently we have duplicate code in inherent functions that also occurs in the default implementation of the `GeneralHash` trait methods, this is unnecessary because we can call through to the trait methods.
ACKs for top commit:
Kixunil:
ACK 975f22f399
apoelstra:
ACK 975f22f399 successfully ran local tests
Tree-SHA512: 74d8905a20d75536abf477dd2226e3cb12d8bd7330b1769e520840df1538362c6cbec6a976dfeb771797732b1f9259ee4f1970cadb69eca67b8b9bbe956ceeca
Add a function `hash_reader` that uses the `BufRead` trait to read
bytes directly into the hash engine.
Add the functionality to:
- as a trait method in the `GeneralHash` trait with default implementation
- as inherent functions to all the hash types
Close: #3050
Currently we have duplicate code in inherent functions that also occurs
in the default implementation of the `GeneralHash` trait methods, this
is unnecessary because we can call through to the trait methods.
c72069e921 Bump MSRV to 1.63 (Martin Habovstiak)
Pull request description:
The version 1.63 satisfies our requirements for MSRV and provides significant benefits so this commit bumps it. This commit also starts using some advantages of the new MSRV, namely namespaced features, weak dependencies and the ability to use trait bounds in `const` context.
This however does not yet migrade the `rand-std` feature because that requires a release of `secp256k1` with the same kind of change - bumping MSRV to 1.63 and removing `rand-std` in favor of weak dependency. (Accompanying PR to secp256k1: https://github.com/rust-bitcoin/rust-secp256k1/pull/709 )
Suggested plan:
* merge both PRs
* at some point release `hashes` and `secp256k`
* remove `rand-std` from `bitcoin`
* release the rest of the crates
ACKs for top commit:
apoelstra:
ACK c72069e921
tcharding:
ACK c72069e921
Tree-SHA512: 0b301ef8145f01967318d3ed1c738d33e6cf9e44f835f3762122b460a536f926916dbd6ea39d6f80b4f95402cd845e924401e75427dbb0731ca5b12b4fa6915e
The version 1.63 satisfies our requirements for MSRV and provides
significant benefits so this commit bumps it. This commit also starts
using some advantages of the new MSRV, namely namespaced features, weak
dependencies and the ability to use trait bounds in `const` context.
This however does not yet migrade the `rand-std` feature because that
requires a release of `secp256k1` with the same kind of change - bumping
MSRV to 1.63 and removing `rand-std` in favor of weak dependency.
In a `HashEngine` the `length` field represents number of bytes
input into the hash engine.
Note also:
> the midstate bytes are only updated when the compression function is
run, which only happens every 64 bytes.
Currently our midstate API allows extracting the midstate after any
amount of input bytes, this is probably not what users want.
Note also that most users should not be using the midstate API anyways.
With all this in mind, add a private `length` field to the `Midstate`
struct and enforce an invariant that it is modulo 64.
Add a single const `Midstate` constructor that panics if the invariant
is violated. The `Midstate` is niche enough that panic is acceptable.
Remove the `from_slice`, `from_byte_array`, and `to_byte_array`
functions because they no longer make sense. Keep `AsRef<[u8]>` for
cheap access to the midstate's inner byte slice.
Note change to `Debug`: `bytes` field now does not include the `0x`
prefix because `as_hex` because of the use of `debug_struct`.
Enjoy nice warm fuzzy feeling from hacking on crypto code.
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.
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).
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.
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).
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.
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.
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`.
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).
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
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
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
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".
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.
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.
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
merge-script
Martin Habovstiak
Fmt Bot
Jamil Lambert, PhD
shuoer86