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.
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.
dae42bef9d do not enable bitcoin-io by default (Antoni Spaanderman)
a14cdaf859 don't enable std by default when testing (Antoni Spaanderman)
e83830dcfc use slice instead of array to not have to hardcode the length (Antoni Spaanderman)
55749d6f61 use `hash.to_byte_array` to check equality with `test.output` (Antoni Spaanderman)
969864e3b0 use fixed size array if possible, otherwise `&'static [u8]` (Antoni Spaanderman)
28ccf70fa6 remove unnecesarry borrow operator (`&`) (Antoni Spaanderman)
fa3a3afd02 remove unnecessary slicing (Antoni Spaanderman)
22e42ab86c fix test code being unnecessarily feature gated (Antoni Spaanderman)
Pull request description:
- remove 2 unnecessary cfg attributes from tests left over from #3167 (it made them not dependent on `alloc` anymore)
- simplify assertion logic by removing unnecessary conversions before comparing
- make tests `no_std` compatible by adding imports to alloc or std
- feature gate tests behind the `alloc` feature if they use anything from the alloc crate (like the `format!` macro)
- `schemars` feature enables `alloc` because (for example) its trait wants implementations to return `String`
- fix `bitcoin-io` always enabling when `std` is enabled (only useful if people depend on `hashes` only, `bitcoin` depends on `bitcoin-io` already)
ACKs for top commit:
tcharding:
ACK dae42bef9d
Kixunil:
ACK dae42bef9d
apoelstra:
ACK dae42bef9d successfully ran local tests
Tree-SHA512: 622fd4963ef21530a98af89bcfc71abe8723aac12d363ab88d9bd30dcf2f75392711bec10e2901fab5f1a30e11897d1aae36e22892738aa1e5670166f91fddd4
`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.
- 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
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
merge-script
Tobin C. Harding
Jamil Lambert, PhD
Fmt Bot
Liam Aharon
Martin Habovstiak
Antoni Spaanderman
leichak