Make sha256t_hash_newtype! evocative of the output.
#1773
Merged
Conversation
This file contains 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
Computing hashes in const fn is useful for esily creating tags for `sha256t`. This adds `const fn` implementation for `sha256::Hash` and the algorithm for computing midstate of tagged hash in `const` context. Part of rust-bitcoin#1427
The Rust API guidelines state that macros should be evocative of the
output, which is a sensible recommendation. We already had this for
`hash_newtype!` macro but didn't for sha256t version.
This changes the macro to have this syntax:
```rust
sha256t_hash_newtype! {
// Order of these structs is fixed.
/// Optional documentation details here. Summary is auto-generated.
/*pub*/ struct Tag = raw(MIDSTATE_BYTES, LEN);
/// Documentation here
#[hash_newtype(forward)] // optional, default is backward
/*pub*/ struct HashType(/* attributes allowed here */ _);
}
```
Closes rust-bitcoin#1427
Previous changes enabled passing the string used as a tag into `sha256t_hash_newtype!` macro rather than hard-coding midstate. This commit takes advantage of it and replaces the hard-coded values with compile-time executed (`const`) hashing.
Kixunil
force-pushed
the
evocative-sha256t
branch
from
f0c0991
to
91f45a2
Compare
tcharding
approved these changes
Apr 3, 2023
|
Cool. |
|
I think #1758 will fix the CI fail. |
|
Looks good to me. I kinda think they should be renamed as
But I'm not gonna hold up the PR over these. |
apoelstra
approved these changes
Apr 3, 2023
|
LOL, you merged it already. OK I guess. :) Regarding naming, all of them are IVs, so telling what's specific about them seems better. Mentioning BIP-340 sounds good. Byte version was added just in case something comes up in the future. Maybe just |
|
@Kixunil Yes, yes, sorry for going ahead and merging -- the PR seemed reasonably large and conflict-prone, and the naming unimportant enough that it could get slipped, that I figured it'd be nicer to do the renaming (if we decided to) in a different PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The Rust API guidelines state that macros should be evocative of the
output, which is a sensible recommendation. We already had this for
hash_newtype!macro but didn't for sha256t version.This changes the macro to have this syntax:
Closes #1427
Depends on #1769
How do you like the syntax? Is weird
struct Foo = bar(..);acceptable?