Add precompiledContracts VM option

[rumkin]

This is the solution for #650 issue. What have been done:

Changes

1. Method VM#init() added.
2. Property VM#isInitialized added.
3. VM constructor option precompiledContracts added.

Checklist

• Create backward compatible automatic initialization:
  • Add VM#init() method.
  • Add VM#isInitialized flag.
  • Update methods which require initialization.
• Test VM to initialize custom precompiles.

[lgtm-com]

This pull request introduces 2 alerts when merging 8226cec into 1824cd0 - view on LGTM.com

new alerts:

• 2 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging e995b39 into 1824cd0 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

fixed alerts:

• 1 for Unused variable, import, function or class

[codecov]

Codecov Report

Merging #660 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #660      +/-   ##
==========================================
+ Coverage    91.4%   91.48%   +0.07%     
==========================================
  Files          31       31              
  Lines        1978     1996      +18     
  Branches      326      328       +2     
==========================================
+ Hits         1808     1826      +18     
  Misses         90       90              
  Partials       80       80

Flag	Coverage Δ
#vm	91.48% <100%> (+0.07%)	⬆️

Impacted Files	Coverage Δ
lib/evm/evm.ts	91.85% <100%> (ø)	⬆️
lib/evm/precompiles/index.ts	100% <100%> (ø)	⬆️
lib/index.ts	98.55% <100%> (+0.43%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1824cd0...eb7e4f8. Read the comment docs.

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging fbe6f62 into 1824cd0 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

fixed alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging eb7e4f8 into 1824cd0 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

fixed alerts:

• 1 for Unused variable, import, function or class

[alcuadrado]

Hey, thanks for this PR @rumkin !

I'm not sure what was the intention behind it, though.

The activate precompiles doesn't actually set the precompiles, but just touches the precompiles accounts. This option was added specifically for testing networks because if those accounts were left untouched/non-existent, their first call would be slightly more expensive. While this behavior is correct, it can be surprising to people who expect the testing network costs to resemble mainnet's.

IIRC, the precompiles are activated or not depending on the currently used HF. Maybe @s1na can confirm this.

Why do you want to pass the VM your own precompiles? Can't you do something similar by creating a custom instance of Common?

[rumkin]

Hi, @alcuadrado. Thanks for joining.

Well, I'm solving here the problem with the floating promises in a VM constructor. I dug into the engine's code to understand how it all works and what's the activatePrecompiles are for. By mistake I decided they are redundant for the thing you described. So now I know that this option is important and couldn't be deleted. But precompiled contracts are hardcoded into the EVM code and just using the same set for everything. To change that I added internal precompiledContracts property to store contracts. But also I made them configurable.

What's about precompiled contracts passed into VM constructor. At first it's important for me. I'm building a service for Ethereum developers and I need to allow people to experiment a lot with the Ethereum engine. Also it would be useful to researchers who want to use it in non-standard way. Probably it would be useful for Buidlr too.

The second thing is that this is required for the issue #652 to apply previous versions of the opcodes, to add new one, or update existing in a more sophisticated way. So I'm planning to add support of the Common to use in the constructor for building the list of active precompiles depending on hardfork version. But if the goal is to implement automatic switching internals, it could require more work and could bring breaking changes. That's why I implemented the minimal working solution.

[s1na]

Hey @rumkin, thanks a lot for all the investigation, solutions and the PR!

I think as you and Pato said, having precompiles be configurable based on the hardfork is a great first step.

After that we can discuss whether we should allow user-configurable precompiles. One reason to reflect on this further is because precompiles are somehow trusted code in EVM. We also don't have an explicit address range for them. I imagine this could be an easy way for developers to shoot themselves in the foot and if we later on want to remove it from the public API it'll be a breaking change.

I definitely see the value in allowing researchers to set their own precompiles (e.g. to test future HFs). That's why I'd be more up for having a flexible VM implementation which allows researchers to explicitly inherit the VM and make it easy for them to provide their own precompiles.

Now on HF-based precompiles, I suggest we first start doing it in the VM, kinda like how we started doing for opcodes and then possibly extracting that to Common (maybe after the mono-repoization!). That shouldn't be very hard I think and can be easily tested.

[rumkin]

@s1na Seems like this PR raises too much questions and solve two different kinds of issues. I moved the initialization issue out to separated PR (#665), please check it.

Now I'm closing this and will open an issue to discuss the probable solution.

@alcuadrado Thanks for participating.

[alcuadrado]

Thanks for all your collaborations @rumkin, they are very appreciated