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

Implement computing SHA256 in const context #1769

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

Kixunil
Copy link
Collaborator

@Kixunil Kixunil commented Apr 1, 2023

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 #1427

I'd like to make sha256t evocative of the output after this which would also do the thing we want to do with #1427 (awkward language to bypass GH automatically closing it...) I'm thinking of this:

sha256t_newtype_hash! {
// optional attrs
/*pub*/ struct TagName = "tag_str"; // weird syntax but I guess OK-ish?

// optional attrs
/*pub*/ struct HashName(_); // allowed to elide the type because it's implied; maybe also allow writing it?
}

What do you think? I was also thinking about this alternative:

sha256t_newtype_hash! {
// optional attrs
/*pub*/ struct TagName;

const TAG: Tag = from_str("tag_str"); // allows for having from_raw([...]) or even from_hex([...])

// optional attrs
/*pub*/ struct HashName(_); // allowed to elide the type because it's implied; maybe also allow writing it?
}

Which looks more natural but maybe too much boilerplate? Which one do you like? Any better ideas? Heads up sanket1729

Also FYI tcharding this is the kind of post-1.48 change I was thinking about. :)

@apoelstra
Copy link
Member

The integration test "failures" look like a series of annoying warnings that we've promoted to errors.

concept ACK this (and will give a real ACK soon). Are the warnings about efficiency purely because we use while loops instead of iterators? Because I expect the compiler can handle a i = 10; while(i--) { .. } loop just as well as any iterator-based code.

As for the question about macro syntax, I prefer the from_str(...) variant since it's more extensible, as you say.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Apr 1, 2023

I don't see any loop errors, just operator precedence. I think I'll #[allow] it.

We could also have struct Tag = hash_str("tag_str"); to decrease boilerplate. (hash_str seems better than from_str).

@Kixunil
Copy link
Collaborator Author

Kixunil commented Apr 1, 2023

Oh, now I understood your question about loops. 🤦‍♂️ Yes, and you are probably right, I tried some stuff in godbolt and looks good. However there's still a reason to consider it: we may move to use native CPU instructions. There are discussions about conditionally running different code in const and non-const (in fact an unstable intrinsic already exists) but those seem far in the future.

Although honestly I'm kinda tempted to do it and thus enable a bunch of things in bitcoin to be const.

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
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 42d3ae0

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 42d3ae0

@apoelstra apoelstra merged commit cd9f706 into rust-bitcoin:master Apr 3, 2023
apoelstra added a commit that referenced this pull request Apr 3, 2023
91f45a2 Replace hardcoded values with compile-time hashing (Martin Habovstiak)
095b795 Make `sha256t_hash_newtype!` evocative of the output. (Martin Habovstiak)

Pull request description:

  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 #1427

  Depends on #1769

  How do you like the syntax? Is weird `struct Foo = bar(..);` acceptable?

ACKs for top commit:
  tcharding:
    ACK 91f45a2
  apoelstra:
    ACK 91f45a2

Tree-SHA512: f6b555b20311a4c1cb097bc296c92459f6fbe16ba102d8c977eb2383dbcae3cc8ffce32e3cb771e7e22197fda26379971aa4feaadc5e2e70d37fa690ae8b8859
@Kixunil Kixunil deleted the const_fn_sha256 branch April 4, 2023 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants