Skip to content
This repository was archived by the owner on Nov 30, 2022. It is now read-only.

Add siphash24 module#46

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
stevenroose:siphash
Jun 12, 2019
Merged

Add siphash24 module#46
apoelstra merged 1 commit intorust-bitcoin:masterfrom
stevenroose:siphash

Conversation

@stevenroose
Copy link
Copy Markdown
Collaborator

@stevenroose stevenroose commented May 14, 2019

Based on the siphasher crate (https://github.com/jedisct1/rust-siphash/) that is a copy of the implementation in std::hash.

@apoelstra
Copy link
Copy Markdown
Member

Is there a significant performance increase from using unsafe code rather than just letting byteorder do the endianness conversions?

@apoelstra
Copy link
Copy Markdown
Member

Also, is there another implementation we can test against? Or some test vectors?

@stevenroose
Copy link
Copy Markdown
Collaborator Author

To be honest, I have no idea about the implementation internals. I just took the one from the siphasher package. I also took their unit tests, but I had to adapt them since they were using the std::hash trait or something.

About more unit tests, I will look for some in other languages that can be verified line per line because I understand the ones I have aren't verbatim from siphasher and that might sound phishy :)

@stevenroose
Copy link
Copy Markdown
Collaborator Author

@apoelstra hehe, I noticed that the test vectors we have are actually the same ones as the reference implementation has: https://github.com/veorq/SipHash/blob/master/vectors.h#L3

I think the siphasher people made a variation on that (so they could use the Hash trait) and I coincidentally reverted their variation back to the original vectors. It's a series of 64 hashes of the series [], [0], [0, 1], [0, 1, 2], ...

Copy link
Copy Markdown
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.

Looks good. Did not verify the test vectors

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants