Merge pull request #940 from input-output-hk/bug/ETCM-678-after-node-restart

etcm-678 etcm-660 fix for removing chain after the node restart

Author: bsuieric

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

@@ -6,16 +6,20 @@ import cats.data.NonEmptyList
 import io.iohk.ethereum.blockchain.sync.regular.BlockImporter.NewCheckpoint
 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.validators.Validators
 import io.iohk.ethereum.domain._
 import io.iohk.ethereum.mpt.MerklePatriciaTrie
 import io.iohk.ethereum.utils.Config.SyncConfig
 import io.iohk.ethereum.utils.Config
-import io.iohk.ethereum.{Fixtures, ObjectGenerators, crypto}
+import io.iohk.ethereum.{Fixtures, Mocks, ObjectGenerators, crypto}
 import io.iohk.ethereum.ledger.Ledger.BlockResult
 import monix.execution.Scheduler
 import org.scalamock.scalatest.MockFactory
 import org.scalatest.BeforeAndAfterAll
+import org.scalatest.concurrent.Eventually.eventually
 import org.scalatest.flatspec.AsyncFlatSpecLike
 import org.scalatest.matchers.should.Matchers
 
@@ -62,6 +66,18 @@ class BlockImporterItSpec
       ethCompatibleStorage = true
     )
 
+  override protected lazy val successValidators: Validators = new Mocks.MockValidatorsAlwaysSucceed {
+    override val ommersValidator: OmmersValidator = (
+        parentHash: ByteString,
+        blockNumber: BigInt,
+        ommers: Seq[BlockHeader],
+        getBlockHeaderByHash: GetBlockHeaderByHash,
+        getNBlocksBack: GetNBlocksBack
+    ) =>
+      new StdOmmersValidator(blockchainConfig, blockHeaderValidator)
+        .validate(parentHash, blockNumber, ommers, getBlockHeaderByHash, getNBlocksBack)
+  }
+
   override lazy val ledger = new TestLedgerImpl(successValidators) {
     override private[ledger] lazy val blockExecution =
       new BlockExecution(blockchain, blockchainConfig, consensus.blockPreparator, blockValidation) {
@@ -135,9 +151,8 @@ class BlockImporterItSpec
     blockImporter ! BlockImporter.Start
     blockImporter ! BlockFetcher.PickedBlocks(NonEmptyList.fromListUnsafe(newBranch))
 
-    Thread.sleep(1000)
     //because the blocks are not valid, we shouldn't reorganise, but at least stay with a current chain, and the best block of the current chain is oldBlock4
-    blockchain.getBestBlock().get shouldEqual oldBlock4
+    eventually { blockchain.getBestBlock().get shouldEqual oldBlock4 }
   }
 
   it should "return a correct new best block after reorganising longer chain to a shorter one if its weight is bigger" in {
@@ -149,8 +164,34 @@ class BlockImporterItSpec
 
     blockImporter ! BlockFetcher.PickedBlocks(NonEmptyList.fromListUnsafe(newBranch))
 
-    Thread.sleep(200)
-    blockchain.getBestBlock().get shouldEqual newBlock3
+    eventually { Thread.sleep(200); blockchain.getBestBlock().get shouldEqual newBlock3 }
+  }
+
+  it should "return Unknown branch, in case of PickedBlocks with block that has a parent that's not in the chain" in {
+    val newBlock4ParentOldBlock3: Block =
+      getBlock(genesisBlock.number + 4, difficulty = 104, parent = oldBlock3.header.hash)
+    val newBlock4WeightParentOldBlock3 = oldWeight3.increase(newBlock4ParentOldBlock3.header)
+
+    //Block n5 with oldBlock4 as parent
+    val newBlock5ParentOldBlock4: Block =
+      getBlock(
+        genesisBlock.number + 5,
+        difficulty = 108,
+        parent = oldBlock4.header.hash,
+        ommers = Seq(oldBlock4.header)
+      )
+
+    blockchain.save(oldBlock2, Nil, oldWeight2, saveAsBestBlock = true)
+    blockchain.save(oldBlock3, Nil, oldWeight3, saveAsBestBlock = true)
+    blockchain.save(oldBlock4, Nil, oldWeight4, saveAsBestBlock = true)
+    // simulation of node restart
+    blockchain.saveBestKnownBlocks(blockchain.getBestBlockNumber() - 1)
+    blockchain.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
+    blockImporter ! BlockFetcher.PickedBlocks(NonEmptyList.fromListUnsafe(List(newBlock5ParentOldBlock4)))
+
+    eventually { blockchain.getBestBlock().get shouldEqual newBlock4ParentOldBlock3 }
   }
 
   it should "switch to a branch with a checkpoint" in {
@@ -163,9 +204,8 @@ class BlockImporterItSpec
 
     blockImporter ! BlockFetcher.PickedBlocks(NonEmptyList.fromListUnsafe(newBranch))
 
-    Thread.sleep(200)
-    blockchain.getBestBlock().get shouldEqual oldBlock5WithCheckpoint
-    blockchain.getLatestCheckpointBlockNumber() shouldEqual oldBlock5WithCheckpoint.header.number
+    eventually { blockchain.getBestBlock().get shouldEqual oldBlock5WithCheckpoint }
+    eventually { blockchain.getLatestCheckpointBlockNumber() shouldEqual oldBlock5WithCheckpoint.header.number }
   }
 
   it should "switch to a branch with a newer checkpoint" in {
@@ -178,9 +218,8 @@ class BlockImporterItSpec
 
     blockImporter ! BlockFetcher.PickedBlocks(NonEmptyList.fromListUnsafe(newBranch))
 
-    Thread.sleep(200)
-    blockchain.getBestBlock().get shouldEqual newBlock4WithCheckpoint
-    blockchain.getLatestCheckpointBlockNumber() shouldEqual newBlock4WithCheckpoint.header.number
+    eventually { blockchain.getBestBlock().get shouldEqual newBlock4WithCheckpoint }
+    eventually { blockchain.getLatestCheckpointBlockNumber() shouldEqual newBlock4WithCheckpoint.header.number }
   }
 
   it should "return a correct checkpointed block after receiving a request for generating a new checkpoint" in {
@@ -199,8 +238,9 @@ class BlockImporterItSpec
 
     val checkpointBlock = checkpointBlockGenerator.generate(newBlock5, Checkpoint(signatures))
 
-    Thread.sleep(1000)
-    blockchain.getBestBlock().get shouldEqual checkpointBlock
-    blockchain.getLatestCheckpointBlockNumber() shouldEqual newBlock5.header.number + 1
+    eventually { Thread.sleep(1000); blockchain.getBestBlock().get shouldEqual checkpointBlock }
+    eventually {
+      Thread.sleep(1000); blockchain.getLatestCheckpointBlockNumber() shouldEqual newBlock5.header.number + 1
+    }
   }
 }

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

