[ETCM-709] Improve ommers validations.

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
   }