`s.parse` is more idiomatic and produces more helpful error messages.
This has been changed repo wide in the main codebase, not including
examples, rustdocs, and in the test module.
`use std::str::FromStr;` has been removed where this change makes
it unnecessary.
a2be82c0c9 Use TBD in deprecated attribute (Tobin C. Harding)
Pull request description:
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`.
ACKs for top commit:
Kixunil:
ACK a2be82c0c9
apoelstra:
ACK a2be82c0c9 successfully ran local tests
Tree-SHA512: b383cc4095484291a7b4dca593ad5e017e3a9de9bfae9d6e9447ae36da32aa1c0d1fd593f49fd52c04db5ca5cdbaae8b30a772f792df13542f0a157a86295746
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`.
The previous change was just a dumb rename with no deprecation and it
also kept the `self` type which should be taken by value since the hash
is `Copy`. This improves on it by adding a deprecated method of the
original name and changing the type to be `self` instead of `&self`.
Previously we had removed `Default` impl on `siphash24::HashEngine` by
reimplementing the type manually. This was a really bad idea as it
inevitably led to API differences that broke the build which we didn't
notice because of unrelated bug. It should've just split the macro from
the start as was suggested but it was claimed to be difficult, I don't
think was the case as can be seen by this PR.
This commit does what the previous one should've done: it renames the
macro to have `_no_default` suffix, creates another one of the original
name that calls into `_no_default` one and moves anything related to
`Default`. This cleanly ensures all previous hashes stay the same while
siphash gets `Default` removed. This also removes all now-conflicting
impls from `siphash24` module which makes the module almost identical to
what it looked like before the change. The only differences are removed
`Default`/`new`, fixes in tests and recent rename of `as_u64` to
`to_u64`.
60b3cabb41 Panic in const context (Tobin C. Harding)
Pull request description:
Now that our MSRV is past 1.57 we can panic in const contexts.
Fix: #2427
ACKs for top commit:
Kixunil:
ACK 60b3cabb41
apoelstra:
ACK 60b3cabb41 successfully ran local tests
Tree-SHA512: 705a8b7d52a11826e6084684706cb7e01dfaa554e4e369739e64e64263537b0c8c0e518b04e96249ecdeea1f22b534594ffd2a89e17ebba85b369d896e820239
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.
In recent work making functions on hash types inherent it seems we
missed `siphash`. Add inherent getters/setters to the `siphash::Hash`
type and call through to them from the `Hash` trait impl.
Note:
- The `extra_test.sh` script runs for all toolchains.
- The `schemars` crate does not use the repo lock files.
- We need to pin some deps when building the `schemars` test.
Pin in the `extra_test.sh` script, and mention it in the docs so the
docs don't go stale next MSRV update.
This was previously unnoticed because of the `run_task` bug.
ref: rust-bitcoin/rust-bitcoin-maintainer-tools#10
e7762e0612 hashes:: Rename const_hash functions (Tobin C. Harding)
Pull request description:
There are a number of issues with the two `const_hash` functions in the `sha256` module:
- The two `const_hash` functions in the `sha256` module differ slightly, one finalizes the hash and one is for computing the midstate.
- They are inefficient and provided for usage for const context only.
Fix both issues by renaming the functions as discussed in #3075.
Close: #3075
ACKs for top commit:
Kixunil:
ACK e7762e0612
apoelstra:
ACK e7762e0612 successfully ran local tests
Tree-SHA512: 2b765bbbaa596d060a555495582b24175f660bf630de489cf0e0199f1c589f13f46dde5c9735bffece10a1ff116a70472f821df66c62a97fffb424f16e5568f9
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
There are a number of issues with the two `const_hash` functions in the
`sha256` module:
- The two `const_hash` functions in the `sha256` module differ slightly,
one finalizes the hash and one is for computing the midstate.
- They are inefficient and provided for usage for const context only.
Fix both issues by renaming the functions as discussed in #3075.
Close: #3075
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.
* The Default bound only makes sense for unkeyed hash functions which
can fire up a new engine without a key. Keyed hash functions, like
SipHash24 or Poly1305 require a secret key to be initialized and
should not implement a default engine generator.
* SipHash24 tests updated to the previous default key "0".
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.
The midstate has not been finalized [0], so use the term in the struct
header.
FTR I don't know _exactly_ what "finalized" means in the context of
sha256 hashing (or hashing in general). This change came from a review
suggestion and we have other mentions of "finalized" in the code.
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.
Done in preparation for adding a `length` field to `Midstate` and also
in preparation for removing the `Display` implementation (will be
justified in the patch that does it).
Currently in the `Debug` impl of `Midstate` we are calling through to
`Display` using the alternate form of printing, we can use `as_hex` to
achieve almost the same thing. Note that in `LowerHex` we use the
`fmt_hex_exact` macro that allows us to reverse the iterator however
when we later attempt to use `f.debug_struct` we cannot use the macro.
Elect to change the current behaviour to `Debug` forwards, shown by the
change to the regression test.
Currently we are using a macro to implement `AsRef` and `Borrow` for
`sha256::Midstate`.
In preparation for adding a length field to the `Midstate` remove the
implementation of `Borrow` but keep `AsRef`.
API breaking change.
The `sha256::Midstate` is a niche use case type, there is no real reason
we need to support serialization/deserialization. If people really want
this they can just get the byte array and serialize it themselves.
API breaking change.
In preparation for changing the `sha256::Midstate` internals stop using
the `arr_newtype_fmt_impl` macro and implement the `fmt` traits
manually.
In doing so, remove the `DISPLAY_BACKWARDS` const but keep the current
behaviour of displaying the midstate backwards.
Put the impl block for `Midstate` under the struct, as is customary.
(Note the diff shows moving some other code around the impl block not
the impl block itself.)
Code move only.
Explicitly import `sha256t` in docs builds and remove explicit link
target. This patch is code churn on its own but the `sha256t` module
will be used again in proceeding patches, done separately to reduce the
size/complexity of proceeding patches.
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
2b56f763d0 hashes: Remove to/from/as_raw_hash (Tobin C. Harding)
Pull request description:
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`.
ACKs for top commit:
Kixunil:
ACK 2b56f763d0
apoelstra:
ACK 2b56f763d0
Tree-SHA512: 3d73aa8250dd775994623c9201dd819256acf2ec82526b3537da74c9e19c2ac5e8bba358a2171f7b02342804cb6b4d5ac4dca88d912b3d46d14e3bc35dd5cb91
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`.
Midstates are not generic objects; they don't have universal
cryptographic properties and if you are using them you should be using a
specific midstate type. Therefore it shouldn't be part of `GeneralHash` or
`HashEngine`. Furthermore, in practice it seems like `sha2` midstates are
the only ones that anybody uses, at least in bitcoin.
Remove the midstate stuff from the `GeneralHash` and `HashEngine`
traits. Keep the `midstate` functionality as inherent functions if it is
used internally. Keep the functionality on `sha256` as inherent public
functions.
Depending on types being in scope when calling macros is bad practice
but we have mistakenly done so in `internal_macros` when using the
`FromSliceError`.
Use `$crate::FromSliceError` in the macro and remove import statements.
Currently we are using a type alias for the `hash160::HashEngine`.
Type alias' allow for potential mixing of types, a `hash160::HashEngine`
struct can better serve our users with not much additional complexity or
maintenance burden.
As we did for the `sha256d::HashEngine`, add a new wrapper type
`hash160::HashEngine` that replaces the current type alias.
Currently we are using a type alias for the `sha256d::HashEngine`.
Type alias' allow for potential mixing of types, a `sha256d::HashEngine`
struct can better serve our users with not much additional complexity or
maintenance burden.
Jamil Lambert, PhD
Fmt Bot
merge-script
Tobin C. Harding
Martin Habovstiak
leichak
Shing Him Ng
Nick Johnson