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 support for Google HighwayHash #5

Closed
wants to merge 1 commit into from
Closed

add support for Google HighwayHash #5

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 12, 2018

Hi,

Here's a pull request to implement what Damian mentioned re: Highway hash ( #4 )

It mentions in Google HighwayHash's tests that they haven't finalized 128- and 256-bit yet in C++, so I've included what I think that code might be... but it's left commented out until that time comes in the future. (edit: this is in a specific comment in the code still but evidently is wrong)

Only the 64-bit is enabled currently and it's implemented in C form.

Some other things in the code: You can see that I wrapped things in HAVE_HIGHWAY, but I also wanted to make special mention that I intentionally left it so that CMakeLists.txt needs touch-ups.. It doesn't conditionally include the library.

( Not a cmake expert and just did this quickly with the hope that you'd clean it up ;) )

Cheers!

@ghost ghost mentioned this pull request Jan 12, 2018
@dgryski
Copy link

dgryski commented Jan 12, 2018

Highway Hash 128 and 256 were finalized a few days ago.

@ghost
Copy link
Author

ghost commented Jan 12, 2018

I think they haven't finalized the changes in the C implementation then. I just skimmed over the commit. Should I poke 'em, y'think?

@dgryski
Copy link

dgryski commented Jan 12, 2018

I believe the changes were running the final mix 6 times for 128 and 10 times for 256. You can also check the Go repo for minio's implementation since they also have updated their version to the latest.

@ghost
Copy link
Author

ghost commented Jan 12, 2018

Hmm. Going based on what's in highwayhash... Look in c_bindings.h. Doesn't look like they export the 128 and 256-bit versions. I see them plainly in the source sitting right there. Strange.

@ghost ghost requested a review from demerphq January 12, 2018 02:59
@demerphq
Copy link
Owner

demerphq commented Jan 12, 2018 via email

@demerphq
Copy link
Owner

Id like to reconcile your changes with mine:

https://github.com/demerphq/smhasher/tree/yves/highwayhash

And then merge them. I used the c++ part, but for the life of me I had to do dodgy shite to make the linking work. Eventually i symlinked highwayhash similarly to you, but I also had to symlink libhighwayhash.so and libhighwayhash.so.0 as well.

One thing that is weird is the validation hash for HighwayHash64 is different. I wonder why.

Another interesting thing is that HighwayHash256 fails avalanche tests. For the number of tests we do it has too many bit which are too far from 50%. This does not seem to affect 128 or 64.

BTW, the timing data from my patch is garbage. I am sure I am not handling the seed properly. Damian if you feel like poking that it would be cool.

Yves

@erthink
Copy link
Contributor

erthink commented Jan 16, 2018

Seems the reason that HighwayHash256 fails avalanche tests is #7.

@demerphq
Copy link
Owner

I got distracted with other things, I will revisit this when I get a chance.

Yves

@ghost ghost closed this by deleting the head repository Mar 16, 2023
This pull request was closed.
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