Fix for SpoofAmplification(v4014) hive test

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryController.java

@@ -302,9 +302,9 @@ public void onMessage(final Packet packet, final DiscoveryPeer sender) {
     switch (packet.getType()) {
       case PING:
         if (peerPermissions.allowInboundBonding(peer)) {
-          addToPeerTable(peer);
           final PingPacketData ping = packet.getPacketData(PingPacketData.class).get();
           respondToPing(ping, packet.getHash(), peer);
+          bond(peer);
         }
         break;
       case PONG:

ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java

@@ -103,7 +103,7 @@ public void neighborsPacketLimited() {
     // hedge against missing one and duplicating another.
     assertThat(agent.streamDiscoveredPeers()).contains(otherPeers.toArray(new DiscoveryPeer[20]));
     assertThat(agent.streamDiscoveredPeers())
-        .allMatch(p -> p.getStatus() == PeerDiscoveryStatus.BONDED);
+        .allMatch(p -> p.getStatus() == PeerDiscoveryStatus.BONDING);
 
     // Use additional agent to exchange messages with agent
     final MockPeerDiscoveryAgent testAgent = helper.startDiscoveryAgent();
@@ -353,7 +353,7 @@ public void simulatePeerRestartingOnDifferentEndpoint(
 
     // Remote agent should have bonded with agent
     assertThat(agent.streamDiscoveredPeers()).hasSize(1);
-    assertThat(agent.streamDiscoveredPeers()).contains(remoteAgent.getAdvertisedPeer().get());
+    assertThat(agent.streamDiscoveredPeers()).contains(remotePeer);
 
     // Create a new remote agent with same id, and new endpoint
     remoteAgent.stop();

ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java

@@ -147,24 +147,25 @@ public void bootstrapPeersRetriesSent() {
   }
 
   private void mockPingPacketCreation(final Packet mockPacket) {
-    mockPacketCreation(PacketType.PING, Optional.empty(), mockPacket);
+    mockPacketCreation(Optional.empty(), mockPacket);
   }
 
-  private void mockPacketCreation(
-      final PacketType type, final DiscoveryPeer peer, final Packet mockPacket) {
-    mockPacketCreation(type, Optional.of(peer), mockPacket);
+  private void mockPacketCreation(final DiscoveryPeer peer, final Packet mockPacket) {
+    mockPacketCreation(Optional.of(peer), mockPacket);
   }
 
-  private void mockPacketCreation(
-      final PacketType type, final Optional<DiscoveryPeer> peer, final Packet mockPacket) {
+  private void mockPacketCreation(final Optional<DiscoveryPeer> peer, final Packet mockPacket) {
     doAnswer(
             invocation -> {
               final Consumer<Packet> handler = invocation.getArgument(2);
               handler.accept(mockPacket);
               return null;
             })
         .when(controller)
-        .createPacket(eq(type), peer.isPresent() ? matchPingDataForPeer(peer.get()) : any(), any());
+        .createPacket(
+            eq(PacketType.PING),
+            peer.isPresent() ? matchPingDataForPeer(peer.get()) : any(),
+            any());
   }
 
   @Test
@@ -276,7 +277,7 @@ public void shouldRespondToPingRequest() {
     final PingPacketData pingPacketData =
         PingPacketData.create(localEndpoint, discoPeer.getEndpoint());
     final Packet discoPeerPing = Packet.create(PacketType.PING, pingPacketData, nodeKeys.get(0));
-    mockPacketCreation(PacketType.PING, discoPeer, discoPeerPing);
+    mockPacketCreation(discoPeer, discoPeerPing);
 
     controller.onMessage(discoPeerPing, discoPeer);
 
@@ -307,7 +308,7 @@ public void shouldNotRespondToExpiredPingRequest() {
             discoPeer.getEndpoint(),
             Instant.now().getEpochSecond() - PacketData.DEFAULT_EXPIRATION_PERIOD_SEC);
     final Packet discoPeerPing = Packet.create(PacketType.PING, pingPacketData, nodeKeys.get(0));
-    mockPacketCreation(PacketType.PING, discoPeer, discoPeerPing);
+    mockPacketCreation(discoPeer, discoPeerPing);
 
     controller.onMessage(discoPeerPing, discoPeer);
 
@@ -471,6 +472,7 @@ public void findNeighborsSentAfterBondingFinished() {
     assertThat(data.getTarget()).isEqualTo(localPeer.getId());
 
     assertThat(controller.streamDiscoveredPeers()).hasSize(1);
+    assertThat(controller.streamDiscoveredPeers().findFirst().isPresent()).isTrue();
     assertThat(controller.streamDiscoveredPeers().findFirst().get().getStatus())
         .isEqualTo(PeerDiscoveryStatus.BONDED);
   }
@@ -594,41 +596,34 @@ public void stopTwice() {
   }
 
   @Test
-  public void shouldAddNewPeerWhenReceivedPingAndPeerTableBucketIsNotFull() {
+  public void shouldBondWithNewPeerWhenReceivedPing() {
     final List<DiscoveryPeer> peers = createPeersInLastBucket(localPeer, 1);
     startPeerDiscoveryController();
 
     final Packet pingPacket = mockPingPacket(peers.get(0), localPeer);
     controller.onMessage(pingPacket, peers.get(0));
-    assertThat(controller.streamDiscoveredPeers()).contains(peers.get(0));
+    verify(controller, times(1)).bond(peers.get(0));
   }
 
   @Test
-  public void shouldNotAddSelfWhenReceivedPingFromSelf() {
+  public void shouldNotAddNewPeerWhenReceivedPing() {
+    final List<DiscoveryPeer> peers = createPeersInLastBucket(localPeer, 1);
     startPeerDiscoveryController();
-    final DiscoveryPeer localPeer = DiscoveryPeer.fromEnode(this.localPeer.getEnodeURL());
-
-    final Packet pingPacket = mockPingPacket(this.localPeer, this.localPeer);
-    controller.onMessage(pingPacket, localPeer);
 
-    assertThat(controller.streamDiscoveredPeers()).doesNotContain(localPeer);
+    final Packet pingPacket = mockPingPacket(peers.get(0), localPeer);
+    controller.onMessage(pingPacket, peers.get(0));
+    assertThat(controller.streamDiscoveredPeers()).doesNotContain(peers.get(0));
   }
 
   @Test
-  public void shouldAddNewPeerWhenReceivedPingAndPeerTableBucketIsFull() {
-    final List<DiscoveryPeer> peers = createPeersInLastBucket(localPeer, 17);
+  public void shouldNotAddSelfWhenReceivedPingFromSelf() {
     startPeerDiscoveryController();
-    // Fill the last bucket.
-    for (int i = 0; i < 16; i++) {
-      peerTable.tryAdd(peers.get(i));
-    }
+    final DiscoveryPeer localPeer = DiscoveryPeer.fromEnode(this.localPeer.getEnodeURL());
 
-    final Packet pingPacket = mockPingPacket(peers.get(16), localPeer);
-    controller.onMessage(pingPacket, peers.get(16));
+    final Packet pingPacket = mockPingPacket(this.localPeer, this.localPeer);
+    controller.onMessage(pingPacket, localPeer);
 
-    assertThat(controller.streamDiscoveredPeers()).contains(peers.get(16));
-    // The first peer added should have been evicted.
-    assertThat(controller.streamDiscoveredPeers()).doesNotContain(peers.get(0));
+    assertThat(controller.streamDiscoveredPeers()).doesNotContain(localPeer);
   }
 
   @Test
@@ -668,7 +663,7 @@ public void shouldNotAddNewPeerWhenReceivedPongFromBlacklistedPeer() {
     List<NodeKey> nodeKeys = PeerDiscoveryTestHelper.generateNodeKeys(1);
     PingPacketData pingPacketData = PingPacketData.create(localEndpoint, discoPeer.getEndpoint());
     final Packet discoPeerPing = Packet.create(PacketType.PING, pingPacketData, nodeKeys.get(0));
-    mockPacketCreation(PacketType.PING, discoPeer, discoPeerPing);
+    mockPacketCreation(discoPeer, discoPeerPing);
 
     controller.start();
     verify(outboundMessageHandler, times(1))
@@ -685,13 +680,13 @@ public void shouldNotAddNewPeerWhenReceivedPongFromBlacklistedPeer() {
     nodeKeys = PeerDiscoveryTestHelper.generateNodeKeys(1);
     pingPacketData = PingPacketData.create(localEndpoint, otherPeer.getEndpoint());
     final Packet pingPacket = Packet.create(PacketType.PING, pingPacketData, nodeKeys.get(0));
-    mockPacketCreation(PacketType.PING, otherPeer, pingPacket);
+    mockPacketCreation(otherPeer, pingPacket);
 
     // Setup ping to be sent to otherPeer2 after neighbors packet is received
     nodeKeys = PeerDiscoveryTestHelper.generateNodeKeys(1);
     pingPacketData = PingPacketData.create(localEndpoint, otherPeer2.getEndpoint());
     final Packet pingPacket2 = Packet.create(PacketType.PING, pingPacketData, nodeKeys.get(0));
-    mockPacketCreation(PacketType.PING, otherPeer2, pingPacket2);
+    mockPacketCreation(otherPeer2, pingPacket2);
 
     final Packet neighborsPacket =
         MockPacketDataFactory.mockNeighborsPacket(discoPeer, otherPeer, otherPeer2);
@@ -746,7 +741,7 @@ public void shouldNotBondWithBlacklistedPeer() {
     List<NodeKey> nodeKeys = PeerDiscoveryTestHelper.generateNodeKeys(1);
     PingPacketData pingPacketData = PingPacketData.create(localEndpoint, discoPeer.getEndpoint());
     final Packet discoPeerPing = Packet.create(PacketType.PING, pingPacketData, nodeKeys.get(0));
-    mockPacketCreation(PacketType.PING, discoPeer, discoPeerPing);
+    mockPacketCreation(discoPeer, discoPeerPing);
 
     controller.start();
     verify(outboundMessageHandler, times(1)).send(any(), matchPacketOfType(PacketType.PING));
@@ -762,13 +757,13 @@ public void shouldNotBondWithBlacklistedPeer() {
     nodeKeys = PeerDiscoveryTestHelper.generateNodeKeys(1);
     pingPacketData = PingPacketData.create(localEndpoint, otherPeer.getEndpoint());
     final Packet pingPacket = Packet.create(PacketType.PING, pingPacketData, nodeKeys.get(0));
-    mockPacketCreation(PacketType.PING, otherPeer, pingPacket);
+    mockPacketCreation(otherPeer, pingPacket);
 
     // Setup ping to be sent to otherPeer2 after neighbors packet is received
     nodeKeys = PeerDiscoveryTestHelper.generateNodeKeys(1);
     pingPacketData = PingPacketData.create(localEndpoint, otherPeer2.getEndpoint());
     final Packet pingPacket2 = Packet.create(PacketType.PING, pingPacketData, nodeKeys.get(0));
-    mockPacketCreation(PacketType.PING, otherPeer2, pingPacket2);
+    mockPacketCreation(otherPeer2, pingPacket2);
 
     // Blacklist peer
     blacklist.add(otherPeer);
@@ -801,7 +796,7 @@ public void shouldRespondToNeighborsRequestFromKnownPeer() {
     final PingPacketData pingPacketData =
         PingPacketData.create(localEndpoint, discoPeer.getEndpoint());
     final Packet discoPeerPing = Packet.create(PacketType.PING, pingPacketData, nodeKeys.get(0));
-    mockPacketCreation(PacketType.PING, discoPeer, discoPeerPing);
+    mockPacketCreation(discoPeer, discoPeerPing);
 
     controller.start();
     verify(outboundMessageHandler, times(1)).send(any(), matchPacketOfType(PacketType.PING));
@@ -841,7 +836,7 @@ public void shouldNotRespondToNeighborsRequestFromUnknownPeer() {
     final PingPacketData pingPacketData =
         PingPacketData.create(localEndpoint, discoPeer.getEndpoint());
     final Packet discoPeerPing = Packet.create(PacketType.PING, pingPacketData, nodeKeys.get(0));
-    mockPacketCreation(PacketType.PING, discoPeer, discoPeerPing);
+    mockPacketCreation(discoPeer, discoPeerPing);
 
     controller.start();
     verify(outboundMessageHandler, times(1)).send(any(), matchPacketOfType(PacketType.PING));
@@ -880,7 +875,7 @@ public void shouldNotRespondToExpiredNeighborsRequest() {
     final PingPacketData pingPacketData =
         PingPacketData.create(localEndpoint, discoPeer.getEndpoint());
     final Packet discoPeerPing = Packet.create(PacketType.PING, pingPacketData, nodeKeys.get(0));
-    mockPacketCreation(PacketType.PING, discoPeer, discoPeerPing);
+    mockPacketCreation(discoPeer, discoPeerPing);
 
     controller.start();
     verify(outboundMessageHandler, times(1)).send(any(), matchPacketOfType(PacketType.PING));
@@ -923,7 +918,7 @@ public void shouldNotRespondToNeighborsRequestFromBlacklistedPeer() {
     final PingPacketData pingPacketData =
         PingPacketData.create(localEndpoint, discoPeer.getEndpoint());
     final Packet discoPeerPing = Packet.create(PacketType.PING, pingPacketData, nodeKeys.get(0));
-    mockPacketCreation(PacketType.PING, discoPeer, discoPeerPing);
+    mockPacketCreation(discoPeer, discoPeerPing);
 
     controller.start();
     verify(outboundMessageHandler, times(1)).send(any(), matchPacketOfType(PacketType.PING));
@@ -1320,18 +1315,11 @@ private List<DiscoveryPeer> createPeersInLastBucket(final Peer host, final int n
     return newPeers;
   }
 
-  private PeerDiscoveryController startPeerDiscoveryController(
-      final DiscoveryPeer... bootstrapPeers) {
-    return startPeerDiscoveryController(LONG_DELAY_FUNCTION, bootstrapPeers);
-  }
-
-  private PeerDiscoveryController startPeerDiscoveryController(
-      final RetryDelayFunction retryDelayFunction, final DiscoveryPeer... bootstrapPeers) {
+  private void startPeerDiscoveryController(final DiscoveryPeer... bootstrapPeers) {
     // Create the controller.
     controller = getControllerBuilder().peers(bootstrapPeers).build();
-    controller.setRetryDelayFunction(retryDelayFunction);
+    controller.setRetryDelayFunction(LONG_DELAY_FUNCTION);
     controller.start();
-    return controller;
   }
 
   static class ControllerBuilder {