Made Hash store a byte array, so Node can store a Hash, rather than a byte slice

[FreddieRidell]

🙋 feature

Key Changes:

• Refactors crypto::hash::Hash to contain a [u8; 32], rather than Blake2bResult
• Refactors storage::node::Node.hash to be a Hash, rather than a Vec<u8>

[yoshuawuyts]

for compat with the JS version we should probably use pretty-fmt here.

[FreddieRidell]

Right, I thought there was somewhere I'd missed that!

Related question: are we aiming for full storage API compatibility with the js version, as well as network API compatibility? If so are the JS maintainers aware that any braking change they make will also break the rust version?

[yoshuawuyts]

Thanks heaps for this PR! Unfortunately in its current state we can't accept it, because it opens us up to timing attacks.

Keeping Blake2bResult around was a deliberate choice, as the Eq and PartialEq methods are guaranteed to be constant-time. By moving the bytes over to a regular buffer we drop that property, which isn't great. (ref)

I do really like some of the ergonomics improvements introduced here; typing .as_bytes().into() gets old fast. I wonder if perhaps we introduce some conversion traits we could achieve similar benefits without losing any (cryptographic) properties.

Thanks again so much; overall I really like this direction!

[FreddieRidell]

Ahh, I should have asked before I picked a TODO to work on, is there a better place to ask in future?

It looks like like Blake2BResult uses constant_time_eq to do its equality check, it should be pretty easy to adapt this solution to use that function. If I made that change would that make this PR acceptable?

[yoshuawuyts]

Ahh, I should have asked before I picked a TODO to work on, is there a better place to ask in future?

Opening an issue is a good way to go about this! Otherwise I'm also around on Freenode and Discord for synchronous chat.

If I made that change would that make this PR acceptable?

Possibly! But looking at this patch I'm thinking that adding more conversion traits on the existing codebase could probably get us the closest to our intended goal with the fewest amount of changes required.

[FreddieRidell]

Possibly!

I've switched Hash to manually implementing PartialEq and Eq using constant_time_eq, let me know what you think?

But looking at this patch I'm thinking that adding more conversion traits on the existing codebase could probably get us the closest to our intended goal with the fewest amount of changes required.

Do you mean implementing From for Hash -> Blake2bResult and stuff like that?

[bltavares]

@yoshuawuyts sorry to bother, but I'm wondering how do you feel in regards of the new constant hash verification in this current PR.