Use optimistic head slot when finding common ancestor. (Consensys#4925)

Avoids restarting sync from the finalized slot unnecessarily when optimistic sync is in use.
Zachinquarantine · Feb 2, 2022 · 662887d · 662887d
1 parent cc2f9b1
commit 662887d
Show file tree

Hide file tree

Showing 4 changed files with 56 additions and 4 deletions.
diff --git a/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java b/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java
@@ -983,9 +983,14 @@ public EnrForkId randomEnrForkId() {
   }
 
   public ForkChoiceState randomForkChoiceState(final boolean optimisticHead) {
+    return randomForkChoiceState(randomUInt64(), optimisticHead);
+  }
+
+  public ForkChoiceState randomForkChoiceState(
+      final UInt64 headBlockSlot, final boolean optimisticHead) {
     return new ForkChoiceState(
         randomBytes32(),
-        randomUInt64(),
+        headBlockSlot,
         randomBytes32(),
         randomBytes32(),
         randomBytes32(),

diff --git a/...src/main/java/tech/pegasys/teku/sync/forward/multipeer/MultipeerCommonAncestorFinder.java b/...src/main/java/tech/pegasys/teku/sync/forward/multipeer/MultipeerCommonAncestorFinder.java
@@ -57,7 +57,7 @@ public SafeFuture<UInt64> findCommonAncestor(final TargetChain targetChain) {
         spec.computeStartSlotAtEpoch(recentChainData.getFinalizedEpoch());
 
     if (targetChain.getPeerCount() == 0) {
-      // No sources to find a common ancestor with, assume its the finalized slot
+      // No sources to find a common ancestor with, assume it's the finalized slot
       return SafeFuture.completedFuture(latestFinalizedSlot);
     }
 

diff --git a/sync/src/main/java/tech/pegasys/teku/sync/forward/singlepeer/CommonAncestor.java b/sync/src/main/java/tech/pegasys/teku/sync/forward/singlepeer/CommonAncestor.java
@@ -38,7 +38,9 @@ public CommonAncestor(final RecentChainData storageClient) {
 
   public SafeFuture<UInt64> getCommonAncestor(
       final SyncSource peer, final UInt64 firstNonFinalSlot, final UInt64 peerHeadSlot) {
-    final UInt64 lowestHeadSlot = storageClient.getHeadSlot().min(peerHeadSlot);
+    final UInt64 ourHeadSlot =
+        storageClient.getOptimisticHeadSlot().orElse(storageClient.getHeadSlot());
+    final UInt64 lowestHeadSlot = ourHeadSlot.min(peerHeadSlot);
     if (lowestHeadSlot.isLessThan(firstNonFinalSlot.plus(OPTIMISTIC_HISTORY_LENGTH))) {
       return SafeFuture.completedFuture(firstNonFinalSlot);
     }
@@ -50,7 +52,7 @@ public SafeFuture<UInt64> getCommonAncestor(
     LOG.debug(
         "Local head slot {}. Have {} non finalized slots, "
             + "will sample ahead every {} slots from {} to {}. Peer head is {}",
-        storageClient.getHeadSlot(),
+        ourHeadSlot,
         localNonFinalisedSlotCount,
         SAMPLE_RATE,
         firstRequestedSlot,

diff --git a/sync/src/test/java/tech/pegasys/teku/sync/forward/singlepeer/CommonAncestorTest.java b/sync/src/test/java/tech/pegasys/teku/sync/forward/singlepeer/CommonAncestorTest.java
@@ -19,6 +19,7 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import java.util.Optional;
 import java.util.concurrent.ExecutionException;
 import org.junit.jupiter.api.Test;
 import tech.pegasys.teku.infrastructure.async.SafeFuture;
@@ -112,4 +113,48 @@ void shouldSearchCommonAncestorWithSufficientRemoteData()
     requestFuture1.complete(null);
     assertThat(futureSlot.get()).isEqualTo(currentLocalHead.minus(2250));
   }
+
+  @Test
+  void shouldUseOptimisticHeadAsLocalChainHead() throws ExecutionException, InterruptedException {
+    final UInt64 firstNonFinalSlot = dataStructureUtil.randomUInt64();
+    final UInt64 currentLocalHead =
+        firstNonFinalSlot.plus(CommonAncestor.OPTIMISTIC_HISTORY_LENGTH.times(10));
+    final UInt64 currentRemoteHead =
+        firstNonFinalSlot.plus(CommonAncestor.OPTIMISTIC_HISTORY_LENGTH.times(9));
+    final CommonAncestor commonAncestor = new CommonAncestor(storageClient);
+    final UInt64 syncStartSlot = currentRemoteHead.minus(CommonAncestor.OPTIMISTIC_HISTORY_LENGTH);
+    final SafeFuture<Void> requestFuture1 = new SafeFuture<>();
+    when(peer.requestBlocksByRange(
+            eq(syncStartSlot),
+            eq(CommonAncestor.BLOCK_COUNT),
+            eq(CommonAncestor.SAMPLE_RATE),
+            any()))
+        .thenReturn(requestFuture1);
+    final PeerStatus status =
+        withPeerHeadSlot(
+            currentRemoteHead,
+            spec.computeEpochAtSlot(currentRemoteHead),
+            dataStructureUtil.randomBytes32());
+    when(storageClient.getHeadSlot()).thenReturn(firstNonFinalSlot);
+    when(storageClient.getOptimisticHeadSlot()).thenReturn(Optional.of(currentLocalHead));
+    when(storageClient.containsBlock(any())).thenReturn(true);
+
+    SafeFuture<UInt64> futureSlot =
+        commonAncestor.getCommonAncestor(peer, firstNonFinalSlot, status.getHeadSlot());
+
+    verify(peer)
+        .requestBlocksByRange(
+            eq(syncStartSlot),
+            eq(CommonAncestor.BLOCK_COUNT),
+            eq(CommonAncestor.SAMPLE_RATE),
+            responseListenerArgumentCaptor.capture());
+
+    assertThat(futureSlot.isDone()).isFalse();
+    final RpcResponseListener<SignedBeaconBlock> responseListener =
+        responseListenerArgumentCaptor.getValue();
+    respondWithBlocksAtSlots(
+        responseListener, currentLocalHead.minus(2850), currentLocalHead.minus(2250));
+    requestFuture1.complete(null);
+    assertThat(futureSlot.get()).isEqualTo(currentLocalHead.minus(2250));
+  }
 }