VM: Implement EIP 2537 (BLS12-381 precompiles) #785

jochem-brouwer · 2020-06-18T16:16:20Z

Closes #738

codecov · 2020-06-18T16:18:24Z

Codecov Report

Merging #785 into master will decrease coverage by 5.91%.
The diff coverage is 35.39%.

Flag	Coverage Δ
#account	`92.85% <ø> (ø)`
#block	`?`
#blockchain	`81.03% <ø> (-0.31%)`	⬇️
#common	`94.50% <ø> (+0.17%)`	⬆️
#ethash	`83.33% <ø> (ø)`
#tx	`94.02% <ø> (ø)`
#vm	`82.28% <35.39%> (-10.45%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

jochem-brouwer · 2020-06-18T16:19:01Z

So there's currently 1 precompile in it (out of 9): the G1ADD precompile. I added some comments and TODO's for things I want to add.

One of the problems I have myself with the current changed I made is that the precompiles are now either async or not-async. I don't think this is clean, so I'd love to know what your thoughts are. The alternative is to make all precompiles by default async (even if they are not) and that thus PrecompileFunc always returns a Promise<ExecResult>.

The reason the precompiles are async is that the MCL package needs to get initialized; this is async. If you then try to call any of the MCL functions the chance is that MCL itself is not initialized and thus throws.

CC @evertonfraga @s1na @holgerd77 @cgewecke

jochem-brouwer · 2020-06-18T16:37:32Z

Woops. It appears we have an async VM.init() function. So I'll revert the function signatures of the precompiles back to the old ones. Only have to figure out how to pass the MCL package in a clean way to the precompile functions.

holgerd77 · 2020-06-19T10:44:31Z

Hi @jochem-brouwer, great start, nice! 😄

Short note: you can use the pre-defined labels here to indicate your PR status (e.g. "WIP" or "Ready for Review")

jochem-brouwer · 2020-06-20T16:01:52Z

Heya! I have decided to keep keep support for async precompile functions. I think this might get useful in the future. Since the precompile functions are not exposed, this should not be a problem (?).

I have added the mcl package directly in the VM. It is globally instantiated and every BLS-precompile will read this package directly from the VM. The VM itself is now passed as an attribute of the PrecompileInput.

Thoughts? @holgerd77 @s1na @cgewecke @evertonfraga

jochem-brouwer · 2020-06-20T18:15:55Z

Note to self: if a file where MCL is imported in throws, then for some reason MCL also throws an extremely verbose error which we should suppress.

lgtm-com · 2020-06-20T18:37:40Z

This pull request introduces 1 alert when merging 1d061e3 into fb82f40 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

jochem-brouwer · 2020-06-21T13:46:37Z

Linking issues of the mcl-wasm package;

holgerd77 · 2020-06-23T08:19:58Z

@jochem-brouwer was scratching my head a bit on this VM -> precompile thing (passing the VM as a parameter and instantiate MCL in the VM, but I understand the reasoning (so to state explicitly: you want to avoid the performance/memory penalty on re-instantiating MCL again and again on every precompile, correct me if I am wrong)).

We shouldn't do these kind of things too often - here it might be justified for now regarding the gains. Otherwise we introduce too much non-easy-to-grasp complexity. Also have no other idea atm, at least not without starting a somewhat major precompile structure refactoring, and give this some statefulness by making Precompiles an object or whatever.

Anyhow - no need to totally get distracted by solving this - but if someone has an alternative idea please post here.

jochem-brouwer · 2020-06-23T09:13:17Z

@holgerd77 Yes this problem is a bit confusing because it actually is two problems:

We have 9 precompiles which all need access to the same library.
The library has an init() function which is async. In this PR support for async precompiles is added, otherwise there's this condition that the library itself is not yet initialized and the precompile thus throws.

Upon thinking about 1): it does make sense if require always returns the same instance of the loaded library (99% sure this is correct). In that case I can remove this pointer to the VM in the PrecompileInput type. The only thing which I don't like here is that I find that initializing this library should be in VM.init(). Otherwise, every precompile has to manually await the promise of initializing this library. I find this a bit ugly and I don't think the code of the precompile is the right way to do this.

So just to summarize the options:

Keep VM pointer in the PrecompileInput so we can initialize the async init function of the library in VM.init().
Remove the VM pointer in PrecompileInput and instead explicitly await the init function of the library in each call to any of the 9 precompiles.

