Add EIP-1884 support for Istanbul

[s1na]

I just added a basic test case to see SELFBALANCE works. It should be tested more thoroughly when Istanbul tests are merged into ethereumjs-testing.

Fixes #579

[s1na]

Tests are failing because I changed the base cost of SLOAD, BALANCE and EXTCODEHASH and base costs are not currently HF-dependent.

One naive solution would be to keep base costs as they are and in the opcode handler, charge the extra gas when current HF is greater than Istanbul.

I think what I'll do is have a list of opcodes modified (or added) in Istanbul as a separate array merge that with the base array if current HF is greater than Istanbul. This goes along the plan of having HF-configurable opcodes (#543).

[s1na]

Apparently there has been some discussion on the last ACD about the SLOAD gas repricing freezing some contracts. For now it doesn't seem to affect the EIP, but we should keep an eye on it.

[coveralls]

Coverage increased (+0.01%) to 95.157% when pulling 56350e1 on istanbul/eip-1884 into edffcde on master.

[holgerd77]

Hmm, this breaks with the concept of having gas price changes collected in the Common library (e.g. for SLOAD), I only expected the Istanbul opcode list to include the newly introduced opcodes (SELFBALANCE).

For the moment this would introduce a lot of inconsistency - we also have added other gas price changes for Istanbul in the istanbul.json file.

Do you have a bigger plan for a complete transition here respectively a strong reasoning for this break apart? Otherwise I would very much suggest we add the price changes to the Common library.

[s1na]

Hmm, I didn't know the base cost of opcodes are also updated in Common (in addition to extra gas parameters like ecAdd). If that has been the intention, there's no corresponding code in the VM to read those base gas costs from Common:

https://github.com/ethereumjs/ethereumjs-vm/blob/edffcde9696283799efc4ab17d0c3e481618c727/lib/evm/interpreter.ts#L105-L112

If we want to keep base gas costs in Common we'd have to:

• Make sure costs for all opcodes are in Common (not the case now, searching swap in ethereumjs-common returned empty)
• Remove them from opcodes.ts to only have one source of truth
• Modify Interpreter to read costs from Common

[holgerd77]

Hmm, maybe we are talking side by side a bit? What is with SLOAD e.g. - the EIP is stating that this is changing gas costs from 200 to 800 - and the 200 price is just "normally" defined in the tangerine whistle json file in the Common library (see link above).

This is just a "normal" gas price change, isn't it?

[holgerd77]

Ah, just had another look. See the point, these gas costs are not taken by Common at all atm. Maybe this is just more of some inconsistency/incompleteness in the Common library.

Anyhow, you are right. Will approve here.

[holgerd77]

Looks good.

[holgerd77]

Ok, I would relatively quickly target a v4.1 release after this is merged, eventually waiting for #577 from @alcuadrado.

What is with the stateless execution prototype #556 (not urgent), do you want to have that included?

[s1na]

The stateless execution prototype we can defer to a future release.

It should be however possible to include EIP-2028 (#570) also in the release. It depends on ethereumjs-tx updating its ethereumjs-common dependency, and then updating both in the VM. Or do you think we should include it in the next release?

[holgerd77]

Makes very much sense to also include EIP-2028.