Clean up pivot selector from peers and unit test (#8020)

* 7582: Add waitForPeer method to PeerSelector and EthPeers

Signed-off-by: Matilda Clerke <[email protected]>

* 7582: Replace all usages of WaitForPeer[s]Task with new EthPeers.waitForPeer method

Signed-off-by: Matilda Clerke <[email protected]>

* 7582: Fix PivotBlockConfirmerTest

Signed-off-by: Matilda Clerke <[email protected]>

* 7582: spotless

Signed-off-by: Matilda Clerke <[email protected]>

* 7582: Fix broken PivotBlockRetrieverTest

Signed-off-by: Matilda Clerke <[email protected]>

* 7582: Fix broken FastSyncActionsTest

Signed-off-by: Matilda Clerke <[email protected]>

* 7582: spotless

Signed-off-by: Matilda Clerke <[email protected]>

* 7582: Simplify PivotSelectorFromPeers.selectNewPivotBlock and add unit tests

Signed-off-by: Matilda Clerke <[email protected]>

* 7582: Fix issues after merge

Signed-off-by: Matilda Clerke <[email protected]>

* 7582: Put AbstractSyncTargetManager.waitForPeerAndThenSetSyncTarget code back separate thread to avoid infinite loop waiting for peers during acceptance tests

Signed-off-by: Matilda Clerke <[email protected]>

* 7582: Remove pivot block checks when waiting for peer in FastSyncActions

Signed-off-by: Matilda Clerke <[email protected]>

* 7582: Remove estimated chain height check from PivotBlockConfirmer when waiting for peers

Signed-off-by: Matilda Clerke <[email protected]>

* 7582: Fix broken PivotBlockRetrieverTest

Signed-off-by: Matilda Clerke <[email protected]>

* Fix compile errors

Signed-off-by: Matilda Clerke <[email protected]>

* spotless

Signed-off-by: Matilda Clerke <[email protected]>

* Refactor mockito usage

Signed-off-by: Matilda Clerke <[email protected]>

---------

Signed-off-by: Matilda Clerke <[email protected]>

Author: Matilda-Clerke

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/fastsync/PivotSelectorFromPeers.java

@@ -23,6 +23,7 @@
 import org.hyperledger.besu.ethereum.eth.sync.TrailingPeerRequirements;
 import org.hyperledger.besu.ethereum.eth.sync.state.SyncState;
 
+import java.util.List;
 import java.util.Optional;
 import java.util.concurrent.CompletableFuture;
 
@@ -86,41 +87,24 @@ protected Optional<FastSyncState> fromBestPeer(final EthPeer peer) {
   }
 
   protected Optional<EthPeer> selectBestPeer() {
-    return ethContext
-        .getEthPeers()
-        .bestPeerMatchingCriteria(this::canPeerDeterminePivotBlock)
-        // Only select a pivot block number when we have a minimum number of height estimates
-        .filter(unused -> enoughFastSyncPeersArePresent());
-  }
-
-  private boolean enoughFastSyncPeersArePresent() {
-    final long peerCount = countPeersThatCanDeterminePivotBlock();
+    List<EthPeer> peers =
+        ethContext
+            .getEthPeers()
+            .streamAvailablePeers()
+            .filter((peer) -> peer.chainState().hasEstimatedHeight() && peer.isFullyValidated())
+            .toList();
+
+    // Only select a pivot block number when we have a minimum number of height estimates
     final int minPeerCount = syncConfig.getSyncMinimumPeerCount();
-    if (peerCount < minPeerCount) {
+    if (peers.size() < minPeerCount) {
       LOG.info(
           "Waiting for valid peers with chain height information.  {} / {} required peers currently available.",
-          peerCount,
+          peers.size(),
           minPeerCount);
-      return false;
+      return Optional.empty();
+    } else {
+      return peers.stream().max(ethContext.getEthPeers().getBestPeerComparator());
     }
-    return true;
-  }
-
-  private long countPeersThatCanDeterminePivotBlock() {
-    return ethContext
-        .getEthPeers()
-        .streamAvailablePeers()
-        .filter(this::canPeerDeterminePivotBlock)
-        .count();
-  }
-
-  private boolean canPeerDeterminePivotBlock(final EthPeer peer) {
-    LOG.debug(
-        "peer {} hasEstimatedHeight {} isFullyValidated? {}",
-        peer.getLoggableId(),
-        peer.chainState().hasEstimatedHeight(),
-        peer.isFullyValidated());
-    return peer.chainState().hasEstimatedHeight() && peer.isFullyValidated();
   }
 
   private long conservativelyEstimatedPivotBlock() {

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/fastsync/PivotSelectorFromPeersTest.java

@@ -0,0 +1,91 @@
+/*
+ * Copyright contributors to Besu.
+ *
+ * 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.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package org.hyperledger.besu.ethereum.eth.sync.fastsync;
+
+import org.hyperledger.besu.ethereum.eth.manager.ChainState;
+import org.hyperledger.besu.ethereum.eth.manager.EthContext;
+import org.hyperledger.besu.ethereum.eth.manager.EthPeer;
+import org.hyperledger.besu.ethereum.eth.manager.EthPeers;
+import org.hyperledger.besu.ethereum.eth.sync.SynchronizerConfiguration;
+import org.hyperledger.besu.ethereum.eth.sync.state.SyncState;
+
+import java.util.Optional;
+import java.util.stream.Stream;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.jupiter.MockitoExtension;
+import org.mockito.junit.jupiter.MockitoSettings;
+import org.mockito.quality.Strictness;
+
+@ExtendWith(MockitoExtension.class)
+@MockitoSettings(strictness = Strictness.LENIENT)
+public class PivotSelectorFromPeersTest {
+  private @Mock EthContext ethContext;
+  private @Mock EthPeers ethPeers;
+  private @Mock SyncState syncState;
+
+  private PivotSelectorFromPeers selector;
+
+  @BeforeEach
+  public void beforeTest() {
+    SynchronizerConfiguration syncConfig =
+        SynchronizerConfiguration.builder().syncMinimumPeerCount(2).syncPivotDistance(1).build();
+
+    selector = new PivotSelectorFromPeers(ethContext, syncConfig, syncState);
+  }
+
+  @Test
+  public void testSelectNewPivotBlock() {
+    EthPeer peer1 = mockPeer(true, 10, true);
+    EthPeer peer2 = mockPeer(true, 8, true);
+
+    Mockito.when(ethContext.getEthPeers()).thenReturn(ethPeers);
+    Mockito.when(ethPeers.streamAvailablePeers()).thenReturn(Stream.of(peer1, peer2));
+    Mockito.when(ethPeers.getBestPeerComparator())
+        .thenReturn((p1, ignored) -> p1 == peer1 ? 1 : -1);
+
+    Optional<FastSyncState> result = selector.selectNewPivotBlock();
+
+    Assertions.assertTrue(result.isPresent());
+    Assertions.assertEquals(9, result.get().getPivotBlockNumber().getAsLong());
+  }
+
+  @Test
+  public void testSelectNewPivotBlockWithInsufficientPeers() {
+    Mockito.when(ethContext.getEthPeers()).thenReturn(ethPeers);
+    Mockito.when(ethPeers.streamAvailablePeers()).thenReturn(Stream.empty());
+
+    Optional<FastSyncState> result = selector.selectNewPivotBlock();
+
+    Assertions.assertTrue(result.isEmpty());
+  }
+
+  private EthPeer mockPeer(
+      final boolean hasEstimatedHeight, final long chainHeight, final boolean isFullyValidated) {
+    EthPeer ethPeer = Mockito.mock(EthPeer.class);
+    ChainState chainState = Mockito.mock(ChainState.class);
+    Mockito.when(ethPeer.chainState()).thenReturn(chainState);
+    Mockito.when(chainState.hasEstimatedHeight()).thenReturn(hasEstimatedHeight);
+    Mockito.when(chainState.getEstimatedHeight()).thenReturn(chainHeight);
+    Mockito.when(ethPeer.isFullyValidated()).thenReturn(isFullyValidated);
+
+    return ethPeer;
+  }
+}