Istanbul: EIP-152 Blake2 F function

[s1na]

Work in progress to fix #568

I took the blake2 logic from blakejs. For now I put it right above the precompile to make testing easier. I'm not sure where we want to maintain this code. Ideas @holgerd77?

Also used test cases in geth, one which does 8 million rounds is failing. I was trying to use the cases in ethereum/tests#619 but they all fail. I think it might be due to us not having implemented EIP-2200 yet.

Also the linter insisted on putting every element of SIGMA8 and IV on their own lines (adding 100 extra LoC), which I find ugly 😛

[coveralls]

Coverage increased (+0.2%) to 95.379% when pulling 86a0b13 on istanbul/blake2f into d4cff4f on master.

[holgerd77]

That's great! 😄

@s1na Not sure, would likely make sense to release this as its own blake2fjs or so library so that others can benefit as well?

[holgerd77]

@s1na Ah, just did some reading on the EIP for the first time, so this (the F part) is just some extract from the Blake 2b hash function? First thought it would be an extra hash function on its own.

So eventually not useful to extract as an own lib? This part of the functionality is not directly callable through the API of the blakejs library?

[s1na]

Ah, just did some reading on the EIP for the first time, so this (the F part) is just some extract from the Blake 2b hash function?

Yeah exactly. It's kind of the core of blake2. You could e.g. build the blake2b hash function on top of this F function.

So eventually not useful to extract as an own lib?

I'm not sure it'll be very useful. We could first release it here, and afterwards maybe move it to ethereumjs-util if we don't want to keep the code here.

This part of the functionality is not directly callable through the API of the blakejs library?

Unfortunately not, I opened dcposch/blakejs#12 a while back to ask whether that's an option, but haven't received a response. I'm not sure the repo is actively maintained.

[holgerd77]

This is generally close-to-be-ready I would assume and we can wait to include this in the v4.1 release?

[holgerd77]

@s1na Yes, likely makes sense to just leave the code here for now I would say.

[s1na]

This is generally close-to-be-ready I would assume and we can wait to include this in the v4.1 release?

Yeah I don't have any more changes in mind, unless some more changes are requested during review.

Something to note is that Alex proposed some changes to the EIP. But if they were merged in the EIP we could open a fresh PR later.

[axic]

@s1na this implementation seems to be very specific to blake2b and it may not work with blake2s (the G function needs to mod by the word with).

[holgerd77]

@axic Ah, the title is a bit misleading on the EIP-152 PR page - still stating Blake2b, also with similar references in the description - you should eventually update this (as it is in the final EIP text just stating Blake2), think this is a potential cause for a lot of confusion with first/superficial readers concluding this is an EIP for Blake2b support.

I would have a tendency to nevertheless merge here and release if the general review remarks are tackled - @s1na or are these Blake2s related changes something which can be easily tested and modified? I think we will mark this Istanbul release anyhow as beta (so only the Istanbul support and not the VM release itself), seems there is still some discussion anyhow on some of the EIPs and this (all EIP implementations here) is also not tested yet against the official test suite. It's pretty likely that some things will further change along the way.

Does this make sense? The Istanbul release is pretty much awaited - together with some other fixes - so that people can start to test and integrate.

[axic]

It turns out the EIP is unclear and there is a debate going on. This PR matches the reference implementation done by the EIP authors and matches the tests given they were generated using that reference.

I hope there will be changes to the EIP, but that can happen in a subsequent PR here.

[holgerd77]

Reviewed by @axic, approved in this comment.