Remove SyncState from SyncTargetManager #1188
Conversation
| @@ -111,6 +111,8 @@ public void start() { | |||
| public TrailingPeerRequirements calculateTrailingPeerRequirements() { | |||
| return syncState.isInSync() | |||
| ? TrailingPeerRequirements.UNRESTRICTED | |||
| : new TrailingPeerRequirements(syncState.chainHeadNumber(), config.getMaxTrailingPeers()); | |||
| : new TrailingPeerRequirements( | |||
| protocolContext.getBlockchain().getChainHeadBlockNumber(), | |||
There was a problem hiding this comment.
It looks like we're loosening the requirements for trailing peers during full sync? It does look like we were being too strict ...
There was a problem hiding this comment.
Not this just removes the indirection when getting the current blockchain head number - previously it went through SyncState which then ran protocolContext.getBlockchain().getChainHeadBlockNumber() so it's just been inlined.
The trailing peer limit during a full sync is set up so that a trailing peer is anyone behind us and at most 25% of our peers are allowed to be trailing. The main aim is to be connected to as many peers who can give us the data we need to sync as possible as few peers who will be asking us for data (which slows us down).
PR description
Removes the dependency on
SyncStatefromSyncTargetManager. Every approach I've tried to fix https://pegasys1.atlassian.net/browse/PAN-1057 winds up with this change included and it makesSyncTargetManagermuch purer and simpler to understand. Reducing shared state generally seems like a good idea. There is some extra complexity added inChainDownloaderbut this still feels like a step in the right direction.