From 4b800165870e2761e7721cc8100c40a20aaaeab8 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Tue, 17 Sep 2024 13:22:06 +1000 Subject: [PATCH 01/49] 7311: Add PeerTask system for use in future PRs Signed-off-by: Matilda Clerke --- .../InvalidPeerTaskResponseException.java | 26 ++ .../peertask/NoAvailablePeerException.java | 17 ++ .../eth/manager/peertask/PeerManager.java | 64 +++++ .../eth/manager/peertask/PeerTask.java | 63 ++++ .../manager/peertask/PeerTaskBehavior.java | 20 ++ .../manager/peertask/PeerTaskExecutor.java | 157 ++++++++++ .../PeerTaskExecutorResponseCode.java | 24 ++ .../peertask/PeerTaskExecutorResult.java | 35 +++ .../peertask/PeerTaskRequestSender.java | 55 ++++ .../eth/manager/peertask/PeerManagerTest.java | 80 ++++++ .../peertask/PeerTaskExecutorTest.java | 268 ++++++++++++++++++ .../peertask/PeerTaskRequestSenderTest.java | 77 +++++ 12 files changed, 886 insertions(+) create mode 100644 ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/InvalidPeerTaskResponseException.java create mode 100644 ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/NoAvailablePeerException.java create mode 100644 ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManager.java create mode 100644 ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java create mode 100644 ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskBehavior.java create mode 100644 ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java create mode 100644 ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResponseCode.java create mode 100644 ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResult.java create mode 100644 ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSender.java create mode 100644 ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManagerTest.java create mode 100644 ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java create mode 100644 ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSenderTest.java diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/InvalidPeerTaskResponseException.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/InvalidPeerTaskResponseException.java new file mode 100644 index 00000000000..824c0860d70 --- /dev/null +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/InvalidPeerTaskResponseException.java @@ -0,0 +1,26 @@ +/* + * Copyright contributors to Hyperledger 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.manager.peertask; + +public class InvalidPeerTaskResponseException extends Exception { + + public InvalidPeerTaskResponseException() { + super(); + } + + public InvalidPeerTaskResponseException(final Throwable cause) { + super(cause); + } +} diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/NoAvailablePeerException.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/NoAvailablePeerException.java new file mode 100644 index 00000000000..40c600d6fb3 --- /dev/null +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/NoAvailablePeerException.java @@ -0,0 +1,17 @@ +/* + * Copyright contributors to Hyperledger 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.manager.peertask; + +public class NoAvailablePeerException extends Exception {} diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManager.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManager.java new file mode 100644 index 00000000000..fc5bc691b72 --- /dev/null +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManager.java @@ -0,0 +1,64 @@ +/* + * Copyright contributors to Hyperledger 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.manager.peertask; + +import org.hyperledger.besu.ethereum.eth.manager.EthPeer; +import org.hyperledger.besu.ethereum.p2p.peers.PeerId; + +import java.util.Collections; +import java.util.Comparator; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.function.Predicate; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** "Manages" the EthPeers for the PeerTaskExecutor */ +public class PeerManager { + private static final Logger LOG = LoggerFactory.getLogger(PeerManager.class); + + // use a synchronized map to ensure the map is never modified by multiple threads at once + private final Map ethPeersByPeerId = + Collections.synchronizedMap(new HashMap<>()); + + /** + * Gets the highest reputation peer matching the supplies filter + * + * @param filter a filter to match prospective peers with + * @return the highest reputation peer matching the supplies filter + * @throws NoAvailablePeerException If there are no suitable peers + */ + public EthPeer getPeer(final Predicate filter) throws NoAvailablePeerException { + LOG.trace("Getting peer from pool of {} peers", ethPeersByPeerId.size()); + return ethPeersByPeerId.values().stream() + .filter(filter) + .max(Comparator.naturalOrder()) + .orElseThrow(NoAvailablePeerException::new); + } + + public Optional getPeerByPeerId(final PeerId peerId) { + return Optional.ofNullable(ethPeersByPeerId.get(peerId)); + } + + public void addPeer(final EthPeer ethPeer) { + ethPeersByPeerId.put(ethPeer.getConnection().getPeer(), ethPeer); + } + + public void removePeer(final PeerId peerId) { + ethPeersByPeerId.remove(peerId); + } +} diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java new file mode 100644 index 00000000000..244908c9216 --- /dev/null +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java @@ -0,0 +1,63 @@ +/* + * Copyright contributors to Hyperledger 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.manager.peertask; + +import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; + +import java.util.Collection; + +/** + * Represents a task to be executed on an EthPeer by the PeerTaskExecutor + * + * @param The type of the result of this PeerTask + */ +public interface PeerTask { + /** + * Returns the SubProtocol used for this PeerTask + * + * @return the SubProtocol used for this PeerTask + */ + String getSubProtocol(); + + /** + * Gets the minimum required block number for a peer to have to successfully execute this task + * + * @return the minimum required block number for a peer to have to successfully execute this task + */ + long getRequiredBlockNumber(); + + /** + * Gets the request data to send to the EthPeer + * + * @return the request data to send to the EthPeer + */ + MessageData getRequestMessage(); + + /** + * Parses the MessageData response from the EthPeer + * + * @param messageData the response MessageData to be parsed + * @return a T built from the response MessageData + * @throws InvalidPeerTaskResponseException if the response messageData is invalid + */ + T parseResponse(MessageData messageData) throws InvalidPeerTaskResponseException; + + /** + * Gets the Collection of behaviors this task is expected to exhibit in the PeetTaskExecutor + * + * @return the Collection of behaviors this task is expected to exhibit in the PeetTaskExecutor + */ + Collection getPeerTaskBehaviors(); +} diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskBehavior.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskBehavior.java new file mode 100644 index 00000000000..fba9000a741 --- /dev/null +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskBehavior.java @@ -0,0 +1,20 @@ +/* + * Copyright contributors to Hyperledger 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.manager.peertask; + +public enum PeerTaskBehavior { + RETRY_WITH_SAME_PEER, + RETRY_WITH_OTHER_PEERS +} diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java new file mode 100644 index 00000000000..fabf88d05d9 --- /dev/null +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -0,0 +1,157 @@ +/* + * Copyright contributors to Hyperledger 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.manager.peertask; + +import org.hyperledger.besu.ethereum.eth.manager.EthPeer; +import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec; +import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; +import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; +import org.hyperledger.besu.metrics.BesuMetricCategory; +import org.hyperledger.besu.plugin.services.MetricsSystem; +import org.hyperledger.besu.plugin.services.metrics.LabelledMetric; +import org.hyperledger.besu.plugin.services.metrics.OperationTimer; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeoutException; +import java.util.function.Supplier; + +/** Manages the execution of PeerTasks, respecting their PeerTaskBehavior */ +public class PeerTaskExecutor { + private static final long[] WAIT_TIME_BEFORE_RETRY = {0, 20000, 5000}; + + private final PeerManager peerManager; + private final PeerTaskRequestSender requestSender; + private final Supplier protocolSpecSupplier; + private final LabelledMetric requestTimer; + + public PeerTaskExecutor( + final PeerManager peerManager, + final PeerTaskRequestSender requestSender, + final Supplier protocolSpecSupplier, + final MetricsSystem metricsSystem) { + this.peerManager = peerManager; + this.requestSender = requestSender; + this.protocolSpecSupplier = protocolSpecSupplier; + requestTimer = + metricsSystem.createLabelledTimer( + BesuMetricCategory.PEERS, "Peer Task Executor Request Time", "", "Task Class Name"); + } + + public PeerTaskExecutorResult execute(final PeerTask peerTask) { + PeerTaskExecutorResult executorResult; + int triesRemaining = + peerTask.getPeerTaskBehaviors().contains(PeerTaskBehavior.RETRY_WITH_OTHER_PEERS) ? 3 : 1; + final Collection usedEthPeers = new ArrayList<>(); + do { + EthPeer peer; + try { + peer = + peerManager.getPeer( + (candidatePeer) -> + isPeerUnused(candidatePeer, usedEthPeers) + && (protocolSpecSupplier.get().isPoS() + || isPeerHeightHighEnough( + candidatePeer, peerTask.getRequiredBlockNumber())) + && isPeerProtocolSuitable(candidatePeer, peerTask.getSubProtocol())); + usedEthPeers.add(peer); + executorResult = executeAgainstPeer(peerTask, peer); + } catch (NoAvailablePeerException e) { + executorResult = + new PeerTaskExecutorResult<>(null, PeerTaskExecutorResponseCode.NO_PEER_AVAILABLE); + } + } while (--triesRemaining > 0 + && executorResult.getResponseCode() != PeerTaskExecutorResponseCode.SUCCESS); + + return executorResult; + } + + public CompletableFuture> executeAsync(final PeerTask peerTask) { + return CompletableFuture.supplyAsync(() -> execute(peerTask)); + } + + public PeerTaskExecutorResult executeAgainstPeer( + final PeerTask peerTask, final EthPeer peer) { + MessageData requestMessageData = peerTask.getRequestMessage(); + PeerTaskExecutorResult executorResult; + int triesRemaining = + peerTask.getPeerTaskBehaviors().contains(PeerTaskBehavior.RETRY_WITH_SAME_PEER) ? 3 : 1; + do { + try { + + MessageData responseMessageData; + try (final OperationTimer.TimingContext timingContext = + requestTimer.labels(peerTask.getClass().getSimpleName()).startTimer()) { + responseMessageData = + requestSender.sendRequest(peerTask.getSubProtocol(), requestMessageData, peer); + } + T result = peerTask.parseResponse(responseMessageData); + peer.recordUsefulResponse(); + executorResult = new PeerTaskExecutorResult<>(result, PeerTaskExecutorResponseCode.SUCCESS); + + } catch (PeerConnection.PeerNotConnected e) { + executorResult = + new PeerTaskExecutorResult<>(null, PeerTaskExecutorResponseCode.PEER_DISCONNECTED); + + } catch (InterruptedException | TimeoutException e) { + peer.recordRequestTimeout(requestMessageData.getCode()); + executorResult = new PeerTaskExecutorResult<>(null, PeerTaskExecutorResponseCode.TIMEOUT); + + } catch (InvalidPeerTaskResponseException e) { + peer.recordUselessResponse(e.getMessage()); + executorResult = + new PeerTaskExecutorResult<>(null, PeerTaskExecutorResponseCode.INVALID_RESPONSE); + + } catch (ExecutionException e) { + executorResult = + new PeerTaskExecutorResult<>(null, PeerTaskExecutorResponseCode.INTERNAL_SERVER_ERROR); + } + } while (--triesRemaining > 0 + && executorResult.getResponseCode() != PeerTaskExecutorResponseCode.SUCCESS + && executorResult.getResponseCode() != PeerTaskExecutorResponseCode.PEER_DISCONNECTED + && sleepBetweenRetries(WAIT_TIME_BEFORE_RETRY[triesRemaining])); + + return executorResult; + } + + public CompletableFuture> executeAgainstPeerAsync( + final PeerTask peerTask, final EthPeer peer) { + return CompletableFuture.supplyAsync(() -> executeAgainstPeer(peerTask, peer)); + } + + private boolean sleepBetweenRetries(final long sleepTime) { + try { + Thread.sleep(sleepTime); + return true; + } catch (InterruptedException e) { + return false; + } + } + + private static boolean isPeerUnused( + final EthPeer ethPeer, final Collection usedEthPeers) { + return !usedEthPeers.contains(ethPeer); + } + + private static boolean isPeerHeightHighEnough(final EthPeer ethPeer, final long requiredHeight) { + return ethPeer.chainState().getEstimatedHeight() >= requiredHeight; + } + + private static boolean isPeerProtocolSuitable(final EthPeer ethPeer, final String protocol) { + return ethPeer.getProtocolName().equals(protocol); + } +} diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResponseCode.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResponseCode.java new file mode 100644 index 00000000000..327461de15a --- /dev/null +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResponseCode.java @@ -0,0 +1,24 @@ +/* + * Copyright contributors to Hyperledger 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.manager.peertask; + +public enum PeerTaskExecutorResponseCode { + SUCCESS, + NO_PEER_AVAILABLE, + PEER_DISCONNECTED, + INTERNAL_SERVER_ERROR, + TIMEOUT, + INVALID_RESPONSE +} diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResult.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResult.java new file mode 100644 index 00000000000..f89bc67f61f --- /dev/null +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResult.java @@ -0,0 +1,35 @@ +/* + * Copyright contributors to Hyperledger 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.manager.peertask; + +import java.util.Optional; + +public class PeerTaskExecutorResult { + private final Optional result; + private final PeerTaskExecutorResponseCode responseCode; + + public PeerTaskExecutorResult(final T result, final PeerTaskExecutorResponseCode responseCode) { + this.result = Optional.ofNullable(result); + this.responseCode = responseCode; + } + + public Optional getResult() { + return result; + } + + public PeerTaskExecutorResponseCode getResponseCode() { + return responseCode; + } +} diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSender.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSender.java new file mode 100644 index 00000000000..9d9ceed3214 --- /dev/null +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSender.java @@ -0,0 +1,55 @@ +/* + * Copyright contributors to Hyperledger 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.manager.peertask; + +import org.hyperledger.besu.ethereum.eth.manager.EthPeer; +import org.hyperledger.besu.ethereum.eth.manager.RequestManager.ResponseStream; +import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; +import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; + +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + +public class PeerTaskRequestSender { + private static final long DEFAULT_TIMEOUT_MS = 20_000; + + private final long timeoutMs; + + public PeerTaskRequestSender() { + this.timeoutMs = DEFAULT_TIMEOUT_MS; + } + + public PeerTaskRequestSender(final long timeoutMs) { + this.timeoutMs = timeoutMs; + } + + public MessageData sendRequest( + final String subProtocol, final MessageData requestMessageData, final EthPeer ethPeer) + throws PeerConnection.PeerNotConnected, + ExecutionException, + InterruptedException, + TimeoutException { + ResponseStream responseStream = + ethPeer.send(requestMessageData, subProtocol, ethPeer.getConnection()); + final CompletableFuture responseMessageDataFuture = new CompletableFuture<>(); + responseStream.then( + (boolean streamClosed, MessageData message, EthPeer peer) -> { + responseMessageDataFuture.complete(message); + }); + return responseMessageDataFuture.get(timeoutMs, TimeUnit.MILLISECONDS); + } +} diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManagerTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManagerTest.java new file mode 100644 index 00000000000..54e0b88b5f1 --- /dev/null +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManagerTest.java @@ -0,0 +1,80 @@ +/* + * Copyright contributors to Hyperledger 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.manager.peertask; + +import org.hyperledger.besu.ethereum.eth.manager.EthPeer; +import org.hyperledger.besu.ethereum.eth.manager.MockPeerConnection; +import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; +import org.hyperledger.besu.ethereum.p2p.rlpx.wire.Capability; + +import java.time.Clock; +import java.util.Collections; +import java.util.Set; + +import org.apache.tuweni.bytes.Bytes; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class PeerManagerTest { + + public PeerManager peerManager; + + @BeforeEach + public void beforeTest() { + peerManager = new PeerManager(); + } + + @Test + public void testGetPeer() throws NoAvailablePeerException { + EthPeer protocol1With5ReputationPeer = + createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol1", 5); + peerManager.addPeer(protocol1With5ReputationPeer); + EthPeer protocol1With4ReputationPeer = + createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol1", 4); + peerManager.addPeer(protocol1With4ReputationPeer); + EthPeer protocol2With50ReputationPeer = + createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol2", 50); + peerManager.addPeer(protocol2With50ReputationPeer); + EthPeer protocol2With4ReputationPeer = + createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol2", 4); + peerManager.addPeer(protocol2With4ReputationPeer); + + EthPeer result = peerManager.getPeer((p) -> p.getProtocolName().equals("protocol1")); + + Assertions.assertSame(protocol1With5ReputationPeer, result); + } + + private EthPeer createTestPeer( + final Set connectionCapabilities, + final String protocolName, + final int reputationAdjustment) { + PeerConnection peerConnection = new MockPeerConnection(connectionCapabilities); + EthPeer peer = + new EthPeer( + peerConnection, + protocolName, + null, + Collections.emptyList(), + 1, + Clock.systemUTC(), + Collections.emptyList(), + Bytes.EMPTY); + for (int i = 0; i < reputationAdjustment; i++) { + peer.getReputation().recordUsefulResponse(); + } + return peer; + } +} diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java new file mode 100644 index 00000000000..029cb19e10b --- /dev/null +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -0,0 +1,268 @@ +/* + * Copyright contributors to Hyperledger 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.manager.peertask; + +import org.hyperledger.besu.ethereum.eth.manager.EthPeer; +import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec; +import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; +import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; +import org.hyperledger.besu.metrics.noop.NoOpMetricsSystem; + +import java.util.Collections; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeoutException; +import java.util.function.Predicate; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.MockitoAnnotations; + +public class PeerTaskExecutorTest { + private @Mock PeerManager peerManager; + private @Mock PeerTaskRequestSender requestSender; + private @Mock ProtocolSpec protocolSpec; + private @Mock PeerTask peerTask; + private @Mock MessageData requestMessageData; + private @Mock MessageData responseMessageData; + private @Mock EthPeer ethPeer; + private AutoCloseable mockCloser; + + private PeerTaskExecutor peerTaskExecutor; + + @BeforeEach + public void beforeTest() { + mockCloser = MockitoAnnotations.openMocks(this); + peerTaskExecutor = + new PeerTaskExecutor( + peerManager, requestSender, () -> protocolSpec, new NoOpMetricsSystem()); + } + + @AfterEach + public void afterTest() throws Exception { + mockCloser.close(); + } + + @Test + public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndSuccessfulFlow() + throws PeerConnection.PeerNotConnected, + ExecutionException, + InterruptedException, + TimeoutException, + InvalidPeerTaskResponseException { + String subprotocol = "subprotocol"; + Object responseObject = new Object(); + + Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); + Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); + Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); + Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) + .thenReturn(responseMessageData); + Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); + + PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); + + Mockito.verify(ethPeer).recordUsefulResponse(); + + Assertions.assertNotNull(result); + Assertions.assertTrue(result.getResult().isPresent()); + Assertions.assertSame(responseObject, result.getResult().get()); + Assertions.assertEquals(PeerTaskExecutorResponseCode.SUCCESS, result.getResponseCode()); + } + + @Test + public void testExecuteAgainstPeerWithRetryBehaviorsAndSuccessfulFlowAfterFirstFailure() + throws PeerConnection.PeerNotConnected, + ExecutionException, + InterruptedException, + TimeoutException, + InvalidPeerTaskResponseException { + String subprotocol = "subprotocol"; + Object responseObject = new Object(); + int requestMessageDataCode = 123; + + Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); + Mockito.when(peerTask.getPeerTaskBehaviors()) + .thenReturn(List.of(PeerTaskBehavior.RETRY_WITH_SAME_PEER)); + + Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); + Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) + .thenThrow(new TimeoutException()) + .thenReturn(responseMessageData); + Mockito.when(requestMessageData.getCode()).thenReturn(requestMessageDataCode); + Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); + + PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); + + Mockito.verify(ethPeer).recordRequestTimeout(requestMessageDataCode); + Mockito.verify(ethPeer).recordUsefulResponse(); + + Assertions.assertNotNull(result); + Assertions.assertTrue(result.getResult().isPresent()); + Assertions.assertSame(responseObject, result.getResult().get()); + Assertions.assertEquals(PeerTaskExecutorResponseCode.SUCCESS, result.getResponseCode()); + } + + @Test + public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndPeerNotConnected() + throws PeerConnection.PeerNotConnected, + ExecutionException, + InterruptedException, + TimeoutException, + InvalidPeerTaskResponseException { + String subprotocol = "subprotocol"; + + Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); + Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); + Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); + Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) + .thenThrow(new PeerConnection.PeerNotConnected("")); + + PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); + + Assertions.assertNotNull(result); + Assertions.assertTrue(result.getResult().isEmpty()); + Assertions.assertEquals( + PeerTaskExecutorResponseCode.PEER_DISCONNECTED, result.getResponseCode()); + } + + @Test + public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndTimeoutException() + throws PeerConnection.PeerNotConnected, + ExecutionException, + InterruptedException, + TimeoutException, + InvalidPeerTaskResponseException { + String subprotocol = "subprotocol"; + int requestMessageDataCode = 123; + + Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); + Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); + Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); + Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) + .thenThrow(new TimeoutException()); + Mockito.when(requestMessageData.getCode()).thenReturn(requestMessageDataCode); + + PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); + + Mockito.verify(ethPeer).recordRequestTimeout(requestMessageDataCode); + + Assertions.assertNotNull(result); + Assertions.assertTrue(result.getResult().isEmpty()); + Assertions.assertEquals(PeerTaskExecutorResponseCode.TIMEOUT, result.getResponseCode()); + } + + @Test + public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndInvalidResponseMessage() + throws PeerConnection.PeerNotConnected, + ExecutionException, + InterruptedException, + TimeoutException, + InvalidPeerTaskResponseException { + String subprotocol = "subprotocol"; + + Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); + Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); + Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); + Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) + .thenReturn(responseMessageData); + Mockito.when(peerTask.parseResponse(responseMessageData)) + .thenThrow(new InvalidPeerTaskResponseException()); + + PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); + + Mockito.verify(ethPeer).recordUselessResponse(null); + + Assertions.assertNotNull(result); + Assertions.assertTrue(result.getResult().isEmpty()); + Assertions.assertEquals( + PeerTaskExecutorResponseCode.INVALID_RESPONSE, result.getResponseCode()); + } + + @Test + @SuppressWarnings("unchecked") + public void testExecuteWithNoPeerTaskBehaviorsAndSuccessFlow() + throws PeerConnection.PeerNotConnected, + ExecutionException, + InterruptedException, + TimeoutException, + InvalidPeerTaskResponseException, + NoAvailablePeerException { + String subprotocol = "subprotocol"; + Object responseObject = new Object(); + + Mockito.when(peerManager.getPeer(Mockito.any(Predicate.class))).thenReturn(ethPeer); + + Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); + Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); + Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); + Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) + .thenReturn(responseMessageData); + Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); + + PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); + + Mockito.verify(ethPeer).recordUsefulResponse(); + + Assertions.assertNotNull(result); + Assertions.assertTrue(result.getResult().isPresent()); + Assertions.assertSame(responseObject, result.getResult().get()); + Assertions.assertEquals(PeerTaskExecutorResponseCode.SUCCESS, result.getResponseCode()); + } + + @Test + @SuppressWarnings("unchecked") + public void testExecuteWithPeerSwitchingAndSuccessFlow() + throws PeerConnection.PeerNotConnected, + ExecutionException, + InterruptedException, + TimeoutException, + InvalidPeerTaskResponseException, + NoAvailablePeerException { + String subprotocol = "subprotocol"; + Object responseObject = new Object(); + int requestMessageDataCode = 123; + EthPeer peer2 = Mockito.mock(EthPeer.class); + + Mockito.when(peerManager.getPeer(Mockito.any(Predicate.class))) + .thenReturn(ethPeer) + .thenReturn(peer2); + + Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); + Mockito.when(peerTask.getPeerTaskBehaviors()) + .thenReturn(List.of(PeerTaskBehavior.RETRY_WITH_OTHER_PEERS)); + Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); + Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) + .thenThrow(new TimeoutException()); + Mockito.when(requestMessageData.getCode()).thenReturn(requestMessageDataCode); + Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, peer2)) + .thenReturn(responseMessageData); + Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); + + PeerTaskExecutorResult result = peerTaskExecutor.execute(peerTask); + + Mockito.verify(ethPeer).recordRequestTimeout(requestMessageDataCode); + Mockito.verify(peer2).recordUsefulResponse(); + + Assertions.assertNotNull(result); + Assertions.assertTrue(result.getResult().isPresent()); + Assertions.assertSame(responseObject, result.getResult().get()); + Assertions.assertEquals(PeerTaskExecutorResponseCode.SUCCESS, result.getResponseCode()); + } +} diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSenderTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSenderTest.java new file mode 100644 index 00000000000..9e161031f51 --- /dev/null +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSenderTest.java @@ -0,0 +1,77 @@ +/* + * Copyright contributors to Hyperledger 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.manager.peertask; + +import org.hyperledger.besu.ethereum.eth.manager.EthPeer; +import org.hyperledger.besu.ethereum.eth.manager.RequestManager; +import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; +import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; + +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeoutException; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.Mockito; + +public class PeerTaskRequestSenderTest { + + private PeerTaskRequestSender peerTaskRequestSender; + + @BeforeEach + public void beforeTest() { + peerTaskRequestSender = new PeerTaskRequestSender(); + } + + @Test + public void testSendRequest() + throws PeerConnection.PeerNotConnected, + ExecutionException, + InterruptedException, + TimeoutException { + String subprotocol = "subprotocol"; + MessageData requestMessageData = Mockito.mock(MessageData.class); + MessageData responseMessageData = Mockito.mock(MessageData.class); + EthPeer peer = Mockito.mock(EthPeer.class); + PeerConnection peerConnection = Mockito.mock(PeerConnection.class); + RequestManager.ResponseStream responseStream = + Mockito.mock(RequestManager.ResponseStream.class); + + Mockito.when(peer.getConnection()).thenReturn(peerConnection); + Mockito.when(peer.send(requestMessageData, subprotocol, peerConnection)) + .thenReturn(responseStream); + + CompletableFuture actualResponseMessageDataFuture = + CompletableFuture.supplyAsync( + () -> { + try { + return peerTaskRequestSender.sendRequest(subprotocol, requestMessageData, peer); + } catch (Exception e) { + throw new RuntimeException(e); + } + }); + Thread.sleep(500); + ArgumentCaptor responseCallbackArgumentCaptor = + ArgumentCaptor.forClass(RequestManager.ResponseCallback.class); + Mockito.verify(responseStream).then(responseCallbackArgumentCaptor.capture()); + RequestManager.ResponseCallback responseCallback = responseCallbackArgumentCaptor.getValue(); + responseCallback.exec(false, responseMessageData, peer); + + Assertions.assertSame(responseMessageData, actualResponseMessageDataFuture.get()); + } +} From a8d5a9f340f017ce70dad533006880faedd1d9ab Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Tue, 17 Sep 2024 13:25:07 +1000 Subject: [PATCH 02/49] 7311: Clean up some warnings Signed-off-by: Matilda Clerke --- .../ethereum/eth/manager/peertask/PeerTaskExecutorTest.java | 6 ++---- .../eth/manager/peertask/PeerTaskRequestSenderTest.java | 6 +----- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index 029cb19e10b..2c015425f1d 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -124,8 +124,7 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndPeerNotConnected() throws PeerConnection.PeerNotConnected, ExecutionException, InterruptedException, - TimeoutException, - InvalidPeerTaskResponseException { + TimeoutException { String subprotocol = "subprotocol"; Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); @@ -147,8 +146,7 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndTimeoutException() throws PeerConnection.PeerNotConnected, ExecutionException, InterruptedException, - TimeoutException, - InvalidPeerTaskResponseException { + TimeoutException { String subprotocol = "subprotocol"; int requestMessageDataCode = 123; diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSenderTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSenderTest.java index 9e161031f51..8bc52604db7 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSenderTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSenderTest.java @@ -21,7 +21,6 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; -import java.util.concurrent.TimeoutException; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; @@ -40,10 +39,7 @@ public void beforeTest() { @Test public void testSendRequest() - throws PeerConnection.PeerNotConnected, - ExecutionException, - InterruptedException, - TimeoutException { + throws PeerConnection.PeerNotConnected, ExecutionException, InterruptedException { String subprotocol = "subprotocol"; MessageData requestMessageData = Mockito.mock(MessageData.class); MessageData responseMessageData = Mockito.mock(MessageData.class); From 08c66fd916e7e0ddc8d3a6870416503eafb4fc0f Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Fri, 20 Sep 2024 14:49:41 +1000 Subject: [PATCH 03/49] 7311: Reduce timeout in PeerTaskRequestSender to 5s Signed-off-by: Matilda Clerke --- .../ethereum/eth/manager/peertask/PeerTaskRequestSender.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSender.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSender.java index 9d9ceed3214..77ff5e7251d 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSender.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSender.java @@ -25,7 +25,7 @@ import java.util.concurrent.TimeoutException; public class PeerTaskRequestSender { - private static final long DEFAULT_TIMEOUT_MS = 20_000; + private static final long DEFAULT_TIMEOUT_MS = 5_000; private final long timeoutMs; From 049cae271c69b2b0572215a0babf5b73393cf3f5 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Fri, 20 Sep 2024 15:13:09 +1000 Subject: [PATCH 04/49] 7311: Refactor PeerManager to be an interface Signed-off-by: Matilda Clerke --- .../manager/peertask/DefaultPeerManager.java | 64 +++++++++++++++++++ .../eth/manager/peertask/PeerManager.java | 51 +++++++-------- ...rTest.java => DefaultPeerManagerTest.java} | 6 +- 3 files changed, 89 insertions(+), 32 deletions(-) create mode 100644 ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerManager.java rename ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/{PeerManagerTest.java => DefaultPeerManagerTest.java} (95%) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerManager.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerManager.java new file mode 100644 index 00000000000..9424ee0b48f --- /dev/null +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerManager.java @@ -0,0 +1,64 @@ +/* + * Copyright contributors to Hyperledger 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.manager.peertask; + +import org.hyperledger.besu.ethereum.eth.manager.EthPeer; +import org.hyperledger.besu.ethereum.p2p.peers.PeerId; + +import java.util.Collections; +import java.util.Comparator; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.function.Predicate; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * This is a simple PeerManager implementation that can be used the default implementation in most + * situations + */ +public class DefaultPeerManager implements PeerManager { + private static final Logger LOG = LoggerFactory.getLogger(DefaultPeerManager.class); + + // use a synchronized map to ensure the map is never modified by multiple threads at once + private final Map ethPeersByPeerId = + Collections.synchronizedMap(new HashMap<>()); + + @Override + public EthPeer getPeer(final Predicate filter) throws NoAvailablePeerException { + LOG.trace("Getting peer from pool of {} peers", ethPeersByPeerId.size()); + return ethPeersByPeerId.values().stream() + .filter(filter) + .max(Comparator.naturalOrder()) + .orElseThrow(NoAvailablePeerException::new); + } + + @Override + public Optional getPeerByPeerId(final PeerId peerId) { + return Optional.ofNullable(ethPeersByPeerId.get(peerId)); + } + + @Override + public void addPeer(final EthPeer ethPeer) { + ethPeersByPeerId.put(ethPeer.getConnection().getPeer(), ethPeer); + } + + @Override + public void removePeer(final PeerId peerId) { + ethPeersByPeerId.remove(peerId); + } +} diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManager.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManager.java index fc5bc691b72..14323474a14 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManager.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManager.java @@ -17,23 +17,11 @@ import org.hyperledger.besu.ethereum.eth.manager.EthPeer; import org.hyperledger.besu.ethereum.p2p.peers.PeerId; -import java.util.Collections; -import java.util.Comparator; -import java.util.HashMap; -import java.util.Map; import java.util.Optional; import java.util.function.Predicate; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - /** "Manages" the EthPeers for the PeerTaskExecutor */ -public class PeerManager { - private static final Logger LOG = LoggerFactory.getLogger(PeerManager.class); - - // use a synchronized map to ensure the map is never modified by multiple threads at once - private final Map ethPeersByPeerId = - Collections.synchronizedMap(new HashMap<>()); +public interface PeerManager { /** * Gets the highest reputation peer matching the supplies filter @@ -42,23 +30,28 @@ public class PeerManager { * @return the highest reputation peer matching the supplies filter * @throws NoAvailablePeerException If there are no suitable peers */ - public EthPeer getPeer(final Predicate filter) throws NoAvailablePeerException { - LOG.trace("Getting peer from pool of {} peers", ethPeersByPeerId.size()); - return ethPeersByPeerId.values().stream() - .filter(filter) - .max(Comparator.naturalOrder()) - .orElseThrow(NoAvailablePeerException::new); - } + EthPeer getPeer(final Predicate filter) throws NoAvailablePeerException; - public Optional getPeerByPeerId(final PeerId peerId) { - return Optional.ofNullable(ethPeersByPeerId.get(peerId)); - } + /** + * Attempts to get the EthPeer identified by peerId + * + * @param peerId the peerId of the desired EthPeer + * @return An Optional\ containing the EthPeer identified by peerId if present in the + * PeerManager, or empty otherwise + */ + Optional getPeerByPeerId(final PeerId peerId); - public void addPeer(final EthPeer ethPeer) { - ethPeersByPeerId.put(ethPeer.getConnection().getPeer(), ethPeer); - } + /** + * Add the supplied EthPeer to the PeerManager + * + * @param ethPeer the EthPeer to be added to the PeerManager + */ + void addPeer(final EthPeer ethPeer); - public void removePeer(final PeerId peerId) { - ethPeersByPeerId.remove(peerId); - } + /** + * Remove the EthPeer identified by peerId from the PeerManager + * + * @param peerId the PeerId of the EthPeer to be removed from the PeerManager + */ + void removePeer(final PeerId peerId); } diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManagerTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerManagerTest.java similarity index 95% rename from ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManagerTest.java rename to ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerManagerTest.java index 54e0b88b5f1..5aa04f8f9a8 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManagerTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerManagerTest.java @@ -28,13 +28,13 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -public class PeerManagerTest { +public class DefaultPeerManagerTest { - public PeerManager peerManager; + public DefaultPeerManager peerManager; @BeforeEach public void beforeTest() { - peerManager = new PeerManager(); + peerManager = new DefaultPeerManager(); } @Test From ad86ae6e711eecd8a0499ea4992a9e5a086e3d9c Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Wed, 25 Sep 2024 16:23:15 +1000 Subject: [PATCH 05/49] 7311: Rename PeerManager to PeerSelector Signed-off-by: Matilda Clerke --- ...PeerManager.java => DefaultPeerSelector.java} | 6 +++--- .../{PeerManager.java => PeerSelector.java} | 16 ++++++++-------- .../eth/manager/peertask/PeerTaskExecutor.java | 8 ++++---- ...gerTest.java => DefaultPeerSelectorTest.java} | 16 ++++++++-------- .../manager/peertask/PeerTaskExecutorTest.java | 8 ++++---- 5 files changed, 27 insertions(+), 27 deletions(-) rename ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/{DefaultPeerManager.java => DefaultPeerSelector.java} (91%) rename ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/{PeerManager.java => PeerSelector.java} (80%) rename ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/{DefaultPeerManagerTest.java => DefaultPeerSelectorTest.java} (85%) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerManager.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java similarity index 91% rename from ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerManager.java rename to ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java index 9424ee0b48f..f3999631732 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerManager.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java @@ -28,11 +28,11 @@ import org.slf4j.LoggerFactory; /** - * This is a simple PeerManager implementation that can be used the default implementation in most + * This is a simple PeerSelector implementation that can be used the default implementation in most * situations */ -public class DefaultPeerManager implements PeerManager { - private static final Logger LOG = LoggerFactory.getLogger(DefaultPeerManager.class); +public class DefaultPeerSelector implements PeerSelector { + private static final Logger LOG = LoggerFactory.getLogger(DefaultPeerSelector.class); // use a synchronized map to ensure the map is never modified by multiple threads at once private final Map ethPeersByPeerId = diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManager.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java similarity index 80% rename from ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManager.java rename to ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java index 14323474a14..73af26ec4c3 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManager.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java @@ -20,11 +20,11 @@ import java.util.Optional; import java.util.function.Predicate; -/** "Manages" the EthPeers for the PeerTaskExecutor */ -public interface PeerManager { +/** Selects the EthPeers for the PeerTaskExecutor */ +public interface PeerSelector { /** - * Gets the highest reputation peer matching the supplies filter + * Gets the highest reputation peer matching the supplied filter * * @param filter a filter to match prospective peers with * @return the highest reputation peer matching the supplies filter @@ -37,21 +37,21 @@ public interface PeerManager { * * @param peerId the peerId of the desired EthPeer * @return An Optional\ containing the EthPeer identified by peerId if present in the - * PeerManager, or empty otherwise + * PeerSelector, or empty otherwise */ Optional getPeerByPeerId(final PeerId peerId); /** - * Add the supplied EthPeer to the PeerManager + * Add the supplied EthPeer to the PeerSelector * - * @param ethPeer the EthPeer to be added to the PeerManager + * @param ethPeer the EthPeer to be added to the PeerSelector */ void addPeer(final EthPeer ethPeer); /** - * Remove the EthPeer identified by peerId from the PeerManager + * Remove the EthPeer identified by peerId from the PeerSelector * - * @param peerId the PeerId of the EthPeer to be removed from the PeerManager + * @param peerId the PeerId of the EthPeer to be removed from the PeerSelector */ void removePeer(final PeerId peerId); } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index fabf88d05d9..ad56987efc6 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -34,17 +34,17 @@ public class PeerTaskExecutor { private static final long[] WAIT_TIME_BEFORE_RETRY = {0, 20000, 5000}; - private final PeerManager peerManager; + private final PeerSelector peerSelector; private final PeerTaskRequestSender requestSender; private final Supplier protocolSpecSupplier; private final LabelledMetric requestTimer; public PeerTaskExecutor( - final PeerManager peerManager, + final PeerSelector peerSelector, final PeerTaskRequestSender requestSender, final Supplier protocolSpecSupplier, final MetricsSystem metricsSystem) { - this.peerManager = peerManager; + this.peerSelector = peerSelector; this.requestSender = requestSender; this.protocolSpecSupplier = protocolSpecSupplier; requestTimer = @@ -61,7 +61,7 @@ public PeerTaskExecutorResult execute(final PeerTask peerTask) { EthPeer peer; try { peer = - peerManager.getPeer( + peerSelector.getPeer( (candidatePeer) -> isPeerUnused(candidatePeer, usedEthPeers) && (protocolSpecSupplier.get().isPoS() diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerManagerTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java similarity index 85% rename from ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerManagerTest.java rename to ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java index 5aa04f8f9a8..f3b0b27033c 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerManagerTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java @@ -28,31 +28,31 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -public class DefaultPeerManagerTest { +public class DefaultPeerSelectorTest { - public DefaultPeerManager peerManager; + public DefaultPeerSelector peerSelector; @BeforeEach public void beforeTest() { - peerManager = new DefaultPeerManager(); + peerSelector = new DefaultPeerSelector(); } @Test public void testGetPeer() throws NoAvailablePeerException { EthPeer protocol1With5ReputationPeer = createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol1", 5); - peerManager.addPeer(protocol1With5ReputationPeer); + peerSelector.addPeer(protocol1With5ReputationPeer); EthPeer protocol1With4ReputationPeer = createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol1", 4); - peerManager.addPeer(protocol1With4ReputationPeer); + peerSelector.addPeer(protocol1With4ReputationPeer); EthPeer protocol2With50ReputationPeer = createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol2", 50); - peerManager.addPeer(protocol2With50ReputationPeer); + peerSelector.addPeer(protocol2With50ReputationPeer); EthPeer protocol2With4ReputationPeer = createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol2", 4); - peerManager.addPeer(protocol2With4ReputationPeer); + peerSelector.addPeer(protocol2With4ReputationPeer); - EthPeer result = peerManager.getPeer((p) -> p.getProtocolName().equals("protocol1")); + EthPeer result = peerSelector.getPeer((p) -> p.getProtocolName().equals("protocol1")); Assertions.assertSame(protocol1With5ReputationPeer, result); } diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index 2c015425f1d..930f4325b6b 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -35,7 +35,7 @@ import org.mockito.MockitoAnnotations; public class PeerTaskExecutorTest { - private @Mock PeerManager peerManager; + private @Mock PeerSelector peerSelector; private @Mock PeerTaskRequestSender requestSender; private @Mock ProtocolSpec protocolSpec; private @Mock PeerTask peerTask; @@ -51,7 +51,7 @@ public void beforeTest() { mockCloser = MockitoAnnotations.openMocks(this); peerTaskExecutor = new PeerTaskExecutor( - peerManager, requestSender, () -> protocolSpec, new NoOpMetricsSystem()); + peerSelector, requestSender, () -> protocolSpec, new NoOpMetricsSystem()); } @AfterEach @@ -205,7 +205,7 @@ public void testExecuteWithNoPeerTaskBehaviorsAndSuccessFlow() String subprotocol = "subprotocol"; Object responseObject = new Object(); - Mockito.when(peerManager.getPeer(Mockito.any(Predicate.class))).thenReturn(ethPeer); + Mockito.when(peerSelector.getPeer(Mockito.any(Predicate.class))).thenReturn(ethPeer); Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); @@ -238,7 +238,7 @@ public void testExecuteWithPeerSwitchingAndSuccessFlow() int requestMessageDataCode = 123; EthPeer peer2 = Mockito.mock(EthPeer.class); - Mockito.when(peerManager.getPeer(Mockito.any(Predicate.class))) + Mockito.when(peerSelector.getPeer(Mockito.any(Predicate.class))) .thenReturn(ethPeer) .thenReturn(peer2); From 38f04ab5a1be4d130b3423703cec2779040a36ee Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Wed, 25 Sep 2024 16:33:04 +1000 Subject: [PATCH 06/49] 7311: Reword PeerSelector javadoc to avoid implementation details Signed-off-by: Matilda Clerke --- .../ethereum/eth/manager/peertask/DefaultPeerSelector.java | 7 +++++++ .../besu/ethereum/eth/manager/peertask/PeerSelector.java | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java index f3999631732..5d32a37bc85 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java @@ -38,6 +38,13 @@ public class DefaultPeerSelector implements PeerSelector { private final Map ethPeersByPeerId = Collections.synchronizedMap(new HashMap<>()); + /** + * Gets the highest reputation peer matching the supplied filter + * + * @param filter a filter to match prospective peers with + * @return the highest reputation peer matching the supplies filter + * @throws NoAvailablePeerException If there are no suitable peers + */ @Override public EthPeer getPeer(final Predicate filter) throws NoAvailablePeerException { LOG.trace("Getting peer from pool of {} peers", ethPeersByPeerId.size()); diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java index 73af26ec4c3..3f5589f93b2 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java @@ -24,10 +24,10 @@ public interface PeerSelector { /** - * Gets the highest reputation peer matching the supplied filter + * Gets a peer matching the supplied filter * * @param filter a filter to match prospective peers with - * @return the highest reputation peer matching the supplies filter + * @return a peer matching the supplied filter * @throws NoAvailablePeerException If there are no suitable peers */ EthPeer getPeer(final Predicate filter) throws NoAvailablePeerException; From 6de3fb366a8b920bd1a830b43ff6a6af96e53315 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Wed, 25 Sep 2024 16:36:51 +1000 Subject: [PATCH 07/49] 7311: Use ConcurrentHashMap in DefaultPeerSelector Signed-off-by: Matilda Clerke --- .../ethereum/eth/manager/peertask/DefaultPeerSelector.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java index 5d32a37bc85..37a6fd1895c 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java @@ -22,6 +22,7 @@ import java.util.HashMap; import java.util.Map; import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; import java.util.function.Predicate; import org.slf4j.Logger; @@ -34,9 +35,7 @@ public class DefaultPeerSelector implements PeerSelector { private static final Logger LOG = LoggerFactory.getLogger(DefaultPeerSelector.class); - // use a synchronized map to ensure the map is never modified by multiple threads at once - private final Map ethPeersByPeerId = - Collections.synchronizedMap(new HashMap<>()); + private final Map ethPeersByPeerId = new ConcurrentHashMap<>(); /** * Gets the highest reputation peer matching the supplied filter From da9cd438aed5f84954ebe66c640c7575d8cbc021 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Wed, 25 Sep 2024 16:40:10 +1000 Subject: [PATCH 08/49] 7311: Reword trace log in DefaultPeerSelector Signed-off-by: Matilda Clerke --- .../besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java index 37a6fd1895c..be6eed7fee7 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java @@ -46,7 +46,7 @@ public class DefaultPeerSelector implements PeerSelector { */ @Override public EthPeer getPeer(final Predicate filter) throws NoAvailablePeerException { - LOG.trace("Getting peer from pool of {} peers", ethPeersByPeerId.size()); + LOG.trace("Finding peer from pool of {} peers", ethPeersByPeerId.size()); return ethPeersByPeerId.values().stream() .filter(filter) .max(Comparator.naturalOrder()) From ce7d24582c1a443050003525abf92c8d80521581 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Wed, 25 Sep 2024 16:40:28 +1000 Subject: [PATCH 09/49] 7311: Remove unused imports Signed-off-by: Matilda Clerke --- .../besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java index be6eed7fee7..c334773d168 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java @@ -17,9 +17,7 @@ import org.hyperledger.besu.ethereum.eth.manager.EthPeer; import org.hyperledger.besu.ethereum.p2p.peers.PeerId; -import java.util.Collections; import java.util.Comparator; -import java.util.HashMap; import java.util.Map; import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; From c9eb22e614ca5ca2a15d149042a0806b129fedde Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Wed, 25 Sep 2024 16:51:37 +1000 Subject: [PATCH 10/49] 7311: Use a 1 second delay between retries in PeerTaskExecutor to match old implementation Signed-off-by: Matilda Clerke --- .../ethereum/eth/manager/peertask/PeerTaskExecutor.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index ad56987efc6..894568b8b72 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -32,7 +32,6 @@ /** Manages the execution of PeerTasks, respecting their PeerTaskBehavior */ public class PeerTaskExecutor { - private static final long[] WAIT_TIME_BEFORE_RETRY = {0, 20000, 5000}; private final PeerSelector peerSelector; private final PeerTaskRequestSender requestSender; @@ -123,7 +122,7 @@ public PeerTaskExecutorResult executeAgainstPeer( } while (--triesRemaining > 0 && executorResult.getResponseCode() != PeerTaskExecutorResponseCode.SUCCESS && executorResult.getResponseCode() != PeerTaskExecutorResponseCode.PEER_DISCONNECTED - && sleepBetweenRetries(WAIT_TIME_BEFORE_RETRY[triesRemaining])); + && sleepBetweenRetries()); return executorResult; } @@ -133,9 +132,10 @@ public CompletableFuture> executeAgainstPeerAsync( return CompletableFuture.supplyAsync(() -> executeAgainstPeer(peerTask, peer)); } - private boolean sleepBetweenRetries(final long sleepTime) { + private boolean sleepBetweenRetries() { try { - Thread.sleep(sleepTime); + //sleep for 1 second to match implemented wait between retries in AbstractRetryingPeerTask + Thread.sleep(1000); return true; } catch (InterruptedException e) { return false; From e2fda731928109306b85e0bd7ff5f84672d99e58 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Wed, 25 Sep 2024 16:54:15 +1000 Subject: [PATCH 11/49] 7311: Add testGetPeerButNoPeerMatchesFilter to DefaultPeerSelectorTest Signed-off-by: Matilda Clerke --- .../peertask/DefaultPeerSelectorTest.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java index f3b0b27033c..bc92da6fb00 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java @@ -57,6 +57,24 @@ public void testGetPeer() throws NoAvailablePeerException { Assertions.assertSame(protocol1With5ReputationPeer, result); } + @Test + public void testGetPeerButNoPeerMatchesFilter() throws NoAvailablePeerException { + EthPeer protocol1With5ReputationPeer = + createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol1", 5); + peerSelector.addPeer(protocol1With5ReputationPeer); + EthPeer protocol1With4ReputationPeer = + createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol1", 4); + peerSelector.addPeer(protocol1With4ReputationPeer); + EthPeer protocol2With50ReputationPeer = + createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol2", 50); + peerSelector.addPeer(protocol2With50ReputationPeer); + EthPeer protocol2With4ReputationPeer = + createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol2", 4); + peerSelector.addPeer(protocol2With4ReputationPeer); + + Assertions.assertThrows(NoAvailablePeerException.class, () -> peerSelector.getPeer((p) -> p.getProtocolName().equals("fake protocol"))); + } + private EthPeer createTestPeer( final Set connectionCapabilities, final String protocolName, From 608feceff4c7105209f8003e8e0eeab0d807b2e0 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Wed, 25 Sep 2024 16:54:54 +1000 Subject: [PATCH 12/49] 7311: Add testGetPeerButNoPeerMatchesFilter to DefaultPeerSelectorTest Signed-off-by: Matilda Clerke --- .../ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java index bc92da6fb00..16d53f474bc 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java @@ -58,7 +58,7 @@ public void testGetPeer() throws NoAvailablePeerException { } @Test - public void testGetPeerButNoPeerMatchesFilter() throws NoAvailablePeerException { + public void testGetPeerButNoPeerMatchesFilter() { EthPeer protocol1With5ReputationPeer = createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol1", 5); peerSelector.addPeer(protocol1With5ReputationPeer); From 2d0780055baeba3922e4b00c173c85c43f843b7b Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Wed, 25 Sep 2024 16:59:02 +1000 Subject: [PATCH 13/49] 7311: spotless Signed-off-by: Matilda Clerke --- .../eth/manager/peertask/PeerTaskExecutor.java | 2 +- .../manager/peertask/DefaultPeerSelectorTest.java | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 894568b8b72..15833989268 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -134,7 +134,7 @@ public CompletableFuture> executeAgainstPeerAsync( private boolean sleepBetweenRetries() { try { - //sleep for 1 second to match implemented wait between retries in AbstractRetryingPeerTask + // sleep for 1 second to match implemented wait between retries in AbstractRetryingPeerTask Thread.sleep(1000); return true; } catch (InterruptedException e) { diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java index 16d53f474bc..8913b80a457 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java @@ -60,19 +60,21 @@ public void testGetPeer() throws NoAvailablePeerException { @Test public void testGetPeerButNoPeerMatchesFilter() { EthPeer protocol1With5ReputationPeer = - createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol1", 5); + createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol1", 5); peerSelector.addPeer(protocol1With5ReputationPeer); EthPeer protocol1With4ReputationPeer = - createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol1", 4); + createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol1", 4); peerSelector.addPeer(protocol1With4ReputationPeer); EthPeer protocol2With50ReputationPeer = - createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol2", 50); + createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol2", 50); peerSelector.addPeer(protocol2With50ReputationPeer); EthPeer protocol2With4ReputationPeer = - createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol2", 4); + createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol2", 4); peerSelector.addPeer(protocol2With4ReputationPeer); - Assertions.assertThrows(NoAvailablePeerException.class, () -> peerSelector.getPeer((p) -> p.getProtocolName().equals("fake protocol"))); + Assertions.assertThrows( + NoAvailablePeerException.class, + () -> peerSelector.getPeer((p) -> p.getProtocolName().equals("fake protocol"))); } private EthPeer createTestPeer( From ad262979632ac98a57bccd95092f1660a27cb372 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Fri, 20 Sep 2024 16:04:33 +1000 Subject: [PATCH 14/49] 7311: Fix MetricsAcceptanceTest Signed-off-by: Matilda Clerke --- .../besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 15833989268..30703496dbc 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -48,7 +48,7 @@ public PeerTaskExecutor( this.protocolSpecSupplier = protocolSpecSupplier; requestTimer = metricsSystem.createLabelledTimer( - BesuMetricCategory.PEERS, "Peer Task Executor Request Time", "", "Task Class Name"); + BesuMetricCategory.PEERS, "PeerTaskExecutor:RequestTime", "Time taken to send a request", "className"); } public PeerTaskExecutorResult execute(final PeerTask peerTask) { From 96c803030c3ae3308ef8c593603a0ede6b0eb2ef Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Fri, 20 Sep 2024 16:20:29 +1000 Subject: [PATCH 15/49] 7311: Fix MetricsAcceptanceTest Signed-off-by: Matilda Clerke --- .../besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 30703496dbc..bd6445226b6 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -48,7 +48,10 @@ public PeerTaskExecutor( this.protocolSpecSupplier = protocolSpecSupplier; requestTimer = metricsSystem.createLabelledTimer( - BesuMetricCategory.PEERS, "PeerTaskExecutor:RequestTime", "Time taken to send a request", "className"); + BesuMetricCategory.PEERS, + "PeerTaskExecutor:RequestTime", + "Time taken to send a request", + "className"); } public PeerTaskExecutorResult execute(final PeerTask peerTask) { From b0f2ed024f59ce6b5846457a90a0beab3a6f2f67 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Thu, 26 Sep 2024 14:42:59 +1000 Subject: [PATCH 16/49] 7311: Modify PeerTaskExecutor metric to include response time from peer Signed-off-by: Matilda Clerke --- .../eth/manager/peertask/PeerTaskExecutor.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index bd6445226b6..76d5eb38394 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -50,7 +50,7 @@ public PeerTaskExecutor( metricsSystem.createLabelledTimer( BesuMetricCategory.PEERS, "PeerTaskExecutor:RequestTime", - "Time taken to send a request", + "Time taken to send a request and receive a response", "className"); } @@ -95,13 +95,14 @@ public PeerTaskExecutorResult executeAgainstPeer( do { try { - MessageData responseMessageData; + T result ; try (final OperationTimer.TimingContext timingContext = requestTimer.labels(peerTask.getClass().getSimpleName()).startTimer()) { - responseMessageData = - requestSender.sendRequest(peerTask.getSubProtocol(), requestMessageData, peer); + MessageData responseMessageData = + requestSender.sendRequest(peerTask.getSubProtocol(), requestMessageData, peer); + + result = peerTask.parseResponse(responseMessageData); } - T result = peerTask.parseResponse(responseMessageData); peer.recordUsefulResponse(); executorResult = new PeerTaskExecutorResult<>(result, PeerTaskExecutorResponseCode.SUCCESS); From 598b519c08992266ea19a8332af8ff5adae7a02b Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Thu, 26 Sep 2024 15:30:42 +1000 Subject: [PATCH 17/49] 7311: Use SubProtocol instead of subprotocol name string in PeerTask Signed-off-by: Matilda Clerke --- .../ethereum/eth/manager/peertask/PeerTask.java | 3 ++- .../eth/manager/peertask/PeerTaskExecutor.java | 9 +++++---- .../manager/peertask/PeerTaskRequestSender.java | 5 +++-- .../manager/peertask/PeerTaskExecutorTest.java | 17 ++++++++++------- .../peertask/PeerTaskRequestSenderTest.java | 6 ++++-- 5 files changed, 24 insertions(+), 16 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java index 244908c9216..7ae78754b22 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java @@ -15,6 +15,7 @@ package org.hyperledger.besu.ethereum.eth.manager.peertask; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; +import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; import java.util.Collection; @@ -29,7 +30,7 @@ public interface PeerTask { * * @return the SubProtocol used for this PeerTask */ - String getSubProtocol(); + SubProtocol getSubProtocol(); /** * Gets the minimum required block number for a peer to have to successfully execute this task diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 76d5eb38394..bab1a1f0bdd 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -18,6 +18,7 @@ import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec; import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; +import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; import org.hyperledger.besu.metrics.BesuMetricCategory; import org.hyperledger.besu.plugin.services.MetricsSystem; import org.hyperledger.besu.plugin.services.metrics.LabelledMetric; @@ -95,11 +96,11 @@ public PeerTaskExecutorResult executeAgainstPeer( do { try { - T result ; + T result; try (final OperationTimer.TimingContext timingContext = requestTimer.labels(peerTask.getClass().getSimpleName()).startTimer()) { MessageData responseMessageData = - requestSender.sendRequest(peerTask.getSubProtocol(), requestMessageData, peer); + requestSender.sendRequest(peerTask.getSubProtocol(), requestMessageData, peer); result = peerTask.parseResponse(responseMessageData); } @@ -155,7 +156,7 @@ private static boolean isPeerHeightHighEnough(final EthPeer ethPeer, final long return ethPeer.chainState().getEstimatedHeight() >= requiredHeight; } - private static boolean isPeerProtocolSuitable(final EthPeer ethPeer, final String protocol) { - return ethPeer.getProtocolName().equals(protocol); + private static boolean isPeerProtocolSuitable(final EthPeer ethPeer, final SubProtocol protocol) { + return ethPeer.getProtocolName().equals(protocol.getName()); } } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSender.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSender.java index 77ff5e7251d..7a597eca8e8 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSender.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSender.java @@ -18,6 +18,7 @@ import org.hyperledger.besu.ethereum.eth.manager.RequestManager.ResponseStream; import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; +import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; @@ -38,13 +39,13 @@ public PeerTaskRequestSender(final long timeoutMs) { } public MessageData sendRequest( - final String subProtocol, final MessageData requestMessageData, final EthPeer ethPeer) + final SubProtocol subProtocol, final MessageData requestMessageData, final EthPeer ethPeer) throws PeerConnection.PeerNotConnected, ExecutionException, InterruptedException, TimeoutException { ResponseStream responseStream = - ethPeer.send(requestMessageData, subProtocol, ethPeer.getConnection()); + ethPeer.send(requestMessageData, subProtocol.getName(), ethPeer.getConnection()); final CompletableFuture responseMessageDataFuture = new CompletableFuture<>(); responseStream.then( (boolean streamClosed, MessageData message, EthPeer peer) -> { diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index 930f4325b6b..413b68f77c5 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -18,6 +18,7 @@ import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec; import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; +import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; import org.hyperledger.besu.metrics.noop.NoOpMetricsSystem; import java.util.Collections; @@ -39,6 +40,7 @@ public class PeerTaskExecutorTest { private @Mock PeerTaskRequestSender requestSender; private @Mock ProtocolSpec protocolSpec; private @Mock PeerTask peerTask; + private @Mock SubProtocol subprotocol; private @Mock MessageData requestMessageData; private @Mock MessageData responseMessageData; private @Mock EthPeer ethPeer; @@ -66,12 +68,13 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndSuccessfulFlow() InterruptedException, TimeoutException, InvalidPeerTaskResponseException { - String subprotocol = "subprotocol"; + Object responseObject = new Object(); Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); + Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); @@ -93,7 +96,6 @@ public void testExecuteAgainstPeerWithRetryBehaviorsAndSuccessfulFlowAfterFirstF InterruptedException, TimeoutException, InvalidPeerTaskResponseException { - String subprotocol = "subprotocol"; Object responseObject = new Object(); int requestMessageDataCode = 123; @@ -102,6 +104,7 @@ public void testExecuteAgainstPeerWithRetryBehaviorsAndSuccessfulFlowAfterFirstF .thenReturn(List.of(PeerTaskBehavior.RETRY_WITH_SAME_PEER)); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); + Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenThrow(new TimeoutException()) .thenReturn(responseMessageData); @@ -125,11 +128,11 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndPeerNotConnected() ExecutionException, InterruptedException, TimeoutException { - String subprotocol = "subprotocol"; Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); + Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenThrow(new PeerConnection.PeerNotConnected("")); @@ -147,12 +150,12 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndTimeoutException() ExecutionException, InterruptedException, TimeoutException { - String subprotocol = "subprotocol"; int requestMessageDataCode = 123; Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); + Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenThrow(new TimeoutException()); Mockito.when(requestMessageData.getCode()).thenReturn(requestMessageDataCode); @@ -173,11 +176,11 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndInvalidResponseMessa InterruptedException, TimeoutException, InvalidPeerTaskResponseException { - String subprotocol = "subprotocol"; Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); + Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)) @@ -202,7 +205,6 @@ public void testExecuteWithNoPeerTaskBehaviorsAndSuccessFlow() TimeoutException, InvalidPeerTaskResponseException, NoAvailablePeerException { - String subprotocol = "subprotocol"; Object responseObject = new Object(); Mockito.when(peerSelector.getPeer(Mockito.any(Predicate.class))).thenReturn(ethPeer); @@ -210,6 +212,7 @@ public void testExecuteWithNoPeerTaskBehaviorsAndSuccessFlow() Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); + Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); @@ -233,7 +236,6 @@ public void testExecuteWithPeerSwitchingAndSuccessFlow() TimeoutException, InvalidPeerTaskResponseException, NoAvailablePeerException { - String subprotocol = "subprotocol"; Object responseObject = new Object(); int requestMessageDataCode = 123; EthPeer peer2 = Mockito.mock(EthPeer.class); @@ -246,6 +248,7 @@ public void testExecuteWithPeerSwitchingAndSuccessFlow() Mockito.when(peerTask.getPeerTaskBehaviors()) .thenReturn(List.of(PeerTaskBehavior.RETRY_WITH_OTHER_PEERS)); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); + Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenThrow(new TimeoutException()); Mockito.when(requestMessageData.getCode()).thenReturn(requestMessageDataCode); diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSenderTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSenderTest.java index 8bc52604db7..4041fb63037 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSenderTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSenderTest.java @@ -18,6 +18,7 @@ import org.hyperledger.besu.ethereum.eth.manager.RequestManager; import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; +import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; @@ -40,7 +41,7 @@ public void beforeTest() { @Test public void testSendRequest() throws PeerConnection.PeerNotConnected, ExecutionException, InterruptedException { - String subprotocol = "subprotocol"; + SubProtocol subprotocol = Mockito.mock(SubProtocol.class); MessageData requestMessageData = Mockito.mock(MessageData.class); MessageData responseMessageData = Mockito.mock(MessageData.class); EthPeer peer = Mockito.mock(EthPeer.class); @@ -49,7 +50,8 @@ public void testSendRequest() Mockito.mock(RequestManager.ResponseStream.class); Mockito.when(peer.getConnection()).thenReturn(peerConnection); - Mockito.when(peer.send(requestMessageData, subprotocol, peerConnection)) + Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); + Mockito.when(peer.send(requestMessageData, "subprotocol", peerConnection)) .thenReturn(responseStream); CompletableFuture actualResponseMessageDataFuture = From bc25b16dcd81f1cbbf94c4ee0486d639f182485c Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Thu, 26 Sep 2024 15:46:58 +1000 Subject: [PATCH 18/49] 7311: rename timing context to ignored to prevent intellij warnings Signed-off-by: Matilda Clerke --- .../besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index bab1a1f0bdd..fa375d0b626 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -97,7 +97,7 @@ public PeerTaskExecutorResult executeAgainstPeer( try { T result; - try (final OperationTimer.TimingContext timingContext = + try (final OperationTimer.TimingContext ignored = requestTimer.labels(peerTask.getClass().getSimpleName()).startTimer()) { MessageData responseMessageData = requestSender.sendRequest(peerTask.getSubProtocol(), requestMessageData, peer); From e31bb7003754a8d06d4f4304004ba04514b7b298 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Thu, 26 Sep 2024 16:10:05 +1000 Subject: [PATCH 19/49] 7311: Use constants for number of retries Signed-off-by: Matilda Clerke --- .../eth/manager/peertask/PeerTaskExecutor.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index fa375d0b626..6fdb89f8c6c 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -34,6 +34,9 @@ /** Manages the execution of PeerTasks, respecting their PeerTaskBehavior */ public class PeerTaskExecutor { + public static final int RETRIES_WITH_SAME_PEER = 3; + public static final int RETRIES_WITH_OTHER_PEER = 3; + public static final int NO_RETRIES = 1; private final PeerSelector peerSelector; private final PeerTaskRequestSender requestSender; private final Supplier protocolSpecSupplier; @@ -58,7 +61,9 @@ public PeerTaskExecutor( public PeerTaskExecutorResult execute(final PeerTask peerTask) { PeerTaskExecutorResult executorResult; int triesRemaining = - peerTask.getPeerTaskBehaviors().contains(PeerTaskBehavior.RETRY_WITH_OTHER_PEERS) ? 3 : 1; + peerTask.getPeerTaskBehaviors().contains(PeerTaskBehavior.RETRY_WITH_OTHER_PEERS) + ? RETRIES_WITH_OTHER_PEER + : NO_RETRIES; final Collection usedEthPeers = new ArrayList<>(); do { EthPeer peer; @@ -92,7 +97,9 @@ public PeerTaskExecutorResult executeAgainstPeer( MessageData requestMessageData = peerTask.getRequestMessage(); PeerTaskExecutorResult executorResult; int triesRemaining = - peerTask.getPeerTaskBehaviors().contains(PeerTaskBehavior.RETRY_WITH_SAME_PEER) ? 3 : 1; + peerTask.getPeerTaskBehaviors().contains(PeerTaskBehavior.RETRY_WITH_SAME_PEER) + ? RETRIES_WITH_SAME_PEER + : NO_RETRIES; do { try { From 41923d3c40ec303a2125241e13d31aec3101dafb Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Thu, 26 Sep 2024 16:37:28 +1000 Subject: [PATCH 20/49] 7311: Convert PeerTaskExecutorResult to a record Signed-off-by: Matilda Clerke --- .../manager/peertask/PeerTaskExecutor.java | 19 +++++----- .../peertask/PeerTaskExecutorResult.java | 22 +++--------- .../peertask/PeerTaskExecutorTest.java | 36 +++++++++---------- 3 files changed, 33 insertions(+), 44 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 6fdb89f8c6c..3187149a0eb 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -26,6 +26,7 @@ import java.util.ArrayList; import java.util.Collection; +import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeoutException; @@ -80,10 +81,10 @@ public PeerTaskExecutorResult execute(final PeerTask peerTask) { executorResult = executeAgainstPeer(peerTask, peer); } catch (NoAvailablePeerException e) { executorResult = - new PeerTaskExecutorResult<>(null, PeerTaskExecutorResponseCode.NO_PEER_AVAILABLE); + new PeerTaskExecutorResult<>(Optional.empty(), PeerTaskExecutorResponseCode.NO_PEER_AVAILABLE); } } while (--triesRemaining > 0 - && executorResult.getResponseCode() != PeerTaskExecutorResponseCode.SUCCESS); + && executorResult.responseCode() != PeerTaskExecutorResponseCode.SUCCESS); return executorResult; } @@ -112,28 +113,28 @@ public PeerTaskExecutorResult executeAgainstPeer( result = peerTask.parseResponse(responseMessageData); } peer.recordUsefulResponse(); - executorResult = new PeerTaskExecutorResult<>(result, PeerTaskExecutorResponseCode.SUCCESS); + executorResult = new PeerTaskExecutorResult<>(Optional.ofNullable(result), PeerTaskExecutorResponseCode.SUCCESS); } catch (PeerConnection.PeerNotConnected e) { executorResult = - new PeerTaskExecutorResult<>(null, PeerTaskExecutorResponseCode.PEER_DISCONNECTED); + new PeerTaskExecutorResult<>(Optional.empty(), PeerTaskExecutorResponseCode.PEER_DISCONNECTED); } catch (InterruptedException | TimeoutException e) { peer.recordRequestTimeout(requestMessageData.getCode()); - executorResult = new PeerTaskExecutorResult<>(null, PeerTaskExecutorResponseCode.TIMEOUT); + executorResult = new PeerTaskExecutorResult<>(Optional.empty(), PeerTaskExecutorResponseCode.TIMEOUT); } catch (InvalidPeerTaskResponseException e) { peer.recordUselessResponse(e.getMessage()); executorResult = - new PeerTaskExecutorResult<>(null, PeerTaskExecutorResponseCode.INVALID_RESPONSE); + new PeerTaskExecutorResult<>(Optional.empty(), PeerTaskExecutorResponseCode.INVALID_RESPONSE); } catch (ExecutionException e) { executorResult = - new PeerTaskExecutorResult<>(null, PeerTaskExecutorResponseCode.INTERNAL_SERVER_ERROR); + new PeerTaskExecutorResult<>(Optional.empty(), PeerTaskExecutorResponseCode.INTERNAL_SERVER_ERROR); } } while (--triesRemaining > 0 - && executorResult.getResponseCode() != PeerTaskExecutorResponseCode.SUCCESS - && executorResult.getResponseCode() != PeerTaskExecutorResponseCode.PEER_DISCONNECTED + && executorResult.responseCode() != PeerTaskExecutorResponseCode.SUCCESS + && executorResult.responseCode() != PeerTaskExecutorResponseCode.PEER_DISCONNECTED && sleepBetweenRetries()); return executorResult; diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResult.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResult.java index f89bc67f61f..ef528cd2ae5 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResult.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResult.java @@ -16,20 +16,8 @@ import java.util.Optional; -public class PeerTaskExecutorResult { - private final Optional result; - private final PeerTaskExecutorResponseCode responseCode; - - public PeerTaskExecutorResult(final T result, final PeerTaskExecutorResponseCode responseCode) { - this.result = Optional.ofNullable(result); - this.responseCode = responseCode; - } - - public Optional getResult() { - return result; - } - - public PeerTaskExecutorResponseCode getResponseCode() { - return responseCode; - } -} +public record PeerTaskExecutorResult ( + Optional result, + PeerTaskExecutorResponseCode responseCode +) +{} diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index 413b68f77c5..297ce08b05b 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -84,9 +84,9 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndSuccessfulFlow() Mockito.verify(ethPeer).recordUsefulResponse(); Assertions.assertNotNull(result); - Assertions.assertTrue(result.getResult().isPresent()); - Assertions.assertSame(responseObject, result.getResult().get()); - Assertions.assertEquals(PeerTaskExecutorResponseCode.SUCCESS, result.getResponseCode()); + Assertions.assertTrue(result.result().isPresent()); + Assertions.assertSame(responseObject, result.result().get()); + Assertions.assertEquals(PeerTaskExecutorResponseCode.SUCCESS, result.responseCode()); } @Test @@ -117,9 +117,9 @@ public void testExecuteAgainstPeerWithRetryBehaviorsAndSuccessfulFlowAfterFirstF Mockito.verify(ethPeer).recordUsefulResponse(); Assertions.assertNotNull(result); - Assertions.assertTrue(result.getResult().isPresent()); - Assertions.assertSame(responseObject, result.getResult().get()); - Assertions.assertEquals(PeerTaskExecutorResponseCode.SUCCESS, result.getResponseCode()); + Assertions.assertTrue(result.result().isPresent()); + Assertions.assertSame(responseObject, result.result().get()); + Assertions.assertEquals(PeerTaskExecutorResponseCode.SUCCESS, result.responseCode()); } @Test @@ -139,9 +139,9 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndPeerNotConnected() PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); Assertions.assertNotNull(result); - Assertions.assertTrue(result.getResult().isEmpty()); + Assertions.assertTrue(result.result().isEmpty()); Assertions.assertEquals( - PeerTaskExecutorResponseCode.PEER_DISCONNECTED, result.getResponseCode()); + PeerTaskExecutorResponseCode.PEER_DISCONNECTED, result.responseCode()); } @Test @@ -165,8 +165,8 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndTimeoutException() Mockito.verify(ethPeer).recordRequestTimeout(requestMessageDataCode); Assertions.assertNotNull(result); - Assertions.assertTrue(result.getResult().isEmpty()); - Assertions.assertEquals(PeerTaskExecutorResponseCode.TIMEOUT, result.getResponseCode()); + Assertions.assertTrue(result.result().isEmpty()); + Assertions.assertEquals(PeerTaskExecutorResponseCode.TIMEOUT, result.responseCode()); } @Test @@ -191,9 +191,9 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndInvalidResponseMessa Mockito.verify(ethPeer).recordUselessResponse(null); Assertions.assertNotNull(result); - Assertions.assertTrue(result.getResult().isEmpty()); + Assertions.assertTrue(result.result().isEmpty()); Assertions.assertEquals( - PeerTaskExecutorResponseCode.INVALID_RESPONSE, result.getResponseCode()); + PeerTaskExecutorResponseCode.INVALID_RESPONSE, result.responseCode()); } @Test @@ -222,9 +222,9 @@ public void testExecuteWithNoPeerTaskBehaviorsAndSuccessFlow() Mockito.verify(ethPeer).recordUsefulResponse(); Assertions.assertNotNull(result); - Assertions.assertTrue(result.getResult().isPresent()); - Assertions.assertSame(responseObject, result.getResult().get()); - Assertions.assertEquals(PeerTaskExecutorResponseCode.SUCCESS, result.getResponseCode()); + Assertions.assertTrue(result.result().isPresent()); + Assertions.assertSame(responseObject, result.result().get()); + Assertions.assertEquals(PeerTaskExecutorResponseCode.SUCCESS, result.responseCode()); } @Test @@ -262,8 +262,8 @@ public void testExecuteWithPeerSwitchingAndSuccessFlow() Mockito.verify(peer2).recordUsefulResponse(); Assertions.assertNotNull(result); - Assertions.assertTrue(result.getResult().isPresent()); - Assertions.assertSame(responseObject, result.getResult().get()); - Assertions.assertEquals(PeerTaskExecutorResponseCode.SUCCESS, result.getResponseCode()); + Assertions.assertTrue(result.result().isPresent()); + Assertions.assertSame(responseObject, result.result().get()); + Assertions.assertEquals(PeerTaskExecutorResponseCode.SUCCESS, result.responseCode()); } } From 720f94ef50d4418ee500f69f9865617a0e42a7eb Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Mon, 30 Sep 2024 08:13:56 +1000 Subject: [PATCH 21/49] 7311: Rename PeerTaskBehavior to PeerTaskRetryBehavior Signed-off-by: Matilda Clerke --- .../besu/ethereum/eth/manager/peertask/PeerTask.java | 2 +- .../ethereum/eth/manager/peertask/PeerTaskExecutor.java | 6 +++--- .../{PeerTaskBehavior.java => PeerTaskRetryBehavior.java} | 2 +- .../ethereum/eth/manager/peertask/PeerTaskExecutorTest.java | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) rename ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/{PeerTaskBehavior.java => PeerTaskRetryBehavior.java} (95%) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java index 7ae78754b22..36bd03531bd 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java @@ -60,5 +60,5 @@ public interface PeerTask { * * @return the Collection of behaviors this task is expected to exhibit in the PeetTaskExecutor */ - Collection getPeerTaskBehaviors(); + Collection getPeerTaskBehaviors(); } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 3187149a0eb..caef72ba76e 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -32,7 +32,7 @@ import java.util.concurrent.TimeoutException; import java.util.function.Supplier; -/** Manages the execution of PeerTasks, respecting their PeerTaskBehavior */ +/** Manages the execution of PeerTasks, respecting their PeerTaskRetryBehavior */ public class PeerTaskExecutor { public static final int RETRIES_WITH_SAME_PEER = 3; @@ -62,7 +62,7 @@ public PeerTaskExecutor( public PeerTaskExecutorResult execute(final PeerTask peerTask) { PeerTaskExecutorResult executorResult; int triesRemaining = - peerTask.getPeerTaskBehaviors().contains(PeerTaskBehavior.RETRY_WITH_OTHER_PEERS) + peerTask.getPeerTaskBehaviors().contains(PeerTaskRetryBehavior.RETRY_WITH_OTHER_PEERS) ? RETRIES_WITH_OTHER_PEER : NO_RETRIES; final Collection usedEthPeers = new ArrayList<>(); @@ -98,7 +98,7 @@ public PeerTaskExecutorResult executeAgainstPeer( MessageData requestMessageData = peerTask.getRequestMessage(); PeerTaskExecutorResult executorResult; int triesRemaining = - peerTask.getPeerTaskBehaviors().contains(PeerTaskBehavior.RETRY_WITH_SAME_PEER) + peerTask.getPeerTaskBehaviors().contains(PeerTaskRetryBehavior.RETRY_WITH_SAME_PEER) ? RETRIES_WITH_SAME_PEER : NO_RETRIES; do { diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskBehavior.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRetryBehavior.java similarity index 95% rename from ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskBehavior.java rename to ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRetryBehavior.java index fba9000a741..53e2def6612 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskBehavior.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRetryBehavior.java @@ -14,7 +14,7 @@ */ package org.hyperledger.besu.ethereum.eth.manager.peertask; -public enum PeerTaskBehavior { +public enum PeerTaskRetryBehavior { RETRY_WITH_SAME_PEER, RETRY_WITH_OTHER_PEERS } diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index 297ce08b05b..0077c904188 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -101,7 +101,7 @@ public void testExecuteAgainstPeerWithRetryBehaviorsAndSuccessfulFlowAfterFirstF Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); Mockito.when(peerTask.getPeerTaskBehaviors()) - .thenReturn(List.of(PeerTaskBehavior.RETRY_WITH_SAME_PEER)); + .thenReturn(List.of(PeerTaskRetryBehavior.RETRY_WITH_SAME_PEER)); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); @@ -246,7 +246,7 @@ public void testExecuteWithPeerSwitchingAndSuccessFlow() Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); Mockito.when(peerTask.getPeerTaskBehaviors()) - .thenReturn(List.of(PeerTaskBehavior.RETRY_WITH_OTHER_PEERS)); + .thenReturn(List.of(PeerTaskRetryBehavior.RETRY_WITH_OTHER_PEERS)); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) From 7d845b327b42b4f7b37716b199052111e0309116 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Mon, 30 Sep 2024 15:57:23 +1000 Subject: [PATCH 22/49] 7311: Move peer selection logic to PeerSelector Signed-off-by: Matilda Clerke --- .../manager/peertask/DefaultPeerSelector.java | 38 ++++- .../eth/manager/peertask/PeerSelector.java | 23 ++- .../manager/peertask/PeerTaskExecutor.java | 49 +++--- .../peertask/PeerTaskExecutorResult.java | 7 +- .../peertask/DefaultPeerSelectorTest.java | 139 +++++++++++------- .../peertask/PeerTaskExecutorTest.java | 22 +-- 6 files changed, 168 insertions(+), 110 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java index c334773d168..41d2e9b700b 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java @@ -15,13 +15,17 @@ package org.hyperledger.besu.ethereum.eth.manager.peertask; import org.hyperledger.besu.ethereum.eth.manager.EthPeer; +import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec; import org.hyperledger.besu.ethereum.p2p.peers.PeerId; +import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; +import java.util.Collection; import java.util.Comparator; import java.util.Map; import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; import java.util.function.Predicate; +import java.util.function.Supplier; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -33,8 +37,13 @@ public class DefaultPeerSelector implements PeerSelector { private static final Logger LOG = LoggerFactory.getLogger(DefaultPeerSelector.class); + private final Supplier protocolSpecSupplier; private final Map ethPeersByPeerId = new ConcurrentHashMap<>(); + public DefaultPeerSelector(final Supplier protocolSpecSupplier) { + this.protocolSpecSupplier = protocolSpecSupplier; + } + /** * Gets the highest reputation peer matching the supplied filter * @@ -42,8 +51,7 @@ public class DefaultPeerSelector implements PeerSelector { * @return the highest reputation peer matching the supplies filter * @throws NoAvailablePeerException If there are no suitable peers */ - @Override - public EthPeer getPeer(final Predicate filter) throws NoAvailablePeerException { + private EthPeer getPeer(final Predicate filter) throws NoAvailablePeerException { LOG.trace("Finding peer from pool of {} peers", ethPeersByPeerId.size()); return ethPeersByPeerId.values().stream() .filter(filter) @@ -51,6 +59,20 @@ public EthPeer getPeer(final Predicate filter) throws NoAvailablePeerEx .orElseThrow(NoAvailablePeerException::new); } + @Override + public EthPeer getPeer( + final Collection usedEthPeers, + final long requiredPeerHeight, + final SubProtocol requiredSubProtocol) + throws NoAvailablePeerException { + return getPeer( + (candidatePeer) -> + isPeerUnused(candidatePeer, usedEthPeers) + && (protocolSpecSupplier.get().isPoS() + || isPeerHeightHighEnough(candidatePeer, requiredPeerHeight)) + && isPeerProtocolSuitable(candidatePeer, requiredSubProtocol)); + } + @Override public Optional getPeerByPeerId(final PeerId peerId) { return Optional.ofNullable(ethPeersByPeerId.get(peerId)); @@ -65,4 +87,16 @@ public void addPeer(final EthPeer ethPeer) { public void removePeer(final PeerId peerId) { ethPeersByPeerId.remove(peerId); } + + private boolean isPeerUnused(final EthPeer ethPeer, final Collection usedEthPeers) { + return !usedEthPeers.contains(ethPeer); + } + + private boolean isPeerHeightHighEnough(final EthPeer ethPeer, final long requiredHeight) { + return ethPeer.chainState().getEstimatedHeight() >= requiredHeight; + } + + private boolean isPeerProtocolSuitable(final EthPeer ethPeer, final SubProtocol protocol) { + return ethPeer.getProtocolName().equals(protocol.getName()); + } } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java index 3f5589f93b2..93d98a193b1 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java @@ -16,21 +16,28 @@ import org.hyperledger.besu.ethereum.eth.manager.EthPeer; import org.hyperledger.besu.ethereum.p2p.peers.PeerId; +import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; +import java.util.Collection; import java.util.Optional; -import java.util.function.Predicate; /** Selects the EthPeers for the PeerTaskExecutor */ public interface PeerSelector { /** - * Gets a peer matching the supplied filter + * Gets a peer with the requiredPeerHeight (if not PoS), and with the requiredSubProtocol, and + * which is not in the supplied collection of usedEthPeers * - * @param filter a filter to match prospective peers with - * @return a peer matching the supplied filter + * @param usedEthPeers a collection of EthPeers to be excluded from selection because they have + * already been used + * @param requiredPeerHeight the minimum peer height required of the selected peer + * @param requiredSubProtocol the SubProtocol required of the peer + * @return a peer matching the supplied conditions * @throws NoAvailablePeerException If there are no suitable peers */ - EthPeer getPeer(final Predicate filter) throws NoAvailablePeerException; + EthPeer getPeer( + Collection usedEthPeers, long requiredPeerHeight, SubProtocol requiredSubProtocol) + throws NoAvailablePeerException; /** * Attempts to get the EthPeer identified by peerId @@ -39,19 +46,19 @@ public interface PeerSelector { * @return An Optional\ containing the EthPeer identified by peerId if present in the * PeerSelector, or empty otherwise */ - Optional getPeerByPeerId(final PeerId peerId); + Optional getPeerByPeerId(PeerId peerId); /** * Add the supplied EthPeer to the PeerSelector * * @param ethPeer the EthPeer to be added to the PeerSelector */ - void addPeer(final EthPeer ethPeer); + void addPeer(EthPeer ethPeer); /** * Remove the EthPeer identified by peerId from the PeerSelector * * @param peerId the PeerId of the EthPeer to be removed from the PeerSelector */ - void removePeer(final PeerId peerId); + void removePeer(PeerId peerId); } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index caef72ba76e..5a71a8f2608 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -18,14 +18,13 @@ import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec; import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; -import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; import org.hyperledger.besu.metrics.BesuMetricCategory; import org.hyperledger.besu.plugin.services.MetricsSystem; import org.hyperledger.besu.plugin.services.metrics.LabelledMetric; import org.hyperledger.besu.plugin.services.metrics.OperationTimer; -import java.util.ArrayList; import java.util.Collection; +import java.util.HashSet; import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; @@ -40,17 +39,15 @@ public class PeerTaskExecutor { public static final int NO_RETRIES = 1; private final PeerSelector peerSelector; private final PeerTaskRequestSender requestSender; - private final Supplier protocolSpecSupplier; + private final LabelledMetric requestTimer; public PeerTaskExecutor( final PeerSelector peerSelector, final PeerTaskRequestSender requestSender, - final Supplier protocolSpecSupplier, final MetricsSystem metricsSystem) { this.peerSelector = peerSelector; this.requestSender = requestSender; - this.protocolSpecSupplier = protocolSpecSupplier; requestTimer = metricsSystem.createLabelledTimer( BesuMetricCategory.PEERS, @@ -65,23 +62,19 @@ public PeerTaskExecutorResult execute(final PeerTask peerTask) { peerTask.getPeerTaskBehaviors().contains(PeerTaskRetryBehavior.RETRY_WITH_OTHER_PEERS) ? RETRIES_WITH_OTHER_PEER : NO_RETRIES; - final Collection usedEthPeers = new ArrayList<>(); + final Collection usedEthPeers = new HashSet<>(); do { EthPeer peer; try { peer = peerSelector.getPeer( - (candidatePeer) -> - isPeerUnused(candidatePeer, usedEthPeers) - && (protocolSpecSupplier.get().isPoS() - || isPeerHeightHighEnough( - candidatePeer, peerTask.getRequiredBlockNumber())) - && isPeerProtocolSuitable(candidatePeer, peerTask.getSubProtocol())); + usedEthPeers, peerTask.getRequiredBlockNumber(), peerTask.getSubProtocol()); usedEthPeers.add(peer); executorResult = executeAgainstPeer(peerTask, peer); } catch (NoAvailablePeerException e) { executorResult = - new PeerTaskExecutorResult<>(Optional.empty(), PeerTaskExecutorResponseCode.NO_PEER_AVAILABLE); + new PeerTaskExecutorResult<>( + Optional.empty(), PeerTaskExecutorResponseCode.NO_PEER_AVAILABLE); } } while (--triesRemaining > 0 && executorResult.responseCode() != PeerTaskExecutorResponseCode.SUCCESS); @@ -103,7 +96,6 @@ public PeerTaskExecutorResult executeAgainstPeer( : NO_RETRIES; do { try { - T result; try (final OperationTimer.TimingContext ignored = requestTimer.labels(peerTask.getClass().getSimpleName()).startTimer()) { @@ -113,24 +105,30 @@ public PeerTaskExecutorResult executeAgainstPeer( result = peerTask.parseResponse(responseMessageData); } peer.recordUsefulResponse(); - executorResult = new PeerTaskExecutorResult<>(Optional.ofNullable(result), PeerTaskExecutorResponseCode.SUCCESS); + executorResult = + new PeerTaskExecutorResult<>( + Optional.ofNullable(result), PeerTaskExecutorResponseCode.SUCCESS); } catch (PeerConnection.PeerNotConnected e) { executorResult = - new PeerTaskExecutorResult<>(Optional.empty(), PeerTaskExecutorResponseCode.PEER_DISCONNECTED); + new PeerTaskExecutorResult<>( + Optional.empty(), PeerTaskExecutorResponseCode.PEER_DISCONNECTED); } catch (InterruptedException | TimeoutException e) { peer.recordRequestTimeout(requestMessageData.getCode()); - executorResult = new PeerTaskExecutorResult<>(Optional.empty(), PeerTaskExecutorResponseCode.TIMEOUT); + executorResult = + new PeerTaskExecutorResult<>(Optional.empty(), PeerTaskExecutorResponseCode.TIMEOUT); } catch (InvalidPeerTaskResponseException e) { peer.recordUselessResponse(e.getMessage()); executorResult = - new PeerTaskExecutorResult<>(Optional.empty(), PeerTaskExecutorResponseCode.INVALID_RESPONSE); + new PeerTaskExecutorResult<>( + Optional.empty(), PeerTaskExecutorResponseCode.INVALID_RESPONSE); } catch (ExecutionException e) { executorResult = - new PeerTaskExecutorResult<>(Optional.empty(), PeerTaskExecutorResponseCode.INTERNAL_SERVER_ERROR); + new PeerTaskExecutorResult<>( + Optional.empty(), PeerTaskExecutorResponseCode.INTERNAL_SERVER_ERROR); } } while (--triesRemaining > 0 && executorResult.responseCode() != PeerTaskExecutorResponseCode.SUCCESS @@ -154,17 +152,4 @@ private boolean sleepBetweenRetries() { return false; } } - - private static boolean isPeerUnused( - final EthPeer ethPeer, final Collection usedEthPeers) { - return !usedEthPeers.contains(ethPeer); - } - - private static boolean isPeerHeightHighEnough(final EthPeer ethPeer, final long requiredHeight) { - return ethPeer.chainState().getEstimatedHeight() >= requiredHeight; - } - - private static boolean isPeerProtocolSuitable(final EthPeer ethPeer, final SubProtocol protocol) { - return ethPeer.getProtocolName().equals(protocol.getName()); - } } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResult.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResult.java index ef528cd2ae5..86dec85c295 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResult.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResult.java @@ -16,8 +16,5 @@ import java.util.Optional; -public record PeerTaskExecutorResult ( - Optional result, - PeerTaskExecutorResponseCode responseCode -) -{} +public record PeerTaskExecutorResult( + Optional result, PeerTaskExecutorResponseCode responseCode) {} diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java index 8913b80a457..add2b1e612c 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java @@ -14,19 +14,23 @@ */ package org.hyperledger.besu.ethereum.eth.manager.peertask; +import org.hyperledger.besu.ethereum.eth.EthProtocol; +import org.hyperledger.besu.ethereum.eth.SnapProtocol; +import org.hyperledger.besu.ethereum.eth.manager.ChainState; import org.hyperledger.besu.ethereum.eth.manager.EthPeer; -import org.hyperledger.besu.ethereum.eth.manager.MockPeerConnection; +import org.hyperledger.besu.ethereum.eth.manager.PeerReputation; +import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec; +import org.hyperledger.besu.ethereum.p2p.peers.Peer; import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; -import org.hyperledger.besu.ethereum.p2p.rlpx.wire.Capability; +import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; -import java.time.Clock; -import java.util.Collections; +import java.util.HashSet; import java.util.Set; -import org.apache.tuweni.bytes.Bytes; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.Mockito; public class DefaultPeerSelectorTest { @@ -34,67 +38,98 @@ public class DefaultPeerSelectorTest { @BeforeEach public void beforeTest() { - peerSelector = new DefaultPeerSelector(); + ProtocolSpec protocolSpec = Mockito.mock(ProtocolSpec.class); + Mockito.when(protocolSpec.isPoS()).thenReturn(false); + peerSelector = new DefaultPeerSelector(() -> protocolSpec); } @Test public void testGetPeer() throws NoAvailablePeerException { - EthPeer protocol1With5ReputationPeer = - createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol1", 5); - peerSelector.addPeer(protocol1With5ReputationPeer); - EthPeer protocol1With4ReputationPeer = - createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol1", 4); - peerSelector.addPeer(protocol1With4ReputationPeer); - EthPeer protocol2With50ReputationPeer = - createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol2", 50); - peerSelector.addPeer(protocol2With50ReputationPeer); - EthPeer protocol2With4ReputationPeer = - createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol2", 4); - peerSelector.addPeer(protocol2With4ReputationPeer); - - EthPeer result = peerSelector.getPeer((p) -> p.getProtocolName().equals("protocol1")); - - Assertions.assertSame(protocol1With5ReputationPeer, result); + EthPeer expectedPeer = createTestPeer(10, EthProtocol.get(), 5); + peerSelector.addPeer(expectedPeer); + EthPeer excludedForLowChainHeightPeer = createTestPeer(5, EthProtocol.get(), 50); + peerSelector.addPeer(excludedForLowChainHeightPeer); + EthPeer excludedForWrongProtocolPeer = createTestPeer(10, SnapProtocol.get(), 50); + peerSelector.addPeer(excludedForWrongProtocolPeer); + EthPeer excludedForLowReputationPeer = createTestPeer(10, EthProtocol.get(), 1); + peerSelector.addPeer(excludedForLowReputationPeer); + EthPeer excludedForBeingAlreadyUsedPeer = createTestPeer(10, EthProtocol.get(), 50); + peerSelector.addPeer(excludedForBeingAlreadyUsedPeer); + + Set usedEthPeers = new HashSet<>(); + usedEthPeers.add(excludedForBeingAlreadyUsedPeer); + + EthPeer result = peerSelector.getPeer(usedEthPeers, 10, EthProtocol.get()); + + Assertions.assertSame(expectedPeer, result); } @Test public void testGetPeerButNoPeerMatchesFilter() { - EthPeer protocol1With5ReputationPeer = - createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol1", 5); - peerSelector.addPeer(protocol1With5ReputationPeer); - EthPeer protocol1With4ReputationPeer = - createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol1", 4); - peerSelector.addPeer(protocol1With4ReputationPeer); - EthPeer protocol2With50ReputationPeer = - createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol2", 50); - peerSelector.addPeer(protocol2With50ReputationPeer); - EthPeer protocol2With4ReputationPeer = - createTestPeer(Set.of(Capability.create("capability1", 1)), "protocol2", 4); - peerSelector.addPeer(protocol2With4ReputationPeer); + EthPeer expectedPeer = createTestPeer(10, EthProtocol.get(), 5); + peerSelector.addPeer(expectedPeer); + EthPeer excludedForLowChainHeightPeer = createTestPeer(5, EthProtocol.get(), 50); + peerSelector.addPeer(excludedForLowChainHeightPeer); + EthPeer excludedForWrongProtocolPeer = createTestPeer(10, SnapProtocol.get(), 50); + peerSelector.addPeer(excludedForWrongProtocolPeer); + EthPeer excludedForLowReputationPeer = createTestPeer(10, EthProtocol.get(), 1); + peerSelector.addPeer(excludedForLowReputationPeer); + EthPeer excludedForBeingAlreadyUsedPeer = createTestPeer(10, EthProtocol.get(), 50); + peerSelector.addPeer(excludedForBeingAlreadyUsedPeer); + + Set usedEthPeers = new HashSet<>(); + usedEthPeers.add(excludedForBeingAlreadyUsedPeer); Assertions.assertThrows( NoAvailablePeerException.class, - () -> peerSelector.getPeer((p) -> p.getProtocolName().equals("fake protocol"))); + () -> peerSelector.getPeer(usedEthPeers, 10, new MockSubProtocol())); } private EthPeer createTestPeer( - final Set connectionCapabilities, - final String protocolName, - final int reputationAdjustment) { - PeerConnection peerConnection = new MockPeerConnection(connectionCapabilities); - EthPeer peer = - new EthPeer( - peerConnection, - protocolName, - null, - Collections.emptyList(), - 1, - Clock.systemUTC(), - Collections.emptyList(), - Bytes.EMPTY); - for (int i = 0; i < reputationAdjustment; i++) { - peer.getReputation().recordUsefulResponse(); + final long chainHeight, final SubProtocol protocol, final int reputation) { + EthPeer ethPeer = Mockito.mock(EthPeer.class); + PeerConnection peerConnection = Mockito.mock(PeerConnection.class); + Peer peer = Mockito.mock(Peer.class); + ChainState chainState = Mockito.mock(ChainState.class); + PeerReputation peerReputation = Mockito.mock(PeerReputation.class); + + Mockito.when(ethPeer.getConnection()).thenReturn(peerConnection); + Mockito.when(peerConnection.getPeer()).thenReturn(peer); + Mockito.when(ethPeer.getProtocolName()).thenReturn(protocol.getName()); + Mockito.when(ethPeer.chainState()).thenReturn(chainState); + Mockito.when(chainState.getEstimatedHeight()).thenReturn(chainHeight); + Mockito.when(ethPeer.getReputation()).thenReturn(peerReputation); + Mockito.when(peerReputation.getScore()).thenReturn(reputation); + + Mockito.when(ethPeer.compareTo(Mockito.any(EthPeer.class))) + .thenAnswer( + (invocationOnMock) -> { + EthPeer otherPeer = invocationOnMock.getArgument(0, EthPeer.class); + return Integer.compare(reputation, otherPeer.getReputation().getScore()); + }); + return ethPeer; + } + + private static class MockSubProtocol implements SubProtocol { + + @Override + public String getName() { + return "Mock"; + } + + @Override + public int messageSpace(final int protocolVersion) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isValidMessageCode(final int protocolVersion, final int code) { + throw new UnsupportedOperationException(); + } + + @Override + public String messageName(final int protocolVersion, final int code) { + throw new UnsupportedOperationException(); } - return peer; } } diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index 0077c904188..a7b59145175 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -15,17 +15,16 @@ package org.hyperledger.besu.ethereum.eth.manager.peertask; import org.hyperledger.besu.ethereum.eth.manager.EthPeer; -import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec; import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; import org.hyperledger.besu.metrics.noop.NoOpMetricsSystem; +import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeoutException; -import java.util.function.Predicate; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; @@ -38,7 +37,6 @@ public class PeerTaskExecutorTest { private @Mock PeerSelector peerSelector; private @Mock PeerTaskRequestSender requestSender; - private @Mock ProtocolSpec protocolSpec; private @Mock PeerTask peerTask; private @Mock SubProtocol subprotocol; private @Mock MessageData requestMessageData; @@ -53,7 +51,7 @@ public void beforeTest() { mockCloser = MockitoAnnotations.openMocks(this); peerTaskExecutor = new PeerTaskExecutor( - peerSelector, requestSender, () -> protocolSpec, new NoOpMetricsSystem()); + peerSelector, requestSender, new NoOpMetricsSystem()); } @AfterEach @@ -140,8 +138,7 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndPeerNotConnected() Assertions.assertNotNull(result); Assertions.assertTrue(result.result().isEmpty()); - Assertions.assertEquals( - PeerTaskExecutorResponseCode.PEER_DISCONNECTED, result.responseCode()); + Assertions.assertEquals(PeerTaskExecutorResponseCode.PEER_DISCONNECTED, result.responseCode()); } @Test @@ -192,8 +189,7 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndInvalidResponseMessa Assertions.assertNotNull(result); Assertions.assertTrue(result.result().isEmpty()); - Assertions.assertEquals( - PeerTaskExecutorResponseCode.INVALID_RESPONSE, result.responseCode()); + Assertions.assertEquals(PeerTaskExecutorResponseCode.INVALID_RESPONSE, result.responseCode()); } @Test @@ -207,10 +203,13 @@ public void testExecuteWithNoPeerTaskBehaviorsAndSuccessFlow() NoAvailablePeerException { Object responseObject = new Object(); - Mockito.when(peerSelector.getPeer(Mockito.any(Predicate.class))).thenReturn(ethPeer); + Mockito.when( + peerSelector.getPeer(Mockito.any(Collection.class), Mockito.eq(10L), Mockito.eq(subprotocol))) + .thenReturn(ethPeer); Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); + Mockito.when(peerTask.getRequiredBlockNumber()).thenReturn(10L); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) @@ -240,15 +239,16 @@ public void testExecuteWithPeerSwitchingAndSuccessFlow() int requestMessageDataCode = 123; EthPeer peer2 = Mockito.mock(EthPeer.class); - Mockito.when(peerSelector.getPeer(Mockito.any(Predicate.class))) + Mockito.when( + peerSelector.getPeer(Mockito.any(Collection.class), Mockito.eq(10L), Mockito.eq(subprotocol))) .thenReturn(ethPeer) .thenReturn(peer2); Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); Mockito.when(peerTask.getPeerTaskBehaviors()) .thenReturn(List.of(PeerTaskRetryBehavior.RETRY_WITH_OTHER_PEERS)); + Mockito.when(peerTask.getRequiredBlockNumber()).thenReturn(10L); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); - Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenThrow(new TimeoutException()); Mockito.when(requestMessageData.getCode()).thenReturn(requestMessageDataCode); From 50c26f138b63203cf8254a9b46c582c91e192469 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Mon, 30 Sep 2024 16:02:30 +1000 Subject: [PATCH 23/49] 7311: spotless Signed-off-by: Matilda Clerke --- .../eth/manager/peertask/PeerTaskExecutor.java | 2 -- .../eth/manager/peertask/PeerTaskExecutorTest.java | 10 +++++----- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 5a71a8f2608..10c882e7e5a 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -15,7 +15,6 @@ package org.hyperledger.besu.ethereum.eth.manager.peertask; import org.hyperledger.besu.ethereum.eth.manager.EthPeer; -import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec; import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; import org.hyperledger.besu.metrics.BesuMetricCategory; @@ -29,7 +28,6 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeoutException; -import java.util.function.Supplier; /** Manages the execution of PeerTasks, respecting their PeerTaskRetryBehavior */ public class PeerTaskExecutor { diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index a7b59145175..15b1747bc7b 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -49,9 +49,7 @@ public class PeerTaskExecutorTest { @BeforeEach public void beforeTest() { mockCloser = MockitoAnnotations.openMocks(this); - peerTaskExecutor = - new PeerTaskExecutor( - peerSelector, requestSender, new NoOpMetricsSystem()); + peerTaskExecutor = new PeerTaskExecutor(peerSelector, requestSender, new NoOpMetricsSystem()); } @AfterEach @@ -204,7 +202,8 @@ public void testExecuteWithNoPeerTaskBehaviorsAndSuccessFlow() Object responseObject = new Object(); Mockito.when( - peerSelector.getPeer(Mockito.any(Collection.class), Mockito.eq(10L), Mockito.eq(subprotocol))) + peerSelector.getPeer( + Mockito.any(Collection.class), Mockito.eq(10L), Mockito.eq(subprotocol))) .thenReturn(ethPeer); Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); @@ -240,7 +239,8 @@ public void testExecuteWithPeerSwitchingAndSuccessFlow() EthPeer peer2 = Mockito.mock(EthPeer.class); Mockito.when( - peerSelector.getPeer(Mockito.any(Collection.class), Mockito.eq(10L), Mockito.eq(subprotocol))) + peerSelector.getPeer( + Mockito.any(Collection.class), Mockito.eq(10L), Mockito.eq(subprotocol))) .thenReturn(ethPeer) .thenReturn(peer2); From e63f4730c6473bad15a8e01004ae3cf5882ebc19 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Fri, 4 Oct 2024 08:55:24 +1000 Subject: [PATCH 24/49] 7311: Make changes as discussed in walkthrough meeting Remove DefaultPeerSelector, make EthPeers implement PeerSelector interface, and add PeerTask.getPeerRequirementFilter Signed-off-by: Matilda Clerke --- .../besu/ethereum/eth/manager/EthPeers.java | 17 ++- .../manager/peertask/DefaultPeerSelector.java | 102 ------------- .../eth/manager/peertask/PeerSelector.java | 30 +--- .../eth/manager/peertask/PeerTask.java | 16 ++- .../manager/peertask/PeerTaskExecutor.java | 17 ++- .../peertask/DefaultPeerSelectorTest.java | 135 ------------------ .../peertask/PeerTaskExecutorTest.java | 21 ++- 7 files changed, 50 insertions(+), 288 deletions(-) delete mode 100644 ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java delete mode 100644 ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java index d1a54d4d3d3..1b413251be7 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java @@ -17,6 +17,8 @@ import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.eth.SnapProtocol; import org.hyperledger.besu.ethereum.eth.manager.EthPeer.DisconnectCallback; +import org.hyperledger.besu.ethereum.eth.manager.exceptions.NoAvailablePeersException; +import org.hyperledger.besu.ethereum.eth.manager.peertask.PeerSelector; import org.hyperledger.besu.ethereum.eth.peervalidation.PeerValidator; import org.hyperledger.besu.ethereum.eth.sync.ChainHeadTracker; import org.hyperledger.besu.ethereum.eth.sync.SnapServerChecker; @@ -26,6 +28,7 @@ import org.hyperledger.besu.ethereum.forkid.ForkIdManager; import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec; import org.hyperledger.besu.ethereum.p2p.peers.Peer; +import org.hyperledger.besu.ethereum.p2p.peers.PeerId; import org.hyperledger.besu.ethereum.p2p.rlpx.RlpxAgent; import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.messages.DisconnectMessage; @@ -61,7 +64,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -public class EthPeers { +public class EthPeers implements PeerSelector { private static final Logger LOG = LoggerFactory.getLogger(EthPeers.class); public static final Comparator TOTAL_DIFFICULTY = Comparator.comparing((final EthPeer p) -> p.chainState().getEstimatedTotalDifficulty()); @@ -465,6 +468,18 @@ public void setTrailingPeerRequirementsSupplier( this.trailingPeerRequirementsSupplier = tprSupplier; } + // Part of the PeerSelector interface, to be split apart later + @Override + public EthPeer getPeer(final Predicate filter) { + return streamBestPeers().filter(filter).findFirst().orElseThrow(NoAvailablePeersException::new); + } + + // Part of the PeerSelector interface, to be split apart later + @Override + public Optional getPeerByPeerId(final PeerId peerId) { + return Optional.ofNullable(activeConnections.get(peerId.getId())); + } + @FunctionalInterface public interface ConnectCallback { void onPeerConnected(EthPeer newPeer); diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java deleted file mode 100644 index 41d2e9b700b..00000000000 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelector.java +++ /dev/null @@ -1,102 +0,0 @@ -/* - * Copyright contributors to Hyperledger 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.manager.peertask; - -import org.hyperledger.besu.ethereum.eth.manager.EthPeer; -import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec; -import org.hyperledger.besu.ethereum.p2p.peers.PeerId; -import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; - -import java.util.Collection; -import java.util.Comparator; -import java.util.Map; -import java.util.Optional; -import java.util.concurrent.ConcurrentHashMap; -import java.util.function.Predicate; -import java.util.function.Supplier; - -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -/** - * This is a simple PeerSelector implementation that can be used the default implementation in most - * situations - */ -public class DefaultPeerSelector implements PeerSelector { - private static final Logger LOG = LoggerFactory.getLogger(DefaultPeerSelector.class); - - private final Supplier protocolSpecSupplier; - private final Map ethPeersByPeerId = new ConcurrentHashMap<>(); - - public DefaultPeerSelector(final Supplier protocolSpecSupplier) { - this.protocolSpecSupplier = protocolSpecSupplier; - } - - /** - * Gets the highest reputation peer matching the supplied filter - * - * @param filter a filter to match prospective peers with - * @return the highest reputation peer matching the supplies filter - * @throws NoAvailablePeerException If there are no suitable peers - */ - private EthPeer getPeer(final Predicate filter) throws NoAvailablePeerException { - LOG.trace("Finding peer from pool of {} peers", ethPeersByPeerId.size()); - return ethPeersByPeerId.values().stream() - .filter(filter) - .max(Comparator.naturalOrder()) - .orElseThrow(NoAvailablePeerException::new); - } - - @Override - public EthPeer getPeer( - final Collection usedEthPeers, - final long requiredPeerHeight, - final SubProtocol requiredSubProtocol) - throws NoAvailablePeerException { - return getPeer( - (candidatePeer) -> - isPeerUnused(candidatePeer, usedEthPeers) - && (protocolSpecSupplier.get().isPoS() - || isPeerHeightHighEnough(candidatePeer, requiredPeerHeight)) - && isPeerProtocolSuitable(candidatePeer, requiredSubProtocol)); - } - - @Override - public Optional getPeerByPeerId(final PeerId peerId) { - return Optional.ofNullable(ethPeersByPeerId.get(peerId)); - } - - @Override - public void addPeer(final EthPeer ethPeer) { - ethPeersByPeerId.put(ethPeer.getConnection().getPeer(), ethPeer); - } - - @Override - public void removePeer(final PeerId peerId) { - ethPeersByPeerId.remove(peerId); - } - - private boolean isPeerUnused(final EthPeer ethPeer, final Collection usedEthPeers) { - return !usedEthPeers.contains(ethPeer); - } - - private boolean isPeerHeightHighEnough(final EthPeer ethPeer, final long requiredHeight) { - return ethPeer.chainState().getEstimatedHeight() >= requiredHeight; - } - - private boolean isPeerProtocolSuitable(final EthPeer ethPeer, final SubProtocol protocol) { - return ethPeer.getProtocolName().equals(protocol.getName()); - } -} diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java index 93d98a193b1..0801f9f00ec 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java @@ -16,28 +16,20 @@ import org.hyperledger.besu.ethereum.eth.manager.EthPeer; import org.hyperledger.besu.ethereum.p2p.peers.PeerId; -import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; -import java.util.Collection; import java.util.Optional; +import java.util.function.Predicate; /** Selects the EthPeers for the PeerTaskExecutor */ public interface PeerSelector { /** - * Gets a peer with the requiredPeerHeight (if not PoS), and with the requiredSubProtocol, and - * which is not in the supplied collection of usedEthPeers + * Gets a peer matching the supplied filter * - * @param usedEthPeers a collection of EthPeers to be excluded from selection because they have - * already been used - * @param requiredPeerHeight the minimum peer height required of the selected peer - * @param requiredSubProtocol the SubProtocol required of the peer + * @param filter a Predicate\ matching desirable peers * @return a peer matching the supplied conditions - * @throws NoAvailablePeerException If there are no suitable peers */ - EthPeer getPeer( - Collection usedEthPeers, long requiredPeerHeight, SubProtocol requiredSubProtocol) - throws NoAvailablePeerException; + EthPeer getPeer(final Predicate filter); /** * Attempts to get the EthPeer identified by peerId @@ -47,18 +39,4 @@ EthPeer getPeer( * PeerSelector, or empty otherwise */ Optional getPeerByPeerId(PeerId peerId); - - /** - * Add the supplied EthPeer to the PeerSelector - * - * @param ethPeer the EthPeer to be added to the PeerSelector - */ - void addPeer(EthPeer ethPeer); - - /** - * Remove the EthPeer identified by peerId from the PeerSelector - * - * @param peerId the PeerId of the EthPeer to be removed from the PeerSelector - */ - void removePeer(PeerId peerId); } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java index 36bd03531bd..1c5ee76c9ab 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java @@ -14,10 +14,12 @@ */ package org.hyperledger.besu.ethereum.eth.manager.peertask; +import org.hyperledger.besu.ethereum.eth.manager.EthPeer; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; import java.util.Collection; +import java.util.function.Predicate; /** * Represents a task to be executed on an EthPeer by the PeerTaskExecutor @@ -32,13 +34,6 @@ public interface PeerTask { */ SubProtocol getSubProtocol(); - /** - * Gets the minimum required block number for a peer to have to successfully execute this task - * - * @return the minimum required block number for a peer to have to successfully execute this task - */ - long getRequiredBlockNumber(); - /** * Gets the request data to send to the EthPeer * @@ -61,4 +56,11 @@ public interface PeerTask { * @return the Collection of behaviors this task is expected to exhibit in the PeetTaskExecutor */ Collection getPeerTaskBehaviors(); + + /** + * Gets a Predicate that checks if an EthPeer is suitable for this PeerTask + * + * @return a Predicate that checks if an EthPeer is suitable for this PeerTask + */ + Predicate getPeerRequirementFilter(); } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 10c882e7e5a..2d9a6edfce7 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -15,6 +15,8 @@ package org.hyperledger.besu.ethereum.eth.manager.peertask; import org.hyperledger.besu.ethereum.eth.manager.EthPeer; +import org.hyperledger.besu.ethereum.eth.manager.EthScheduler; +import org.hyperledger.besu.ethereum.eth.manager.exceptions.NoAvailablePeersException; import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; import org.hyperledger.besu.metrics.BesuMetricCategory; @@ -37,15 +39,18 @@ public class PeerTaskExecutor { public static final int NO_RETRIES = 1; private final PeerSelector peerSelector; private final PeerTaskRequestSender requestSender; + private final EthScheduler ethScheduler; private final LabelledMetric requestTimer; public PeerTaskExecutor( final PeerSelector peerSelector, final PeerTaskRequestSender requestSender, + final EthScheduler ethScheduler, final MetricsSystem metricsSystem) { this.peerSelector = peerSelector; this.requestSender = requestSender; + this.ethScheduler = ethScheduler; requestTimer = metricsSystem.createLabelledTimer( BesuMetricCategory.PEERS, @@ -66,10 +71,12 @@ public PeerTaskExecutorResult execute(final PeerTask peerTask) { try { peer = peerSelector.getPeer( - usedEthPeers, peerTask.getRequiredBlockNumber(), peerTask.getSubProtocol()); + (candidatePeer) -> + peerTask.getPeerRequirementFilter().test(candidatePeer) + && !usedEthPeers.contains(candidatePeer)); usedEthPeers.add(peer); executorResult = executeAgainstPeer(peerTask, peer); - } catch (NoAvailablePeerException e) { + } catch (NoAvailablePeersException e) { executorResult = new PeerTaskExecutorResult<>( Optional.empty(), PeerTaskExecutorResponseCode.NO_PEER_AVAILABLE); @@ -81,7 +88,8 @@ public PeerTaskExecutorResult execute(final PeerTask peerTask) { } public CompletableFuture> executeAsync(final PeerTask peerTask) { - return CompletableFuture.supplyAsync(() -> execute(peerTask)); + return ethScheduler.scheduleSyncWorkerTask( + () -> CompletableFuture.completedFuture(execute(peerTask))); } public PeerTaskExecutorResult executeAgainstPeer( @@ -138,7 +146,8 @@ public PeerTaskExecutorResult executeAgainstPeer( public CompletableFuture> executeAgainstPeerAsync( final PeerTask peerTask, final EthPeer peer) { - return CompletableFuture.supplyAsync(() -> executeAgainstPeer(peerTask, peer)); + return ethScheduler.scheduleSyncWorkerTask( + () -> CompletableFuture.completedFuture(executeAgainstPeer(peerTask, peer))); } private boolean sleepBetweenRetries() { diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java deleted file mode 100644 index add2b1e612c..00000000000 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerSelectorTest.java +++ /dev/null @@ -1,135 +0,0 @@ -/* - * Copyright contributors to Hyperledger 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.manager.peertask; - -import org.hyperledger.besu.ethereum.eth.EthProtocol; -import org.hyperledger.besu.ethereum.eth.SnapProtocol; -import org.hyperledger.besu.ethereum.eth.manager.ChainState; -import org.hyperledger.besu.ethereum.eth.manager.EthPeer; -import org.hyperledger.besu.ethereum.eth.manager.PeerReputation; -import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec; -import org.hyperledger.besu.ethereum.p2p.peers.Peer; -import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; -import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; - -import java.util.HashSet; -import java.util.Set; - -import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.mockito.Mockito; - -public class DefaultPeerSelectorTest { - - public DefaultPeerSelector peerSelector; - - @BeforeEach - public void beforeTest() { - ProtocolSpec protocolSpec = Mockito.mock(ProtocolSpec.class); - Mockito.when(protocolSpec.isPoS()).thenReturn(false); - peerSelector = new DefaultPeerSelector(() -> protocolSpec); - } - - @Test - public void testGetPeer() throws NoAvailablePeerException { - EthPeer expectedPeer = createTestPeer(10, EthProtocol.get(), 5); - peerSelector.addPeer(expectedPeer); - EthPeer excludedForLowChainHeightPeer = createTestPeer(5, EthProtocol.get(), 50); - peerSelector.addPeer(excludedForLowChainHeightPeer); - EthPeer excludedForWrongProtocolPeer = createTestPeer(10, SnapProtocol.get(), 50); - peerSelector.addPeer(excludedForWrongProtocolPeer); - EthPeer excludedForLowReputationPeer = createTestPeer(10, EthProtocol.get(), 1); - peerSelector.addPeer(excludedForLowReputationPeer); - EthPeer excludedForBeingAlreadyUsedPeer = createTestPeer(10, EthProtocol.get(), 50); - peerSelector.addPeer(excludedForBeingAlreadyUsedPeer); - - Set usedEthPeers = new HashSet<>(); - usedEthPeers.add(excludedForBeingAlreadyUsedPeer); - - EthPeer result = peerSelector.getPeer(usedEthPeers, 10, EthProtocol.get()); - - Assertions.assertSame(expectedPeer, result); - } - - @Test - public void testGetPeerButNoPeerMatchesFilter() { - EthPeer expectedPeer = createTestPeer(10, EthProtocol.get(), 5); - peerSelector.addPeer(expectedPeer); - EthPeer excludedForLowChainHeightPeer = createTestPeer(5, EthProtocol.get(), 50); - peerSelector.addPeer(excludedForLowChainHeightPeer); - EthPeer excludedForWrongProtocolPeer = createTestPeer(10, SnapProtocol.get(), 50); - peerSelector.addPeer(excludedForWrongProtocolPeer); - EthPeer excludedForLowReputationPeer = createTestPeer(10, EthProtocol.get(), 1); - peerSelector.addPeer(excludedForLowReputationPeer); - EthPeer excludedForBeingAlreadyUsedPeer = createTestPeer(10, EthProtocol.get(), 50); - peerSelector.addPeer(excludedForBeingAlreadyUsedPeer); - - Set usedEthPeers = new HashSet<>(); - usedEthPeers.add(excludedForBeingAlreadyUsedPeer); - - Assertions.assertThrows( - NoAvailablePeerException.class, - () -> peerSelector.getPeer(usedEthPeers, 10, new MockSubProtocol())); - } - - private EthPeer createTestPeer( - final long chainHeight, final SubProtocol protocol, final int reputation) { - EthPeer ethPeer = Mockito.mock(EthPeer.class); - PeerConnection peerConnection = Mockito.mock(PeerConnection.class); - Peer peer = Mockito.mock(Peer.class); - ChainState chainState = Mockito.mock(ChainState.class); - PeerReputation peerReputation = Mockito.mock(PeerReputation.class); - - Mockito.when(ethPeer.getConnection()).thenReturn(peerConnection); - Mockito.when(peerConnection.getPeer()).thenReturn(peer); - Mockito.when(ethPeer.getProtocolName()).thenReturn(protocol.getName()); - Mockito.when(ethPeer.chainState()).thenReturn(chainState); - Mockito.when(chainState.getEstimatedHeight()).thenReturn(chainHeight); - Mockito.when(ethPeer.getReputation()).thenReturn(peerReputation); - Mockito.when(peerReputation.getScore()).thenReturn(reputation); - - Mockito.when(ethPeer.compareTo(Mockito.any(EthPeer.class))) - .thenAnswer( - (invocationOnMock) -> { - EthPeer otherPeer = invocationOnMock.getArgument(0, EthPeer.class); - return Integer.compare(reputation, otherPeer.getReputation().getScore()); - }); - return ethPeer; - } - - private static class MockSubProtocol implements SubProtocol { - - @Override - public String getName() { - return "Mock"; - } - - @Override - public int messageSpace(final int protocolVersion) { - throw new UnsupportedOperationException(); - } - - @Override - public boolean isValidMessageCode(final int protocolVersion, final int code) { - throw new UnsupportedOperationException(); - } - - @Override - public String messageName(final int protocolVersion, final int code) { - throw new UnsupportedOperationException(); - } - } -} diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index 15b1747bc7b..98afe58b11c 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -15,16 +15,17 @@ package org.hyperledger.besu.ethereum.eth.manager.peertask; import org.hyperledger.besu.ethereum.eth.manager.EthPeer; +import org.hyperledger.besu.ethereum.eth.manager.EthScheduler; import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; import org.hyperledger.besu.metrics.noop.NoOpMetricsSystem; -import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeoutException; +import java.util.function.Predicate; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; @@ -37,6 +38,7 @@ public class PeerTaskExecutorTest { private @Mock PeerSelector peerSelector; private @Mock PeerTaskRequestSender requestSender; + private @Mock EthScheduler ethScheduler; private @Mock PeerTask peerTask; private @Mock SubProtocol subprotocol; private @Mock MessageData requestMessageData; @@ -49,7 +51,8 @@ public class PeerTaskExecutorTest { @BeforeEach public void beforeTest() { mockCloser = MockitoAnnotations.openMocks(this); - peerTaskExecutor = new PeerTaskExecutor(peerSelector, requestSender, new NoOpMetricsSystem()); + peerTaskExecutor = + new PeerTaskExecutor(peerSelector, requestSender, ethScheduler, new NoOpMetricsSystem()); } @AfterEach @@ -201,14 +204,10 @@ public void testExecuteWithNoPeerTaskBehaviorsAndSuccessFlow() NoAvailablePeerException { Object responseObject = new Object(); - Mockito.when( - peerSelector.getPeer( - Mockito.any(Collection.class), Mockito.eq(10L), Mockito.eq(subprotocol))) - .thenReturn(ethPeer); + Mockito.when(peerSelector.getPeer(Mockito.any(Predicate.class))).thenReturn(ethPeer); Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); - Mockito.when(peerTask.getRequiredBlockNumber()).thenReturn(10L); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) @@ -232,22 +231,18 @@ public void testExecuteWithPeerSwitchingAndSuccessFlow() ExecutionException, InterruptedException, TimeoutException, - InvalidPeerTaskResponseException, - NoAvailablePeerException { + InvalidPeerTaskResponseException { Object responseObject = new Object(); int requestMessageDataCode = 123; EthPeer peer2 = Mockito.mock(EthPeer.class); - Mockito.when( - peerSelector.getPeer( - Mockito.any(Collection.class), Mockito.eq(10L), Mockito.eq(subprotocol))) + Mockito.when(peerSelector.getPeer(Mockito.any(Predicate.class))) .thenReturn(ethPeer) .thenReturn(peer2); Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); Mockito.when(peerTask.getPeerTaskBehaviors()) .thenReturn(List.of(PeerTaskRetryBehavior.RETRY_WITH_OTHER_PEERS)); - Mockito.when(peerTask.getRequiredBlockNumber()).thenReturn(10L); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenThrow(new TimeoutException()); From 6d2cb9567837c305c6280efb6ebde72741cd0f08 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Mon, 7 Oct 2024 08:46:08 +1100 Subject: [PATCH 25/49] 7311: Rename getPeerTaskBehavior to getPeerTaskRetryBehavior Signed-off-by: Matilda Clerke --- .../ethereum/eth/manager/peertask/PeerTask.java | 2 +- .../eth/manager/peertask/PeerTaskExecutor.java | 4 ++-- .../eth/manager/peertask/PeerTaskExecutorTest.java | 14 +++++++------- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java index 1c5ee76c9ab..4436022c9ad 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java @@ -55,7 +55,7 @@ public interface PeerTask { * * @return the Collection of behaviors this task is expected to exhibit in the PeetTaskExecutor */ - Collection getPeerTaskBehaviors(); + Collection getPeerTaskRetryBehaviors(); /** * Gets a Predicate that checks if an EthPeer is suitable for this PeerTask diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 2d9a6edfce7..d4b3f603d19 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -62,7 +62,7 @@ public PeerTaskExecutor( public PeerTaskExecutorResult execute(final PeerTask peerTask) { PeerTaskExecutorResult executorResult; int triesRemaining = - peerTask.getPeerTaskBehaviors().contains(PeerTaskRetryBehavior.RETRY_WITH_OTHER_PEERS) + peerTask.getPeerTaskRetryBehaviors().contains(PeerTaskRetryBehavior.RETRY_WITH_OTHER_PEERS) ? RETRIES_WITH_OTHER_PEER : NO_RETRIES; final Collection usedEthPeers = new HashSet<>(); @@ -97,7 +97,7 @@ public PeerTaskExecutorResult executeAgainstPeer( MessageData requestMessageData = peerTask.getRequestMessage(); PeerTaskExecutorResult executorResult; int triesRemaining = - peerTask.getPeerTaskBehaviors().contains(PeerTaskRetryBehavior.RETRY_WITH_SAME_PEER) + peerTask.getPeerTaskRetryBehaviors().contains(PeerTaskRetryBehavior.RETRY_WITH_SAME_PEER) ? RETRIES_WITH_SAME_PEER : NO_RETRIES; do { diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index 98afe58b11c..a999aa7116e 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -71,7 +71,7 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndSuccessfulFlow() Object responseObject = new Object(); Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); - Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); + Mockito.when(peerTask.getPeerTaskRetryBehaviors()).thenReturn(Collections.emptyList()); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) @@ -99,7 +99,7 @@ public void testExecuteAgainstPeerWithRetryBehaviorsAndSuccessfulFlowAfterFirstF int requestMessageDataCode = 123; Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); - Mockito.when(peerTask.getPeerTaskBehaviors()) + Mockito.when(peerTask.getPeerTaskRetryBehaviors()) .thenReturn(List.of(PeerTaskRetryBehavior.RETRY_WITH_SAME_PEER)); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); @@ -129,7 +129,7 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndPeerNotConnected() TimeoutException { Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); - Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); + Mockito.when(peerTask.getPeerTaskRetryBehaviors()).thenReturn(Collections.emptyList()); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) @@ -151,7 +151,7 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndTimeoutException() int requestMessageDataCode = 123; Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); - Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); + Mockito.when(peerTask.getPeerTaskRetryBehaviors()).thenReturn(Collections.emptyList()); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) @@ -176,7 +176,7 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndInvalidResponseMessa InvalidPeerTaskResponseException { Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); - Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); + Mockito.when(peerTask.getPeerTaskRetryBehaviors()).thenReturn(Collections.emptyList()); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) @@ -207,7 +207,7 @@ public void testExecuteWithNoPeerTaskBehaviorsAndSuccessFlow() Mockito.when(peerSelector.getPeer(Mockito.any(Predicate.class))).thenReturn(ethPeer); Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); - Mockito.when(peerTask.getPeerTaskBehaviors()).thenReturn(Collections.emptyList()); + Mockito.when(peerTask.getPeerTaskRetryBehaviors()).thenReturn(Collections.emptyList()); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) @@ -241,7 +241,7 @@ public void testExecuteWithPeerSwitchingAndSuccessFlow() .thenReturn(peer2); Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); - Mockito.when(peerTask.getPeerTaskBehaviors()) + Mockito.when(peerTask.getPeerTaskRetryBehaviors()) .thenReturn(List.of(PeerTaskRetryBehavior.RETRY_WITH_OTHER_PEERS)); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) From d84520ab831b8e33503c44622ae422047464c7ca Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Mon, 7 Oct 2024 08:46:22 +1100 Subject: [PATCH 26/49] 7311: Rename getPeerTaskBehavior to getPeerTaskRetryBehavior Signed-off-by: Matilda Clerke --- .../ethereum/eth/manager/peertask/PeerTaskExecutorTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index a999aa7116e..1bad6f2cc02 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -200,8 +200,7 @@ public void testExecuteWithNoPeerTaskBehaviorsAndSuccessFlow() ExecutionException, InterruptedException, TimeoutException, - InvalidPeerTaskResponseException, - NoAvailablePeerException { + InvalidPeerTaskResponseException { Object responseObject = new Object(); Mockito.when(peerSelector.getPeer(Mockito.any(Predicate.class))).thenReturn(ethPeer); From 0896e31de71514287c6bceaa7fc3ec6355b7cc31 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Mon, 7 Oct 2024 09:12:03 +1100 Subject: [PATCH 27/49] 7311: Rework PeerTaskExecutor retry system to be 0-based Signed-off-by: Matilda Clerke --- .../eth/manager/peertask/PeerTaskExecutor.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index d4b3f603d19..1d56c61a7e9 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -34,9 +34,9 @@ /** Manages the execution of PeerTasks, respecting their PeerTaskRetryBehavior */ public class PeerTaskExecutor { - public static final int RETRIES_WITH_SAME_PEER = 3; - public static final int RETRIES_WITH_OTHER_PEER = 3; - public static final int NO_RETRIES = 1; + public static final int RETRIES_WITH_SAME_PEER = 2; + public static final int RETRIES_WITH_OTHER_PEER = 2; + public static final int NO_RETRIES = 0; private final PeerSelector peerSelector; private final PeerTaskRequestSender requestSender; private final EthScheduler ethScheduler; @@ -61,7 +61,7 @@ public PeerTaskExecutor( public PeerTaskExecutorResult execute(final PeerTask peerTask) { PeerTaskExecutorResult executorResult; - int triesRemaining = + int retriesRemaining = peerTask.getPeerTaskRetryBehaviors().contains(PeerTaskRetryBehavior.RETRY_WITH_OTHER_PEERS) ? RETRIES_WITH_OTHER_PEER : NO_RETRIES; @@ -81,7 +81,7 @@ public PeerTaskExecutorResult execute(final PeerTask peerTask) { new PeerTaskExecutorResult<>( Optional.empty(), PeerTaskExecutorResponseCode.NO_PEER_AVAILABLE); } - } while (--triesRemaining > 0 + } while (retriesRemaining-- > 0 && executorResult.responseCode() != PeerTaskExecutorResponseCode.SUCCESS); return executorResult; @@ -96,7 +96,7 @@ public PeerTaskExecutorResult executeAgainstPeer( final PeerTask peerTask, final EthPeer peer) { MessageData requestMessageData = peerTask.getRequestMessage(); PeerTaskExecutorResult executorResult; - int triesRemaining = + int retriesRemaining = peerTask.getPeerTaskRetryBehaviors().contains(PeerTaskRetryBehavior.RETRY_WITH_SAME_PEER) ? RETRIES_WITH_SAME_PEER : NO_RETRIES; @@ -136,7 +136,7 @@ public PeerTaskExecutorResult executeAgainstPeer( new PeerTaskExecutorResult<>( Optional.empty(), PeerTaskExecutorResponseCode.INTERNAL_SERVER_ERROR); } - } while (--triesRemaining > 0 + } while (retriesRemaining-- > 0 && executorResult.responseCode() != PeerTaskExecutorResponseCode.SUCCESS && executorResult.responseCode() != PeerTaskExecutorResponseCode.PEER_DISCONNECTED && sleepBetweenRetries()); From c047f428bf5959f542e7849f0858c9f12f2725c4 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Tue, 8 Oct 2024 12:37:22 +1100 Subject: [PATCH 28/49] 7311: Remove unused async methods in PeerTaskExecutor Signed-off-by: Matilda Clerke --- .../eth/manager/peertask/PeerTaskExecutor.java | 16 ---------------- .../manager/peertask/PeerTaskExecutorTest.java | 4 +--- 2 files changed, 1 insertion(+), 19 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 1d56c61a7e9..7e38d8cfa7b 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -15,7 +15,6 @@ package org.hyperledger.besu.ethereum.eth.manager.peertask; import org.hyperledger.besu.ethereum.eth.manager.EthPeer; -import org.hyperledger.besu.ethereum.eth.manager.EthScheduler; import org.hyperledger.besu.ethereum.eth.manager.exceptions.NoAvailablePeersException; import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; @@ -27,7 +26,6 @@ import java.util.Collection; import java.util.HashSet; import java.util.Optional; -import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeoutException; @@ -39,18 +37,15 @@ public class PeerTaskExecutor { public static final int NO_RETRIES = 0; private final PeerSelector peerSelector; private final PeerTaskRequestSender requestSender; - private final EthScheduler ethScheduler; private final LabelledMetric requestTimer; public PeerTaskExecutor( final PeerSelector peerSelector, final PeerTaskRequestSender requestSender, - final EthScheduler ethScheduler, final MetricsSystem metricsSystem) { this.peerSelector = peerSelector; this.requestSender = requestSender; - this.ethScheduler = ethScheduler; requestTimer = metricsSystem.createLabelledTimer( BesuMetricCategory.PEERS, @@ -87,11 +82,6 @@ public PeerTaskExecutorResult execute(final PeerTask peerTask) { return executorResult; } - public CompletableFuture> executeAsync(final PeerTask peerTask) { - return ethScheduler.scheduleSyncWorkerTask( - () -> CompletableFuture.completedFuture(execute(peerTask))); - } - public PeerTaskExecutorResult executeAgainstPeer( final PeerTask peerTask, final EthPeer peer) { MessageData requestMessageData = peerTask.getRequestMessage(); @@ -144,12 +134,6 @@ public PeerTaskExecutorResult executeAgainstPeer( return executorResult; } - public CompletableFuture> executeAgainstPeerAsync( - final PeerTask peerTask, final EthPeer peer) { - return ethScheduler.scheduleSyncWorkerTask( - () -> CompletableFuture.completedFuture(executeAgainstPeer(peerTask, peer))); - } - private boolean sleepBetweenRetries() { try { // sleep for 1 second to match implemented wait between retries in AbstractRetryingPeerTask diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index 1bad6f2cc02..55878d144e9 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -15,7 +15,6 @@ package org.hyperledger.besu.ethereum.eth.manager.peertask; import org.hyperledger.besu.ethereum.eth.manager.EthPeer; -import org.hyperledger.besu.ethereum.eth.manager.EthScheduler; import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; @@ -38,7 +37,6 @@ public class PeerTaskExecutorTest { private @Mock PeerSelector peerSelector; private @Mock PeerTaskRequestSender requestSender; - private @Mock EthScheduler ethScheduler; private @Mock PeerTask peerTask; private @Mock SubProtocol subprotocol; private @Mock MessageData requestMessageData; @@ -52,7 +50,7 @@ public class PeerTaskExecutorTest { public void beforeTest() { mockCloser = MockitoAnnotations.openMocks(this); peerTaskExecutor = - new PeerTaskExecutor(peerSelector, requestSender, ethScheduler, new NoOpMetricsSystem()); + new PeerTaskExecutor(peerSelector, requestSender, new NoOpMetricsSystem()); } @AfterEach From 5aa6b0be5fe883e96752edb847fec0e41bcad618 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Tue, 8 Oct 2024 14:53:40 +1100 Subject: [PATCH 29/49] 7311: Return Optional in PeerSelector.getPeer and utilise existing peer selection behavior in EthPeers Signed-off-by: Matilda Clerke --- .../besu/ethereum/eth/manager/EthPeers.java | 5 ++--- .../eth/manager/peertask/PeerSelector.java | 2 +- .../manager/peertask/PeerTaskExecutor.java | 20 +++++++++---------- .../peertask/PeerTaskExecutorTest.java | 11 +++++----- 4 files changed, 18 insertions(+), 20 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java index 1b413251be7..47ceba65980 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java @@ -17,7 +17,6 @@ import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.eth.SnapProtocol; import org.hyperledger.besu.ethereum.eth.manager.EthPeer.DisconnectCallback; -import org.hyperledger.besu.ethereum.eth.manager.exceptions.NoAvailablePeersException; import org.hyperledger.besu.ethereum.eth.manager.peertask.PeerSelector; import org.hyperledger.besu.ethereum.eth.peervalidation.PeerValidator; import org.hyperledger.besu.ethereum.eth.sync.ChainHeadTracker; @@ -470,8 +469,8 @@ public void setTrailingPeerRequirementsSupplier( // Part of the PeerSelector interface, to be split apart later @Override - public EthPeer getPeer(final Predicate filter) { - return streamBestPeers().filter(filter).findFirst().orElseThrow(NoAvailablePeersException::new); + public Optional getPeer(final Predicate filter) { + return bestPeerMatchingCriteria(filter); } // Part of the PeerSelector interface, to be split apart later diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java index 0801f9f00ec..8f7ab33e42d 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerSelector.java @@ -29,7 +29,7 @@ public interface PeerSelector { * @param filter a Predicate\ matching desirable peers * @return a peer matching the supplied conditions */ - EthPeer getPeer(final Predicate filter); + Optional getPeer(final Predicate filter); /** * Attempts to get the EthPeer identified by peerId diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 7e38d8cfa7b..1734c1e8765 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -15,7 +15,6 @@ package org.hyperledger.besu.ethereum.eth.manager.peertask; import org.hyperledger.besu.ethereum.eth.manager.EthPeer; -import org.hyperledger.besu.ethereum.eth.manager.exceptions.NoAvailablePeersException; import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; import org.hyperledger.besu.metrics.BesuMetricCategory; @@ -62,20 +61,19 @@ public PeerTaskExecutorResult execute(final PeerTask peerTask) { : NO_RETRIES; final Collection usedEthPeers = new HashSet<>(); do { - EthPeer peer; - try { - peer = - peerSelector.getPeer( - (candidatePeer) -> - peerTask.getPeerRequirementFilter().test(candidatePeer) - && !usedEthPeers.contains(candidatePeer)); - usedEthPeers.add(peer); - executorResult = executeAgainstPeer(peerTask, peer); - } catch (NoAvailablePeersException e) { + Optional peer = + peerSelector.getPeer( + (candidatePeer) -> + peerTask.getPeerRequirementFilter().test(candidatePeer) + && !usedEthPeers.contains(candidatePeer)); + if (peer.isEmpty()) { executorResult = new PeerTaskExecutorResult<>( Optional.empty(), PeerTaskExecutorResponseCode.NO_PEER_AVAILABLE); + continue; } + usedEthPeers.add(peer.get()); + executorResult = executeAgainstPeer(peerTask, peer.get()); } while (retriesRemaining-- > 0 && executorResult.responseCode() != PeerTaskExecutorResponseCode.SUCCESS); diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index 55878d144e9..6dfd8d0e203 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -22,6 +22,7 @@ import java.util.Collections; import java.util.List; +import java.util.Optional; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeoutException; import java.util.function.Predicate; @@ -49,8 +50,7 @@ public class PeerTaskExecutorTest { @BeforeEach public void beforeTest() { mockCloser = MockitoAnnotations.openMocks(this); - peerTaskExecutor = - new PeerTaskExecutor(peerSelector, requestSender, new NoOpMetricsSystem()); + peerTaskExecutor = new PeerTaskExecutor(peerSelector, requestSender, new NoOpMetricsSystem()); } @AfterEach @@ -201,7 +201,8 @@ public void testExecuteWithNoPeerTaskBehaviorsAndSuccessFlow() InvalidPeerTaskResponseException { Object responseObject = new Object(); - Mockito.when(peerSelector.getPeer(Mockito.any(Predicate.class))).thenReturn(ethPeer); + Mockito.when(peerSelector.getPeer(Mockito.any(Predicate.class))) + .thenReturn(Optional.of(ethPeer)); Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); Mockito.when(peerTask.getPeerTaskRetryBehaviors()).thenReturn(Collections.emptyList()); @@ -234,8 +235,8 @@ public void testExecuteWithPeerSwitchingAndSuccessFlow() EthPeer peer2 = Mockito.mock(EthPeer.class); Mockito.when(peerSelector.getPeer(Mockito.any(Predicate.class))) - .thenReturn(ethPeer) - .thenReturn(peer2); + .thenReturn(Optional.of(ethPeer)) + .thenReturn(Optional.of(peer2)); Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); Mockito.when(peerTask.getPeerTaskRetryBehaviors()) From 8becdb3ef5077216ad464f7c71ba62aacb11ba33 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Tue, 8 Oct 2024 16:05:12 +1100 Subject: [PATCH 30/49] 7311: Redo getPeer again to include hasAvailableRequestCapacity check Signed-off-by: Matilda Clerke --- .../org/hyperledger/besu/ethereum/eth/manager/EthPeers.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java index 47ceba65980..d070c35ce19 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java @@ -470,7 +470,7 @@ public void setTrailingPeerRequirementsSupplier( // Part of the PeerSelector interface, to be split apart later @Override public Optional getPeer(final Predicate filter) { - return bestPeerMatchingCriteria(filter); + return streamBestPeers().filter(filter).filter(EthPeer::hasAvailableRequestCapacity).findFirst(); } // Part of the PeerSelector interface, to be split apart later From 8186a77d70b92323442b32b0e73a17d8318e4d0c Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Wed, 9 Oct 2024 10:29:03 +1100 Subject: [PATCH 31/49] 7311: Rework getPeer again to use LEAST_TO_MOST_BUSY comparator Signed-off-by: Matilda Clerke --- .../org/hyperledger/besu/ethereum/eth/manager/EthPeers.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java index d070c35ce19..09a998cf647 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java @@ -470,7 +470,10 @@ public void setTrailingPeerRequirementsSupplier( // Part of the PeerSelector interface, to be split apart later @Override public Optional getPeer(final Predicate filter) { - return streamBestPeers().filter(filter).filter(EthPeer::hasAvailableRequestCapacity).findFirst(); + return streamAvailablePeers() + .filter(filter) + .filter(EthPeer::hasAvailableRequestCapacity) + .min(LEAST_TO_MOST_BUSY); } // Part of the PeerSelector interface, to be split apart later From 37b0ec26597b2192bbfa2d0adadbb7c7c7d75fbe Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Wed, 9 Oct 2024 10:30:42 +1100 Subject: [PATCH 32/49] 7311: Import PeerNotConnected class instead of using fully qualified class name Signed-off-by: Matilda Clerke --- .../besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 1734c1e8765..f3ddc5abbe8 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -15,7 +15,7 @@ package org.hyperledger.besu.ethereum.eth.manager.peertask; import org.hyperledger.besu.ethereum.eth.manager.EthPeer; -import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; +import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection.PeerNotConnected; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; import org.hyperledger.besu.metrics.BesuMetricCategory; import org.hyperledger.besu.plugin.services.MetricsSystem; @@ -103,7 +103,7 @@ public PeerTaskExecutorResult executeAgainstPeer( new PeerTaskExecutorResult<>( Optional.ofNullable(result), PeerTaskExecutorResponseCode.SUCCESS); - } catch (PeerConnection.PeerNotConnected e) { + } catch (PeerNotConnected e) { executorResult = new PeerTaskExecutorResult<>( Optional.empty(), PeerTaskExecutorResponseCode.PEER_DISCONNECTED); From 545fd5c29dd00a7f0f3117dd864b4650a9b643ac Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Thu, 10 Oct 2024 08:41:18 +1100 Subject: [PATCH 33/49] 7311: Change to specifying retry counts in PeerTask instead of behavior enums Signed-off-by: Matilda Clerke --- .../eth/manager/peertask/PeerTask.java | 18 +++++++++++++---- .../manager/peertask/PeerTaskExecutor.java | 13 ++---------- .../peertask/PeerTaskRetryBehavior.java | 20 ------------------- .../peertask/PeerTaskExecutorTest.java | 20 +++++++++---------- 4 files changed, 25 insertions(+), 46 deletions(-) delete mode 100644 ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRetryBehavior.java diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java index 4436022c9ad..1d6464ac26d 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java @@ -18,7 +18,6 @@ import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; -import java.util.Collection; import java.util.function.Predicate; /** @@ -51,11 +50,22 @@ public interface PeerTask { T parseResponse(MessageData messageData) throws InvalidPeerTaskResponseException; /** - * Gets the Collection of behaviors this task is expected to exhibit in the PeetTaskExecutor + * Gets the number of times this task may be attempted against other peers * - * @return the Collection of behaviors this task is expected to exhibit in the PeetTaskExecutor + * @return the number of times this task may be attempted against other peers */ - Collection getPeerTaskRetryBehaviors(); + default int getRetriesWithOtherPeer() { + return 5; + } + + /** + * Gets the number of times this task may be attempted against the same peer + * + * @return the number of times this task may be attempted against the same peer + */ + default int getRetriesWithSamePeer() { + return 5; + } /** * Gets a Predicate that checks if an EthPeer is suitable for this PeerTask diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index f3ddc5abbe8..7b485ee6c37 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -31,9 +31,6 @@ /** Manages the execution of PeerTasks, respecting their PeerTaskRetryBehavior */ public class PeerTaskExecutor { - public static final int RETRIES_WITH_SAME_PEER = 2; - public static final int RETRIES_WITH_OTHER_PEER = 2; - public static final int NO_RETRIES = 0; private final PeerSelector peerSelector; private final PeerTaskRequestSender requestSender; @@ -55,10 +52,7 @@ public PeerTaskExecutor( public PeerTaskExecutorResult execute(final PeerTask peerTask) { PeerTaskExecutorResult executorResult; - int retriesRemaining = - peerTask.getPeerTaskRetryBehaviors().contains(PeerTaskRetryBehavior.RETRY_WITH_OTHER_PEERS) - ? RETRIES_WITH_OTHER_PEER - : NO_RETRIES; + int retriesRemaining = peerTask.getRetriesWithOtherPeer(); final Collection usedEthPeers = new HashSet<>(); do { Optional peer = @@ -84,10 +78,7 @@ public PeerTaskExecutorResult executeAgainstPeer( final PeerTask peerTask, final EthPeer peer) { MessageData requestMessageData = peerTask.getRequestMessage(); PeerTaskExecutorResult executorResult; - int retriesRemaining = - peerTask.getPeerTaskRetryBehaviors().contains(PeerTaskRetryBehavior.RETRY_WITH_SAME_PEER) - ? RETRIES_WITH_SAME_PEER - : NO_RETRIES; + int retriesRemaining = peerTask.getRetriesWithSamePeer(); do { try { T result; diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRetryBehavior.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRetryBehavior.java deleted file mode 100644 index 53e2def6612..00000000000 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRetryBehavior.java +++ /dev/null @@ -1,20 +0,0 @@ -/* - * Copyright contributors to Hyperledger 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.manager.peertask; - -public enum PeerTaskRetryBehavior { - RETRY_WITH_SAME_PEER, - RETRY_WITH_OTHER_PEERS -} diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index 6dfd8d0e203..0015c1ffbcb 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -20,8 +20,6 @@ import org.hyperledger.besu.ethereum.p2p.rlpx.wire.SubProtocol; import org.hyperledger.besu.metrics.noop.NoOpMetricsSystem; -import java.util.Collections; -import java.util.List; import java.util.Optional; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeoutException; @@ -69,7 +67,7 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndSuccessfulFlow() Object responseObject = new Object(); Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); - Mockito.when(peerTask.getPeerTaskRetryBehaviors()).thenReturn(Collections.emptyList()); + Mockito.when(peerTask.getRetriesWithSamePeer()).thenReturn(0); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) @@ -97,8 +95,7 @@ public void testExecuteAgainstPeerWithRetryBehaviorsAndSuccessfulFlowAfterFirstF int requestMessageDataCode = 123; Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); - Mockito.when(peerTask.getPeerTaskRetryBehaviors()) - .thenReturn(List.of(PeerTaskRetryBehavior.RETRY_WITH_SAME_PEER)); + Mockito.when(peerTask.getRetriesWithSamePeer()).thenReturn(2); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); @@ -127,7 +124,7 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndPeerNotConnected() TimeoutException { Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); - Mockito.when(peerTask.getPeerTaskRetryBehaviors()).thenReturn(Collections.emptyList()); + Mockito.when(peerTask.getRetriesWithSamePeer()).thenReturn(0); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) @@ -149,7 +146,7 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndTimeoutException() int requestMessageDataCode = 123; Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); - Mockito.when(peerTask.getPeerTaskRetryBehaviors()).thenReturn(Collections.emptyList()); + Mockito.when(peerTask.getRetriesWithSamePeer()).thenReturn(0); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) @@ -174,7 +171,7 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndInvalidResponseMessa InvalidPeerTaskResponseException { Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); - Mockito.when(peerTask.getPeerTaskRetryBehaviors()).thenReturn(Collections.emptyList()); + Mockito.when(peerTask.getRetriesWithSamePeer()).thenReturn(0); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) @@ -205,7 +202,8 @@ public void testExecuteWithNoPeerTaskBehaviorsAndSuccessFlow() .thenReturn(Optional.of(ethPeer)); Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); - Mockito.when(peerTask.getPeerTaskRetryBehaviors()).thenReturn(Collections.emptyList()); + Mockito.when(peerTask.getRetriesWithOtherPeer()).thenReturn(0); + Mockito.when(peerTask.getRetriesWithSamePeer()).thenReturn(0); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) @@ -239,8 +237,8 @@ public void testExecuteWithPeerSwitchingAndSuccessFlow() .thenReturn(Optional.of(peer2)); Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); - Mockito.when(peerTask.getPeerTaskRetryBehaviors()) - .thenReturn(List.of(PeerTaskRetryBehavior.RETRY_WITH_OTHER_PEERS)); + Mockito.when(peerTask.getRetriesWithOtherPeer()).thenReturn(2); + Mockito.when(peerTask.getRetriesWithSamePeer()).thenReturn(0); Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenThrow(new TimeoutException()); From 1c268b70cd64613b57953714ea291c084696dd91 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Thu, 10 Oct 2024 15:14:49 +1100 Subject: [PATCH 34/49] 7311: Add additional metrics to PeerTaskExecutor Signed-off-by: Matilda Clerke --- .../manager/peertask/PeerTaskExecutor.java | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 7b485ee6c37..7c096698fba 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -19,6 +19,7 @@ import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; import org.hyperledger.besu.metrics.BesuMetricCategory; import org.hyperledger.besu.plugin.services.MetricsSystem; +import org.hyperledger.besu.plugin.services.metrics.Counter; import org.hyperledger.besu.plugin.services.metrics.LabelledMetric; import org.hyperledger.besu.plugin.services.metrics.OperationTimer; @@ -35,6 +36,9 @@ public class PeerTaskExecutor { private final PeerTaskRequestSender requestSender; private final LabelledMetric requestTimer; + private final LabelledMetric timeoutCounter; + private final LabelledMetric invalidResponseCounter; + private final LabelledMetric internalExceptionCounter; public PeerTaskExecutor( final PeerSelector peerSelector, @@ -48,6 +52,24 @@ public PeerTaskExecutor( "PeerTaskExecutor:RequestTime", "Time taken to send a request and receive a response", "className"); + timeoutCounter = + metricsSystem.createLabelledCounter( + BesuMetricCategory.PEERS, + "PeerTaskExecutor:TimeoutCounter", + "Counter of the number of timeouts occurred", + "className"); + invalidResponseCounter = + metricsSystem.createLabelledCounter( + BesuMetricCategory.PEERS, + "PeerTaskExecutor:InvalidResponseCounter", + "Counter of the number of invalid responses received", + "className"); + internalExceptionCounter = + metricsSystem.createLabelledCounter( + BesuMetricCategory.PEERS, + "PeerTaskExecutor:InternalExceptionCounter", + "Counter of the number of internal exceptions occurred", + "className"); } public PeerTaskExecutorResult execute(final PeerTask peerTask) { @@ -80,6 +102,7 @@ public PeerTaskExecutorResult executeAgainstPeer( PeerTaskExecutorResult executorResult; int retriesRemaining = peerTask.getRetriesWithSamePeer(); do { + try { T result; try (final OperationTimer.TimingContext ignored = @@ -101,16 +124,19 @@ public PeerTaskExecutorResult executeAgainstPeer( } catch (InterruptedException | TimeoutException e) { peer.recordRequestTimeout(requestMessageData.getCode()); + timeoutCounter.labels(peerTask.getClass().getSimpleName()).inc(); executorResult = new PeerTaskExecutorResult<>(Optional.empty(), PeerTaskExecutorResponseCode.TIMEOUT); } catch (InvalidPeerTaskResponseException e) { peer.recordUselessResponse(e.getMessage()); + invalidResponseCounter.labels(peerTask.getClass().getSimpleName()).inc(); executorResult = new PeerTaskExecutorResult<>( Optional.empty(), PeerTaskExecutorResponseCode.INVALID_RESPONSE); } catch (ExecutionException e) { + internalExceptionCounter.labels(peerTask.getClass().getSimpleName()).inc(); executorResult = new PeerTaskExecutorResult<>( Optional.empty(), PeerTaskExecutorResponseCode.INTERNAL_SERVER_ERROR); From b06f38b2607f5c479d6d15f3780c0d0f190c17e2 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Thu, 10 Oct 2024 16:14:19 +1100 Subject: [PATCH 35/49] 7311: Add Predicate to PeerTask to check for partial success Signed-off-by: Matilda Clerke --- .../eth/manager/peertask/PeerTask.java | 7 +++ .../manager/peertask/PeerTaskExecutor.java | 15 +++++-- .../PeerTaskExecutorResponseCode.java | 1 + .../peertask/PeerTaskExecutorTest.java | 43 ++++++++++++++++--- 4 files changed, 56 insertions(+), 10 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java index 1d6464ac26d..57976ac2e9a 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java @@ -73,4 +73,11 @@ default int getRetriesWithSamePeer() { * @return a Predicate that checks if an EthPeer is suitable for this PeerTask */ Predicate getPeerRequirementFilter(); + + /** + * Checks if the supplied result is considered a partial success + * + * @return true if the supplied result is considered a partial success + */ + boolean isPartialSuccessTest(T result); } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 7c096698fba..5d1883c7d80 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -112,10 +112,17 @@ public PeerTaskExecutorResult executeAgainstPeer( result = peerTask.parseResponse(responseMessageData); } - peer.recordUsefulResponse(); - executorResult = - new PeerTaskExecutorResult<>( - Optional.ofNullable(result), PeerTaskExecutorResponseCode.SUCCESS); + + if (peerTask.isPartialSuccessTest(result)) { + executorResult = + new PeerTaskExecutorResult<>( + Optional.ofNullable(result), PeerTaskExecutorResponseCode.PARTIAL_SUCCESS); + } else { + peer.recordUsefulResponse(); + executorResult = + new PeerTaskExecutorResult<>( + Optional.ofNullable(result), PeerTaskExecutorResponseCode.SUCCESS); + } } catch (PeerNotConnected e) { executorResult = diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResponseCode.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResponseCode.java index 327461de15a..123c3267c04 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResponseCode.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResponseCode.java @@ -16,6 +16,7 @@ public enum PeerTaskExecutorResponseCode { SUCCESS, + PARTIAL_SUCCESS, NO_PEER_AVAILABLE, PEER_DISCONNECTED, INTERNAL_SERVER_ERROR, diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index 0015c1ffbcb..a4e78056e14 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -57,7 +57,7 @@ public void afterTest() throws Exception { } @Test - public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndSuccessfulFlow() + public void testExecuteAgainstPeerWithNoRetriesAndSuccessfulFlow() throws PeerConnection.PeerNotConnected, ExecutionException, InterruptedException, @@ -73,6 +73,7 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndSuccessfulFlow() Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); + Mockito.when(peerTask.isPartialSuccessTest(responseObject)).thenReturn(false); PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); @@ -85,7 +86,34 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndSuccessfulFlow() } @Test - public void testExecuteAgainstPeerWithRetryBehaviorsAndSuccessfulFlowAfterFirstFailure() + public void testExecuteAgainstPeerWithNoRetriesAndPartialSuccessfulFlow() + throws PeerConnection.PeerNotConnected, + ExecutionException, + InterruptedException, + TimeoutException, + InvalidPeerTaskResponseException { + + Object responseObject = new Object(); + + Mockito.when(peerTask.getRequestMessage()).thenReturn(requestMessageData); + Mockito.when(peerTask.getRetriesWithSamePeer()).thenReturn(0); + Mockito.when(peerTask.getSubProtocol()).thenReturn(subprotocol); + Mockito.when(subprotocol.getName()).thenReturn("subprotocol"); + Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) + .thenReturn(responseMessageData); + Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); + Mockito.when(peerTask.isPartialSuccessTest(responseObject)).thenReturn(true); + + PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); + + Assertions.assertNotNull(result); + Assertions.assertTrue(result.result().isPresent()); + Assertions.assertSame(responseObject, result.result().get()); + Assertions.assertEquals(PeerTaskExecutorResponseCode.PARTIAL_SUCCESS, result.responseCode()); + } + + @Test + public void testExecuteAgainstPeerWithRetriesAndSuccessfulFlowAfterFirstFailure() throws PeerConnection.PeerNotConnected, ExecutionException, InterruptedException, @@ -104,6 +132,7 @@ public void testExecuteAgainstPeerWithRetryBehaviorsAndSuccessfulFlowAfterFirstF .thenReturn(responseMessageData); Mockito.when(requestMessageData.getCode()).thenReturn(requestMessageDataCode); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); + Mockito.when(peerTask.isPartialSuccessTest(responseObject)).thenReturn(false); PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); @@ -117,7 +146,7 @@ public void testExecuteAgainstPeerWithRetryBehaviorsAndSuccessfulFlowAfterFirstF } @Test - public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndPeerNotConnected() + public void testExecuteAgainstPeerWithNoRetriesAndPeerNotConnected() throws PeerConnection.PeerNotConnected, ExecutionException, InterruptedException, @@ -138,7 +167,7 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndPeerNotConnected() } @Test - public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndTimeoutException() + public void testExecuteAgainstPeerWithNoRetriesAndTimeoutException() throws PeerConnection.PeerNotConnected, ExecutionException, InterruptedException, @@ -163,7 +192,7 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndTimeoutException() } @Test - public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndInvalidResponseMessage() + public void testExecuteAgainstPeerWithNoRetriesAndInvalidResponseMessage() throws PeerConnection.PeerNotConnected, ExecutionException, InterruptedException, @@ -190,7 +219,7 @@ public void testExecuteAgainstPeerWithNoPeerTaskBehaviorsAndInvalidResponseMessa @Test @SuppressWarnings("unchecked") - public void testExecuteWithNoPeerTaskBehaviorsAndSuccessFlow() + public void testExecuteWithNoRetriesAndSuccessFlow() throws PeerConnection.PeerNotConnected, ExecutionException, InterruptedException, @@ -209,6 +238,7 @@ public void testExecuteWithNoPeerTaskBehaviorsAndSuccessFlow() Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); + Mockito.when(peerTask.isPartialSuccessTest(responseObject)).thenReturn(false); PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); @@ -246,6 +276,7 @@ public void testExecuteWithPeerSwitchingAndSuccessFlow() Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, peer2)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); + Mockito.when(peerTask.isPartialSuccessTest(responseObject)).thenReturn(false); PeerTaskExecutorResult result = peerTaskExecutor.execute(peerTask); From 3c12d3d8c33641487265f57fecfb05ec840c3f4b Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Thu, 10 Oct 2024 16:18:58 +1100 Subject: [PATCH 36/49] 7311: Fix incorrect name on isPartialSuccessTest Signed-off-by: Matilda Clerke --- .../besu/ethereum/eth/manager/peertask/PeerTask.java | 2 +- .../eth/manager/peertask/PeerTaskExecutor.java | 2 +- .../eth/manager/peertask/PeerTaskExecutorTest.java | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java index 57976ac2e9a..40b995503f8 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java @@ -79,5 +79,5 @@ default int getRetriesWithSamePeer() { * * @return true if the supplied result is considered a partial success */ - boolean isPartialSuccessTest(T result); + boolean isPartialSuccess(T result); } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 5d1883c7d80..9b062977461 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -113,7 +113,7 @@ public PeerTaskExecutorResult executeAgainstPeer( result = peerTask.parseResponse(responseMessageData); } - if (peerTask.isPartialSuccessTest(result)) { + if (peerTask.isPartialSuccess(result)) { executorResult = new PeerTaskExecutorResult<>( Optional.ofNullable(result), PeerTaskExecutorResponseCode.PARTIAL_SUCCESS); diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index a4e78056e14..853210e685f 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -73,7 +73,7 @@ public void testExecuteAgainstPeerWithNoRetriesAndSuccessfulFlow() Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); - Mockito.when(peerTask.isPartialSuccessTest(responseObject)).thenReturn(false); + Mockito.when(peerTask.isPartialSuccess(responseObject)).thenReturn(false); PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); @@ -102,7 +102,7 @@ public void testExecuteAgainstPeerWithNoRetriesAndPartialSuccessfulFlow() Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); - Mockito.when(peerTask.isPartialSuccessTest(responseObject)).thenReturn(true); + Mockito.when(peerTask.isPartialSuccess(responseObject)).thenReturn(true); PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); @@ -132,7 +132,7 @@ public void testExecuteAgainstPeerWithRetriesAndSuccessfulFlowAfterFirstFailure( .thenReturn(responseMessageData); Mockito.when(requestMessageData.getCode()).thenReturn(requestMessageDataCode); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); - Mockito.when(peerTask.isPartialSuccessTest(responseObject)).thenReturn(false); + Mockito.when(peerTask.isPartialSuccess(responseObject)).thenReturn(false); PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); @@ -238,7 +238,7 @@ public void testExecuteWithNoRetriesAndSuccessFlow() Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); - Mockito.when(peerTask.isPartialSuccessTest(responseObject)).thenReturn(false); + Mockito.when(peerTask.isPartialSuccess(responseObject)).thenReturn(false); PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); @@ -276,7 +276,7 @@ public void testExecuteWithPeerSwitchingAndSuccessFlow() Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, peer2)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); - Mockito.when(peerTask.isPartialSuccessTest(responseObject)).thenReturn(false); + Mockito.when(peerTask.isPartialSuccess(responseObject)).thenReturn(false); PeerTaskExecutorResult result = peerTaskExecutor.execute(peerTask); From d66dd3a4cd0ab6ec267ea87a740905ab495cafff Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Fri, 11 Oct 2024 11:10:30 +1100 Subject: [PATCH 37/49] 7311: Add partialSuccessCounter and inflightRequestGauge in PeerTaskExecutor Signed-off-by: Matilda Clerke --- .../manager/peertask/PeerTaskExecutor.java | 40 ++++++++++++++++--- .../besu/metrics/noop/NoOpMetricsSystem.java | 9 +++++ .../besu/metrics/noop/NoOpValueCollector.java | 6 +++ .../opentelemetry/OpenTelemetryGauge.java | 8 ++++ .../metrics/prometheus/PrometheusGauge.java | 18 +++++++-- .../services/metrics/LabelledGauge.java | 2 + 6 files changed, 74 insertions(+), 9 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 9b062977461..6fee0a05779 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -20,14 +20,18 @@ import org.hyperledger.besu.metrics.BesuMetricCategory; import org.hyperledger.besu.plugin.services.MetricsSystem; import org.hyperledger.besu.plugin.services.metrics.Counter; +import org.hyperledger.besu.plugin.services.metrics.LabelledGauge; import org.hyperledger.besu.plugin.services.metrics.LabelledMetric; import org.hyperledger.besu.plugin.services.metrics.OperationTimer; import java.util.Collection; import java.util.HashSet; +import java.util.Map; import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicInteger; /** Manages the execution of PeerTasks, respecting their PeerTaskRetryBehavior */ public class PeerTaskExecutor { @@ -36,9 +40,12 @@ public class PeerTaskExecutor { private final PeerTaskRequestSender requestSender; private final LabelledMetric requestTimer; + private final LabelledMetric partialSuccessCounter; private final LabelledMetric timeoutCounter; private final LabelledMetric invalidResponseCounter; private final LabelledMetric internalExceptionCounter; + private final LabelledGauge inflightRequestGauge; + private final Map inflightRequestCountByClassName; public PeerTaskExecutor( final PeerSelector peerSelector, @@ -52,6 +59,12 @@ public PeerTaskExecutor( "PeerTaskExecutor:RequestTime", "Time taken to send a request and receive a response", "className"); + partialSuccessCounter = + metricsSystem.createLabelledCounter( + BesuMetricCategory.PEERS, + "PeerTaskExecutor:PartialSuccessCounter", + "Counter of the number of partial success occurred", + "className"); timeoutCounter = metricsSystem.createLabelledCounter( BesuMetricCategory.PEERS, @@ -70,6 +83,13 @@ public PeerTaskExecutor( "PeerTaskExecutor:InternalExceptionCounter", "Counter of the number of internal exceptions occurred", "className"); + inflightRequestGauge = + metricsSystem.createLabelledGauge( + BesuMetricCategory.PEERS, + "PeerTaskExecutor:InflightRequestGauge", + "Gauge of the number of inflight requests", + "className"); + inflightRequestCountByClassName = new ConcurrentHashMap<>(); } public PeerTaskExecutorResult execute(final PeerTask peerTask) { @@ -98,22 +118,32 @@ public PeerTaskExecutorResult execute(final PeerTask peerTask) { public PeerTaskExecutorResult executeAgainstPeer( final PeerTask peerTask, final EthPeer peer) { + String taskClassName = peerTask.getClass().getSimpleName(); + AtomicInteger inflightRequestCountForThisTaskClass = + inflightRequestCountByClassName.getOrDefault(taskClassName, new AtomicInteger(0)); + if (!inflightRequestGauge.isLabelsObserved(taskClassName)) { + inflightRequestGauge.labels(inflightRequestCountForThisTaskClass::get, taskClassName); + } MessageData requestMessageData = peerTask.getRequestMessage(); PeerTaskExecutorResult executorResult; int retriesRemaining = peerTask.getRetriesWithSamePeer(); do { - try { T result; try (final OperationTimer.TimingContext ignored = - requestTimer.labels(peerTask.getClass().getSimpleName()).startTimer()) { + requestTimer.labels(taskClassName).startTimer()) { + inflightRequestCountForThisTaskClass.incrementAndGet(); + MessageData responseMessageData = requestSender.sendRequest(peerTask.getSubProtocol(), requestMessageData, peer); result = peerTask.parseResponse(responseMessageData); + } finally { + inflightRequestCountForThisTaskClass.decrementAndGet(); } if (peerTask.isPartialSuccess(result)) { + partialSuccessCounter.labels(taskClassName).inc(); executorResult = new PeerTaskExecutorResult<>( Optional.ofNullable(result), PeerTaskExecutorResponseCode.PARTIAL_SUCCESS); @@ -131,19 +161,19 @@ public PeerTaskExecutorResult executeAgainstPeer( } catch (InterruptedException | TimeoutException e) { peer.recordRequestTimeout(requestMessageData.getCode()); - timeoutCounter.labels(peerTask.getClass().getSimpleName()).inc(); + timeoutCounter.labels(taskClassName).inc(); executorResult = new PeerTaskExecutorResult<>(Optional.empty(), PeerTaskExecutorResponseCode.TIMEOUT); } catch (InvalidPeerTaskResponseException e) { peer.recordUselessResponse(e.getMessage()); - invalidResponseCounter.labels(peerTask.getClass().getSimpleName()).inc(); + invalidResponseCounter.labels(taskClassName).inc(); executorResult = new PeerTaskExecutorResult<>( Optional.empty(), PeerTaskExecutorResponseCode.INVALID_RESPONSE); } catch (ExecutionException e) { - internalExceptionCounter.labels(peerTask.getClass().getSimpleName()).inc(); + internalExceptionCounter.labels(taskClassName).inc(); executorResult = new PeerTaskExecutorResult<>( Optional.empty(), PeerTaskExecutorResponseCode.INTERNAL_SERVER_ERROR); diff --git a/metrics/core/src/main/java/org/hyperledger/besu/metrics/noop/NoOpMetricsSystem.java b/metrics/core/src/main/java/org/hyperledger/besu/metrics/noop/NoOpMetricsSystem.java index 2d1ee26cfd1..5f876fa4d80 100644 --- a/metrics/core/src/main/java/org/hyperledger/besu/metrics/noop/NoOpMetricsSystem.java +++ b/metrics/core/src/main/java/org/hyperledger/besu/metrics/noop/NoOpMetricsSystem.java @@ -253,5 +253,14 @@ public void labels(final DoubleSupplier valueSupplier, final String... labelValu "The count of labels used must match the count of labels expected."); Preconditions.checkNotNull(valueSupplier, "No valueSupplier specified"); } + + @Override + public boolean isLabelsObserved(final String... labelValues) { + Preconditions.checkArgument( + labelValues.length == labelCount, + "The count of labels used must match the count of labels expected."); + final String labelValuesString = String.join(",", labelValues); + return labelValuesCache.contains(labelValuesString); + } } } diff --git a/metrics/core/src/main/java/org/hyperledger/besu/metrics/noop/NoOpValueCollector.java b/metrics/core/src/main/java/org/hyperledger/besu/metrics/noop/NoOpValueCollector.java index 144e7f3187c..6f36f10d2c7 100644 --- a/metrics/core/src/main/java/org/hyperledger/besu/metrics/noop/NoOpValueCollector.java +++ b/metrics/core/src/main/java/org/hyperledger/besu/metrics/noop/NoOpValueCollector.java @@ -36,4 +36,10 @@ public synchronized void labels(final DoubleSupplier valueSupplier, final String } labelValuesCreated.add(labelValuesString); } + + @Override + public boolean isLabelsObserved(final String... labelValues) { + final String labelValuesString = String.join(",", labelValues); + return labelValuesCreated.contains(labelValuesString); + } } diff --git a/metrics/core/src/main/java/org/hyperledger/besu/metrics/opentelemetry/OpenTelemetryGauge.java b/metrics/core/src/main/java/org/hyperledger/besu/metrics/opentelemetry/OpenTelemetryGauge.java index 6aea56586a8..e1d785c68ec 100644 --- a/metrics/core/src/main/java/org/hyperledger/besu/metrics/opentelemetry/OpenTelemetryGauge.java +++ b/metrics/core/src/main/java/org/hyperledger/besu/metrics/opentelemetry/OpenTelemetryGauge.java @@ -63,6 +63,14 @@ public void labels(final DoubleSupplier valueSupplier, final String... labelValu } } + @Override + public boolean isLabelsObserved(final String... labelValues) { + Preconditions.checkArgument( + labelValues.length == labelNames.size(), + "label values and label names need the same number of elements"); + return observationsMap.containsKey(getLabels(labelValues)); + } + private Attributes getLabels(final String... labelValues) { final AttributesBuilder labelsBuilder = Attributes.builder(); for (int i = 0; i < labelNames.size(); i++) { diff --git a/metrics/core/src/main/java/org/hyperledger/besu/metrics/prometheus/PrometheusGauge.java b/metrics/core/src/main/java/org/hyperledger/besu/metrics/prometheus/PrometheusGauge.java index 83000dfac32..b69e3f90626 100644 --- a/metrics/core/src/main/java/org/hyperledger/besu/metrics/prometheus/PrometheusGauge.java +++ b/metrics/core/src/main/java/org/hyperledger/besu/metrics/prometheus/PrometheusGauge.java @@ -47,10 +47,7 @@ public PrometheusGauge( @Override public synchronized void labels(final DoubleSupplier valueSupplier, final String... labelValues) { - if (labelValues.length != labelNames.size()) { - throw new IllegalArgumentException( - "Label values and label names must be the same cardinality"); - } + validateLabelsCardinality(labelValues); if (observationsMap.putIfAbsent(List.of(labelValues), valueSupplier) != null) { final String labelValuesString = String.join(",", labelValues); throw new IllegalArgumentException( @@ -58,6 +55,12 @@ public synchronized void labels(final DoubleSupplier valueSupplier, final String } } + @Override + public boolean isLabelsObserved(final String... labelValues) { + validateLabelsCardinality(labelValues); + return observationsMap.containsKey(List.of(labelValues)); + } + @Override public List collect() { final List samples = new ArrayList<>(); @@ -68,4 +71,11 @@ public List collect() { metricName, labelNames, labels, valueSupplier.getAsDouble()))); return List.of(new MetricFamilySamples(metricName, Type.GAUGE, help, samples)); } + + private void validateLabelsCardinality(final String... labelValues) { + if (labelValues.length != labelNames.size()) { + throw new IllegalArgumentException( + "Label values and label names must be the same cardinality"); + } + } } diff --git a/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/LabelledGauge.java b/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/LabelledGauge.java index 724e31c58de..c2f64c1113f 100644 --- a/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/LabelledGauge.java +++ b/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/LabelledGauge.java @@ -25,4 +25,6 @@ public interface LabelledGauge { * @param labelValues the label values */ void labels(final DoubleSupplier valueSupplier, final String... labelValues); + + boolean isLabelsObserved(final String... labelValues); } From a3f5d4ac689cf1c572fe5c2bdad18077c29555ef Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Fri, 11 Oct 2024 14:35:06 +1100 Subject: [PATCH 38/49] 7311: Also filter by whether a peer is fully validated Signed-off-by: Matilda Clerke --- .../java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java | 1 + 1 file changed, 1 insertion(+) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java index 09a998cf647..d19c7dfca2e 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java @@ -473,6 +473,7 @@ public Optional getPeer(final Predicate filter) { return streamAvailablePeers() .filter(filter) .filter(EthPeer::hasAvailableRequestCapacity) + .filter(EthPeer::isFullyValidated) .min(LEAST_TO_MOST_BUSY); } From 3a689800880d42f656907df019757f4eb4f398b2 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Fri, 11 Oct 2024 15:57:44 +1100 Subject: [PATCH 39/49] 7311: Fix up inflight requests gauge in PeerTaskExecutor Signed-off-by: Matilda Clerke --- .../eth/manager/peertask/PeerTaskExecutor.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 6fee0a05779..2653fd6f359 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -120,10 +120,13 @@ public PeerTaskExecutorResult executeAgainstPeer( final PeerTask peerTask, final EthPeer peer) { String taskClassName = peerTask.getClass().getSimpleName(); AtomicInteger inflightRequestCountForThisTaskClass = - inflightRequestCountByClassName.getOrDefault(taskClassName, new AtomicInteger(0)); - if (!inflightRequestGauge.isLabelsObserved(taskClassName)) { - inflightRequestGauge.labels(inflightRequestCountForThisTaskClass::get, taskClassName); - } + inflightRequestCountByClassName.computeIfAbsent( + taskClassName, + (k) -> { + AtomicInteger inflightRequests = new AtomicInteger(0); + inflightRequestGauge.labels(inflightRequests::get, taskClassName); + return inflightRequests; + }); MessageData requestMessageData = peerTask.getRequestMessage(); PeerTaskExecutorResult executorResult; int retriesRemaining = peerTask.getRetriesWithSamePeer(); From 56c1f9d1bc4f3d9516d9399b687b18911f049a95 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Fri, 11 Oct 2024 16:09:35 +1100 Subject: [PATCH 40/49] 7311: Update plugin api hash Signed-off-by: Matilda Clerke --- plugin-api/build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin-api/build.gradle b/plugin-api/build.gradle index dd2da0a15ec..0b44097b90c 100644 --- a/plugin-api/build.gradle +++ b/plugin-api/build.gradle @@ -71,7 +71,7 @@ Calculated : ${currentHash} tasks.register('checkAPIChanges', FileStateChecker) { description = "Checks that the API for the Plugin-API project does not change without deliberate thought" files = sourceSets.main.allJava.files - knownHash = '4jVaj9yW88nHbX0KmTR3dPQRvj9x8Pvh5E9Ry7KRT6w=' + knownHash = 'LBcFfNI00D9oKdnuLo2t5sXZZXfYZQSDB/t58U0PmgI=' } check.dependsOn('checkAPIChanges') From e733452205e54c73fe1ce7f6522ef41e029f79ae Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Mon, 14 Oct 2024 08:47:21 +1100 Subject: [PATCH 41/49] 7311: Add javadoc to LabelledGauge.isLabelsObserved Signed-off-by: Matilda Clerke --- .../besu/plugin/services/metrics/LabelledGauge.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/LabelledGauge.java b/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/LabelledGauge.java index c2f64c1113f..16a2f1e59b1 100644 --- a/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/LabelledGauge.java +++ b/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/LabelledGauge.java @@ -26,5 +26,12 @@ public interface LabelledGauge { */ void labels(final DoubleSupplier valueSupplier, final String... labelValues); + /** + * Checks whether the supplied labelValues are already observed by this LabelledGauge + * + * @param labelValues The labelValues to check + * @return true if the supplied labelValues are already observed by this LabelledGauge, false + * otherwise + */ boolean isLabelsObserved(final String... labelValues); } From b3a252be4337b160d35b883952b228420dbbfc55 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Mon, 14 Oct 2024 08:53:23 +1100 Subject: [PATCH 42/49] 7311: Update plugin-api hash Signed-off-by: Matilda Clerke --- plugin-api/build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin-api/build.gradle b/plugin-api/build.gradle index 0b44097b90c..5d2d28d196f 100644 --- a/plugin-api/build.gradle +++ b/plugin-api/build.gradle @@ -71,7 +71,7 @@ Calculated : ${currentHash} tasks.register('checkAPIChanges', FileStateChecker) { description = "Checks that the API for the Plugin-API project does not change without deliberate thought" files = sourceSets.main.allJava.files - knownHash = 'LBcFfNI00D9oKdnuLo2t5sXZZXfYZQSDB/t58U0PmgI=' + knownHash = 'VN2JB2HPpEUDQaDvd7QcMkmmgedasVChfA8tnSf1GHU=' } check.dependsOn('checkAPIChanges') From 3c96ebaf09679c098722b9c260ce86f6a69f4288 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Mon, 14 Oct 2024 09:13:20 +1100 Subject: [PATCH 43/49] 7311: Update changelog Signed-off-by: Matilda Clerke --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c26ef6e777..91a23d61fa8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## [Unreleased] ### Breaking Changes +- Added isLabelsObserved to LabelledGauge in plugin-api. Implementing classes will need to also implement this method ### Upcoming Breaking Changes From 44fd3a88705f0b3bce0b4f4c4e56d5eb3bfcf18e Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Thu, 17 Oct 2024 10:17:21 +1100 Subject: [PATCH 44/49] 7311: Use taskName instead of className for labelNames Signed-off-by: Matilda Clerke --- .../eth/manager/peertask/PeerTaskExecutor.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 2653fd6f359..e769fcc8e4c 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -58,37 +58,37 @@ public PeerTaskExecutor( BesuMetricCategory.PEERS, "PeerTaskExecutor:RequestTime", "Time taken to send a request and receive a response", - "className"); + "taskName"); partialSuccessCounter = metricsSystem.createLabelledCounter( BesuMetricCategory.PEERS, "PeerTaskExecutor:PartialSuccessCounter", "Counter of the number of partial success occurred", - "className"); + "taskName"); timeoutCounter = metricsSystem.createLabelledCounter( BesuMetricCategory.PEERS, "PeerTaskExecutor:TimeoutCounter", "Counter of the number of timeouts occurred", - "className"); + "taskName"); invalidResponseCounter = metricsSystem.createLabelledCounter( BesuMetricCategory.PEERS, "PeerTaskExecutor:InvalidResponseCounter", "Counter of the number of invalid responses received", - "className"); + "taskName"); internalExceptionCounter = metricsSystem.createLabelledCounter( BesuMetricCategory.PEERS, "PeerTaskExecutor:InternalExceptionCounter", "Counter of the number of internal exceptions occurred", - "className"); + "taskName"); inflightRequestGauge = metricsSystem.createLabelledGauge( BesuMetricCategory.PEERS, "PeerTaskExecutor:InflightRequestGauge", "Gauge of the number of inflight requests", - "className"); + "taskName"); inflightRequestCountByClassName = new ConcurrentHashMap<>(); } From ac1c4ed9d475f08c885cc71db42443c6fa52c540 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Thu, 17 Oct 2024 10:19:47 +1100 Subject: [PATCH 45/49] 7311: Use snake_case for metric names Signed-off-by: Matilda Clerke --- .../eth/manager/peertask/PeerTaskExecutor.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index e769fcc8e4c..fcd53281573 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -56,37 +56,37 @@ public PeerTaskExecutor( requestTimer = metricsSystem.createLabelledTimer( BesuMetricCategory.PEERS, - "PeerTaskExecutor:RequestTime", + "request_time", "Time taken to send a request and receive a response", "taskName"); partialSuccessCounter = metricsSystem.createLabelledCounter( BesuMetricCategory.PEERS, - "PeerTaskExecutor:PartialSuccessCounter", + "partial_success_counter", "Counter of the number of partial success occurred", "taskName"); timeoutCounter = metricsSystem.createLabelledCounter( BesuMetricCategory.PEERS, - "PeerTaskExecutor:TimeoutCounter", + "timeout_counter", "Counter of the number of timeouts occurred", "taskName"); invalidResponseCounter = metricsSystem.createLabelledCounter( BesuMetricCategory.PEERS, - "PeerTaskExecutor:InvalidResponseCounter", + "invalid_response_counter", "Counter of the number of invalid responses received", "taskName"); internalExceptionCounter = metricsSystem.createLabelledCounter( BesuMetricCategory.PEERS, - "PeerTaskExecutor:InternalExceptionCounter", + "internal_exception_counter", "Counter of the number of internal exceptions occurred", "taskName"); inflightRequestGauge = metricsSystem.createLabelledGauge( BesuMetricCategory.PEERS, - "PeerTaskExecutor:InflightRequestGauge", + "inflight_request_gauge", "Gauge of the number of inflight requests", "taskName"); inflightRequestCountByClassName = new ConcurrentHashMap<>(); From 750353585f8368312d4b3743c3001f8027557c04 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Thu, 17 Oct 2024 10:21:16 +1100 Subject: [PATCH 46/49] 7311: Use _total metric name suffix Signed-off-by: Matilda Clerke --- .../ethereum/eth/manager/peertask/PeerTaskExecutor.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index fcd53281573..06717914360 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -62,25 +62,25 @@ public PeerTaskExecutor( partialSuccessCounter = metricsSystem.createLabelledCounter( BesuMetricCategory.PEERS, - "partial_success_counter", + "partial_success_total", "Counter of the number of partial success occurred", "taskName"); timeoutCounter = metricsSystem.createLabelledCounter( BesuMetricCategory.PEERS, - "timeout_counter", + "timeout_total", "Counter of the number of timeouts occurred", "taskName"); invalidResponseCounter = metricsSystem.createLabelledCounter( BesuMetricCategory.PEERS, - "invalid_response_counter", + "invalid_response_total", "Counter of the number of invalid responses received", "taskName"); internalExceptionCounter = metricsSystem.createLabelledCounter( BesuMetricCategory.PEERS, - "internal_exception_counter", + "internal_exception_total", "Counter of the number of internal exceptions occurred", "taskName"); inflightRequestGauge = From ed2594169dfb84fe71850adf6ae74e35150ad139 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Thu, 17 Oct 2024 14:12:58 +1100 Subject: [PATCH 47/49] 7311: rework partial success handling Signed-off-by: Matilda Clerke --- .../eth/manager/peertask/PeerTask.java | 6 +++--- .../manager/peertask/PeerTaskExecutor.java | 19 +++++++------------ .../PeerTaskExecutorResponseCode.java | 1 - .../peertask/PeerTaskExecutorTest.java | 15 +++++++-------- 4 files changed, 17 insertions(+), 24 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java index 40b995503f8..1243846ac3d 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java @@ -75,9 +75,9 @@ default int getRetriesWithSamePeer() { Predicate getPeerRequirementFilter(); /** - * Checks if the supplied result is considered a partial success + * Checks if the supplied result is considered a success * - * @return true if the supplied result is considered a partial success + * @return true if the supplied result is considered a success */ - boolean isPartialSuccess(T result); + boolean isSuccess(T result); } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 06717914360..984cedccecb 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -40,7 +40,6 @@ public class PeerTaskExecutor { private final PeerTaskRequestSender requestSender; private final LabelledMetric requestTimer; - private final LabelledMetric partialSuccessCounter; private final LabelledMetric timeoutCounter; private final LabelledMetric invalidResponseCounter; private final LabelledMetric internalExceptionCounter; @@ -59,12 +58,6 @@ public PeerTaskExecutor( "request_time", "Time taken to send a request and receive a response", "taskName"); - partialSuccessCounter = - metricsSystem.createLabelledCounter( - BesuMetricCategory.PEERS, - "partial_success_total", - "Counter of the number of partial success occurred", - "taskName"); timeoutCounter = metricsSystem.createLabelledCounter( BesuMetricCategory.PEERS, @@ -145,16 +138,18 @@ public PeerTaskExecutorResult executeAgainstPeer( inflightRequestCountForThisTaskClass.decrementAndGet(); } - if (peerTask.isPartialSuccess(result)) { - partialSuccessCounter.labels(taskClassName).inc(); + if (peerTask.isSuccess(result)) { + peer.recordUsefulResponse(); executorResult = new PeerTaskExecutorResult<>( - Optional.ofNullable(result), PeerTaskExecutorResponseCode.PARTIAL_SUCCESS); + Optional.ofNullable(result), PeerTaskExecutorResponseCode.SUCCESS); } else { - peer.recordUsefulResponse(); + // At this point, the result is most likely empty. Technically, this is a valid result, so + // we don't penalise the peer, but it's also a useless result, so we return + // INVALID_RESPONSE code executorResult = new PeerTaskExecutorResult<>( - Optional.ofNullable(result), PeerTaskExecutorResponseCode.SUCCESS); + Optional.ofNullable(result), PeerTaskExecutorResponseCode.INVALID_RESPONSE); } } catch (PeerNotConnected e) { diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResponseCode.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResponseCode.java index 123c3267c04..327461de15a 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResponseCode.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResponseCode.java @@ -16,7 +16,6 @@ public enum PeerTaskExecutorResponseCode { SUCCESS, - PARTIAL_SUCCESS, NO_PEER_AVAILABLE, PEER_DISCONNECTED, INTERNAL_SERVER_ERROR, diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index 853210e685f..4226f3a1332 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -73,7 +73,7 @@ public void testExecuteAgainstPeerWithNoRetriesAndSuccessfulFlow() Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); - Mockito.when(peerTask.isPartialSuccess(responseObject)).thenReturn(false); + Mockito.when(peerTask.isSuccess(responseObject)).thenReturn(true); PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); @@ -102,14 +102,13 @@ public void testExecuteAgainstPeerWithNoRetriesAndPartialSuccessfulFlow() Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); - Mockito.when(peerTask.isPartialSuccess(responseObject)).thenReturn(true); + Mockito.when(peerTask.isSuccess(responseObject)).thenReturn(false); PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); Assertions.assertNotNull(result); - Assertions.assertTrue(result.result().isPresent()); - Assertions.assertSame(responseObject, result.result().get()); - Assertions.assertEquals(PeerTaskExecutorResponseCode.PARTIAL_SUCCESS, result.responseCode()); + Assertions.assertTrue(result.result().isEmpty()); + Assertions.assertEquals(PeerTaskExecutorResponseCode.INVALID_RESPONSE, result.responseCode()); } @Test @@ -132,7 +131,7 @@ public void testExecuteAgainstPeerWithRetriesAndSuccessfulFlowAfterFirstFailure( .thenReturn(responseMessageData); Mockito.when(requestMessageData.getCode()).thenReturn(requestMessageDataCode); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); - Mockito.when(peerTask.isPartialSuccess(responseObject)).thenReturn(false); + Mockito.when(peerTask.isSuccess(responseObject)).thenReturn(true); PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); @@ -238,7 +237,7 @@ public void testExecuteWithNoRetriesAndSuccessFlow() Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); - Mockito.when(peerTask.isPartialSuccess(responseObject)).thenReturn(false); + Mockito.when(peerTask.isSuccess(responseObject)).thenReturn(true); PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); @@ -276,7 +275,7 @@ public void testExecuteWithPeerSwitchingAndSuccessFlow() Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, peer2)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); - Mockito.when(peerTask.isPartialSuccess(responseObject)).thenReturn(false); + Mockito.when(peerTask.isSuccess(responseObject)).thenReturn(true); PeerTaskExecutorResult result = peerTaskExecutor.execute(peerTask); From 5a796369e23eff5b9176d9940d2b7ac4e92f55f1 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Thu, 17 Oct 2024 14:52:46 +1100 Subject: [PATCH 48/49] 7311: Add default implementation to LabelledGauge.isLabelsObserved Signed-off-by: Matilda Clerke --- CHANGELOG.md | 2 +- plugin-api/build.gradle | 2 +- .../besu/plugin/services/metrics/LabelledGauge.java | 4 +++- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd5c7ccb0e7..d9c8290f26d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,9 +1,9 @@ # Changelog ## [Unreleased] +- Added isLabelsObserved to LabelledGauge in plugin-api. Default implementation returns false. ### Breaking Changes -- Added isLabelsObserved to LabelledGauge in plugin-api. Implementing classes will need to also implement this method ### Upcoming Breaking Changes diff --git a/plugin-api/build.gradle b/plugin-api/build.gradle index 5d2d28d196f..91fb45239dc 100644 --- a/plugin-api/build.gradle +++ b/plugin-api/build.gradle @@ -71,7 +71,7 @@ Calculated : ${currentHash} tasks.register('checkAPIChanges', FileStateChecker) { description = "Checks that the API for the Plugin-API project does not change without deliberate thought" files = sourceSets.main.allJava.files - knownHash = 'VN2JB2HPpEUDQaDvd7QcMkmmgedasVChfA8tnSf1GHU=' + knownHash = 'WRdnBaP05fItpWHYSFz/vBBlRWL3sLGqzR3tzd+pOkA=' } check.dependsOn('checkAPIChanges') diff --git a/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/LabelledGauge.java b/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/LabelledGauge.java index 16a2f1e59b1..5357c6505ae 100644 --- a/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/LabelledGauge.java +++ b/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/metrics/LabelledGauge.java @@ -33,5 +33,7 @@ public interface LabelledGauge { * @return true if the supplied labelValues are already observed by this LabelledGauge, false * otherwise */ - boolean isLabelsObserved(final String... labelValues); + default boolean isLabelsObserved(final String... labelValues) { + return false; + } } From b075a917df8030372754af6c0130d422ecefaace Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Fri, 18 Oct 2024 10:29:53 +1100 Subject: [PATCH 49/49] 7311: Fix broken unit test Signed-off-by: Matilda Clerke --- .../ethereum/eth/manager/peertask/PeerTaskExecutorTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index 4226f3a1332..0262e276da2 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -107,7 +107,7 @@ public void testExecuteAgainstPeerWithNoRetriesAndPartialSuccessfulFlow() PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); Assertions.assertNotNull(result); - Assertions.assertTrue(result.result().isEmpty()); + Assertions.assertTrue(result.result().isPresent()); Assertions.assertEquals(PeerTaskExecutorResponseCode.INVALID_RESPONSE, result.responseCode()); }