diff --git a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/SynchronizerConfiguration.java b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/SynchronizerConfiguration.java index ac3f1d53cf..639900b72a 100644 --- a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/SynchronizerConfiguration.java +++ b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/SynchronizerConfiguration.java @@ -244,20 +244,20 @@ public void parseBlockPropagationRange(final String arg) { @CommandLine.Option( names = "--Xsynchronizer-downloader-change-target-threshold-by-height", hidden = true, - defaultValue = "20", + defaultValue = "200", paramLabel = "", description = "Minimum height difference before switching fast sync download peers (default: ${DEFAULT-VALUE})") - private long downloaderChangeTargetThresholdByHeight = 20L; + private long downloaderChangeTargetThresholdByHeight = 200L; @CommandLine.Option( names = "--Xsynchronizer-downloader-change-target-threshold-by-td", hidden = true, - defaultValue = "1000000000", + defaultValue = "1000000000000000000", paramLabel = "", description = "Minimum total difficulty difference before switching fast sync download peers (default: ${DEFAULT-VALUE})") - private UInt256 downloaderChangeTargetThresholdByTd = UInt256.of(1_000_000_000L); + private UInt256 downloaderChangeTargetThresholdByTd = UInt256.of(1_000_000_000_000_000_000L); @CommandLine.Option( names = "--Xsynchronizer-downloader-header-request-size", diff --git a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/BetterSyncTargetEvaluator.java b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/BetterSyncTargetEvaluator.java new file mode 100644 index 0000000000..83dc05ccf7 --- /dev/null +++ b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/BetterSyncTargetEvaluator.java @@ -0,0 +1,63 @@ +/* + * Copyright 2019 ConsenSys AG. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ +package tech.pegasys.pantheon.ethereum.eth.sync.fullsync; + +import tech.pegasys.pantheon.ethereum.eth.manager.ChainState; +import tech.pegasys.pantheon.ethereum.eth.manager.EthPeer; +import tech.pegasys.pantheon.ethereum.eth.manager.EthPeers; +import tech.pegasys.pantheon.ethereum.eth.sync.SynchronizerConfiguration; +import tech.pegasys.pantheon.util.uint.UInt256; + +import java.util.Optional; + +public class BetterSyncTargetEvaluator { + + private final SynchronizerConfiguration config; + private final EthPeers ethPeers; + + public BetterSyncTargetEvaluator( + final SynchronizerConfiguration config, final EthPeers ethPeers) { + this.config = config; + this.ethPeers = ethPeers; + } + + public boolean shouldSwitchSyncTarget(final EthPeer currentSyncTarget) { + final ChainState currentPeerChainState = currentSyncTarget.chainState(); + final Optional maybeBestPeer = ethPeers.bestPeer(); + + return maybeBestPeer + .map( + bestPeer -> { + if (EthPeers.BEST_CHAIN.compare(bestPeer, currentSyncTarget) <= 0) { + // Our current target is better or equal to the best peer + return false; + } + // Require some threshold to be exceeded before switching targets to keep some + // stability when multiple peers are in range of each other + final ChainState bestPeerChainState = bestPeer.chainState(); + final UInt256 tdDifference = + bestPeerChainState + .getBestBlock() + .getTotalDifficulty() + .minus(currentPeerChainState.getBestBlock().getTotalDifficulty()); + if (tdDifference.compareTo(config.downloaderChangeTargetThresholdByTd()) > 0) { + return true; + } + final long heightDifference = + bestPeerChainState.getEstimatedHeight() + - currentPeerChainState.getEstimatedHeight(); + return heightDifference > config.downloaderChangeTargetThresholdByHeight(); + }) + .orElse(false); + } +} 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 4d826615fe..32a1e4bcfa 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 @@ -17,10 +17,8 @@ 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; 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.SyncTarget; @@ -38,9 +36,9 @@ class FullSyncTargetManager extends SyncTargetManager { private static final Logger LOG = LogManager.getLogger(); - private final SynchronizerConfiguration config; private final ProtocolContext protocolContext; private final EthContext ethContext; + private final BetterSyncTargetEvaluator betterSyncTargetEvaluator; FullSyncTargetManager( final SynchronizerConfiguration config, @@ -49,9 +47,9 @@ class FullSyncTargetManager extends SyncTargetManager { final EthContext ethContext, final MetricsSystem metricsSystem) { super(config, protocolSchedule, protocolContext, ethContext, metricsSystem); - this.config = config; this.protocolContext = protocolContext; this.ethContext = ethContext; + betterSyncTargetEvaluator = new BetterSyncTargetEvaluator(config, ethContext.getEthPeers()); } @Override @@ -100,36 +98,7 @@ public boolean isSyncTargetReached(final EthPeer peer) { @Override public boolean shouldSwitchSyncTarget(final SyncTarget currentTarget) { - final EthPeer currentPeer = currentTarget.peer(); - final ChainState currentPeerChainState = currentPeer.chainState(); - final Optional maybeBestPeer = ethContext.getEthPeers().bestPeer(); - - return maybeBestPeer - .map( - bestPeer -> { - if (EthPeers.BEST_CHAIN.compare(bestPeer, currentPeer) <= 0) { - // Our current target is better or equal to the best peer - return false; - } - // Require some threshold to be exceeded before switching targets to keep some - // stability - // when multiple peers are in range of each other - final ChainState bestPeerChainState = bestPeer.chainState(); - final long heightDifference = - bestPeerChainState.getEstimatedHeight() - - currentPeerChainState.getEstimatedHeight(); - if (heightDifference == 0 && bestPeerChainState.getEstimatedHeight() == 0) { - // Only check td if we don't have a height metric - final UInt256 tdDifference = - bestPeerChainState - .getBestBlock() - .getTotalDifficulty() - .minus(currentPeerChainState.getBestBlock().getTotalDifficulty()); - return tdDifference.compareTo(config.downloaderChangeTargetThresholdByTd()) > 0; - } - return heightDifference > config.downloaderChangeTargetThresholdByHeight(); - }) - .orElse(false); + return betterSyncTargetEvaluator.shouldSwitchSyncTarget(currentTarget.peer()); } @Override diff --git a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/BetterSyncTargetEvaluatorTest.java b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/BetterSyncTargetEvaluatorTest.java new file mode 100644 index 0000000000..cd1366c8da --- /dev/null +++ b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/BetterSyncTargetEvaluatorTest.java @@ -0,0 +1,157 @@ +/* + * Copyright 2019 ConsenSys AG. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ +package tech.pegasys.pantheon.ethereum.eth.sync.fullsync; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import tech.pegasys.pantheon.ethereum.core.Hash; +import tech.pegasys.pantheon.ethereum.eth.manager.ChainState; +import tech.pegasys.pantheon.ethereum.eth.manager.EthPeer; +import tech.pegasys.pantheon.ethereum.eth.manager.EthPeers; +import tech.pegasys.pantheon.ethereum.eth.sync.SynchronizerConfiguration; +import tech.pegasys.pantheon.util.uint.UInt256; + +import java.util.Optional; + +import org.junit.Test; + +public class BetterSyncTargetEvaluatorTest { + + private static final int CURRENT_TARGET_HEIGHT = 10; + private static final int CURRENT_TARGET_TD = 50; + private static final int HEIGHT_THRESHOLD = 100; + private static final int TD_THRESHOLD = 5; + private final EthPeers ethPeers = mock(EthPeers.class); + private final EthPeer currentTarget = peer(CURRENT_TARGET_HEIGHT, CURRENT_TARGET_TD); + private final BetterSyncTargetEvaluator evaluator = + new BetterSyncTargetEvaluator( + SynchronizerConfiguration.builder() + .downloaderChangeTargetThresholdByHeight(HEIGHT_THRESHOLD) + .downloaderChangeTargetThresholdByTd(UInt256.of(TD_THRESHOLD)) + .build(), + ethPeers); + + @Test + public void shouldNotSwitchTargetsIfNoBestPeerIsAvailable() { + when(ethPeers.bestPeer()).thenReturn(Optional.empty()); + + assertThat(evaluator.shouldSwitchSyncTarget(currentTarget)).isFalse(); + } + + @Test + public void shouldNotSwitchTargetWhenBestPeerHasLowerHeightAndDifficulty() { + bestPeerWithDelta(-1, -1); + assertThat(evaluator.shouldSwitchSyncTarget(currentTarget)).isFalse(); + } + + @Test + public void shouldNotSwitchTargetWhenBestPeerHasSameHeightAndLowerDifficulty() { + bestPeerWithDelta(0, -1); + assertThat(evaluator.shouldSwitchSyncTarget(currentTarget)).isFalse(); + } + + @Test + public void shouldNotSwitchTargetWhenBestPeerHasLowerHeightAndSameDifficulty() { + bestPeerWithDelta(-1, 0); + assertThat(evaluator.shouldSwitchSyncTarget(currentTarget)).isFalse(); + } + + @Test + public void shouldNotSwitchTargetWhenBestPeerHasGreaterHeightAndLowerDifficulty() { + bestPeerWithDelta(HEIGHT_THRESHOLD + 1, -1); + assertThat(evaluator.shouldSwitchSyncTarget(currentTarget)).isFalse(); + } + + @Test + public void shouldNotSwitchTargetWhenBestPeerHasEqualHeightAndDifficulty() { + bestPeerWithDelta(0, 0); + assertThat(evaluator.shouldSwitchSyncTarget(currentTarget)).isFalse(); + } + + @Test + public void shouldNotSwitchWhenHeightAndTdHigherWithinThreshold() { + bestPeerWithDelta(HEIGHT_THRESHOLD - 1, TD_THRESHOLD - 1); + assertThat(evaluator.shouldSwitchSyncTarget(currentTarget)).isFalse(); + } + + @Test + public void shouldNotSwitchWhenHeightAndTdHigherEqualToThreshold() { + bestPeerWithDelta(HEIGHT_THRESHOLD, TD_THRESHOLD); + assertThat(evaluator.shouldSwitchSyncTarget(currentTarget)).isFalse(); + } + + @Test + public void shouldSwitchWhenHeightExceedsThresholdAndDifficultyEqual() { + bestPeerWithDelta(HEIGHT_THRESHOLD + 1, 0); + assertThat(evaluator.shouldSwitchSyncTarget(currentTarget)).isTrue(); + } + + @Test + public void shouldSwitchWhenHeightExceedsThresholdAndDifficultyWithinThreshold() { + bestPeerWithDelta(HEIGHT_THRESHOLD + 1, TD_THRESHOLD - 1); + assertThat(evaluator.shouldSwitchSyncTarget(currentTarget)).isTrue(); + } + + @Test + public void shouldSwitchWhenHeightAndDifficultyExceedThreshold() { + bestPeerWithDelta(HEIGHT_THRESHOLD + 1, TD_THRESHOLD + 1); + assertThat(evaluator.shouldSwitchSyncTarget(currentTarget)).isTrue(); + } + + @Test + public void shouldNotSwitchWhenHeightExceedsThresholdButDifficultyIsLower() { + bestPeerWithDelta(HEIGHT_THRESHOLD + 1, -1); + assertThat(evaluator.shouldSwitchSyncTarget(currentTarget)).isFalse(); + } + + @Test + public void shouldSwitchWhenDifficultyExceedsThresholdAndHeightIsEqual() { + bestPeerWithDelta(0, TD_THRESHOLD + 1); + assertThat(evaluator.shouldSwitchSyncTarget(currentTarget)).isTrue(); + } + + @Test + public void shouldSwitchWhenDifficultyExceedsThresholdAndHeightIsLower() { + bestPeerWithDelta(-1, TD_THRESHOLD + 1); + assertThat(evaluator.shouldSwitchSyncTarget(currentTarget)).isTrue(); + } + + @Test + public void shouldSwitchWhenDifficultyExceedsThresholdAndHeightIsWithinThreshold() { + bestPeerWithDelta(HEIGHT_THRESHOLD - 1, TD_THRESHOLD + 1); + assertThat(evaluator.shouldSwitchSyncTarget(currentTarget)).isTrue(); + } + + @Test + public void shouldSwitchWhenHeightAndDifficultyExceedsThreshold() { + bestPeerWithDelta(HEIGHT_THRESHOLD + 1, TD_THRESHOLD + 1); + assertThat(evaluator.shouldSwitchSyncTarget(currentTarget)).isTrue(); + } + + private void bestPeerWithDelta(final long height, final long totalDifficulty) { + final EthPeer bestPeer = + peer(CURRENT_TARGET_HEIGHT + height, CURRENT_TARGET_TD + totalDifficulty); + when(ethPeers.bestPeer()).thenReturn(Optional.of(bestPeer)); + } + + private EthPeer peer(final long chainHeight, final long totalDifficulty) { + final EthPeer peer = mock(EthPeer.class); + final ChainState chainState = new ChainState(); + chainState.updateHeightEstimate(chainHeight); + chainState.statusReceived(Hash.EMPTY, UInt256.of(totalDifficulty)); + when(peer.chainState()).thenReturn(chainState); + return peer; + } +} diff --git a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncChainDownloaderTest.java b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncChainDownloaderTest.java index 8eb88e5309..7b39ab7c20 100644 --- a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncChainDownloaderTest.java +++ b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncChainDownloaderTest.java @@ -26,6 +26,7 @@ import tech.pegasys.pantheon.ethereum.core.Block; import tech.pegasys.pantheon.ethereum.core.BlockBody; import tech.pegasys.pantheon.ethereum.core.BlockDataGenerator; +import tech.pegasys.pantheon.ethereum.core.BlockDataGenerator.BlockOptions; import tech.pegasys.pantheon.ethereum.core.BlockHeader; import tech.pegasys.pantheon.ethereum.core.TransactionReceipt; import tech.pegasys.pantheon.ethereum.eth.manager.EthContext; @@ -426,16 +427,10 @@ public void doesNotSwitchSyncTarget_betterTdUnderThreshold() { assertThat(syncState.syncTarget().get().peer()).isEqualTo(bestPeer.getEthPeer()); // Update otherPeer so that its a better target and send some responses to push logic forward - bestPeer - .getEthPeer() - .chainState() - .updateForAnnouncedBlock( - gen.header(1000), localBlockchain.getChainHead().getTotalDifficulty().plus(201)); - otherPeer - .getEthPeer() - .chainState() - .updateForAnnouncedBlock( - gen.header(1000), localBlockchain.getChainHead().getTotalDifficulty().plus(300)); + final BlockHeader newBestBlock = + gen.header(1000, new BlockOptions().setDifficulty(UInt256.ZERO)); + bestPeer.getEthPeer().chainState().updateForAnnouncedBlock(newBestBlock, localTd.plus(201)); + otherPeer.getEthPeer().chainState().updateForAnnouncedBlock(newBestBlock, localTd.plus(300)); // Process through first task cycle final CompletableFuture firstTask = downloader.getCurrentTask();