Implement EIP-1344: CHAINID opcode in LegacyVM

[gumb0]

https://eips.ethereum.org/EIPS/eip-1344

Note that it activates CHAINID for version 1, that is CHAINID works only in contracts newly-deployed with external transaction after Istanbul fork. Account versioning is disabled now in Istanbul.

@winsvega Note the changes in testeth: for now I leave it to CHAINID always returning zero in tests. To make it work properly, you'd need to put correct chainID (read from config) into EnvInfos used there. It seemed not very trivial to change it in this PR, because of state tests running with several configs for each test and other complications in testeth code.

[codecov-io]

Codecov Report

Merging #5696 into master will increase coverage by 0.08%.
The diff coverage is 92.04%.

@@            Coverage Diff            @@
##           master   #5696      +/-   ##
=========================================
+ Coverage   63.02%   63.1%   +0.08%     
=========================================
  Files         353     353              
  Lines       30120   30164      +44     
  Branches     3378    3380       +2     
=========================================
+ Hits        18984   19036      +52     
+ Misses       9907    9903       -4     
+ Partials     1229    1225       -4

[chfast]

Looks ok, but was this EIP accepted?

[gumb0]

@chfast It was not, I think it's better not to merge until it is.

[fubuloubu]

Under discussion this Friday. Should have a yay/nay from that.

[fubuloubu]

Was accepted for Istanbul as of the call today, however @karalabe made the suggestion that implementations should reference a hard-coded value of chain ID present in the client instead of the value provided by EIP-155 compatible transactions (which the reference implementation uses)

[gumb0]

@fubuloubu This is what we already do here - returning constant value from chain config.

[gumb0]

I'll disable account versioning for Istanbul before merging this.

[fubuloubu]

Are there consensus tests for this new opcode in https://github.com/ethereum/tests yet?

[winsvega]

No. Somebody on it?

[fubuloubu]

Don't think so, but I can help with that. Two test scenarios I can think of:

1. CHAINID opcode (0x46) when called prior to Istanbul fork block produces revert due to invalid opcode
2. CHAINID opcode after Istanbul fork block pushes the network's chain ID (obtained from the client's configuration) onto the stack

[winsvega]

I think this is a good case to create a template tests for new opcode.
then make sure that we have this template tests for every opcode.

by template I mean construction of tests with following scenarios:
OPCODE CALL

1. opcode called from tx init code
   1.2) opcode called from create init code
   1.3) opcode called from crreate2 init code
2. opcode called in contract code
3. opcode called by (CALLCODE/DELEGATE/CALL/STATICCALL...)
4. opcode called on the second level (point3 * point3)

OPCODE OOG
5) opcode called then OOG happens
6) opcode called by point(3) then OOG happens
7) opcode called by point(3) then REVERT happens
8) opcode called and OOG happens upon calling the opcode (copy of point (5,6,7))

and so on ideas...

[gumb0]

Rebased.

[gumb0]

Rebased.

[winsvega]

the chainid is parsed from genesis config, right?

[gumb0]

@winsvega Yes, in aleth it is. For testeth see my comment in the PR description.

[winsvega]

looks like chainid is unreasonable for state tests. we just moved all blockchain specific logic tests into blockchain test suite. and now for chainid in state tests we will need to reimplement mock function again. chainid and network are two different parameters. moreover for blockchain tests it will require test format change as well.

I would like to seal the current state of tests before doing this changes.

[holiman]

looks like chainid is unreasonable for state tests. we just moved all blockchain specific logic tests into blockchain test suite. and now for chainid in state tests we will need to reimplement mock function again. chainid and network are two different parameters. moreover for blockchain tests it will require test format change as well.

No, we just need to agree on the chainid for statetests and blockchain tests.