Fix attempt to send unsupported message (13) via cap eth/67 (#4732)

besu/src/main/java/org/hyperledger/besu/controller/BesuControllerBuilder.java

@@ -490,6 +490,7 @@ public abstract class BesuControllerBuilder implements MiningParameterOverrides
             clock,
             metricsSystem,
             getFullSyncTerminationCondition(protocolContext.getBlockchain()),
+            ethProtocolManager,
             pivotBlockSelector);
 
     return toUse;

consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/protocol/BftProtocolManager.java

@@ -113,4 +113,9 @@ public class BftProtocolManager implements ProtocolManager {
       final boolean initiatedByPeer) {
     peers.remove(peerConnection);
   }
+
+  @Override
+  public int getHighestProtocolVersion() {
+    return supportedCapability.getVersion();
+  }
 }

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManager.java

@@ -46,6 +46,7 @@ import org.hyperledger.besu.ethereum.worldstate.WorldStateArchive;
 
 import java.math.BigInteger;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -224,6 +225,14 @@ public class EthProtocolManager implements ProtocolManager, MinedBlockObserver {
     return capabilities.build();
   }
 
+  @Override
+  public int getHighestProtocolVersion() {
+    return getSupportedCapabilities().stream()
+        .max(Comparator.comparing(Capability::getVersion))
+        .map(Capability::getVersion)
+        .orElse(0);
+  }
+
   @Override
   public List<Capability> getSupportedCapabilities() {
     return supportedCapabilities;

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapProtocolManager.java

@@ -31,6 +31,7 @@ import org.hyperledger.besu.ethereum.rlp.RLPException;
 import org.hyperledger.besu.ethereum.worldstate.WorldStateArchive;
 
 import java.math.BigInteger;
+import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -141,4 +142,12 @@ public class SnapProtocolManager implements ProtocolManager {
       final PeerConnection connection,
       final DisconnectReason reason,
       final boolean initiatedByPeer) {}
+
+  @Override
+  public int getHighestProtocolVersion() {
+    return getSupportedCapabilities().stream()
+        .max(Comparator.comparing(Capability::getVersion))
+        .map(Capability::getVersion)
+        .orElse(0);
+  }
 }

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/DefaultSynchronizer.java

@@ -35,6 +35,7 @@ import org.hyperledger.besu.ethereum.eth.sync.state.PendingBlocksManager;
 import org.hyperledger.besu.ethereum.eth.sync.state.SyncState;
 import org.hyperledger.besu.ethereum.eth.sync.worldstate.WorldStatePeerTrieNodeFinder;
 import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule;
+import org.hyperledger.besu.ethereum.p2p.network.ProtocolManager;
 import org.hyperledger.besu.ethereum.storage.StorageProvider;
 import org.hyperledger.besu.ethereum.worldstate.PeerTrieNodeFinder;
 import org.hyperledger.besu.ethereum.worldstate.Pruner;
@@ -65,6 +66,7 @@ public class DefaultSynchronizer implements Synchronizer, UnverifiedForkchoiceLi
   private final Optional<FullSyncDownloader> fullSyncDownloader;
   private final EthContext ethContext;
   private final ProtocolContext protocolContext;
+  private final ProtocolManager protocolManager;
   private final WorldStateStorage worldStateStorage;
   private final MetricsSystem metricsSystem;
   private final PivotBlockSelector pivotBlockSelector;
@@ -84,9 +86,11 @@ public class DefaultSynchronizer implements Synchronizer, UnverifiedForkchoiceLi
       final Clock clock,
       final MetricsSystem metricsSystem,
       final SyncTerminationCondition terminationCondition,
+      final ProtocolManager protocolManager,
       final PivotBlockSelector pivotBlockSelector) {
     this.maybePruner = maybePruner;
     this.syncState = syncState;
+    this.protocolManager = protocolManager;
     this.pivotBlockSelector = pivotBlockSelector;
     this.ethContext = ethContext;
     this.protocolContext = protocolContext;
@@ -272,7 +276,7 @@ public class DefaultSynchronizer implements Synchronizer, UnverifiedForkchoiceLi
       final Optional<PeerTrieNodeFinder> fallbackNodeFinder =
           Optional.of(
               new WorldStatePeerTrieNodeFinder(
-                  ethContext, protocolContext.getBlockchain(), metricsSystem));
+                  ethContext, protocolManager, protocolContext.getBlockchain(), metricsSystem));
       ((BonsaiWorldStateArchive) protocolContext.getWorldStateArchive())
           .useFallbackNodeFinder(fallbackNodeFinder);
       ((BonsaiWorldStateKeyValueStorage) worldStateStorage)

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/worldstate/WorldStatePeerTrieNodeFinder.java

@@ -17,10 +17,12 @@ package org.hyperledger.besu.ethereum.eth.sync.worldstate;
 import org.hyperledger.besu.datatypes.Hash;
 import org.hyperledger.besu.ethereum.chain.Blockchain;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
