EIP support for the VM and Common

[holgerd77]

This is my suggestion for a gentle introduction on EIP support for the VM. The solution plays well with our current setup, is backwards-compatible but should also be flexible enough to carry us on the first introductions here (and maybe will turn out sufficient as a longer-term solution as well).

So this can be used in the VM as follows:

if (this._activatedEIPs.includes('EIP2537') {
  const gasPrice = this._common.paramByEIP('gasPrices', 'Bls12381G1AddGas', 'EIP2537')
  // Activate the precompiles or whatever is to be done
}

Once EIPs move into HFs we can easily shift the EIP config in Common to the respective HF config on the proposed solution.

Some design decisions:

1. EIP names for API on the VM: I've chosen the string version here - so e.g. 'EIP2537' and not the plain number 2537. I find this better readable since EIPs are always stated with an EIP-prefix and I don't see a downside. But at the end this is a bit a matter of subjective taste and I would also be open for a change if someone has strong opinions on this.

2. param() vs paramByEIP(): paramByEIP() could theoretically also have been integrated into param() and the function would still be well-defined. I decided it's not worth it and overloads the semantics of this function with the need for too much textual explanation, e.g. on the case of (as it is now) automatically falling back to the HF-centric version when no HF is provided. So - therefore - an extra function, think this doesn't hurt.

3. EIP and HF dependencies: I couldn't decide on a design decision for eventual EIP and/or HF dependencies (so if e.g. an EIP depends on another EIP or should only be executed in a certain HF environment) - as we talked about in the call. I especially couldn't decide if to integrate in Common or the VM directly. So for now I just left this, I think we can let this sink in, gain some experiences on EIP implementations and decide/add later. For EIP2537 I imagine this doesn't play a role (@jochem-brouwer: correct me if I am wrong here), since there is no EIP dependency and the EIP is so isolated in its functionality that the precompiles should run in whatever HF environment.

I've already done the EIP2537 integration here, since this made also the test creation easier (possible).

@jochem-brouwer this might be a good starting ground for the suggestion to start fresh on #785 as suggested in the thread there (so to manually take things over) once/in case we merge here. Hope this should then be doable without too much hazzle. 😬 😄

[codecov]

Codecov Report

Merging #856 into master will decrease coverage by 1.33%.
The diff coverage is 100.00%.

Flag	Coverage Δ
#account	92.85% <ø> (ø)
#block	78.22% <ø> (?)
#blockchain	81.03% <ø> (ø)
#common	94.32% <100.00%> (+0.34%)	⬆️
#ethash	83.33% <ø> (+1.11%)	⬆️
#tx	94.16% <ø> (ø)
#vm	92.73% <100.00%> (+0.02%)	⬆️

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

[jochem-brouwer]

Some small nitpick comments and one test which is not being ran. Looks good in general, complete with tests and docs! Once this is merged, will merge this in the BLS PR and start using it 😄

[ryanio]

Looks great!

My only thought about EIP names for API on the VM is that I just converted the opcodes tables/list to Map for speed/performance reasons (#852), and I think we should use the same pattern here like const EIPs: Map<number, EIP>

[holgerd77]

@ryanio had a short look but I can't really imagine what should be mapped to what respectively how that would look like. Another aspect: is this really meaningful for performance on such short lists?

For now I'll leave this to @jochem-brouwer to review and eventually merge. Maybe you can do a short follow-up PR (or do along some other work) on this if it turns out we want to change.

[jochem-brouwer]

I agree with @holgerd77 here, I don't think it makes much sense to use a Map just for performance. But if it makes our code more consistent and would not complicate things as much it might be worth a PR at some point @ryanio.