@@ -215,6 +215,19 @@ trait Blockchain {
   def getStateStorage: StateStorage
 
   def mptStateSavedKeys(): Observable[Either[IterationError, ByteString]]
+
+  /**
+    * Strict check if given block hash is in chain
+    * Using any of getXXXByHash is not always accurate - after restart the best block is often lower than before restart
+    * The result of that is returning data of blocks which we don't consider as a part of the chain anymore
+    * @param hash block hash
+    */
+  def isInChain(hash: ByteString): Boolean = {
+    (for {
+      header <- getBlockHeaderByHash(hash) if header.number <= getBestBlockNumber()
+      hash <- getHashByBlockNumber(header.number)
+    } yield header.hash == hash).getOrElse(false)
+  }
 }
 // scalastyle:on
 

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

@@ -11,8 +11,7 @@ class BranchResolution(blockchain: Blockchain) extends Logger {
       InvalidBranch
     } else {
       val knownParentOrGenesis = blockchain
-        .getBlockHeaderByHash(headers.head.parentHash)
-        .isDefined || headers.head.hash == blockchain.genesisHeader.hash
+        .isInChain(headers.head.parentHash) || headers.head.hash == blockchain.genesisHeader.hash
 
       if (!knownParentOrGenesis)
         UnknownBranch

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

@@ -26,35 +26,44 @@ class BlockchainSpec extends AnyFlatSpec with Matchers with ScalaCheckPropertyCh
     val validBlock = Fixtures.Blocks.ValidBlock.block
     blockchain.storeBlock(validBlock).commit()
     val block = blockchain.getBlockByHash(validBlock.header.hash)
-    assert(block.isDefined)
-    assert(validBlock == block.get)
+    block.isDefined should ===(true)
+    validBlock should ===(block.get)
     val blockHeader = blockchain.getBlockHeaderByHash(validBlock.header.hash)
-    assert(blockHeader.isDefined)
-    assert(validBlock.header == blockHeader.get)
+    blockHeader.isDefined should ===(true)
+    validBlock.header should ===(blockHeader.get)
     val blockBody = blockchain.getBlockBodyByHash(validBlock.header.hash)
-    assert(blockBody.isDefined)
-    assert(validBlock.body == blockBody.get)
+    blockBody.isDefined should ===(true)
+    validBlock.body should ===(blockBody.get)
   }
 
   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 = blockchain.getBlockByNumber(validBlock.header.number)
