From 771e8f3724be3e89ada2df0862008ff603b1999a Mon Sep 17 00:00:00 2001 From: Nicolas Tallar Date: Wed, 25 Nov 2020 14:49:53 -0300 Subject: [PATCH] [ETCM-415] Fix random subset of peers where block is broadcasted; removed unnecesary broadcast-new-block-hashes config --- src/main/resources/application.conf | 4 ---- .../blockchain/sync/regular/BlockBroadcast.scala | 7 ++----- src/main/scala/io/iohk/ethereum/utils/Config.scala | 2 -- .../blockchain/sync/BlockBroadcastSpec.scala | 14 -------------- .../ethereum/blockchain/sync/TestSyncConfig.scala | 1 - 5 files changed, 2 insertions(+), 26 deletions(-) diff --git a/src/main/resources/application.conf b/src/main/resources/application.conf index bd28bfcd33..805435934f 100644 --- a/src/main/resources/application.conf +++ b/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 diff --git a/src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockBroadcast.scala b/src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockBroadcast.scala index 536684a8a0..0e64d9a787 100644 --- a/src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockBroadcast.scala +++ b/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 } } diff --git a/src/main/scala/io/iohk/ethereum/utils/Config.scala b/src/main/scala/io/iohk/ethereum/utils/Config.scala index 517816eb14..61e9923d6b 100644 --- a/src/main/scala/io/iohk/ethereum/utils/Config.scala +++ b/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"), diff --git a/src/test/scala/io/iohk/ethereum/blockchain/sync/BlockBroadcastSpec.scala b/src/test/scala/io/iohk/ethereum/blockchain/sync/BlockBroadcastSpec.scala index 21e8b72e52..97f1debe3f 100644 --- a/src/test/scala/io/iohk/ethereum/blockchain/sync/BlockBroadcastSpec.scala +++ b/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() diff --git a/src/test/scala/io/iohk/ethereum/blockchain/sync/TestSyncConfig.scala b/src/test/scala/io/iohk/ethereum/blockchain/sync/TestSyncConfig.scala index e774577e0a..e1adad07cd 100644 --- a/src/test/scala/io/iohk/ethereum/blockchain/sync/TestSyncConfig.scala +++ b/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,