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.
Signed-off-by: Adrian Sutton <adrian.sutton@consensys.net>

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

@@ -165,7 +165,7 @@ public class ChainDownloader<C> {
   }
 
   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 @@ import org.apache.logging.log4j.Logger;
 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 abstract class SyncTargetManager<C> {
   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 @@ class FastSyncTargetManager<C> extends SyncTargetManager<C> {
   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 @@ class FullSyncTargetManager<C> extends SyncTargetManager<C> {
       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 @@ class FullSyncTargetManager<C> extends SyncTargetManager<C> {
     }
   }
 
+  @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 class FullSyncDownloaderTest {
     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);
   }