Use From field in a PING packet when creating a peer table entry (#…

…6225) * Use P2P 'from' host when parsing incoming P2P packets, if it is present Signed-off-by: Matthew Whitehead <[email protected]> * Use UDP source address if PING 'from' address is 127.0.0.1 and add a unit test. Signed-off-by: Matthew Whitehead <[email protected]> * Spotless Java, address PR comment Signed-off-by: Matthew Whitehead <[email protected]> * Refactor handleIncomingPacket to allow for specific trace logs to show how selection is being done Signed-off-by: Matthew Whitehead <[email protected]> * Add change log entry Signed-off-by: Matthew Whitehead <[email protected]> * Refactor handleIncomingPacket Signed-off-by: Matthew Whitehead <[email protected]> --------- Signed-off-by: Matthew Whitehead <[email protected]>
hyperledger · Jan 19, 2024 · 925f494 · 925f494
1 parent 958a072
commit 925f494
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 1 deletion.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -87,6 +87,7 @@ https://hyperledger.jfrog.io/artifactory/besu-binaries/besu/23.10.3-hotfix/besu-
 ### Bug fixes
 - Fix Docker image name clash between Besu and evmtool [#6194](https://github.com/hyperledger/besu/pull/6194)
 - Fix `logIndex` in `eth_getTransactionReceipt` JSON RPC method [#6206](https://github.com/hyperledger/besu/pull/6206)
+- Fix the way an advertised host configured with `--p2p-host` is treated when communicating with the originator of a PING packet [#6225](https://github.com/hyperledger/besu/pull/6225)
 
 ### Download Links
 https://hyperledger.jfrog.io/artifactory/besu-binaries/besu/23.10.3/besu-23.10.3.zip / sha256 da7ef8a6ceb88d3e327cacddcdb32218d1750b464c14165a74068f6dc6e0871a

diff --git a/...eum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgent.java b/...eum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgent.java
@@ -282,8 +282,29 @@ protected void handleIncomingPacket(final Endpoint sourceEndpoint, final Packet
             .flatMap(Endpoint::getTcpPort)
             .orElse(udpPort);
 
+    // If the host is present in the P2P PING packet itself, use that as the endpoint. If the P2P
+    // PING packet specifies 127.0.0.1 (the default if a custom value is not specified with
+    // --p2p-host or via a suitable --nat-method) we ignore it in favour of the UDP source address.
+    // The likelihood is that the UDP source will be 127.0.0.1 anyway, but this reduces the chance
+    // of an unexpected change in behaviour as a result of
+    // https://github.com/hyperledger/besu/issues/6224 being fixed.
+    final String host =
+        packet
+            .getPacketData(PingPacketData.class)
+            .flatMap(PingPacketData::getFrom)
+            .map(Endpoint::getHost)
+            .filter(abc -> !abc.equals("127.0.0.1"))
+            .stream()
+            .peek(
+                h ->
+                    LOG.trace(
+                        "Using \"From\" endpoint {} specified in ping packet. Ignoring UDP source host {}",
+                        h,
+                        sourceEndpoint.getHost()))
+            .findFirst()
+            .orElseGet(sourceEndpoint::getHost);
+
     // Notify the peer controller.
-    final String host = sourceEndpoint.getHost();
     final DiscoveryPeer peer =
         DiscoveryPeer.fromEnode(
             EnodeURLImpl.builder()

diff --git a/...p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java b/...p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java
@@ -244,6 +244,30 @@ public void neighborsPacketLimited() {
     }
   }
 
+  @Test
+  public void endpointHonoursCustomAdvertisedAddressInPingPacket() {
+
+    // Start a peer with the default advertised host
+    final MockPeerDiscoveryAgent agent1 = helper.startDiscoveryAgent();
+
+    // Start another peer with its advertised host set to a custom value
+    final MockPeerDiscoveryAgent agent2 = helper.startDiscoveryAgent("192.168.0.1");
+
+    // Send a PING so we can exchange messages
+    Packet packet = helper.createPingPacket(agent2, agent1);
+    helper.sendMessageBetweenAgents(agent2, agent1, packet);
+
+    // Agent 1's peers should have endpoints that match the custom advertised value...
+    agent1
+        .streamDiscoveredPeers()
+        .forEach(peer -> assertThat(peer.getEndpoint().getHost()).isEqualTo("192.168.0.1"));
+
+    // ...but agent 2's peers should have endpoints that match the default
+    agent2
+        .streamDiscoveredPeers()
+        .forEach(peer -> assertThat(peer.getEndpoint().getHost()).isEqualTo("127.0.0.1"));
+  }
+
   @Test
   public void shouldEvictPeerWhenPermissionsRevoked() {
     final PeerPermissionsDenylist denylist = PeerPermissionsDenylist.create();

diff --git a/...2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryTestHelper.java b/...2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryTestHelper.java
@@ -165,6 +165,14 @@ public MockPeerDiscoveryAgent startDiscoveryAgent(final DiscoveryPeer... bootstr
     return startDiscoveryAgent(agentBuilder);
   }
 
+  public MockPeerDiscoveryAgent startDiscoveryAgent(
+      final String advertisedHost, final DiscoveryPeer... bootstrapPeers) {
+    final AgentBuilder agentBuilder =
+        agentBuilder().bootstrapPeers(bootstrapPeers).advertisedHost(advertisedHost);
+
+    return startDiscoveryAgent(agentBuilder);
+  }
+
   /**
    * Start a single discovery agent with the provided bootstrap peers.
    *