Fix FastSyncCheckpointHeaderManager to ensure that the first checkpoint header, if present, is always the common ancestor. (#750)

Signed-off-by: Adrian Sutton <adrian.sutton@consensys.net>

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fastsync/FastSyncChainDownloader.java

@@ -38,7 +38,6 @@ public class FastSyncChainDownloader<C> {
   private final ProtocolContext<C> protocolContext;
   private final EthContext ethContext;
   private final LabelledMetric<OperationTimer> ethTasksTimer;
-  private final BlockHeader pivotBlockHeader;
 
   FastSyncChainDownloader(
       final SynchronizerConfiguration config,
@@ -53,7 +52,6 @@ public class FastSyncChainDownloader<C> {
     this.protocolContext = protocolContext;
     this.ethContext = ethContext;
     this.ethTasksTimer = ethTasksTimer;
-    this.pivotBlockHeader = pivotBlockHeader;
     chainDownloader =
         new ChainDownloader<>(
             config,
@@ -86,12 +84,7 @@ public class FastSyncChainDownloader<C> {
   private CompletableFuture<List<Block>> importBlocksForCheckpoints(
       final List<BlockHeader> checkpointHeaders) {
     if (checkpointHeaders.size() < 2) {
-      final BlockHeader chainHeadHeader = protocolContext.getBlockchain().getChainHeadHeader();
+      return CompletableFuture.completedFuture(emptyList());
-      if (chainHeadHeader.getNumber() < pivotBlockHeader.getNumber()) {
-        checkpointHeaders.add(0, chainHeadHeader);
-      } else {
-        return CompletableFuture.completedFuture(emptyList());
-      }
     }
     final PipelinedImportChainSegmentTask<C, BlockWithReceipts> importTask =
         PipelinedImportChainSegmentTask.forCheckpoints(

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fastsync/FastSyncCheckpointHeaderManager.java

@@ -28,6 +28,8 @@ import java.util.concurrent.CompletableFuture;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
+import com.google.common.collect.Lists;
+
 class FastSyncCheckpointHeaderManager<C> extends CheckpointHeaderManager<C> {
   private final SynchronizerConfiguration config;
   private final BlockHeader pivotBlockHeader;
@@ -58,6 +60,9 @@ class FastSyncCheckpointHeaderManager<C> extends CheckpointHeaderManager<C> {
               if (shouldDownloadMoreCheckpoints()
                   && nextChainSegmentIncludesPivotBlock(lastSegmentEnd)
                   && pivotBlockNotAlreadyIncluded(lastSegmentEnd)) {
+                if (checkpointHeaders.isEmpty()) {
+                  return Lists.newArrayList(lastHeader, pivotBlockHeader);
+                }
                 return concat(checkpointHeaders, pivotBlockHeader);
               }
               return checkpointHeaders;

ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fastsync/FastSyncCheckpointHeaderManagerTest.java

@@ -14,7 +14,6 @@ package tech.pegasys.pantheon.ethereum.eth.sync.fastsync;
 
 import static java.util.Arrays.asList;
 import static java.util.Collections.emptyList;
-import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.spy;
 
@@ -125,10 +124,10 @@ public class FastSyncCheckpointHeaderManagerTest {
   }
 
   @Test
-  public void shouldHaveOnlyPivotBlockWhenCommonAncestorImmediatelyBeforePivotBlock() {
+  public void shouldHaveCommonAncestorAndPivotBlockWhenCommonAncestorImmediatelyBeforePivotBlock() {
-    final SyncTarget syncTarget =
+    final BlockHeader commonAncestor = block(pivotBlockHeader.getNumber() - 1);
-        syncState.setSyncTarget(peer.getEthPeer(), block(pivotBlockHeader.getNumber() - 1));
+    final SyncTarget syncTarget = syncState.setSyncTarget(peer.getEthPeer(), commonAncestor);
-    assertCheckpointHeaders(syncTarget, singletonList(pivotBlockHeader));
+    assertCheckpointHeaders(syncTarget, asList(commonAncestor, pivotBlockHeader));
   }
 
   private void assertCheckpointHeaders(