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

discovery: SyncManager improvements #2932

Merged
merged 7 commits into from Apr 26, 2019

Conversation

Projects
None yet
4 participants
@wpaulino
Copy link
Collaborator

commented Apr 11, 2019

In this PR, we make some improvements to the recently introduced SyncManager. These improvements include:

  • Active syncers will no longer attempt to synchronize our graph with remote peers. Instead, we'll now rely on synchronizing our graph with remote peers through the routine historical syncs performed by the SyncManager. Since active syncers will no longer attempt this synchronization, the SyncManager's round-robin is no longer needed.

  • Handling initial historical sync disconnections: Every time lnd starts up, it attempts an initial historical graph sync with the first peer that connects. If the peer ends up disconnecting, then we wouldn't handle finding a replacement to continue performing the initial historical sync.

  • Queueing active syncers until the initial historical sync completes: We do this to ensure we can properly handle any new channel updates at tip. This is required for fresh nodes that are syncing the channel graph for the first time. If we begin accepting updates at tip while the initial historical sync is still ongoing, then we risk not processing certain updates since we've yet to learn of the channels themselves.

NOTE: To test, start up a fresh mainnet node and attempt to sync the graph. Bumping up DISC logs to debug will serve useful. You should not see any of your peers in an ActiveSync state until the initial historical sync has completed. You can also disconnect the peer we're performing the initial historical sync with to ensure we find a replacement (assuming you have other peers connected).

@wpaulino wpaulino changed the title discovery: sync manager improvements discovery: SyncManager improvements Apr 11, 2019

@wpaulino wpaulino force-pushed the wpaulino:sync-manager-improvements branch 2 times, most recently from 49898b9 to 227ea1f Apr 11, 2019

Show resolved Hide resolved discovery/syncer.go
Show resolved Hide resolved discovery/sync_manager.go
Show resolved Hide resolved discovery/sync_manager.go

@wpaulino wpaulino force-pushed the wpaulino:sync-manager-improvements branch 2 times, most recently from ea281d4 to f809537 Apr 11, 2019

@cfromknecht cfromknecht added this to the 0.6.1 milestone Apr 12, 2019

@wpaulino wpaulino force-pushed the wpaulino:sync-manager-improvements branch from f809537 to d6ba95c Apr 17, 2019

@wpaulino wpaulino force-pushed the wpaulino:sync-manager-improvements branch 7 times, most recently from 38a1753 to 4c87533 Apr 20, 2019

Show resolved Hide resolved discovery/syncer.go Outdated
Show resolved Hide resolved discovery/sync_manager.go
Show resolved Hide resolved discovery/sync_manager.go Outdated
Show resolved Hide resolved discovery/sync_manager.go
Show resolved Hide resolved discovery/sync_manager.go Outdated
Show resolved Hide resolved discovery/sync_manager.go Outdated
Show resolved Hide resolved routing/router.go Outdated
@halseth
Copy link
Collaborator

left a comment

Looks more or less good to me now :)

wpaulino added some commits Apr 11, 2019

discovery: remove channel synchronization from ActiveSync GossipSyncers
In this commit, we remove the ability for ActiveSync GossipSyncers to
synchronize our graph with our remote peers. This serves as a starting
point towards allowing the daemon to only synchronize our graph through
historical syncs, which will be routinely done by the SyncManager.
discovery+server: remove roundRobinHandler and related code
Since ActiveSync GossipSyncers no longer synchronize our state with the
remote peers, none of the logic surrounding the round-robin is required
within the SyncManager.
discovery: synchronize new/stale GossipSyncers with syncerHandler
Now that the roundRobinHandler is no longer present, this commit aims to
clean up and simplify some of the logic surrounding initializing/tearing
down new/stale GossipSyncers from the SyncManager. Along the way, we
also synchronize these calls with the syncerHandler, which will serve
useful in future work that allows us to recovery from initial historical
sync disconnections.
discovery: make historicalSync transition synchronous
We do this to ensure that the state transition from chansSynced to
syncingChans has occurred by the time we return back to the caller.
discovery: handle initial historical sync disconnection
In this commit, we add logic to handle a peer with whom we're performing
an initial historical sync disconnecting. This is required to ensure we
get as much of the graph as possible when starting a fresh node. It will
also serve useful to ensure we do not get stalled once we prevent active
GossipSyncers from starting until the initial historical sync has
completed.
discovery: queue active syncers until initial historical sync signal
In this commit, we begin to queue any active syncers until the initial
historical sync has completed. We do this to ensure we can properly
handle any new channel updates at tip. This is required for fresh nodes
that are syncing the channel graph for the first time. If we begin
accepting updates at tip while the initial historical sync is still
ongoing, then we risk not processing certain updates since we've yet to
learn of the channels themselves.

@wpaulino wpaulino force-pushed the wpaulino:sync-manager-improvements branch from 4c87533 to d68842e Apr 24, 2019

@cfromknecht
Copy link
Collaborator

left a comment

Yay code deletion! LGTM 🦐

@Roasbeef Roasbeef merged commit a783f35 into lightningnetwork:master Apr 26, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls First build on sync-manager-improvements at 59.882%
Details

@wpaulino wpaulino deleted the wpaulino:sync-manager-improvements branch Apr 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.