Skip to content
This repository has been archived by the owner. It is now read-only.

[Hackathon] Blake2b f compression precompile #1614

Merged
merged 14 commits into from Aug 2, 2019

Conversation

@iikirilov
Copy link
Contributor

iikirilov commented Jun 27, 2019

PR description

Introduces the Blake2b F compression function as a precompile in Pantheon as specified in ethereum/EIPs#2129

Using the test vectors from https://github.com/keep-network/go-ethereum/blob/f-precompile/core/vm/contracts_test.go#L350

Implementation of Blake2b F compression function ported from https://github.com/keep-network/blake2 (BSD license)

Fixed Issue(s)

PegaSysEng/BountiedWork#19

@iikirilov iikirilov force-pushed the iikirilov:BLAKE2b_hackathon branch 4 times, most recently from c8664fa to c32274d Jun 27, 2019
Copy link
Member

shemnon left a comment

Looks reasonable so far.

@iikirilov iikirilov force-pushed the iikirilov:BLAKE2b_hackathon branch 9 times, most recently from 56d68dd to 2d5c73d Jul 2, 2019
@iikirilov iikirilov force-pushed the iikirilov:BLAKE2b_hackathon branch from 2d5c73d to e4fecfa Jul 10, 2019
@iikirilov

This comment has been minimized.

Copy link
Contributor Author

iikirilov commented Jul 11, 2019

This PR has passed the deadline for the hackathon period but it is still incomplete.

  1. The precompile was not integrated with the precompile registry.
  2. The EIP has not been finalized so some implementation details/test vectors may change.
  3. The gas calculation was not tested.
  4. The name of the precompile was not agreed.

I have the capacity to follow up on this should that be requested by the reviewer.

@iikirilov iikirilov force-pushed the iikirilov:BLAKE2b_hackathon branch from e4fecfa to d722745 Jul 14, 2019
@shemnon

This comment has been minimized.

Copy link
Member

shemnon commented Jul 26, 2019

The gas cost appears to be specified in ethereum/EIPs#2129. While this is still open the got some positive feedback on the All Core Devs call. Can you finish the gas cost calculations and function registration based on these values?

@iikirilov

This comment has been minimized.

Copy link
Contributor Author

iikirilov commented Jul 26, 2019

@shemnon you will find that the gas requirement method already reflects this. Should this be abstracted to the FrontierGasCalculator?

@shemnon

This comment has been minimized.

Copy link
Member

shemnon commented Jul 26, 2019

No need to bring it into frontier. You listed some other TODOs -

  1. The precompile was not integrated with the precompile registry.
  2. The EIP has not been finalized so some implementation details/test vectors may change.
  3. The gas calculation was not tested.
  4. The name of the precompile was not agreed.

For 1 this task is outstanding.
For 2 I am of the opinion the changes won't be dramatic, and likely there will be no more changes.
For 3 can you add some simple gas calculation tests based on the existing test vectors
For 4 lets call it "BLAKE2BF", if a different name comes up it's a trivial change we can handle later.

@iikirilov iikirilov force-pushed the iikirilov:BLAKE2b_hackathon branch from d722745 to f33393c Jul 30, 2019
@iikirilov

This comment has been minimized.

Copy link
Contributor Author

iikirilov commented Jul 30, 2019

I have addressed the above. Waiting for the tests cases to be updated and we should be good to go.

iikirilov added 7 commits Jun 27, 2019
*represent uint as long for safe mod and comparison operation. Using UnsignedInts.remainder(divedent, divisor) converts int to long for each round.
*use BC Pack utils for long <-> bytes
@iikirilov iikirilov force-pushed the iikirilov:BLAKE2b_hackathon branch from 3e838d0 to cecadd3 Jul 31, 2019
iikirilov and others added 3 commits Jul 31, 2019
@shemnon
shemnon approved these changes Aug 2, 2019
@shemnon shemnon merged commit 2168b8c into PegaSysEng:master Aug 2, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details
@iikirilov iikirilov deleted the iikirilov:BLAKE2b_hackathon branch Aug 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants
You can’t perform that action at this time.