Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Range sync w/ dynamic target #2464

Merged
merged 1 commit into from May 3, 2021
Merged

Conversation

dapplion
Copy link
Contributor

@dapplion dapplion commented May 2, 2021

Implements Lighthouse's range sync which features:

  • Able to go from synced to syncing dynamically depending on current connected peer status
  • Merges the initial sync + regular sync code.

Differs from #2111 in that SyncChain instances have dynamic targets instead of static. This solves the critical issues in #2111.

  • In Lodestar there is much higher rotation of peers than in Lighthouse so having a static target causes too much SyncChain changes
  • Changing SyncChains has a high memory cost due to the pre-fetched block ranges.
  • This PR uses a single SyncChain for finalized sync and a single SyncChain for head sync. This eliminates such issues. This approach is essentially what we have right now in master.
  • The downside of dynamic targets is reduced capacity to track and penalize peers in attack situations. However I think this trade-off is well worth-it given that our current sync stalls the node into a non-recoverable position (see Node gets stuck in Prater - critical #2461). We can revisit using static targets when we have a very stable node, which will allow to have a very low peer rotation.

Closes #2461 Closes #2460

@dapplion dapplion changed the title Range sync dynamic target Range sync w/ dynamic target May 2, 2021
@github-actions github-actions bot added Api scope-networking All issues related to networking, gossip, and libp2p. labels May 2, 2021
@codeclimate
Copy link

codeclimate bot commented May 2, 2021

Code Climate has analyzed commit 9c072ba and detected 9 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 9

View more on Code Climate.

Add peer reporting to sync chain

Move sync progress logger to the node

Remove chainMaybeStuck timeout

Handle Range Sync errors and return better

Fix Sync Range state logic

Make node notifier logs consistent

Throw error in Batch methods instead of in factory

Narrow error capturing in SyncChain.processBatch

Update RangeSync comments

Propagate info if peer has been removed from SyncChain

Sync subscribe to RangeSync to update sync state

Log SyncChain id to identify it

Fix bug in SyncChain downloading an extra batch on finalized sync

Fix block ordering in getUnfinalizedBlocksAtSlots

Re-status peers after completing a SyncChain

RangeSync remove peers from Head sync chains only

Reduce scope of Sync instance's properties

Add peersPerSyncChain metrics

Stop gossip handlers if sync falls behind

Accept considering Synced even if headSlot = 0

RangeSync should start and stop chains if necessary only

Add lodestar_peers_per_sync_chain metrics view

Add lodestar_process_chain_segment_time metric

Add lodestar API for getSyncChainsDebugState

Fix SyncChain priorization

Fix SyncChain startSyncing promise handling

Remove unused subscribeCoreTopics fn

Test sync range updateChains logic

Clean-up instance modules and constructor args

Don't store status in peer metastore

Polish module organization

Don't use AbortController unnecessarily

Add and fix comments

- Remove IPeerManager interface
- Merge Sync start into constructor
- Declare Sync.close() in interface
- Declare const CHECK_PING_STATUS_INTERVAL

Revert onUnknownBlockRoot changes

If a node witness the genesis event consider starting gossip

emit internal chain event clockEpoch

Make SyncChain target dynamic

Add isSingleNode option

Simplify remoteSyncType modules

Fine tune sync state logic


Add syncStatus metric
@dapplion dapplion force-pushed the dapplion/range-sync-dynamic-target branch from f723113 to 9c072ba Compare May 3, 2021 11:41
@wemeetagain wemeetagain merged commit c2123fa into master May 3, 2021
@wemeetagain wemeetagain deleted the dapplion/range-sync-dynamic-target branch May 3, 2021 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope-networking All issues related to networking, gossip, and libp2p.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node gets stuck in Prater - critical Add metric for node's sync status
2 participants