Remove the `GeneralHash` trait by tcharding · Pull Request #4085 · rust-bitcoin/rust-bitcoin

tcharding · 2025-02-20T03:02:52Z

This is the done as part of #4051.

Requires some surgery on the Hmac and Hkdf types as well as a few other patches to maintain the logic that is currently provided by the trait. Final patch is a pure red diff - enjoy.

github-actions · 2025-02-20T03:04:09Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

tcharding · 2025-02-20T03:06:46Z

In case its not obvious, the regression test in patch one is to prove that the change to hash_again in 82bb884 is correct.

github-actions · 2025-02-20T03:07:14Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

github-actions · 2025-02-20T03:18:04Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

Kixunil · 2025-02-20T10:06:35Z

hashes/src/lib.rs

 /// A hashing engine which bytes can be serialized into.
 pub trait HashEngine: Clone {
+    /// The `Hash` type returned when finalizing this engine.
+    type Hash: Hash;


If you're going to mess with the traits here it should be GeneralHash (but I think it shouldn't be required anyway).

GeneralHash is folded into Hash and deleted here.

As for why do we need the bound -- I tried removing it and it broke the HmacEngine which wants to be able to finalize its inner engine (obtaining a HashEngine::Hash type) and then call .as_ref to convert it to bytes so that it can iterate over it and do some xor magic.

I think we should leave this be and revisit this in a followup, since it's a nontrivial change.

IIRC I've originally suggested AsRef<[u8]> bound which should work for the vast majority of use cases (but I wouldn't mind also having Borrow, Copy and equality traits).

Can we merge as is and follow up? I did try various things yesterday and couldn't get anything nicer to work.

I could be wrong because every experiment requires loads of boring changes but I definitely tried AsRef<[u8]> trait bound.

I'm not really sure if we should merge it in this state. The code before was not horribly broken, so clean history has value and the changes I have in mind touch a significant chunk of code, I think.

I think we should merge it. This gets rid of the GeneralHash trait and greatly reduces the amount of stuff we need to think about (we have two interacting traits, Hash and HashEngine, instead of three, and it seems like we're moving toward also getting rid of Hash.)

Even if this means that we have a series of big diffs, conceptually it'll be easier to follow removing the big GeneralHash trait in one go, then reducing or removing Hash in another go.

OK, I think I'll be able to review properly tomorrow.

Gonna queue this for merge.

Crap, so many things slipping by

apoelstra · 2025-02-20T16:02:47Z

caac660 looks great! Started local CI but it might be many hours before it finishes, since today there are many 5+-commit PRs for some reason.

To do in followups:

Revisit whether we need the Hash bound on HmacEngine::Hash or whether something weaker (AsRef<Self::Bytes>, or, somehow, nothing)
Revisit the Bytes type and see if we can't replace it with an associated constant (I remember we couldn't before, but things have changed now)
Revisit which functions we should add to hmac and hkdf which are skipped in this PR since they're missing in the existing API

Kixunil · 2025-02-20T18:20:48Z

Revisit the Bytes type and see if we can't replace it with an associated constant (I remember we couldn't before, but things have changed now)

I think it should be possible to remove it entirely.

tcharding · 2025-02-20T23:43:46Z

Promoted to #4094

apoelstra · 2025-02-21T17:42:59Z

I got 18 hours into this and was 1/3 done. I stopped the build to defragment my nix store and my btrfs extents and will restart in a bit.

The timing here is pretty bad ... I am using btrfs over SATA on a spinning disk drive for my Nix store. I have a new PCIe drive with ext4 which should be 10-1000x faster, but I can't cut over to it when I'm remote ... and I left town yesterday for a couple weeks. I shouldn't have kept procrastinating on it..

tcharding · 2025-02-21T21:02:40Z

We can just queue up drafts on top of this for any hashes PRs till you are back home. I'm not stressing about hashes at the moment because amounts is taking so long.

apoelstra · 2025-02-22T03:52:51Z

Compliing with features hex serde and nothing else seems to fail on one commit.

apoelstra · 2025-02-22T14:16:42Z

In ed6fa5a (the very first commit):

Your new regression test doesn't compile (yet). If you cd into hashes/ on this commit and run cargo test --all-features it fails.

tcharding · 2025-02-22T20:56:15Z

CI should not pass then, I'll investigate. Thanks

apoelstra · 2025-02-23T15:40:53Z

@tcharding it does compile by the last commit, so I'm not too surprised that CI passes since it tests only the tip.

It looks like you added the regression then reordered the commits but didn't tweak it, or something.

