Conversation
|
Concept ACK |
|
concept ACK from me too. As a PR this could be split up into multiple commits. I guess in the bitcoin_hashes README we should add a blurb explaining that the "standard" way to use this is to define newtypes for the actual hashes in your project, and implement constructors for those which use raw hashes internally. But we should also provide top-level functions in that library for directly producing all the raw hashes, because that's often what people want when they just need to one-off hash some data. |
|
Whats the status of this @Kixunil? |
|
This got burned by the dependency hole but since ranged versions fixes it I'd like to take a look at this soon. |
The `Hash` trait requires for the hash type to support hashing arbitrary data. This is not great, since hash newtypes usually hash specific data. Hashing arbitrary data into newtypes is usually a bug or misunderstanding of the API. This change separates methods performing hashing of arbitrary data to a separate `RawHash` trait and provides a few helpers to make usage easier: * adds a *private* `engine()` method to the hash newtype * adds `finalize()` method to `HashEngine` * adds `hashes::hash` function Additionally, because of visibility issues, this moves many hash newtypes from the dedicated `hash_types` module to the modules that produce said hashes. Closes rust-bitcoin#1481
9a8cdb7 to
742cea0
Compare
|
Pushed broken version because a) I have to run and would be shame to lose so much work, b) the dependency hole is not fixed |
|
Yeah the dependency hole is fixed, took me a few minutes to remember what we did. We have to bump the version of |
| hash_newtype! { | ||
| /// Filter hash, as defined in BIP-157 | ||
| pub struct FilterHash(sha256d::Hash); | ||
|
|
||
| /// Filter header, as defined in BIP-157 | ||
| pub struct FilterHeader(sha256d::Hash); | ||
| } | ||
|
|
||
| crate::hash_types::impl_hashencode!(FilterHash); | ||
| crate::hash_types::impl_hashencode!(FilterHeader); |
There was a problem hiding this comment.
Yes, and it's effectively required for this PR because the fields are private. (Well, I could call from_raw_hash everywhere but that's annoying.) We can merge yours first if you like - it could make the changes easier to understand. I will just have to check if our PRs don't disagree about some moves. As a rule of thumb, the types should be placed wherever they are constructed by hashing the bytes.
| } | ||
|
|
||
| /// Trait which applies to general-purpose hashes that may hash arbitrary data. | ||
| pub trait RawHash: Hash { |
There was a problem hiding this comment.
Perhaps we should put all_zeros on Hash not on RawHash since it is only used for Bitcoin specific hashes which implement Hash?
There was a problem hiding this comment.
Only some hashes actually use all-zero values (besides maybe tests). From what I remember, TxId (in coinbase prev outs) and BlockHash (in genesis block). Only having it on hashes which actually do so is best IMO.
I will try to check how disruptive would it be to remove it from the trait completely.
There was a problem hiding this comment.
Even better. We could even add an AllZeros trait.
There was a problem hiding this comment.
Unless an algorithm uses all_zeros I don't want any more traits. I actually want RawHash to be duplicated as inherent methods on all our raw hashes since it's very annoying to have to import the trait. (We need RawHash because of merkle trees.)
It is - there's
This may be true regardless because I don't see |
|
I'm not disagreeing with you on the technicalities but I got this branch building locally using the version bump. |
This is Kix's work from rust-bitcoin#1555, picking it up to help out. I've just rebased it. The `Hash` trait requires for the hash type to support hashing arbitrary data. This is not great, since hash newtypes usually hash specific data. Hashing arbitrary data into newtypes is usually a bug or misunderstanding of the API. This change separates methods performing hashing of arbitrary data to a separate RawHash trait and provides a few helpers to make usage easier: - Add a private `engine()` method to the hash newtype - Add a `finalize()` method to `HashEngine` - Add a `hashes::hash` function Moves `TxMerkleNode` hash to the `merkle_tree` module because that is where implement the custom hashing and need access to its private inner hash. Closes rust-bitcoin#1481 Co-developed-by: Martin Habovstiak <martin.habovstiak@gmail.com>
|
Closing as stale. Did not look at the changes but the |
The
Hashtrait requires for the hash type to support hashing arbitrary data. This is not great, since hash newtypes usually hash specific data. Hashing arbitrary data into newtypes is usually a bug or misunderstanding of the API.This change separates methods performing hashing of arbitrary data to a separate
RawHashtrait and provides a few helpers to make usage easier:engine()method to the hash newtypefinalize()method toHashEnginehashes::hashfunctionAdditionally, because of visibility issues, this moves many hash newtypes from the dedicated
hash_typesmodule to the modules that produce said hashes.Closes #1481
This doesn't implement the trick to allow it to compile yet, will do later.