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 hash function Tip5 #188

Closed
wants to merge 1 commit into from
Closed

add hash function Tip5 #188

wants to merge 1 commit into from

Conversation

jan-ferdinand
Copy link

This PR is trying to address #187.

Unfortunately, the tests are not passing. I suspect this has to do with me raising the upper bound for the number of bytes in a Digest (see here). However, the feedback provided by the failing tests doesn't let me pinpoint the root cause very easily. Since I don't know the codebase very well, I'd be happy if someone in the know could take a look.

Additionally, I'm not sure if my handling of bytes is correct in all places. For example (but not limited to): my current implementation of the Hasher's trait method fn hash(bytes: &[u8]) interprets 8 bytes as a field element of the field with $2^{64} - 2^{32} + 1$ elements1. This can lead to hash collisions, depending on the type of objects being fed into the method. I'd appreciate some guidance here.

Footnotes

  1. This is the only field the hash function Tip5 is defined over, so there is no need to be generic with respect to the field.

@facebook-github-bot
Copy link

Hi @jan-ferdinand!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@ameya-deshmukh
Copy link

Hi! I'll try hacking on this and will let you know if tests pass for me. In addition to this, happy to reproduce and review this PR

@Nashtare
Copy link
Contributor

I don't think it makes sense to increase the size of Digest bytes for all existing instantiations, as this need comes solely from the specific version of Tip5 you want to implement. Instead I'd suggest having the output length defined by an associated constant.

@jan-ferdinand
Copy link
Author

jan-ferdinand commented Apr 15, 2023

@Nashtare, I agree. However, taken straight from the description of the Digest trait:

Ideally, the length of the returned array should be defined by an associated constant, but using associated constants in const generics is not supported by Rust yet. Thus, we put an upper limit on the possible digest size. For digests which are smaller than 32 [now 40] bytes, the unused bytes should be set to 0.

Refactoring the trait was beyond what I was willing to invest into this PR for the time being.

@jan-ferdinand jan-ferdinand closed this by deleting the head repository Dec 3, 2023
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.

4 participants