[ETCM-709] Improve ommers validations (#948)

Replaced OmmersAncestorsError by more specific OmmerIsAncestorError and OmmerParentIsNotAncestorError.
Renamed OmmersNotValidError to OmmersHeaderError and added the underlying reason for failure
Refactored validateOmmersAncestors method.
Refactored validateOmmersHeaders method to not hide the underlying failing reason.
Reorganized some packages because some classes were not in the correct package and/or the Spec was not in the correct package. Renamed some specs to better reflect the class being tested

Author: Leonor Boga

src/ets/scala/io/iohk/ethereum/ets/blockchain/BlockchainTestConfig.scala

@@ -2,7 +2,7 @@ package io.iohk.ethereum.ets.blockchain
 
 import akka.util.ByteString
 import io.iohk.ethereum.consensus.Protocol
-import io.iohk.ethereum.consensus.ethash.validators.ValidatorsExecutor
+import io.iohk.ethereum.consensus.validators.std.ValidatorsExecutor
 import io.iohk.ethereum.domain.{Address, UInt256}
 import io.iohk.ethereum.utils.{BlockchainConfig, DaoForkConfig, MonetaryPolicyConfig}
 import org.bouncycastle.util.encoders.Hex

src/ets/scala/io/iohk/ethereum/ets/blockchain/ScenarioSetup.scala

@@ -3,7 +3,7 @@ package io.iohk.ethereum.ets.blockchain
 import akka.util.ByteString
 import io.iohk.ethereum.consensus.Protocol.NoAdditionalEthashData
 import io.iohk.ethereum.consensus.ethash.EthashConsensus
-import io.iohk.ethereum.consensus.ethash.validators.ValidatorsExecutor
+import io.iohk.ethereum.consensus.validators.std.ValidatorsExecutor
 import io.iohk.ethereum.consensus.{ConsensusConfig, FullConsensusConfig, TestConsensus, ethash}
 import io.iohk.ethereum.db.components.Storages.PruningModeComponent
 import io.iohk.ethereum.db.components.{EphemDataSourceComponent, Storages}
@@ -18,6 +18,7 @@ import io.iohk.ethereum.utils.BigIntExtensionMethods._
 import io.iohk.ethereum.utils.{BlockchainConfig, Config}
 import monix.execution.Scheduler
 import org.bouncycastle.util.encoders.Hex
+
 import scala.util.{Failure, Success, Try}
 
 object ScenarioSetup {

src/it/scala/io/iohk/ethereum/ledger/BlockImporterItSpec.scala

@@ -8,8 +8,9 @@ import io.iohk.ethereum.blockchain.sync.regular.{BlockFetcher, BlockImporter}
 import io.iohk.ethereum.checkpointing.CheckpointingTestHelpers
 import io.iohk.ethereum.consensus.{GetBlockHeaderByHash, GetNBlocksBack}
 import io.iohk.ethereum.consensus.blocks.CheckpointBlockGenerator
-import io.iohk.ethereum.consensus.ethash.validators.{OmmersValidator, StdOmmersValidator}
+import io.iohk.ethereum.consensus.ethash.validators.OmmersValidator
 import io.iohk.ethereum.consensus.validators.Validators
+import io.iohk.ethereum.consensus.validators.std.StdOmmersValidator
 import io.iohk.ethereum.domain._
 import io.iohk.ethereum.mpt.MerklePatriciaTrie
 import io.iohk.ethereum.utils.Config.SyncConfig
@@ -74,7 +75,7 @@ class BlockImporterItSpec
         getBlockHeaderByHash: GetBlockHeaderByHash,
         getNBlocksBack: GetNBlocksBack
     ) =>
-      new StdOmmersValidator(blockchainConfig, blockHeaderValidator)
+      new StdOmmersValidator(blockHeaderValidator)
         .validate(parentHash, blockNumber, ommers, getBlockHeaderByHash, getNBlocksBack)
   }
 

src/it/scala/io/iohk/ethereum/sync/RegularSyncItSpec.scala

@@ -152,7 +152,12 @@ class RegularSyncItSpec extends FreeSpecBase with Matchers with BeforeAndAfterAl
       _ <- peer1.waitForRegularSyncLoadLastBlock(length)
     } yield {
       assert(peer1.bl.getBestBlock().get.hash == peer2.bl.getBestBlock().get.hash)
-      assert(peer1.bl.getBestBlock().get.number == peer2.bl.getBestBlock().get.number && peer1.bl.getBestBlock().get.number == length)
+      assert(
+        peer1.bl.getBestBlock().get.number == peer2.bl.getBestBlock().get.number && peer1.bl
+          .getBestBlock()
+          .get
+          .number == length
+      )
       assert(peer1.bl.getLatestCheckpointBlockNumber() == peer2.bl.getLatestCheckpointBlockNumber())
     }
   }

