From 1e381fd5f4e8b95afc5c030c45fcc6b47db38c4c Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 28 Feb 2022 14:58:18 +0530 Subject: [PATCH 1/3] Loop once in computeMostCommonTarget --- .../src/sync/range/utils/chainTarget.ts | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/packages/lodestar/src/sync/range/utils/chainTarget.ts b/packages/lodestar/src/sync/range/utils/chainTarget.ts index 13fc47d49f66..9869c7834c8d 100644 --- a/packages/lodestar/src/sync/range/utils/chainTarget.ts +++ b/packages/lodestar/src/sync/range/utils/chainTarget.ts @@ -11,21 +11,24 @@ export type ChainTarget = { }; export function computeMostCommonTarget(targets: ChainTarget[]): ChainTarget | null { - const targetsById = new Map(); + if (targets.length === 0) { + return null; + } + const countById = new Map(); + 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; } From 8aaa10b4a9858c955a8f54938739c1d4f1f75968 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 28 Feb 2022 15:06:49 +0530 Subject: [PATCH 2/3] Don't null SyncChain target --- packages/lodestar/src/sync/range/chain.ts | 23 ++++++++++++------- packages/lodestar/src/sync/range/range.ts | 3 ++- .../src/sync/range/utils/chainTarget.ts | 4 ++-- .../test/unit/sync/range/chain.test.ts | 2 ++ 4 files changed, 21 insertions(+), 11 deletions(-) diff --git a/packages/lodestar/src/sync/range/chain.ts b/packages/lodestar/src/sync/range/chain.ts index d9b1e2b8af4a..9a27cffc3d6c 100644 --- a/packages/lodestar/src/sync/range/chain.ts +++ b/packages/lodestar/src/sync/range/chain.ts @@ -86,8 +86,11 @@ export class SyncChain { /** Short string id to identify this SyncChain in logs */ readonly logId: string; readonly syncType: RangeSyncType; - /** Should sync up until this slot, then stop */ - target: ChainTarget | null = null; + /** + * 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; /** Number of validated epochs. For the SyncRange to prevent switching chains too fast */ validatedEpochs = 0; @@ -111,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; @@ -224,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, @@ -235,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); + } } /** @@ -256,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; } @@ -364,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; } diff --git a/packages/lodestar/src/sync/range/range.ts b/packages/lodestar/src/sync/range/range.ts index 1e9d59e91b99..7b2dd6afa2f5 100644 --- a/packages/lodestar/src/sync/range/range.ts +++ b/packages/lodestar/src/sync/range/range.ts @@ -159,7 +159,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 { @@ -226,6 +226,7 @@ export class RangeSync extends (EventEmitter as {new (): RangeSyncEmitter}) { if (!syncChain) { syncChain = new SyncChain( startEpoch, + target, syncType, { processChainSegment: this.processChainSegment, diff --git a/packages/lodestar/src/sync/range/utils/chainTarget.ts b/packages/lodestar/src/sync/range/utils/chainTarget.ts index 9869c7834c8d..9cb9c88d5787 100644 --- a/packages/lodestar/src/sync/range/utils/chainTarget.ts +++ b/packages/lodestar/src/sync/range/utils/chainTarget.ts @@ -10,9 +10,9 @@ export type ChainTarget = { root: Root; }; -export function computeMostCommonTarget(targets: ChainTarget[]): ChainTarget | null { +export function computeMostCommonTarget(targets: ChainTarget[]): ChainTarget { if (targets.length === 0) { - return null; + throw Error("Must provide at least one target"); } const countById = new Map(); diff --git a/packages/lodestar/test/unit/sync/range/chain.test.ts b/packages/lodestar/test/unit/sync/range/chain.test.ts index 6644ec779a1d..d256b77aad56 100644 --- a/packages/lodestar/test/unit/sync/range/chain.test.ts +++ b/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} From ea0a363341baad202e1fb04cb34d02678b589d4b Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 28 Feb 2022 15:10:29 +0530 Subject: [PATCH 3/3] Inline shouldRemoveChain --- packages/lodestar/src/sync/range/range.ts | 13 +++++++++++-- .../lodestar/src/sync/range/utils/index.ts | 1 - .../src/sync/range/utils/shouldRemoveChain.ts | 18 ------------------ 3 files changed, 11 insertions(+), 21 deletions(-) delete mode 100644 packages/lodestar/src/sync/range/utils/shouldRemoveChain.ts diff --git a/packages/lodestar/src/sync/range/range.ts b/packages/lodestar/src/sync/range/range.ts index 7b2dd6afa2f5..152bed30861e 100644 --- a/packages/lodestar/src/sync/range/range.ts +++ b/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} 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"; @@ -249,7 +249,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}); diff --git a/packages/lodestar/src/sync/range/utils/index.ts b/packages/lodestar/src/sync/range/utils/index.ts index a1f514e86a3e..210bf9c0224b 100644 --- a/packages/lodestar/src/sync/range/utils/index.ts +++ b/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"; diff --git a/packages/lodestar/src/sync/range/utils/shouldRemoveChain.ts b/packages/lodestar/src/sync/range/utils/shouldRemoveChain.ts deleted file mode 100644 index 78d80608e558..000000000000 --- a/packages/lodestar/src/sync/range/utils/shouldRemoveChain.ts +++ /dev/null @@ -1,18 +0,0 @@ -import {Slot} from "@chainsafe/lodestar-types"; -import {IBeaconChain} from "../../../chain"; -import {SyncChain} from "../chain"; - -/** - * Checks if a Finalized or Head chain should be removed - */ -export function shouldRemoveChain(syncChain: SyncChain, localFinalizedSlot: Slot, chain: IBeaconChain): boolean { - return ( - // 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 !== null && - (syncChain.target.slot < localFinalizedSlot || chain.forkChoice.hasBlock(syncChain.target.root))) - ); -}