(2 does of course not impact performance; I just find the location of awaiting this initialization a bit ugly)

Thoughts? @holgerd77 @ryanio @cgewecke @s1na

jochem-brouwer · 2020-06-26T16:56:34Z

@evertonfraga Do you see why mcl-wasm is not installing on the checks?

evertonfraga · 2020-06-26T20:02:05Z

@jochem-brouwer I see the cause: it's using the CI code from master and trying to use for your PR, instead of using the CI instructions from your own branch.

I fail to understand why, though. It must be some new feature from github actions, because only they have control over these things.

The jobs being run in this PR are already using the dependency cache, even though the code for it is not integrated into this branch.

Loading cache: CI run
But your branch does not use cache in CI: eip-2537 vm-build.yml
Only master does: master vm-build.yml

evertonfraga · 2020-06-26T20:03:51Z

I'd recommend to update this branch with latest master, and make sure to commit and push your package-lock.json.

jochem-brouwer · 2020-07-06T15:33:45Z

Cool - now we got all precompiles working! Only thing left to add are some error checks (only G1 failing tests work at this point) and a minor amount of cleaning some things so the code is consistent.

jochem-brouwer · 2020-07-28T07:47:13Z

Very exciting news! All tests from the test dir pass locally.

However, I am having a discussion about this particular test currently as I am not entirely sure if this test is correct.

To do: integrate the official state tests into ethereumjs-testing and then also run the tests on these.

lgtm-com · 2020-07-28T07:48:58Z

This pull request introduces 1 alert when merging 0259294 into 071fdd9 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

lgtm-com · 2020-07-28T08:00:46Z

This pull request introduces 1 alert when merging e5a4a03 into 071fdd9 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

jochem-brouwer · 2020-07-28T08:13:37Z

Browser tests are timing out. We might want to remove all these csv files anyways since they will be tested in the state tests if these are implemented in the ethereumjs-testing lib.

lgtm-com · 2020-07-30T05:43:16Z

This pull request introduces 1 alert when merging 3f5a092 into 071fdd9 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

jochem-brouwer · 2020-07-30T06:52:12Z

npm run build && npm run test:state -- --fork=Berlin --dir=stEIP2537

1..1403
# tests 1403
# pass  1403

😄 . (Running on a local fork of the tests repository, as currently ethereumjs-testing does not support the newest tests yet)

[VM] reject G2 points not on curve [VM] lint and add two more negative test cases [VM] BLS: add invalid subgroup checks

[VM] lint

[VM] fix gas used on BLS precompiles in case of an error [VM] lint [VM] bump ethereumjs-testing version

jochem-brouwer · 2020-09-06T15:40:59Z

Sat down and did first checked every commit in order to cleanup the changelog, then rebased master. Looks like I did a git merge at some point instead of git rebase which clogged up the commit logs. Also squashed the commits to a healthy amount of 13 (currently).

As we have discussed we will (temporarily) disable BLS precompiles in browser until we figure out what is causing these problems on the CI.

jochem-brouwer · 2020-09-06T16:13:53Z

I am removing the BLS EIP from Berlin HF and implementing the VM EIP support.

Some notes:

(1) as we have discussed we should move the EIP support to Common.

(2) we should move precompile availability checks into Common. precompiles/index.ts is now getting a bit bloated to check this availability, and it would make a lot of sense that this support is in Common.

If at some point we implement an EIP which wants to create a precompile at one of the addresses which is already occupied (like an EIP which wants to use a BLS address) then we can check in the Common (1) if there is a precompile and (2) what it is, as it might not be the case that "precompile is available" means that it is always the same one.

[VM] add berlin tests to package/CI

jochem-brouwer · 2020-09-06T17:42:05Z

Tests should now pass, the ones which run EIP2537 are disabled in browser now. Also note that I removed some tests from the PR CI (Dao and Homestead). Opened an issue regarding that BLS does not run in browser.

holgerd77

Updated the branch, tests pass, this looks good now, thanks @jochem, really impressive work! 😄

