[ETCM-415] Fix random subset of peers where block is broadcasted; removed unnecesary broadcast-new-block-hashes config (#814)

Author: Nicolás Tallar

src/main/resources/application.conf

@@ -388,10 +388,6 @@ mantis {
     # Maximum number of hashes processed form NewBlockHashes packet
     max-new-hashes = 64
 
-    # Set to false to disable broadcasting the NewBlockHashes message, as its usefulness is debatable,
-    # especially in the context of private networks
-    broadcast-new-block-hashes = true
-
     # This a recovery mechanism for the issue of missing state nodes during blocks execution:
     # off - missing state node will result in an exception
     # on - missing state node will be redownloaded from a peer and block execution will be retried. This can repeat

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

@@ -28,10 +28,7 @@ class BlockBroadcast(val etcPeerManager: ActorRef, syncConfig: SyncConfig) {
 
     broadcastNewBlock(newBlock, peersWithoutBlock)
 
-    if (syncConfig.broadcastNewBlockHashes) {
-      // NOTE: the usefulness of this message is debatable, especially in private networks
-      broadcastNewBlockHash(newBlock, peersWithoutBlock)
-    }
+    broadcastNewBlockHash(newBlock, peersWithoutBlock)
   }
 
   private def shouldSendNewBlock(newBlock: NewBlock, peerInfo: PeerInfo): Boolean =
@@ -58,6 +55,6 @@ class BlockBroadcast(val etcPeerManager: ActorRef, syncConfig: SyncConfig) {
     */
   private[sync] def obtainRandomPeerSubset(peers: Set[Peer]): Set[Peer] = {
     val numberOfPeersToSend = Math.sqrt(peers.size).toInt
-    Random.shuffle(peers).take(numberOfPeersToSend)
+    Random.shuffle(peers.toSeq).take(numberOfPeersToSend).toSet
   }
 }

src/main/scala/io/iohk/ethereum/utils/Config.scala

@@ -116,7 +116,6 @@ object Config {
       fastSyncThrottle: FiniteDuration,
       maxQueuedBlockNumberAhead: Int,
       maxQueuedBlockNumberBehind: Int,
-      broadcastNewBlockHashes: Boolean,
       maxNewBlockHashAge: Int,
       maxNewHashes: Int,
       redownloadMissingStateNodes: Boolean,
@@ -161,7 +160,6 @@ object Config {
         maxQueuedBlockNumberAhead = syncConfig.getInt("max-queued-block-number-ahead"),
         maxNewBlockHashAge = syncConfig.getInt("max-new-block-hash-age"),
         maxNewHashes = syncConfig.getInt("max-new-hashes"),
-        broadcastNewBlockHashes = syncConfig.getBoolean("broadcast-new-block-hashes"),
         redownloadMissingStateNodes = syncConfig.getBoolean("redownload-missing-state-nodes"),
         fastSyncBlockValidationK = syncConfig.getInt("fast-sync-block-validation-k"),
         fastSyncBlockValidationN = syncConfig.getInt("fast-sync-block-validation-n"),

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

@@ -14,7 +14,6 @@ import io.iohk.ethereum.network.p2p.messages.PV62.NewBlockHashes
 import io.iohk.ethereum.network.p2p.messages.{PV62, Versions}
 import io.iohk.ethereum.utils.Config
 
-import scala.concurrent.duration._
 import org.scalatest.flatspec.AnyFlatSpecLike
 import org.scalatest.matchers.should.Matchers
 
@@ -122,19 +121,6 @@ class BlockBroadcastSpec
     etcPeerManagerProbe.expectNoMessage()
   }
 
-  it should "not broadcast NewBlockHashes message when disable by configuration" in new TestSetup {
-    val updatedConfig = syncConfig.copy(broadcastNewBlockHashes = false)
-    override val blockBroadcast = new BlockBroadcast(etcPeerManagerProbe.ref, updatedConfig)
-
-    val blockHeader: BlockHeader = baseBlockHeader.copy(number = initialPeerInfo.maxBlockNumber + 1)
-    val newBlock = NewBlock(Block(blockHeader, BlockBody(Nil, Nil)), initialPeerInfo.chainWeight.increase(blockHeader))
-
-    blockBroadcast.broadcastBlock(newBlock, Map(peer -> initialPeerInfo))
-
-    etcPeerManagerProbe.expectMsg(EtcPeerManagerActor.SendMessage(newBlock, peer.id))
-    etcPeerManagerProbe.expectNoMessage(100.millis)
-  }
-
   class TestSetup(implicit system: ActorSystem) {
     val etcPeerManagerProbe = TestProbe()
 

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

@@ -33,7 +33,6 @@ trait TestSyncConfig extends SyncConfigBuilder {
     maxQueuedBlockNumberBehind = 10,
     maxNewBlockHashAge = 20,
     maxNewHashes = 64,
-    broadcastNewBlockHashes = true,
     redownloadMissingStateNodes = true,
     fastSyncBlockValidationK = 100,
     fastSyncBlockValidationN = 2048,