[ETCM-377] Fix consistency issue between cache and persisted block number on rollbacks

[ntallar]

Description

Fixes None.get problems on testnet when accessing the best block, this were caused by having our block number higher that the block we had

Proposed Solution

• Refactor removeBlock method for easier readability
• Treat best blocks cache as the most up-to-date values
• When saving block, move persistance of best block number only after the in-memory block number was possibly updated
• When removing a block, update the persisted best blocks if they would be referring to blocks we no longer have

I finally decided not to remove the caching of mpt nodes from our archive state storage to not over extend this PR, we can include that later

Testing

Added unit tests should pass

[KonradStaniec]

question: I am not sure why we need to update this block in storage when we update it when we update it here:

    if (withState)
      stateStorage.onBlockRollback(block.number, bestBlockNumber) { () => persistBestBlocksData() }

when cache is full or requires flushing

[ntallar]

It's not necessary for consistency's sake

But I wanted to follow the same approach than here, it's also best to keep this value as up-to-date as possible.

Though we have some conditions before to update this values, it could have not been the case if they were false

[KonradStaniec]

As you are fixing best block number handling when rollbackin, do you think this ugly thing is still necessary in BlockchainImpl:

  //FIXME EC-495 this method should not be need when best block is handled properly during rollback
  def persistCachedNodes(): Unit = {
    if (stateStorage.forcePersist(RollBackFlush)) {
      persistBestBlocksData()
    }
  }

We always call it before rollbacks happen (it is ugly hack from the past, as there was some troubles with cached block number when rollbacking earlier)

[ntallar]

Hmm it doesn't seem to be necessary and I'd gladly remove it... though I'm a bit unsure just in case I add further possibilities of introduced issues by changing that

Though if you agree with removing it (or other reviewers do) I'll remove it

[mirkoAlic]

IMHO, if we can remove it now, let's do it! =)

[mirkoAlic]

${ByteStringUtils.hash2string(blockHash)} ?

[KonradStaniec]

As for code it lgtm, did you try to run it on mordor/mainnet or some local privnet wheere you can freerly kill peers ?

[ntallar]

As for code it lgtm, did you try to run it on mordor/mainnet or some local privnet wheere you can freerly kill peers ?

Given our time constraints I only synced some blocks from mordor (with regular sync) to some minimal checks, I'll re-deploy the nomad testnet after this PR gets merged to check how it works there

[mirkoAlic]

LGTM!

[KonradStaniec]

ahh testing in prod :D let it be then!