Skip to content

Commit

Permalink
[ETCM-709] Improve ommers validations
Browse files Browse the repository at this point in the history
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
Raised the log level of some logs in BlockFetcher because there are barely any logs this actor when debug logs are disabled
  • Loading branch information
Leonor Boga committed Apr 2, 2021
1 parent 6c88bad commit 31c4792
Show file tree
Hide file tree
Showing 30 changed files with 219 additions and 123 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -74,7 +75,7 @@ class BlockImporterItSpec
getBlockHeaderByHash: GetBlockHeaderByHash,
getNBlocksBack: GetNBlocksBack
) =>
new StdOmmersValidator(blockchainConfig, blockHeaderValidator)
new StdOmmersValidator(blockHeaderValidator)
.validate(parentHash, blockNumber, ommers, getBlockHeaderByHash, getNBlocksBack)
}

Expand Down
7 changes: 6 additions & 1 deletion src/it/scala/io/iohk/ethereum/sync/RegularSyncItSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -96,7 +102,8 @@ object RegularSyncItSpecUtils {
broadcasterRef,
pendingTransactionsManager,
regularSync
))
)
)

lazy val regularSync = system.actorOf(
RegularSync.props(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -113,9 +114,10 @@ class BlockFetcher(
}

fetchBlocks(newState)

case InvalidateBlocksFrom(blockNr, reason, withBlacklist) =>
val (blockProvider, newState) = state.invalidateBlocksFrom(blockNr, withBlacklist)
log.debug("Invalidate blocks from {}", blockNr)
log.info("Invalidate blocks from {}", blockNr)
blockProvider.foreach(peersClient ! BlacklistPeer(_, reason))
fetchBlocks(newState)
}
Expand All @@ -124,7 +126,7 @@ class BlockFetcher(
case Response(_, BlockHeaders(headers)) if state.isFetchingHeaders =>
val newState =
if (state.fetchingHeadersState == AwaitingHeadersToBeIgnored) {
log.debug(
log.info(
"Received {} headers starting from block {} that will be ignored",
headers.size,
headers.headOption.map(_.number)
Expand All @@ -149,17 +151,17 @@ class BlockFetcher(

fetchBlocks(newState)
case RetryHeadersRequest if state.isFetchingHeaders =>
log.debug("Something failed on a headers request, cancelling the request and re-fetching")
log.info("Something failed on a headers request, cancelling the request and re-fetching")

val newState = state.withHeaderFetchReceived
fetchBlocks(newState)
}

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")
log.info("Block bodies will be ignored due to an invalidation was requested for them")
fetchBlocks(state.withBodiesFetchReceived)
} else {
val newState =
Expand All @@ -171,19 +173,22 @@ 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")
log.info("Something failed on a bodies request, cancelling the request and re-fetching")
val newState = state.withBodiesFetchReceived
fetchBlocks(newState)
}

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 => {
Expand All @@ -196,7 +201,7 @@ class BlockFetcher(

validatedNode match {
case Left(err) =>
log.debug(err)
log.info(err)
peersClient ! BlacklistPeer(peer.id, err)
fetchStateNode(fetcher.hash, fetcher.replyTo, state)
case Right(node) =>
Expand All @@ -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))
Expand Down Expand Up @@ -247,7 +255,7 @@ class BlockFetcher(
}

private def handleFutureBlock(block: Block, state: BlockFetcherState): Unit = {
log.debug("Ignoring received block as it doesn't match local state or fetch side is not on top")
log.info("Ignoring received block as it doesn't match local state or fetch side is not on top")
val newState = state.withPossibleNewTopAt(block.number)
supervisor ! ProgressProtocol.GotNewBlock(newState.knownTop)
fetchBlocks(newState)
Expand All @@ -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()
Expand All @@ -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)
}
}
Expand All @@ -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.info("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.info("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.info("New checkpoint block {} imported from the inside", blockNr)

val newState = state
.clearQueues()
Expand Down Expand Up @@ -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.error("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)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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._

Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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._

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}

Expand Down Expand Up @@ -36,9 +37,10 @@ object OmmersValidator {

object OmmersError {
case object OmmersLengthError extends OmmersError
case object OmmersNotValidError extends OmmersError
case class OmmersHeaderError(error: Option[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
}

Expand Down
Loading

0 comments on commit 31c4792

Please sign in to comment.