-    assert(block.isDefined)
-    assert(validBlock == block.get)
+    block.isDefined should ===(true)
+    validBlock should ===(block.get)
+  }
+
+  it should "be able to do strict check of block existence in the chain" in new EphemBlockchainTestSetup {
+    val validBlock = Fixtures.Blocks.ValidBlock.block
+    blockchain.save(validBlock, Seq.empty, ChainWeight(100, 100), saveAsBestBlock = true)
+    blockchain.isInChain(validBlock.hash) === (false)
+    // simulation of node restart
+    blockchain.saveBestKnownBlocks(validBlock.header.number - 1)
+    blockchain.isInChain(validBlock.hash) should ===(false)
   }
 
   it should "be able to query a stored blockHeader by it's number" in new EphemBlockchainTestSetup {
     val validHeader = Fixtures.Blocks.ValidBlock.header
     blockchain.storeBlockHeader(validHeader).commit()
     val header = blockchain.getBlockHeaderByNumber(validHeader.number)
-    assert(header.isDefined)
-    assert(validHeader == header.get)
+    header.isDefined should ===(true)
+    validHeader should ===(header.get)
   }
 
   it should "not return a value if not stored" in new EphemBlockchainTestSetup {
-    assert(blockchain.getBlockByNumber(Fixtures.Blocks.ValidBlock.header.number).isEmpty)
-    assert(blockchain.getBlockByHash(Fixtures.Blocks.ValidBlock.header.hash).isEmpty)
+    blockchain.getBlockByNumber(Fixtures.Blocks.ValidBlock.header.number).isEmpty should ===(true)
+    blockchain.getBlockByHash(Fixtures.Blocks.ValidBlock.header.hash).isEmpty should ===(true)
   }
 
   it should "be able to store a block with checkpoint and retrieve it and checkpoint" in new EphemBlockchainTestSetup {
@@ -66,8 +75,8 @@ class BlockchainSpec extends AnyFlatSpec with Matchers with ScalaCheckPropertyCh
     blockchain.save(validBlock, Seq.empty, ChainWeight(0, 0), saveAsBestBlock = true)
 
     val retrievedBlock = blockchain.getBlockByHash(validBlock.header.hash)
-    assert(retrievedBlock.isDefined)
-    assert(validBlock == retrievedBlock.get)
+    retrievedBlock.isDefined should ===(true)
+    validBlock should ===(retrievedBlock.get)
 
     blockchain.getLatestCheckpointBlockNumber() should ===(validBlock.number)
     blockchain.getBestBlockNumber() should ===(validBlock.number)

src/test/scala/io/iohk/ethereum/ledger/BranchResolutionSpec.scala

@@ -41,8 +41,7 @@ class BranchResolutionSpec
       val headers = getChainHeadersNel(5, 10)
 
       setGenesisHeader(genesisHeader) // Check genesis block
-      setBestBlockNumber(10)
-      setHeaderByHash(headers.head.parentHash, None)
+      setHeaderInChain(headers.head.parentHash, result = false)
 
       ledger.resolveBranch(headers) shouldEqual UnknownBranch
     }
@@ -52,21 +51,21 @@ class BranchResolutionSpec
         val headers = getChainHeadersNel(1, 10)
 
         setBestBlockNumber(10)
-        setHeaderByHash(headers.head.parentHash, Some(getBlock(0).header))
+        setHeaderInChain(headers.head.parentHash)
         setChainWeightByHash(headers.head.parentHash, ChainWeight.zero)
 
         val oldBlocks = getChain(1, 10, headers.head.parentHash, headers.head.difficulty - 1)
         oldBlocks.map(b => setBlockByNumber(b.header.number, Some(b)))
 
