The `hashes` crate has a bunch of similar types defined by a bunch of
similar macros and impl blocks, all of which makes it difficult to tell
exactly what is implemented where. In an effort to make the code easier
to read order the `from_byte_array` constructor in the same place across
the crate. Note also we typically put constructors up the top, also
`from_byte_array` is the likely most used constructor so put it first.
FWIW I discovered this while polishing the HTML docs.
Internal change only.
We had an initial go at this but we didn't do the `Hash` trait method.
In order to do so we need to hack the serde code a fair bit, note the
public visitor types.
3b7ba4f977 Remove the SliceIndex implementation from hash types (Tobin C. Harding)
Pull request description:
If folk really want to index into a hash they can us `as_byte_array` then index that.
Includes a bump to the version number of `hashes` to `v0.15.0` - this is because otherwise `secp` won't build since we are breaking an API that is used in the current release of secp.
Fix: #3115
ACKs for top commit:
apoelstra:
ACK 3b7ba4f977 successfully ran local tests
Tree-SHA512: 0ba93268cd8133fe683183c5e39ab8b3bf25c15bfa5767d2934d67a5f6a0d2f65f6c9304952315fe8a33abfce488d810a8d28400a28facfb658879ed06acca63
`length` was changed to `bytes_hashed` and the type changed from `usize`
to `u64`.
The addition under the `hashes_fuzz` feature flag has been changed from
`usize` to `u64` to fix the fuzz error.
When `hashes_fuzz` is on the field `length` is refered to but it was
changed to `bytes_hashed` in a previous PR.
This has been fixed by changing `length` to `bytes_hashed`.
If folk really want to index into a hash they can us `as_byte_array`
then index that.
Includes a bump to the version number of `hashes` to `v0.15.0` - this
is because otherwise `secp` won't build since we are breaking an API
that is used in the current release of secp.
Fix: #3115
The hash engine types have a `length` field that is used to cache the
number of bytes hashed so far, as such it is an arbitrary number and
could use a `u64` instead of `usize`.
While we are at it rename `length` to `bytes_hashed` to remove any
ambiguity of what this field is. Note this field is private, we already
have the public getter `n_bytes_hashes` to get the value.
Introduce a private function `incomplete_block_size`, the purpose of
this function is to put all the casts in one place so they can be well
documented and easily understood.
Fix: #3016
be13397570 Make hmac & hkdf more robust against buggy `Hash` (Martin Habovstiak)
94c0614bda Enforce that `Hash::Bytes` is an array (Martin Habovstiak)
Pull request description:
This makes sure `Hash::Bytes` is an array. We've discussed this somewhere but I don't remember where.
I'm not sure if the second commit is actually valuable but hopefully shouldn't make things worse.
ACKs for top commit:
apoelstra:
ACK be13397570 successfully ran local tests; yep, this looks like an improvement. Agreed that the second commit has questionable value but doe not make things worse
tcharding:
ACK be13397570
Tree-SHA512: 0fed982084f0f98927c2b4a275cec81cb4bbc0efbf01551a0a4a8b6b39a4504830243ee8d55a5c0418d81b5d4babc7b22332dbacc0609ced8fada84d2961ae71
In the future we would like to guarantee the correctness of `LEN` which
is currently not entirely possible, so this at least adds a sealed trait
enforcing the `Bytes` type to be an array. Consumers concerned about the
validity of the length can access the `LEN` constant on `Bytes` instead
to get the correct length of the array.
- make tests no_std compatible by adding imports to alloc or std
- feature gate tests behind the 'alloc' feature if they use anything
from 'alloc' (like the `format!` macro)
- schemars feature enables alloc
2169b75bba Use lower case error messages (Jamil Lambert, PhD)
Pull request description:
Error messages should be lower case, except for proper nouns and variable names. These have all been changed.
~~They should also state what went wrong. Some expect error messages were positive, giving the correct behaviour or correct input. These have been changed so that they are now negative, i.e. saying what went wrong.~~
EDIT: After further discussion it was decided not to change the expect messages.
ACKs for top commit:
Kixunil:
ACK 2169b75bba
tcharding:
ACK 2169b75bba
Tree-SHA512: 92442c869e0141532425f6fca5195fd319b65026f68c4230a65ad70253565d98931b2b44ee202975c307280525c505147e272297dc81207312e40c43d007021c
In an effort to shrink the API of `hashes` remove the `from_raw_hash`,
`to_raw_hash`, and `as_raw_hash` inherent functions from types created
with the `hash_newtype` macro.
There are a few reasons why this is favourable:
- It allows stable crates to use the macro and not expose unstable
`hashes` types in their API.
- It makes types created with the macro less "general" in the sense that
its more obscure to just hash any data into them. This allows us to
write cleaner APIs in `rust-bitcoin`.
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.
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.
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
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.
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.
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)
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).
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.
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
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
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.
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.
`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.
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
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
Jamil Lambert, PhD
Martin Habovstiak
Antoni Spaanderman
448 OG
josibake
Riccardo Casatta