+import org.hyperledger.besu.ethereum.eth.EthProtocolVersion;
 import org.hyperledger.besu.ethereum.eth.manager.EthContext;
 import org.hyperledger.besu.ethereum.eth.manager.snap.RetryingGetTrieNodeFromPeerTask;
 import org.hyperledger.besu.ethereum.eth.manager.task.EthTask;
 import org.hyperledger.besu.ethereum.eth.manager.task.RetryingGetNodeDataFromPeerTask;
+import org.hyperledger.besu.ethereum.p2p.network.ProtocolManager;
 import org.hyperledger.besu.ethereum.trie.CompactEncoding;
 import org.hyperledger.besu.ethereum.worldstate.PeerTrieNodeFinder;
 import org.hyperledger.besu.plugin.services.MetricsSystem;
@@ -34,6 +36,7 @@ import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
 import org.apache.tuweni.bytes.Bytes;
@@ -51,13 +54,18 @@ public class WorldStatePeerTrieNodeFinder implements PeerTrieNodeFinder {
 
   private static final long TIMEOUT_SECONDS = 1;
 
+  final ProtocolManager protocolManager;
   final EthContext ethContext;
   final Blockchain blockchain;
   final MetricsSystem metricsSystem;
 
   public WorldStatePeerTrieNodeFinder(
-      final EthContext ethContext, final Blockchain blockchain, final MetricsSystem metricsSystem) {
+      final EthContext ethContext,
+      final ProtocolManager protocolManager,
+      final Blockchain blockchain,
+      final MetricsSystem metricsSystem) {
     this.ethContext = ethContext;
+    this.protocolManager = protocolManager;
     this.blockchain = blockchain;
     this.metricsSystem = metricsSystem;
   }
@@ -90,7 +98,10 @@ public class WorldStatePeerTrieNodeFinder implements PeerTrieNodeFinder {
       return cachedValue;
     }
     final Optional<Bytes> response =
-        findByGetNodeData(Hash.wrap(nodeHash))
+        // Call findByGetNodeData only if protocol version < eth 67
+        (protocolManager.getHighestProtocolVersion() < EthProtocolVersion.V67
+                ? findByGetNodeData(Hash.wrap(nodeHash))
+                : Optional.<Bytes>empty())
             .or(
                 () ->
                     findByGetTrieNodeData(Hash.wrap(nodeHash), Optional.of(accountHash), location));
@@ -105,7 +116,11 @@ public class WorldStatePeerTrieNodeFinder implements PeerTrieNodeFinder {
     return response;
   }
 
+  @VisibleForTesting
   public Optional<Bytes> findByGetNodeData(final Hash nodeHash) {
+    if (protocolManager.getHighestProtocolVersion() >= EthProtocolVersion.V67) {
+      return Optional.empty();
+    }
     final BlockHeader chainHead = blockchain.getChainHeadHeader();
     final RetryingGetNodeDataFromPeerTask retryingGetNodeDataFromPeerTask =
         RetryingGetNodeDataFromPeerTask.forHashes(
@@ -125,6 +140,7 @@ public class WorldStatePeerTrieNodeFinder implements PeerTrieNodeFinder {
     return Optional.empty();
   }
 
+  @VisibleForTesting
   public Optional<Bytes> findByGetTrieNodeData(
       final Hash nodeHash, final Optional<Bytes32> accountHash, final Bytes location) {
     final BlockHeader chainHead = blockchain.getChainHeadHeader();

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/worldstate/WorldStatePeerTrieNodeFinderTest.java

@@ -14,13 +14,19 @@
  */
 package org.hyperledger.besu.ethereum.eth.sync.worldstate;
 
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import org.hyperledger.besu.datatypes.Hash;
 import org.hyperledger.besu.ethereum.chain.Blockchain;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
 import org.hyperledger.besu.ethereum.core.BlockHeaderTestFixture;
+import org.hyperledger.besu.ethereum.eth.EthProtocolVersion;
 import org.hyperledger.besu.ethereum.eth.manager.EthPeers;
 import org.hyperledger.besu.ethereum.eth.manager.EthProtocolManager;
 import org.hyperledger.besu.ethereum.eth.manager.EthProtocolManagerTestUtil;
@@ -66,12 +72,16 @@ public class WorldStatePeerTrieNodeFinderTest {
 
   @Before
   public void setup() throws Exception {
-    ethProtocolManager = EthProtocolManagerTestUtil.create();
+    ethProtocolManager = spy(EthProtocolManagerTestUtil.create());
     ethPeers = ethProtocolManager.ethContext().getEthPeers();
     snapProtocolManager = SnapProtocolManagerTestUtil.create(ethPeers);
     worldStatePeerTrieNodeFinder =
-        new WorldStatePeerTrieNodeFinder(
-            ethProtocolManager.ethContext(), blockchain, new NoOpMetricsSystem());
+        spy(
+            new WorldStatePeerTrieNodeFinder(
+                ethProtocolManager.ethContext(),
+                ethProtocolManager,
+                blockchain,
+                new NoOpMetricsSystem()));
   }
 
   private RespondingEthPeer.Responder respondToGetNodeDataRequest(
@@ -103,7 +113,7 @@ public class WorldStatePeerTrieNodeFinderTest {
 
     BlockHeader blockHeader = blockHeaderBuilder.number(1000).buildHeader();
     when(blockchain.getChainHeadHeader()).thenReturn(blockHeader);
-
+    when(ethProtocolManager.getHighestProtocolVersion()).thenReturn(EthProtocolVersion.V66);
     final Bytes32 nodeValue = Bytes32.random();
     final Bytes32 nodeHash = Hash.hash(nodeValue);
 
@@ -133,6 +143,7 @@ public class WorldStatePeerTrieNodeFinderTest {
 
     final BlockHeader blockHeader = blockHeaderBuilder.number(1000).buildHeader();
     when(blockchain.getChainHeadHeader()).thenReturn(blockHeader);
+    when(ethProtocolManager.getHighestProtocolVersion()).thenReturn(EthProtocolVersion.V66);
 
     final Bytes32 nodeValue = Bytes32.random();
     final Bytes32 nodeHash = Hash.hash(nodeValue);
@@ -163,6 +174,7 @@ public class WorldStatePeerTrieNodeFinderTest {
 
     final BlockHeader blockHeader = blockHeaderBuilder.number(1000).buildHeader();
     when(blockchain.getChainHeadHeader()).thenReturn(blockHeader);
+    when(ethProtocolManager.getHighestProtocolVersion()).thenReturn(EthProtocolVersion.V66);
 
     final Bytes32 nodeValue = Bytes32.random();
     final Bytes32 nodeHash = Hash.hash(nodeValue);
@@ -182,6 +194,7 @@ public class WorldStatePeerTrieNodeFinderTest {
 
     BlockHeader blockHeader = blockHeaderBuilder.number(1000).buildHeader();
     when(blockchain.getChainHeadHeader()).thenReturn(blockHeader);
+    when(ethProtocolManager.getHighestProtocolVersion()).thenReturn(EthProtocolVersion.V66);
 
     final Hash accountHash = Hash.wrap(Bytes32.random());
     final Bytes32 nodeValue = Bytes32.random();
@@ -213,6 +226,7 @@ public class WorldStatePeerTrieNodeFinderTest {
 
     final BlockHeader blockHeader = blockHeaderBuilder.number(1000).buildHeader();
     when(blockchain.getChainHeadHeader()).thenReturn(blockHeader);
+    when(ethProtocolManager.getHighestProtocolVersion()).thenReturn(EthProtocolVersion.V66);
 
     final Hash accountHash = Hash.wrap(Bytes32.random());
     final Bytes32 nodeValue = Bytes32.random();
@@ -258,4 +272,36 @@ public class WorldStatePeerTrieNodeFinderTest {
         worldStatePeerTrieNodeFinder.getAccountStorageTrieNode(accountHash, Bytes.EMPTY, nodeHash);
     Assertions.assertThat(response.accountStateTrieNode).isEmpty();
   }
+
+  @Test
+  public void getNodeDataRequestShouldBeCalled_IfProtocolIsEth66() {
+    final BlockHeader blockHeader = blockHeaderBuilder.number(1000).buildHeader();
+    when(blockchain.getChainHeadHeader()).thenReturn(blockHeader);
+    when(ethProtocolManager.getHighestProtocolVersion()).thenReturn(EthProtocolVersion.V66);
+
+    worldStatePeerTrieNodeFinder.getAccountStorageTrieNode(
+        Hash.wrap(Bytes32.random()), Bytes.EMPTY, Hash.wrap(Bytes32.random()));
+
+    // assert findByGetNodeData is called once
+    verify(worldStatePeerTrieNodeFinder, times(1)).findByGetNodeData(any());
+
+    // assert findByGetTrieNodeData is called once
+    verify(worldStatePeerTrieNodeFinder, times(1)).findByGetTrieNodeData(any(), any(), any());
+  }
+
+  @Test
+  public void getNodeDataRequestShouldNotBeCalled_IfProtocolIsEth67() {
+    final BlockHeader blockHeader = blockHeaderBuilder.number(1000).buildHeader();
+    when(blockchain.getChainHeadHeader()).thenReturn(blockHeader);
+    when(ethProtocolManager.getHighestProtocolVersion()).thenReturn(EthProtocolVersion.V67);
+
+    worldStatePeerTrieNodeFinder.getAccountStorageTrieNode(
+        Hash.wrap(Bytes32.random()), Bytes.EMPTY, Hash.wrap(Bytes32.random()));
+
+    // assert findByGetNodeData is never called
+    verify(worldStatePeerTrieNodeFinder, never()).findByGetNodeData(any());
+
+    // assert findByGetTrieNodeData is called once
+    verify(worldStatePeerTrieNodeFinder, times(1)).findByGetTrieNodeData(any(), any(), any());
+  }
 }

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/ProtocolManager.java

@@ -69,6 +69,13 @@ public interface ProtocolManager extends AutoCloseable {
   void handleDisconnect(
       PeerConnection peerConnection, DisconnectReason disconnectReason, boolean initiatedByPeer);
 
+  /**
+   * Returns the highest capability in the list of capabilities supported by this manager.
+   *
+   * @return the highest capability
+   */
+  int getHighestProtocolVersion();
+
   @Override
   default void close() {
     stop();