-
Notifications
You must be signed in to change notification settings - Fork 2.2k
discovery/sync_manager: restart historical sync on first connected peer #3103
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/sync_manager: restart historical sync on first connected peer #3103
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative approach would be to modify the historical sync interval in the integration tests, though I think this is still useful given the following scenario: if you've lost all your peers and start receiving new updates at tip, you won't be able to process some of these channels if you've yet to see their channel announcement, so performing a historical sync before doing so would make sense.
LGTM 📦
discovery/sync_manager.go
Outdated
case !initialHistoricalSyncCompleted: | ||
s.setSyncType(PassiveSync) | ||
m.inactiveSyncers[s.cfg.peerPub] = s | ||
|
||
// Otherwise, it should be initialized as active. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: seems like a redundant comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed: 9c9ac36
// TestSyncManagerHistoricalSyncOnReconnect tests that the sync manager will | ||
// re-trigger a historical sync when a new peer connects after a historical | ||
// sync has completed, but we have lost all peers. | ||
func TestSyncManagerHistoricalSyncOnReconnect(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: would prefer if this commit was squashed with what introduced the behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer splitting a bugfix and the test reproducing the bug in separate commits. Especially when I'm reviewing this makes it easy to cherry-pick the test commit on master to exercise the failing scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure but that wasn't the case here. The non-test changes alone didn't have an impact on the tests 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after squash! ✅
538b403
to
9c9ac36
Compare
To handle the case where we have been without peers, and get a new connection, we reset the historical scan booleans when the first active syncer is connected to trigger another historical sync.
…nect TestSyncManagerHistoricalSyncOnReconnect tests that the sync manager will re-trigger a historical sync when a new peer connects after a historical sync has completed, but we have lost all peers.
9c9ac36
to
6ba6982
Compare
To handle the case where we have been without peers, and get a new
connection, we reset the historical scan booleans when the first active
syncer is connected to trigger another historical sync.
This would cause integration tests to be flaky (especially with Neutrino), since we would risk not getting graph updates after having being disconnected.