Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 5 additions & 3 deletions src/it/scala/io/iohk/ethereum/sync/RegularSyncItSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,9 @@ class RegularSyncItSpec extends FreeSpecBase with Matchers with BeforeAndAfterAl
_ <- peer2.importBlocksUntil(30)(IdentityUpdate)
_ <- peer1.startRegularSync()
_ <- peer2.startRegularSync()
_ <- peer2.addCheckpointedBlock(peer2.blockchainReader.getBestBranch().getBlockByNumber(25).get)
_ <- peer2.addCheckpointedBlock(
peer2.blockchainReader.getBlockByNumber(peer2.blockchainReader.getBestBranch(), 25).get
)
_ <- peer2.waitForRegularSyncLoadLastBlock(length)
_ <- peer1.connectToPeers(Set(peer2.node))
_ <- peer1.waitForRegularSyncLoadLastBlock(length)
Expand Down Expand Up @@ -181,8 +183,8 @@ class RegularSyncItSpec extends FreeSpecBase with Matchers with BeforeAndAfterAl
)
)
(
peer1.blockchainReader.getBestBranch().getBlockByNumber(blockNumer + 1),
peer2.blockchainReader.getBestBranch().getBlockByNumber(blockNumer + 1)
peer1.blockchainReader.getBlockByNumber(peer1.blockchainReader.getBestBranch(), blockNumer + 1),
peer2.blockchainReader.getBlockByNumber(peer2.blockchainReader.getBestBranch(), blockNumer + 1)
) match {
case (Some(blockP1), Some(blockP2)) =>
assert(peer1.bl.getChainWeightByHash(blockP1.hash) == peer2.bl.getChainWeightByHash(blockP2.hash))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,7 @@ object RegularSyncItSpecUtils {
Task(blockNumber match {
case Some(bNumber) =>
blockchainReader
.getBestBranch()
.getBlockByNumber(bNumber)
.getBlockByNumber(blockchainReader.getBestBranch(), bNumber)
.getOrElse(throw new RuntimeException(s"block by number: $bNumber doesn't exist"))
case None => blockchainReader.getBestBlock().get
}).flatMap { block =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import io.iohk.ethereum.db.storage.pruning.PruningMode
import io.iohk.ethereum.domain.BlockHeader.HeaderExtraFields.HefEmpty
import io.iohk.ethereum.domain.Blockchain
import io.iohk.ethereum.domain._
import io.iohk.ethereum.domain.branch.Branch
import io.iohk.ethereum.jsonrpc.ProofService.EmptyStorageValueProof
import io.iohk.ethereum.jsonrpc.ProofService.StorageProof
import io.iohk.ethereum.jsonrpc.ProofService.StorageProofKey
Expand Down Expand Up @@ -102,9 +101,7 @@ object DumpChainApp

val blockchain: Blockchain = new BlockchainMock(genesisHash)
val blockchainReader: BlockchainReader = mock[BlockchainReader]
val bestChain: Branch = mock[Branch]
(blockchainReader.getBestBranch _).expects().anyNumberOfTimes().returning(bestChain)
(bestChain.getHashByBlockNumber _).expects(*).returning(Some(genesisHash))
(blockchainReader.getHashByBlockNumber _).expects(*, *).returning(Some(genesisHash))

val nodeStatus: NodeStatus =
NodeStatus(key = nodeKey, serverStatus = ServerStatus.NotListening, discoveryStatus = ServerStatus.NotListening)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ trait OmmersValidator {
val getNBlocksBack: (ByteString, Int) => List[Block] =
(_, n) =>
((blockNumber - n) until blockNumber).toList
.flatMap(nb => bestBranch.getBlockByNumber(nb))
.flatMap(nb => blockchainReader.getBlockByNumber(bestBranch, nb))

validate(parentHash, blockNumber, ommers, getBlockHeaderByHash, getNBlocksBack)
}
Expand Down
4 changes: 2 additions & 2 deletions src/main/scala/io/iohk/ethereum/domain/Blockchain.scala
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ class BlockchainImpl(
val latestCheckpointNumber = getLatestCheckpointBlockNumber()

val blockNumberMappingUpdates =
if (blockchainReader.getBestBranch().getHashByBlockNumber(block.number).contains(blockHash))
if (blockchainReader.getHashByBlockNumber(blockchainReader.getBestBranch(), block.number).contains(blockHash))
removeBlockNumberMapping(block.number)
else blockNumberMappingStorage.emptyBatchUpdate

Expand Down Expand Up @@ -280,7 +280,7 @@ class BlockchainImpl(
): BigInt =
if (blockNumberToCheck > 0) {
val maybePreviousCheckpointBlockNumber = for {
currentBlock <- blockchainReader.getBestBranch().getBlockByNumber(blockNumberToCheck)
currentBlock <- blockchainReader.getBlockByNumber(blockchainReader.getBestBranch(), blockNumberToCheck)
if currentBlock.hasCheckpoint &&
currentBlock.number < latestCheckpointBlockNumber
} yield currentBlock.number
Expand Down
50 changes: 39 additions & 11 deletions src/main/scala/io/iohk/ethereum/domain/BlockchainReader.scala
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import io.iohk.ethereum.db.storage.ReceiptStorage
import io.iohk.ethereum.db.storage.StateStorage
import io.iohk.ethereum.domain.branch.BestBranch
import io.iohk.ethereum.domain.branch.Branch
import io.iohk.ethereum.domain.branch.EmptyBranch$
import io.iohk.ethereum.domain.branch.EmptyBranch
import io.iohk.ethereum.mpt.MptNode
import io.iohk.ethereum.utils.Logger

Expand Down Expand Up @@ -71,16 +71,14 @@ class BlockchainReader(
def getReceiptsByHash(blockhash: ByteString): Option[Seq[Receipt]] = receiptStorage.get(blockhash)

/** get the current best stored branch */
def getBestBranch(): Branch =
getBestBlock()
.map { block =>
new BestBranch(
block.header,
blockNumberMappingStorage,
this
)
}
.getOrElse(EmptyBranch$)
def getBestBranch(): Branch = {
val number = getBestBlockNumber()
blockNumberMappingStorage
.get(number)
.orElse(blockNumberMappingStorage.get(appStateStorage.getBestBlockNumber()))
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this for? This is a work-around not related to this change, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is not related to this change.
It's related to anastasiia's comment. It allows to be consistent with getBestBlock which fallbacks to what is in the storage if the block from memory does not exists.

.map(hash => BestBranch(hash, number))
.getOrElse(EmptyBranch)
}

def getBestBlockNumber(): BigInt = {
val bestSavedBlockNumber = appStateStorage.getBestBlockNumber()
Expand Down Expand Up @@ -113,6 +111,36 @@ class BlockchainReader(
def genesisBlock: Block =
getBlockByNumber(0).get

/** Returns a block inside this branch based on its number */
def getBlockByNumber(branch: Branch, number: BigInt): Option[Block] = branch match {
case BestBranch(_, tipBlockNumber) if tipBlockNumber >= number && number >= 0 =>
for {
hash <- getHashByBlockNumber(number)
block <- getBlockByHash(hash)
} yield block
case EmptyBranch | BestBranch(_, _) => None
}

/** Returns a block hash for the block at the given height if any */
def getHashByBlockNumber(branch: Branch, number: BigInt): Option[ByteString] = branch match {
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think of using a type alias for ByteString when we are talking about a hash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer a value class or a tagged type, because the type alias would just be "cosmetic". But it would probably be out of scope to introduce the new type in this PR.

case BestBranch(_, tipBlockNumber) =>
if (tipBlockNumber >= number && number >= 0) {
blockNumberMappingStorage.get(number)
} else None

case EmptyBranch => None
}

/** Checks if given block hash is in this chain. (i.e. is an ancestor of the tip block) */
def isInChain(branch: Branch, hash: ByteString): Boolean = branch match {
case BestBranch(_, tipBlockNumber) =>
(for {
header <- getBlockHeaderByHash(hash) if header.number <= tipBlockNumber
hashFromBestChain <- getHashByBlockNumber(branch, header.number)
} yield header.hash == hashFromBestChain).getOrElse(false)
case EmptyBranch => false
}

/** Allows to query for a block based on it's number
*
* @param number Block number
Expand Down
41 changes: 0 additions & 41 deletions src/main/scala/io/iohk/ethereum/domain/branch/BestBranch.scala

This file was deleted.

15 changes: 3 additions & 12 deletions src/main/scala/io/iohk/ethereum/domain/branch/Branch.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,8 @@ package io.iohk.ethereum.domain.branch

import akka.util.ByteString

import io.iohk.ethereum.domain.Block
sealed trait Branch

/** An interface to manipulate blockchain branches */
trait Branch {
case class BestBranch(tipBlockHash: ByteString, tipBlockNumber: BigInt) extends Branch
Copy link
Contributor

Choose a reason for hiding this comment

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

I can imagine this case class being used for all the branches, not only the best one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not contain data specific to the best branch, yes. Right now it is more a marker type to tell the code that we can use the block number -> hash index.


/** Returns a block inside this branch based on its number */
def getBlockByNumber(number: BigInt): Option[Block]

/** Returns a block hash for the block at the given height if any */
def getHashByBlockNumber(number: BigInt): Option[ByteString]

/** Checks if given block hash is in this chain. (i.e. is an ancestor of the tip block) */
def isInChain(hash: ByteString): Boolean
}
case object EmptyBranch extends Branch
Copy link
Contributor

Choose a reason for hiding this comment

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

So we'll have multiple types of branch later on, right? For the purposes of the code you're changing now (branch as produced by getBestBranch) you could also name the trait BestBranch and rename empty branch to Genesis, which resolves to a block number of 1. (Or is it 0?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we will have several types yes (well, best branch and "other" branch). I don't really know what to do with the EmptyBranch as I return it when even the genesis is not found (normally that can happen only during tests). I wonder if we should not add the genesis as a parameter to the block storage, to ensure that it is always there.

13 changes: 0 additions & 13 deletions src/main/scala/io/iohk/ethereum/domain/branch/EmptyBranch$.scala

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,13 @@ import io.iohk.ethereum.blockchain.sync.regular.RegularSync.NewCheckpoint
import io.iohk.ethereum.consensus.blocks.CheckpointBlockGenerator
import io.iohk.ethereum.crypto.ECDSASignature
import io.iohk.ethereum.domain.Block
import io.iohk.ethereum.domain.Blockchain
import io.iohk.ethereum.domain.BlockchainReader
import io.iohk.ethereum.domain.Checkpoint
import io.iohk.ethereum.ledger.BlockQueue
import io.iohk.ethereum.utils.ByteStringUtils
import io.iohk.ethereum.utils.Logger

class CheckpointingService(
blockchain: Blockchain,
blockchainReader: BlockchainReader,
blockQueue: BlockQueue,
checkpointBlockGenerator: CheckpointBlockGenerator,
Expand All @@ -36,7 +34,7 @@ class CheckpointingService(
req.parentCheckpoint.forall(blockchainReader.getBlockHeaderByHash(_).exists(_.number < blockToReturnNum))

Task {
blockchainReader.getBestBranch().getBlockByNumber(blockToReturnNum)
blockchainReader.getBlockByNumber(blockchainReader.getBestBranch(), blockToReturnNum)
}.flatMap {
case Some(b) if isValidParent =>
Task.now(Right(GetLatestBlockResponse(Some(BlockInfo(b.hash, b.number)))))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,7 @@ class EthProofService(
private def resolveBlock(blockParam: BlockParam): Either[JsonRpcError, ResolvedBlock] = {
def getBlock(number: BigInt): Either[JsonRpcError, Block] =
blockchainReader
.getBestBranch()
.getBlockByNumber(number)
.getBlockByNumber(blockchainReader.getBestBranch(), number)
.toRight(JsonRpcError.InvalidParams(s"Block $number not found"))

def getLatestBlock(): Either[JsonRpcError, Block] =
Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/io/iohk/ethereum/jsonrpc/EthTxService.scala
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ class EthTxService(
Task {
val bestBranch = blockchainReader.getBestBranch()
val gasPrice = ((bestBlock - blockDifference) to bestBlock)
.flatMap(nb => bestBranch.getBlockByNumber(nb))
.flatMap(nb => blockchainReader.getBlockByNumber(bestBranch, nb))
.flatMap(_.body.transactionList)
.map(_.tx.gasPrice)
if (gasPrice.nonEmpty) {
Expand Down
3 changes: 1 addition & 2 deletions src/main/scala/io/iohk/ethereum/jsonrpc/ResolveBlock.scala
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ trait ResolveBlock {

private def getBlock(number: BigInt): Either[JsonRpcError, Block] =
blockchainReader
.getBestBranch()
.getBlockByNumber(number)
.getBlockByNumber(blockchainReader.getBestBranch(), number)
.toRight(JsonRpcError.InvalidParams(s"Block $number not found"))

private def getLatestBlock(): Either[JsonRpcError, Block] =
Expand Down
4 changes: 2 additions & 2 deletions src/main/scala/io/iohk/ethereum/jsonrpc/TestService.scala
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ class TestService(

val blockOpt = request.parameters.blockHashOrNumber
.fold(
number => blockchainReader.getBestBranch().getBlockByNumber(number),
number => blockchainReader.getBlockByNumber(blockchainReader.getBestBranch(), number),
blockHash => blockchainReader.getBlockByHash(blockHash)
)

Expand Down Expand Up @@ -408,7 +408,7 @@ class TestService(

val blockOpt = request.parameters.blockHashOrNumber
.fold(
number => blockchainReader.getBestBranch().getBlockByNumber(number),
number => blockchainReader.getBlockByNumber(blockchainReader.getBestBranch(), number),
hash => blockchainReader.getBlockByHash(hash)
)

Expand Down
4 changes: 2 additions & 2 deletions src/main/scala/io/iohk/ethereum/ledger/BlockImport.scala
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ class BlockImport(
private def isPossibleNewBestBlock(newBlock: BlockHeader, currentBestBlock: BlockHeader): Boolean =
newBlock.parentHash == currentBestBlock.hash && newBlock.number == currentBestBlock.number + 1

private def measureBlockMetrics(importResult: BlockImportResult)(implicit blockchainConfig: BlockchainConfig): Unit =
private def measureBlockMetrics(importResult: BlockImportResult): Unit =
importResult match {
case BlockImportedToTop(blockImportData) =>
blockImportData.foreach(blockData => BlockMetrics.measure(blockData.block, blockchainReader.getBlockByHash))
Expand Down Expand Up @@ -318,7 +318,7 @@ class BlockImport(
private def removeBlocksUntil(parent: ByteString, fromNumber: BigInt): List[BlockData] = {
@tailrec
def removeBlocksUntil(parent: ByteString, fromNumber: BigInt, acc: List[BlockData]): List[BlockData] =
blockchainReader.getBestBranch().getBlockByNumber(fromNumber) match {
blockchainReader.getBlockByNumber(blockchainReader.getBestBranch(), fromNumber) match {
case Some(block) if block.header.hash == parent || fromNumber == 0 =>
acc

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class BlockValidation(
val numbers = (block.header.number - remaining) until block.header.number
val bestBranch = blockchainReader.getBestBranch()
val blocks =
(numbers.toList.flatMap(nb => bestBranch.getBlockByNumber(nb)) :+ block) ::: queuedBlocks
(numbers.toList.flatMap(nb => blockchainReader.getBlockByNumber(bestBranch, nb)) :+ block) ::: queuedBlocks
blocks
}
}
Expand Down
8 changes: 5 additions & 3 deletions src/main/scala/io/iohk/ethereum/ledger/BranchResolution.scala
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ class BranchResolution(blockchain: Blockchain, blockchainReader: BlockchainReade
InvalidBranch
} else {
val knownParentOrGenesis = blockchainReader
.getBestBranch()
.isInChain(headers.head.parentHash) || headers.head.hash == blockchainReader.genesisHeader.hash
.isInChain(
blockchainReader.getBestBranch(),
headers.head.parentHash
) || headers.head.hash == blockchainReader.genesisHeader.hash

if (!knownParentOrGenesis)
UnknownBranch
Expand Down Expand Up @@ -76,7 +78,7 @@ class BranchResolution(blockchain: Blockchain, blockchainReader: BlockchainReade
private def getTopBlocksFromNumber(from: BigInt): List[Block] = {
val bestBranch = blockchainReader.getBestBranch()
(from to blockchainReader.getBestBlockNumber())
.flatMap(nb => bestBranch.getBlockByNumber(nb))
.flatMap(nb => blockchainReader.getBlockByNumber(bestBranch, nb))
.toList
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,6 @@ trait CheckpointingServiceBuilder {

lazy val checkpointingService =
new CheckpointingService(
blockchain,
blockchainReader,
blockQueue,
checkpointBlockGenerator,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ class TestEthBlockServiceWrapper(
.map(
_.map { blockByBlockResponse =>
val bestBranch = blockchainReader.getBestBranch()
val fullBlock = bestBranch.getBlockByNumber(blockByBlockResponse.blockResponse.get.number).get
val fullBlock = blockchainReader.getBlockByNumber(bestBranch, blockByBlockResponse.blockResponse.get.number).get
BlockByNumberResponse(blockByBlockResponse.blockResponse.map(response => toEthResponse(fullBlock, response)))
}
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ class TransactionHistoryService(
val getLastCheckpoint = Task(blockchain.getLatestCheckpointBlockNumber()).memoizeOnSuccess
val txnsFromBlocks = Observable
.from(fromBlocks.reverse)
.mapParallelOrdered(10)(blockNr => Task(blockchainReader.getBestBranch().getBlockByNumber(blockNr)))(
.mapParallelOrdered(10)(blockNr =>
Task(blockchainReader.getBlockByNumber(blockchainReader.getBestBranch(), blockNr))
)(
OverflowStrategy.Unbounded
)
.collect { case Some(block) => block }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ class StdOmmersValidatorSpec extends AnyFlatSpec with Matchers with ScalaCheckPr
val getNBlocksBack: (ByteString, Int) => List[Block] =
(_, n) =>
((ommersBlockNumber - n) until ommersBlockNumber).toList
.flatMap(nb => blockchainReader.getBestBranch().getBlockByNumber(nb))
.flatMap(nb => blockchainReader.getBlockByNumber(blockchainReader.getBestBranch(), nb))

ommersValidator.validateOmmersAncestors(
ommersBlockParentHash,
Expand Down
Loading