VM: Fix Blake2B with messages with a length >= 5

[jochem-brouwer]

Closes #1482

I need to verify that these changes are correct. I noticed that in the original spec (https://github.com/dcposch/blakejs/blob/master/blake2b.js) the B2B_GET32 receives an Uint8Array, where we pass an Uint32Array. The Uint32Array we are using is already little endian.

I have locally fuzzed 1000 random messages (rounds <= 2^16-1) against Infura and verified these were the same as which the VM reports after these changes.

[codecov]

Codecov Report

Merging #1486 (4458bcd) into master (d361b55) will decrease coverage by 0.14%.
The diff coverage is 100.00%.

Flag	Coverage Δ
block	86.60% <ø> (ø)
blockchain	82.78% <ø> (-0.07%)	⬇️
client	83.67% <ø> (-0.48%)	⬇️
common	94.08% <ø> (-0.19%)	⬇️
devp2p	82.79% <ø> (+0.13%)	⬆️
ethash	86.56% <ø> (ø)
tx	87.34% <ø> (-0.13%)	⬇️
vm	79.38% <100.00%> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

[jochem-brouwer]

I noticed that Uint32 arrays endianness is based upon the OS endianness. So we might want to test this also on big endian OS - then this might fail.

[holgerd77]

@jochem-brouwer pretty cool that you got something on this that quickly! 😄

This is what I got from Sina, asked him per DM if he has got some insight:

oof that's a tricky one 🙂 I copied the blake2 implementation from a library and added the wrapping which makes it a precompile. From the issue description I gather the problem should be somehow with the m variable (message being hashed). But having looked at it for some time nothing jumps out in the precompile-part of the code and I don't really understand the blake2 part enough...

[jochem-brouwer]

I have created a dirty fuzzer and ran 1000 random inputs against Infura and checked if the VM reported the same outcome.

I have tested rounds up to 2^16-1, if I use more rounds (one could theoretically do 2^32-1 rounds) I run against out of gas at Infura.

(If necessary I can dump the fuzzer in a comment here)

[jochem-brouwer]

To clear up any confusion about endianness; we actually read the message in little endian as can be seen here.

[jochem-brouwer]

I am pretty convinced this is now fixed.

[axic]

Is it worth adding an upstream test case for this in https://github.com/ethereum/tests?

[jochem-brouwer]

Hi @axic, I have contacted @winsvega from ethereum/tests, they will add this case to ethereum/tests 😄

[s1na]

LGTM

[holgerd77]

Very cool, Ori has even already added some new test cases for this (see ethereum/tests#948).

Will merge, planning monorepo releases for Monday or Tuesday anyhow, so this will get into a release soon.