Client: refactor execution

[holgerd77]

Ready for review, see description below.

[codecov]

Codecov Report

Merging #1068 (6fe4a1d) into master (28ea476) will decrease coverage by 0.96%.
The diff coverage is 80.57%.

Flag	Coverage Δ
block	81.11% <80.95%> (-0.07%)	⬇️
blockchain	82.83% <80.95%> (-0.19%)	⬇️
client	87.55% <80.17%> (-0.61%)	⬇️
common	86.28% <100.00%> (-0.51%)	⬇️
devp2p	82.73% <ø> (?)
ethash	82.08% <ø> (ø)
tx	90.15% <ø> (+0.15%)	⬆️
vm	83.09% <ø> (ø)

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

[holgerd77]

Ok, ready. 😄

Some summary on this PR: this work here was initially triggered when I wanted to add some additional HF logic to the execution logic in FullSynchronizer and realized that this is not sustainable since the synchronizer class got more and more bloated and mixed up with the execution logic.

I have therefore in a first step along 3351298, 7633061 and (later) c437c28 extracted the execution code parts from FullSynchronizer and created an own execution module, which has it's own base class Execution and a subclass VMExecution, following the structural setup of the other client modules. This should help us to write cleaner execution logic code and better separate the functionality. It also - maybe this is the main point - decouples the execution from the sync logic, so this should prepare both for adding additional forms of execution (e.g. some form of StatelessExecution) as well as using the same execution modules for different types of sync (e.g. beam sync).

Note that the current structure of the execution classes is rather meant to be some start, so feel free to evolve on this along future work.

On a second step I have then integrated the execution code from VM.runBlockchain() directly in the execution module here 413a392 following the suggestion from @jochem-brouwer in this discussion #1066 (comment). This gives us more control over the code since we now have both the blockchain iterator run and the enclosing try/catch block in the client which we will need to solve issues like #1066 (not yet integrated in this PR) as well as generally handle execution failures appropriately. This integration was a bit more tricky (a least for me) than I thought, my initial goal here has been to make this functionally equivalent to the version before. Also feel free here to evolve on this code part.

With these changes from above the current testdouble based testing of VM execution with mocking several functions came finally to its limits. I've therefore added a new static constructor factory Blockchain.fromBlocksData() (would fromBlockData() be a better name?) in ed3f007 which allows for one-line creation of Blockchain objects and which I have used in ed3f007 and a3c32cc to switch to a real-data based testing for VM execution in the client. Note that test coverage can still be improved, rather a start here.

On a4683e6 and 87bc662 I've finally added HF switch logic for both the chain download and the execution. There are now two common instances chainCommon and execCommon created based upon the common instance provided (this is still needed to allow e.g. to pass in a custom chain, see one of the tests for an example). These two common instances are then updated by the synchronizer (chainCommon) and the execution class (execCommon) to independently track the respective active hardforks to correctly submit block download requests (the forkhash changes on HF switches) as well as VM executions. All this is rather some preparation and will likely need some real world testing and some refinement. We can do this (better) once #1032 is merged with the early-on occuring HF switches on Rinkeby and Goerli.

Ok. Open for review. 😄

[holgerd77]

Phew, the fight along a5421b8 can't be seen in this commit. 😜

(actually a whole morning)

[cgewecke]

👍

Does the vm's execCommon need a similar reset here?

[holgerd77]

No, not here since the HF switches on block download and block execution occur independently. A HF change for execution is handled along the VMExecution.run() method.

[cgewecke]

This all looks excellent to me! Great. 💯

Just one question about how common works in FullSync's nextBlockFork section...

I think the fromBlocksData name makes sense / is correct.

[holgerd77]

Rebased this on top of the clique PR #1032 - needed to add a small typing fix. I will also merge this one in (with admin merge), feel the very strong need to streamline things a bit due to the current extra constraints and PR has been reviewed by @cgewecke, let me nevertheless know if you think this is inappropriately deviating from the protocol.

Then we have both of the large PRs out of the way and can concentrate on doing "normally" scoped work again 😄 and @jochem-brouwer (and eventually others) has (have) a solid and stable base for further work on #1048.

Let me nevertheless know though if you have final comments on this PR, happily doing follow-up PRs on this doing latest fixes or changes.