-        ledger.resolveBranch(headers) shouldEqual NewBetterBranch(oldBlocks.toList)
+        ledger.resolveBranch(headers) shouldEqual NewBetterBranch(oldBlocks)
       }
 
     "report no need for a chain switch the headers do not have chain weight greater than currently known branch" in
       new BranchResolutionTestSetup {
         val headers = getChainHeadersNel(1, 10)
 
         setBestBlockNumber(10)
-        setHeaderByHash(headers.head.parentHash, Some(getBlock(0).header))
+        setHeaderInChain(headers.head.parentHash)
         setChainWeightByHash(headers.head.parentHash, ChainWeight.zero)
 
         val oldBlocks = getChain(1, 10, headers.head.parentHash, headers.head.difficulty)
@@ -78,6 +77,7 @@ class BranchResolutionSpec
     "correctly handle a branch that goes up to the genesis block" in new BranchResolutionTestSetup {
       val headers = genesisHeader :: getChainHeadersNel(1, 10, genesisHeader.hash)
 
+      setHeaderInChain(genesisHeader.parentHash, result = false)
       setGenesisHeader(genesisHeader)
       setBestBlockNumber(10)
       setChainWeightByHash(genesisHeader.hash, ChainWeight.zero)
@@ -93,6 +93,7 @@ class BranchResolutionSpec
       val differentGenesis: BlockHeader = genesisHeader.copy(extraData = ByteString("I'm different ;("))
       val headers = differentGenesis :: getChainHeadersNel(1, 10, differentGenesis.hash)
 
+      setHeaderInChain(differentGenesis.parentHash, result = false)
       setGenesisHeader(genesisHeader)
       setBestBlockNumber(10)
 
@@ -104,7 +105,7 @@ class BranchResolutionSpec
       val commonParent = headers.toList(1)
 
       setBestBlockNumber(8)
-      setHeaderByHash(headers.head.parentHash, Some(getBlock(0).header))
+      setHeaderInChain(headers.head.parentHash)
       setChainWeightByHash(commonParent.hash, ChainWeight.zero)
 
       val oldBlocks = getChain(3, 8, commonParent.hash)
@@ -123,7 +124,7 @@ class BranchResolutionSpec
       val longerBranchLowerWeight = getChain(2, 10, commonParent.hash, difficulty = 100)
       val shorterBranchHigherWeight = getChainNel(2, 8, commonParent.hash, difficulty = 200)
 
-      setHeaderByHash(commonParent.hash, Some(commonParent.header))
+      setHeaderInChain(commonParent.hash)
       setChainWeightForBlock(commonParent, parentWeight)
       setBestBlockNumber(longerBranchLowerWeight.last.number)
       longerBranchLowerWeight.foreach(b => setBlockByNumber(b.number, Some(b)))
@@ -150,7 +151,7 @@ class BranchResolutionSpec
 
           val noCheckpointBranch = getChain(2, checkpointBranchLength + 2, commonParent.hash)
 
-          setHeaderByHash(commonParent.hash, Some(commonParent.header))
+          setHeaderInChain(commonParent.hash)
           setChainWeightForBlock(commonParent, parentWeight)
           setBestBlockNumber(noCheckpointBranch.last.number)
           noCheckpointBranch.foreach(b => setBlockByNumber(b.number, Some(b)))
@@ -176,7 +177,7 @@ class BranchResolutionSpec
 
           val noCheckpointBranch = getChainNel(2, checkpointBranchLength + 2, commonParent.hash)
 
-          setHeaderByHash(commonParent.hash, Some(commonParent.header))
+          setHeaderInChain(commonParent.hash)
           setChainWeightForBlock(commonParent, parentWeight)
           setBestBlockNumber(checkpointBranch.last.number)
           checkpointBranch.map(b => setBlockByNumber(b.number, Some(b)))

src/test/scala/io/iohk/ethereum/ledger/LedgerTestSetup.scala

@@ -400,16 +400,14 @@ trait MockBlockchain extends MockFactory { self: TestSetupWithVmAndValidators =>
       .once()
   }
 
-  def setHeaderByHash(hash: ByteString, header: Option[BlockHeader]): CallHandler1[ByteString, Option[BlockHeader]] =
-    (blockchain.getBlockHeaderByHash _).expects(hash).returning(header)
+  def setHeaderInChain(hash: ByteString, result: Boolean = true): CallHandler1[ByteString, Boolean] =
+    (blockchain.isInChain _).expects(hash).returning(result)
 
   def setBlockByNumber(number: BigInt, block: Option[Block]): CallHandler1[BigInt, Option[Block]] =
     (blockchain.getBlockByNumber _).expects(number).returning(block)
 
-  def setGenesisHeader(header: BlockHeader): Unit = {
+  def setGenesisHeader(header: BlockHeader): Unit =
     (() => blockchain.genesisHeader).expects().returning(header)
-    setHeaderByHash(header.parentHash, None)
-  }
 }
 
 trait EphemBlockchain extends TestSetupWithVmAndValidators with MockFactory {