Fix discovery packet size (#2252)

* Fix discovery packet size

Signed-off-by: Antoine Toulme <antoine@lunar-ocean.com>

* Add assertion to check the size of the packet once encoded

Signed-off-by: Antoine Toulme <antoine@lunar-ocean.com>

* code review

Signed-off-by: Antoine Toulme <antoine@lunar-ocean.com>

Co-authored-by: Sally MacFarlane <sally.macfarlane@consensys.net>

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

@@ -578,9 +578,12 @@ public class PeerDiscoveryController {
     if (packetData.getExpiration() < Instant.now().getEpochSecond()) {
       return;
     }
-    // TODO: for now return 16 peers. Other implementations calculate how many
-    // peers they can fit in a 1280-byte payload.
-    final List<DiscoveryPeer> peers = peerTable.nearestPeers(packetData.getTarget(), 16);
+    // Each peer is encoded as 16 bytes for address, 4 bytes for port, 4 bytes for tcp port
+    // and 64 bytes for id. This is prepended by 97 bytes of hash, signature and type.
+    // 16 + 4 + 4 + 64 = 88 bytes
+    // 88 * 13 = 1144 bytes
+    // To fit under 1280 bytes, we must return just 13 peers maximum.
+    final List<DiscoveryPeer> peers = peerTable.nearestPeers(packetData.getTarget(), 13);
     final PacketData data = NeighborsPacketData.create(peers);
     sendPacket(sender, PacketType.NEIGHBORS, data);
   }

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

@@ -224,19 +224,20 @@ public class PeerDiscoveryAgentTest {
     final IncomingPacket neighborsPacket = incomingPackets.get(0);
     assertThat(neighborsPacket.fromAgent).isEqualTo(agent);
 
-    // Assert that we only received 16 items.
+    // Assert that we only received 13 items.
     assertThat(neighborsPacket.packet.getPacketData(NeighborsPacketData.class).isPresent())
         .isTrue();
     final NeighborsPacketData neighbors =
         neighborsPacket.packet.getPacketData(NeighborsPacketData.class).get();
     assertThat(neighbors).isNotNull();
-    assertThat(neighbors.getNodes()).hasSize(16);
+    assertThat(neighbors.getNodes()).hasSize(13);
+    assertThat(neighborsPacket.packet.encode().length()).isLessThanOrEqualTo(1280); // under max MTU
 
-    // Assert that after removing those 16 items we're left with either 4 or 5.
-    // If we are left with 5, the test peer was returned as an item, assert that this is the case.
+    // Assert that after removing those 13 items we're left with either 7 or 8.
+    // If we are left with 8, the test peer was returned as an item, assert that this is the case.
     otherPeers.removeAll(neighbors.getNodes());
-    assertThat(otherPeers.size()).isBetween(4, 5);
-    if (otherPeers.size() == 5) {
+    assertThat(otherPeers.size()).isBetween(7, 8);
+    if (otherPeers.size() == 8) {
       assertThat(testAgent.getAdvertisedPeer().isPresent()).isTrue();
       assertThat(neighbors.getNodes()).contains(testAgent.getAdvertisedPeer().get());
     }