tcharding · 2025-02-23T20:28:27Z

Oh woops, cheers. Will fix.

github-actions · 2025-02-25T02:21:59Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

apoelstra

ACK 820a1b7; successfully ran local tests

Add a simple regression test prior to patching the `sha256::Hash::hash_again` function.

Add an associated const `Hash` to the `HashEngine` trait. Also add a `finalize` method that converts the engine to the associated hash. For now just use the existent `from_engine` stuff. We can refactor later.

We would like to do away with the `GeneralHash` trait. Currently we bound `Hmac` and `HmacEngine` on it but this is unnecessary now that we have added `HashEngine::finalize` and `HashEngine::Hash`. Bound the `HmacEngine` on `HashEngine` (which has an associated `Hash` type returned by `finilalize`). Bound `Hmac` type on `T::Hash` where `T` is `HashEngine`. Includes some minor shortening of local variable names around hmac engine usage. Note this means that `Hmac` no longer implements `GeneralHash`.

Add a standalone `hash` function that is a drop in replacement for `GeneralHash::hash`. Do not add it to `hmac` - this is in parity with the current code because `Hmac` does not implement `GeneralHash::hash`. Use the new function in `bitcoin` removing all occurrences of `GeneralHash` from `bitcoin`. In `hashes` replace usage of `GeneralHash::hash` with the new `hash` function.

Add a standalone `hash_byte_chunks` function that is a drop in replacement for `GeneralHash::hash_byte_chunks`. Do not add it to `hmac` - this is in parity with the current code because `Hmac` does not implement `GeneralHash::hash_byte_chunks`.

We would like to remove the `GeneralHash` trait but we want to keep the `hash_reader` functionality. Add a stand alone function (when `hashes` is enabled) `hash_reader`. Remove the extension trait.

This import is not needed. Interesting that the linter does not catch it.

Now that we are able to unambiguously go from a hash engine to its associated hash type there is no longer any need for the `GeneralHash` trait. Please note that IMO this concept of a general hash type as opposed to one where one can hash arbitrary data still exists in the codebase - it is implicitly in the `hash_newtype` macro. Remove the `GeneralHash` trait.

github-actions · 2025-03-06T00:51:20Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

tcharding · 2025-03-06T00:51:25Z

Rebased. Also did a couple more changes making local variable more terse like I did already in other places before hand - I think.

tcharding · 2025-03-06T00:58:20Z

Note to self: Soon as this lands rebase #4017.

Kixunil

ACK 95ad91c

I'd strongly prefer to drop the commit adding the functions we want to deprecate but I'm not going to block the PR over it.

Kixunil · 2025-03-07T12:32:21Z

hashes/src/internal_macros.rs

+            }
+            engine.finalize()
+        }
+


I'd like to see this commit dropped if you can be bothered.

I guess you mean 2b6ef31.

Let's promote this to an issue (or just a comment on #4205). I'm 8 hours into a 9-hour local CI run on this.

I agree it'd be better if this function hadn't existed to begin with, but we've gotta deprecate some copies anyway in #4205 so it won't be too bad to also delete this one.

OK, ~~if the CI succeeds and~~ no other problem will be found let's keep it. But it the PR will need to be modified please drop it.

LOL, didn't notice this was already 1h ago :D

Kixunil · 2025-03-07T12:34:36Z

hashes/src/hash160/mod.rs

 mod benches {
    use test::Bencher;

-    use crate::{hash160, GeneralHash as _, Hash as _, HashEngine};


I believe that the reason linter doesn't catch it is that we're calling a function defined on it that's also inherent function and the linter doesn't know they behave the same.

Kixunil · 2025-03-07T12:37:34Z

hashes/src/internal_macros.rs

-
-            fn from_engine(e: Self::Engine) -> Hash<$($gen),*> { Self::from_engine(e) }
-        }
-


Th commit message: "concept of a general hash type as opposed to one where one can hash arbitrary data still exists in the codebase"

is funny, arbitrary data = general hash type, I think you meant "as opposed to hash that only hashes specific kinds of data". And yes, general hash type exists as a concept but its' expressed with HashEngine instead.

In 95ad91c:

Agreed, the commit message should be improved. It's the last commit in the PR so it'd be cheap for me to re-run if @tcharding wants to change it. But IMO it's not a big enough deal to hold up the PR over.

apoelstra

ACK 95ad91c; successfully ran local tests

