diff --git a/src/it/scala/io/iohk/ethereum/ledger/BlockImporterItSpec.scala b/src/it/scala/io/iohk/ethereum/ledger/BlockImporterItSpec.scala index 011928c07a..bce2ad7935 100644 --- a/src/it/scala/io/iohk/ethereum/ledger/BlockImporterItSpec.scala +++ b/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 + } } } diff --git a/src/main/scala/io/iohk/ethereum/domain/Blockchain.scala b/src/main/scala/io/iohk/ethereum/domain/Blockchain.scala index e1c6c32ac1..8c78bf0e3f 100644 --- a/src/main/scala/io/iohk/ethereum/domain/Blockchain.scala +++ b/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 diff --git a/src/main/scala/io/iohk/ethereum/ledger/BranchResolution.scala b/src/main/scala/io/iohk/ethereum/ledger/BranchResolution.scala index e5e2a425fd..34ee766a86 100644 --- a/src/main/scala/io/iohk/ethereum/ledger/BranchResolution.scala +++ b/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 diff --git a/src/test/scala/io/iohk/ethereum/domain/BlockchainSpec.scala b/src/test/scala/io/iohk/ethereum/domain/BlockchainSpec.scala index dd9e4e4392..1a6d73c5c8 100644 --- a/src/test/scala/io/iohk/ethereum/domain/BlockchainSpec.scala +++ b/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) diff --git a/src/test/scala/io/iohk/ethereum/ledger/BranchResolutionSpec.scala b/src/test/scala/io/iohk/ethereum/ledger/BranchResolutionSpec.scala index 3771aa7429..ba1f3addbd 100644 --- a/src/test/scala/io/iohk/ethereum/ledger/BranchResolutionSpec.scala +++ b/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,13 +51,13 @@ 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 @@ -66,7 +65,7 @@ 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) @@ -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))) diff --git a/src/test/scala/io/iohk/ethereum/ledger/LedgerTestSetup.scala b/src/test/scala/io/iohk/ethereum/ledger/LedgerTestSetup.scala index ddda90b46f..4999d3fb00 100644 --- a/src/test/scala/io/iohk/ethereum/ledger/LedgerTestSetup.scala +++ 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 {