From 925f4946b8cd62bbb047558c857e47b38a4957f1 Mon Sep 17 00:00:00 2001 From: Matt Whitehead Date: Fri, 19 Jan 2024 10:34:02 +0000 Subject: [PATCH] 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 * Use UDP source address if PING 'from' address is 127.0.0.1 and add a unit test. Signed-off-by: Matthew Whitehead * Spotless Java, address PR comment Signed-off-by: Matthew Whitehead * Refactor handleIncomingPacket to allow for specific trace logs to show how selection is being done Signed-off-by: Matthew Whitehead * Add change log entry Signed-off-by: Matthew Whitehead * Refactor handleIncomingPacket Signed-off-by: Matthew Whitehead --------- Signed-off-by: Matthew Whitehead --- CHANGELOG.md | 1 + .../p2p/discovery/PeerDiscoveryAgent.java | 23 +++++++++++++++++- .../p2p/discovery/PeerDiscoveryAgentTest.java | 24 +++++++++++++++++++ .../discovery/PeerDiscoveryTestHelper.java | 8 +++++++ 4 files changed, 55 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eecedac55ad..7f84aa88b75 100644 --- 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/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgent.java b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgent.java index 8af596018a1..2324630d831 100644 --- a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgent.java +++ b/ethereum/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/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java b/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java index 97d167a26fb..240f4673eb5 100644 --- a/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java +++ b/ethereum/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/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryTestHelper.java b/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryTestHelper.java index e51b6320a6e..464273243ad 100644 --- a/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryTestHelper.java +++ b/ethereum/p2p/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. *