From 5fa175060677440758d18bd7d876bdd65a335904 Mon Sep 17 00:00:00 2001 From: Stefan Pingel <16143240+pinges@users.noreply.github.com> Date: Sun, 12 May 2024 20:55:17 +1000 Subject: [PATCH] Peering - Find and remove peers from the peer table that share the same IP and TCP port with different discovery ports (#7089) Find and remove peers from the peer table that share the same IP and TCP port with different discovery ports --------- Signed-off-by: stefan.pingel@consensys.net Signed-off-by: Sally MacFarlane Co-authored-by: Sally MacFarlane --- CHANGELOG.md | 1 + ethereum/p2p/build.gradle | 1 + .../internal/PeerDiscoveryController.java | 53 +++++++++------ .../p2p/discovery/internal/PeerTable.java | 64 ++++++++++++++++++- .../internal/RecursivePeerRefreshState.java | 3 +- .../p2p/discovery/internal/PeerTableTest.java | 58 +++++++++++++++++ gradle/versions.gradle | 1 + 7 files changed, 158 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ffd0a3a2a..dcda75ad57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,7 @@ - Remove deprecated Goerli testnet [#7049](https://github.com/hyperledger/besu/pull/7049) - Default bonsai to use full-flat db and code-storage-by-code-hash [#6984](https://github.com/hyperledger/besu/pull/6894) - New RPC methods miner_setExtraData and miner_getExtraData [#7078](https://github.com/hyperledger/besu/pull/7078) +- Disconnect peers that have multiple discovery ports since they give us bad neighbours [#7089](https://github.com/hyperledger/besu/pull/7089) ### Bug fixes - Fix txpool dump/restore race condition [#6665](https://github.com/hyperledger/besu/pull/6665) diff --git a/ethereum/p2p/build.gradle b/ethereum/p2p/build.gradle index dccbe0fb87..7f56178978 100644 --- a/ethereum/p2p/build.gradle +++ b/ethereum/p2p/build.gradle @@ -56,6 +56,7 @@ dependencies { implementation 'io.tmio:tuweni-io' implementation 'io.tmio:tuweni-rlp' implementation 'io.tmio:tuweni-units' + implementation 'org.apache.commons:commons-collections4' implementation 'org.jetbrains.kotlin:kotlin-stdlib' implementation 'org.owasp.encoder:encoder' implementation 'org.xerial.snappy:snappy-java' diff --git a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryController.java b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryController.java index 7d0eff64db..8eb07e4338 100644 --- a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryController.java +++ b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryController.java @@ -310,6 +310,9 @@ public class PeerDiscoveryController { } final DiscoveryPeer peer = resolvePeer(sender); + if (peer == null) { + return; + } final Bytes peerId = peer.getId(); switch (packet.getType()) { case PING: @@ -399,30 +402,33 @@ public class PeerDiscoveryController { } private boolean addToPeerTable(final DiscoveryPeer peer) { - // Reset the last seen timestamp. - final long now = System.currentTimeMillis(); - if (peer.getFirstDiscovered() == 0) { - peer.setFirstDiscovered(now); - } - peer.setLastSeen(now); + final PeerTable.AddResult result = peerTable.tryAdd(peer); + if (result.getOutcome() != PeerTable.AddResult.AddOutcome.INVALID) { - if (peer.getStatus() != PeerDiscoveryStatus.BONDED) { - peer.setStatus(PeerDiscoveryStatus.BONDED); - connectOnRlpxLayer(peer); - } + // Reset the last seen timestamp. + final long now = System.currentTimeMillis(); + if (peer.getFirstDiscovered() == 0) { + peer.setFirstDiscovered(now); + } + peer.setLastSeen(now); - final PeerTable.AddResult result = peerTable.tryAdd(peer); + if (peer.getStatus() != PeerDiscoveryStatus.BONDED) { + peer.setStatus(PeerDiscoveryStatus.BONDED); + connectOnRlpxLayer(peer); + } - if (result.getOutcome() == PeerTable.AddResult.AddOutcome.ALREADY_EXISTED) { - // Bump peer. - peerTable.tryEvict(peer); - peerTable.tryAdd(peer); - } else if (result.getOutcome() == PeerTable.AddResult.AddOutcome.BUCKET_FULL) { - peerTable.tryEvict(result.getEvictionCandidate()); - peerTable.tryAdd(peer); - } + if (result.getOutcome() == PeerTable.AddResult.AddOutcome.ALREADY_EXISTED) { + // Bump peer. + peerTable.tryEvict(peer); + peerTable.tryAdd(peer); + } else if (result.getOutcome() == PeerTable.AddResult.AddOutcome.BUCKET_FULL) { + peerTable.tryEvict(result.getEvictionCandidate()); + peerTable.tryAdd(peer); + } - return true; + return true; + } + return false; } void connectOnRlpxLayer(final DiscoveryPeer peer) { @@ -688,7 +694,9 @@ public class PeerDiscoveryController { public void handleBondingRequest(final DiscoveryPeer peer) { final DiscoveryPeer peerToBond = resolvePeer(peer); - + if (peerToBond == null) { + return; + } if (peerPermissions.allowOutboundBonding(peerToBond) && PeerDiscoveryStatus.KNOWN.equals(peerToBond.getStatus())) { bond(peerToBond); @@ -697,6 +705,9 @@ public class PeerDiscoveryController { // Load the peer first from the table, then from bonding cache or use the instance that comes in. private DiscoveryPeer resolvePeer(final DiscoveryPeer peer) { + if (peerTable.ipAddressIsInvalid(peer.getEndpoint())) { + return null; + } final Optional maybeKnownPeer = peerTable.get(peer).filter(known -> known.discoveryEndpointMatches(peer)); DiscoveryPeer resolvedPeer = maybeKnownPeer.orElse(peer); diff --git a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerTable.java b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerTable.java index f0e0be1fe2..50250e1c2a 100644 --- a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerTable.java +++ b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerTable.java @@ -19,12 +19,14 @@ import static java.util.stream.Collectors.toList; import org.hyperledger.besu.crypto.Hash; import org.hyperledger.besu.ethereum.p2p.discovery.DiscoveryPeer; +import org.hyperledger.besu.ethereum.p2p.discovery.Endpoint; import org.hyperledger.besu.ethereum.p2p.discovery.PeerDiscoveryStatus; import org.hyperledger.besu.ethereum.p2p.discovery.internal.PeerTable.AddResult.AddOutcome; import org.hyperledger.besu.ethereum.p2p.peers.Peer; import org.hyperledger.besu.ethereum.p2p.peers.PeerId; import java.util.Arrays; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Optional; @@ -33,6 +35,7 @@ import java.util.concurrent.ForkJoinPool; import java.util.stream.Stream; import com.google.common.hash.BloomFilter; +import org.apache.commons.collections4.queue.CircularFifoQueue; import org.apache.tuweni.bytes.Bytes; /** @@ -51,6 +54,10 @@ public class PeerTable { private final Map distanceCache; private BloomFilter idBloom; private int evictionCnt = 0; + private final LinkedHashMapWithMaximumSize ipAddressCheckMap = + new LinkedHashMapWithMaximumSize<>(DEFAULT_BUCKET_SIZE * N_BUCKETS); + private final CircularFifoQueue invalidIPs = + new CircularFifoQueue<>(DEFAULT_BUCKET_SIZE * N_BUCKETS); /** * Builds a new peer table, where distance is calculated using the provided nodeId as a baseline. @@ -97,6 +104,7 @@ public class PeerTable { *
  • the operation failed because the k-bucket was full, in which case a candidate is proposed * for eviction. *
  • the operation failed because the peer already existed. + *
  • the operation failed because the IP address is invalid. * * * @param peer The peer to add. @@ -104,6 +112,9 @@ public class PeerTable { * @see AddOutcome */ public AddResult tryAdd(final DiscoveryPeer peer) { + if (ipAddressIsInvalid(peer.getEndpoint())) { + return AddResult.invalid(); + } final Bytes id = peer.getId(); final int distance = distanceFrom(peer); @@ -129,6 +140,7 @@ public class PeerTable { if (!res.isPresent()) { idBloom.put(id); distanceCache.put(id, distance); + ipAddressCheckMap.put(getKey(peer.getEndpoint()), peer.getEndpoint().getUdpPort()); return AddResult.added(); } @@ -200,6 +212,34 @@ public class PeerTable { return Arrays.stream(table).flatMap(e -> e.getPeers().stream()); } + boolean ipAddressIsInvalid(final Endpoint endpoint) { + final String key = getKey(endpoint); + if (invalidIPs.contains(key)) { + return true; + } + if (ipAddressCheckMap.containsKey(key) && ipAddressCheckMap.get(key) != endpoint.getUdpPort()) { + // This peer has multiple discovery services on the same IP address + TCP port. + invalidIPs.add(key); + for (final Bucket bucket : table) { + bucket.getPeers().stream() + .filter(p -> p.getEndpoint().getHost().equals(endpoint.getHost())) + .forEach(p -> evictAndStore(p, bucket, key)); + } + return true; + } else { + return false; + } + } + + private void evictAndStore(final DiscoveryPeer peer, final Bucket bucket, final String key) { + bucket.evict(peer); + invalidIPs.add(key); + } + + private static String getKey(final Endpoint endpoint) { + return endpoint.getHost() + endpoint.getFunctionalTcpPort(); + } + /** * Calculates the XOR distance between the keccak-256 hashes of our node ID and the provided * {@link DiscoveryPeer}. @@ -216,6 +256,7 @@ public class PeerTable { /** A class that encapsulates the result of a peer addition to the table. */ public static class AddResult { + /** The outcome of the operation. */ public enum AddOutcome { @@ -229,7 +270,10 @@ public class PeerTable { ALREADY_EXISTED, /** The caller requested to add ourselves. */ - SELF + SELF, + + /** The peer was not added because the IP address is invalid. */ + INVALID } private final AddOutcome outcome; @@ -256,6 +300,10 @@ public class PeerTable { return new AddResult(AddOutcome.SELF, null); } + public static AddResult invalid() { + return new AddResult((AddOutcome.INVALID), null); + } + public AddOutcome getOutcome() { return outcome; } @@ -265,6 +313,20 @@ public class PeerTable { } } + private static class LinkedHashMapWithMaximumSize extends LinkedHashMap { + private final int maxSize; + + public LinkedHashMapWithMaximumSize(final int maxSize) { + super(maxSize, 0.75f, false); + this.maxSize = maxSize; + } + + @Override + protected boolean removeEldestEntry(final Map.Entry eldest) { + return size() > maxSize; + } + } + static class EvictResult { public enum EvictOutcome { EVICTED, diff --git a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java index a392ee034d..0f3073dfd1 100644 --- a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java +++ b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java @@ -185,7 +185,8 @@ public class RecursivePeerRefreshState { private boolean satisfiesMapAdditionCriteria(final DiscoveryPeer discoPeer) { return !oneTrueMap.containsKey(discoPeer.getId()) && (initialPeers.contains(discoPeer) || !peerTable.get(discoPeer).isPresent()) - && !discoPeer.getId().equals(localPeer.getId()); + && !discoPeer.getId().equals(localPeer.getId()) + && !peerTable.ipAddressIsInvalid(discoPeer.getEndpoint()); } void onNeighboursReceived(final DiscoveryPeer peer, final List peers) { diff --git a/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerTableTest.java b/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerTableTest.java index dff9d23165..bcef09da81 100644 --- a/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerTableTest.java +++ b/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerTableTest.java @@ -184,4 +184,62 @@ public class PeerTableTest { final EvictResult evictResult = table.tryEvict(peer); assertThat(evictResult.getOutcome()).isEqualTo(EvictOutcome.SELF); } + + @Test + public void ipAddressIsInvalidReturnsTrue() { + final Endpoint endpoint1 = new Endpoint("1.1.1.1", 2, Optional.of(Integer.valueOf(1))); + final Endpoint endpoint2 = new Endpoint("1.1.1.1", 3, Optional.of(Integer.valueOf(1))); + final DiscoveryPeer peer1 = DiscoveryPeer.fromIdAndEndpoint(Peer.randomId(), endpoint1); + final DiscoveryPeer peer2 = DiscoveryPeer.fromIdAndEndpoint(Peer.randomId(), endpoint2); + final PeerTable table = new PeerTable(Bytes.random(64)); + + final PeerTable.AddResult addResult1 = table.tryAdd(peer1); + assertThat(addResult1.getOutcome()).isEqualTo(PeerTable.AddResult.added().getOutcome()); + + assertThat(table.ipAddressIsInvalid(peer2.getEndpoint())).isEqualTo(true); + } + + @Test + public void ipAddressIsInvalidReturnsFalse() { + final Endpoint endpoint1 = new Endpoint("1.1.1.1", 2, Optional.of(Integer.valueOf(1))); + final Endpoint endpoint2 = new Endpoint("1.1.1.1", 3, Optional.of(Integer.valueOf(2))); + final DiscoveryPeer peer1 = DiscoveryPeer.fromIdAndEndpoint(Peer.randomId(), endpoint1); + final DiscoveryPeer peer2 = DiscoveryPeer.fromIdAndEndpoint(Peer.randomId(), endpoint2); + final PeerTable table = new PeerTable(Bytes.random(64)); + + final PeerTable.AddResult addResult1 = table.tryAdd(peer1); + assertThat(addResult1.getOutcome()).isEqualTo(PeerTable.AddResult.added().getOutcome()); + + assertThat(table.ipAddressIsInvalid(peer2.getEndpoint())).isEqualTo(false); + } + + @Test + public void invalidIPAddressNotAdded() { + final Endpoint endpoint1 = new Endpoint("1.1.1.1", 2, Optional.of(Integer.valueOf(1))); + final Endpoint endpoint2 = new Endpoint("1.1.1.1", 3, Optional.of(Integer.valueOf(1))); + final DiscoveryPeer peer1 = DiscoveryPeer.fromIdAndEndpoint(Peer.randomId(), endpoint1); + final DiscoveryPeer peer2 = DiscoveryPeer.fromIdAndEndpoint(Peer.randomId(), endpoint2); + final PeerTable table = new PeerTable(Bytes.random(64)); + + final PeerTable.AddResult addResult1 = table.tryAdd(peer1); + assertThat(addResult1.getOutcome()).isEqualTo(PeerTable.AddResult.added().getOutcome()); + + final PeerTable.AddResult addResult2 = table.tryAdd(peer2); + assertThat(addResult2.getOutcome()).isEqualTo(PeerTable.AddResult.invalid().getOutcome()); + } + + @Test + public void validIPAddressAdded() { + final Endpoint endpoint1 = new Endpoint("1.1.1.1", 2, Optional.of(Integer.valueOf(1))); + final Endpoint endpoint2 = new Endpoint("1.1.1.1", 3, Optional.of(Integer.valueOf(2))); + final DiscoveryPeer peer1 = DiscoveryPeer.fromIdAndEndpoint(Peer.randomId(), endpoint1); + final DiscoveryPeer peer2 = DiscoveryPeer.fromIdAndEndpoint(Peer.randomId(), endpoint2); + final PeerTable table = new PeerTable(Bytes.random(64)); + + final PeerTable.AddResult addResult1 = table.tryAdd(peer1); + assertThat(addResult1.getOutcome()).isEqualTo(PeerTable.AddResult.added().getOutcome()); + + final PeerTable.AddResult addResult2 = table.tryAdd(peer2); + assertThat(addResult2.getOutcome()).isEqualTo(PeerTable.AddResult.added().getOutcome()); + } } diff --git a/gradle/versions.gradle b/gradle/versions.gradle index fa82df18a5..427e6e8189 100644 --- a/gradle/versions.gradle +++ b/gradle/versions.gradle @@ -134,6 +134,7 @@ dependencyManagement { dependency 'org.apache.commons:commons-compress:1.26.0' dependency 'org.apache.commons:commons-lang3:3.14.0' dependency 'org.apache.commons:commons-text:1.11.0' + dependency 'org.apache.commons:commons-collections4:4.4' dependencySet(group: 'org.apache.logging.log4j', version: '2.22.1') { entry 'log4j-api'