src/it/scala/io/iohk/ethereum/sync/util/RegularSyncItSpecUtils.scala

@@ -7,7 +7,13 @@ import io.iohk.ethereum.Mocks.MockValidatorsAlwaysSucceed
 import io.iohk.ethereum.blockchain.sync.regular.BlockBroadcast.BlockToBroadcast
 import io.iohk.ethereum.blockchain.sync.regular.BlockBroadcasterActor.BroadcastBlock
 import io.iohk.ethereum.blockchain.sync.regular.BlockImporter.Start
-import io.iohk.ethereum.blockchain.sync.regular.{BlockBroadcast, BlockBroadcasterActor, BlockFetcher, BlockImporter, RegularSync}
+import io.iohk.ethereum.blockchain.sync.regular.{
+  BlockBroadcast,
+  BlockBroadcasterActor,
+  BlockFetcher,
+  BlockImporter,
+  RegularSync
+}
 import io.iohk.ethereum.blockchain.sync.regular.RegularSync.NewCheckpoint
 import io.iohk.ethereum.blockchain.sync.{PeersClient, SyncProtocol}
 import io.iohk.ethereum.checkpointing.CheckpointingTestHelpers
@@ -96,7 +102,8 @@ object RegularSyncItSpecUtils {
         broadcasterRef,
         pendingTransactionsManager,
         regularSync
-      ))
+      )
+    )
 
     lazy val regularSync = system.actorOf(
       RegularSync.props(

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcher.scala

@@ -98,6 +98,7 @@ class BlockFetcher(
 
   private def handleCommands(state: BlockFetcherState): Receive = {
     case PickBlocks(amount) => state.pickBlocks(amount) |> handlePickedBlocks(state) |> fetchBlocks
+
     case StrictPickBlocks(from, atLeastWith) =>
       // FIXME: Consider having StrictPickBlocks calls guaranteeing this
       // from parameter could be negative or 0 so we should cap it to 1 if that's the case
@@ -113,6 +114,7 @@ class BlockFetcher(
       }
 
       fetchBlocks(newState)
+
     case InvalidateBlocksFrom(blockNr, reason, withBlacklist) =>
       val (blockProvider, newState) = state.invalidateBlocksFrom(blockNr, withBlacklist)
       log.debug("Invalidate blocks from {}", blockNr)
@@ -157,7 +159,7 @@ class BlockFetcher(
 
   private def handleBodiesMessages(state: BlockFetcherState): Receive = {
     case Response(peer, BlockBodies(bodies)) if state.isFetchingBodies =>
-      log.debug(s"Received ${bodies.size} block bodies")
+      log.debug("Received {} block bodies", bodies.size)
       if (state.fetchingBodiesState == AwaitingBodiesToBeIgnored) {
         log.debug("Block bodies will be ignored due to an invalidation was requested for them")
         fetchBlocks(state.withBodiesFetchReceived)
@@ -171,9 +173,10 @@ class BlockFetcher(
               state.withBodiesFetchReceived.handleRequestedBlocks(newBlocks, peer.id)
           }
         val waitingHeadersDequeued = state.waitingHeaders.size - newState.waitingHeaders.size
-        log.debug(s"Processed $waitingHeadersDequeued new blocks from received block bodies")
+        log.debug("Processed {} new blocks from received block bodies", waitingHeadersDequeued)
         fetchBlocks(newState)
       }
+
     case RetryBodiesRequest if state.isFetchingBodies =>
       log.debug("Something failed on a bodies request, cancelling the request and re-fetching")
       val newState = state.withBodiesFetchReceived
@@ -182,8 +185,10 @@ class BlockFetcher(
 
   private def handleStateNodeMessages(state: BlockFetcherState): Receive = {
     case FetchStateNode(hash) => fetchStateNode(hash, sender(), state)
+
     case RetryFetchStateNode if state.isFetchingStateNode =>
       state.stateNodeFetcher.foreach(fetcher => fetchStateNode(fetcher.hash, fetcher.replyTo, state))
+
     case Response(peer, NodeData(values)) if state.isFetchingStateNode =>
       log.debug("Received state node response from peer {}", peer)
       state.stateNodeFetcher.foreach(fetcher => {
@@ -215,10 +220,13 @@ class BlockFetcher(
       }
       supervisor ! ProgressProtocol.GotNewBlock(newState.knownTop)
       fetchBlocks(newState)
+
     case MessageFromPeer(CommonMessages.NewBlock(block, _), peerId) =>
       handleNewBlock(block, peerId, state)
+
     case MessageFromPeer(PV64.NewBlock(block, _), peerId) =>
       handleNewBlock(block, peerId, state)
+
     case BlockImportFailed(blockNr, reason) =>
       val (peerId, newState) = state.invalidateBlocksFrom(blockNr)
       peerId.foreach(id => peersClient ! BlacklistPeer(id, reason))
@@ -259,8 +267,9 @@ class BlockFetcher(
     state.tryInsertBlock(block, peerId) match {
       case Left(_) if block.number <= state.lastBlock =>
         log.debug(
-          s"Checkpoint block ${ByteStringUtils.hash2string(blockHash)} is older than current last block ${state.lastBlock}" +
-            s" - clearing the queues and putting checkpoint to ready blocks queue"
+          "Checkpoint block {} is older than current last block {} - clearing the queues and putting checkpoint to ready blocks queue",
+          ByteStringUtils.hash2string(blockHash),
+          state.lastBlock
         )
         val newState = state
           .clearQueues()
@@ -279,7 +288,7 @@ class BlockFetcher(
         log.debug(error)
         handleFutureBlock(block, state)
       case Right(state) =>
-        log.debug(s"Checkpoint block [${ByteStringUtils.hash2string(blockHash)}] fit into queues")
+        log.debug("Checkpoint block [{}] fit into queues", ByteStringUtils.hash2string(blockHash))
         fetchBlocks(state)
     }
   }
@@ -289,20 +298,20 @@ class BlockFetcher(
     //ex. After a successful handshake, fetcher will receive the info about the header of the peer best block
     case MessageFromPeer(BlockHeaders(headers), _) =>
       headers.lastOption.map { bh =>
-        log.debug(s"Candidate for new top at block ${bh.number}, current known top ${state.knownTop}")
+        log.debug("Candidate for new top at block {}, current known top {}", bh.number, state.knownTop)
         val newState = state.withPossibleNewTopAt(bh.number)
         fetchBlocks(newState)
       }
     //keep fetcher state updated in case new mined block was imported
     case InternalLastBlockImport(blockNr) =>
-      log.debug(s"New mined block $blockNr imported from the inside")
+      log.debug("New mined block {} imported from the inside", blockNr)
       val newState = state.withLastBlock(blockNr).withPossibleNewTopAt(blockNr)
 
       fetchBlocks(newState)
 
     //keep fetcher state updated in case new checkpoint block was imported
     case InternalCheckpointImport(blockNr) =>
-      log.debug(s"New checkpoint block $blockNr imported from the inside")
+      log.debug("New checkpoint block {} imported from the inside", blockNr)
 
       val newState = state
         .clearQueues()
@@ -414,13 +423,16 @@ class BlockFetcher(
 
   private def handleRequestResult(fallback: FetchMsg)(msg: Any): Task[Any] = msg match {
     case failed: RequestFailed =>
-      log.debug("Request failed due to {}", failed)
+      log.warning("Request failed due to {}", failed)
       Task.now(fallback)
+
     case NoSuitablePeer =>
       Task.now(fallback).delayExecution(syncConfig.syncRetryInterval)
+
     case Failure(cause) =>
       log.error(cause, "Unexpected error on the request result")
       Task.now(fallback)
+
     case m =>
       Task.now(m)
   }

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockImporter.scala

@@ -15,10 +15,12 @@ import io.iohk.ethereum.network.PeerId
 import io.iohk.ethereum.ommers.OmmersPool.AddOmmers
 import io.iohk.ethereum.transactions.PendingTransactionsManager
 import io.iohk.ethereum.transactions.PendingTransactionsManager.{AddUncheckedTransactions, RemoveTransactions}
+import io.iohk.ethereum.utils.ByteStringUtils
 import io.iohk.ethereum.utils.Config.SyncConfig
 import io.iohk.ethereum.utils.FunctorOps._
 import monix.eval.Task
 import monix.execution.Scheduler
+import org.bouncycastle.util.encoders.Hex
 
 import scala.concurrent.duration._
 
@@ -207,7 +209,12 @@ class BlockImporter(
             tryImportBlocks(restOfBlocks, importedBlocks)
 
           case err @ (UnknownParent | BlockImportFailed(_)) =>
-            log.error("Block {} import failed", blocks.head.number)
+            log.error(
+              "Block {} import failed, with hash {} and parent hash {}",
+              blocks.head.number,
+              blocks.head.header.hashAsHexString,
+              ByteStringUtils.hash2string(blocks.head.header.parentHash)
+            )
             Task.now((importedBlocks, Some(err)))
         }
         .onErrorHandle {

src/main/scala/io/iohk/ethereum/consensus/ConsensusBuilder.scala

@@ -2,7 +2,7 @@ package io.iohk.ethereum.consensus
 
 import io.iohk.ethereum.consensus.Protocol.{NoAdditionalEthashData, RestrictedEthashMinerData}
 import io.iohk.ethereum.consensus.ethash.EthashConsensus
-import io.iohk.ethereum.consensus.ethash.validators.ValidatorsExecutor
+import io.iohk.ethereum.consensus.validators.std.ValidatorsExecutor
 import io.iohk.ethereum.nodebuilder._
 import io.iohk.ethereum.utils.{Config, Logger}
 

src/main/scala/io/iohk/ethereum/consensus/ethash/EthashConsensus.scala

@@ -13,15 +13,16 @@ import io.iohk.ethereum.consensus.ethash.blocks.{
   EthashBlockGeneratorImpl,
   RestrictedEthashBlockGeneratorImpl
 }
-import io.iohk.ethereum.consensus.ethash.validators.ValidatorsExecutor
 import io.iohk.ethereum.consensus.validators.Validators
+import io.iohk.ethereum.consensus.validators.std.ValidatorsExecutor
 import io.iohk.ethereum.domain.BlockchainImpl
 import io.iohk.ethereum.jsonrpc.AkkaTaskOps.TaskActorOps
 import io.iohk.ethereum.ledger.BlockPreparator
 import io.iohk.ethereum.ledger.Ledger.VMImpl
 import io.iohk.ethereum.nodebuilder.Node
 import io.iohk.ethereum.utils.{BlockchainConfig, Logger}
 import monix.eval.Task
+
 import scala.concurrent.duration._
 
 /**

src/main/scala/io/iohk/ethereum/consensus/ethash/blocks/EthashBlockGenerator.scala

@@ -5,12 +5,12 @@ import akka.util.ByteString
 import io.iohk.ethereum.consensus.ConsensusConfig
 import io.iohk.ethereum.consensus.blocks._
 import io.iohk.ethereum.consensus.difficulty.DifficultyCalculator
-import io.iohk.ethereum.consensus.ethash.validators.ValidatorsExecutor
 import io.iohk.ethereum.crypto.kec256
 import io.iohk.ethereum.domain._
 import io.iohk.ethereum.ledger.{BlockPreparator, InMemoryWorldStateProxy}
 import io.iohk.ethereum.utils.BlockchainConfig
 import io.iohk.ethereum.consensus.ConsensusMetrics
+import io.iohk.ethereum.consensus.validators.std.ValidatorsExecutor
 
 /** Internal API, used for testing (especially mocks) */
 trait EthashBlockGenerator extends TestBlockGenerator {

src/main/scala/io/iohk/ethereum/consensus/ethash/blocks/RestrictedEthashBlockGeneratorImpl.scala

@@ -4,12 +4,12 @@ import io.iohk.ethereum.consensus.ConsensusConfig
 import io.iohk.ethereum.consensus.blocks.{BlockTimestampProvider, DefaultBlockTimestampProvider, PendingBlockAndState}
 import io.iohk.ethereum.consensus.difficulty.DifficultyCalculator
 import io.iohk.ethereum.consensus.ethash.RestrictedEthashSigner
-import io.iohk.ethereum.consensus.ethash.validators.ValidatorsExecutor
 import io.iohk.ethereum.domain.{Address, Block, Blockchain, SignedTransaction}
 import io.iohk.ethereum.ledger.{BlockPreparator, InMemoryWorldStateProxy}
 import io.iohk.ethereum.utils.BlockchainConfig
 import org.bouncycastle.crypto.AsymmetricCipherKeyPair
 import io.iohk.ethereum.consensus.ConsensusMetrics
+import io.iohk.ethereum.consensus.validators.std.ValidatorsExecutor
 
 class RestrictedEthashBlockGeneratorImpl(
     validators: ValidatorsExecutor,

src/main/scala/io/iohk/ethereum/consensus/ethash/validators/OmmersValidator.scala

@@ -2,6 +2,7 @@ package io.iohk.ethereum.consensus.ethash.validators
 
 import akka.util.ByteString
 import io.iohk.ethereum.consensus.ethash.validators.OmmersValidator.{OmmersError, OmmersValid}
+import io.iohk.ethereum.consensus.validators.BlockHeaderError
 import io.iohk.ethereum.consensus.{GetBlockHeaderByHash, GetNBlocksBack}
 import io.iohk.ethereum.domain.{Block, BlockHeader, Blockchain}
 
@@ -36,9 +37,10 @@ object OmmersValidator {
 
   object OmmersError {
     case object OmmersLengthError extends OmmersError
-    case object OmmersNotValidError extends OmmersError
+    case class OmmersHeaderError(errors: List[BlockHeaderError]) extends OmmersError
     case object OmmersUsedBeforeError extends OmmersError
-    case object OmmersAncestorsError extends OmmersError
+    case object OmmerIsAncestorError extends OmmersError
+    case object OmmerParentIsNotAncestorError extends OmmersError
     case object OmmersDuplicatedError extends OmmersError
   }