From c2f2e7212ab6d49e55d7a5cd6e97acc6accea979 Mon Sep 17 00:00:00 2001 From: Trent Mohay <37158202+rain-on@users.noreply.github.com> Date: Tue, 26 Mar 2019 15:18:08 +1100 Subject: [PATCH] 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 --- .../ethereum/eth/sync/ChainDownloader.java | 2 +- .../ethereum/eth/sync/SyncTargetManager.java | 15 ++++++++++++++- .../eth/sync/fastsync/FastSyncTargetManager.java | 5 +++++ .../eth/sync/fullsync/FullSyncTargetManager.java | 16 ++++++++++------ .../sync/fullsync/FullSyncDownloaderTest.java | 4 ++-- 5 files changed, 32 insertions(+), 10 deletions(-) diff --git a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/ChainDownloader.java b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/ChainDownloader.java index 3b76aef0a3..92047ef893 100644 --- a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/ChainDownloader.java +++ b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/ChainDownloader.java @@ -165,7 +165,7 @@ public class ChainDownloader { } private boolean finishedSyncingToCurrentTarget() { - return syncTargetManager.isSyncTargetDisconnected() + return !syncTargetManager.syncTargetCanProvideMoreBlocks() || checkpointHeaderManager.checkpointsHaveTimedOut() || chainSegmentsHaveTimedOut(); } diff --git a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/SyncTargetManager.java b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/SyncTargetManager.java index 537d2ed959..0477456630 100644 --- a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/SyncTargetManager.java +++ b/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 { 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 protocolSchedule; private final ProtocolContext protocolContext; private final EthContext ethContext; - private final SyncState syncState; private final MetricsSystem metricsSystem; public SyncTargetManager( @@ -144,4 +146,15 @@ public abstract class SyncTargetManager { 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); + } } diff --git a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fastsync/FastSyncTargetManager.java b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fastsync/FastSyncTargetManager.java index e5c4b6d4d4..2752453169 100644 --- a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fastsync/FastSyncTargetManager.java +++ b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fastsync/FastSyncTargetManager.java @@ -118,4 +118,9 @@ class FastSyncTargetManager extends SyncTargetManager { public boolean shouldContinueDownloading() { return !protocolContext.getBlockchain().getChainHeadHash().equals(pivotBlockHeader.getHash()); } + + @Override + public boolean isSyncTargetReached(final EthPeer peer) { + return syncState.chainHeadNumber() >= pivotBlockHeader.getNumber(); + } } diff --git a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncTargetManager.java b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncTargetManager.java index df486d494f..43bbce0d55 100644 --- a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncTargetManager.java +++ b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncTargetManager.java @@ -41,7 +41,6 @@ class FullSyncTargetManager extends SyncTargetManager { private final SynchronizerConfiguration config; private final ProtocolContext protocolContext; private final EthContext ethContext; - private final SyncState syncState; FullSyncTargetManager( final SynchronizerConfiguration config, @@ -54,7 +53,6 @@ class FullSyncTargetManager extends SyncTargetManager { this.config = config; this.protocolContext = protocolContext; this.ethContext = ethContext; - this.syncState = syncState; } @Override @@ -82,10 +80,7 @@ class FullSyncTargetManager extends SyncTargetManager { 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 extends SyncTargetManager { } } + @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(); diff --git a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncDownloaderTest.java b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncDownloaderTest.java index a16f244898..1b1603ff09 100644 --- a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncDownloaderTest.java +++ b/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); }