Add blake2_f precompile #11017

dvdplm · 2019-09-02T20:19:51Z

As per EIP 152, adds a new precompile at 0x09 to expose the Blake2 compression function, aka the "F function".

Spec: https://tools.ietf.org/html/rfc7693#section-3.2
EIP-152: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-152.md

Closes #10994

Note: the F compression function in this PR is not optimised (or rather, it's optimised for readability ;). This will be addressed in a separate PR as the intention here is to be 1. spec compliant and 2. get the precompile plumbing in place.

Note-2: The chainspecs are not yet updated a part from foundation.json. Would appreciate a review of the format used there and I'll replicate it elsewhere.

Co-Authored-By: David <[email protected]>

* master: EIP 1108: Reduce alt_bn128 precompile gas costs (#11008) Fix deadlock in `network-devp2p` (#11013) Implement EIP-1283 reenable transition, EIP-1706 and EIP-2200 (#10191)

Add remaining test vectors

seunlanlege

Looks good 😎

ethcore/builtin/src/lib.rs

json/src/spec/builtin.rs

* master: Extract snapshot to own crate (#11010) Edit publish-onchain.sh to use https (#11016)

Co-Authored-By: Andronik Ordian <[email protected]>

Return a cost of u32::MAX if conversion fails Fix docs and todos

Co-Authored-By: Niklas Adolfsson <[email protected]>

Co-Authored-By: Andronik Ordian <[email protected]>

…:paritytech/parity-ethereum into dp/feature/eip-152-add-blake2-precompile * 'dp/feature/eip-152-add-blake2-precompile' of github.com:paritytech/parity-ethereum: Update json/src/spec/builtin.rs Update json/src/spec/builtin.rs Update util/EIP-152/src/lib.rs

niklasad1 · 2019-09-04T09:42:34Z

@dvdplm do we got any benches for this?

Would be nice but maybe it is intended to be part of #11020?

EDIT: nvm, didn't read the issue properly sorry

niklasad1

LGTM

dvdplm · 2019-09-04T09:48:14Z

Let's not merge this PR before I say so. There are problems with the EIP being discussed in the gitter and until we know what is going to happen we're better off not merging this.

ethcore/res/ethereum/foundation.json

holiman · 2019-09-09T10:33:40Z

Let's not merge this PR before I say so. There are problems with the EIP being discussed in the gitter and until we know what is going to happen we're better off not merging this.

It was decided on ACD on friday that nothing changes on the implementation: ethereum/EIPs#152 (comment) .

There seems to be some work going on in the background to make use of a BLAKE2b-variant with non-standard rounds. Unfortunately @mhluongo could not divulge further details.

It is unknown what would be a reasonable upper bound for rounds for such a non-standard BLAKE2b. As a result the rounds parameter seemed to be left at 32-bit.

It was agreed that the EIP should be updated to reflect it describes a very specific superset of BLAKE2b, but still a subset of BLAKE2. The EIP also should include the actual compression function and parameters (SIGMA, IV) – and not leave it as a reference to the RFC.

There was no agreement about introducing the m_len field, which is a devex improvement and gas saving. It would make sense for client implementers to judge the complexity and decide upon that.

Please get this merged so we can kick off cross-client testing. Btw: did you guys implement Istanbul in state tests yet?

* master: fix: remove unused error-chain (#11028) fix: remove needless use of itertools (#11029) Convert `std::test` benchmarks to use Criterion (#10999) Fix block detail updating (#11015) [trace] introduce trace failed to Ext (#11019) cli: update usage and version headers (#10924) [private-tx] remove unused rand (#11024)

sorpaas · 2019-09-09T13:47:59Z

ethcore/builtin/src/lib.rs

+			// 1. immutable,
+			// 2. comprised of `u64`
+			// …and because we're using the (known) length of `h`
+			slice::from_raw_parts(h.as_ptr() as *const u8, h.len() * size_of::<u64>())


It might be better to remove this unsafe. What I worry is someone compiling parity-ethereum to a big endian platform. There may not be many, but once people do that it breaks consensus. We may want to just specify the exact encoding how conversion from u64 array to bytearray happens.

There could be other problems with big endian, e.g. with ethash https://github.com/paritytech/parity-ethereum/blob/5baa7e8fb50512b0de11b0994fa1b136de289831/ethash/src/compute.rs#L273-L276

~~but in this case it should work with big endian if I'm not mistaken~~

…he-right-place * master: cleanup json crate (#11027) [spec] add istanbul test spec (#11033) [json-spec] make blake2 pricing spec more readable (#11034) Add blake2_f precompile (#11017) Add new line after writing block to hex file. (#10984) fix: remove unused error-chain (#11028) fix: remove needless use of itertools (#11029) Convert `std::test` benchmarks to use Criterion (#10999) Fix block detail updating (#11015) [trace] introduce trace failed to Ext (#11019) cli: update usage and version headers (#10924) [private-tx] remove unused rand (#11024)

* feat: implement eip1108 * doc nit: price per point pair Co-Authored-By: David <[email protected]> * fix: test-build * fix: update chain specs * fix: basic_authority.json chain spec * fix: change `eip_transition == 0x7fffffffffffff` * Merge na-eip1108 * Pre-compile plumbing * Use type alias instead of struct * Extract the data from incoming EVM call and forward it to blake2_f * Fetch parity-common from git for now * Fix broken cost impl Add remaining test vectors * cleanup * Trailing comma in json spec * Update ethcore/builtin/src/lib.rs Co-Authored-By: Andronik Ordian <[email protected]> * Update ethcore/builtin/src/lib.rs Co-Authored-By: Andronik Ordian <[email protected]> * Rename pricer to be Blake2-specific Return a cost of u32::MAX if conversion fails Fix docs and todos * Fix error handling in cost() * fix chainspec * EIP-152 crate * Update ethcore/builtin/src/lib.rs Co-Authored-By: Seun LanLege <[email protected]> * Address grumbles * Update ethcore/builtin/src/lib.rs Co-Authored-By: Niklas Adolfsson <[email protected]> * Switch tests to use hex! * remove parity-crypto from git * Sort out the SIGMA * Prefer arrays to vecs * Update util/EIP-152/src/lib.rs Co-Authored-By: Niklas Adolfsson <[email protected]> * Update json/src/spec/builtin.rs Co-Authored-By: Andronik Ordian <[email protected]> * Update json/src/spec/builtin.rs Co-Authored-By: Andronik Ordian <[email protected]> * Review feedback * Do not change chainspecs yet * Do not assume endianess: precompile output is LE

* add more tx tests (#11038) * Fix parallel transactions race-condition (#10995) * Add blake2_f precompile (#11017) * [trace] introduce trace failed to Ext (#11019) * Edit publish-onchain.sh to use https (#11016) * Fix deadlock in network-devp2p (#11013) * EIP 1108: Reduce alt_bn128 precompile gas costs (#11008) * xDai chain support and nodes list update (#10989) * EIP 2028: transaction gas lowered from 68 to 16 (#10987) * EIP-1344 Add CHAINID op-code (#10983) * manual publish jobs for releases, no changes for nightlies (#10977) * [blooms-db] Fix benchmarks (#10974) * Verify transaction against its block during import (#10954) * Better error message for rpc gas price errors (#10931) * Fix fork choice (#10837) * Fix compilation on recent nightlies (#10991)

* add more tx tests (#11038) * Fix parallel transactions race-condition (#10995) * Add blake2_f precompile (#11017) * [trace] introduce trace failed to Ext (#11019) * Edit publish-onchain.sh to use https (#11016) * Fix deadlock in network-devp2p (#11013) * EIP 1108: Reduce alt_bn128 precompile gas costs (#11008) * xDai chain support and nodes list update (#10989) * EIP 2028: transaction gas lowered from 68 to 16 (#10987) * EIP-1344 Add CHAINID op-code (#10983) * manual publish jobs for releases, no changes for nightlies (#10977) * [blooms-db] Fix benchmarks (#10974) * Verify transaction against its block during import (#10954) * Better error message for rpc gas price errors (#10931) * tx-pool: accept local tx with higher gas price when pool full (#10901) * Fix fork choice (#10837) * Cleanup unused vm dependencies (#10787) * Fix compilation on recent nightlies (#10991)

* master: (70 commits) ethcore: remove `test-helper feat` from build (#11047) Include test-helpers from ethjson (#11045) [ethcore]: cleanup dependencies (#11043) add more tx tests (#11038) Fix parallel transactions race-condition (#10995) [ethcore]: make it compile without `test-helpers` feature (#11036) Benchmarks for block verification (#11035) Move snapshot related traits to their proper place (#11012) cleanup json crate (#11027) [spec] add istanbul test spec (#11033) [json-spec] make blake2 pricing spec more readable (#11034) Add blake2_f precompile (#11017) Add new line after writing block to hex file. (#10984) fix: remove unused error-chain (#11028) fix: remove needless use of itertools (#11029) Convert `std::test` benchmarks to use Criterion (#10999) Fix block detail updating (#11015) [trace] introduce trace failed to Ext (#11019) cli: update usage and version headers (#10924) [private-tx] remove unused rand (#11024) ...

niklasad1 and others added 11 commits August 29, 2019 17:16

feat: implement eip1108

7bf1f51

doc nit: price per point pair

0e2c0c1

Co-Authored-By: David <[email protected]>

fix: test-build

4a9bc52

fix: update chain specs

4a10715

fix: basic_authority.json chain spec

7c54aa4

fix: change eip_transition == 0x7fffffffffffff

0e8ae07

Merge na-eip1108

42044fe

Pre-compile plumbing

6194306

Use type alias instead of struct

8abba67

Extract the data from incoming EVM call and forward it to blake2_f

1de24c7

Fetch parity-common from git for now

2975573

dvdplm requested a review from sorpaas September 2, 2019 20:20

dvdplm self-assigned this Sep 2, 2019

dvdplm added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Sep 2, 2019

dvdplm requested review from seunlanlege and removed request for sorpaas September 2, 2019 20:20

dvdplm added 4 commits September 2, 2019 22:22

Merge branch 'master' into dp/feature/eip-152-add-blake2-precompile

30aceb5

* master: EIP 1108: Reduce alt_bn128 precompile gas costs (#11008) Fix deadlock in `network-devp2p` (#11013) Implement EIP-1283 reenable transition, EIP-1706 and EIP-2200 (#10191)

Fix broken cost impl

e23bf2c

Add remaining test vectors

cleanup

41f044c

Trailing comma in json spec

3fd92e9

seunlanlege approved these changes Sep 3, 2019

View reviewed changes

ordian reviewed Sep 3, 2019

View reviewed changes

dvdplm and others added 3 commits September 3, 2019 12:12

Merge branch 'master' into dp/feature/eip-152-add-blake2-precompile

edd360a

* master: Extract snapshot to own crate (#11010) Edit publish-onchain.sh to use https (#11016)

Update ethcore/builtin/src/lib.rs

4c1ddbb

Co-Authored-By: Andronik Ordian <[email protected]>

Update ethcore/builtin/src/lib.rs

dea870d

Co-Authored-By: Andronik Ordian <[email protected]>

ordian added this to the 2.7 milestone Sep 3, 2019

ordian added B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 M4-core ⛓ Core client code / Rust. labels Sep 3, 2019

Rename pricer to be Blake2-specific

af5ac2e

Return a cost of u32::MAX if conversion fails Fix docs and todos

dvdplm and others added 5 commits September 4, 2019 11:25

Update util/EIP-152/src/lib.rs

81d6556

Co-Authored-By: Niklas Adolfsson <[email protected]>

Update json/src/spec/builtin.rs

68c376e

Co-Authored-By: Andronik Ordian <[email protected]>

Update json/src/spec/builtin.rs

ad61a15

Co-Authored-By: Andronik Ordian <[email protected]>

Review feedback

362bd90

niklasad1 approved these changes Sep 4, 2019

View reviewed changes

ordian added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Sep 4, 2019

ordian reviewed Sep 9, 2019

View reviewed changes

ethcore/res/ethereum/foundation.json Outdated Show resolved Hide resolved

ordian removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Sep 9, 2019

Do not change chainspecs yet

aa258e7

sorpaas reviewed Sep 9, 2019

View reviewed changes

Do not assume endianess: precompile output is LE

b1dbd02

sorpaas approved these changes Sep 9, 2019

View reviewed changes

sorpaas added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 9, 2019

sorpaas merged commit d8d7abc into master Sep 9, 2019

sorpaas deleted the dp/feature/eip-152-add-blake2-precompile branch September 9, 2019 22:26

This was referenced Sep 12, 2019

v2.6.3-beta #11040

Merged

v2.5.8-stable #11041

Merged

niklasad1 mentioned this pull request Nov 1, 2019

[chain specs]: activate Istanbul on mainnet #11228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add blake2_f precompile #11017

Add blake2_f precompile #11017

dvdplm commented Sep 2, 2019 •

edited

Loading

seunlanlege left a comment

niklasad1 commented Sep 4, 2019 •

edited

Loading

niklasad1 left a comment •

edited

Loading

dvdplm commented Sep 4, 2019

holiman commented Sep 9, 2019 •

edited

Loading

sorpaas Sep 9, 2019

ordian Sep 9, 2019 •

edited

Loading

Add blake2_f precompile #11017

Add blake2_f precompile #11017

Conversation

dvdplm commented Sep 2, 2019 • edited Loading

seunlanlege left a comment

Choose a reason for hiding this comment

niklasad1 commented Sep 4, 2019 • edited Loading

niklasad1 left a comment • edited Loading

Choose a reason for hiding this comment

dvdplm commented Sep 4, 2019

holiman commented Sep 9, 2019 • edited Loading

sorpaas Sep 9, 2019

Choose a reason for hiding this comment

ordian Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

dvdplm commented Sep 2, 2019 •

edited

Loading

niklasad1 commented Sep 4, 2019 •

edited

Loading

niklasad1 left a comment •

edited

Loading

holiman commented Sep 9, 2019 •

edited

Loading

ordian Sep 9, 2019 •

edited

Loading