The GeneralHash trait was removed (see CHANGELOG rust-bitcoin#4085), but internal_macros.rs still referenced it in macro docs. Updated hash_trait_impls! and general_hash_type! documentation to reflect actual current requirements

31a07f1 docs(hashes): replace deprecated GeneralHash references (Bashmunta) Pull request description: The GeneralHash trait was removed (hashes/CHANGELOG.md #4085), but internal_macros.rs still referenced it in docs. Updated hash_trait_impls! and general_hash_type! documentation to reflect actual current requirements ACKs for top commit: apoelstra: ACK 31a07f1; successfully ran local tests tcharding: ACK 31a07f1 Tree-SHA512: 8d407a05767f0209f447d0f3923aeb4f5528c144c6a234b35721412d9e50ba8b24a950ff09fadaf6603ce63c75ed14c1b85480caf516db08b879c4e6378c8cbf

github-actions bot added C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate C-io PRs modifying the io crate test labels Feb 20, 2025

github-actions bot added the API break This PR requires a version bump for the next release label Feb 20, 2025

tcharding force-pushed the 02-20-remove-general-hash branch from 022a5c7 to 7038ca3 Compare February 20, 2025 03:04

tcharding force-pushed the 02-20-remove-general-hash branch from 7038ca3 to caac660 Compare February 20, 2025 03:11

tcharding mentioned this pull request Feb 20, 2025

hashes: Add an api test module #4017

Merged

Kixunil reviewed Feb 20, 2025

View reviewed changes

tcharding mentioned this pull request Feb 20, 2025

Improve HashEngine trait #4094

Open

tcharding force-pushed the 02-20-remove-general-hash branch from caac660 to 820a1b7 Compare February 25, 2025 02:20

apoelstra previously approved these changes Feb 26, 2025

View reviewed changes

tcharding added 3 commits March 6, 2025 11:43

Add hash_again regression test

84623ff

Add a simple regression test prior to patching the `sha256::Hash::hash_again` function.

Add Hash type and finalize method to HashEngine

ab63b7a

Add an associated const `Hash` to the `HashEngine` trait. Also add a `finalize` method that converts the engine to the associated hash. For now just use the existent `from_engine` stuff. We can refactor later.

tcharding added 5 commits March 6, 2025 11:47

io: Use function in place of GeneralHashExt

791501e

We would like to remove the `GeneralHash` trait but we want to keep the `hash_reader` functionality. Add a stand alone function (when `hashes` is enabled) `hash_reader`. Remove the extension trait.

Remove unused trait import

6426e59

This import is not needed. Interesting that the linter does not catch it.

tcharding dismissed apoelstra’s stale review via 95ad91c March 6, 2025 00:49

tcharding force-pushed the 02-20-remove-general-hash branch from 820a1b7 to 95ad91c Compare March 6, 2025 00:49

Kixunil approved these changes Mar 7, 2025

View reviewed changes

apoelstra mentioned this pull request Mar 7, 2025

Improve Hmac API #4205

Closed

apoelstra approved these changes Mar 7, 2025

View reviewed changes

apoelstra merged commit 5581c49 into rust-bitcoin:master Mar 7, 2025
24 checks passed

tcharding deleted the 02-20-remove-general-hash branch March 9, 2025 21:59

Bashmunta mentioned this pull request Nov 29, 2025

docs(hashes): replace deprecated GeneralHash references #5347

Merged


		fn from_engine(e: Self::Engine) -> Hash<$($gen),*> { Self::from_engine(e) }
		}

Conversation

tcharding commented Feb 20, 2025

Uh oh!

github-actions bot commented Feb 20, 2025

Uh oh!

tcharding commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 20, 2025

Uh oh!

github-actions bot commented Feb 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Feb 20, 2025

Uh oh!

Kixunil commented Feb 20, 2025

Uh oh!

tcharding commented Feb 20, 2025

Uh oh!

apoelstra commented Feb 21, 2025

Uh oh!

tcharding commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Feb 22, 2025

Uh oh!

apoelstra commented Feb 22, 2025

Uh oh!

tcharding commented Feb 22, 2025

Uh oh!

apoelstra commented Feb 23, 2025

Uh oh!

tcharding commented Feb 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 25, 2025

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 6, 2025

Uh oh!

tcharding commented Mar 6, 2025

Uh oh!

tcharding commented Mar 6, 2025

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Feb 20, 2025 •

edited

Loading

tcharding commented Feb 21, 2025 •

edited

Loading

tcharding commented Feb 23, 2025 •

edited

Loading

Kixunil Mar 7, 2025 •

edited

Loading