I gave this only a structural review and mainly looked at the integration code parts, these look good. I also skimmed through the implementation of the single precompiles itself but just for a rough look. Since I don't think that we realistically get someone on the level to do an in-depth review of the curve logic here I think we can (and should, to be able to move forward) on the current state of the DRAFT EIP merge in here and open this up for experimentation. Official tests pass which gives a somewhat high (with some deficits as Jochem mentioned) level of assurance that the implementation is correct. If the implementation is given a final ok for Berlin integration we should nevertheless give this code another look and eventually also actively help (now with the new tutorial on test creation 😄 ) to add to the official test suite with more test cases.

One high-level note on the precompile implementation: on a structural level there is a high level of structural similarities - e.g. on the zero bytes check - I wonder if it's worth to add 1-3 util functions which can be shared and reduce the size of the code base here.

Will now merge this in. The EIP integration part will need some update on the shift to Common, but I can do that on the follow-up work on #863

holgerd77 · 2020-09-09T09:02:29Z

packages/vm/README.md

+Currently supported EIPs are:
+
+- [EIP-2537](https://eips.ethereum.org/EIPS/eip-2537): BLS precompiles
+


Won't make this a blocker but please do not update README files along feature PRs yet. Let's please keep this to the release PRs once the releases are actually done. Then one can go through the - now already existing 😄 - release notes and do a consistent update on the README files. This will otherwise confuse users of the latest releases published on npm.

We do update the changelog right? So the changelog does have new versions (which are not available on npm yet, which is also a bit confusing), but we keep the readme at the latest published version?

holgerd77 · 2020-09-09T09:16:30Z

packages/vm/lib/evm/precompiles/index.ts

 }

