[ETCM-283] Fix and Refactor tests

Author: mirkoAlic

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

@@ -3,7 +3,6 @@ package io.iohk.ethereum.sync.util
 import akka.actor.ActorRef
 import akka.util.ByteString
 import cats.effect.Resource
-import io.iohk.ethereum.Mocks.MockValidatorsAlwaysSucceed
 import io.iohk.ethereum.blockchain.sync.{PeersClient, SyncProtocol}
 import io.iohk.ethereum.blockchain.sync.regular.BlockBroadcasterActor.BroadcastBlock
 import io.iohk.ethereum.blockchain.sync.regular.RegularSync
@@ -63,7 +62,7 @@ object RegularSyncItSpecUtils {
       "pending-transactions-manager"
     )
 
-    lazy val validators = new MockValidatorsAlwaysSucceed
+    lazy val validators = buildEthashConsensus.validators
 
     lazy val regularSync = system.actorOf(
       RegularSync.props(

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcherState.scala

@@ -148,7 +148,7 @@ case class BlockFetcherState(
         else
           bodiesAreOrderedSubsetOfRequested(remainingHeaders, respondedBodies, matchedBlocks)
     }
-  
+
   /**
     * If blocks is empty collection - headers in queue are removed as the cause is:
     *   - the headers are from rejected fork and therefore it won't be possible to resolve blocks for them
@@ -161,7 +161,6 @@ case class BlockFetcherState(
         waitingHeaders = Queue.empty
       )
     else
-      // We could optimize this by stopping as soon as a block is not enqueued.
       blocks.foldLeft(this) { case (state, block) =>
         state.enqueueRequestedBlock(block, fromPeer)
       }
@@ -174,7 +173,7 @@ case class BlockFetcherState(
     waitingHeaders.dequeueOption
       .map { case (waitingHeader, waitingHeadersTail) =>
         if (waitingHeader.hash == block.hash)
-          withPeerForBlocks(fromPeer, Seq(block.header.number))
+          withPeerForBlocks(fromPeer, Seq(block.number))
             .withPossibleNewTopAt(block.number)
             .withLastBlock(block.number)
             .copy(

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

@@ -4,11 +4,7 @@ import java.net.InetSocketAddress
 import akka.actor.ActorSystem
 import akka.testkit.{TestKit, TestProbe}
 import com.miguno.akka.testing.VirtualTime
-import io.iohk.ethereum.Mocks.{
-  MockValidatorsAlwaysSucceed,
-  MockValidatorsFailingOnBlockBodies,
-  MockValidatorsFailOnSpecificBlockNumber
-}
+import io.iohk.ethereum.Mocks.{MockValidatorsAlwaysSucceed, MockValidatorsFailingOnBlockBodies}
 import io.iohk.ethereum.BlockHelpers
 import io.iohk.ethereum.Fixtures.{Blocks => FixtureBlocks}
 import io.iohk.ethereum.blockchain.sync.PeersClient.BlacklistPeer
@@ -129,7 +125,7 @@ class BlockFetcherSpec extends TestKit(ActorSystem("BlockFetcherSpec_System")) w
       peersClient.expectMsgPF() { case PeersClient.Request(msg, _, _) if msg == firstGetBlockHeadersRequest => () }
     }
 
-    "should not append new blocks if the received data does not match" in new TestSetup {
+    "should not enqueue requested blocks if the received bodies does not match" in new TestSetup {
 
       // Important: Here we are forcing the mismatch between request headers and received bodies
       override lazy val validators = new MockValidatorsFailingOnBlockBodies
@@ -151,8 +147,14 @@ class BlockFetcherSpec extends TestKit(ActorSystem("BlockFetcherSpec_System")) w
       val getBlockBodiesResponse = BlockBodies(chain.map(_.body))
       peersClient.reply(PeersClient.Response(fakePeer, getBlockBodiesResponse))
 
+      // Fetcher should blacklist the peer and retry asking for the same bodies
       peersClient.expectMsgClass(classOf[BlacklistPeer])
       peersClient.expectMsgPF() { case PeersClient.Request(msg, _, _) if msg == getBlockBodiesRequest => () }
+
+      // Fetcher should not enqueue any new block
+      importer.send(blockFetcher, PickBlocks(syncConfig.blocksBatchSize))
+      importer.ignoreMsg({ case BlockImporter.NotOnTop => true })
+      importer.expectNoMessage()
     }
 
     "should be able to handle block bodies received in several parts" in new TestSetup {
@@ -172,13 +174,13 @@ class BlockFetcherSpec extends TestKit(ActorSystem("BlockFetcherSpec_System")) w
       peersClient.expectMsgPF() { case PeersClient.Request(msg, _, _) if msg == getBlockBodiesRequest1 => () }
 
       // It will receive all the requested bodies, but splitted in 2 parts.
-      val (subChain1, subChain2) = chain.splitAt(syncConfig.blockHeadersPerRequest / 2)
+      val (subChain1, subChain2) = chain.splitAt(syncConfig.blockBodiesPerRequest / 2)
 
       val getBlockBodiesResponse1 = BlockBodies(subChain1.map(_.body))
       peersClient.reply(PeersClient.Response(fakePeer, getBlockBodiesResponse1))
 
       val getBlockHeadersRequest2 =
-        GetBlockHeaders(Left(subChain1.last.number + 1), syncConfig.blockHeadersPerRequest, skip = 0, reverse = false)
+        GetBlockHeaders(Left(chain.last.number + 1), syncConfig.blockHeadersPerRequest, skip = 0, reverse = false)
       peersClient.expectMsgPF() { case PeersClient.Request(msg, _, _) if msg == getBlockHeadersRequest2 => () }
 
       val getBlockBodiesRequest2 = GetBlockBodies(subChain2.map(_.hash))
@@ -188,13 +190,16 @@ class BlockFetcherSpec extends TestKit(ActorSystem("BlockFetcherSpec_System")) w
       peersClient.reply(PeersClient.Response(fakePeer, getBlockBodiesResponse2))
 
       peersClient.expectNoMessage()
-    }
 
-    "should ignore response, without blacklist the peer, in case a sub ordered block bodies chain is received" in new TestSetup {
+      // Fetcher should enqueue all the received blocks
+      importer.send(blockFetcher, PickBlocks(chain.size))
+      importer.ignoreMsg({ case BlockImporter.NotOnTop => true })
+      importer.expectMsgPF() { case BlockFetcher.PickedBlocks(blocks) =>
+        blocks.map(_.hash).toList shouldEqual chain.map(_.hash)
+      }
+    }
 
-      // Important: Here (in a hacky way) we are enforcing received bodies
-      // to be a sub ordered chain that fetcher can't append given their current state
-      override lazy val validators = new MockValidatorsFailOnSpecificBlockNumber(1)
+    "should stop requesting, without blacklist the peer, in case empty bodies are received" in new TestSetup {
 
       startFetcher()
 
@@ -210,12 +215,31 @@ class BlockFetcherSpec extends TestKit(ActorSystem("BlockFetcherSpec_System")) w
       val getBlockBodiesRequest1 = GetBlockBodies(chain.map(_.hash))
       peersClient.expectMsgPF() { case PeersClient.Request(msg, _, _) if msg == getBlockBodiesRequest1 => () }
 
-      val (subChain1, _) = chain.splitAt(syncConfig.blockHeadersPerRequest / 2)
+      // It will receive part of the requested bodies.
+      val (subChain1, subChain2) = chain.splitAt(syncConfig.blockBodiesPerRequest / 2)
 
       val getBlockBodiesResponse1 = BlockBodies(subChain1.map(_.body))
       peersClient.reply(PeersClient.Response(fakePeer, getBlockBodiesResponse1))
 
-      peersClient.expectMsgPF() { case PeersClient.Request(msg, _, _) if msg == getBlockBodiesRequest1 => () }
+      val getBlockHeadersRequest2 =
+        GetBlockHeaders(Left(chain.last.number + 1), syncConfig.blockHeadersPerRequest, skip = 0, reverse = false)
+      peersClient.expectMsgPF() { case PeersClient.Request(msg, _, _) if msg == getBlockHeadersRequest2 => () }
+
+      val getBlockBodiesRequest2 = GetBlockBodies(subChain2.map(_.hash))
+      peersClient.expectMsgPF() { case PeersClient.Request(msg, _, _) if msg == getBlockBodiesRequest2 => () }
+
+      // We receive empty bodies instead of the second part
+      val getBlockBodiesResponse2 = BlockBodies(List())
+      peersClient.reply(PeersClient.Response(fakePeer, getBlockBodiesResponse2))
+
+      peersClient.expectNoMessage()
+
+      // If we try to pick the whole chain we should only receive the first part
+      importer.send(blockFetcher, PickBlocks(chain.size))
+      importer.ignoreMsg({ case BlockImporter.NotOnTop => true })
+      importer.expectMsgPF() { case BlockFetcher.PickedBlocks(blocks) =>
+        blocks.map(_.hash).toList shouldEqual subChain1.map(_.hash)
+      }
     }
 
     "should ensure blocks passed to importer are always forming chain" in new TestSetup {

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

@@ -3,10 +3,7 @@ package io.iohk.ethereum.blockchain.sync.regular
 import akka.actor.ActorSystem
 import akka.testkit.{TestKit, TestProbe}
 import io.iohk.ethereum.Mocks.MockValidatorsAlwaysSucceed
-import io.iohk.ethereum.domain.Block
 import io.iohk.ethereum.BlockHelpers
-import io.iohk.ethereum.Fixtures.Blocks.ValidBlock
-import io.iohk.ethereum.domain.Block
 import io.iohk.ethereum.network.PeerId
 import org.scalatest.matchers.should.Matchers
 import org.scalatest.wordspec.AnyWordSpecLike
@@ -30,35 +27,35 @@ class BlockFetcherStateSpec extends TestKit(ActorSystem()) with AnyWordSpecLike
 
     "handling requested blocks" should {
       "clear headers queue if got empty list of blocks" in {
+        val importer = TestProbe().ref
         val headers = BlockHelpers.generateChain(5, BlockHelpers.genesis).map(_.header)
         val peer = PeerId("foo")
 
         val result = BlockFetcherState
-          .initial(TestProbe().ref, 0)
+          .initial(importer, validators.blockValidator, 0)
           .appendHeaders(headers)
-          .map(_.handleRequestedBlocks(peer, List()))
-          .map(_.waitingHeaders)
+          .map(_.handleRequestedBlocks(List(), peer))
 
-        assert(result === Right(Queue.empty))
-        result.lastBlock shouldEqual 0
+        assert(result.map(_.waitingHeaders) === Right(Queue.empty))
+        assert(result.map(_.lastBlock) === Right(headers.last.number))
       }
 
       "enqueue requested blocks" in {
-
+        val importer = TestProbe().ref
         val blocks = BlockHelpers.generateChain(5, BlockHelpers.genesis)
         val peer = PeerId("foo")
 
         val result = BlockFetcherState
-          .initial(TestProbe().ref, 0)
+          .initial(importer, validators.blockValidator, 0)
           .appendHeaders(blocks.map(_.header))
-          .map(_.handleRequestedBlocks(peer, blocks.map(_.body)))
+          .map(_.handleRequestedBlocks(blocks, peer))
 
-        assert(result.waitingHeaders === Right(Queue.empty))
-        result.lastBlock shouldEqual blocks.lastBlock.number
-        blocks.forEach { block =>
-          result.blockProviders(block.number) shouldEqual fakePeerId
+        assert(result.map(_.waitingHeaders) === Right(Queue.empty))
+        assert(result.map(_.lastBlock) === Right(blocks.last.number))
+        blocks.foreach { block =>
+          assert(result.map(_.blockProviders(block.number)) === Right(peer))
         }
-        result.knownTop shouldEqual blocks.lastBlock.number
+        assert(result.map(_.knownTop) === Right(blocks.last.number))
       }
     }
   }