From f8977619514151a8d37876ffe9deaf1892467e42 Mon Sep 17 00:00:00 2001 From: Adrian Sutton Date: Mon, 1 Apr 2019 06:30:32 +1000 Subject: [PATCH] Remove SyncState from SyncTargetManager (#1188) Signed-off-by: Adrian Sutton --- .../ethereum/eth/sync/ChainDownloader.java | 23 +++++++++++++++---- .../ethereum/eth/sync/SyncTargetManager.java | 23 ++++++------------- .../fastsync/FastSyncChainDownloader.java | 1 - .../sync/fastsync/FastSyncTargetManager.java | 8 +++---- .../eth/sync/fullsync/FullSyncDownloader.java | 6 +++-- .../sync/fullsync/FullSyncTargetManager.java | 10 ++++---- .../ethereum/eth/sync/state/SyncState.java | 20 ++++------------ .../sync/fullsync/FullSyncDownloaderTest.java | 9 +++++--- .../fullsync/FullSyncTargetManagerTest.java | 9 +++----- 9 files changed, 52 insertions(+), 57 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 92047ef893..c0a13d8fa7 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 @@ -96,7 +96,8 @@ public class ChainDownloader { // Find target, pull checkpoint headers, import, repeat currentTask = waitForPeers() - .thenCompose(r -> syncTargetManager.findSyncTarget()) + .thenCompose(r -> syncTargetManager.findSyncTarget(syncState.syncTarget())) + .thenApply(this::updateSyncState) .thenCompose(this::pullCheckpointHeaders) .thenCompose(this::importBlocks) .thenCompose(r -> checkSyncTarget()) @@ -128,6 +129,20 @@ public class ChainDownloader { }); } + private SyncTarget updateSyncState(final SyncTarget newTarget) { + if (isSameAsCurrentTarget(newTarget)) { + return syncState.syncTarget().get(); + } + return syncState.setSyncTarget(newTarget.peer(), newTarget.commonAncestor()); + } + + private Boolean isSameAsCurrentTarget(final SyncTarget newTarget) { + return syncState + .syncTarget() + .map(currentTarget -> currentTarget.equals(newTarget)) + .orElse(false); + } + private CompletableFuture> pullCheckpointHeaders(final SyncTarget syncTarget) { return syncTargetManager.isSyncTargetDisconnected() ? CompletableFuture.completedFuture(emptyList()) @@ -151,7 +166,7 @@ public class ChainDownloader { clearSyncTarget(syncTarget); return CompletableFuture.completedFuture(null); } - if (finishedSyncingToCurrentTarget()) { + if (finishedSyncingToCurrentTarget(syncTarget)) { LOG.info("Finished syncing to target: {}.", syncTarget); clearSyncTarget(syncTarget); // Wait a bit before checking for a new sync target @@ -164,8 +179,8 @@ public class ChainDownloader { return CompletableFuture.completedFuture(null); } - private boolean finishedSyncingToCurrentTarget() { - return !syncTargetManager.syncTargetCanProvideMoreBlocks() + private boolean finishedSyncingToCurrentTarget(final SyncTarget syncTarget) { + return !syncTargetManager.syncTargetCanProvideMoreBlocks(syncTarget) || 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 0477456630..5b61dc87a6 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 @@ -18,7 +18,6 @@ import tech.pegasys.pantheon.ethereum.ProtocolContext; import tech.pegasys.pantheon.ethereum.eth.manager.EthContext; import tech.pegasys.pantheon.ethereum.eth.manager.EthPeer; import tech.pegasys.pantheon.ethereum.eth.manager.task.WaitForPeerTask; -import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncState; import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncTarget; import tech.pegasys.pantheon.ethereum.eth.sync.tasks.DetermineCommonAncestorTask; import tech.pegasys.pantheon.ethereum.mainnet.ProtocolSchedule; @@ -35,8 +34,6 @@ 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; @@ -50,19 +47,17 @@ public abstract class SyncTargetManager { final ProtocolSchedule protocolSchedule, final ProtocolContext protocolContext, final EthContext ethContext, - final SyncState syncState, final MetricsSystem metricsSystem) { this.config = config; this.protocolSchedule = protocolSchedule; this.protocolContext = protocolContext; this.ethContext = ethContext; - this.syncState = syncState; this.metricsSystem = metricsSystem; } - public CompletableFuture findSyncTarget() { - return syncState - .syncTarget() + public CompletableFuture findSyncTarget( + final Optional currentSyncTarget) { + return currentSyncTarget .map(CompletableFuture::completedFuture) // Return an existing sync target if present .orElseGet(this::selectNewSyncTarget); } @@ -93,7 +88,7 @@ public abstract class SyncTargetManager { if (target == null) { return waitForPeerAndThenSetSyncTarget(); } - final SyncTarget syncTarget = syncState.setSyncTarget(bestPeer, target); + final SyncTarget syncTarget = new SyncTarget(bestPeer, target); LOG.info( "Found common ancestor with peer {} at block {}", bestPeer, @@ -120,7 +115,7 @@ public abstract class SyncTargetManager { protected abstract CompletableFuture> selectBestAvailableSyncTarget(); private CompletableFuture waitForPeerAndThenSetSyncTarget() { - return waitForNewPeer().handle((r, t) -> r).thenCompose((r) -> findSyncTarget()); + return waitForNewPeer().handle((r, t) -> r).thenCompose((r) -> selectNewSyncTarget()); } private CompletableFuture waitForNewPeer() { @@ -149,12 +144,8 @@ public abstract class SyncTargetManager { 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(); + public boolean syncTargetCanProvideMoreBlocks(final SyncTarget syncTarget) { + final EthPeer currentSyncingPeer = syncTarget.peer(); return !isSyncTargetDisconnected() && !isSyncTargetReached(currentSyncingPeer); } } diff --git a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fastsync/FastSyncChainDownloader.java b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fastsync/FastSyncChainDownloader.java index 6823aa0a34..73daf0677f 100644 --- a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fastsync/FastSyncChainDownloader.java +++ b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fastsync/FastSyncChainDownloader.java @@ -66,7 +66,6 @@ public class FastSyncChainDownloader { protocolSchedule, protocolContext, ethContext, - syncState, metricsSystem, pivotBlockHeader), new FastSyncCheckpointHeaderManager<>( 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 2752453169..48a09b2d9e 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 @@ -16,12 +16,12 @@ import static java.util.concurrent.CompletableFuture.completedFuture; import static tech.pegasys.pantheon.ethereum.eth.sync.fastsync.PivotBlockRetriever.MAX_PIVOT_BLOCK_RETRIES; import tech.pegasys.pantheon.ethereum.ProtocolContext; +import tech.pegasys.pantheon.ethereum.chain.MutableBlockchain; import tech.pegasys.pantheon.ethereum.core.BlockHeader; import tech.pegasys.pantheon.ethereum.eth.manager.EthContext; import tech.pegasys.pantheon.ethereum.eth.manager.EthPeer; import tech.pegasys.pantheon.ethereum.eth.sync.SyncTargetManager; import tech.pegasys.pantheon.ethereum.eth.sync.SynchronizerConfiguration; -import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncState; import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncTarget; import tech.pegasys.pantheon.ethereum.eth.sync.tasks.RetryingGetHeaderFromPeerByNumberTask; import tech.pegasys.pantheon.ethereum.mainnet.ProtocolSchedule; @@ -49,10 +49,9 @@ class FastSyncTargetManager extends SyncTargetManager { final ProtocolSchedule protocolSchedule, final ProtocolContext protocolContext, final EthContext ethContext, - final SyncState syncState, final MetricsSystem metricsSystem, final BlockHeader pivotBlockHeader) { - super(config, protocolSchedule, protocolContext, ethContext, syncState, metricsSystem); + super(config, protocolSchedule, protocolContext, ethContext, metricsSystem); this.protocolSchedule = protocolSchedule; this.protocolContext = protocolContext; this.ethContext = ethContext; @@ -121,6 +120,7 @@ class FastSyncTargetManager extends SyncTargetManager { @Override public boolean isSyncTargetReached(final EthPeer peer) { - return syncState.chainHeadNumber() >= pivotBlockHeader.getNumber(); + final MutableBlockchain blockchain = protocolContext.getBlockchain(); + return blockchain.getChainHeadBlockNumber() >= pivotBlockHeader.getNumber(); } } diff --git a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncDownloader.java b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncDownloader.java index cd2c3ca0f8..87dd43e6db 100644 --- a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncDownloader.java +++ b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncDownloader.java @@ -61,7 +61,7 @@ public class FullSyncDownloader { ethContext, syncState, new FullSyncTargetManager<>( - config, protocolSchedule, protocolContext, ethContext, syncState, metricsSystem), + config, protocolSchedule, protocolContext, ethContext, metricsSystem), new CheckpointHeaderManager<>( config, protocolContext, ethContext, syncState, protocolSchedule, metricsSystem), this::importBlocksForCheckpoints, @@ -111,6 +111,8 @@ public class FullSyncDownloader { public TrailingPeerRequirements calculateTrailingPeerRequirements() { return syncState.isInSync() ? TrailingPeerRequirements.UNRESTRICTED - : new TrailingPeerRequirements(syncState.chainHeadNumber(), config.getMaxTrailingPeers()); + : new TrailingPeerRequirements( + protocolContext.getBlockchain().getChainHeadBlockNumber(), + config.getMaxTrailingPeers()); } } 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 43bbce0d55..4d826615fe 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 @@ -15,6 +15,7 @@ package tech.pegasys.pantheon.ethereum.eth.sync.fullsync; import static java.util.concurrent.CompletableFuture.completedFuture; import tech.pegasys.pantheon.ethereum.ProtocolContext; +import tech.pegasys.pantheon.ethereum.chain.MutableBlockchain; import tech.pegasys.pantheon.ethereum.core.BlockHeader; import tech.pegasys.pantheon.ethereum.eth.manager.ChainState; import tech.pegasys.pantheon.ethereum.eth.manager.EthContext; @@ -22,7 +23,6 @@ import tech.pegasys.pantheon.ethereum.eth.manager.EthPeer; import tech.pegasys.pantheon.ethereum.eth.manager.EthPeers; import tech.pegasys.pantheon.ethereum.eth.sync.SyncTargetManager; import tech.pegasys.pantheon.ethereum.eth.sync.SynchronizerConfiguration; -import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncState; import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncTarget; import tech.pegasys.pantheon.ethereum.mainnet.ProtocolSchedule; import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage.DisconnectReason; @@ -47,9 +47,8 @@ class FullSyncTargetManager extends SyncTargetManager { final ProtocolSchedule protocolSchedule, final ProtocolContext protocolContext, final EthContext ethContext, - final SyncState syncState, final MetricsSystem metricsSystem) { - super(config, protocolSchedule, protocolContext, ethContext, syncState, metricsSystem); + super(config, protocolSchedule, protocolContext, ethContext, metricsSystem); this.config = config; this.protocolContext = protocolContext; this.ethContext = ethContext; @@ -93,9 +92,10 @@ class FullSyncTargetManager extends SyncTargetManager { public boolean isSyncTargetReached(final EthPeer peer) { final long peerHeight = peer.chainState().getEstimatedHeight(); final UInt256 peerTd = peer.chainState().getBestBlock().getTotalDifficulty(); + final MutableBlockchain blockchain = protocolContext.getBlockchain(); - return peerTd.compareTo(syncState.chainHeadTotalDifficulty()) <= 0 - && peerHeight <= syncState.chainHeadNumber(); + return peerTd.compareTo(blockchain.getChainHead().getTotalDifficulty()) <= 0 + && peerHeight <= blockchain.getChainHeadBlockNumber(); } @Override diff --git a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/state/SyncState.java b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/state/SyncState.java index c3df8531d2..93ab606d67 100644 --- a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/state/SyncState.java +++ b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/state/SyncState.java @@ -19,7 +19,6 @@ import tech.pegasys.pantheon.ethereum.core.Synchronizer.SyncStatusListener; import tech.pegasys.pantheon.ethereum.eth.manager.EthPeer; import tech.pegasys.pantheon.ethereum.eth.manager.EthPeers; import tech.pegasys.pantheon.util.Subscribers; -import tech.pegasys.pantheon.util.uint.UInt256; import java.util.Optional; @@ -38,7 +37,7 @@ public class SyncState { public SyncState(final Blockchain blockchain, final EthPeers ethPeers) { this.blockchain = blockchain; this.ethPeers = ethPeers; - this.startingBlock = chainHeadNumber(); + this.startingBlock = this.blockchain.getChainHeadBlockNumber(); blockchain.observeBlockAdded( (event, chain) -> { if (event.isNewCanonicalHead()) { @@ -62,21 +61,15 @@ public class SyncState { } public SyncStatus syncStatus() { - return new SyncStatus(startingBlock(), chainHeadNumber(), bestChainHeight()); + final long chainHeadBlockNumber = blockchain.getChainHeadBlockNumber(); + return new SyncStatus( + startingBlock(), chainHeadBlockNumber, bestChainHeight(chainHeadBlockNumber)); } public long startingBlock() { return startingBlock; } - public long chainHeadNumber() { - return blockchain.getChainHeadBlockNumber(); - } - - public UInt256 chainHeadTotalDifficulty() { - return blockchain.getChainHead().getTotalDifficulty(); - } - public Optional syncTarget() { return syncTarget; } @@ -118,11 +111,6 @@ public class SyncState { target.addPeerChainEstimatedHeightListener(estimatedHeight -> checkInSync()); } - public long bestChainHeight() { - final long localChainHeight = blockchain.getChainHeadBlockNumber(); - return bestChainHeight(localChainHeight); - } - public long bestChainHeight(final long localChainHeight) { return Math.max( localChainHeight, 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 561e9b73a2..9febe80f35 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 @@ -382,7 +382,8 @@ public class FullSyncDownloaderTest { peerB .getEthPeer() .chainState() - .updateForAnnouncedBlock(gen.header(), syncState.chainHeadTotalDifficulty().plus(300)); + .updateForAnnouncedBlock( + gen.header(), localBlockchain.getChainHead().getTotalDifficulty().plus(300)); // Process through first task cycle final CompletableFuture firstTask = downloader.getCurrentTask(); @@ -426,11 +427,13 @@ public class FullSyncDownloaderTest { bestPeer .getEthPeer() .chainState() - .updateForAnnouncedBlock(gen.header(1000), syncState.chainHeadTotalDifficulty().plus(201)); + .updateForAnnouncedBlock( + gen.header(1000), localBlockchain.getChainHead().getTotalDifficulty().plus(201)); otherPeer .getEthPeer() .chainState() - .updateForAnnouncedBlock(gen.header(1000), syncState.chainHeadTotalDifficulty().plus(300)); + .updateForAnnouncedBlock( + gen.header(1000), localBlockchain.getChainHead().getTotalDifficulty().plus(300)); // Process through first task cycle final CompletableFuture firstTask = downloader.getCurrentTask(); diff --git a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncTargetManagerTest.java b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncTargetManagerTest.java index 4736a9841c..fe74613d35 100644 --- a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncTargetManagerTest.java +++ b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncTargetManagerTest.java @@ -27,13 +27,13 @@ import tech.pegasys.pantheon.ethereum.eth.manager.RespondingEthPeer; import tech.pegasys.pantheon.ethereum.eth.manager.RespondingEthPeer.Responder; import tech.pegasys.pantheon.ethereum.eth.manager.ethtaskutils.BlockchainSetupUtil; import tech.pegasys.pantheon.ethereum.eth.sync.SynchronizerConfiguration; -import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncState; import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncTarget; import tech.pegasys.pantheon.ethereum.mainnet.MainnetProtocolSchedule; import tech.pegasys.pantheon.ethereum.mainnet.ProtocolSchedule; import tech.pegasys.pantheon.ethereum.worldstate.WorldStateArchive; import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem; +import java.util.Optional; import java.util.concurrent.CompletableFuture; import org.junit.Before; @@ -64,8 +64,6 @@ public class FullSyncTargetManagerTest { EthProtocolManagerTestUtil.create( localBlockchain, localWorldState, new EthScheduler(1, 1, 1, new NoOpMetricsSystem())); final EthContext ethContext = ethProtocolManager.ethContext(); - final SyncState syncState = - new SyncState(protocolContext.getBlockchain(), ethContext.getEthPeers()); localBlockchainSetup.importFirstBlocks(5); otherBlockchainSetup.importFirstBlocks(20); syncTargetManager = @@ -74,7 +72,6 @@ public class FullSyncTargetManagerTest { protocolSchedule, protocolContext, ethContext, - syncState, new NoOpMetricsSystem()); } @@ -85,7 +82,7 @@ public class FullSyncTargetManagerTest { final RespondingEthPeer bestPeer = EthProtocolManagerTestUtil.createPeer(ethProtocolManager, 20); - final CompletableFuture result = syncTargetManager.findSyncTarget(); + final CompletableFuture result = syncTargetManager.findSyncTarget(Optional.empty()); bestPeer.respond(responder); @@ -100,7 +97,7 @@ public class FullSyncTargetManagerTest { final RespondingEthPeer bestPeer = EthProtocolManagerTestUtil.createPeer(ethProtocolManager, 20); - final CompletableFuture result = syncTargetManager.findSyncTarget(); + final CompletableFuture result = syncTargetManager.findSyncTarget(Optional.empty()); bestPeer.respond(responder);