5ec17a2ee8 CI: Hobble WASM job (Tobin C. Harding)
Pull request description:
The WASM job is not working. `wasm-pack build` works fine but `wasm-pack test` doesn't compile.
Comment out the line that runs `wasm-pack test`.
ACKs for top commit:
apoelstra:
ACK 5ec17a2ee8c963d182777a9661b1bab786956bd4; successfully ran local tests; LGTM
Tree-SHA512: 5ea88bd99f625a8c669dc1dec02486dd549e9ac83d7670c6c7b3b25c86de3fc9a8dbc959dc39de92305dd5e254e0b8ed9424cae5e5941b66a6476ee433009fdd
1cb24c1f15 hashes:: Polish crate level rustdocs (Tobin C. Harding)
98691186dc hashes: Move engine functions (Tobin C. Harding)
12f261c009 hashes: Re-order from_byte_array (Tobin C. Harding)
c11587d60d hashes: Rename hash_type macro (Tobin C. Harding)
62617cf9ac hashes: Move from_engine function to other macro (Tobin C. Harding)
bb7dd2c479 hashes: Move DISPLAY_BACKWARD to top of impl block (Tobin C. Harding)
71013afe07 hashes: Put attribute under doc (Tobin C. Harding)
Pull request description:
`hashes 1.0.0` can't be far away, here is a quick polish (done while I waited for my car to get fixed).
Everything here is internal except stuff to docs. Note please I claim "internal change only" in a bunch of patches that do code moves but these effect the docs build because order is preserved in some instances.
ACKs for top commit:
apoelstra:
ACK 1cb24c1f150bc2d65d0be439f2005f41d95ad23c; successfully ran local tests
Tree-SHA512: 430c451afab8fc92fb4596bf2d4b36c086333fe72b3fe5858925b75597641b8c4f5e49f7643888fa19b675d3070ce9a3606623cd56bdba6cfc59e459fbdda440
The `sha256t` module is unique in that it implements its methods
manually (as call throughs) instead of using the macros. To make it more
clear what is implemented re-order the engine constructor and getter to
better mirror the layout in the macros.
Internal change only.
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.
Conceptually (and using traits) we split the hashes into "general"
hash types and more restricted hash types (`Hash`). Also we observe that
the `hash_type` macro defines all the inherent functions name
identically to the `GeneralHash` trait methods.
Rename the trait to describe better what it does.
Internal change only.
The `from_engine` function is associated with a general hash type but we
are defining it in the `hash_type` macro which holds nothing but
functions associated with the `Hash` trait. By "associated" I mean
methods on the type as opposed to trait methods.
In preparation for re-naming the `hash_type` macro move the
`from_engine` function there. Requires duplicating the code in the
`siphash` impl block, this is as expected because the `siphash` requires
a custom implementation of the general hashing functionality.
Internal change only.
We use `TBD` in our `deprecated` string and it was discovered that there
is an exception on this string so as not to warn because it is used
internally by the Rust language. However there is a special lint to
enable warnings, lets use it.
Add `#![warn(deprecated_in_future)]` to the coding conventions section
of all crates except `fuzz`.
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
In preparation for releasing `io v0.2.0` bump the version number,
add a changelog entry, update the lock files, and depend on the new
version in all crates that depend on `io`.
be94c9e54b Rename Midstate::into_parts to Midstate::to_parts since it derives Copy (Shing Him Ng)
Pull request description:
This is a follow up to #3010Fixes#3079
ACKs for top commit:
tcharding:
ACK be94c9e54b
apoelstra:
ACK be94c9e54b successfully ran local tests
Tree-SHA512: 0d838a2064136d050d319116f6df3d598323b04a137e7bb7cb5f3f1a87d72ad1ee4d2f3b228a2f9d68e7ca117c0f922ef6551f783eb39c8db0db1188e4732c41
8bb0d3f667 Fix buggy cfg in rustdocs (Tobin C. Harding)
Pull request description:
In b9643bf3e9 we introduced an incorrect `cfg` attribute, that has just shown up, no clue why clippy only just presented me with this error now. Anywho, the current code is buggy and the rustdoc tests are never being run.
Fix `cfg` attribute to use the feature name correctly and fix the imports so the code runs.
Maintain the explicit `main` so that we can return an error using the `?` operator. Remove the empty `main` because its not needed anymore, it is a hang-over from Rust back in the day (before main was automatically added, IIUC).
ACKs for top commit:
apoelstra:
ACK 8bb0d3f667 successfully ran local tests
Tree-SHA512: 27f571ac3644417c06d0b4eb6fb122b39ac1068aefa4bcfc03f1febe2d031fb30616883c55c42c2ec80d419572fe7eba9bcc239e3c0e0e178ec7eaf8533b9efe
In b9643bf3e9 we introduced an incorrect
`cfg` attribute, that has just shown up, no clue why clippy only just
presented me with this error now. Anywho, the current code is buggy and
the rustdoc tests are never being run.
Fix `cfg` attribute to use the feature name correctly and fix the
imports so the code runs.
Maintain the explicit `main` so that we can return an error using the
`?` operator. Remove the empty `main` because its not needed anymore,
it is a hang-over from Rust back in the day (before main was
automatically added, IIUC).
8b9d657667 hashes: Remove unused file (Tobin C. Harding)
Pull request description:
Recently we moved the tests from `impls` to `tests/io.rs` but forgot to delete the original file.
Remove the already unused `impls.rs` file.
ACKs for top commit:
apoelstra:
ACK 8b9d657667 successfully ran local tests
Tree-SHA512: f701825239aff18faa0cd7adf69e1a8830dfb532df9d52c6d545e6352054ae6cb8c625c9fc05b7db374822d3478acf16858bc38ffbecf45575f555eb80f010e1
58704c2eff Remove schemars all together (Tobin C. Harding)
Pull request description:
We introduced schemars as a personal favor to a user, and it broke our CI repeatedly but eventually it seemed like it was stable (mainly, our MSRV caught up with its MSRV) so we just let it slide. In the end having schemars on hashes but nowhere else in the rust-bitcoin ecosystem did not prove that useful.
Remove schemars all together.
Fix: #3393
ACKs for top commit:
apoelstra:
ACK 58704c2eff successfully ran local tests
Tree-SHA512: 11c136797f28903c7d6b5199ad55d86bc4bc29ee8dd6f0d575e029f4dbebebabed57ebce6cf773b286297ea84f18d0b6cc58e150299e99457e048226478b49cc
cbfddb0394 hashes: Rename length field and use u64 (Tobin C. Harding)
Pull request description:
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
ACKs for top commit:
apoelstra:
ACK cbfddb0394 successfully ran local tests
Tree-SHA512: a9d932938afcbd6dfb9db471a02fa7e3fff8f0659906627001ad241390b9af57088fd34afeae551c70c2c49783e6296f110b57ff9de6fed2609f4648ec8fd934
We introduced schemars as a personal favor to a user, and it broke our
CI repeatedly but eventually it seemed like it was stable (mainly, our
MSRV caught up with its MSRV) so we just let it slide. In the end having
schemars on hashes but nowhere else in the rust-bitcoin ecosystem did
not prove that useful.
Remove schemars all together.
Fix: #3393
f6abdcc001 Allow unused in `macros.rs` docs (Jamil Lambert, PhD)
fd89ddf401 Remove or fix unused variables and methods in docs (Jamil Lambert, PhD)
ff6b1d4f19 Remove unused variables and methods from docs (Jamil Lambert, PhD)
e58cda6f92 Remove `unused_imports` in docs (Jamil Lambert, PhD)
Pull request description:
As mentioned in #3362 examples in documentation are not linted in the same way as other code, but should still contain correctly written code.
#![doc(test(attr(warn(unused))))] has been added to all lib.rs files
In the docs throughout all crates:
- Unused imports have been removed.
- Unused variables, structs and enums have been used e.g. with an `assert_eq!` or prefixed with `_`
- Unused methods have been called in the example code.
ACKs for top commit:
tcharding:
ACK f6abdcc001
apoelstra:
ACK f6abdcc001 successfully ran local tests
Tree-SHA512: c3de1775ecde6971056e9fed2c9fa1621785787a6a6ccbf3a6dbd11e18d42d4956949f3f8adfc75d94fd25db998b04adb1c346cc2c2ba47f4dc37402e1388277
Examples in documentation are not linted in the same way as other code,
but should still contain correctly written code.
Throughout all of the crates except internals (another commit) unused
variables have been prefixed with `_`, unused imports have been removed,
and a warn attribute added to all of the `lib.rs` files.
25d906d936 Use UFCS in macros (Liam Aharon)
Pull request description:
Closes#3304
ACKs for top commit:
apoelstra:
ACK 25d906d936 successfully ran local tests
tcharding:
ACK 25d906d936
Tree-SHA512: a02a8507bcec73dab1ec8d49e45eab327d989c276d931520e0fff312faf7d3165292c300fb9414314f9b94b1255ee49a0fb64303df6d971c9089226b6873c36a
With a recent nightly toolchain `clippy` gives us an error:
error: missing documentation for a constant
I'm not sure why the error is emitted but wrapping the function in a
`tests` module as is standard practice clears the error.
bd8ad1f5e2 Add basic `miri` checks (Martin Habovstiak)
fb5971cc2b Fix UB in `siphash24` (Martin Habovstiak)
Pull request description:
We have a bit of `unsafe` code in the crates which should really be checked with `miri`. Thus this adds a basic CI check that automatically determines which crates need `miri` checking and checks them. It also makes sure to enable all target features so that SIMD code can be checked as well.
This doesn't try to do anything fancy with maintainer tools or run task for now, since I just want to test the basic idea.
Closes#3192
ACKs for top commit:
storopoli:
ACK bd8ad1f5e2
tcharding:
ACK bd8ad1f5e2
sanket1729:
ACK bd8ad1f5e2
apoelstra:
ACK bd8ad1f5e2 successfully ran local tests; wow, good find!
Tree-SHA512: a0d33c7851d6d6b288ca8cc1a902f187814dd82e3528c6f8169fdc0ba71991b99451276aaba5e3b6cde6029e09158063d65e48a71d1e01ee20302b9f653584ef
ae93e226e3 Remove hashes io feature (Tobin C. Harding)
Pull request description:
Currently we only get `std::io::Write` impls when the `bitcoin-io` dependency is used. This is overly restrictive, it would be nice to have `std::io::Write` imlps even without the `bitcoin-io` dependency.
Copy the logic out of the `bitcoin_io::impl_write` macro into `hashes` but feature gate it differently.
Call the new macro inside `hash_type` (and in `hmac`), remove the `impls` module, and move the tests to the integration test directory.
Remove the `io` feature from `hashes`, now if users enable `std` they get `std::io::Write` impls and if they enable `bitcoin-io` they get `bitcoin_io::Write` impls as well.
ACKs for top commit:
Kixunil:
ACK ae93e226e3
apoelstra:
ACK ae93e226e3 successfully ran local tests
Tree-SHA512: d47c9c060750e8a024c46cbf7afe8d0d1245fa1f5e575f36b3a11e2460d3620ad9def1a6331dafe77d46affc99b043ec9679e619ce8ddfa32436a5826ece09e4
fe46225ed0 Allow unused imports when running bench code (Tobin C. Harding)
eb67e873e0 Allow unused variables in release mode (Tobin C. Harding)
Pull request description:
Two patches to clear the million warnings when running the bench code.
ACKs for top commit:
apoelstra:
ACK fe46225ed0 successfully ran local tests; though in the first commit you could also use `cfg_attr` FWIW
Kixunil:
ACK fe46225ed0
Tree-SHA512: 3f705e0441d8c0e41e9ceb5473572810ff2513f7e5531c1b7889418a3a85ac8622e50e271c7a3b5c386fb3f5629b85d4bd79739c4a02b51d58da86890721d8d2
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
Currently we only get `std::io::Write` impls when the `bitcoin-io`
dependency is used. This is overly restrictive, it would be nice to have
`std::io::Write` imlps even without the `bitcoin-io` dependency.
Copy the logic out of the `bitcoin_io::impl_write` macro into `hashes`
but feature gate it differently.
Call the new macro inside `hash_type` (and in `hmac`), remove the
`impls` module, and move the tests to the integration test directory.
Remove the `io` feature from `hashes`, now if users enable `std` they
get `std::io::Write` impls and if they enable `bitcoin-io` they get
`bitcoin_io::Write` impls as well.
The code in `siphash24` was obtaining the pointer in buffer at offset by
accessing an element at that offset instead of accessing a range or
simply computing the offset of the pointer from the start. This is UB
because one canot access past `T` even if the allocation is known to be
large enough. This change fixes it by using a range and also replaces
complicated code with simpler use of `from_le_bytes`.
It's quite likely that this can be improved further, possibly even
removing the `unsafe` without speed penalty but it's a larger task
that's not a priority right now.
d72f730211 hashes: Use $crate in internal macros (Tobin C. Harding)
Pull request description:
These are only called from within the crate but it is still more correct to use `$crate` and saves this from biting us later if we copy the code someplace else.
Internal change only.
ACKs for top commit:
Kixunil:
ACK d72f730211
apoelstra:
ACK d72f730211 successfully ran local tests
Tree-SHA512: d278643c3fbeb28ca377ebf59958054dd2893c46b48469e03a8c7517c5b0b33271de061ae662c400d45962724fe4d13cada41fd5b839a1ff784521ac69c9db72
Support for Rust arrays is now much better so slice-accepting
methods that require a fixed length can be replaced with a method that
accepts an array.
`from_slice()` has been deprecated. A `from_byte_array()` function
already exists to be used instead.
These are only called from within the crate but it is still more correct
to use `$crate` and saves this from biting us later if we copy the code
someplace else.
Internal change only.
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
Use the newly added requirement that `Hash::Bytes` is an array to
protect the implementation of hmac and hkdf against implementations that
would accidentally return slices of different sizes from the `AsRef`
impl.
Jamil Lambert, PhD
merge-script
Tobin C. Harding
Fmt Bot
Shing Him Ng
Liam Aharon
Martin Habovstiak