-
Notifications
You must be signed in to change notification settings - Fork 138
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 cube hash implementation #100
Conversation
This is a port of the Bernstein's "simple" C implementation. https://github.com/floodyberry/supercop/blob/master/crypto_hash/cubehash1632/simple/cubehash.c
5386b1f
to
0c3245d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some possible optimizations and suggestions but lgtm
src/cubehash/src/lib.rs
Outdated
/// | ||
/// Panics if hash_bytes > 64 | ||
pub fn new(hash_bytes: u16) -> Self { | ||
assert!(hash_bytes <= 64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check also for empty output?
assert!(hash_bytes > 0 && hash_bytes <= 64)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the parameter, it's fixed to be 32 now.
|
||
for _i in 0..10 { | ||
transform(&mut state); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is a worthwhile optimization or not but I thought it worth suggesting: the state here will depend only on the value hash_bytes
, and I'd expect in practice we will only use cubehash with 1 or perhaps 2 different output sizes. It may be worth precomputing the initial state for those 1 or 2 hash_bytes
values that we plan to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this on the meeting today, and decided to fix the hash_bytes
parameter to be 32. This allows us to simplify code and make it a bit more efficient.
So I replaced the initialization routine with constructing a static array.
fn test_hello() { | ||
assert_eq!(hash64(b"hello"), "f7acd519f51a6caa5387ae730ed999c4c31766d8477e4e1eef4275e9df07dc4c08adc4b64c9dc8359d711020f78627a4d1bcfecadd28a5263c05faf75e96555a".to_string()); | ||
assert_eq!(hash64(b"Hello"), "dcc0503aae279a3c8c95fa1181d37c418783204e2e3048a081392fd61bace883a1f7c4c96b16b4060c42104f1ce45a622f1a9abaeb994beb107fed53a78f588c".to_string()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not able to immediately find the source of these test vectors. Can you add a comment with a link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I produced them by writing a simple wrapper around the reference C implementation, but I also double-checked the result on the Wikipedia. Added a comment.
fn transform(state: &mut [Wrapping<u32>; 32]) { | ||
let mut y: [Wrapping<u32>; 16] = [Wrapping(0); 16]; | ||
|
||
for _round in 0..CUBEHASH_ROUNDS { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no objections to this implementation of transform
but it might be worth considering if using simd::u32x4
or native SSE2 intrinsic is worthwhile. This is demonstrated in the implementation in SUPERCOP https://github.com/floodyberry/supercop/blob/master/crypto_hash/cubehash1616/emmintrin4/cubehash.c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will run in Wasm, so I don’t think using any such intrinsics are useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. And webassembly SIMD is still unstable in Rust rust-lang/rust#74372 :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also hoping that in 2021 compilers can already automatically vectorize code like that without bold hints.
rustc
does vectorize some parts when compiled in release mode, but not everything, of course: https://godbolt.org/z/vPnaehaar
This is a port of the Bernstein's "simple" C implementation (CubeHash 160+16/32+160/N)
https://github.com/floodyberry/supercop/blob/master/crypto_hash/cubehash1632/simple/cubehash.c
I removed all the code that handles partial byte updates, this simplifies code quite a bit.
We are only going to hash full bytes anyway.