-function getPrecompile(address: string, common: Common): PrecompileFunc {
+function getPrecompile(address: string, common: Common, vm: VM): PrecompileFunc {


Note: this new vm parameter can be removed again once we have EIP support moved to Common.

Yes, should fix once that is merged in

jochem-brouwer added package: vm PR state: WIP labels Jun 19, 2020

jochem-brouwer marked this pull request as draft June 20, 2020 18:43

This was referenced Jun 20, 2020

Cannot import scalar values from BLS12-381 Ethereum test suite herumi/mcl-wasm#9

Closed

mulVec is not exposed herumi/mcl-wasm#10

Closed

This was referenced Jun 21, 2020

Expose mclBnFp_mapToG1 and mclBnFp2_mapToG2 herumi/mcl-wasm#13

Closed

MapToG1 seems to return unexpected values (is it Draft07?) herumi/mcl#79

Closed

This was referenced Jun 25, 2020

Ethereum Core Devs Meeting 90 Agenda ethereum/pm#189

Closed

Incomplete testing kit EIP-2537 matter-labs/eip1962#11

Open

evertonfraga changed the title ~~[WIP] VM: Implement EIP 2537 (BLS12-381 precompiles)~~ VM: Implement EIP 2537 (BLS12-381 precompiles) Jun 30, 2020

jochem-brouwer added 12 commits September 6, 2020 17:19

add G2ADD precompile

8cd8251

[VM] add G1MUL precompile

5e61c65

[VM] add G2MUL precompile

508132d

[VM] add MapToFp2 precompile

54958e7

[VM] BLS precompiles: add out of gas. Clean tests

29a9fd2

[VM] add BLS Pairing precompile

f2013dd

[VM] add G1 multiExp and G2 multiExp precompiles

e88c1a7

[VM] BLS precompile: reject G1 points not on curve

c9a3d6e

[VM] add MapToG1 precompile

097ae28

[VM] reject G2 points not on curve [VM] lint and add two more negative test cases [VM] BLS: add invalid subgroup checks

[VM] EIP 2537: add extra tests / fix G1/G2 zero points

e86ba53

[VM] lint

[VM] fix gas schedule for BLS precompiles with variable length input

2679e62

[VM] fix gas used on BLS precompiles in case of an error [VM] lint [VM] bump ethereumjs-testing version

[VM] add Berlin state tests, add empty input test

db7474f

jochem-brouwer force-pushed the eip-2537 branch from 2f24bf9 to db7474f Compare September 6, 2020 15:28

jochem-brouwer added 2 commits September 6, 2020 19:11

[VM] remove BLS from Berlin HF, add EIP2537 support

806d334

[VM] add berlin tests to package/CI

[VM] skip EIP2537 browser tests

4763f36

jochem-brouwer requested review from evertonfraga, holgerd77, ryanio and s1na and removed request for s1na, ryanio and holgerd77 September 6, 2020 17:36

jochem-brouwer mentioned this pull request Sep 6, 2020

VM: Add BLS precompile support for browsers #862

Closed

Merge branch 'master' into eip-2537

e8865cf

holgerd77 approved these changes Sep 9, 2020

View reviewed changes

holgerd77 merged commit 3a33008 into master Sep 9, 2020

holgerd77 deleted the eip-2537 branch September 9, 2020 09:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VM: Implement EIP 2537 (BLS12-381 precompiles) #785

VM: Implement EIP 2537 (BLS12-381 precompiles) #785

jochem-brouwer commented Jun 18, 2020 •

edited

Loading

codecov bot commented Jun 18, 2020 •

edited

Loading

jochem-brouwer commented Jun 18, 2020 •

edited

Loading

jochem-brouwer commented Jun 18, 2020

holgerd77 commented Jun 19, 2020

jochem-brouwer commented Jun 20, 2020

jochem-brouwer commented Jun 20, 2020 •

edited

Loading

lgtm-com bot commented Jun 20, 2020

jochem-brouwer commented Jun 21, 2020

holgerd77 commented Jun 23, 2020 •

edited

Loading

jochem-brouwer commented Jun 23, 2020

jochem-brouwer commented Jun 26, 2020

evertonfraga commented Jun 26, 2020

evertonfraga commented Jun 26, 2020

jochem-brouwer commented Jul 6, 2020

jochem-brouwer commented Jul 28, 2020

lgtm-com bot commented Jul 28, 2020

lgtm-com bot commented Jul 28, 2020

jochem-brouwer commented Jul 28, 2020

lgtm-com bot commented Jul 30, 2020

jochem-brouwer commented Jul 30, 2020

jochem-brouwer commented Sep 6, 2020

jochem-brouwer commented Sep 6, 2020

jochem-brouwer commented Sep 6, 2020

holgerd77 left a comment

holgerd77 Sep 9, 2020

jochem-brouwer Sep 9, 2020

holgerd77 Sep 9, 2020

jochem-brouwer Sep 9, 2020

		Currently supported EIPs are:

		- [EIP-2537](https://eips.ethereum.org/EIPS/eip-2537): BLS precompiles

VM: Implement EIP 2537 (BLS12-381 precompiles) #785

VM: Implement EIP 2537 (BLS12-381 precompiles) #785

Conversation

jochem-brouwer commented Jun 18, 2020 • edited Loading

codecov bot commented Jun 18, 2020 • edited Loading

Codecov Report

jochem-brouwer commented Jun 18, 2020 • edited Loading

jochem-brouwer commented Jun 18, 2020

holgerd77 commented Jun 19, 2020

jochem-brouwer commented Jun 20, 2020

jochem-brouwer commented Jun 20, 2020 • edited Loading

lgtm-com bot commented Jun 20, 2020

jochem-brouwer commented Jun 21, 2020

holgerd77 commented Jun 23, 2020 • edited Loading

jochem-brouwer commented Jun 23, 2020

jochem-brouwer commented Jun 26, 2020

evertonfraga commented Jun 26, 2020

evertonfraga commented Jun 26, 2020

jochem-brouwer commented Jul 6, 2020

jochem-brouwer commented Jul 28, 2020

lgtm-com bot commented Jul 28, 2020

lgtm-com bot commented Jul 28, 2020

jochem-brouwer commented Jul 28, 2020

lgtm-com bot commented Jul 30, 2020

jochem-brouwer commented Jul 30, 2020

jochem-brouwer commented Sep 6, 2020

jochem-brouwer commented Sep 6, 2020

jochem-brouwer commented Sep 6, 2020

holgerd77 left a comment

Choose a reason for hiding this comment

holgerd77 Sep 9, 2020

Choose a reason for hiding this comment

jochem-brouwer Sep 9, 2020

Choose a reason for hiding this comment

holgerd77 Sep 9, 2020

Choose a reason for hiding this comment

jochem-brouwer Sep 9, 2020

Choose a reason for hiding this comment

jochem-brouwer commented Jun 18, 2020 •

edited

Loading

codecov bot commented Jun 18, 2020 •

edited

Loading

jochem-brouwer commented Jun 18, 2020 •

edited

Loading

jochem-brouwer commented Jun 20, 2020 •

edited

Loading

holgerd77 commented Jun 23, 2020 •

edited

Loading