fix unit tests

Author: Aurélien Richez

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

@@ -124,7 +124,7 @@ class RegularSyncItSpec extends FreeSpecBase with Matchers with BeforeAndAfterAl
       _ <- peer2.importBlocksUntil(30)(IdentityUpdate)
       _ <- peer1.startRegularSync()
       _ <- peer2.startRegularSync()
-      _ <- peer2.addCheckpointedBlock(peer2.blockchainReader.getBlockByNumber(25).get)
+      _ <- peer2.addCheckpointedBlock(peer2.blockchainReader.getBestBranch().get.getBlockByNumber(25).get)
       _ <- peer2.waitForRegularSyncLoadLastBlock(length)
       _ <- peer1.connectToPeers(Set(peer2.node))
       _ <- peer1.waitForRegularSyncLoadLastBlock(length)
@@ -181,8 +181,8 @@ class RegularSyncItSpec extends FreeSpecBase with Matchers with BeforeAndAfterAl
         )
       )
       (
-        peer1.blockchainReader.getBlockByNumber(blockNumer + 1),
-        peer2.blockchainReader.getBlockByNumber(blockNumer + 1)
+        peer1.blockchainReader.getBestBranch().get.getBlockByNumber(blockNumer + 1),
+        peer2.blockchainReader.getBestBranch().get.getBlockByNumber(blockNumer + 1)
       ) match {
         case (Some(blockP1), Some(blockP2)) =>
           assert(peer1.bl.getChainWeightByHash(blockP1.hash) == peer2.bl.getChainWeightByHash(blockP2.hash))

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

@@ -175,6 +175,8 @@ object RegularSyncItSpecUtils {
       Task(blockNumber match {
         case Some(bNumber) =>
           blockchainReader
+            .getBestBranch()
+            .get
             .getBlockByNumber(bNumber)
             .getOrElse(throw new RuntimeException(s"block by number: $bNumber doesn't exist"))
         case None => blockchainReader.getBestBlock().get

src/it/scala/io/iohk/ethereum/txExecTest/util/DumpChainApp.scala

@@ -28,6 +28,7 @@ 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.BlockchainBranch
 import io.iohk.ethereum.jsonrpc.ProofService.EmptyStorageValueProof
 import io.iohk.ethereum.jsonrpc.ProofService.StorageProof
 import io.iohk.ethereum.jsonrpc.ProofService.StorageProofKey
@@ -103,7 +104,9 @@ object DumpChainApp
 
   val blockchain: Blockchain = new BlockchainMock(genesisHash)
   val blockchainReader: BlockchainReader = mock[BlockchainReader]
-  (blockchainReader.getHashByBlockNumber _).expects(*).returning(Some(genesisHash))
+  val bestChain: BlockchainBranch = mock[BlockchainBranch]
+  (blockchainReader.getBestBranch _).expects().anyNumberOfTimes().returning(Some(bestChain))
+  (bestChain.getHashByBlockNumber _).expects(*).returning(Some(genesisHash))
 
   val nodeStatus: NodeStatus =
     NodeStatus(key = nodeKey, serverStatus = ServerStatus.NotListening, discoveryStatus = ServerStatus.NotListening)

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

@@ -29,8 +29,11 @@ trait OmmersValidator {
   ): Either[OmmersError, OmmersValid] = {
 
     val getBlockHeaderByHash: ByteString => Option[BlockHeader] = blockchainReader.getBlockHeaderByHash
+    val bestBranch = blockchainReader.getBestBranch()
     val getNBlocksBack: (ByteString, Int) => List[Block] =
-      (_, n) => ((blockNumber - n) until blockNumber).toList.flatMap(blockchainReader.getBestBranch.getBlockByNumber)
+      (_, n) =>
+        ((blockNumber - n) until blockNumber).toList
+          .flatMap(nb => bestBranch.flatMap(_.getBlockByNumber(nb)))
 
     validate(parentHash, blockNumber, ommers, getBlockHeaderByHash, getNBlocksBack)
   }

src/main/scala/io/iohk/ethereum/domain/Blockchain.scala

@@ -139,7 +139,8 @@ class BlockchainImpl(
   override def isInChain(hash: ByteString): Boolean =
     (for {
       header <- blockchainReader.getBlockHeaderByHash(hash) if header.number <= blockchainReader.getBestBlockNumber()
-      hash <- blockchainReader.getBestBranch().getHashByBlockNumber(header.number)
+      bestBranch <- blockchainReader.getBestBranch()
+      hash <- bestBranch.getHashByBlockNumber(header.number)
     } yield header.hash == hash).getOrElse(false)
 
   override def getChainWeightByHash(blockhash: ByteString): Option[ChainWeight] = chainWeightStorage.get(blockhash)
@@ -309,7 +310,7 @@ class BlockchainImpl(
     val latestCheckpointNumber = getLatestCheckpointBlockNumber()
 
     val blockNumberMappingUpdates =
-      if (blockchainReader.getBestBranch().getHashByBlockNumber(block.number).contains(blockHash))
+      if (blockchainReader.getBestBranch().flatMap(_.getHashByBlockNumber(block.number)).contains(blockHash))
         removeBlockNumberMapping(block.number)
       else blockNumberMappingStorage.emptyBatchUpdate
 
@@ -378,7 +379,7 @@ class BlockchainImpl(
   ): BigInt =
     if (blockNumberToCheck > 0) {
       val maybePreviousCheckpointBlockNumber = for {
-        currentBlock <- blockchainReader.getBestBranch.getBlockByNumber(blockNumberToCheck)
+        currentBlock <- blockchainReader.getBestBranch().flatMap(_.getBlockByNumber(blockNumberToCheck))
         if currentBlock.hasCheckpoint &&
           currentBlock.number < latestCheckpointBlockNumber
       } yield currentBlock.number

src/main/scala/io/iohk/ethereum/domain/BlockchainReader.scala

@@ -69,12 +69,17 @@ class BlockchainReader(
     */
   def getReceiptsByHash(blockhash: ByteString): Option[Seq[Receipt]] = receiptStorage.get(blockhash)
 
-  def getBestBranch(): BlockchainBranch =
-    new BestBlockchainBranch(
-      getBestBlock().map(_.header).getOrElse(genesisHeader),
-      blockNumberMappingStorage,
-      this
-    )
+  /** get the current best stored branch */
+  // FIXME this should not be an option as we should always have the genesis
+  // but some tests prevent it to simply be BlockchainBranch for now
+  def getBestBranch(): Option[BlockchainBranch] =
+    getBestBlock().map { block =>
+      new BestBlockchainBranch(
+        block.header,
+        blockNumberMappingStorage,
+        this
+      )
+    }
 
   def getBestBlockNumber(): BigInt = {
     val bestSavedBlockNumber = appStateStorage.getBestBlockNumber()
@@ -105,7 +110,7 @@ class BlockchainReader(
     getBlockHeaderByNumber(0).get
 
   def genesisBlock: Block =
-    getBestBranch().getBlockByNumber(0).get
+    getBlockByNumber(0).get
 
   /** Allows to query for a block based on it's number
     *

src/main/scala/io/iohk/ethereum/domain/branch/BestBlockchainBranch.scala

@@ -25,7 +25,7 @@ class BestBlockchainBranch(
    * */
 
   override def getBlockByNumber(number: BigInt): Option[Block] =
-    if (tipBlockHeader.number <= number && number > 0) {
+    if (tipBlockHeader.number >= number && number >= 0) {
       for {
         hash <- getHashByBlockNumber(number)
         block <- blockchainReader.getBlockByHash(hash)

src/main/scala/io/iohk/ethereum/jsonrpc/CheckpointingService.scala

@@ -36,7 +36,7 @@ class CheckpointingService(
       req.parentCheckpoint.forall(blockchainReader.getBlockHeaderByHash(_).exists(_.number < blockToReturnNum))
 
     Task {
-      blockchainReader.getBestBranch.getBlockByNumber(blockToReturnNum)
+      blockchainReader.getBestBranch().flatMap(_.getBlockByNumber(blockToReturnNum))
     }.flatMap {
       case Some(b) if isValidParent =>
         Task.now(Right(GetLatestBlockResponse(Some(BlockInfo(b.hash, b.number)))))

src/main/scala/io/iohk/ethereum/jsonrpc/EthProofService.scala

@@ -204,8 +204,9 @@ class EthProofService(
 
   private def resolveBlock(blockParam: BlockParam): Either[JsonRpcError, ResolvedBlock] = {
     def getBlock(number: BigInt): Either[JsonRpcError, Block] =
-      blockchainReader.getBestBranch
-        .getBlockByNumber(number)
+      blockchainReader
+        .getBestBranch()
+        .flatMap(_.getBlockByNumber(number))
         .toRight(JsonRpcError.InvalidParams(s"Block $number not found"))
 
     def getLatestBlock(): Either[JsonRpcError, Block] =

src/main/scala/io/iohk/ethereum/jsonrpc/EthTxService.scala

@@ -156,8 +156,9 @@ class EthTxService(
     val bestBlock = blockchainReader.getBestBlockNumber()
 
     Task {
+      val bestBranch = blockchainReader.getBestBranch()
       val gasPrice = ((bestBlock - blockDifference) to bestBlock)
-        .flatMap(blockchainReader.getBestBranch.getBlockByNumber)
+        .flatMap(nb => bestBranch.flatMap(_.getBlockByNumber(nb)))
         .flatMap(_.body.transactionList)
         .map(_.tx.gasPrice)
       if (gasPrice.nonEmpty) {

src/main/scala/io/iohk/ethereum/jsonrpc/ResolveBlock.scala

@@ -33,8 +33,9 @@ trait ResolveBlock {
     }
 
   private def getBlock(number: BigInt): Either[JsonRpcError, Block] =
-    blockchainReader.getBestBranch
-      .getBlockByNumber(number)
+    blockchainReader
+      .getBestBranch()
+      .flatMap(_.getBlockByNumber(number))
       .toRight(JsonRpcError.InvalidParams(s"Block $number not found"))
 
   private def getLatestBlock(): Either[JsonRpcError, Block] =

src/main/scala/io/iohk/ethereum/jsonrpc/TestService.scala

@@ -367,7 +367,7 @@ class TestService(
 
     val blockOpt = request.parameters.blockHashOrNumber
       .fold(
-        number => blockchainReader.getBestBranch.getBlockByNumber(number),
+        number => blockchainReader.getBestBranch().flatMap(_.getBlockByNumber(number)),
         blockHash => blockchainReader.getBlockByHash(blockHash)
       )
 
@@ -407,7 +407,7 @@ class TestService(
 
     val blockOpt = request.parameters.blockHashOrNumber
       .fold(
-        number => blockchainReader.getBestBranch.getBlockByNumber(number),
+        number => blockchainReader.getBestBranch().flatMap(_.getBlockByNumber(number)),
         hash => blockchainReader.getBlockByHash(hash)
       )
 

src/main/scala/io/iohk/ethereum/ledger/BlockImport.scala

@@ -308,7 +308,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.getBestBranch().flatMap(_.getBlockByNumber(fromNumber)) match {
         case Some(block) if block.header.hash == parent || fromNumber == 0 =>
           acc
 

src/main/scala/io/iohk/ethereum/ledger/BlockValidation.scala

@@ -41,8 +41,9 @@ class BlockValidation(
           val remaining = n - queuedBlocks.length - 1
 
           val numbers = (block.header.number - remaining) until block.header.number
+          val bestBranch = blockchainReader.getBestBranch()
           val blocks =
-            (numbers.toList.flatMap(blockchainReader.getBestBranch.getBlockByNumber) :+ block) ::: queuedBlocks
+            (numbers.toList.flatMap(nb => bestBranch.flatMap(_.getBlockByNumber(nb))) :+ block) ::: queuedBlocks
           blocks
       }
     }

src/main/scala/io/iohk/ethereum/ledger/BranchResolution.scala

@@ -72,10 +72,12 @@ class BranchResolution(blockchain: Blockchain, blockchainReader: BlockchainReade
     }
   }
 
-  private def getTopBlocksFromNumber(from: BigInt): List[Block] =
+  private def getTopBlocksFromNumber(from: BigInt): List[Block] = {
+    val bestBranch = blockchainReader.getBestBranch()
     (from to blockchainReader.getBestBlockNumber())
-      .flatMap(blockchainReader.getBestBranch.getBlockByNumber)
+      .flatMap(nb => bestBranch.flatMap(_.getBlockByNumber(nb)))
       .toList
+  }
 }
 
 sealed trait BranchResolutionResult

src/main/scala/io/iohk/ethereum/testmode/TestEthBlockServiceWrapper.scala

@@ -75,8 +75,8 @@ class TestEthBlockServiceWrapper(
     .getBlockByNumber(request)
     .map(
       _.map { blockByBlockResponse =>
-        val fullBlock =
-          blockchainReader.getBestBranch.getBlockByNumber(blockByBlockResponse.blockResponse.get.number).get
+        val bestBranch = blockchainReader.getBestBranch().get
+        val fullBlock = bestBranch.getBlockByNumber(blockByBlockResponse.blockResponse.get.number).get
         BlockByNumberResponse(blockByBlockResponse.blockResponse.map(response => toEthResponse(fullBlock, response)))
       }
     )

src/main/scala/io/iohk/ethereum/transactions/TransactionHistoryService.scala

@@ -33,7 +33,7 @@ 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.getBestBranch().flatMap(_.getBlockByNumber(blockNr))))(
         OverflowStrategy.Unbounded
       )
       .collect { case Some(block) => block }

src/test/scala/io/iohk/ethereum/consensus/pow/validators/StdOmmersValidatorSpec.scala

@@ -79,7 +79,9 @@ class StdOmmersValidatorSpec extends AnyFlatSpec with Matchers with ScalaCheckPr
 
   it should "report failure if there is an ommer which that is not parent of an ancestor" in new BlockUtils {
     val getNBlocksBack: (ByteString, Int) => List[Block] =
-      (_, n) => ((ommersBlockNumber - n) until ommersBlockNumber).toList.flatMap(blockchainReader.getBlockByNumber)
+      (_, n) =>
+        ((ommersBlockNumber - n) until ommersBlockNumber).toList
+          .flatMap(nb => blockchainReader.getBestBranch().flatMap(_.getBlockByNumber(nb)))
 
     ommersValidator.validateOmmersAncestors(
       ommersBlockParentHash,
@@ -462,6 +464,7 @@ class StdOmmersValidatorSpec extends AnyFlatSpec with Matchers with ScalaCheckPr
       .and(blockchain.storeBlock(block95))
       .and(blockchain.storeBlock(block96))
       .commit()
+    blockchain.saveBestKnownBlocks(block96.number)
 
   }
 }

src/test/scala/io/iohk/ethereum/domain/BlockchainSpec.scala

@@ -44,7 +44,8 @@ class BlockchainSpec extends AnyFlatSpec with Matchers with ScalaCheckPropertyCh
   it should "be able to store a block and retrieve it by number" in new EphemBlockchainTestSetup {
     val validBlock = Fixtures.Blocks.ValidBlock.block
     blockchain.storeBlock(validBlock).commit()
-    val block = blockchainReader.getBlockByNumber(validBlock.header.number)
+    blockchain.saveBestKnownBlocks(validBlock.number)
+    val block = blockchainReader.getBestBranch().get.getBlockByNumber(validBlock.header.number)
     block.isDefined should ===(true)
     validBlock should ===(block.get)
   }
@@ -67,8 +68,10 @@ class BlockchainSpec extends AnyFlatSpec with Matchers with ScalaCheckPropertyCh
   }
 
   it should "not return a value if not stored" in new EphemBlockchainTestSetup {
-    blockchainReader.getBlockByNumber(Fixtures.Blocks.ValidBlock.header.number).isEmpty should ===(true)
-    blockchainReader.getBlockByHash(Fixtures.Blocks.ValidBlock.header.hash).isEmpty should ===(true)
+    blockchainReader
+      .getBestBranch()
+      .flatMap(_.getBlockByNumber(Fixtures.Blocks.ValidBlock.header.number)) shouldBe None
+    blockchainReader.getBlockByHash(Fixtures.Blocks.ValidBlock.header.hash) shouldBe None
   }
 
   it should "be able to store a block with checkpoint and retrieve it and checkpoint" in new EphemBlockchainTestSetup {

src/test/scala/io/iohk/ethereum/jsonrpc/CheckpointingServiceSpec.scala

@@ -23,6 +23,7 @@ import io.iohk.ethereum.domain.BlockBody
 import io.iohk.ethereum.domain.BlockchainImpl
 import io.iohk.ethereum.domain.BlockchainReader
 import io.iohk.ethereum.domain.Checkpoint
+import io.iohk.ethereum.domain.branch.BlockchainBranch
 import io.iohk.ethereum.jsonrpc.CheckpointingService._
 import io.iohk.ethereum.ledger.BlockQueue
 
@@ -53,7 +54,7 @@ class CheckpointingServiceSpec
       val expectedResponse = GetLatestBlockResponse(Some(BlockInfo(block.hash, block.number)))
 
       (blockchainReader.getBestBlockNumber _).expects().returning(bestBlockNum)
-      (blockchainReader.getBlockByNumber _).expects(checkpointedBlockNum).returning(Some(block))
+      (bestChain.getBlockByNumber _).expects(checkpointedBlockNum).returning(Some(block))
       val result = service.getLatestBlock(request)
 
       result.runSyncUnsafe() shouldEqual Right(expectedResponse)
@@ -83,7 +84,7 @@ class CheckpointingServiceSpec
       (blockchainReader.getBlockHeaderByHash _)
         .expects(hash)
         .returning(Some(previousCheckpoint.header.copy(number = 0)))
-      (blockchainReader.getBlockByNumber _).expects(checkpointedBlockNum).returning(Some(block))
+      (bestChain.getBlockByNumber _).expects(checkpointedBlockNum).returning(Some(block))
       val result = service.getLatestBlock(request)
 
       result.runSyncUnsafe() shouldEqual Right(expectedResponse)
@@ -111,7 +112,7 @@ class CheckpointingServiceSpec
       (blockchainReader.getBlockHeaderByHash _)
         .expects(hash)
         .returning(Some(previousCheckpoint.header.copy(number = bestBlockNum)))
-      (blockchainReader.getBlockByNumber _).expects(*).returning(Some(previousCheckpoint))
+      (bestChain.getBlockByNumber _).expects(*).returning(Some(previousCheckpoint))
       val result = service.getLatestBlock(request)
 
       result.runSyncUnsafe() shouldEqual Right(expectedResponse)
@@ -139,7 +140,7 @@ class CheckpointingServiceSpec
 
       (blockchainReader.getBestBlockNumber _).expects().returning(bestBlockNum)
       (blockchainReader.getBlockHeaderByHash _).expects(hash).returning(None)
-      (blockchainReader.getBlockByNumber _).expects(checkpointedBlockNum).returning(Some(block))
+      (bestChain.getBlockByNumber _).expects(checkpointedBlockNum).returning(Some(block))
       val result = service.getLatestBlock(request)
 
       result.runSyncUnsafe() shouldEqual Right(expectedResponse)
@@ -167,13 +168,13 @@ class CheckpointingServiceSpec
     (blockchainReader.getBestBlockNumber _)
       .expects()
       .returning(7)
-    (blockchainReader.getBlockByNumber _)
+    (bestChain.getBlockByNumber _)
       .expects(BigInt(4))
       .returning(None)
     (blockchainReader.getBestBlockNumber _)
       .expects()
       .returning(7)
-    (blockchainReader.getBlockByNumber _)
+    (bestChain.getBlockByNumber _)
       .expects(BigInt(4))
       .returning(Some(block))
 
@@ -185,6 +186,8 @@ class CheckpointingServiceSpec
   trait TestSetup {
     val blockchain: BlockchainImpl = mock[BlockchainImpl]
     val blockchainReader: BlockchainReader = mock[BlockchainReader]
+    val bestChain: BlockchainBranch = mock[BlockchainBranch]
+    (blockchainReader.getBestBranch _).expects().anyNumberOfTimes().returning(Some(bestChain))
     val blockQueue: BlockQueue = mock[BlockQueue]
     val syncController: TestProbe = TestProbe()
     val checkpointBlockGenerator: CheckpointBlockGenerator = new CheckpointBlockGenerator()