[ECTM-104] Pivot block selection algorithm #711

mmrozek · 2020-09-30T12:09:05Z

Description

Add mechanism of voting (based on different clients implementations) to choose pivot block (renamed from target block to consistency)

Proposed Solution

Algorithm:

Find the highest block number (best block from all peers)
Define expected pivot block - bestBlock - pivotBlockOffset
Ask minPeersToChoosePivotBlock + peersToChoosePivotBlockMargin peers with the best block >= expected pivot block for expected pivot block header
Wait for all response and check if minPeersToChoosePivotBlock peers have the same header
If not ask the next peer (if available)
If there are no more peers restart the whole process after timeout

KonradStaniec · 2020-10-05T08:09:40Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSyncPivotBlockSelector.scala

+      case MessageFromPeer(blockHeaders: BlockHeaders, peerId) =>
+        peerEventBus ! Unsubscribe(MessageClassifier(Set(BlockHeaders.code), PeerSelector.WithId(peerId)))
+        val updatedPeersToAsk = peersToAsk - peerId
+        val targetBlockHeaderOpt = blockHeaders.headers.find(header => header.number == targetBlockNumber)


maybe we should validate that we received exactly one header with required number ?

KonradStaniec · 2020-10-05T08:10:58Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSyncPivotBlockSelector.scala

+    val peersSortedByBestNumber = peersWithBestBlockNumbers.toList.sortBy(-_._2)
+    val bestPeerBestBlockNumber = peersSortedByBestNumber.head._2
+    val expectedPivotBlock = (bestPeerBestBlockNumber - syncConfig.pivotBlockOffset).max(0)
+    val peersToAsk = peersSortedByBestNumber.takeWhile(_._2 >= expectedPivotBlock).map(_._1)


maybe we should reschedule request for pivot header if there is not enough peers to ask ?

KonradStaniec · 2020-10-05T08:12:07Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSyncPivotBlockSelector.scala

+    }
+  }
+
+  def tryChooseTargetBlock(peersWithBestBlockNumbers: Map[Peer, BigInt]): Unit = {


general not: there is a lot of _._1 and smiilar operators which hurts readability of this function

KonradStaniec · 2020-10-05T08:22:20Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSyncPivotBlockSelector.scala

+      peersToAsk: Set[PeerId],
+      targetBlockNumber: BigInt,
+      timeout: Cancellable,
+      headers: Map[BlockHeader, Int]


maybe it would be cleanet to keep this data in : Map[ByteString, BlockHeaderWithVotes] where BlockHeaderWithVotes is case class BlockHeaderWithVotes(header: BlockHeader, votes: Int) that way we would able to avoid this _._1 tuple operators and could use header.get(..) instead of find.

KonradStaniec · 2020-10-05T08:25:09Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSyncPivotBlockSelector.scala

+            } else {
+              context become waitingForPivotBlock(updatedPeersToAsk, targetBlockNumber, timeout, updatedHeaders)
+            }
+          case None =>


Just wondering, if one of the peers did not respond or responded badly, maybe we should restart whole process if we won't have enough votes ?

KonradStaniec · 2020-10-05T13:47:39Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSyncPivotBlockSelector.scala

