Minor range sync tweaks

packages/lodestar/src/sync/range/chain.ts

@@ -90,7 +90,7 @@ export class SyncChain {
    * Should sync up until this slot, then stop.
    * Finalized SyncChains have a dynamic target, so if this chain has no peers the target can become null
    */
-  target: ChainTarget | null = null;
+  target: ChainTarget;
 
   /** Number of validated epochs. For the SyncRange to prevent switching chains too fast */
   validatedEpochs = 0;
@@ -114,12 +114,14 @@ export class SyncChain {
 
   constructor(
     startEpoch: Epoch,
+    initialTarget: ChainTarget,
     syncType: RangeSyncType,
     fns: SyncChainFns,
     modules: SyncChainModules,
     opts?: SyncChainOpts
   ) {
     this.startEpoch = startEpoch;
+    this.target = initialTarget;
     this.syncType = syncType;
     this.processChainSegment = fns.processChainSegment;
     this.downloadBeaconBlocksByRange = fns.downloadBeaconBlocksByRange;
@@ -227,8 +229,8 @@ export class SyncChain {
   /** Full debug state for lodestar API */
   getDebugState(): SyncChainDebugState {
     return {
-      targetRoot: this.target && toHexString(this.target.root),
-      targetSlot: this.target && this.target.slot,
+      targetRoot: toHexString(this.target.root),
+      targetSlot: this.target.slot,
       syncType: this.syncType,
       status: this.status,
       startEpoch: this.startEpoch,
@@ -238,8 +240,10 @@ export class SyncChain {
   }
 
   private computeTarget(): void {
-    const targets = this.peerset.values();
-    this.target = computeMostCommonTarget(targets);
+    if (this.peerset.size > 0) {
+      const targets = this.peerset.values();
+      this.target = computeMostCommonTarget(targets);
+    }
   }
 
   /**
@@ -259,7 +263,7 @@ export class SyncChain {
 
         // If startEpoch of the next batch to be processed > targetEpoch -> Done
         const toBeProcessedEpoch = toBeProcessedStartEpoch(toArr(this.batches), this.startEpoch, this.opts);
-        if (this.target && computeStartSlotAtEpoch(toBeProcessedEpoch) >= this.target.slot) {
+        if (computeStartSlotAtEpoch(toBeProcessedEpoch) >= this.target.slot) {
           break;
         }
 
@@ -367,7 +371,7 @@ export class SyncChain {
     const toBeDownloadedSlot = computeStartSlotAtEpoch(startEpoch) + BATCH_SLOT_OFFSET;
 
     // Don't request batches beyond the target head slot
-    if (this.target && toBeDownloadedSlot > this.target.slot) {
+    if (toBeDownloadedSlot > this.target.slot) {
       return null;
     }
 

packages/lodestar/src/sync/range/range.ts

@@ -9,7 +9,7 @@ import {IBeaconChain} from "../../chain";
 import {INetwork} from "../../network";
 import {IMetrics} from "../../metrics";
 import {RangeSyncType, getRangeSyncType, rangeSyncTypes} from "../utils/remoteSyncType";
-import {updateChains, shouldRemoveChain} from "./utils";
+import {updateChains} from "./utils";
 import {ChainTarget, SyncChainFns, SyncChain, SyncChainOpts, SyncChainDebugState} from "./chain";
 import {PartiallyVerifiedBlockFlags} from "../../chain/blocks";
 
@@ -164,7 +164,7 @@ export class RangeSync extends (EventEmitter as {new (): RangeSyncEmitter}) {
   get state(): RangeSyncState {
     const syncingHeadTargets: ChainTarget[] = [];
     for (const chain of this.chains.values()) {
-      if (chain.isSyncing && chain.target) {
+      if (chain.isSyncing) {
         if (chain.syncType === RangeSyncType.Finalized) {
           return {status: RangeSyncStatus.Finalized, target: chain.target};
         } else {
@@ -236,6 +236,7 @@ export class RangeSync extends (EventEmitter as {new (): RangeSyncEmitter}) {
     if (!syncChain) {
       syncChain = new SyncChain(
         startEpoch,
+        target,
         syncType,
         {
           processChainSegment: this.processChainSegment,
@@ -259,7 +260,16 @@ export class RangeSync extends (EventEmitter as {new (): RangeSyncEmitter}) {
 
     // Remove chains that are out-dated, peer-empty, completed or failed
     for (const [id, syncChain] of this.chains.entries()) {
-      if (shouldRemoveChain(syncChain, localFinalizedSlot, this.chain)) {
+      // Checks if a Finalized or Head chain should be removed
+      if (
+        // Sync chain has completed syncing or encountered an error
+        syncChain.isRemovable ||
+        // Sync chain has no more peers to download from
+        syncChain.peers === 0 ||
+        // Outdated: our chain has progressed beyond this sync chain
+        syncChain.target.slot < localFinalizedSlot ||
+        this.chain.forkChoice.hasBlock(syncChain.target.root)
+      ) {
         syncChain.remove();
         this.chains.delete(id);
         this.logger.debug("Removed syncChain", {id: syncChain.logId});

packages/lodestar/src/sync/range/utils/chainTarget.ts

@@ -10,22 +10,25 @@ export type ChainTarget = {
   root: Root;
 };
 
-export function computeMostCommonTarget(targets: ChainTarget[]): ChainTarget | null {
-  const targetsById = new Map<string, ChainTarget>();
+export function computeMostCommonTarget(targets: ChainTarget[]): ChainTarget {
+  if (targets.length === 0) {
+    throw Error("Must provide at least one target");
+  }
+
   const countById = new Map<string, number>();
 
+  let mostCommonTarget = targets[0];
+  let mostCommonCount = 0;
+
   for (const target of targets) {
     const targetId = `${target.slot}-${toHexString(target.root)}`;
-    targetsById.set(targetId, target);
-    countById.set(targetId, 1 + (countById.get(targetId) ?? 0));
-  }
-
-  let mostCommon: {count: number; targetId: string} | null = null;
-  for (const [targetId, count] of countById.entries()) {
-    if (!mostCommon || count > mostCommon.count) {
-      mostCommon = {count, targetId};
+    const count = 1 + (countById.get(targetId) ?? 0);
+    countById.set(targetId, count);
+    if (count > mostCommonCount) {
+      mostCommonCount = count;
+      mostCommonTarget = target;
     }
   }
 
-  return mostCommon && (targetsById.get(mostCommon.targetId) ?? null);
+  return mostCommonTarget;
 }

packages/lodestar/src/sync/range/utils/index.ts

@@ -2,5 +2,4 @@ export * from "./batches";
 export * from "./chainTarget";
 export * from "./hashBlocks";
 export * from "./peerBalancer";
-export * from "./shouldRemoveChain";
 export * from "./updateChains";

packages/lodestar/test/unit/sync/range/chain.test.ts

@@ -95,6 +95,7 @@ describe("sync / range / chain", () => {
         const onEnd: SyncChainFns["onEnd"] = (err) => (err ? reject(err) : resolve());
         const initialSync = new SyncChain(
           startEpoch,
+          target,
           syncType,
           {processChainSegment, downloadBeaconBlocksByRange, reportPeer, onEnd},
           {config, logger}
@@ -126,6 +127,7 @@ describe("sync / range / chain", () => {
       const onEnd: SyncChainFns["onEnd"] = (err) => (err ? reject(err) : resolve());
       const initialSync = new SyncChain(
         startEpoch,
+        target,
         syncType,
         {processChainSegment, downloadBeaconBlocksByRange, reportPeer, onEnd},
         {config, logger}