Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Implement EIP-152: Add Blake2 compression function F precompile #5751

Merged
merged 9 commits into from
Sep 23, 2019
Merged

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Sep 19, 2019

@gumb0 gumb0 added this to the Istanbul milestone Sep 19, 2019
@codecov-io
Copy link

codecov-io commented Sep 19, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@7d82c60). Click here to learn what that means.
The diff coverage is 91.08%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #5751   +/-   ##
========================================
  Coverage          ?   63.8%           
========================================
  Files             ?     358           
  Lines             ?   30604           
  Branches          ?    3403           
========================================
  Hits              ?   19526           
  Misses            ?    9848           
  Partials          ?    1230

@gumb0 gumb0 mentioned this pull request Sep 20, 2019
18 tasks
uint64_t w;
memcpy(&w, src, sizeof w);
return w;
#else
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chfast Should we care about big endian platforms?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so.


s.t[0] = load64(_t0.data());
s.t[1] = load64(_t1.data());
s.t[0] = *reinterpret_cast<uint64_t const*>(_t0.data());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should stay load64 because otherwise this is undefined behavior if the memory is not aligned to 8 bytes.

BLAKE2B_BLOCKBYTES = 128,
};

typedef struct blake2b_state__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop typedef.


enum blake2b_constant
{
BLAKE2B_BLOCKBYTES = 128,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convert to constexpr.

} blake2b_state;

// clang-format off
const uint64_t blake2b_IV[8] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constexpr.

return w;
}

inline uint64_t rotr64(const uint64_t w, const unsigned c) noexcept
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
inline uint64_t rotr64(const uint64_t w, const unsigned c) noexcept
inline constexpr uint64_t rotr64(uint64_t w, unsigned c) noexcept

{
uint64_t m[16];
uint64_t v[16];
size_t i;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move i to loops.


#include <libdevcore/Exceptions.h>

// The Blake 2 F compression function implemenation is based on the reference implementation,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chfast do we need any kind of license note here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not required because the reference implementation is in public domain, but I think it is nice to leave it here.

@gumb0
Copy link
Member Author

gumb0 commented Sep 23, 2019

benchmarking test vectors from EIP (+ another one for 1200 rounds) currently looks like this to me

Running tests using path: "../../test/jsontests"
Running 1 test case...
Test Case "bench_blake2compression": 
bench_blake2compression/test4: 67 ns
bench_blake2compression/test5: 343 ns
bench_blake2compression/test6: 257 ns
bench_blake2compression/test7: 57 ns
bench_blake2compression/test_1200rounds: 18502 ns
Running tests using path: "../../test/jsontests"
Running 1 test case...
Test Case "bench_blake2compression_maxrounds": 
bench_blake2compression_maxrounds/test8: 66246045792 ns

@gumb0 gumb0 removed the in progress label Sep 23, 2019
@gumb0
Copy link
Member Author

gumb0 commented Sep 23, 2019

This passes state tests from ethereum/tests#619, so I'm going to merge soon

@gumb0 gumb0 merged commit f57fa86 into master Sep 23, 2019
@gumb0 gumb0 deleted the eip-152 branch September 23, 2019 15:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants