Synchroniser to wait for new peer if best is up to date (#1161)

When chain downloading, the SyncTargetManager continues to
attempt to download headers/blocks from the current syncTarget,
even if the system is "insync". This is due to the fact that
the "insync" check is only performed at initial peer selection,
rather than on each loop of the chain downloader.

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/ChainDownloader.java

@@ -165,7 +165,7 @@ private CompletableFuture<Void> checkSyncTarget() {
   }
 
   private boolean finishedSyncingToCurrentTarget() {
-    return syncTargetManager.isSyncTargetDisconnected()
+    return !syncTargetManager.syncTargetCanProvideMoreBlocks()
         || checkpointHeaderManager.checkpointsHaveTimedOut()
         || chainSegmentsHaveTimedOut();
   }

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/SyncTargetManager.java

@@ -34,13 +34,15 @@
 public abstract class SyncTargetManager<C> {
 
   private static final Logger LOG = LogManager.getLogger();
+
+  protected final SyncState syncState;
+
   private volatile long syncTargetDisconnectListenerId;
   private volatile boolean syncTargetDisconnected = false;
   private final SynchronizerConfiguration config;
   private final ProtocolSchedule<C> protocolSchedule;
   private final ProtocolContext<C> protocolContext;
   private final EthContext ethContext;
-  private final SyncState syncState;
   private final MetricsSystem metricsSystem;
 
   public SyncTargetManager(
@@ -144,4 +146,15 @@ public void clearSyncTarget(final SyncTarget syncTarget) {
   public abstract boolean shouldSwitchSyncTarget(final SyncTarget currentTarget);
 
   public abstract boolean shouldContinueDownloading();
+
+  public abstract boolean isSyncTargetReached(final EthPeer peer);
+
+  public boolean syncTargetCanProvideMoreBlocks() {
+    if (!syncState.syncTarget().isPresent()) {
+      LOG.warn("SyncTarget should be set, but is not.");
+      return false;
+    }
+    final EthPeer currentSyncingPeer = syncState.syncTarget().get().peer();
+    return !isSyncTargetDisconnected() && !isSyncTargetReached(currentSyncingPeer);
+  }
 }

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fastsync/FastSyncTargetManager.java

@@ -118,4 +118,9 @@ public boolean shouldSwitchSyncTarget(final SyncTarget currentTarget) {
   public boolean shouldContinueDownloading() {
     return !protocolContext.getBlockchain().getChainHeadHash().equals(pivotBlockHeader.getHash());
   }
+
+  @Override
+  public boolean isSyncTargetReached(final EthPeer peer) {
+    return syncState.chainHeadNumber() >= pivotBlockHeader.getNumber();
+  }
 }

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncTargetManager.java

@@ -41,7 +41,6 @@ class FullSyncTargetManager<C> extends SyncTargetManager<C> {
   private final SynchronizerConfiguration config;
   private final ProtocolContext<C> protocolContext;
   private final EthContext ethContext;
-  private final SyncState syncState;
 
   FullSyncTargetManager(
       final SynchronizerConfiguration config,
@@ -54,7 +53,6 @@ class FullSyncTargetManager<C> extends SyncTargetManager<C> {
     this.config = config;
     this.protocolContext = protocolContext;
     this.ethContext = ethContext;
-    this.syncState = syncState;
   }
 
   @Override
@@ -82,10 +80,7 @@ protected CompletableFuture<Optional<EthPeer>> selectBestAvailableSyncTarget() {
       return completedFuture(Optional.empty());
     } else {
       final EthPeer bestPeer = maybeBestPeer.get();
-      final long peerHeight = bestPeer.chainState().getEstimatedHeight();
-      final UInt256 peerTd = bestPeer.chainState().getBestBlock().getTotalDifficulty();
-      if (peerTd.compareTo(syncState.chainHeadTotalDifficulty()) <= 0
-          && peerHeight <= syncState.chainHeadNumber()) {
+      if (isSyncTargetReached(bestPeer)) {
         // We're caught up to our best peer, try again when a new peer connects
         LOG.debug("Caught up to best peer: " + bestPeer.chainState().getEstimatedHeight());
         return completedFuture(Optional.empty());
@@ -94,6 +89,15 @@ protected CompletableFuture<Optional<EthPeer>> selectBestAvailableSyncTarget() {
     }
   }
 
+  @Override
+  public boolean isSyncTargetReached(final EthPeer peer) {
+    final long peerHeight = peer.chainState().getEstimatedHeight();
+    final UInt256 peerTd = peer.chainState().getBestBlock().getTotalDifficulty();
+
+    return peerTd.compareTo(syncState.chainHeadTotalDifficulty()) <= 0
+        && peerHeight <= syncState.chainHeadNumber();
+  }
+
   @Override
   public boolean shouldSwitchSyncTarget(final SyncTarget currentTarget) {
     final EthPeer currentPeer = currentTarget.peer();

ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncDownloaderTest.java

@@ -230,8 +230,8 @@ public void syncsToBetterChain_fromFork() {
     peer.respondWhileOtherThreadsWork(
         responder, () -> localBlockchain.getChainHeadBlockNumber() < targetBlock);
 
-    assertThat(syncState.syncTarget()).isPresent();
-    assertThat(syncState.syncTarget().get().peer()).isEqualTo(peer.getEthPeer());
+    // Synctarget should not exist as chain has fully downloaded.
+    assertThat(syncState.syncTarget().isPresent()).isFalse();
     assertThat(localBlockchain.getChainHeadBlockNumber()).isEqualTo(targetBlock);
   }