[ETCM-1044] add best block hash into storage (#1077)

* Adds bestBlock hash in app state

* use getBlockByHash to get the best block number instead of getBlockByNumber

* Adds a first test in BlockchainReaderSpec

Author: Aurélien Richez

src/it/scala/io/iohk/ethereum/ledger/BlockImporterItSpec.scala

@@ -107,7 +107,7 @@ class BlockImporterItSpec
     blockchainWriter.save(oldBlock3, Nil, oldWeight3, saveAsBestBlock = true)
     blockchainWriter.save(oldBlock4, Nil, oldWeight4, saveAsBestBlock = true)
     // simulation of node restart
-    blockchain.saveBestKnownBlocks(blockchainReader.getBestBlockNumber() - 1)
+    blockchain.saveBestKnownBlocks(oldBlock3.header.hash, oldBlock3.number)
     blockchainWriter.save(newBlock4ParentOldBlock3, Nil, newBlock4WeightParentOldBlock3, saveAsBestBlock = true)
 
     //not reorganising anymore until oldBlock4(not part of the chain anymore), no block/ommer validation when not part of the chain, resolveBranch is returning UnknownBranch

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

@@ -196,7 +196,11 @@ class BlockchainMock(genesisHash: ByteString) extends Blockchain {
 
   def getBestBlockNumber(): BigInt = ???
 
-  def saveBestKnownBlocks(bestBlockNumber: BigInt, latestCheckpointNumber: Option[BigInt] = None): Unit = ???
+  override def saveBestKnownBlocks(
+      bestBlockhash: ByteString,
+      bestBlockNumber: BigInt,
+      latestCheckpointNumber: Option[BigInt] = None
+  ): Unit = ???
 
   def getBestBlock(): Option[Block] = ???
 
@@ -205,4 +209,5 @@ class BlockchainMock(genesisHash: ByteString) extends Blockchain {
   override def getBackingMptStorage(blockNumber: BigInt): MptStorage = ???
 
   override def getReadOnlyMptStorage(): MptStorage = ???
+
 }

src/main/scala/io/iohk/ethereum/blockchain/sync/fast/FastSync.scala

@@ -941,7 +941,7 @@ class FastSync(
       }
 
     def assignBlockchainWork(peerWithInfo: PeerWithInfo): Unit = {
-      val PeerWithInfo(peer, peerInfo) = peerWithInfo
+      val PeerWithInfo(peer, _) = peerWithInfo
       log.debug(s"Assigning blockchain work for peer [{}]", peer.id.value)
       if (syncState.receiptsQueue.nonEmpty) {
         requestReceipts(peer)

src/main/scala/io/iohk/ethereum/consensus/ConsensusImpl.scala

@@ -302,9 +302,9 @@ class ConsensusImpl(
       case BlockData(block, _, _) if block.hasCheckpoint => block.number
     }.maximumOption
 
-    val bestNumber = oldBranch.last.block.header.number
-    blockchain.saveBestKnownBlocks(bestNumber, checkpointNumber)
-    executedBlocks.foreach(data => blockQueue.enqueueBlock(data.block, bestNumber))
+    val bestHeader = oldBranch.last.block.header
+    blockchain.saveBestKnownBlocks(bestHeader.hash, bestHeader.number, checkpointNumber)
+    executedBlocks.foreach(data => blockQueue.enqueueBlock(data.block, bestHeader.number))
 
     newBranch.diff(executedBlocks.map(_.block)).headOption.foreach { block =>
       blockQueue.removeSubtree(block.header.hash)

src/main/scala/io/iohk/ethereum/db/storage/AppStateStorage.scala

@@ -2,11 +2,15 @@ package io.iohk.ethereum.db.storage
 
 import java.math.BigInteger
 
+import akka.util.ByteString
+
 import scala.collection.immutable.ArraySeq
 
 import io.iohk.ethereum.db.dataSource.DataSource
 import io.iohk.ethereum.db.dataSource.DataSourceBatchUpdate
 import io.iohk.ethereum.db.storage.AppStateStorage._
+import io.iohk.ethereum.domain.appstate.BlockInfo
+import io.iohk.ethereum.utils.Hex
 
 /** This class is used to store app state variables
   *   Key: see AppStateStorage.Keys
@@ -27,6 +31,16 @@ class AppStateStorage(val dataSource: DataSource) extends TransactionalKeyValueS
   def getBestBlockNumber(): BigInt =
     getBigInt(Keys.BestBlockNumber)
 
+  def getBestBlockInfo(): BlockInfo =
+    BlockInfo( // TODO ETCM-1090 provide the genesis hash as default
+      get(Keys.BestBlockHash).map(v => ByteString(Hex.decode(v))).getOrElse(ByteString.empty),
+      getBigInt(Keys.BestBlockNumber)
+    )
+
+  def putBestBlockInfo(b: BlockInfo): DataSourceBatchUpdate =
+    put(Keys.BestBlockNumber, b.number.toString)
+      .and(put(Keys.BestBlockHash, Hex.toHexString(b.hash.toArray)))
+
   def putBestBlockNumber(bestBlockNumber: BigInt): DataSourceBatchUpdate =
     put(Keys.BestBlockNumber, bestBlockNumber.toString)
 
@@ -72,9 +86,11 @@ object AppStateStorage {
 
   object Keys {
     val BestBlockNumber = "BestBlockNumber"
+    val BestBlockHash = "BestBlockHash"
     val FastSyncDone = "FastSyncDone"
     val EstimatedHighestBlock = "EstimatedHighestBlock"
     val SyncStartingBlock = "SyncStartingBlock"
     val LatestCheckpointBlockNumber = "LatestCheckpointBlockNumber"
   }
+
 }

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

@@ -7,6 +7,7 @@ import scala.annotation.tailrec
 import io.iohk.ethereum.db.dataSource.DataSourceBatchUpdate
 import io.iohk.ethereum.db.storage._
 import io.iohk.ethereum.domain
+import io.iohk.ethereum.domain.appstate.BlockInfo
 import io.iohk.ethereum.jsonrpc.ProofService.StorageProof
 import io.iohk.ethereum.ledger.InMemoryWorldStateProxy
 import io.iohk.ethereum.ledger.InMemoryWorldStateProxyStorage
@@ -64,7 +65,11 @@ trait Blockchain {
 
   def removeBlock(hash: ByteString): Unit
 
-  def saveBestKnownBlocks(bestBlockNumber: BigInt, latestCheckpointNumber: Option[BigInt] = None): Unit
+  def saveBestKnownBlocks(
+      bestBlockHash: ByteString,
+      bestBlockNumber: BigInt,
+      latestCheckpointNumber: Option[BigInt] = None
+  ): Unit
 
 }
 
@@ -126,20 +131,28 @@ class BlockchainImpl(
 
   def getReadOnlyMptStorage(): MptStorage = stateStorage.getReadOnlyStorage
 
-  override def saveBestKnownBlocks(bestBlockNumber: BigInt, latestCheckpointNumber: Option[BigInt] = None): Unit =
+  override def saveBestKnownBlocks(
+      bestBlockHash: ByteString,
+      bestBlockNumber: BigInt,
+      latestCheckpointNumber: Option[BigInt] = None
+  ): Unit =
     latestCheckpointNumber match {
       case Some(number) =>
-        saveBestKnownBlockAndLatestCheckpointNumber(bestBlockNumber, number)
+        saveBestKnownBlockAndLatestCheckpointNumber(bestBlockHash, bestBlockNumber, number)
       case None =>
-        saveBestKnownBlock(bestBlockNumber)
+        saveBestKnownBlock(bestBlockHash, bestBlockNumber)
     }
 
-  private def saveBestKnownBlock(bestBlockNumber: BigInt): Unit =
-    appStateStorage.putBestBlockNumber(bestBlockNumber).commit()
+  private def saveBestKnownBlock(bestBlockHash: ByteString, bestBlockNumber: BigInt): Unit =
+    appStateStorage.putBestBlockInfo(BlockInfo(bestBlockHash, bestBlockNumber)).commit()
 
-  private def saveBestKnownBlockAndLatestCheckpointNumber(number: BigInt, latestCheckpointNumber: BigInt): Unit =
+  private def saveBestKnownBlockAndLatestCheckpointNumber(
+      bestBlockHash: ByteString,
+      number: BigInt,
+      latestCheckpointNumber: BigInt
+  ): Unit =
     appStateStorage
-      .putBestBlockNumber(number)
+      .putBestBlockInfo(BlockInfo(bestBlockHash, number))
       .and(appStateStorage.putLatestCheckpointBlockNumber(latestCheckpointNumber))
       .commit()
 
@@ -163,15 +176,15 @@ class BlockchainImpl(
     log.debug(s"Trying to remove block ${block.idTag}")
 
     val txList = block.body.transactionList
-    val bestBlockNumber = blockchainReader.getBestBlockNumber()
     val latestCheckpointNumber = getLatestCheckpointBlockNumber()
 
     val blockNumberMappingUpdates =
       if (blockchainReader.getHashByBlockNumber(blockchainReader.getBestBranch(), block.number).contains(blockHash))
         removeBlockNumberMapping(block.number)
       else blockNumberMappingStorage.emptyBatchUpdate
 
-    val newBestBlockNumber: BigInt = (bestBlockNumber - 1).max(0)
+    val potentialNewBestBlockNumber: BigInt = (block.number - 1).max(0)
+    val potentialNewBestBlockHash: ByteString = block.header.parentHash
     val newLatestCheckpointNumber: BigInt =
       if (block.hasCheckpoint && block.number == latestCheckpointNumber) {
         findPreviousCheckpointBlockNumber(block.number, block.number)
@@ -187,8 +200,8 @@ class BlockchainImpl(
       into the case of having an incomplete best block and so an inconsistent db
      */
     val bestBlockNumberUpdates =
-      if (appStateStorage.getBestBlockNumber() > newBestBlockNumber)
-        appStateStorage.putBestBlockNumber(newBestBlockNumber)
+      if (appStateStorage.getBestBlockNumber() > potentialNewBestBlockNumber)
+        appStateStorage.putBestBlockInfo(BlockInfo(potentialNewBestBlockHash, potentialNewBestBlockNumber))
       else appStateStorage.emptyBatchUpdate
     val latestCheckpointNumberUpdates =
       if (appStateStorage.getLatestCheckpointBlockNumber() > newLatestCheckpointNumber)
@@ -197,7 +210,7 @@ class BlockchainImpl(
 
     log.debug(
       "Persisting app info data into database. Persisted block number is {}. Persisted checkpoint number is {}",
-      newBestBlockNumber,
+      potentialNewBestBlockNumber,
       newLatestCheckpointNumber
     )
 
@@ -215,7 +228,7 @@ class BlockchainImpl(
     log.debug(
       "Removed block with hash {}. New best block number - {}, new best checkpoint block number - {}",
       ByteStringUtils.hash2string(blockHash),
-      newBestBlockNumber,
+      potentialNewBestBlockNumber,
       newLatestCheckpointNumber
     )
   }

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

@@ -13,6 +13,7 @@ import io.iohk.ethereum.domain.branch.Branch
 import io.iohk.ethereum.domain.branch.EmptyBranch
 import io.iohk.ethereum.mpt.MerklePatriciaTrie
 import io.iohk.ethereum.mpt.MptNode
+import io.iohk.ethereum.utils.Hex
 import io.iohk.ethereum.utils.Logger
 
 class BlockchainReader(
@@ -84,9 +85,17 @@ class BlockchainReader(
 
   //returns the best known block if it's available in the storage
   def getBestBlock(): Option[Block] = {
-    val bestBlockNumber = getBestBlockNumber()
-    log.debug("Trying to get best block with number {}", bestBlockNumber)
-    getBlockByNumber(bestBlockNumber)
+    val bestKnownBlockinfo = appStateStorage.getBestBlockInfo()
+    log.debug("Trying to get best block with number {}", bestKnownBlockinfo.number)
+    val bestBlock = getBlockByHash(bestKnownBlockinfo.hash)
+    if (bestBlock.isEmpty) {
+      log.error(
+        "Best block {} (number: {}) not found in storage.",
+        Hex.toHexString(bestKnownBlockinfo.hash.toArray),
+        bestKnownBlockinfo.number
+      )
+    }
+    bestBlock
   }
 
   def genesisHeader: BlockHeader =

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

@@ -11,6 +11,7 @@ import io.iohk.ethereum.db.storage.ChainWeightStorage
 import io.iohk.ethereum.db.storage.ReceiptStorage
 import io.iohk.ethereum.db.storage.TransactionMappingStorage
 import io.iohk.ethereum.db.storage.TransactionMappingStorage.TransactionLocation
+import io.iohk.ethereum.domain.appstate.BlockInfo
 import io.iohk.ethereum.utils.Logger
 
 class BlockchainWriter(
@@ -31,14 +32,14 @@ class BlockchainWriter(
         block.header.number
       )
       appStateStorage
-        .putBestBlockNumber(block.header.number)
+        .putBestBlockInfo(BlockInfo(block.header.hash, block.header.number))
         .and(appStateStorage.putLatestCheckpointBlockNumber(block.header.number))
     } else if (saveAsBestBlock) {
       log.debug(
         "New best known block number - {}",
         block.header.number
       )
-      appStateStorage.putBestBlockNumber(block.header.number)
+      appStateStorage.putBestBlockInfo(BlockInfo(block.header.hash, block.header.number))
     } else {
       appStateStorage.emptyBatchUpdate
     }

src/main/scala/io/iohk/ethereum/domain/appstate/BlockInfo.scala

@@ -0,0 +1,5 @@
+package io.iohk.ethereum.domain.appstate
+
+import akka.util.ByteString
+
+case class BlockInfo(hash: ByteString, number: BigInt)

src/main/scala/io/iohk/ethereum/nodebuilder/StdNode.scala

@@ -1,6 +1,7 @@
 package io.iohk.ethereum.nodebuilder
 
 import akka.actor.typed.ActorSystem
+import akka.util.ByteString
 
 import scala.concurrent.Await
 import scala.concurrent.ExecutionContext.Implicits.global
@@ -18,6 +19,7 @@ import io.iohk.ethereum.network.discovery.PeerDiscoveryManager
 import io.iohk.ethereum.nodebuilder.tooling.PeriodicConsistencyCheck
 import io.iohk.ethereum.nodebuilder.tooling.StorageConsistencyChecker
 import io.iohk.ethereum.utils.Config
+import io.iohk.ethereum.utils.Hex
 
 /** A standard node is everything Ethereum prescribes except the mining algorithm,
   * which is plugged in dynamically.
@@ -32,6 +34,8 @@ abstract class BaseNode extends Node {
   def start(): Unit = {
     startMetricsClient()
 
+    fixDatabase()
+
     loadGenesisData()
 
     runDBConsistencyCheck()
@@ -132,6 +136,24 @@ abstract class BaseNode extends Node {
     tryAndLogFailure(() => Metrics.get().close())
     tryAndLogFailure(() => storagesInstance.dataSource.close())
   }
+
+  def fixDatabase(): Unit = {
+    // FIXME this is a temporary solution to avoid an incompatibility due to the introduction of the best block hash
+    // We can remove this fix when we release an incompatible version.
+    val bestBlockInfo = storagesInstance.storages.appStateStorage.getBestBlockInfo()
+    if (bestBlockInfo.hash == ByteString.empty && bestBlockInfo.number > 0) {
+      log.warn("Fixing best block hash into database for block {}", bestBlockInfo.number)
+      storagesInstance.storages.blockNumberMappingStorage.get(bestBlockInfo.number) match {
+        case Some(hash) =>
+          log.warn("Putting {} as the best block hash", Hex.toHexString(hash.toArray))
+          storagesInstance.storages.appStateStorage.putBestBlockInfo(bestBlockInfo.copy(hash = hash)).commit()
+        case None =>
+          log.error("No block found for number {} when trying to fix database", bestBlockInfo.number)
+      }
+
+    }
+
+  }
 }
 
 class StdNode extends BaseNode with StdMiningBuilder

src/test/scala/io/iohk/ethereum/ObjectGenerators.scala

@@ -33,8 +33,6 @@ trait ObjectGenerators {
 
   def intGen(min: Int, max: Int): Gen[Int] = Gen.choose(min, max)
 
-  def posIntGen(min: Int, max: Int): Gen[Int] = Gen.choose(min, max).suchThat(_ > 0)
-
   def intGen: Gen[Int] = Gen.choose(Int.MinValue, Int.MaxValue)
 
   def longGen: Gen[Long] = Gen.choose(Long.MinValue, Long.MaxValue)

src/test/scala/io/iohk/ethereum/blockchain/sync/SyncStateSchedulerSpec.scala

@@ -252,7 +252,7 @@ class SyncStateSchedulerSpec
         buildScheduler()
       val header = Fixtures.Blocks.ValidBlock.header.copy(stateRoot = worldHash, number = 1)
       schedulerBlockchainWriter.storeBlockHeader(header).commit()
-      schedulerBlockchain.saveBestKnownBlocks(1)
+      schedulerBlockchain.saveBestKnownBlocks(header.hash, 1)
       var state = scheduler.initState(worldHash).get
       while (state.activeRequest.nonEmpty) {
         val (allMissingNodes1, state2) = scheduler.getAllMissingNodes(state)

src/test/scala/io/iohk/ethereum/blockchain/sync/regular/RegularSyncSpec.scala

@@ -738,7 +738,7 @@ class RegularSyncSpec
           goToTop()
 
           val num: BigInt = 42
-          blockchain.saveBestKnownBlocks(num, Some(num))
+          blockchain.saveBestKnownBlocks(testBlocks.head.hash, num, Some(num))
 
           etcPeerManager.expectMsg(GetHandshakedPeers)
           etcPeerManager.reply(HandshakedPeers(handshakedPeers))

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

@@ -464,7 +464,7 @@ class StdOmmersValidatorSpec extends AnyFlatSpec with Matchers with ScalaCheckPr
       .and(blockchainWriter.storeBlock(block95))
       .and(blockchainWriter.storeBlock(block96))
       .commit()
-    blockchain.saveBestKnownBlocks(block96.number)
+    blockchain.saveBestKnownBlocks(block96.hash, block96.number)
 
   }
 }

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

@@ -0,0 +1,25 @@
+package io.iohk.ethereum.domain
+
+import org.bouncycastle.util.encoders.Hex
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+import org.scalatestplus.scalacheck.ScalaCheckPropertyChecks
+
+import io.iohk.ethereum.ObjectGenerators
+import io.iohk.ethereum.blockchain.sync.EphemBlockchainTestSetup
+import io.iohk.ethereum.network.p2p.messages.BaseETH6XMessages.NewBlock
+import io.iohk.ethereum.security.SecureRandomBuilder
+
+class BlockchainReaderSpec extends AnyFlatSpec with Matchers with ScalaCheckPropertyChecks with SecureRandomBuilder {
+
+  val chainId: Option[Byte] = Hex.decode("3d").headOption
+
+  "BlockchainReader" should "be able to get the best block after it was stored by BlockchainWriter" in new EphemBlockchainTestSetup {
+    forAll(ObjectGenerators.newBlockGen(secureRandom, chainId)) { case NewBlock(block, weight) =>
+      blockchainWriter.save(block, Nil, ChainWeight(0, weight), true)
+
+      blockchainReader.getBestBlock() shouldBe Some(block)
+    }
+  }
+
+}