[FIX] Return executed blocks sorted on execution error

[ntallar]

Description

Fixes missing blocks on testnet, caused by:

1. An invalid block (1505: 0b) was generated by the miner of the node (due to https://jira.iohk.io/browse/ETCM-301), probably caused by an expected race condition between the miner and the importing process
2. The above block import required a reorg of more than one block. From [1502: 8c, 1503: 87, 1504: 6c] to [1502: 8a, 1503: 5a, 1504: 0b, 1505: 0b] (the list are: [block number: first two bytes of the hash])
3. Failure in executing it caused the reorg to be reverted and BlockImport.revertChainReorganisation to be called with the executedBlocks=[1505: 0b, 1504: 0b, 1503: 5a, 1502: 8a]
4. This caused the call: removeBlocksUntil(0b, 1502), meaning that all blocks from the db were deleted
5. Eventually block 1506 had an ommer whose parent was deleted in the last step, meaning that it was marked as invalid by this node with reason OmmersNotValidError. As this was the chain followed by the two other miners, the testnet irrevocably forked

Simple scenario to reproduce:

Pre-requisites:

• Due to https://jira.iohk.io/browse/ETCM-329 the mocked miner generates invalid blocks in some occasions, the ValidationAfterExecError(s"Block has invalid state root hash, expected ${Hex.toHexString(header.stateRoot.toArray)} but got ${Hex.toHexString(stateRootHash.toArray)}") validation has to be disabled
• Patch Mantis to consider a block to be invalid, I did so by changing BlockExecution.executeAndValidateBlock so that all blocks with number 13 are considered invalid

Reproducing it

1. Start a node and mine 12 blocks in a mocked way
2. Mine 5 blocks having the block 8 as it's parent
3. As the last mined block has number 13 it will be detected as invalid
4. Query the blocks by number (through JSON RPC) and check that blocks 1-7 were deleted

Proposed Solution

Return the executedBlocks in proper order

Testing

• Run the scenario to reproduce and see that it no longer causes any issues
• No test was added due to the complexity of producing this exact scenario

[mirkoAlic]

i'm struggling with this analysis in the code. Maybe as a work arround we could add a suffix at variables name level, in order to know if the list is in incremental or decremental order. WDYT?

ex.
remainingBlocks -> remainingBlocksIncOrder // [bn=1, bn=2, bn=3]
executedBlocks -> executedBlocksDecOrder // [bn=3, bn=2, bn=1]

And also add a comment regarding the order of the list returned by this function

[mirkoAlic]

wdyt about adding a regression test at least at integration test level?

[ntallar]

Regression test at an integration level wasn't that easy, I finally added one at unit level that was quite straightforward to add

[mirkoAlic]

LGTM!