+  ): Receive =
+    handleCommonMessages orElse {
+      case MessageFromPeer(blockHeaders: BlockHeaders, peerId) =>
+        peerEventBus ! Unsubscribe(MessageClassifier(Set(BlockHeaders.code), PeerSelector.WithId(peerId)))


minor: this whole handler is a litte bit to long for my taste mabye we could split it up into smaller functions ?

I tried, but in this way, it is the most readable

KonradStaniec · 2020-10-05T14:01:44Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSyncPivotBlockSelector.scala

+              headers.get(targetBlockHeader.hash).map(_.vote).getOrElse(BlockHeaderWithVotes(targetBlockHeader))
+            val updatedHeaders = headers.updated(targetBlockHeader.hash, newValue)
+            val BlockHeaderWithVotes(mostPopularBlockHeader, updatedVotes) = updatedHeaders.mostVotedHeader
+            if (updatedVotes >= minPeersToChoosePivotBlock) {


If i understand correctly how this works now is that even if we have lets say 7 peers to whom we send request, we finish selection when we receive minPeersToChoosePivotBlock same blocks, and we do not wait for the rest of responses ?
I wonder if this does not make such scenario possible:

We do our selection and receive enough votes for block, but we do not receive header from lets say peer1

We are back to fast sync and start making requests, and we make request for x block headers from peer1, it means we create PeerRequestHandler which subsribes to PeerEventBus for BlockHeaders messages from Peer1 .

peer1 respond with pivot block. PivotBlockSelector do not receive response as it already finished, but PeerRequestHandler from fast sync headers request is still alive and receievs this pivot block

we blacklist peer1 for response which do not match our fast sync request.

I think it is possible. But it is the same case when we assume the timeout - this response could be received later. I could add collecting for all msgs if you prefer, but in a case when we have a lot of peers it won't perform very well

KonradStaniec

code lgtm! Waiting for testing result on mainnet and mordor

mmrozek · 2020-10-07T07:07:54Z

@KonradStaniec I successfully synced with Mordor. Today I will try to run it against the mainnet

mirkoAlic · 2020-10-07T11:37:09Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSync.scala

-          log.info(s"Current target block is fresh enough, starting state download")
-          if (syncState.targetBlock.stateRoot == ByteString(MerklePatriciaTrie.EmptyRootHash)) {
-            syncState = syncState.copy(pendingMptNodes = Seq())
+    private def updatePivotSyncState(state: FinalBlockProcessingResult, targetBlockHeader: BlockHeader): Unit =


(minor) maybe just to safe some space:

private def updatePivotSyncState(state: FinalBlockProcessingResult, targetBlockHeader: BlockHeader): Unit = state match { case ImportedLastBlock if (targetBlockHeader.number - syncState.pivotBlock.number <= syncConfig.maxTargetDifference) => log.info(s"Current target block is fresh enough, starting state download") if (syncState.pivotBlock.stateRoot == ByteString(MerklePatriciaTrie.EmptyRootHash)) { syncState = syncState.copy(pendingMptNodes = Seq()) } else { syncState = syncState.copy(pendingMptNodes = Seq(StateMptNodeHash(syncState.pivotBlock.stateRoot))) } case _ => log.info( s"FinalBlockProcessingResult: $state, changing target to ${targetBlockHeader.number}, new safe target is ${syncState.safeDownloadTarget}" ) syncState = syncState.updateTargetBlock(targetBlockHeader, syncConfig.fastSyncBlockValidationX, updateFailures = true) }

mirkoAlic · 2020-10-07T11:59:17Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSync.scala

-      safeDownloadTarget = newTarget.number + numberOfSafeBlocks,
-      targetBlockUpdateFailures = if (updateFailures) targetBlockUpdateFailures + 1 else targetBlockUpdateFailures
-    )
+    def updateTargetBlock(newTarget: BlockHeader, numberOfSafeBlocks: BigInt, updateFailures: Boolean): SyncState =


updatePivotBlock(newPivot: ..?

mirkoAlic · 2020-10-07T12:00:03Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSync.scala

@@ -328,8 +360,8 @@ class FastSync(
            handleRewind(header, peer, syncConfig.fastSyncBlockValidationN)
          case HeadersProcessingFinished =>
            processSyncing()
-          case ImportedTargetBlock  =>
-            updateTargetBlock(ImportedLastBlock)
+          case ImportedTargetBlock =>


ImportedPivotBlock ?

kapke · 2020-10-07T12:43:31Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSync.scala

-      val targetBlockSelector = context.actorOf(FastSyncTargetBlockSelector.props(etcPeerManager, peerEventBus, syncConfig, scheduler), "target-block-selector")
-      targetBlockSelector ! FastSyncTargetBlockSelector.ChooseTargetBlock
-      context become syncingHandler.waitingForTargetBlockUpdate(ImportedLastBlock)
+      val pivotBlockSelector = context.actorOf(


Does it make sense maybe to create PivotBlockSelector actor once and ask him for pivot block whenever there's a need for that?

I am not sure. I don't want to change that in this PR. Will see after fastsync refactoring what solution will be the best

kapke · 2020-10-07T12:46:50Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSyncPivotBlockSelector.scala

+    val scheduler: Scheduler
+) extends Actor
+    with ActorLogging
+    with PeerListSupport


PeersClient instead?

Why? We don't need PeersClient functionality here

kapke · 2020-10-07T12:50:31Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSyncPivotBlockSelector.scala

+  import FastSyncPivotBlockSelector._
+  import syncConfig._
+
+  val fastSync: ActorRef = context.parent


minor: could/should fastSync actor be passed through constructor? Or sender() used?

kapke · 2020-10-07T12:56:55Z

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

+import scala.concurrent.Await
+import scala.concurrent.duration._
+
+class FastSyncPivotBlockSelectorSpec extends AnyFlatSpec with Matchers with BeforeAndAfter {


WithActorSystemShutdown?

kapke · 2020-10-07T12:59:06Z

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

+    val peer3TestProbe: TestProbe = TestProbe("peer3")(system)
+    val peer4TestProbe: TestProbe = TestProbe("peer4")(system)
+
+    val peer1 = Peer(new InetSocketAddress("127.0.0.1", 0), peer1TestProbe.ref, false)


shoduln't these peers be extracted into separate file?

mirkoAlic · 2020-10-07T13:22:32Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSyncPivotBlockSelector.scala

+import scala.concurrent.ExecutionContext.Implicits.global
+import scala.concurrent.duration.FiniteDuration
+
+class FastSyncPivotBlockSelector(


The code of this actor seems ok, but IMHO, we could reflect the Actor behaviour in more expressive way, i will leave some comments regarding that.

mirkoAlic · 2020-10-07T13:23:22Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSyncPivotBlockSelector.scala

+
+  override def receive: Receive = idle
+
+  def idle: Receive = handleCommonMessages orElse { case ChoosePivotBlock =>


why not ChoosePivotBlock -> StartPivotBlockElection ?

Because from parent (fast sync) perspective we wnt to choose a pivot block. It doesn't matter if we use an election for that or sth else

fair enough, then just a minor one: ChoosePivotBlock to SelectPivotBlock ? i mean the actor is called FastSyncPivotBlockSelector...also, it does matter if is fastSync related? or it could be PivotBlockSelector ?

Btw, regaring using the name Start...Election, take into consideration that programming with actors, behind the scenes, is defining a protocol, so, not sure if not express the way the protocol behaves is best than explicitly mention it. I mean if this was a trait instead of an Actor, i would be agree with you regarding not mention the actual implementation.

mirkoAlic · 2020-10-07T13:23:43Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSyncPivotBlockSelector.scala

+    }
+  }
+
+  def waitingForPivotBlock(


Why not runningPivotBlockElection ?

Possible scenarios:

VoteReceived aka MessageFromPeer ...

ElectionExpired aka PivotBlockTimeout

mirkoAlic · 2020-10-07T13:28:30Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSyncPivotBlockSelector.scala

+        scheduleRetry(startRetryInterval)
+    }
+
+  private def votingProcess(


why not countingVotes ?

From my perspective, at this stage these are states the code should try to reflect better:

ConsensusReached: send response and cleanup
ElectionInProgress: continue runningPivotBlockElection
ConsensusNotReached.NoMoreVoters: schedule a new election startPivotBlockElection
ConsensusNotReached.AddNewVoter: continue runningPivotBlockElection (adds extra time)

I don't see a value of changing votingProcess to countingVotes. For me counting votes is process after the election. But it is not our case

think is a thin line, i mean, you are checking or counting votes and defining what to do. But yeah, using the word process is valid too.

mirkoAlic · 2020-10-07T13:32:14Z

src/main/scala/io/iohk/ethereum/blockchain/sync/FastSyncPivotBlockSelector.scala

+  override def receive: Receive = idle
+
+  def idle: Receive = handleCommonMessages orElse { case ChoosePivotBlock =>
+    val peersUsedToChooseTarget = peersToDownloadFrom.collect { case (peer, PeerInfo(_, _, true, maxBlockNumber, _)) =>


the code till the if could be an auxiliar function like: getVoters and given the data that returns we express which path we should take (re-schedule or running the election)

mirkoAlic · 2020-10-07T15:09:03Z

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

+    with BeforeAndAfter
+    with WithActorSystemShutDown {
+
+  "FastSyncPivotBlockSelector" should "download pivot block from peers" in new TestSetup {


(minor) it has the old name

mirkoAlic

LGTM!

mmrozek requested a review from KonradStaniec September 30, 2020 12:09

mmrozek force-pushed the ectm-104-target-block-selection branch 4 times, most recently from cc65b2c to 955765e Compare October 2, 2020 12:25

mmrozek requested review from ntallar and mirkoAlic October 2, 2020 12:38

mmrozek force-pushed the ectm-104-target-block-selection branch from 955765e to 8a0d99f Compare October 2, 2020 12:40

mmrozek marked this pull request as ready for review October 2, 2020 12:40

KonradStaniec reviewed Oct 5, 2020

View reviewed changes

mmrozek force-pushed the ectm-104-target-block-selection branch from 6a31724 to cd35418 Compare October 5, 2020 12:21

KonradStaniec reviewed Oct 5, 2020

View reviewed changes

mmrozek force-pushed the ectm-104-target-block-selection branch from cd35418 to 797b12b Compare October 6, 2020 10:38

mmrozek changed the title ~~[ECTM-104] Target block selection algorithm~~ [ECTM-104] Pivot block selection algorithm Oct 6, 2020

KonradStaniec reviewed Oct 7, 2020

View reviewed changes

mirkoAlic reviewed Oct 7, 2020

View reviewed changes

kapke reviewed Oct 7, 2020

View reviewed changes

mirkoAlic reviewed Oct 7, 2020

View reviewed changes

Michal Mrozek added 3 commits October 7, 2020 15:56

[ECTM-104] Pivot block selection algorithm

b86e546

[ECTM-104] Refactoring names

e192874

[ECTM-104] PR fixes

8932dc1

mmrozek force-pushed the ectm-104-target-block-selection branch from 7dd239f to 8932dc1 Compare October 7, 2020 13:56

mirkoAlic reviewed Oct 7, 2020

View reviewed changes

mirkoAlic approved these changes Oct 7, 2020

View reviewed changes

KonradStaniec approved these changes Oct 8, 2020

View reviewed changes

Merge branch 'develop' into ectm-104-target-block-selection

ee6fd9c

mmrozek merged commit 6e30ab6 into develop Oct 8, 2020

mmrozek deleted the ectm-104-target-block-selection branch October 8, 2020 11:48


		override def receive: Receive = idle

		def idle: Receive = handleCommonMessages orElse { case ChoosePivotBlock =>

[ECTM-104] Pivot block selection algorithm #711

[ECTM-104] Pivot block selection algorithm #711

Uh oh!

Conversation

mmrozek commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Proposed Solution

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KonradStaniec left a comment

Choose a reason for hiding this comment

Uh oh!

mmrozek commented Oct 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mirkoAlic Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mirkoAlic Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mirkoAlic left a comment

Choose a reason for hiding this comment

mmrozek commented Sep 30, 2020 •

edited

Loading

mirkoAlic Oct 7, 2020 •

edited

Loading

mirkoAlic Oct 7, 2020 •

edited

Loading