Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Implementation of EIP 2537 #11707

Merged
merged 9 commits into from
Jun 4, 2020

Conversation

shamatar
Copy link

This PR implements EIP 2537 using the full reference implementation from here (specialized BLS12-381 engine).

Both json specs and native builtins are implemented (pricers and executors).

Closes #11677

@ordian
Copy link
Collaborator

ordian commented May 13, 2020

cc #11697

@vorot93 vorot93 requested review from adria0 and sorpaas May 21, 2020 11:02
@vorot93 vorot93 added the M4-core ⛓ Core client code / Rust. label May 21, 2020
ethcore/builtin/src/lib.rs Outdated Show resolved Hide resolved
ethcore/builtin/Cargo.toml Outdated Show resolved Hide resolved
@adria0
Copy link

adria0 commented May 22, 2020

I'll review the https://github.com/matter-labs/eip1962/tree/1c79dba99c970c99066e249627c29b38dc949677 that is the used revision at this moment.

@shamatar
Copy link
Author

Updated with a constant and specific revision

ethcore/builtin/src/lib.rs Outdated Show resolved Hide resolved
@adria0 adria0 marked this pull request as ready for review May 29, 2020 21:30
@adria0 adria0 added the A0-pleasereview 🤓 Pull request needs code review. label May 29, 2020
@adria0
Copy link

adria0 commented Jun 2, 2020

@sorpaas, if you have time, could you review it, please?

@vorot93 vorot93 mentioned this pull request Jun 3, 2020
Copy link

@vorot93 vorot93 left a comment

Choose a reason for hiding this comment

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

LGTM modulo some convention nits by @sorpaas and me

json/src/spec/builtin.rs Outdated Show resolved Hide resolved
json/src/spec/builtin.rs Outdated Show resolved Hide resolved
ethcore/builtin/src/lib.rs Outdated Show resolved Hide resolved
ethcore/builtin/src/lib.rs Outdated Show resolved Hide resolved
@shamatar
Copy link
Author

shamatar commented Jun 3, 2020

I'll fix those tomorrow

@shamatar
Copy link
Author

shamatar commented Jun 4, 2020

@vorot93 Ubuntu tests looks to go fine, but were cancelled due to MacOS tests. For MacOS tests I can not see logs and find an error, can you help me with it?

@shamatar
Copy link
Author

shamatar commented Jun 4, 2020

All tests pass on my MacOS laptop by the way

@vorot93
Copy link

vorot93 commented Jun 4, 2020

Restarted the tests

Comment on lines 4896 to 4898
"0x7fffffffffffff": {
"price": { "bls12_const_operations": { "price": 600 }}
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
"0x7fffffffffffff": {
"price": { "bls12_const_operations": { "price": 600 }}
}
"bls12_const_operations": { "price": 600 }

Either you set

  • activate_at: block_number, and pricing > bls12_const_operations if there's a single pricing, or
  • pricing > block_number > price > bls12_const_operations if pricing is expected to change

but not both!

Copy link
Author

Choose a reason for hiding this comment

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

This part I didn't change, only fixed names there and activation to be in a distant future as was suggested above.

Copy link

Choose a reason for hiding this comment

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

I may have missed this part, but still needs to be fixed.

Copy link
Author

Choose a reason for hiding this comment

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

Should be fixed now

Comment on lines 126 to 128
"0": {
"price": { "bls12_const_operations": { "price": 600 }}
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
"0": {
"price": { "bls12_const_operations": { "price": 600 }}
}
"bls12_const_operations": { "price": 600 }

also the same in this spec

@vorot93 vorot93 added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 4, 2020
@vorot93 vorot93 merged commit 1b0bbd5 into openethereum:master Jun 4, 2020
adria0 pushed a commit that referenced this pull request Jun 9, 2020
dvdplm added a commit that referenced this pull request Jun 30, 2020
* master:
  Update parity-crypto dependency (#11791)
  ethcore/private-tcx: fix deadlock caused by conflicting lock order (#11764)
  Feature/publish draft release ci (#11786)
  Trigger custom docker build when adding ci-build-docker to the commit (#11782)
  Fix alpine docker image entry point (#11749)
  Use BigUint::modpow for ModExp precompile (#11772)
  Add YOLO-1 testnet (#11757)
  Upgrade num to 0.2, use enum-primitive-derive (#11636)
  Update Simple Subroutines to the latest spec  (#11731)
  update secret-store reference (#11761)
  Implementation of EIP 2537 (#11707)
  Add missing forks to fork ID (#11747)
  Use nightly tag for master branch (#11732)
dvdplm added a commit that referenced this pull request Jun 30, 2020
…ereum/openethereum into dp/chore/upgrade-to-rocksdb-0.14

* 'dp/chore/upgrade-to-rocksdb-0.14' of github.com:openethereum/openethereum:
  Update parity-crypto dependency (#11791)
  ethcore/private-tcx: fix deadlock caused by conflicting lock order (#11764)
  Feature/publish draft release ci (#11786)
  Trigger custom docker build when adding ci-build-docker to the commit (#11782)
  Fix alpine docker image entry point (#11749)
  Use BigUint::modpow for ModExp precompile (#11772)
  Add YOLO-1 testnet (#11757)
  Upgrade num to 0.2, use enum-primitive-derive (#11636)
  Update Simple Subroutines to the latest spec  (#11731)
  update secret-store reference (#11761)
  Implementation of EIP 2537 (#11707)
  Add missing forks to fork ID (#11747)
  Use nightly tag for master branch (#11732)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement EIP2537 - precompile for BLS12-381 curve operations
6 participants