Blake2b F precompile #4

pdyraga · 2019-06-20T00:03:03Z

Here we present an implementation of Blake2b F compression function precompile based on the ethereum/EIPs#2129

Description

The precompile lives at the address 0x09 and should be called with a standard staticcall instruction. The expected input data structure is as follows:

[ 4 bytes for rounds ][ 64 bytes for h ][ 128 bytes for m ] \
[ 8 bytes for t_0 ][ 8 bytes for t_1 ][ 1 byte for f ]

where:

rounds - number of rounds
h - state vector
m - message block vector
t_0, t_1 - offset counter
f - final block indicator flag

There is a sample Truffle project prepared which can be used to play with this precompile:
https://github.com/pdyraga/f-precompile-call :

truffle migrate --reset --network local && truffle exec ./scripts/query-f.js --network local

Istanbul hardfork

Since we hope to have this implementation included in the Istanbul hardfork, all control structures related to precompiles and hardfork have been added in this PR. Before you'll be able to use this precompile you need to enable Istanbul HF. One possibility is to use genesis.json and set "istanbulBlock": 0 in "config" section.

Sample call

 function F(uint32 rounds, bytes32[2] memory h, bytes32[4] memory m, uint64[2] memory t, bool f) public view returns (bytes32[2] memory) {
      bytes32[2] memory output;

      bytes memory args = abi.encodePacked(rounds, h[0], h[1], m[0], m[1], m[2], m[3], t[0], t[1], f);

      assembly {
            if iszero(staticcall(not(0), 0x09, add(args, 32), 0xd5, output, 0x40)) {
                revert(0, 0)
            }
      }

      return output;
    }

    function callF() public view returns (bytes32[2] memory) {
      uint32 rounds = 12;

      bytes32[2] memory h;
      h[0] = hex"b02406865b80cde9fec1e321559891be456158a6d71cf0eac1eb845c70d4a3d8"; 
      h[1] = hex"948e82cc975dcbd6984780bab7e236382fb585df5bc4b043cb3e866544413d92";
  
      bytes32[4] memory m;
      m[0] = hex"87868584838281808f8e8d8c8b8a898897969594939291909f9e9d9c9b9a9998";
      m[1] = hex"a7a6a5a4a3a2a1a0afaeadacabaaa9a8b7b6b5b4b3b2b1b0bfbebdbcbbbab9b8";
      m[2] = hex"c7c6c5c4c3c2c1c0cfcecdcccbcac9c8d7d6d5d4d3d2d1d0dfdedddcdbdad9d8";
      m[3] = hex"e7e6e5e4e3e2e1e0efeeedecebeae9e8f7f6f5f4f3f2f1f000fefdfcfbfaf9f8";
 
      uint64[2] memory t;
      t[0] = 383; // 000000000000017f
      t[1] = 0;	// 0000000000000000

      bool f = true; // 01

      return F(rounds, h, m, t, f);
    }

So far the precompiled Istanbul contracts list contains exactly the same contracts as the one for Byzantium. We'll work on adding a support for Blake2 F precompile in Istanbul in the next commits. Next to PrecompiledContractsIstanbul list, I introduced all the required control structures for the fork.

The contract calls keep-network/blake2 implementation of the F compression function and lives under address 0x09. The called F function is optimized for 64-bit WORDs. Test cases were generated from test vectors from keep-network/blake2 library.

core/vm/contracts.go

hadv · 2019-06-24T03:25:56Z

core/vm/contracts.go

 	"errors"
 	"math/big"

+	"github.com/keep-network/blake2"
+


nit: no need blank line here ;)

holiman · 2019-06-24T08:59:53Z

core/vm/contracts.go

+	h[4] = binary.BigEndian.Uint64(input[32:40])
+	h[5] = binary.BigEndian.Uint64(input[40:48])
+	h[6] = binary.BigEndian.Uint64(input[48:56])
+	h[7] = binary.BigEndian.Uint64(input[56:64])


The EIP needs to specify how to handle cases where the input data is too short. Other precompiles typially return an error in that case. It's very important that such decisions are not left "as implementation details", since it's consensus-critical behaviour.

In this case, the client woud panic if given an input shorter than 64 bytes, afaict

I'll add it to the EIP spec, waiting for the draft and competing PRs to be resolved

We now expect the input to be exactly 213 bytes long.

