Update hash function

[ralexstokes]

Description

The older spec used blake2 as the hash function. The latest spec uses keccak256.

Present Behaviour

Uses the old hash.

Expected Behaviour

Should use the new hash.

Steps to resolve

Use the correct hash function.

I started this work on a branch and seem to be triggering a panic due to this line:

https://github.com/sigp/signature-schemes/blob/master/src/amcl_utils.rs#L36

And think we should just update that crate to use keccak256 as well.

Figured I would open this up for discussion before making more changes.

[ralexstokes]

ideally that function is just generic over our hashing function... i just checked though and the tiny_keccak hash does not implement std::hash::Hasher

[paulhauner]

Oh yep, I didn't notice the change to keccack!

I also noticed that the hashing libs I saw didn't use std::hash::Hasher. I don't know/can't remember why that is.

I agree that it would be ideal that signature-schemes is generic over different hashers, but I wouldn't spend much time upgrading it because we're hopefully going to move to something by zcash in the medium-term.

Why exactly is there a panic due to lighthouse moving to keccack, I would have thought signature-schemes would be unaffected by how we hash?

[ralexstokes]

I believe it has to do w/ this:

https://github.com/sigp/lighthouse/blob/master/beacon_chain/utils/hashing/src/lib.rs#L13

and pretty sure it traces to this:

https://github.com/sigp/signature-schemes/blob/master/src/amcl_utils.rs#L36

Seems like GroupG1::map_it expects a buffer of a certain size (which is the panic I'm seeing)...

Is there a reason the buffer is this size?

[paulhauner]

Oh yeah, sorry I'm with you now. So you were modifying signature-schemes and getting the panic?

Milagro expects 48 bytes (the curve is 381 bit) and it will cause an OOB panic if you give it any less. I raised this on the spec at one point but it didn't really go anywhere.

My approach would be to leave it as it is for now, then revisit this once we're closer to a point where out signatures need to match other clients. AFAIK we don't even have test vectors yet, so it's likely that no-one is matching at this point. Unless of course it's blocking our progress.

[ralexstokes]

gotcha -- yeah i would have to dig into the crypto a bit to say more...

i'll just extend the buffer for now like it was