Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Mar 24, 2021
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
68 changes: 54 additions & 14 deletions src/it/scala/io/iohk/ethereum/ledger/BlockImporterItSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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)
}
dzajkowski marked this conversation as resolved.
Show resolved Hide resolved

override lazy val ledger = new TestLedgerImpl(successValidators) {
override private[ledger] lazy val blockExecution =
new BlockExecution(blockchain, blockchainConfig, consensus.blockPreparator, blockValidation) {
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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
}
}
}
13 changes: 13 additions & 0 deletions src/main/scala/io/iohk/ethereum/domain/Blockchain.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
37 changes: 23 additions & 14 deletions src/test/scala/io/iohk/ethereum/domain/BlockchainSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
Expand Down
19 changes: 10 additions & 9 deletions src/test/scala/io/iohk/ethereum/ledger/BranchResolutionSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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)
Expand All @@ -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)
bsuieric marked this conversation as resolved.
Show resolved Hide resolved
setGenesisHeader(genesisHeader)
setBestBlockNumber(10)
setChainWeightByHash(genesisHeader.hash, ChainWeight.zero)
Expand All @@ -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)

Expand All @@ -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)
Expand All @@ -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)))
Expand All @@ -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)))
Expand All @@ -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)))
Expand Down
8 changes: 3 additions & 5 deletions src/test/scala/io/iohk/ethereum/ledger/LedgerTestSetup.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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] =
bsuieric marked this conversation as resolved.
Show resolved Hide resolved
(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 {
Expand Down