The PR was opened as an initial implementation before we figure out some details. I should probably open it in a Draft mode.

holiman · 2019-06-24T09:00:35Z

core/vm/contracts.go

+		f = true
+	}
+
+	rounds := binary.BigEndian.Uint32(input[316:320])


actually, panic on shorter than 320 bytes in length

Yep, it was wrong. We now expect the input to be exactly 213 bytes long.

holiman · 2019-06-24T09:07:08Z

core/vm/contracts.go

+type blake2F struct{}
+
+func (c *blake2F) RequiredGas(input []byte) uint64 {
+	return 1 //TODO: implement


Even if you don't quite know what constants to use, perhaps you have an idea about the general pricing scheme? Like, is it rounds * K *input_size ? Would be good to have something, even if we determine the constants later. It would make it easier for others people to help tune the constants.

The benchmarks so far lead me to believe that a round is ~ 1 gas (!), and that this precompile will be dominated by calling cost. We can provide a more robust set of benchmarks that including input size as a floating param.

More discussion here

I believe the methodology of pricing relative to ecrecover is sound. Once we have a wider range of inputs on benchmarks this will be straightforward.

I opened this PR in an early-mode before we had a chance to discuss benchmarks and gas price. Based on what we know so far, and using the same methodology as for the EIP-1108 it seems that it should be 1 gas/round.

I was hoping to remove this TODO and put a final value here after we agree on the gas in the EIP. I am not sure how the process should look here.

I set the gas price to 1/round based on our benchmarks.

holiman · 2019-06-24T11:46:04Z

core/vm/contracts.go

+	t[1] = binary.BigEndian.Uint64(input[248:256])
+
+	var f bool
+	if input[287] == 1 {


Interesting -- this right here checks if the f is strictly b 0000 0001. So if it's b 1111 1111, it would be set to false. I think it would be more sensible to look purely at the lsb (and, naturally, to have that specified in the EIP).

I updated the check to:

if input[208] == 0x00000001 { f = true }

I am not sure if it is what you meant by looking at the least significant bit.

I meant something like

if (input[208] & 0x1) != 0 { f = true }

There are basically three variants:

Check if input[208] != 0. So b 1111 1110 counts as true

Check if input[208] == 1 (what you do now). So b 1111 1111 counts as false

Check if only least significant bit.

My order of preference would be 3, 1, 2. Another option would be to do exactly what solidity does, whatever that is, to resolve an input parameter into a bool. @axic what would solidity do ?

I just experimented a bit with solidity. Afaict, for boolean, the rule seems to be nonzero = true (variant 1). So if we want to follow the same logic, the check would be

f := (input[208] != 0)

Good point, updated as requested.

holiman · 2019-06-24T11:49:20Z

core/vm/contracts.go

+
+	rounds := binary.BigEndian.Uint32(input[316:320])
+
+	blake2.F(&h, m, t, f, int(rounds))


Is it really safe to cast a uint32 to int -- what about 32-bit systems? Will it become negative on 0xffff ffff?

We no longer cast to int - the number of rounds is uint32 everywhere now.

holiman · 2019-06-24T11:59:41Z

Why is all the padding needed? The caller sends along 320 bytes, but the method totally ignores bytes 192 - 216 , 256 - 286, 288 - 316 ?

core/vm/contracts.go

We decided to switch the behaviour of F precompile to accept abi.encodePacked parameters instead of left-padded ones (for values smaller than 32 bytes).

No changes in what modules are imported. I've just removed the empty line and auto-formatted imports.

pdyraga · 2019-06-25T18:47:34Z

Why is all the padding needed? The caller sends along 320 bytes, but the method totally ignores bytes 192 - 216 , 256 - 286, 288 - 316 ?

We no longer have padding. We now accept exactly 213 bytes of abi-encoded parameters.

holiman · 2019-06-25T20:28:58Z

core/vm/contracts.go

+type blake2F struct{}
+
+func (c *blake2F) RequiredGas(input []byte) uint64 {
+	if len(input) != 213 {


If input larger than 213 is used, this will throw. If that's the intended behaviour, then the EIP must also be updated to explicitly state that only exactly 213 bytes is accepted.

Yes, my intention was to update the EIP to state it explicitly that exactly 213 bytes is accepted. I am not sure if there is any use case for passing more bytes and ignoring some of them. I personally think it's less error-prone to expect the exact length and fail when the length does not match.

core/vm/contracts.go

holiman · 2019-06-25T20:29:37Z

core/vm/contracts.go

+}
+
+var (
+	blake2FInputLength = 213


this should be a const, no?

Yeah, it could be:

const blake2FInputLength = 213 var errBlake2FIncorrectInputLength = errors.New( "input length for Blake2 F precompile should be exactly 213 bytes", )

but thought it's better to initialize them in one block, similarly how it's done above the bn256Pairing struct definition. I wasn't sure about the conventions used in the project.

Please let me know what's the preference here and I'll alter it.

I think const is preferrable, rather than calling it a var just for the sake of indentation...

Good point, updated.

core/vm/contracts.go

holiman · 2019-06-25T20:35:55Z

core/vm/contracts.go

+
+func (c *blake2F) Run(input []byte) ([]byte, error) {
+	if len(input) != 213 {
+		return nil, errBlake2FIncorrectInputLength


If it's only used in this place, might as well define the error inline

Suggested change

return nil, errBlake2FIncorrectInputLength

return nil, errors.New("invalid input length for Blake2 F precompile")

It's also used in tests, see TestPrecompileBlake2FMalformedInput.

Co-Authored-By: Martin Holst Swende <martin@swende.se>

iikirilov · 2019-06-30T13:56:18Z

core/vm/contracts.go

+	t[1] = binary.BigEndian.Uint64(input[200:208])
+
+	var f bool
+	if input[208] == 0x00000001 {


Suggested change

if input[208] == 0x00000001 {

if input[208] != 0x00000000 {

This will be checked in a different way, just != 0

We now point to keep-network/blake2b/compression to include only what is really needed - F compression function

compression.F accepts `m` parameter as an array of uint64 instead of byte slice now. I have also improved test vectors to cover various cases of rounds parameter.

Gas price depends solely on the number of rounds. Having this parameter as the first one lets to evaluate gas price easier.

I made a mistake when refactoring `Run(input []byte)` to do the same and not updated `RequiredGas(input []byte)`. Fixing my mistake now.

pdyraga · 2019-07-01T16:23:34Z

All comments addressed. I still need to do some more extensive tests but I think it's ready for another round of review.

Instead of pointing to a commit with no official tag, we now point to an exact release number.

pdyraga · 2019-07-02T11:36:07Z

Tested with https://github.com/pdyraga/f-precompile-call, works as expected:

$ truffle migrate --reset --network local && truffle exec ./scripts/query-f.js --network local
Compiling ./contracts/PrecompileCall.sol...
Writing artifacts to ./build/contracts

(...)

Using network 'local'.

querying...
0xccfc282ed60927145b46f8d0fa97afd07010c51d20821e9748923ea42a37a0fa,0x0609a13be7c1e14b6e10a4b63d85d1d56d3d370d80f97b0a61a4f22ed6462dee

daira · 2019-07-04T19:17:28Z

core/vm/contracts.go

+	for i := 0; i < 8; i++ {
+		offset := i * 8
+		binary.BigEndian.PutUint64(output[offset:offset+8], h[i])
+	}


This code may be using the wrong byte order, as discussed starting at ethereum/EIPs#2129 (comment) .

holiman · 2019-07-29T08:51:58Z

Is this PR now 'final' ? If so, could you open a PR against go-ethereum proper?

pdyraga · 2019-07-29T08:54:01Z

@holiman I need to update EIP with the requested test vectors and double-check possible byte ordering problems. Please expect a PR against go-ethereum this week.

holiman · 2019-08-06T14:48:11Z

How is it going? Istanbul is not far off now, and we're trying to get things in as soon as possible, to allow more time for testing and test creation.

pdyraga · 2019-08-07T09:30:11Z

How is it going? Istanbul is not far off now, and we're trying to get things in as soon as possible, to allow more time for testing and test creation.

Working on it today, sorry for the delay.

karalabe · 2019-08-07T10:00:37Z

vendor/vendor.json

+			"revision": "02884c605a94951d05165fd56c69d6239f63b3d1",
+			"revisionTime": "2019-07-02T09:51:07Z",
+			"version": "v1.0",
+			"versionExact": "v1.0"


Since this is actually code taken verbatim from Go (as far as I understand it), please move it to crypto/blake2b inside the repo. I don't like the idea that we have a middleman repo in between for code that's copy pasted from elsewhere anyway.

This is not entirely copy-paste. Go code does not expose F compression function, we had to modify it to allow for an arbitrary number of rounds, have the interface compatible with what we expect from the precompile and removed some pieces that are not really a part of compression but rather hashing. Please compare:
https://github.com/keep-network/blake2b/blob/master/compression/f.go#L39
https://github.com/golang/crypto/blob/master/blake2b/blake2b_generic.go#L30

karalabe · 2019-08-07T10:00:55Z

vendor/vendor.json

@@ -965,5 +973,5 @@
 			"revisionTime": "2017-08-11T01:42:03Z"
 		}
 	],
-	"rootPath": "github.com/ethereum/go-ethereum"
+	"rootPath": "github.com/keep-network/go-ethereum"


Take care to fix this if you open a PR upstream.

BLAKE2b RFC Appendix A specifies test vector that we have implemented here. See https://tools.ietf.org/html/rfc7693#appendix-A For now, we leave blake2FTests are slice because we'll probably add more test vectors there.

The final block indicator flag for F precompile must be either 0 or 1.

holiman · 2019-08-13T12:24:04Z

core/vm/contracts.go

 func (c *blake2F) Run(input []byte) ([]byte, error) {
 	if len(input) != blake2FInputLength {
 		return nil, errBlake2FIncorrectInputLength
 	}
+	if input[212] != blake2FNonFinalBlockBytes && input[212] != blake2FFinalBlockBytes {


Now you changed it to be super strict. Is that in line with the EIP? It wasn't when I checked yesterday, IIRC

Yes, the EIP still says

The boolean f parameter is considered as true if at least 1 bit out of the 8 bits is set.

I am going to update EIP in an ~hour. I should probably push the EIP change first, sorry. Changed based on this discussion.

holiman · 2019-08-16T10:10:55Z

@pdyraga please file this PR against go-ethereum, and let's do the remaining the work there.

We're getting to a point where we are cutting very close to deadlines.

So just open a PR, that's enough if you don't have more time right now. If you do have some more time, it'd be great if you also rebase it against current master (and if you have even more time, if you also squash the commits).

Otherwise, one if us will have to take this branch, rebase it, and file it ourselves (if it eventually get's squashed, it will be better if you are the PR submitter, so you get commit-credi).

pdyraga · 2019-08-16T10:12:36Z

I'll do it today @holiman

I am currently working on the last test vectors for the EIP. Should have both ready today.

holiman · 2019-08-16T10:23:31Z

Great!
But just so you know - having the final test vectors is good, but if you don't have time for both test vectors and upstream PR, then I'd prioritize the PR.

pdyraga · 2019-08-16T11:16:26Z

@holiman Regarding squashing commits - is the preferred approach to have just one commit in the PR or did you mean that I should just clean up the history a little bit?

pdyraga · 2019-08-16T11:19:14Z

Thanks for all the comments. Closing here, and opening on go-ethereum soon.

pdyraga added 5 commits June 17, 2019 19:19

Added dependency to github.com/keep-network/blake2-f

bc39c31

keep-network/blake2 dependency updated to the newest version

4d5287c

Reverted mistakenly modified rootPath in vendor.json

214f72f

pdyraga commented Jun 20, 2019

View reviewed changes

core/vm/contracts.go Outdated Show resolved Hide resolved

pdyraga mentioned this pull request Jun 20, 2019

Blake2b F precompile keep-network/EIPs#2

Closed

mhluongo mentioned this pull request Jun 21, 2019

EIP-152: Blake2b F precompile ethereum/EIPs#2129

Merged

hadv reviewed Jun 24, 2019

View reviewed changes

holiman suggested changes Jun 24, 2019

View reviewed changes

holiman reviewed Jun 24, 2019

View reviewed changes

s1na mentioned this pull request Jun 24, 2019

[Istanbul] EIP-2129/152: Add Blake2b F Hash Function ethereumjs/ethereumjs-util#203

Closed

pdyraga commented Jun 24, 2019

View reviewed changes

core/vm/contracts.go Show resolved Hide resolved

pdyraga added 4 commits June 25, 2019 19:42

Accept abi.encodePacked parameters to F precompile

e7fafe2

We decided to switch the behaviour of F precompile to accept abi.encodePacked parameters instead of left-padded ones (for values smaller than 32 bytes).

Allow only 213-bytes input in F precompile

fc1426f

Set Blake2b F precompile price to 1gas/round

5e9c041

Reordered imports

c2bdb96

No changes in what modules are imported. I've just removed the empty line and auto-formatted imports.

holiman reviewed Jun 25, 2019

View reviewed changes

core/vm/contracts.go Outdated Show resolved Hide resolved

holiman reviewed Jun 25, 2019

View reviewed changes

core/vm/contracts.go Outdated Show resolved Hide resolved

holiman reviewed Jun 25, 2019

View reviewed changes

Use blake2FInputLength constant to check the input length

0124f1f

Co-Authored-By: Martin Holst Swende <martin@swende.se>

gitcoinbot mentioned this pull request Jun 27, 2019

Champion wanted: BLAKE2b precompile ethcatherders/PM#64

Closed

iikirilov reviewed Jun 30, 2019

View reviewed changes

pdyraga added 2 commits July 1, 2019 14:50

Made blake2FInputLength a constant

cbd54af

Updaded keep-network/blake2b reference to the newest version

b4e46b1

We now point to keep-network/blake2b/compression to include only what is really needed - F compression function

pdyraga added 5 commits July 1, 2019 15:50

Switched to the new interface of compression.F, updated test vectors

b667e1c

compression.F accepts `m` parameter as an array of uint64 instead of byte slice now. I have also improved test vectors to cover various cases of rounds parameter.

Accept rounds parameter as the first one to F precompile

97b4056

Gas price depends solely on the number of rounds. Having this parameter as the first one lets to evaluate gas price easier.

nonzero f bytes are considered as 'true'

f4b25c5

Simplified condition assigning a value to f parameter

d5ff1ab

Gas calculation function reads number of rounds from the first 4 bytes

d907056

I made a mistake when refactoring `Run(input []byte)` to do the same and not updated `RequiredGas(input []byte)`. Fixing my mistake now.

Updated blake2b compression lib reference to point to v1.0 release

a8a06f1

Instead of pointing to a commit with no official tag, we now point to an exact release number.

daira reviewed Jul 4, 2019

View reviewed changes

holiman mentioned this pull request Aug 6, 2019

Istanbul hard-fork meta ethereum/go-ethereum#19919

Closed

10 tasks

karalabe reviewed Aug 7, 2019

View reviewed changes

pdyraga added 3 commits August 9, 2019 13:58

Accept h,m,t bytes for BLAKE2b precompile as little-endian

251a800

Replaced generated test vectors with one from BLAKE2b RFC

f245982

BLAKE2b RFC Appendix A specifies test vector that we have implemented here. See https://tools.ietf.org/html/rfc7693#appendix-A For now, we leave blake2FTests are slice because we'll probably add more test vectors there.

Strict validation of the final block indicator flag

4877166

The final block indicator flag for F precompile must be either 0 or 1.

holiman reviewed Aug 13, 2019

View reviewed changes

Decode test vector with Hex2Bytes before modifying it

0e51e86

pdyraga closed this Aug 16, 2019


		rounds := binary.BigEndian.Uint32(input[316:320])

		blake2.F(&h, m, t, f, int(rounds))

	return nil, errBlake2FIncorrectInputLength
	return nil, errors.New("invalid input length for Blake2 F precompile")

Blake2b F precompile #4

Blake2b F precompile #4

Conversation

pdyraga commented Jun 20, 2019 • edited Loading

Description

Istanbul hardfork

Sample call

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhluongo Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman Jun 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman commented Jun 24, 2019

pdyraga commented Jun 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iikirilov Jun 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdyraga commented Jul 1, 2019

pdyraga commented Jul 2, 2019

Choose a reason for hiding this comment

holiman commented Jul 29, 2019

pdyraga commented Jul 29, 2019

holiman commented Aug 6, 2019

pdyraga commented Aug 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman commented Aug 16, 2019

pdyraga commented Aug 16, 2019

holiman commented Aug 16, 2019

pdyraga commented Aug 16, 2019

pdyraga commented Aug 16, 2019

pdyraga commented Jun 20, 2019 •

edited

Loading

mhluongo Jun 24, 2019 •

edited

Loading

holiman Jun 24, 2019 •

edited

Loading

holiman Jun 25, 2019 •

edited

Loading

holiman Jun 24, 2019 •

edited

Loading

iikirilov Jun 30, 2019 •

edited

Loading