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

Change Hashing Function from Blake2b to Sha3 #1564

Closed
hwwhww opened this issue Dec 9, 2018 · 4 comments
Closed

Change Hashing Function from Blake2b to Sha3 #1564

hwwhww opened this issue Dec 9, 2018 · 4 comments

Comments

@hwwhww
Copy link
Contributor

hwwhww commented Dec 9, 2018

What is wrong?

Per ethereum/consensus-specs#227

How can it be fixed

  1. Use this one:
    from eth_hash.auto import keccak
    
  2. Remove eth/utils/blake.py and related tests.
@ChihChengLiang
Copy link
Contributor

The spec says keccak is used now and might switch to another hash function in the future.

This means that we are replacing all hash in the codebase now and will do the same in the future.
Propose to implement a hash wrapper in eth/utils, say hash_function.py.

# eth/utils/hash_function.py
from eth_typing import Hash32
from eth_hash.auto import keccak

def hash_(data: bytes) -> Hash32:
    return Hash32(keccak(data))

and in the rest of the codebase just do

from eth.utils.hash_function import hash_
hash_(something)

so the future switch of hash function would become just a one-liner fix in hash_function.py.

@jannikluhn
Copy link
Contributor

Should this be based in py-evm or in eth_hash? Asking because if we implement tree hashing in py-ssz (which would make sense from my point of view), it would be convenient to use the same function.

hash_ is probably not a good name as it's not clear that it's for the beacon chain and it also looks quite similar to just hash. What about hash_eth20?

@ChihChengLiang
Copy link
Contributor

Ahhhh ... sorry @jannikluhn, I missed your comment. I would recommend py-ssz should have its own wrapper for the hash function that is based in eth_hash, so the boundaries between py-evm, py-ssz, and eth_hash are clean.

I agree the point that hash_ is unclear, but hash_eth20 sounds like 20. What about hash_bcc?

@hwwhww
Copy link
Contributor Author

hwwhww commented Dec 17, 2018

Should this be based in py-evm or in eth_hash? Asking because if we implement tree hashing in py-ssz (which would make sense from my point of view), it would be convenient to use the same function.

IMO eth_hash should be stable and the output from eth_hash should be the same. That said, I think the wrapper should be in py-evm and py-ssz level.

hash_bcc

But it's not only for beacon chain! Maybe hash_eth2?

@hwwhww hwwhww added this to the Beacon chain MVP milestone Dec 17, 2018
@hwwhww hwwhww closed this as completed Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants