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

feat!: update hasher design #229

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

AaronFeickert
Copy link
Contributor

@AaronFeickert AaronFeickert commented May 9, 2024

This PR updates the domain-separated hasher design.

Currently, the handling of domain separators is done via an initial length-prefixed human-readable concatenation of a domain separator, version number, and label that requires special handling due to encoding.

The design proposed here simplifies things. All input entering an underlying hash function (domain separator, version number, label, and any subsequent data) is prefixed by a tag byte that indicates its purpose. If the input is of variable length, it is prefixed by its length as well.

This removes the need for more complex encoding, is very robust against collisions, and is flexible in that it allows for other input types to be added in the future if desired.

Closes #227. Supersedes #226.

BREAKING CHANGE: This changes the way that domain-separated hashing is performed, so it is extremely breaking. Existing domain-separated hashes will not match.

@AaronFeickert
Copy link
Contributor Author

This PR is incomplete, but I'm opening it as a draft to get feedback.

Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

This seems robust, with a cleaner and simpler implementation. I am in favour.

src/hashing.rs Outdated Show resolved Hide resolved
@AaronFeickert
Copy link
Contributor Author

Blocked on a decision about whether to use a Borsh/CBOR design instead, as is used elsewhere.

src/hashing.rs Outdated Show resolved Hide resolved
@sdbondi
Copy link
Member

sdbondi commented Jul 19, 2024

In favour of the bugfix and simplifications in this PR, and the tagging makes sense to me. I don't think we need to choose between DS borsh hashing and DS raw bytes hashing. They both have their place. I would simply copy and paste the Borsh hasher to Tari crypto under the borsh feature flag (and bring docs up to the tari_crypto standard) in another PR. We can then use it with minimal effort in the two repos.

I don't think we should introduce CBOR into Tari crypto, as this is firmly a use case/concern of the L2.

I would perhaps clarify that the update (and therefore chain) methods are not simply adding the given preimage bytes to the hasher, and these methods should be called in a consistent/canonical way for every given hash construction e.g. make it clear that hasher.chain(b"A").chain(b"B").finalize() == hasher.chain(b"AB").finalize() does not apply as it does with other hashers.

@AaronFeickert
Copy link
Contributor Author

I would perhaps clarify that the update (and therefore chain) methods are not simply adding the given preimage bytes to the hasher, and these methods should be called in a consistent/canonical way for every given hash construction

Good call. I'll update the documentation accordingly.

Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider updating hasher domain separation
5 participants