Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ETCM-177] Simplify ommers handling #745

Merged
merged 4 commits into from
Oct 23, 2020
Merged

[ETCM-177] Simplify ommers handling #745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c86cbd5
[ETCM-177] Remove manual removal for ommers pool
mirkoAlic Oct 19, 2020
7428266
[ETCM-177] remove Either for generateBlock
mirkoAlic Oct 19, 2020
3bcc571
[ETCM-177] Add ommers only at resolve branch level
mirkoAlic Oct 20, 2020
3178761
Merge branch 'develop' into etcm-177-improve-ommers-pool-handling
mirkoAlic Oct 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockImporter.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import io.iohk.ethereum.ledger._
import io.iohk.ethereum.mpt.MerklePatriciaTrie.MissingNodeException
import io.iohk.ethereum.network.PeerId
import io.iohk.ethereum.network.p2p.messages.CommonMessages.NewBlock
import io.iohk.ethereum.ommers.OmmersPool.{AddOmmers, RemoveOmmers}
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
Expand Down Expand Up @@ -221,17 +221,16 @@ class BlockImporter(
case BlockImportedToTop(importedBlocksData) =>
val (blocks, tds) = importedBlocksData.map(data => (data.block, data.td)).unzip
broadcastBlocks(blocks, tds)
updateTxAndOmmerPools(importedBlocksData.map(_.block), Seq.empty)
updateTxPool(importedBlocksData.map(_.block), Seq.empty)

case BlockEnqueued =>
ommersPool ! AddOmmers(block.header)
case BlockEnqueued => ()

case DuplicateBlock => ()

case UnknownParent => () // This is normal when receiving broadcast blocks

case ChainReorganised(oldBranch, newBranch, totalDifficulties) =>
updateTxAndOmmerPools(newBranch, oldBranch)
updateTxPool(newBranch, oldBranch)
broadcastBlocks(newBranch, totalDifficulties)

case BlockImportFailed(error) =>
Expand All @@ -256,12 +255,9 @@ class BlockImporter(

private def broadcastNewBlocks(blocks: List[NewBlock]): Unit = broadcaster ! BroadcastBlocks(blocks)

private def updateTxAndOmmerPools(blocksAdded: Seq[Block], blocksRemoved: Seq[Block]): Unit = {
blocksRemoved.headOption.foreach(block => ommersPool ! AddOmmers(block.header))
private def updateTxPool(blocksAdded: Seq[Block], blocksRemoved: Seq[Block]): Unit = {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like much the middle ground of keeping the pool and ignoring any problems when using it on the miner, but only updating the pool it half of the times, should we:

  1. Remove the pool entirely and have no support with mining blocks with ommers
  2. Keep the pool updates as on develop and use the patch for producing blocks without ommers for handling the case that it has any invalid one

blocksRemoved.foreach(block => pendingTransactionsManager ! AddUncheckedTransactions(block.body.transactionList))

blocksAdded.foreach { block =>
ommersPool ! RemoveOmmers(block.header :: block.body.uncleNodesList.toList)
pendingTransactionsManager ! RemoveTransactions(block.body.transactionList)
}
}
Expand Down
3 changes: 1 addition & 2 deletions src/main/scala/io/iohk/ethereum/consensus/blocks/BlockGenerator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package io.iohk.ethereum.consensus.blocks

import io.iohk.ethereum.domain.{Address, Block, SignedTransaction}
import io.iohk.ethereum.ledger.BlockPreparationError

/**
* We use a `BlockGenerator` to create the next block.
Expand Down Expand Up @@ -41,7 +40,7 @@ trait BlockGenerator {
transactions: Seq[SignedTransaction],
beneficiary: Address,
x: X
): Either[BlockPreparationError, PendingBlock]
): PendingBlock
}

/**
Expand Down
6 changes: 3 additions & 3 deletions src/main/scala/io/iohk/ethereum/consensus/blocks/NoOmmersBlockGenerator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package io.iohk.ethereum.consensus.blocks
import io.iohk.ethereum.consensus.ConsensusConfig
import io.iohk.ethereum.consensus.difficulty.DifficultyCalculator
import io.iohk.ethereum.domain._
import io.iohk.ethereum.ledger.{BlockPreparationError, BlockPreparator}
import io.iohk.ethereum.ledger.BlockPreparator
import io.iohk.ethereum.utils.BlockchainConfig

abstract class NoOmmersBlockGenerator(
Expand Down Expand Up @@ -43,14 +43,14 @@ abstract class NoOmmersBlockGenerator(
transactions: Seq[SignedTransaction],
beneficiary: Address,
x: Nil.type
): Either[BlockPreparationError, PendingBlock] = {
): PendingBlock = {

val pHeader = parent.header
val blockNumber = pHeader.number + 1

val prepared = prepareBlock(parent, transactions, beneficiary, blockNumber, blockPreparator, x)
cache.updateAndGet((t: List[PendingBlockAndState]) => (prepared :: t).take(blockCacheSize))

Right(prepared.pendingBlock)
prepared.pendingBlock
}
}
8 changes: 3 additions & 5 deletions src/main/scala/io/iohk/ethereum/consensus/ethash/EthashBlockCreator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,9 @@ class EthashBlockCreator(
val transactions =
if (withTransactions) getTransactionsFromPool else Future.successful(PendingTransactionsResponse(Nil))
getOmmersFromPool(parentBlock.hash).zip(transactions).flatMap { case (ommers, pendingTxs) =>
blockGenerator
.generateBlock(parentBlock, pendingTxs.pendingTransactions.map(_.stx.tx), coinbase, ommers.headers) match {
case Right(pb) => Future.successful(pb)
case Left(err) => Future.failed(new RuntimeException(s"Error while generating block for mining: $err"))
}
val pendingBlock = blockGenerator
.generateBlock(parentBlock, pendingTxs.pendingTransactions.map(_.stx.tx), coinbase, ommers.headers)
Future.successful(pendingBlock)
}
}

Expand Down
24 changes: 11 additions & 13 deletions src/main/scala/io/iohk/ethereum/consensus/ethash/blocks/EthashBlockGenerator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ 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.{BlockPreparationError, BlockPreparator}
import io.iohk.ethereum.ledger.BlockPreparator
import io.iohk.ethereum.utils.BlockchainConfig

/** Internal API, used for testing (especially mocks) */
Expand Down Expand Up @@ -73,25 +73,23 @@ class EthashBlockGeneratorImpl(
transactions: Seq[SignedTransaction],
beneficiary: Address,
x: Ommers
): Either[BlockPreparationError, PendingBlock] = {
): PendingBlock = {
val pHeader = parent.header
val blockNumber = pHeader.number + 1
val parentHash = pHeader.hash

val ommersV = validators.ommersValidator
val ommers = validators.ommersValidator.validate(parentHash, blockNumber, x, blockchain) match {
case Left(_) => emptyX
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we log this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm...now i'm thinking we could log this at ommers pool level once we finally do all the validations. Btw, the amount of ommers per block is something that we measure, so we couyld produce metrics, i mean, it won't be an issue to get track of that.

case Right(_) => x
}

val result: Either[InvalidOmmers, PendingBlockAndState] = ommersV
.validate(parentHash, blockNumber, x, blockchain)
.left
.map(InvalidOmmers)
.flatMap { _ =>
val prepared = prepareBlock(parent, transactions, beneficiary, blockNumber, blockPreparator, x)
Right(prepared)
}
val prepared = prepareBlock(parent, transactions, beneficiary, blockNumber, blockPreparator, ommers)

result.right.foreach(b => cache.updateAndGet((t: List[PendingBlockAndState]) => (b :: t).take(blockCacheSize)))
cache.updateAndGet { t: List[PendingBlockAndState] =>
(prepared :: t).take(blockCacheSize)
}

result.map(_.pendingBlock)
prepared.pendingBlock
}

def withBlockTimestampProvider(blockTimestampProvider: BlockTimestampProvider): EthashBlockGeneratorImpl =
Expand Down
4 changes: 0 additions & 4 deletions src/main/scala/io/iohk/ethereum/consensus/ethash/blocks/package.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
package io.iohk.ethereum.consensus.ethash

import io.iohk.ethereum.consensus.ethash.validators.OmmersValidator.OmmersError
import io.iohk.ethereum.domain.BlockHeader
import io.iohk.ethereum.domain.BlockHeaderImplicits._
import io.iohk.ethereum.ledger.BlockPreparationError
import io.iohk.ethereum.rlp.{RLPEncodeable, RLPList, RLPSerializable}

package object blocks {
Expand All @@ -19,6 +17,4 @@ package object blocks {
implicit class OmmersSeqEnc(blockHeaders: Seq[BlockHeader]) extends RLPSerializable {
override def toRLPEncodable: RLPEncodeable = RLPList(blockHeaders.map(_.toRLPEncodable): _*)
}

final case class InvalidOmmers(reason: OmmersError) extends BlockPreparationError
}
35 changes: 16 additions & 19 deletions src/main/scala/io/iohk/ethereum/jsonrpc/EthService.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,27 +539,24 @@ class EthService(
import io.iohk.ethereum.consensus.ethash.EthashUtils.{epoch, seed}

val bestBlock = blockchain.getBestBlock()
getOmmersFromPool(bestBlock.hash).zip(getTransactionsFromPool).map { case (ommers, pendingTxs) =>
val blockGenerator = ethash.blockGenerator
blockGenerator.generateBlock(
bestBlock,
pendingTxs.pendingTransactions.map(_.stx.tx),
consensusConfig.coinbase,
ommers.headers
) match {
case Right(pb) =>
Right(
GetWorkResponse(
powHeaderHash = ByteString(kec256(BlockHeader.getEncodedWithoutNonce(pb.block.header))),
dagSeed = seed(epoch(pb.block.header.number.toLong)),
target = ByteString((BigInt(2).pow(256) / pb.block.header.difficulty).toByteArray)
)
val response: ServiceResponse[GetWorkResponse] =
getOmmersFromPool(bestBlock.hash).zip(getTransactionsFromPool).map { case (ommers, pendingTxs) =>
val blockGenerator = ethash.blockGenerator
val pb = blockGenerator.generateBlock(
bestBlock,
pendingTxs.pendingTransactions.map(_.stx.tx),
consensusConfig.coinbase,
ommers.headers
)
Right(
GetWorkResponse(
powHeaderHash = ByteString(kec256(BlockHeader.getEncodedWithoutNonce(pb.block.header))),
dagSeed = seed(epoch(pb.block.header.number.toLong)),
target = ByteString((BigInt(2).pow(256) / pb.block.header.difficulty).toByteArray)
)
case Left(err) =>
log.error(s"unable to prepare block because of $err")
Left(JsonRpcErrors.InternalError)
)
}
}
response
})(Future.successful(Left(JsonRpcErrors.ConsensusIsNotEthash)))

private def getOmmersFromPool(parentBlockHash: ByteString): Future[OmmersPool.Ommers] =
Expand Down
8 changes: 3 additions & 5 deletions src/main/scala/io/iohk/ethereum/jsonrpc/TestService.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,15 +150,13 @@ class TestService(
.mapTo[PendingTransactionsResponse]
.recover { case _ => PendingTransactionsResponse(Nil) }
.flatMap { pendingTxs =>
consensus.blockGenerator.generateBlock(
val pb = consensus.blockGenerator.generateBlock(
parentBlock,
pendingTxs.pendingTransactions.map(_.stx.tx),
etherbase,
Nil
) match {
case Right(pb) => Future.successful(pb)
case Left(err) => Future.failed(new RuntimeException(s"Error while generating block for mining: $err"))
}
)
Future.successful(pb)
}
}
}
13 changes: 1 addition & 12 deletions src/main/scala/io/iohk/ethereum/ommers/OmmersPool.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import akka.util.ByteString
import akka.actor.{Actor, ActorLogging, Props}
import org.bouncycastle.util.encoders.Hex
import io.iohk.ethereum.domain.{BlockHeader, Blockchain}
import io.iohk.ethereum.ommers.OmmersPool.{AddOmmers, GetOmmers, RemoveOmmers}
import io.iohk.ethereum.ommers.OmmersPool.{AddOmmers, GetOmmers}
import scala.annotation.tailrec

class OmmersPool(blockchain: Blockchain, ommersPoolSize: Int, ommerGenerationLimit: Int, returnedOmmersSizeLimit: Int)
Expand All @@ -18,11 +18,6 @@ class OmmersPool(blockchain: Blockchain, ommersPoolSize: Int, ommerGenerationLim
ommersPool = (ommers ++ ommersPool).take(ommersPoolSize).distinct
logStatus(event = "Ommers after add", ommers = ommersPool)

case RemoveOmmers(ommers) =>
val toDelete = ommers.map(_.hash).toSet
ommersPool = ommersPool.filter(b => !toDelete.contains(b.hash))
logStatus(event = "Ommers after remove", ommers = ommersPool)

case GetOmmers(parentBlockHash) =>
val ancestors = collectAncestors(parentBlockHash, ommerGenerationLimit)
val ommers = ommersPool
Expand Down Expand Up @@ -81,12 +76,6 @@ object OmmersPool {
def apply(b: BlockHeader*): AddOmmers = AddOmmers(b.toList)
}

case class RemoveOmmers(ommers: List[BlockHeader])

object RemoveOmmers {
def apply(b: BlockHeader*): RemoveOmmers = RemoveOmmers(b.toList)
}

case class GetOmmers(parentBlockHash: ByteString)

case class Ommers(headers: Seq[BlockHeader])
Expand Down
16 changes: 0 additions & 16 deletions src/test/scala/io/iohk/ethereum/blockchain/sync/regular/RegularSyncSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import io.iohk.ethereum.domain.BlockHeaderImplicits._
import io.iohk.ethereum.network.p2p.messages.PV62._
import io.iohk.ethereum.network.p2p.messages.PV63.{GetNodeData, NodeData}
import io.iohk.ethereum.network.{EtcPeerManagerActor, Peer, PeerEventBusActor}
import io.iohk.ethereum.ommers.OmmersPool.RemoveOmmers
import io.iohk.ethereum.utils.Config.SyncConfig
import org.scalamock.scalatest.MockFactory
import org.scalatest.BeforeAndAfterEach
Expand Down Expand Up @@ -406,13 +405,6 @@ class RegularSyncSpec
case _ => false
}
}
"update ommers for imported block" in new OnTopFixture(testSystem) {
goToTop()

sendNewBlock()

ommersPool.expectMsg(RemoveOmmers(newBlock.header :: newBlock.body.uncleNodesList.toList))
}
"fetch hashes if received NewHashes message" in new OnTopFixture(testSystem) {
goToTop()

Expand Down Expand Up @@ -480,14 +472,6 @@ class RegularSyncSpec
case _ => false
}
}

"update ommers after successful import" in new OnTopFixture(testSystem) {
goToTop()

regularSync ! RegularSync.MinedBlock(newBlock)

ommersPool.expectMsg(RemoveOmmers(newBlock.header :: newBlock.body.uncleNodesList.toList))
}
}

"handling checkpoints" should {
Expand Down
Loading