Add HF consistency checks

[jochem-brouwer]

In this specific test I noticed that I had to specifically pass on the Common object which is used by the VM in order to get the right results. I would assume that any package which is used downstream (tx/block) would use the Common of the VM in case it is ran. But this is not the case. One can create a VM which points to Istanbul, and then instantiate a block which uses Chainstart. This leads to the very weird situation where the EVM executes rules according to Istanbul (e.g. gas calculations), but the block will validate the rules against Chainstart. This sounds like a very bug-prone feature and it seems very inconsistent to me.

I'd say a way to fix this is to call setHardfork on the tx/block before we run it and thus set the common to the same hardfork as the one the VM is using. But this would be a breaking change as now suddenly the Common of tx/block can switch forks when you run it. We could also explicitly communicate that if you want to ensure "correctness", then you need to always pass on the common of the VM in case you create a tx/block.

One other nasty problem here is that in the constructor of block/tx there are some validations/calculations which explicitly depend upon the fork. In block it checks DAO extra data, and if calcDifficultyFromHeader is set to true then it also calculates difficulty (which could thus change if you are on a different fork due to the difficulty bomb). In tx it also verifies that the v value of the signature is correct.

[holgerd77]

From my perspective this is neither a bug nor an inconsistency but is a by-design choice.

Both the VM and e.g. a block are independent entities. If you create a VM with common set to istanbul, you get a VM comforming to the istanbul rules. If you instantiate a block e.g. retrieved by RPC from byzantium times you should instantiate it with byzantium.

Then you can decide how to use these independent entities. If you then run this byzantium block in the istanbul VM this is prone to get somewhat wrong. And this won't get any better if you automatically switch common from the block. In this example you would force a Byzantium block into an istanbul behavior, likely even more error prone than the original behavior might be.

Therefore you've got these two "operation modes".

1. If you are in a static HF context you create an outer Common and then use this for both instantiations.
2. If you want rather the HF to switch on context one can use the hardforkByBlockNumber options like e.g. done in this example

[jochem-brouwer]

Gave this some thought, good points.

Do you think we should add explicit checks in the VM and Block in the future? If you add Txs to a Block with the wrong common, it will throw. Same if you try to run a Block with a different Common HF in the VM?

[holgerd77]

Haven't given this much thought yet, but - yeah - such checks might be worth to add I would say on first thought.

[holgerd77]

Have updated the title to reflect the latest state of the discussion.

[holgerd77]

Do you think we should add explicit checks in the VM and Block in the future? If you add Txs to a Block with the wrong common, it will throw. Same if you try to run a Block with a different Common HF in the VM?

I guess these consistency checks Jochem mentioned would still make sense. Good first issue since very local changes.