Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

When picking fast sync pivot block, use the peer with the best total difficulty #899

Merged
merged 3 commits into from
Feb 19, 2019
Merged

When picking fast sync pivot block, use the peer with the best total difficulty #899

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e811e9
When picking fast sync pivot block, use the peer with the best total …
ajsutton Feb 19, 2019
d6cec35
Merge branch 'master' into best-total-difficulty
ajsutton Feb 19, 2019
66028eb
Merge branch 'master' into best-total-difficulty
ajsutton Feb 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/manager/EthPeers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ public class EthPeers {
public static final Comparator<EthPeer> CHAIN_HEIGHT =
Comparator.comparing(((final EthPeer p) -> p.chainState().getEstimatedHeight()));

private static final Comparator<EthPeer> HIGHEST_TOTAL_DIFFICULTY_PEER =
TOTAL_DIFFICULTY.thenComparing(CHAIN_HEIGHT);
Copy link
Contributor

@shemnon shemnon Feb 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from total difficulty (which is the only thing the yellow paper defines) do we know what other clients do to distinguish heavier chains? Chain height is a quick win but Ghost also originally defined ommers as adding to the difficulty (but that's not in the yellow paper). Section 10 implies chain height but it doesn't make it into the formal definition equations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing that matters is total difficulty - if two chains have the same difficulty then clients will generally stick with whichever one they had first (ie equal difficulty does not trigger a re-org) although I have a vague memory of Geth including randomness in there for some reason.

Since Byzantium the difficulty of a block is increased if it contains any ommers.


public static final Comparator<EthPeer> BEST_CHAIN = CHAIN_HEIGHT.thenComparing(TOTAL_DIFFICULTY);

public static final Comparator<EthPeer> LEAST_TO_MOST_BUSY =
Expand Down Expand Up @@ -93,6 +96,10 @@ public Optional<EthPeer> bestPeer() {
return availablePeers().max(BEST_CHAIN);
}

public Optional<EthPeer> highestTotalDifficultyPeer() {
return availablePeers().max(HIGHEST_TOTAL_DIFFICULTY_PEER);
}

public Optional<EthPeer> idlePeer() {
return idlePeers().min(LEAST_TO_MOST_BUSY);
}
Expand Down
2 changes: 1 addition & 1 deletion ...m/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fastsync/FastSyncActions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ public CompletableFuture<FastSyncState> selectPivotBlock(final FastSyncState fas
private CompletableFuture<FastSyncState> selectPivotBlockFromPeers() {
return ethContext
.getEthPeers()
.bestPeer()
.highestTotalDifficultyPeer()
.filter(peer -> peer.chainState().hasEstimatedHeight())
.map(
peer -> {
Expand Down
12 changes: 12 additions & 0 deletions ...h/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fastsync/FastSyncActionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import tech.pegasys.pantheon.ethereum.mainnet.ProtocolSchedule;
import tech.pegasys.pantheon.metrics.LabelledMetric;
import tech.pegasys.pantheon.metrics.OperationTimer;
import tech.pegasys.pantheon.util.uint.UInt256;

import java.util.concurrent.CompletableFuture;
import java.util.concurrent.atomic.AtomicInteger;
Expand Down Expand Up @@ -150,6 +151,17 @@ public void selectPivotBlockShouldSelectBlockPivotDistanceFromBestPeer() {
assertThat(result).isCompletedWithValue(expected);
}

@Test
public void selectPivotBlockShouldConsiderTotalDifficultyWhenSelectingBestPeer() {
EthProtocolManagerTestUtil.createPeer(ethProtocolManager, UInt256.of(1000), 5500);
EthProtocolManagerTestUtil.createPeer(ethProtocolManager, UInt256.of(2000), 4000);

final CompletableFuture<FastSyncState> result =
fastSyncActions.selectPivotBlock(EMPTY_SYNC_STATE);
final FastSyncState expected = new FastSyncState(3000);
assertThat(result).isCompletedWithValue(expected);
}

@Test
public void selectPivotBlockShouldWaitAndRetryIfNoPeersAreAvailable() {
final CompletableFuture<FastSyncState> result =
Expand Down