replace trait Hashable with fn keccak

[debris]

part of #6418

changes:

• renamed const variables SHA3_XXX to KECCAK_XXX
• TransactionView, BlockView and HeaderView has now pub fn hash() which returns their hash. Usage of it does not require import of Hashable trait
• removed trait Hashable
• added fn keccak. Using function is more idiomatic. Also it's more flexible and can be easily used with options and iterators. We can now write:

  fn old_usage(option: Option<H256>, vector: Vec<H256>) {
    let x = option.map(|o| o.sha3());
    // or 
    let x = option.as_ref().map(Hashable::sha3);
  
    let y = vector.into_iter().map(|i| i.sha3()).collect();
    // or
    let y = vector.iter().map(Hashable::sha3).collect();
  }
  
  fn new_usage(option: Option<H256>, vector: Vec<H256>) {
    let x = option.map(keccak);
    // or
    let x = option.as_ref().map(keccak);
  
    let y = vector.into_iter().map(keccak).collect();
    // or
    let y = vector.iter().map(keccak).collect();
  }

• fn keccak is no longer a part of util. This is a critical step required to make modules independent

@paritytech/core-devs can you please review the code? The sooner we agree on those changes and merge them, the less hassle it will be to keep it up-to-date

[eira-fransham]

This is just a renaming pass and still uses the C implementation, right? There is unsafe code that relies on the precise semantics of the C SHA3 (specifically, that it uses an internal buffer and so the input and output arrays can be the same/overlapping). I think that it might be better if we don't do that (and use the output buffer directly to reduce memcpy calls) but for now that's the assumption we make.

[eira-fransham]

There's some suspicious stuff here but I don't know enough about it to explicitly reject it.

[eira-fransham]

Why do we ignore the return value of keccak_256? Probably at least worth putting a comment explaining that.

[eira-fransham]

Is this really more ergonomic? What's wrong with using a trait here? I wouldn't have an opinion either way if this was original code but since it's the stated purpose of the PR it deserves analysis.

[debris]

It's one of the purposes :) imo, there is no point in using the trait if simple function is enough

[eira-fransham]

Bikeshed: _into implies taking ownership of T's data and transforming it into something rather than writing it to a buffer, this confused me when first reading this code. I recommend write_keccak, but it's a matter of taste. I think it's good to avoid names that already have a common, conflicting use in Rust code.

[debris]

This is just a renaming pass and still uses the C implementation, right?

Yes. Cause the C implementation is still ~10% faster than we achieved with rust.

There is unsafe code that relies on the precise semantics of the C SHA3 (specifically, that it uses an internal buffer and so the input and output arrays can be the same/overlapping). I think that it might be better if we don't do that (and use the output buffer directly to reduce memcpy calls) but for now that's the assumption we make.

I'm not sure I understand your point. Do you want to return internal buffer from keccak function? It's not possible with this type of hash function.

[eira-fransham]

Essentially all I'm saying is that some unsafe code relies on the hash function allowing input and output to overlap, so as long as that still holds we're all good. It looks like it does, I just wanted to make certain.

[debris]

Yes, nothing has changed internally

[folsen]

s/sha3/keccak ?

[folsen]

In util/rlp/src/common.rs there's some references to SHA3_NULL_RLPand SHA3_EMPTY, I'm guessing they should be changed.

[folsen]

s/sha3/keccak

[rphmeier]

LGTM now that grumbles have been addressed