Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add inherent functions to hashes #2852

Merged

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Jun 11, 2024

Currently we have a trait Hash that is required for Hmac, Hkdf, and other use cases. However, it is unegonomic for users who just want to do a simple hash to have to import the trait.

Add inherent functions to all hash types including those created with the new wrapper type macros.

This patch introduces some duplicate code but we are trying to make progress in the hashes API re-write. We can come back and de-dublicate later.

Includes making to_byte_array,from_byte_array, as_byte_array, and all_zeros const where easily possible.

@github-actions github-actions bot added C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate test C-base58 labels Jun 11, 2024
@tcharding tcharding force-pushed the 06-11-hashes-inherent-functions branch from f317d82 to 9dc4bd6 Compare June 11, 2024 03:03
@apoelstra
Copy link
Member

concept ACK 9dc4bd68d19de94bf1058234ee9b5506c65e0a49. This looks great. I think we should make as_byte_array and to_byte_array const as well. (Same with the engine stuff but that's nontrivial since we'd need to do a similar transformation of HashEngine and probably rewrite some loop constructs. So that can be a later low-priority thing.)

I also think we should drop all_zeros, but that also belongs in a separate PR because I think the goal with this one is that it's "refactor-only" and does not reduce the API at all.

@tcharding
Copy link
Member Author

Yeah, I'm trying to do it in basic steps but it totally flies against normal dev process of patching things into an unwanted state.

@apoelstra
Copy link
Member

Unfortunately in Rust everything is so tightly coupled that it's hard to make significant changes without either doing unreviewable full-rewrite PRs like 2770 (which then don't even work because the language is limited..) or having unwanted states.

@tcharding
Copy link
Member Author

On that note, I'm stumped on how we are supposed to do the primitives PR, will comment over there though so discussion doesn't start here.

@tcharding tcharding force-pushed the 06-11-hashes-inherent-functions branch from 9dc4bd6 to bb037d4 Compare June 11, 2024 23:08
@tcharding tcharding marked this pull request as ready for review June 11, 2024 23:09
@apoelstra
Copy link
Member

ACK bb037d4cabbe807b65990fd7c5048b6c372df8fc except that I think we should make the two *_byte_array methods const.

Currently we have a trait `Hash` that is required for `Hmac`, `Hkdf`,
and other use cases. However, it is unegonomic for users who just want
to do a simple hash to have to import the trait.

Add inherent functions to all hash types including those created with
the new wrapper type macros.

This patch introduces some duplicate code but we are trying to make
progress in the hashes API re-write. We can come back and de-dublicate
later.

Includes making `to_byte_array`,`from_byte_array`, `as_byte_array`, and
`all_zeros` const where easily possible.
@tcharding tcharding force-pushed the 06-11-hashes-inherent-functions branch from bb037d4 to 63b584b Compare June 14, 2024 00:18
@tcharding tcharding force-pushed the 06-11-hashes-inherent-functions branch from 63b584b to 18b2788 Compare June 14, 2024 00:51
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 18b2788

@apoelstra apoelstra merged commit 72ce271 into rust-bitcoin:master Jun 14, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-base58 C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants