hashes: Fix a bunch of bugs caused by broken CI script#3200
Merged
apoelstra merged 8 commits intorust-bitcoin:masterfrom Aug 22, 2024
Merged
hashes: Fix a bunch of bugs caused by broken CI script#3200apoelstra merged 8 commits intorust-bitcoin:masterfrom
apoelstra merged 8 commits intorust-bitcoin:masterfrom
Conversation
The `hash_reader` function is new and unreleased, it should never have been put into the `sha256t_hash_newtype` macro, and its broken.
The `hash_reader` function is only available when `bitcoin-io` is enabled - it should be feature gated.
We lost this impl at some stage in the last few weeks and did not notice because of a bug in `run_task` [0]. Implement `JsonSchema` for `siphash` as we do for all the other hash types. [0] rust-bitcoin/rust-bitcoin-maintainer-tools#10
In rust-bitcoin#3010 we added a `length` field to the `sha256::Midstate` which broke the `schemars` test but we did not notice because of the current bug [0] in the `run_task` CI script. [0] rust-bitcoin/rust-bitcoin-maintainer-tools#10
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
d303af1 to
01ce7e6
Compare
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.
The `serde` impls and `FromStr` are missing from `siphash`, add them.
There is a bug in `run_task` [0] that drastically reduces our test coverage. We can bypass it by putting `fuzz` last in the list. [0] rust-bitcoin/rust-bitcoin-maintainer-tools#10
01ce7e6 to
009e747
Compare
Closed
Member
|
Hmm, I was not expecting that to pass since I enabled the |
Member
|
Oof, I had disabled testing non-default features on this repo at some point between June 8 and Aug 10, and did not comment a reason or anything. Fixed. |
Closed
Contributor
|
This fixed the test failure for me. |
Closed
Member
Author
|
I created #3206 in case its easier for you to merge as a single PR. |
Kixunil
approved these changes
Aug 22, 2024
| })); | ||
| schema.into() | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
I'd rather fix it by splitting the macro since that will just blanket-solve everything related to that problem.
| ``` | ||
| To run the tests with the current MSRV you will need to pin some | ||
| dependencies. See the `hashes/contrib/extra_tests.sh` script for the | ||
| current list of pins. |
Collaborator
There was a problem hiding this comment.
A declarative solution would be nice.
| fn as_byte_array(&self) -> &Self::Bytes { self.as_byte_array() } | ||
|
|
||
| fn from_byte_array(bytes: Self::Bytes) -> Self { Hash(bytes) } | ||
| fn from_byte_array(bytes: Self::Bytes) -> Self { Self::from_byte_array(bytes) } |
Collaborator
There was a problem hiding this comment.
Again, this is caused by not using the macro.
apoelstra
added a commit
that referenced
this pull request
Aug 23, 2024
d03e545 ci: fix `rev` to be `ref` when pinning bitcoin-maintainer-tools (Andrew Poelstra) d04b6aa bitcoin: add a couple missing prelude imports (Andrew Poelstra) 30f6bd4 Add primitives to the CRATES list (Andrew Poelstra) e1e0230 primitives: Enable alloc from serde (Tobin C. Harding) ce36a37 primitives: Test ordered feature (Tobin C. Harding) Pull request description: This is a rebase of #3195 on master (since #3200 was merged) and adds one commit which fixes a few missing imports, which it appears was missed in the other "fix CI" PRs. ACKs for top commit: Kixunil: ACK d03e545 tcharding: ACK d03e545 Tree-SHA512: 6f5dc5c2cbb42716e83e46e70dbf4d644f9e80f6b3d2e75a276e37d33d461f4a86e67eb5c32444dbd23bb9767abedc88bfe958b1beba39852c382cf73736711f
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is just the bug fixes pulled out of #2861
The root problem here is the bug described in rust-bitcoin/rust-bitcoin-maintainer-tools#10
This PR works around the bug by putting
fuzzlast in the list.