From d818464f3b4741480984f0cbd6c99e6e5818b122 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Thu, 2 May 2019 17:43:10 -0400 Subject: [PATCH 01/19] Add LimitedSet utility --- .../pegasys/pantheon/util/LimitedSet.java | 48 +++++++++++++++ .../pegasys/pantheon/util/LimitedSetTest.java | 60 +++++++++++++++++++ 2 files changed, 108 insertions(+) create mode 100644 util/src/main/java/tech/pegasys/pantheon/util/LimitedSet.java create mode 100644 util/src/test/java/tech/pegasys/pantheon/util/LimitedSetTest.java diff --git a/util/src/main/java/tech/pegasys/pantheon/util/LimitedSet.java b/util/src/main/java/tech/pegasys/pantheon/util/LimitedSet.java new file mode 100644 index 0000000000..7bb3573089 --- /dev/null +++ b/util/src/main/java/tech/pegasys/pantheon/util/LimitedSet.java @@ -0,0 +1,48 @@ +/* + * Copyright 2019 ConsenSys AG. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ +package tech.pegasys.pantheon.util; + +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Set; + +/** Helper that creates a thread-safe set with a maximum capacity. */ +public final class LimitedSet { + public enum Mode { + DROP_LEAST_RECENTLY_ACCESSED, + DROP_OLDEST_ELEMENT + } + + private LimitedSet() {} + + /** + * @param initialCapacity The initial size to allocate for the set. + * @param maxSize The maximum number of elements to keep in the set. + * @param mode A mode that determines which element is evicted when the set exceeds its max size. + * @param The type of object held in the set. + * @return A thread-safe set that will evict elements when the max size is exceeded. + */ + public static final Set create( + final int initialCapacity, final int maxSize, final Mode mode) { + final boolean useAccessOrder = mode.equals(Mode.DROP_LEAST_RECENTLY_ACCESSED); + return Collections.synchronizedSet( + Collections.newSetFromMap( + new LinkedHashMap(initialCapacity, 0.75f, useAccessOrder) { + @Override + protected boolean removeEldestEntry(final Map.Entry eldest) { + return size() > maxSize; + } + })); + } +} diff --git a/util/src/test/java/tech/pegasys/pantheon/util/LimitedSetTest.java b/util/src/test/java/tech/pegasys/pantheon/util/LimitedSetTest.java new file mode 100644 index 0000000000..390e6df6ad --- /dev/null +++ b/util/src/test/java/tech/pegasys/pantheon/util/LimitedSetTest.java @@ -0,0 +1,60 @@ +/* + * Copyright 2019 ConsenSys AG. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ +package tech.pegasys.pantheon.util; + +import static org.assertj.core.api.Assertions.assertThat; + +import tech.pegasys.pantheon.util.LimitedSet.Mode; + +import java.util.Set; + +import org.junit.Test; + +public class LimitedSetTest { + + @Test + public void create_evictOldest() { + final Set set = LimitedSet.create(1, 2, Mode.DROP_OLDEST_ELEMENT); + set.add(1); + assertThat(set.size()).isEqualTo(1); + set.add(2); + assertThat(set.size()).isEqualTo(2); + + // Access element 1 then add a new element that will put us over the limit + set.add(1); + + set.add(3); + assertThat(set.size()).isEqualTo(2); + // Element 1 should have been evicted + assertThat(set.contains(3)).isTrue(); + assertThat(set.contains(2)).isTrue(); + } + + @Test + public void create_evictLeastRecentlyAccessed() { + final Set set = LimitedSet.create(1, 2, Mode.DROP_LEAST_RECENTLY_ACCESSED); + set.add(1); + assertThat(set.size()).isEqualTo(1); + set.add(2); + assertThat(set.size()).isEqualTo(2); + + // Access element 1 then add a new element that will put us over the limit + set.add(1); + + set.add(3); + assertThat(set.size()).isEqualTo(2); + // Element 2 should have been evicted + assertThat(set.contains(3)).isTrue(); + assertThat(set.contains(1)).isTrue(); + } +} From 885389d304ef78a06d00102932ca8ea6db68eedb Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Fri, 3 May 2019 10:52:37 -0400 Subject: [PATCH 02/19] Add a minimal PeerPermissions interface --- .../p2p/network/PeerReputationManager.java | 52 ++++++++ .../p2p/permissions/PeerPermissions.java | 84 ++++++++++++ .../permissions/PeerPermissionsBlacklist.java | 77 +++++++++++ .../PeerPermissionsBlacklistTest.java | 126 ++++++++++++++++++ .../p2p/permissions/PeerPermissionsTest.java | 96 +++++++++++++ 5 files changed, 435 insertions(+) create mode 100644 ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/PeerReputationManager.java create mode 100644 ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java create mode 100644 ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklist.java create mode 100644 ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklistTest.java create mode 100644 ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsTest.java diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/PeerReputationManager.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/PeerReputationManager.java new file mode 100644 index 0000000000..07faeb49fe --- /dev/null +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/PeerReputationManager.java @@ -0,0 +1,52 @@ +/* + * Copyright 2018 ConsenSys AG. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ +package tech.pegasys.pantheon.ethereum.p2p.network; + +import tech.pegasys.pantheon.ethereum.p2p.api.DisconnectCallback; +import tech.pegasys.pantheon.ethereum.p2p.api.PeerConnection; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissionsBlacklist; +import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage.DisconnectReason; + +import java.util.Set; + +import com.google.common.collect.ImmutableSet; + +public class PeerReputationManager implements DisconnectCallback { + private static final Set locallyTriggeredDisconnectReasons = + ImmutableSet.of( + DisconnectReason.BREACH_OF_PROTOCOL, DisconnectReason.INCOMPATIBLE_P2P_PROTOCOL_VERSION); + + private static final Set remotelyTriggeredDisconnectReasons = + ImmutableSet.of(DisconnectReason.INCOMPATIBLE_P2P_PROTOCOL_VERSION); + + private final PeerPermissionsBlacklist blacklist; + + public PeerReputationManager(final PeerPermissionsBlacklist blacklist) { + this.blacklist = blacklist; + } + + @Override + public void onDisconnect( + final PeerConnection connection, + final DisconnectReason reason, + final boolean initiatedByPeer) { + if (shouldBlock(reason, initiatedByPeer)) { + blacklist.add(connection.getPeer()); + } + } + + private boolean shouldBlock(final DisconnectReason reason, final boolean initiatedByPeer) { + return (!initiatedByPeer && locallyTriggeredDisconnectReasons.contains(reason)) + || (initiatedByPeer && remotelyTriggeredDisconnectReasons.contains(reason)); + } +} diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java new file mode 100644 index 0000000000..4a28e5b7eb --- /dev/null +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java @@ -0,0 +1,84 @@ +/* + * Copyright 2019 ConsenSys AG. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ +package tech.pegasys.pantheon.ethereum.p2p.permissions; + +import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; +import tech.pegasys.pantheon.util.Subscribers; + +import java.util.Arrays; +import java.util.List; + +public abstract class PeerPermissions { + private final Subscribers updateSubscribers = new Subscribers<>(); + + public static PeerPermissions noop() { + return new PeerPermissions() { + + @Override + public boolean isPermitted(final Peer peer) { + return true; + } + }; + } + + public static PeerPermissions combine(final PeerPermissions... permissions) { + return new CombinedPeerPermissions(Arrays.asList(permissions)); + } + + public static PeerPermissions combine(final List permissions) { + return new CombinedPeerPermissions(permissions); + } + + /** + * @param peer The {@link Peer} object representing the remote node + * @return True if we are allowed to communicate with this peer. + */ + public abstract boolean isPermitted(final Peer peer); + + public void subscribeUpdate(final PermissionsUpdateCallback callback) { + updateSubscribers.subscribe(callback); + } + + protected void dispatchUpdate() { + updateSubscribers.forEach(PermissionsUpdateCallback::onUpdate); + } + + public interface PermissionsUpdateCallback { + void onUpdate(); + } + + private static class CombinedPeerPermissions extends PeerPermissions { + private final List permissions; + + private CombinedPeerPermissions(final List permissions) { + this.permissions = permissions; + } + + @Override + public void subscribeUpdate(final PermissionsUpdateCallback callback) { + for (final PeerPermissions permission : permissions) { + permission.subscribeUpdate(callback); + } + } + + @Override + public boolean isPermitted(final Peer peer) { + for (PeerPermissions permission : permissions) { + if (!permission.isPermitted(peer)) { + return false; + } + } + return true; + } + } +} diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklist.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklist.java new file mode 100644 index 0000000000..9ae8c85c8e --- /dev/null +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklist.java @@ -0,0 +1,77 @@ +/* + * Copyright 2019 ConsenSys AG. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ +package tech.pegasys.pantheon.ethereum.p2p.permissions; + +import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; +import tech.pegasys.pantheon.util.LimitedSet; +import tech.pegasys.pantheon.util.LimitedSet.Mode; +import tech.pegasys.pantheon.util.bytes.BytesValue; + +import java.util.OptionalInt; +import java.util.Set; + +import io.vertx.core.impl.ConcurrentHashSet; + +public class PeerPermissionsBlacklist extends PeerPermissions { + private static int DEFAULT_INITIAL_CAPACITY = 20; + + private final Set blacklist; + + private PeerPermissionsBlacklist(final int initialCapacity, final OptionalInt maxSize) { + if (maxSize.isPresent()) { + blacklist = + LimitedSet.create(initialCapacity, maxSize.getAsInt(), Mode.DROP_LEAST_RECENTLY_ACCESSED); + } else { + blacklist = new ConcurrentHashSet<>(initialCapacity); + } + } + + private PeerPermissionsBlacklist(final OptionalInt maxSize) { + this(DEFAULT_INITIAL_CAPACITY, maxSize); + } + + public static PeerPermissionsBlacklist create() { + return new PeerPermissionsBlacklist(OptionalInt.empty()); + } + + public static PeerPermissionsBlacklist create(final int maxSize) { + return new PeerPermissionsBlacklist(OptionalInt.of(maxSize)); + } + + @Override + public boolean isPermitted(final Peer peer) { + return !blacklist.contains(peer.getId()); + } + + public void add(final Peer peer) { + if (blacklist.add(peer.getId())) { + dispatchUpdate(); + } + } + + public void remove(final Peer peer) { + if (blacklist.remove(peer.getId())) { + dispatchUpdate(); + } + } + + public void add(final BytesValue peerId) { + blacklist.add(peerId); + dispatchUpdate(); + } + + public void remove(final BytesValue peerId) { + blacklist.remove(peerId); + dispatchUpdate(); + } +} diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklistTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklistTest.java new file mode 100644 index 0000000000..2a1fa5e80f --- /dev/null +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklistTest.java @@ -0,0 +1,126 @@ +/* + * Copyright 2019 ConsenSys AG. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ +package tech.pegasys.pantheon.ethereum.p2p.permissions; + +import static org.assertj.core.api.Assertions.assertThat; + +import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeer; +import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; +import tech.pegasys.pantheon.util.enode.EnodeURL; + +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import org.junit.Test; + +public class PeerPermissionsBlacklistTest { + + @Test + public void trackedPeerIsNotPermitted() { + PeerPermissionsBlacklist blacklist = PeerPermissionsBlacklist.create(); + + Peer peer = createPeer(); + assertThat(blacklist.isPermitted(peer)).isTrue(); + + blacklist.add(peer); + assertThat(blacklist.isPermitted(peer)).isFalse(); + + blacklist.remove(peer); + assertThat(blacklist.isPermitted(peer)).isTrue(); + } + + @Test + public void subscribeUpdate() { + PeerPermissionsBlacklist blacklist = PeerPermissionsBlacklist.create(); + final AtomicInteger callbackCount = new AtomicInteger(0); + Peer peer = createPeer(); + + blacklist.subscribeUpdate(callbackCount::incrementAndGet); + + assertThat(blacklist.isPermitted(peer)).isTrue(); + assertThat(callbackCount).hasValue(0); + + blacklist.add(peer); + assertThat(callbackCount).hasValue(1); + + blacklist.add(peer); + assertThat(callbackCount).hasValue(1); + + blacklist.remove(peer); + assertThat(callbackCount).hasValue(2); + + blacklist.remove(peer); + assertThat(callbackCount).hasValue(2); + + blacklist.add(peer); + assertThat(callbackCount).hasValue(3); + } + + @Test + public void createWithLimitedCapacity() { + final PeerPermissionsBlacklist blacklist = PeerPermissionsBlacklist.create(2); + Peer peerA = createPeer(); + Peer peerB = createPeer(); + Peer peerC = createPeer(); + + // All peers are initially permitted + assertThat(blacklist.isPermitted(peerA)).isTrue(); + assertThat(blacklist.isPermitted(peerB)).isTrue(); + assertThat(blacklist.isPermitted(peerC)).isTrue(); + + // Add peerA + blacklist.add(peerA); + assertThat(blacklist.isPermitted(peerA)).isFalse(); + assertThat(blacklist.isPermitted(peerB)).isTrue(); + assertThat(blacklist.isPermitted(peerC)).isTrue(); + + // Add peerB + blacklist.add(peerB); + assertThat(blacklist.isPermitted(peerA)).isFalse(); + assertThat(blacklist.isPermitted(peerB)).isFalse(); + assertThat(blacklist.isPermitted(peerC)).isTrue(); + + // Add peerC + // Limit is exceeded and peerA should drop off of the list and be allowed + blacklist.add(peerC); + assertThat(blacklist.isPermitted(peerA)).isTrue(); + assertThat(blacklist.isPermitted(peerB)).isFalse(); + assertThat(blacklist.isPermitted(peerC)).isFalse(); + } + + @Test + public void createWithUnlimitedCapacity() { + final PeerPermissionsBlacklist blacklist = PeerPermissionsBlacklist.create(); + final int peerCount = 200; + final List peers = + Stream.generate(this::createPeer).limit(peerCount).collect(Collectors.toList()); + + peers.forEach(p -> assertThat(blacklist.isPermitted(p)).isTrue()); + peers.forEach(blacklist::add); + peers.forEach(p -> assertThat(blacklist.isPermitted(p)).isFalse()); + + peers.forEach(blacklist::remove); + peers.forEach(p -> assertThat(blacklist.isPermitted(p)).isTrue()); + } + + private Peer createPeer() { + return DefaultPeer.fromEnodeURL( + EnodeURL.builder() + .nodeId(Peer.randomId()) + .ipAddress("127.0.0.1") + .listeningPort(EnodeURL.DEFAULT_LISTENING_PORT) + .build()); + } +} diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsTest.java new file mode 100644 index 0000000000..f6d25faffd --- /dev/null +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsTest.java @@ -0,0 +1,96 @@ +/* + * Copyright 2019 ConsenSys AG. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ +package tech.pegasys.pantheon.ethereum.p2p.permissions; + +import static org.assertj.core.api.Assertions.assertThat; + +import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeer; +import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; +import tech.pegasys.pantheon.util.enode.EnodeURL; + +import java.util.concurrent.atomic.AtomicInteger; + +import org.junit.Test; + +public class PeerPermissionsTest { + @Test + public void subscribeUpdate() { + TestPeerPermissions peerPermissions = new TestPeerPermissions(false); + final AtomicInteger callbackCount = new AtomicInteger(0); + + peerPermissions.subscribeUpdate(callbackCount::incrementAndGet); + + peerPermissions.allowPeers(true); + assertThat(callbackCount).hasValue(1); + + peerPermissions.allowPeers(false); + assertThat(callbackCount).hasValue(2); + } + + @Test + public void subscribeUpdate_forCombinedPermissions() { + TestPeerPermissions peerPermissionsA = new TestPeerPermissions(false); + TestPeerPermissions peerPermissionsB = new TestPeerPermissions(false); + PeerPermissions combined = PeerPermissions.combine(peerPermissionsA, peerPermissionsB); + + final AtomicInteger callbackCount = new AtomicInteger(0); + + combined.subscribeUpdate(callbackCount::incrementAndGet); + + peerPermissionsA.allowPeers(true); + assertThat(callbackCount).hasValue(1); + + peerPermissionsB.allowPeers(true); + assertThat(callbackCount).hasValue(2); + } + + @Test + public void isPermitted_forCombinedPermissions() { + final PeerPermissions allowPeers = new TestPeerPermissions(true); + final PeerPermissions disallowPeers = new TestPeerPermissions(false); + + Peer peer = + DefaultPeer.fromEnodeURL( + EnodeURL.builder() + .listeningPort(30303) + .discoveryPort(30303) + .nodeId(Peer.randomId()) + .ipAddress("127.0.0.1") + .build()); + + assertThat(PeerPermissions.combine(allowPeers, disallowPeers).isPermitted(peer)).isFalse(); + assertThat(PeerPermissions.combine(disallowPeers, disallowPeers).isPermitted(peer)).isFalse(); + assertThat(PeerPermissions.combine(disallowPeers, disallowPeers).isPermitted(peer)).isFalse(); + assertThat(PeerPermissions.combine(allowPeers, disallowPeers).isPermitted(peer)).isFalse(); + assertThat(PeerPermissions.combine(allowPeers, allowPeers).isPermitted(peer)).isTrue(); + } + + private static class TestPeerPermissions extends PeerPermissions { + + private boolean allowPeers; + + public TestPeerPermissions(final boolean allowPeers) { + this.allowPeers = allowPeers; + } + + public void allowPeers(final boolean doAllowPeers) { + this.allowPeers = doAllowPeers; + dispatchUpdate(); + } + + @Override + public boolean isPermitted(final Peer peer) { + return allowPeers; + } + } +} From 5467e3040afda33621866df3b728356df71f7a67 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Mon, 13 May 2019 14:13:20 -0400 Subject: [PATCH 03/19] Break up PeerBlacklist into smaller components using new interface --- .../dsl/node/ThreadPantheonNodeRunner.java | 2 - .../ethereum/eth/transactions/TestNode.java | 2 - .../JsonRpcHttpServiceRpcApisTest.java | 2 - .../p2p/discovery/PeerDiscoveryAgent.java | 10 +- .../discovery/VertxPeerDiscoveryAgent.java | 6 +- .../internal/PeerDiscoveryController.java | 18 +- .../internal/RecursivePeerRefreshState.java | 18 +- .../p2p/network/DefaultP2PNetwork.java | 46 ++-- .../ethereum/p2p/peers/PeerBlacklist.java | 117 ---------- .../p2p/discovery/PeerDiscoveryAgentTest.java | 127 +--------- .../discovery/PeerDiscoveryTestHelper.java | 20 +- .../PeerDiscoveryTimestampsTest.java | 4 +- .../internal/MockPeerDiscoveryAgent.java | 6 +- .../internal/PeerDiscoveryControllerTest.java | 38 ++- .../PeerDiscoveryTableRefreshTest.java | 4 +- .../RecursivePeerRefreshStateTest.java | 24 +- .../p2p/network/DefaultP2PNetworkTest.java | 8 +- .../NetworkingServiceLifecycleTest.java | 2 - .../ethereum/p2p/network/P2PNetworkTest.java | 10 +- .../network/PeerReputationManagerTest.java | 107 +++++++++ .../ethereum/p2p/peers/PeerBlacklistTest.java | 219 ------------------ .../tech/pegasys/pantheon/RunnerBuilder.java | 16 +- .../tech/pegasys/pantheon/RunnerTest.java | 1 - 23 files changed, 228 insertions(+), 579 deletions(-) delete mode 100644 ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklist.java create mode 100644 ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/PeerReputationManagerTest.java delete mode 100644 ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklistTest.java diff --git a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/ThreadPantheonNodeRunner.java b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/ThreadPantheonNodeRunner.java index 4a99510f3f..9e077c4a09 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/ThreadPantheonNodeRunner.java +++ b/acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/ThreadPantheonNodeRunner.java @@ -32,7 +32,6 @@ import java.io.IOException; import java.nio.file.Path; import java.time.Clock; -import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -108,7 +107,6 @@ public void startNode(final PantheonNode node) { .jsonRpcConfiguration(node.jsonRpcConfiguration()) .webSocketConfiguration(node.webSocketConfiguration()) .dataDir(node.homeDirectory()) - .bannedNodeIds(Collections.emptySet()) .metricsSystem(noOpMetricsSystem) .metricsConfiguration(node.metricsConfiguration()) .p2pEnabled(node.isP2pEnabled()) diff --git a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TestNode.java b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TestNode.java index b04fa57225..b4139d34b9 100644 --- a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TestNode.java +++ b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TestNode.java @@ -44,7 +44,6 @@ import tech.pegasys.pantheon.ethereum.p2p.network.DefaultP2PNetwork; import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeer; import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage.DisconnectReason; import tech.pegasys.pantheon.ethereum.worldstate.WorldStateArchive; import tech.pegasys.pantheon.metrics.MetricsSystem; @@ -126,7 +125,6 @@ public TestNode( .vertx(vertx) .keyPair(this.kp) .config(networkingConfiguration) - .peerBlacklist(new PeerBlacklist()) .metricsSystem(new NoOpMetricsSystem()) .supportedCapabilities(capabilities) .build()) diff --git a/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/JsonRpcHttpServiceRpcApisTest.java b/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/JsonRpcHttpServiceRpcApisTest.java index f3868ca990..b017c70799 100644 --- a/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/JsonRpcHttpServiceRpcApisTest.java +++ b/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/JsonRpcHttpServiceRpcApisTest.java @@ -37,7 +37,6 @@ import tech.pegasys.pantheon.ethereum.p2p.config.NetworkingConfiguration; import tech.pegasys.pantheon.ethereum.p2p.config.RlpxConfiguration; import tech.pegasys.pantheon.ethereum.p2p.network.DefaultP2PNetwork; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; import tech.pegasys.pantheon.ethereum.p2p.wire.Capability; import tech.pegasys.pantheon.ethereum.permissioning.AccountLocalConfigPermissioningController; import tech.pegasys.pantheon.ethereum.permissioning.NodeLocalConfigPermissioningController; @@ -243,7 +242,6 @@ private P2PNetwork createP2pNetwork() { .keyPair(SECP256K1.KeyPair.generate()) .vertx(vertx) .config(config) - .peerBlacklist(new PeerBlacklist()) .metricsSystem(new NoOpMetricsSystem()) .build(); diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java index 72e4bfd725..b794c206c9 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java @@ -31,7 +31,7 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PingPacketData; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.TimerUtil; import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeerId; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage; import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningController; import tech.pegasys.pantheon.metrics.MetricsSystem; @@ -71,7 +71,7 @@ public abstract class PeerDiscoveryAgent implements DisconnectCallback { protected final List bootstrapPeers; private final List peerRequirements = new CopyOnWriteArrayList<>(); - private final PeerBlacklist peerBlacklist; + private final PeerPermissions peerPermissions; private final Optional nodePermissioningController; private final MetricsSystem metricsSystem; /* The peer controller, which takes care of the state machine of peers. */ @@ -94,7 +94,7 @@ public abstract class PeerDiscoveryAgent implements DisconnectCallback { public PeerDiscoveryAgent( final SECP256K1.KeyPair keyPair, final DiscoveryConfiguration config, - final PeerBlacklist peerBlacklist, + final PeerPermissions peerPermissions, final Optional nodePermissioningController, final MetricsSystem metricsSystem) { this.metricsSystem = metricsSystem; @@ -103,7 +103,7 @@ public PeerDiscoveryAgent( validateConfiguration(config); - this.peerBlacklist = peerBlacklist; + this.peerPermissions = peerPermissions; this.nodePermissioningController = nodePermissioningController; this.bootstrapPeers = config.getBootnodes().stream().map(DiscoveryPeer::fromEnode).collect(Collectors.toList()); @@ -174,7 +174,7 @@ private PeerDiscoveryController createController() { createWorkerExecutor(), PEER_REFRESH_INTERVAL_MS, PeerRequirement.combine(peerRequirements), - peerBlacklist, + peerPermissions, nodePermissioningController, peerBondedObservers, peerDroppedObservers, diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/VertxPeerDiscoveryAgent.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/VertxPeerDiscoveryAgent.java index 6b307ae0a9..9de51be986 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/VertxPeerDiscoveryAgent.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/VertxPeerDiscoveryAgent.java @@ -21,7 +21,7 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerDiscoveryController.AsyncExecutor; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.TimerUtil; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.VertxTimerUtil; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningController; import tech.pegasys.pantheon.metrics.MetricCategory; import tech.pegasys.pantheon.metrics.MetricsSystem; @@ -58,10 +58,10 @@ public VertxPeerDiscoveryAgent( final Vertx vertx, final KeyPair keyPair, final DiscoveryConfiguration config, - final PeerBlacklist peerBlacklist, + final PeerPermissions peerPermissions, final Optional nodePermissioningController, final MetricsSystem metricsSystem) { - super(keyPair, config, peerBlacklist, nodePermissioningController, metricsSystem); + super(keyPair, config, peerPermissions, nodePermissioningController, metricsSystem); checkArgument(vertx != null, "vertx instance cannot be null"); this.vertx = vertx; diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java index 81b4ffb7b5..875105e9e7 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java @@ -26,7 +26,7 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerTable.EvictResult; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerTable.EvictResult.EvictOutcome; import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningController; import tech.pegasys.pantheon.metrics.Counter; import tech.pegasys.pantheon.metrics.LabelledMetric; @@ -118,7 +118,7 @@ public class PeerDiscoveryController { // The peer representation of this node private final DiscoveryPeer localPeer; private final OutboundMessageHandler outboundMessageHandler; - private final PeerBlacklist peerBlacklist; + private final PeerPermissions peerPermissions; private final Optional nodePermissioningController; private final DiscoveryProtocolLogger discoveryProtocolLogger; private final LabelledMetric interactionCounter; @@ -151,7 +151,7 @@ public PeerDiscoveryController( final AsyncExecutor workerExecutor, final long tableRefreshIntervalMs, final PeerRequirement peerRequirement, - final PeerBlacklist peerBlacklist, + final PeerPermissions peerPermissions, final Optional nodePermissioningController, final Subscribers> peerBondedObservers, final Subscribers> peerDroppedObservers, @@ -164,7 +164,7 @@ public PeerDiscoveryController( this.workerExecutor = workerExecutor; this.tableRefreshIntervalMs = tableRefreshIntervalMs; this.peerRequirement = peerRequirement; - this.peerBlacklist = peerBlacklist; + this.peerPermissions = peerPermissions; this.nodePermissioningController = nodePermissioningController; this.outboundMessageHandler = outboundMessageHandler; this.peerBondedObservers = peerBondedObservers; @@ -205,7 +205,7 @@ public void start() { recursivePeerRefreshState = new RecursivePeerRefreshState( - peerBlacklist, + peerPermissions, nodePermissioningController, this::bond, this::findNodes, @@ -286,11 +286,11 @@ public void onMessage(final Packet packet, final DiscoveryPeer sender) { final Optional maybeKnownPeer = peerTable.get(sender); final DiscoveryPeer peer = maybeKnownPeer.orElse(sender); final boolean peerKnown = maybeKnownPeer.isPresent(); - final boolean peerBlacklisted = peerBlacklist.contains(peer); + final boolean peerPermitted = peerPermissions.isPermitted(peer); switch (packet.getType()) { case PING: - if (!peerBlacklisted && addToPeerTable(peer)) { + if (peerPermitted && addToPeerTable(peer)) { final PingPacketData ping = packet.getPacketData(PingPacketData.class).get(); respondToPing(ping, packet.getHash(), peer); } @@ -299,7 +299,7 @@ public void onMessage(final Packet packet, final DiscoveryPeer sender) { matchInteraction(packet) .ifPresent( interaction -> { - if (peerBlacklisted) { + if (!peerPermitted) { return; } addToPeerTable(peer); @@ -314,7 +314,7 @@ public void onMessage(final Packet packet, final DiscoveryPeer sender) { peer, getPeersFromNeighborsPacket(packet))); break; case FIND_NEIGHBORS: - if (!peerKnown || peerBlacklisted) { + if (!peerKnown || !peerPermitted) { break; } final FindNeighborsPacketData fn = diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java index 6efd328297..d051394728 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java @@ -16,7 +16,7 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.DiscoveryPeer; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryStatus; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningController; import tech.pegasys.pantheon.util.bytes.BytesValue; @@ -38,7 +38,7 @@ public class RecursivePeerRefreshState { private static final Logger LOG = LogManager.getLogger(); private static final int MAX_CONCURRENT_REQUESTS = 3; private BytesValue target; - private final PeerBlacklist peerBlacklist; + private final PeerPermissions peerPermissions; private final Optional nodePermissioningController; private final PeerTable peerTable; private final DiscoveryPeer localPeer; @@ -58,7 +58,7 @@ public class RecursivePeerRefreshState { List initialPeers; RecursivePeerRefreshState( - final PeerBlacklist peerBlacklist, + final PeerPermissions peerPermissions, final Optional nodePermissioningController, final BondingAgent bondingAgent, final FindNeighbourDispatcher neighborFinder, @@ -67,7 +67,7 @@ public class RecursivePeerRefreshState { final PeerTable peerTable, final int timeoutPeriodInSeconds, final int maxRounds) { - this.peerBlacklist = peerBlacklist; + this.peerPermissions = peerPermissions; this.nodePermissioningController = nodePermissioningController; this.bondingAgent = bondingAgent; this.findNeighbourDispatcher = neighborFinder; @@ -186,16 +186,18 @@ private void neighboursCancelOutstandingRequests() { private boolean satisfiesMapAdditionCriteria(final DiscoveryPeer discoPeer) { return !oneTrueMap.containsKey(discoPeer.getId()) - && !peerBlacklist.contains(discoPeer) && isPeerPermitted(discoPeer) && (initialPeers.contains(discoPeer) || !peerTable.get(discoPeer).isPresent()) && !discoPeer.getId().equals(localPeer.getId()); } private Boolean isPeerPermitted(final DiscoveryPeer discoPeer) { - return nodePermissioningController - .map(controller -> controller.isPermitted(localPeer.getEnodeURL(), discoPeer.getEnodeURL())) - .orElse(true); + return peerPermissions.isPermitted(discoPeer) + && nodePermissioningController + .map( + controller -> + controller.isPermitted(localPeer.getEnodeURL(), discoPeer.getEnodeURL())) + .orElse(true); } void onNeighboursReceived(final DiscoveryPeer peer, final List peers) { diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java index d3a2e819a2..bab0e445bf 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java @@ -37,7 +37,8 @@ import tech.pegasys.pantheon.ethereum.p2p.network.netty.PeerConnectionRegistry; import tech.pegasys.pantheon.ethereum.p2p.network.netty.TimeoutHandler; import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissionsBlacklist; import tech.pegasys.pantheon.ethereum.p2p.wire.Capability; import tech.pegasys.pantheon.ethereum.p2p.wire.PeerInfo; import tech.pegasys.pantheon.ethereum.p2p.wire.SubProtocol; @@ -154,7 +155,7 @@ public class DefaultP2PNetwork implements P2PNetwork { private volatile Optional localEnode = Optional.empty(); private volatile Optional ourPeerInfo = Optional.empty(); - private final PeerBlacklist peerBlacklist; + private final PeerPermissions peerPermissions; private final Optional nodePermissioningController; private final Optional blockchain; @@ -189,7 +190,7 @@ public class DefaultP2PNetwork implements P2PNetwork { * @param keyPair This node's keypair. * @param config The network configuration to use. * @param supportedCapabilities The wire protocol capabilities to advertise to connected peers. - * @param peerBlacklist The peers with which this node will not connect + * @param peerPermissions An object that determines whether peers are allowed to connect * @param metricsSystem The metrics system to capture metrics with. * @param nodePermissioningController Controls node permissioning. * @param blockchain The blockchain to subscribe to BlockAddedEvents. @@ -199,7 +200,7 @@ public class DefaultP2PNetwork implements P2PNetwork { final SECP256K1.KeyPair keyPair, final NetworkingConfiguration config, final List supportedCapabilities, - final PeerBlacklist peerBlacklist, + final PeerPermissions peerPermissions, final MetricsSystem metricsSystem, final Optional nodePermissioningController, final Blockchain blockchain) { @@ -208,7 +209,6 @@ public class DefaultP2PNetwork implements P2PNetwork { this.keyPair = keyPair; this.config = config; this.supportedCapabilities = supportedCapabilities; - this.peerBlacklist = peerBlacklist; this.nodePermissioningController = nodePermissioningController; this.blockchain = Optional.ofNullable(blockchain); this.peerMaintainConnectionList = new HashSet<>(); @@ -218,12 +218,17 @@ public class DefaultP2PNetwork implements P2PNetwork { this.subProtocols = config.getSupportedProtocols(); this.maxPeers = config.getRlpx().getMaxPeers(); + // Set up permissions + final PeerPermissionsBlacklist misbehavingPeers = PeerPermissionsBlacklist.create(500); + PeerReputationManager reputationManager = new PeerReputationManager(misbehavingPeers); + this.peerPermissions = PeerPermissions.combine(peerPermissions, misbehavingPeers); + peerDiscoveryAgent.addPeerRequirement(() -> connections.size() >= maxPeers); this.nodePermissioningController.ifPresent( c -> c.subscribeToUpdates(this::checkCurrentConnections)); + subscribeDisconnect(reputationManager); subscribeDisconnect(peerDiscoveryAgent); - subscribeDisconnect(peerBlacklist); subscribeDisconnect(connections); outboundMessagesCounter = @@ -613,14 +618,10 @@ private synchronized void checkCurrentConnections() { } private boolean isPeerAllowed(final PeerConnection conn) { - return isPeerAllowed(conn.getRemoteEnode()); + return isPeerAllowed(conn.getPeer()); } private boolean isPeerAllowed(final Peer peer) { - return isPeerAllowed(peer.getEnodeURL()); - } - - private boolean isPeerAllowed(final EnodeURL enode) { final Optional maybeEnode = getLocalEnode(); if (!maybeEnode.isPresent()) { // If the network isn't ready yet, deny connections @@ -628,15 +629,17 @@ private boolean isPeerAllowed(final EnodeURL enode) { } final EnodeURL localEnode = maybeEnode.get(); - if (peerBlacklist.contains(enode.getNodeId())) { + if (peer.getId().equals(nodeId)) { + // Peer matches our node id return false; } - if (enode.getNodeId().equals(nodeId)) { - // Peer matches our node id + if (!peerPermissions.isPermitted(peer)) { return false; } - return nodePermissioningController.map(c -> c.isPermitted(localEnode, enode)).orElse(true); + return nodePermissioningController + .map(c -> c.isPermitted(localEnode, peer.getEnodeURL())) + .orElse(true); } @VisibleForTesting @@ -747,7 +750,7 @@ public static class Builder { private KeyPair keyPair; private NetworkingConfiguration config = NetworkingConfiguration.create(); private List supportedCapabilities; - private PeerBlacklist peerBlacklist; + private PeerPermissions peerPermissions = PeerPermissions.noop(); private MetricsSystem metricsSystem; private Optional nodePermissioningController = Optional.empty(); private Blockchain blockchain = null; @@ -766,7 +769,7 @@ private P2PNetwork doBuild() { keyPair, config, supportedCapabilities, - peerBlacklist, + peerPermissions, metricsSystem, nodePermissioningController, blockchain); @@ -778,7 +781,6 @@ private void validate() { checkState( supportedCapabilities != null && supportedCapabilities.size() > 0, "Supported capabilities must be set and non-empty."); - checkState(peerBlacklist != null, "PeerBlacklist must be set."); checkState(metricsSystem != null, "MetricsSystem must be set."); checkState( !nodePermissioningController.isPresent() || blockchain != null, @@ -792,7 +794,7 @@ private PeerDiscoveryAgent createDiscoveryAgent() { vertx, keyPair, config.getDiscovery(), - peerBlacklist, + peerPermissions, nodePermissioningController, metricsSystem); } @@ -826,9 +828,9 @@ public Builder supportedCapabilities(final Capability... supportedCapabilities) return this; } - public Builder peerBlacklist(final PeerBlacklist peerBlacklist) { - checkNotNull(peerBlacklist); - this.peerBlacklist = peerBlacklist; + public Builder peerPermissions(final PeerPermissions peerPermissions) { + checkNotNull(peerPermissions); + this.peerPermissions = peerPermissions; return this; } diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklist.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklist.java deleted file mode 100644 index 9a84be454f..0000000000 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklist.java +++ /dev/null @@ -1,117 +0,0 @@ -/* - * Copyright 2018 ConsenSys AG. - * - * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on - * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the - * specific language governing permissions and limitations under the License. - */ -package tech.pegasys.pantheon.ethereum.p2p.peers; - -import tech.pegasys.pantheon.ethereum.p2p.api.DisconnectCallback; -import tech.pegasys.pantheon.ethereum.p2p.api.PeerConnection; -import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage; -import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage.DisconnectReason; -import tech.pegasys.pantheon.util.bytes.BytesValue; - -import java.util.Collections; -import java.util.LinkedHashMap; -import java.util.Map; -import java.util.Set; - -import com.google.common.collect.ImmutableSet; - -/** - * A list of nodes that the running client will not communicate with. This can be because of network - * issues, protocol issues, or by being explicitly set on the command line. - * - *

Peers are stored and identified strictly by their nodeId, the convenience methods taking - * {@link Peer}s and {@link PeerConnection}s redirect to the methods that take {@link BytesValue} - * object that represent the node ID of the banned nodes. - * - *

The storage list is not infinite. A default cap of 500 is applied and nodes are removed on a - * first added first removed basis. Adding a new copy of the same node will not affect the priority - * for removal. The exception to this is a list of banned nodes passed in by reference to the - * constructor. This list neither adds nor removes from that list passed in. - */ -public class PeerBlacklist implements DisconnectCallback { - private static final int DEFAULT_BLACKLIST_CAP = 500; - - private static final Set locallyTriggeredBlacklistReasons = - ImmutableSet.of( - DisconnectReason.BREACH_OF_PROTOCOL, DisconnectReason.INCOMPATIBLE_P2P_PROTOCOL_VERSION); - - private static final Set remotelyTriggeredBlacklistReasons = - ImmutableSet.of(DisconnectReason.INCOMPATIBLE_P2P_PROTOCOL_VERSION); - - private final int blacklistCap; - private final Set blacklistedNodeIds = - Collections.synchronizedSet( - Collections.newSetFromMap( - new LinkedHashMap(20, 0.75f, true) { - @Override - protected boolean removeEldestEntry(final Map.Entry eldest) { - return size() > blacklistCap; - } - })); - - /** These nodes are always banned for the life of this list. They are not subject to rollover. */ - private final Set bannedNodeIds; - - public PeerBlacklist(final int blacklistCap, final Set bannedNodeIds) { - this.blacklistCap = blacklistCap; - this.bannedNodeIds = bannedNodeIds; - } - - public PeerBlacklist(final int blacklistCap) { - this(blacklistCap, Collections.emptySet()); - } - - public PeerBlacklist(final Set bannedNodeIds) { - this(DEFAULT_BLACKLIST_CAP, bannedNodeIds); - } - - public PeerBlacklist() { - this(DEFAULT_BLACKLIST_CAP, Collections.emptySet()); - } - - public boolean contains(final BytesValue nodeId) { - return blacklistedNodeIds.contains(nodeId) || bannedNodeIds.contains(nodeId); - } - - public boolean contains(final PeerConnection peer) { - return contains(peer.getPeerInfo().getNodeId()); - } - - public boolean contains(final Peer peer) { - return contains(peer.getId()); - } - - public void add(final Peer peer) { - add(peer.getId()); - } - - public void add(final BytesValue peerId) { - blacklistedNodeIds.add(peerId); - } - - @Override - public void onDisconnect( - final PeerConnection connection, - final DisconnectReason reason, - final boolean initiatedByPeer) { - if (shouldBlacklistForDisconnect(reason, initiatedByPeer)) { - add(connection.getPeerInfo().getNodeId()); - } - } - - private boolean shouldBlacklistForDisconnect( - final DisconnectMessage.DisconnectReason reason, final boolean initiatedByPeer) { - return (!initiatedByPeer && locallyTriggeredBlacklistReasons.contains(reason)) - || (initiatedByPeer && remotelyTriggeredBlacklistReasons.contains(reason)); - } -} diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java index 55d26a23c8..36031fa025 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java @@ -27,7 +27,7 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.Packet; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PacketType; import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissionsBlacklist; import tech.pegasys.pantheon.ethereum.p2p.wire.PeerInfo; import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage.DisconnectReason; import tech.pegasys.pantheon.util.bytes.BytesValue; @@ -157,33 +157,6 @@ public void shouldEvictPeerOnDisconnect() { assertThat(peerDiscoveryAgent2.streamDiscoveredPeers().collect(toList()).size()).isEqualTo(0); } - @Test - public void doesNotBlacklistPeerForNormalDisconnect() { - // Start an agent with no bootstrap peers. - final PeerBlacklist blacklist = new PeerBlacklist(); - final MockPeerDiscoveryAgent agent = - helper.startDiscoveryAgent(Collections.emptyList(), blacklist); - // Setup peer - final MockPeerDiscoveryAgent otherNode = helper.startDiscoveryAgent(); - final PeerConnection wirePeer = createAnonymousPeerConnection(otherNode.getId()); - - // Bond to peer - bondViaIncomingPing(agent, otherNode); - assertThat(agent.streamDiscoveredPeers()).hasSize(1); - - // Disconnect with innocuous reason - blacklist.onDisconnect(wirePeer, DisconnectReason.TOO_MANY_PEERS, false); - agent.onDisconnect(wirePeer, DisconnectReason.TOO_MANY_PEERS, false); - // Confirm peer was removed - assertThat(agent.streamDiscoveredPeers()).hasSize(0); - - // Bond again - bondViaIncomingPing(agent, otherNode); - - // Check peer was allowed to connect - assertThat(agent.streamDiscoveredPeers()).hasSize(1); - } - protected void bondViaIncomingPing( final MockPeerDiscoveryAgent agent, final MockPeerDiscoveryAgent otherNode) { final Packet pingPacket = helper.createPingPacket(otherNode, agent); @@ -191,107 +164,17 @@ protected void bondViaIncomingPing( } @Test - public void blacklistPeerForBadBehavior() { - // Start an agent with no bootstrap peers. - final PeerBlacklist blacklist = new PeerBlacklist(); - final MockPeerDiscoveryAgent agent = - helper.startDiscoveryAgent(Collections.emptyList(), blacklist); - // Setup peer - final MockPeerDiscoveryAgent otherNode = helper.startDiscoveryAgent(); - final PeerConnection wirePeer = createAnonymousPeerConnection(otherNode.getId()); - - // Bond to peer - bondViaIncomingPing(agent, otherNode); - assertThat(agent.streamDiscoveredPeers()).hasSize(1); - - // Disconnect with problematic reason - blacklist.onDisconnect(wirePeer, DisconnectReason.BREACH_OF_PROTOCOL, false); - agent.onDisconnect(wirePeer, DisconnectReason.BREACH_OF_PROTOCOL, false); - // Confirm peer was removed - assertThat(agent.streamDiscoveredPeers()).hasSize(0); - - // Bond again - bondViaIncomingPing(agent, otherNode); - - // Check peer was not allowed to connect - assertThat(agent.streamDiscoveredPeers()).hasSize(0); - } - - @Test - public void doesNotBlacklistPeerForOurBadBehavior() throws Exception { + public void dontBondWithNonPermittedPeer() { // Start an agent with no bootstrap peers. - final PeerBlacklist blacklist = new PeerBlacklist(); + final PeerPermissionsBlacklist blacklist = PeerPermissionsBlacklist.create(); final MockPeerDiscoveryAgent agent = helper.startDiscoveryAgent(Collections.emptyList(), blacklist); // Setup peer final MockPeerDiscoveryAgent otherNode = helper.startDiscoveryAgent(); - final PeerConnection wirePeer = createAnonymousPeerConnection(otherNode.getId()); - - // Bond to peer - bondViaIncomingPing(agent, otherNode); - assertThat(agent.streamDiscoveredPeers()).hasSize(1); - // Disconnect with problematic reason - blacklist.onDisconnect(wirePeer, DisconnectReason.BREACH_OF_PROTOCOL, true); - agent.onDisconnect(wirePeer, DisconnectReason.BREACH_OF_PROTOCOL, true); - // Confirm peer was removed - assertThat(agent.streamDiscoveredPeers()).hasSize(0); - - // Bond again - bondViaIncomingPing(agent, otherNode); - - // Check peer was allowed to connect - assertThat(agent.streamDiscoveredPeers()).hasSize(1); - } - - @Test - public void blacklistIncompatiblePeer() throws Exception { - // Start an agent with no bootstrap peers. - final PeerBlacklist blacklist = new PeerBlacklist(); - final MockPeerDiscoveryAgent agent = - helper.startDiscoveryAgent(Collections.emptyList(), blacklist); - // Setup peer - final MockPeerDiscoveryAgent otherNode = helper.startDiscoveryAgent(); - final PeerConnection wirePeer = createAnonymousPeerConnection(otherNode.getId()); - - // Bond to peer - bondViaIncomingPing(agent, otherNode); - assertThat(agent.streamDiscoveredPeers()).hasSize(1); - - // Disconnect - blacklist.onDisconnect(wirePeer, DisconnectReason.INCOMPATIBLE_P2P_PROTOCOL_VERSION, false); - agent.onDisconnect(wirePeer, DisconnectReason.INCOMPATIBLE_P2P_PROTOCOL_VERSION, false); - // Confirm peer was removed - assertThat(agent.streamDiscoveredPeers()).hasSize(0); - - // Bond again - bondViaIncomingPing(agent, otherNode); - - // Check peer was not allowed to connect - assertThat(agent.streamDiscoveredPeers()).hasSize(0); - } - - @Test - public void blacklistIncompatiblePeerWhoIssuesDisconnect() throws Exception { - // Start an agent with no bootstrap peers. - final PeerBlacklist blacklist = new PeerBlacklist(); - final MockPeerDiscoveryAgent agent = - helper.startDiscoveryAgent(Collections.emptyList(), blacklist); - // Setup peer - final MockPeerDiscoveryAgent otherNode = helper.startDiscoveryAgent(); - final PeerConnection wirePeer = createAnonymousPeerConnection(otherNode.getId()); - - // Bond to peer - bondViaIncomingPing(agent, otherNode); - assertThat(agent.streamDiscoveredPeers()).hasSize(1); - - // Disconnect - blacklist.onDisconnect(wirePeer, DisconnectReason.INCOMPATIBLE_P2P_PROTOCOL_VERSION, true); - agent.onDisconnect(wirePeer, DisconnectReason.INCOMPATIBLE_P2P_PROTOCOL_VERSION, true); - // Confirm peer was removed - assertThat(agent.streamDiscoveredPeers()).hasSize(0); + blacklist.add(otherNode.getId()); - // Bond again + // Bond bondViaIncomingPing(agent, otherNode); // Check peer was not allowed to connect diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryTestHelper.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryTestHelper.java index ad9264061c..3ed39eb33a 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryTestHelper.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryTestHelper.java @@ -22,7 +22,7 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PacketType; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PingPacketData; import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningController; import tech.pegasys.pantheon.util.bytes.BytesValue; import tech.pegasys.pantheon.util.enode.EnodeURL; @@ -139,13 +139,13 @@ public MockPeerDiscoveryAgent startDiscoveryAgent(final DiscoveryPeer... bootstr * Start a single discovery agent with the provided bootstrap peers. * * @param bootstrapPeers the list of bootstrap peers - * @param blacklist the peer blacklist + * @param peerPermissions peer permissions * @return a list of discovery agents. */ public MockPeerDiscoveryAgent startDiscoveryAgent( - final List bootstrapPeers, final PeerBlacklist blacklist) { + final List bootstrapPeers, final PeerPermissions peerPermissions) { final AgentBuilder agentBuilder = - agentBuilder().bootstrapPeers(bootstrapPeers).blacklist(blacklist); + agentBuilder().bootstrapPeers(bootstrapPeers).peerPermissions(peerPermissions); return startDiscoveryAgent(agentBuilder); } @@ -178,10 +178,10 @@ public static class AgentBuilder { private final Map agents; private final AtomicInteger nextAvailablePort; - private PeerBlacklist blacklist = new PeerBlacklist(); private Optional nodePermissioningController = Optional.empty(); private List bootnodes = Collections.emptyList(); private boolean active = true; + private PeerPermissions peerPermissions = PeerPermissions.noop(); private AgentBuilder( final Map agents, @@ -213,8 +213,8 @@ public AgentBuilder nodePermissioningController(final NodePermissioningControlle return this; } - public AgentBuilder blacklist(final PeerBlacklist blacklist) { - this.blacklist = blacklist; + public AgentBuilder peerPermissions(final PeerPermissions peerPermissions) { + this.peerPermissions = peerPermissions; return this; } @@ -230,7 +230,11 @@ public MockPeerDiscoveryAgent build() { config.setActive(active); return new MockPeerDiscoveryAgent( - SECP256K1.KeyPair.generate(), config, blacklist, nodePermissioningController, agents); + SECP256K1.KeyPair.generate(), + config, + peerPermissions, + nodePermissioningController, + agents); } } } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryTimestampsTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryTimestampsTest.java index cc845ff247..d910949754 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryTimestampsTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryTimestampsTest.java @@ -26,7 +26,7 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerDiscoveryController; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerTable; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PingPacketData; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem; import tech.pegasys.pantheon.util.Subscribers; @@ -63,7 +63,7 @@ public void lastSeenAndFirstDiscoveredTimestampsUpdatedOnMessage() { new BlockingAsyncExecutor(), TimeUnit.HOURS.toMillis(1), () -> true, - new PeerBlacklist(), + PeerPermissions.noop(), Optional.empty(), new Subscribers<>(), new Subscribers<>(), diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/MockPeerDiscoveryAgent.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/MockPeerDiscoveryAgent.java index 508bd4998d..477595420f 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/MockPeerDiscoveryAgent.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/MockPeerDiscoveryAgent.java @@ -17,7 +17,7 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.DiscoveryPeer; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryAgent; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerDiscoveryController.AsyncExecutor; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningController; import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem; import tech.pegasys.pantheon.util.bytes.BytesValue; @@ -39,10 +39,10 @@ public class MockPeerDiscoveryAgent extends PeerDiscoveryAgent { public MockPeerDiscoveryAgent( final KeyPair keyPair, final DiscoveryConfiguration config, - final PeerBlacklist peerBlacklist, + final PeerPermissions peerPermissions, final Optional nodePermissioningController, final Map agentNetwork) { - super(keyPair, config, peerBlacklist, nodePermissioningController, new NoOpMetricsSystem()); + super(keyPair, config, peerPermissions, nodePermissioningController, new NoOpMetricsSystem()); this.agentNetwork = agentNetwork; } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java index 714fece115..74b97d97db 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java @@ -40,7 +40,8 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryTestHelper; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerTable.EvictResult; import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissionsBlacklist; import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningController; import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem; import tech.pegasys.pantheon.util.Subscribers; @@ -57,9 +58,7 @@ import java.util.Collections; import java.util.List; import java.util.Optional; -import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Consumer; import java.util.stream.Collectors; @@ -597,12 +596,12 @@ public void shouldNotAddNewPeerWhenReceivedPongFromBlacklistedPeer() { final DiscoveryPeer otherPeer = peers.get(1); final DiscoveryPeer otherPeer2 = peers.get(2); - final PeerBlacklist blacklist = new PeerBlacklist(); + final PeerPermissionsBlacklist blacklist = PeerPermissionsBlacklist.create(); final OutboundMessageHandler outboundMessageHandler = mock(OutboundMessageHandler.class); controller = getControllerBuilder() .peers(discoPeer) - .blacklist(blacklist) + .peerPermissions(blacklist) .outboundMessageHandler(outboundMessageHandler) .build(); @@ -675,12 +674,12 @@ public void shouldNotBondWithBlacklistedPeer() { final DiscoveryPeer otherPeer = peers.get(1); final DiscoveryPeer otherPeer2 = peers.get(2); - final PeerBlacklist blacklist = new PeerBlacklist(); + final PeerPermissionsBlacklist blacklist = PeerPermissionsBlacklist.create(); final OutboundMessageHandler outboundMessageHandler = mock(OutboundMessageHandler.class); controller = getControllerBuilder() .peers(discoPeer) - .blacklist(blacklist) + .peerPermissions(blacklist) .outboundMessageHandler(outboundMessageHandler) .build(); @@ -726,18 +725,15 @@ public void shouldNotBondWithBlacklistedPeer() { } @Test - public void shouldRespondToNeighborsRequestFromKnownPeer() - throws InterruptedException, ExecutionException, TimeoutException { + public void shouldRespondToNeighborsRequestFromKnownPeer() { final List peers = createPeersInLastBucket(localPeer, 1); final DiscoveryPeer discoPeer = peers.get(0); - final PeerBlacklist blacklist = new PeerBlacklist(); final OutboundMessageHandler outboundMessageHandler = mock(OutboundMessageHandler.class); controller = getControllerBuilder() .peers(discoPeer) - .blacklist(blacklist) .outboundMessageHandler(outboundMessageHandler) .build(); @@ -768,19 +764,16 @@ public void shouldRespondToNeighborsRequestFromKnownPeer() } @Test - public void shouldNotRespondToNeighborsRequestFromUnknownPeer() - throws InterruptedException, ExecutionException, TimeoutException { + public void shouldNotRespondToNeighborsRequestFromUnknownPeer() { final List peers = createPeersInLastBucket(localPeer, 2); final DiscoveryPeer discoPeer = peers.get(0); final DiscoveryPeer otherPeer = peers.get(1); - final PeerBlacklist blacklist = new PeerBlacklist(); final OutboundMessageHandler outboundMessageHandler = mock(OutboundMessageHandler.class); controller = getControllerBuilder() .peers(discoPeer) - .blacklist(blacklist) .outboundMessageHandler(outboundMessageHandler) .build(); @@ -816,12 +809,12 @@ public void shouldNotRespondToNeighborsRequestFromBlacklistedPeer() { final DiscoveryPeer discoPeer = peers.get(0); - final PeerBlacklist blacklist = new PeerBlacklist(); + final PeerPermissionsBlacklist blacklist = PeerPermissionsBlacklist.create(); final OutboundMessageHandler outboundMessageHandler = mock(OutboundMessageHandler.class); controller = getControllerBuilder() .peers(discoPeer) - .blacklist(blacklist) + .peerPermissions(blacklist) .outboundMessageHandler(outboundMessageHandler) .build(); @@ -990,8 +983,6 @@ public void shouldNotBondWithNonPermittedPeer() { final DiscoveryPeer notPermittedPeer = peers.get(1); final DiscoveryPeer permittedPeer = peers.get(2); - final PeerBlacklist blacklist = new PeerBlacklist(); - final NodePermissioningController nodePermissioningController = new NodePermissioningControllerTestHelper(localPeer) .withPermittedPeers(discoveryPeer, permittedPeer) @@ -1002,7 +993,6 @@ public void shouldNotBondWithNonPermittedPeer() { controller = getControllerBuilder() .peers(discoveryPeer) - .blacklist(blacklist) .nodePermissioningController(nodePermissioningController) .outboundMessageHandler(outboundMessageHandler) .build(); @@ -1147,7 +1137,6 @@ private PeerDiscoveryController startPeerDiscoveryController( static class ControllerBuilder { private Collection discoPeers = Collections.emptyList(); - private PeerBlacklist blacklist = new PeerBlacklist(); private Optional nodePermissioningController = Optional.empty(); private MockTimerUtil timerUtil = new MockTimerUtil(); private KeyPair keypair; @@ -1157,6 +1146,7 @@ static class ControllerBuilder { private static final PeerDiscoveryTestHelper helper = new PeerDiscoveryTestHelper(); private Subscribers> peerBondedObservers = new Subscribers<>(); private Subscribers> peerDroppedObservers = new Subscribers<>(); + private PeerPermissions peerPermissions = PeerPermissions.noop(); public static ControllerBuilder create() { return new ControllerBuilder(); @@ -1172,8 +1162,8 @@ ControllerBuilder peers(final DiscoveryPeer... discoPeers) { return this; } - ControllerBuilder blacklist(final PeerBlacklist blacklist) { - this.blacklist = blacklist; + ControllerBuilder peerPermissions(final PeerPermissions peerPermissions) { + this.peerPermissions = peerPermissions; return this; } @@ -1237,7 +1227,7 @@ PeerDiscoveryController build() { new BlockingAsyncExecutor(), TABLE_REFRESH_INTERVAL_MS, PEER_REQUIREMENT, - blacklist, + peerPermissions, nodePermissioningController, peerBondedObservers, peerDroppedObservers, diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryTableRefreshTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryTableRefreshTest.java index 7547c3d469..62802192b5 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryTableRefreshTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryTableRefreshTest.java @@ -25,7 +25,7 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.DiscoveryPeer; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryStatus; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryTestHelper; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem; import tech.pegasys.pantheon.util.Subscribers; import tech.pegasys.pantheon.util.bytes.BytesValue; @@ -64,7 +64,7 @@ public void tableRefreshSingleNode() { new BlockingAsyncExecutor(), 0, () -> true, - new PeerBlacklist(), + PeerPermissions.noop(), Optional.empty(), new Subscribers<>(), new Subscribers<>(), diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshStateTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshStateTest.java index 4d1cc9f229..c79e7314cf 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshStateTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshStateTest.java @@ -28,7 +28,7 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryStatus; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.RecursivePeerRefreshState.BondingAgent; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.RecursivePeerRefreshState.FindNeighbourDispatcher; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; import tech.pegasys.pantheon.ethereum.permissioning.LocalPermissioningConfiguration; import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningController; import tech.pegasys.pantheon.util.bytes.BytesValue; @@ -40,11 +40,12 @@ import java.util.List; import java.util.Optional; +import org.junit.Before; import org.junit.Test; public class RecursivePeerRefreshStateTest { private static final BytesValue TARGET = createId(0); - private final PeerBlacklist peerBlacklist = mock(PeerBlacklist.class); + private final PeerPermissions peerPermissions = mock(PeerPermissions.class); private final BondingAgent bondingAgent = mock(BondingAgent.class); private final FindNeighbourDispatcher neighborFinder = mock(FindNeighbourDispatcher.class); private final MockTimerUtil timerUtil = new MockTimerUtil(); @@ -57,7 +58,7 @@ public class RecursivePeerRefreshStateTest { private RecursivePeerRefreshState recursivePeerRefreshState = new RecursivePeerRefreshState( - peerBlacklist, + peerPermissions, Optional.empty(), bondingAgent, neighborFinder, @@ -67,6 +68,12 @@ public class RecursivePeerRefreshStateTest { 5, 100); + @Before + public void setup() { + // Default peerPermissions to be permissive + when(peerPermissions.isPermitted(any())).thenReturn(true); + } + @Test public void shouldBondWithInitialNodesWhenStarted() { recursivePeerRefreshState.start(asList(peer1, peer2, peer3), TARGET); @@ -171,7 +178,7 @@ public void shouldStopWhenAllNodesHaveBeenQueried() { public void shouldStopWhenMaximumNumberOfRoundsReached() { recursivePeerRefreshState = new RecursivePeerRefreshState( - peerBlacklist, + peerPermissions, Optional.empty(), bondingAgent, neighborFinder, @@ -431,16 +438,15 @@ public void shouldBondWithPeersInNeighboursResponseReceivedAfterTimeout() { } @Test - public void shouldNotBondWithNodesOnBlacklist() { + public void shouldNotBondWithNonPermittedNode() { final DiscoveryPeer peerA = createPeer(1, "127.0.0.1", 1, 1); final DiscoveryPeer peerB = createPeer(2, "127.0.0.2", 2, 2); - final PeerBlacklist blacklist = new PeerBlacklist(); - blacklist.add(peerB); + when(peerPermissions.isPermitted(peerB)).thenReturn(false); recursivePeerRefreshState = new RecursivePeerRefreshState( - blacklist, + peerPermissions, Optional.empty(), bondingAgent, neighborFinder, @@ -501,7 +507,7 @@ public void shouldNotBondWithNodesNotPermitted() throws Exception { recursivePeerRefreshState = new RecursivePeerRefreshState( - peerBlacklist, + peerPermissions, Optional.of(nodeWhitelistController), bondingAgent, neighborFinder, diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetworkTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetworkTest.java index edf0c360cd..74d6e467b8 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetworkTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetworkTest.java @@ -44,7 +44,6 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryStatus; import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeer; import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; import tech.pegasys.pantheon.ethereum.p2p.wire.Capability; import tech.pegasys.pantheon.ethereum.p2p.wire.PeerInfo; import tech.pegasys.pantheon.ethereum.p2p.wire.SubProtocol; @@ -564,6 +563,7 @@ private PeerConnection mockPeerConnection() { private PeerConnection mockPeerConnection(final Peer remotePeer) { final EnodeURL remoteEnode = remotePeer.getEnodeURL(); + final Peer peer = DefaultPeer.fromEnodeURL(remoteEnode); final PeerInfo peerInfo = new PeerInfo( 5, @@ -573,8 +573,9 @@ private PeerConnection mockPeerConnection(final Peer remotePeer) { remoteEnode.getNodeId()); final PeerConnection peerConnection = mock(PeerConnection.class); - when(peerConnection.getRemoteEnode()).thenReturn(remoteEnode); - when(peerConnection.getPeerInfo()).thenReturn(peerInfo); + lenient().when(peerConnection.getRemoteEnode()).thenReturn(remoteEnode); + lenient().when(peerConnection.getPeerInfo()).thenReturn(peerInfo); + lenient().when(peerConnection.getPeer()).thenReturn(peer); return peerConnection; } @@ -616,7 +617,6 @@ private DefaultP2PNetwork.Builder builder() { .vertx(vertx) .config(config) .keyPair(KeyPair.generate()) - .peerBlacklist(new PeerBlacklist()) .metricsSystem(new NoOpMetricsSystem()) .supportedCapabilities(Arrays.asList(Capability.create("eth", 63))); } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/NetworkingServiceLifecycleTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/NetworkingServiceLifecycleTest.java index 0ac734a46f..bfe3b1615f 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/NetworkingServiceLifecycleTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/NetworkingServiceLifecycleTest.java @@ -23,7 +23,6 @@ import tech.pegasys.pantheon.ethereum.p2p.config.DiscoveryConfiguration; import tech.pegasys.pantheon.ethereum.p2p.config.NetworkingConfiguration; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryServiceException; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; import tech.pegasys.pantheon.ethereum.p2p.wire.Capability; import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem; import tech.pegasys.pantheon.util.enode.EnodeURL; @@ -158,7 +157,6 @@ private DefaultP2PNetwork.Builder builder() { .vertx(vertx) .keyPair(keyPair) .config(config) - .peerBlacklist(new PeerBlacklist()) .metricsSystem(new NoOpMetricsSystem()) .supportedCapabilities(Arrays.asList(Capability.create("eth", 63))); } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/P2PNetworkTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/P2PNetworkTest.java index b768dc2192..221bde8566 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/P2PNetworkTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/P2PNetworkTest.java @@ -28,7 +28,7 @@ import tech.pegasys.pantheon.ethereum.p2p.network.exceptions.IncompatiblePeerException; import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeer; import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissionsBlacklist; import tech.pegasys.pantheon.ethereum.p2p.wire.Capability; import tech.pegasys.pantheon.ethereum.p2p.wire.SubProtocol; import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage.DisconnectReason; @@ -218,11 +218,10 @@ public void rejectPeerWithNoSharedCaps() throws Exception { @Test public void rejectIncomingConnectionFromBlacklistedPeer() throws Exception { - final PeerBlacklist localBlacklist = new PeerBlacklist(); - final PeerBlacklist remoteBlacklist = new PeerBlacklist(); + final PeerPermissionsBlacklist localBlacklist = PeerPermissionsBlacklist.create(); - try (final P2PNetwork localNetwork = builder().peerBlacklist(localBlacklist).build(); - final P2PNetwork remoteNetwork = builder().peerBlacklist(remoteBlacklist).build()) { + try (final P2PNetwork localNetwork = builder().peerPermissions(localBlacklist).build(); + final P2PNetwork remoteNetwork = builder().build()) { localNetwork.start(); remoteNetwork.start(); @@ -365,7 +364,6 @@ private DefaultP2PNetwork.Builder builder() { .vertx(vertx) .config(config) .keyPair(KeyPair.generate()) - .peerBlacklist(new PeerBlacklist()) .metricsSystem(new NoOpMetricsSystem()) .supportedCapabilities(Arrays.asList(Capability.create("eth", 63))); } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/PeerReputationManagerTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/PeerReputationManagerTest.java new file mode 100644 index 0000000000..fab54dcfb7 --- /dev/null +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/PeerReputationManagerTest.java @@ -0,0 +1,107 @@ +/* + * Copyright 2019 ConsenSys AG. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ +package tech.pegasys.pantheon.ethereum.p2p.network; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import tech.pegasys.pantheon.ethereum.p2p.api.PeerConnection; +import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeer; +import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissionsBlacklist; +import tech.pegasys.pantheon.ethereum.p2p.wire.PeerInfo; +import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage.DisconnectReason; +import tech.pegasys.pantheon.util.bytes.BytesValue; +import tech.pegasys.pantheon.util.enode.EnodeURL; + +import org.junit.Test; + +public class PeerReputationManagerTest { + private final PeerReputationManager peerReputationManager; + private final PeerPermissionsBlacklist blacklist; + + public PeerReputationManagerTest() { + blacklist = PeerPermissionsBlacklist.create(); + peerReputationManager = new PeerReputationManager(blacklist); + } + + @Test + public void doesNotBlacklistPeerForNormalDisconnect() { + final PeerConnection peer = generatePeerConnection(); + + assertThat(blacklist.isPermitted(peer.getPeer())).isTrue(); + + peerReputationManager.onDisconnect(peer, DisconnectReason.TOO_MANY_PEERS, false); + + assertThat(blacklist.isPermitted(peer.getPeer())).isTrue(); + } + + @Test + public void blacklistPeerForBadBehavior() { + final PeerConnection peer = generatePeerConnection(); + + assertThat(blacklist.isPermitted(peer.getPeer())).isTrue(); + peerReputationManager.onDisconnect(peer, DisconnectReason.BREACH_OF_PROTOCOL, false); + assertThat(blacklist.isPermitted(peer.getPeer())).isFalse(); + } + + @Test + public void doesNotBlacklistPeerForOurBadBehavior() { + final PeerConnection peer = generatePeerConnection(); + + assertThat(blacklist.isPermitted(peer.getPeer())).isTrue(); + peerReputationManager.onDisconnect(peer, DisconnectReason.BREACH_OF_PROTOCOL, true); + assertThat(blacklist.isPermitted(peer.getPeer())).isTrue(); + } + + @Test + public void blacklistIncompatiblePeer() { + final PeerConnection peer = generatePeerConnection(); + + assertThat(blacklist.isPermitted(peer.getPeer())).isTrue(); + peerReputationManager.onDisconnect( + peer, DisconnectReason.INCOMPATIBLE_P2P_PROTOCOL_VERSION, false); + assertThat(blacklist.isPermitted(peer.getPeer())).isFalse(); + } + + @Test + public void blacklistIncompatiblePeerWhoIssuesDisconnect() { + final PeerConnection peer = generatePeerConnection(); + + assertThat(blacklist.isPermitted(peer.getPeer())).isTrue(); + peerReputationManager.onDisconnect( + peer, DisconnectReason.INCOMPATIBLE_P2P_PROTOCOL_VERSION, true); + assertThat(blacklist.isPermitted(peer.getPeer())).isFalse(); + } + + private PeerConnection generatePeerConnection() { + final BytesValue nodeId = Peer.randomId(); + final PeerConnection conn = mock(PeerConnection.class); + final PeerInfo peerInfo = mock(PeerInfo.class); + final Peer peer = + DefaultPeer.fromEnodeURL( + EnodeURL.builder() + .nodeId(Peer.randomId()) + .ipAddress("10.9.8.7") + .discoveryPort(65535) + .listeningPort(65534) + .build()); + + when(peerInfo.getNodeId()).thenReturn(nodeId); + when(conn.getPeerInfo()).thenReturn(peerInfo); + when(conn.getPeer()).thenReturn(peer); + + return conn; + } +} diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklistTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklistTest.java deleted file mode 100644 index 587d4a14ab..0000000000 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/peers/PeerBlacklistTest.java +++ /dev/null @@ -1,219 +0,0 @@ -/* - * Copyright 2018 ConsenSys AG. - * - * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on - * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the - * specific language governing permissions and limitations under the License. - */ -package tech.pegasys.pantheon.ethereum.p2p.peers; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -import tech.pegasys.pantheon.ethereum.p2p.api.PeerConnection; -import tech.pegasys.pantheon.ethereum.p2p.wire.PeerInfo; -import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage.DisconnectReason; -import tech.pegasys.pantheon.util.bytes.BytesValue; -import tech.pegasys.pantheon.util.enode.EnodeURL; - -import java.util.Collections; - -import org.junit.Test; - -public class PeerBlacklistTest { - private int nodeIdValue = 1; - - @Test - public void directlyAddingPeerWorks() { - final PeerBlacklist blacklist = new PeerBlacklist(); - final Peer peer = generatePeer(); - - assertThat(blacklist.contains(peer)).isFalse(); - - blacklist.add(peer); - - assertThat(blacklist.contains(peer)).isTrue(); - } - - @Test - public void directlyAddingPeerByPeerIdWorks() { - final PeerBlacklist blacklist = new PeerBlacklist(); - final Peer peer = generatePeer(); - - assertThat(blacklist.contains(peer)).isFalse(); - - blacklist.add(peer.getId()); - - assertThat(blacklist.contains(peer)).isTrue(); - } - - @Test - public void banningPeerByPeerIdWorks() { - final Peer peer = generatePeer(); - final PeerBlacklist blacklist = new PeerBlacklist(Collections.singleton(peer.getId())); - - assertThat(blacklist.contains(peer)).isTrue(); - - blacklist.add(peer.getId()); - - assertThat(blacklist.contains(peer)).isTrue(); - } - - @Test - public void bannedNodesDoNotRollover() { - final Peer bannedPeer = generatePeer(); - final Peer peer1 = generatePeer(); - final Peer peer2 = generatePeer(); - final Peer peer3 = generatePeer(); - final PeerBlacklist blacklist = new PeerBlacklist(2, Collections.singleton(bannedPeer.getId())); - - assertThat(blacklist.contains(bannedPeer)).isTrue(); - assertThat(blacklist.contains(peer1)).isFalse(); - assertThat(blacklist.contains(peer2)).isFalse(); - assertThat(blacklist.contains(peer3)).isFalse(); - - // fill to the limit - blacklist.add(peer1.getId()); - blacklist.add(peer2.getId()); - assertThat(blacklist.contains(bannedPeer)).isTrue(); - assertThat(blacklist.contains(peer1)).isTrue(); - assertThat(blacklist.contains(peer2)).isTrue(); - assertThat(blacklist.contains(peer3)).isFalse(); - - // trigger rollover - blacklist.add(peer3.getId()); - assertThat(blacklist.contains(bannedPeer)).isTrue(); - assertThat(blacklist.contains(peer1)).isFalse(); - assertThat(blacklist.contains(peer2)).isTrue(); - assertThat(blacklist.contains(peer3)).isTrue(); - } - - @Test - public void doesNotBlacklistPeerForNormalDisconnect() { - final PeerBlacklist blacklist = new PeerBlacklist(); - final PeerConnection peer = generatePeerConnection(); - - assertThat(blacklist.contains(peer)).isFalse(); - - blacklist.onDisconnect(peer, DisconnectReason.TOO_MANY_PEERS, false); - - assertThat(blacklist.contains(peer)).isFalse(); - } - - @Test - public void blacklistPeerForBadBehavior() { - - final PeerBlacklist blacklist = new PeerBlacklist(); - final PeerConnection peer = generatePeerConnection(); - - assertThat(blacklist.contains(peer)).isFalse(); - - blacklist.onDisconnect(peer, DisconnectReason.BREACH_OF_PROTOCOL, false); - - assertThat(blacklist.contains(peer)).isTrue(); - } - - @Test - public void doesNotBlacklistPeerForOurBadBehavior() { - final PeerBlacklist blacklist = new PeerBlacklist(); - final PeerConnection peer = generatePeerConnection(); - - assertThat(blacklist.contains(peer)).isFalse(); - - blacklist.onDisconnect(peer, DisconnectReason.BREACH_OF_PROTOCOL, true); - - assertThat(blacklist.contains(peer)).isFalse(); - } - - @Test - public void blacklistIncompatiblePeer() { - final PeerBlacklist blacklist = new PeerBlacklist(); - final PeerConnection peer = generatePeerConnection(); - - assertThat(blacklist.contains(peer)).isFalse(); - - blacklist.onDisconnect(peer, DisconnectReason.INCOMPATIBLE_P2P_PROTOCOL_VERSION, false); - - assertThat(blacklist.contains(peer)).isTrue(); - } - - @Test - public void blacklistIncompatiblePeerWhoIssuesDisconnect() { - final PeerBlacklist blacklist = new PeerBlacklist(); - final PeerConnection peer = generatePeerConnection(); - - assertThat(blacklist.contains(peer)).isFalse(); - - blacklist.onDisconnect(peer, DisconnectReason.INCOMPATIBLE_P2P_PROTOCOL_VERSION, true); - - assertThat(blacklist.contains(peer)).isTrue(); - } - - @Test - public void capsSizeOfList() { - - final PeerBlacklist blacklist = new PeerBlacklist(2); - final PeerConnection peer1 = generatePeerConnection(); - final PeerConnection peer2 = generatePeerConnection(); - final PeerConnection peer3 = generatePeerConnection(); - - // Add first peer - blacklist.onDisconnect(peer1, DisconnectReason.BREACH_OF_PROTOCOL, false); - assertThat(blacklist.contains(peer1)).isTrue(); - assertThat(blacklist.contains(peer2)).isFalse(); - assertThat(blacklist.contains(peer3)).isFalse(); - - // Add second peer - blacklist.onDisconnect(peer2, DisconnectReason.BREACH_OF_PROTOCOL, false); - assertThat(blacklist.contains(peer1)).isTrue(); - assertThat(blacklist.contains(peer2)).isTrue(); - assertThat(blacklist.contains(peer3)).isFalse(); - - // Adding third peer should kick out least recently accessed peer - blacklist.onDisconnect(peer3, DisconnectReason.BREACH_OF_PROTOCOL, false); - assertThat(blacklist.contains(peer1)).isFalse(); - assertThat(blacklist.contains(peer2)).isTrue(); - assertThat(blacklist.contains(peer3)).isTrue(); - - // Adding peer1 back in should kick out peer2 - blacklist.onDisconnect(peer1, DisconnectReason.BREACH_OF_PROTOCOL, false); - assertThat(blacklist.contains(peer1)).isTrue(); - assertThat(blacklist.contains(peer2)).isFalse(); - assertThat(blacklist.contains(peer3)).isTrue(); - - // Adding peer2 back in should kick out peer3 - blacklist.onDisconnect(peer2, DisconnectReason.BREACH_OF_PROTOCOL, false); - assertThat(blacklist.contains(peer1)).isTrue(); - assertThat(blacklist.contains(peer2)).isTrue(); - assertThat(blacklist.contains(peer3)).isFalse(); - } - - private PeerConnection generatePeerConnection() { - final BytesValue nodeId = BytesValue.of(nodeIdValue++); - final PeerConnection peer = mock(PeerConnection.class); - final PeerInfo peerInfo = mock(PeerInfo.class); - - when(peerInfo.getNodeId()).thenReturn(nodeId); - when(peer.getPeerInfo()).thenReturn(peerInfo); - - return peer; - } - - private Peer generatePeer() { - final byte[] id = new byte[64]; - id[0] = (byte) nodeIdValue++; - return DefaultPeer.fromEnodeURL( - EnodeURL.builder() - .nodeId(id) - .ipAddress("10.9.8.7") - .discoveryPort(65535) - .listeningPort(65534) - .build()); - } -} diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java b/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java index c4c12899e1..af19a38f95 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java @@ -58,7 +58,7 @@ import tech.pegasys.pantheon.ethereum.p2p.config.SubProtocolConfiguration; import tech.pegasys.pantheon.ethereum.p2p.network.DefaultP2PNetwork; import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeer; -import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; +import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissionsBlacklist; import tech.pegasys.pantheon.ethereum.p2p.wire.Capability; import tech.pegasys.pantheon.ethereum.p2p.wire.SubProtocol; import tech.pegasys.pantheon.ethereum.permissioning.AccountLocalConfigPermissioningController; @@ -105,7 +105,7 @@ public class RunnerBuilder { private GraphQLRpcConfiguration graphQLRpcConfiguration; private WebSocketConfiguration webSocketConfiguration; private Path dataDir; - private Collection bannedNodeIds; + private Collection bannedNodeIds = new ArrayList<>(); private MetricsConfiguration metricsConfiguration; private MetricsSystem metricsSystem; private Optional permissioningConfiguration = Optional.empty(); @@ -179,7 +179,7 @@ public RunnerBuilder dataDir(final Path dataDir) { } public RunnerBuilder bannedNodeIds(final Collection bannedNodeIds) { - this.bannedNodeIds = bannedNodeIds; + this.bannedNodeIds.addAll(bannedNodeIds); return this; } @@ -241,9 +241,11 @@ public Runner build() { .setClientId(PantheonInfo.version()) .setSupportedProtocols(subProtocols); - final PeerBlacklist peerBlacklist = - new PeerBlacklist( - bannedNodeIds.stream().map(BytesValue::fromHexString).collect(Collectors.toSet())); + final PeerPermissionsBlacklist bannedNodes = PeerPermissionsBlacklist.create(); + // TODO - validate and parse banned node ids as BytesValue's in {@link PantheonCommand} + bannedNodeIds.stream() + .map(n -> BytesValue.fromHexString(n, EnodeURL.NODE_ID_SIZE)) + .forEach(bannedNodes::add); final List bootnodes = discoveryConfiguration.getBootnodes(); @@ -268,7 +270,7 @@ public Runner build() { .vertx(vertx) .keyPair(keyPair) .config(networkConfig) - .peerBlacklist(peerBlacklist) + .peerPermissions(bannedNodes) .metricsSystem(metricsSystem) .supportedCapabilities(caps) .nodePermissioningController(nodePermissioningController) diff --git a/pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java b/pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java index b4cb3d0b6b..e3c1419cee 100644 --- a/pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java +++ b/pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java @@ -169,7 +169,6 @@ private void syncFromGenesis(final SyncMode mode) throws Exception { .p2pListenPort(0) .maxPeers(3) .metricsSystem(noOpMetricsSystem) - .bannedNodeIds(emptySet()) .staticNodes(emptySet()); Runner runnerBehind = null; From 69c244dabdc3ed11fcaec56ec72bd743f23de953 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Mon, 13 May 2019 14:37:33 -0400 Subject: [PATCH 04/19] Dedupe identical functions --- .../ethereum/p2p/network/DefaultP2PNetwork.java | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java index bab0e445bf..9c7268ef2a 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java @@ -18,7 +18,6 @@ import tech.pegasys.pantheon.crypto.SECP256K1; import tech.pegasys.pantheon.crypto.SECP256K1.KeyPair; -import tech.pegasys.pantheon.ethereum.chain.BlockAddedEvent; import tech.pegasys.pantheon.ethereum.chain.Blockchain; import tech.pegasys.pantheon.ethereum.p2p.api.DisconnectCallback; import tech.pegasys.pantheon.ethereum.p2p.api.Message; @@ -557,7 +556,8 @@ public void start() { synchronized (this) { if (!blockAddedObserverId.isPresent()) { blockAddedObserverId = - OptionalLong.of(blockchain.get().observeBlockAdded(this::handleBlockAddedEvent)); + OptionalLong.of( + blockchain.get().observeBlockAdded((evt, chain) -> checkCurrentConnections())); } } } else { @@ -594,18 +594,6 @@ private Consumer handlePeerDroppedEvents() { }; } - private synchronized void handleBlockAddedEvent( - final BlockAddedEvent event, final Blockchain blockchain) { - connections - .getPeerConnections() - .forEach( - peerConnection -> { - if (!isPeerAllowed(peerConnection)) { - peerConnection.disconnect(DisconnectReason.REQUESTED); - } - }); - } - private synchronized void checkCurrentConnections() { connections .getPeerConnections() From 7dc7111cd3413f447352df4dd9eb372a003bfdc1 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Mon, 13 May 2019 14:52:43 -0400 Subject: [PATCH 05/19] Modify PeerPermissions callback to consume info on type of update --- .../p2p/permissions/PeerPermissions.java | 8 ++----- .../permissions/PeerPermissionsBlacklist.java | 18 +++++++------- .../PermissionsUpdateCallback.java | 24 +++++++++++++++++++ .../PeerPermissionsBlacklistTest.java | 17 +++++++++++-- .../p2p/permissions/PeerPermissionsTest.java | 23 +++++++++++++++--- 5 files changed, 69 insertions(+), 21 deletions(-) create mode 100644 ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PermissionsUpdateCallback.java diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java index 4a28e5b7eb..1f75ad6d6e 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java @@ -49,12 +49,8 @@ public void subscribeUpdate(final PermissionsUpdateCallback callback) { updateSubscribers.subscribe(callback); } - protected void dispatchUpdate() { - updateSubscribers.forEach(PermissionsUpdateCallback::onUpdate); - } - - public interface PermissionsUpdateCallback { - void onUpdate(); + protected void dispatchUpdate(final boolean permissionsRestricted) { + updateSubscribers.forEach(s -> s.onUpdate(permissionsRestricted)); } private static class CombinedPeerPermissions extends PeerPermissions { diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklist.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklist.java index 9ae8c85c8e..78708b60a5 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklist.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklist.java @@ -54,24 +54,22 @@ public boolean isPermitted(final Peer peer) { } public void add(final Peer peer) { - if (blacklist.add(peer.getId())) { - dispatchUpdate(); - } + add(peer.getId()); } public void remove(final Peer peer) { - if (blacklist.remove(peer.getId())) { - dispatchUpdate(); - } + remove(peer.getId()); } public void add(final BytesValue peerId) { - blacklist.add(peerId); - dispatchUpdate(); + if (blacklist.add(peerId)) { + dispatchUpdate(true); + } } public void remove(final BytesValue peerId) { - blacklist.remove(peerId); - dispatchUpdate(); + if (blacklist.remove(peerId)) { + dispatchUpdate(false); + } } } diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PermissionsUpdateCallback.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PermissionsUpdateCallback.java new file mode 100644 index 0000000000..c644316988 --- /dev/null +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PermissionsUpdateCallback.java @@ -0,0 +1,24 @@ +/* + * Copyright 2019 ConsenSys AG. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ +package tech.pegasys.pantheon.ethereum.p2p.permissions; + +public interface PermissionsUpdateCallback { + + /** + * @param permissionsRestricted True if permissions were narrowed in any way, meaning that + * previously permitted peers may no longer be permitted. False indicates that permissions + * were made less restrictive, meaning peers that were previously restricted may now be + * permitted. + */ + void onUpdate(final boolean permissionsRestricted); +} diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklistTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklistTest.java index 2a1fa5e80f..a739589628 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklistTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklistTest.java @@ -45,27 +45,40 @@ public void trackedPeerIsNotPermitted() { public void subscribeUpdate() { PeerPermissionsBlacklist blacklist = PeerPermissionsBlacklist.create(); final AtomicInteger callbackCount = new AtomicInteger(0); + final AtomicInteger restrictedCallbackCount = new AtomicInteger(0); Peer peer = createPeer(); - blacklist.subscribeUpdate(callbackCount::incrementAndGet); + blacklist.subscribeUpdate( + (permissionsRestricted) -> { + callbackCount.incrementAndGet(); + if (permissionsRestricted) { + restrictedCallbackCount.incrementAndGet(); + } + }); assertThat(blacklist.isPermitted(peer)).isTrue(); assertThat(callbackCount).hasValue(0); + assertThat(restrictedCallbackCount).hasValue(0); blacklist.add(peer); assertThat(callbackCount).hasValue(1); + assertThat(restrictedCallbackCount).hasValue(1); blacklist.add(peer); assertThat(callbackCount).hasValue(1); + assertThat(restrictedCallbackCount).hasValue(1); blacklist.remove(peer); assertThat(callbackCount).hasValue(2); + assertThat(restrictedCallbackCount).hasValue(1); blacklist.remove(peer); assertThat(callbackCount).hasValue(2); + assertThat(restrictedCallbackCount).hasValue(1); blacklist.add(peer); assertThat(callbackCount).hasValue(3); + assertThat(restrictedCallbackCount).hasValue(2); } @Test @@ -120,7 +133,7 @@ private Peer createPeer() { EnodeURL.builder() .nodeId(Peer.randomId()) .ipAddress("127.0.0.1") - .listeningPort(EnodeURL.DEFAULT_LISTENING_PORT) + .discoveryAndListeningPorts(EnodeURL.DEFAULT_LISTENING_PORT) .build()); } } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsTest.java index f6d25faffd..7a920bc793 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsTest.java @@ -28,7 +28,7 @@ public void subscribeUpdate() { TestPeerPermissions peerPermissions = new TestPeerPermissions(false); final AtomicInteger callbackCount = new AtomicInteger(0); - peerPermissions.subscribeUpdate(callbackCount::incrementAndGet); + peerPermissions.subscribeUpdate((permissionsRestricted) -> callbackCount.incrementAndGet()); peerPermissions.allowPeers(true); assertThat(callbackCount).hasValue(1); @@ -44,14 +44,31 @@ public void subscribeUpdate_forCombinedPermissions() { PeerPermissions combined = PeerPermissions.combine(peerPermissionsA, peerPermissionsB); final AtomicInteger callbackCount = new AtomicInteger(0); + final AtomicInteger restrictedCallbackCount = new AtomicInteger(0); - combined.subscribeUpdate(callbackCount::incrementAndGet); + combined.subscribeUpdate( + (permissionsRestricted) -> { + callbackCount.incrementAndGet(); + if (permissionsRestricted) { + restrictedCallbackCount.incrementAndGet(); + } + }); peerPermissionsA.allowPeers(true); assertThat(callbackCount).hasValue(1); + assertThat(restrictedCallbackCount).hasValue(0); peerPermissionsB.allowPeers(true); assertThat(callbackCount).hasValue(2); + assertThat(restrictedCallbackCount).hasValue(0); + + peerPermissionsA.allowPeers(false); + assertThat(callbackCount).hasValue(3); + assertThat(restrictedCallbackCount).hasValue(1); + + peerPermissionsB.allowPeers(false); + assertThat(callbackCount).hasValue(4); + assertThat(restrictedCallbackCount).hasValue(2); } @Test @@ -85,7 +102,7 @@ public TestPeerPermissions(final boolean allowPeers) { public void allowPeers(final boolean doAllowPeers) { this.allowPeers = doAllowPeers; - dispatchUpdate(); + dispatchUpdate(!doAllowPeers); } @Override From c252f00666558fcc59999efbcbd52bab965ee6d4 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Mon, 13 May 2019 15:09:30 -0400 Subject: [PATCH 06/19] PeerPermissions update supports an optional list of affected peers --- .../p2p/permissions/PeerPermissions.java | 6 +- .../permissions/PeerPermissionsBlacklist.java | 14 +++- .../PermissionsUpdateCallback.java | 11 ++- .../PeerPermissionsBlacklistTest.java | 81 ++++++++++++++++++- .../p2p/permissions/PeerPermissionsTest.java | 8 +- 5 files changed, 109 insertions(+), 11 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java index 1f75ad6d6e..535bbbb0c6 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java @@ -17,6 +17,7 @@ import java.util.Arrays; import java.util.List; +import java.util.Optional; public abstract class PeerPermissions { private final Subscribers updateSubscribers = new Subscribers<>(); @@ -49,8 +50,9 @@ public void subscribeUpdate(final PermissionsUpdateCallback callback) { updateSubscribers.subscribe(callback); } - protected void dispatchUpdate(final boolean permissionsRestricted) { - updateSubscribers.forEach(s -> s.onUpdate(permissionsRestricted)); + protected void dispatchUpdate( + final boolean permissionsRestricted, final Optional> affectedPeers) { + updateSubscribers.forEach(s -> s.onUpdate(permissionsRestricted, affectedPeers)); } private static class CombinedPeerPermissions extends PeerPermissions { diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklist.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklist.java index 78708b60a5..94e55f58a1 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklist.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklist.java @@ -17,6 +17,8 @@ import tech.pegasys.pantheon.util.LimitedSet.Mode; import tech.pegasys.pantheon.util.bytes.BytesValue; +import java.util.Collections; +import java.util.Optional; import java.util.OptionalInt; import java.util.Set; @@ -54,22 +56,26 @@ public boolean isPermitted(final Peer peer) { } public void add(final Peer peer) { - add(peer.getId()); + if (blacklist.add(peer.getId())) { + dispatchUpdate(true, Optional.of(Collections.singletonList(peer))); + } } public void remove(final Peer peer) { - remove(peer.getId()); + if (blacklist.remove(peer.getId())) { + dispatchUpdate(false, Optional.of(Collections.singletonList(peer))); + } } public void add(final BytesValue peerId) { if (blacklist.add(peerId)) { - dispatchUpdate(true); + dispatchUpdate(true, Optional.empty()); } } public void remove(final BytesValue peerId) { if (blacklist.remove(peerId)) { - dispatchUpdate(false); + dispatchUpdate(false, Optional.empty()); } } } diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PermissionsUpdateCallback.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PermissionsUpdateCallback.java index c644316988..9cd1cb6bed 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PermissionsUpdateCallback.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PermissionsUpdateCallback.java @@ -12,6 +12,11 @@ */ package tech.pegasys.pantheon.ethereum.p2p.permissions; +import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; + +import java.util.List; +import java.util.Optional; + public interface PermissionsUpdateCallback { /** @@ -19,6 +24,10 @@ public interface PermissionsUpdateCallback { * previously permitted peers may no longer be permitted. False indicates that permissions * were made less restrictive, meaning peers that were previously restricted may now be * permitted. + * @param affectedPeers If non-empty, contains the entire set of peers affected by this + * permissions update. If permissions were restricted, this is the list of peers that are no + * longer permitted. If permissions were broadened, this is the list of peers that are now + * permitted. */ - void onUpdate(final boolean permissionsRestricted); + void onUpdate(final boolean permissionsRestricted, final Optional> affectedPeers); } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklistTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklistTest.java index a739589628..dbcba83a87 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklistTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsBlacklistTest.java @@ -18,6 +18,7 @@ import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; import tech.pegasys.pantheon.util.enode.EnodeURL; +import java.util.Collections; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; @@ -27,6 +28,84 @@ public class PeerPermissionsBlacklistTest { + @Test + public void add_peer() { + PeerPermissionsBlacklist blacklist = PeerPermissionsBlacklist.create(); + Peer peer = createPeer(); + + final AtomicInteger callbackCount = new AtomicInteger(0); + blacklist.subscribeUpdate( + (restricted, affectedPeers) -> { + callbackCount.incrementAndGet(); + assertThat(restricted).isTrue(); + assertThat(affectedPeers).contains(Collections.singletonList(peer)); + }); + + assertThat(callbackCount).hasValue(0); + + blacklist.add(peer); + assertThat(callbackCount).hasValue(1); + } + + @Test + public void remove_peer() { + PeerPermissionsBlacklist blacklist = PeerPermissionsBlacklist.create(); + Peer peer = createPeer(); + blacklist.add(peer); + + final AtomicInteger callbackCount = new AtomicInteger(0); + blacklist.subscribeUpdate( + (restricted, affectedPeers) -> { + callbackCount.incrementAndGet(); + assertThat(restricted).isFalse(); + assertThat(affectedPeers).contains(Collections.singletonList(peer)); + }); + + assertThat(callbackCount).hasValue(0); + + blacklist.remove(peer); + assertThat(callbackCount).hasValue(1); + } + + @Test + public void add_id() { + PeerPermissionsBlacklist blacklist = PeerPermissionsBlacklist.create(); + Peer peer = createPeer(); + + final AtomicInteger callbackCount = new AtomicInteger(0); + blacklist.subscribeUpdate( + (restricted, affectedPeers) -> { + callbackCount.incrementAndGet(); + assertThat(restricted).isTrue(); + assertThat(affectedPeers).isEmpty(); + }); + + assertThat(callbackCount).hasValue(0); + + blacklist.add(peer.getId()); + assertThat(callbackCount).hasValue(1); + } + + @Test + public void remove_id() { + PeerPermissionsBlacklist blacklist = PeerPermissionsBlacklist.create(); + Peer peer = createPeer(); + blacklist.add(peer); + + final AtomicInteger callbackCount = new AtomicInteger(0); + blacklist.subscribeUpdate( + (restricted, affectedPeers) -> { + callbackCount.incrementAndGet(); + assertThat(restricted).isFalse(); + assertThat(affectedPeers).isEmpty(); + }); + + assertThat(callbackCount).hasValue(0); + + blacklist.remove(peer.getId()); + assertThat(callbackCount).hasValue(1); + } + @Test public void trackedPeerIsNotPermitted() { PeerPermissionsBlacklist blacklist = PeerPermissionsBlacklist.create(); @@ -49,7 +128,7 @@ public void subscribeUpdate() { Peer peer = createPeer(); blacklist.subscribeUpdate( - (permissionsRestricted) -> { + (permissionsRestricted, affectedPeers) -> { callbackCount.incrementAndGet(); if (permissionsRestricted) { restrictedCallbackCount.incrementAndGet(); diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsTest.java index 7a920bc793..ebf2275463 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsTest.java @@ -18,6 +18,7 @@ import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; import tech.pegasys.pantheon.util.enode.EnodeURL; +import java.util.Optional; import java.util.concurrent.atomic.AtomicInteger; import org.junit.Test; @@ -28,7 +29,8 @@ public void subscribeUpdate() { TestPeerPermissions peerPermissions = new TestPeerPermissions(false); final AtomicInteger callbackCount = new AtomicInteger(0); - peerPermissions.subscribeUpdate((permissionsRestricted) -> callbackCount.incrementAndGet()); + peerPermissions.subscribeUpdate( + (permissionsRestricted, affectedPeers) -> callbackCount.incrementAndGet()); peerPermissions.allowPeers(true); assertThat(callbackCount).hasValue(1); @@ -47,7 +49,7 @@ public void subscribeUpdate_forCombinedPermissions() { final AtomicInteger restrictedCallbackCount = new AtomicInteger(0); combined.subscribeUpdate( - (permissionsRestricted) -> { + (permissionsRestricted, affectedPeers) -> { callbackCount.incrementAndGet(); if (permissionsRestricted) { restrictedCallbackCount.incrementAndGet(); @@ -102,7 +104,7 @@ public TestPeerPermissions(final boolean allowPeers) { public void allowPeers(final boolean doAllowPeers) { this.allowPeers = doAllowPeers; - dispatchUpdate(!doAllowPeers); + dispatchUpdate(!doAllowPeers, Optional.empty()); } @Override From 3d8926d5998926d9a45c09cde01e1a306c0f40aa Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Mon, 13 May 2019 15:24:15 -0400 Subject: [PATCH 07/19] Simplify permissions checks in discovery controller --- .../internal/PeerDiscoveryController.java | 25 ++++++++----------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java index 875105e9e7..b08a0fae74 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java @@ -198,9 +198,7 @@ public void start() { } final List initialDiscoveryPeers = - bootstrapNodes.stream() - .filter(p -> isPeerPermitted(localPeer, p)) - .collect(Collectors.toList()); + bootstrapNodes.stream().filter(this::isPeerPermitted).collect(Collectors.toList()); initialDiscoveryPeers.stream().forEach(peerTable::tryAdd); recursivePeerRefreshState = @@ -252,10 +250,11 @@ public CompletableFuture stop() { return CompletableFuture.completedFuture(null); } - private boolean isPeerPermitted(final Peer sourcePeer, final Peer destinationPeer) { - return nodePermissioningController - .map(c -> c.isPermitted(sourcePeer.getEnodeURL(), destinationPeer.getEnodeURL())) - .orElse(true); + private boolean isPeerPermitted(final Peer remotePeer) { + return peerPermissions.isPermitted(remotePeer) + && nodePermissioningController + .map(c -> c.isPermitted(localPeer.getEnodeURL(), remotePeer.getEnodeURL())) + .orElse(true); } /** @@ -277,8 +276,8 @@ public void onMessage(final Packet packet, final DiscoveryPeer sender) { return; } - if (!isPeerPermitted(sender, localPeer)) { - LOG.trace("Dropping packet from peer not in the whitelist ({})", sender.getEnodeURLString()); + if (!isPeerPermitted(sender)) { + LOG.trace("Dropping packet from disallowed peer ({})", sender.getEnodeURLString()); return; } @@ -286,11 +285,10 @@ public void onMessage(final Packet packet, final DiscoveryPeer sender) { final Optional maybeKnownPeer = peerTable.get(sender); final DiscoveryPeer peer = maybeKnownPeer.orElse(sender); final boolean peerKnown = maybeKnownPeer.isPresent(); - final boolean peerPermitted = peerPermissions.isPermitted(peer); switch (packet.getType()) { case PING: - if (peerPermitted && addToPeerTable(peer)) { + if (addToPeerTable(peer)) { final PingPacketData ping = packet.getPacketData(PingPacketData.class).get(); respondToPing(ping, packet.getHash(), peer); } @@ -299,9 +297,6 @@ public void onMessage(final Packet packet, final DiscoveryPeer sender) { matchInteraction(packet) .ifPresent( interaction -> { - if (!peerPermitted) { - return; - } addToPeerTable(peer); recursivePeerRefreshState.onBondingComplete(peer); }); @@ -314,7 +309,7 @@ public void onMessage(final Packet packet, final DiscoveryPeer sender) { peer, getPeersFromNeighborsPacket(packet))); break; case FIND_NEIGHBORS: - if (!peerKnown || !peerPermitted) { + if (!peerKnown) { break; } final FindNeighborsPacketData fn = From 83c93e969ea076548cb49351ac27bd4e639c7549 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Mon, 13 May 2019 15:58:13 -0400 Subject: [PATCH 08/19] Remove peer dropped event handling (dead code) --- .../p2p/discovery/PeerDiscoveryAgent.java | 26 --------------- .../internal/PeerDiscoveryController.java | 22 ------------- .../p2p/network/DefaultP2PNetwork.java | 16 ---------- .../PeerDiscoveryTimestampsTest.java | 1 - .../internal/PeerDiscoveryControllerTest.java | 32 ------------------- .../PeerDiscoveryTableRefreshTest.java | 1 - 6 files changed, 98 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java index b794c206c9..8e01329884 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java @@ -22,7 +22,6 @@ import tech.pegasys.pantheon.ethereum.p2p.api.PeerConnection; import tech.pegasys.pantheon.ethereum.p2p.config.DiscoveryConfiguration; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryEvent.PeerBondedEvent; -import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryEvent.PeerDroppedEvent; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.Packet; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerDiscoveryController; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerDiscoveryController.AsyncExecutor; @@ -89,7 +88,6 @@ public abstract class PeerDiscoveryAgent implements DisconnectCallback { /* Is discovery enabled? */ private boolean isActive = false; private final Subscribers> peerBondedObservers = new Subscribers<>(); - private final Subscribers> peerDroppedObservers = new Subscribers<>(); public PeerDiscoveryAgent( final SECP256K1.KeyPair keyPair, @@ -177,7 +175,6 @@ private PeerDiscoveryController createController() { peerPermissions, nodePermissioningController, peerBondedObservers, - peerDroppedObservers, metricsSystem); } @@ -272,29 +269,6 @@ public boolean removePeerBondedObserver(final long observerId) { return peerBondedObservers.unsubscribe(observerId); } - /** - * Adds an observer that will get called when a peer is dropped from the peer table. - * - *

No guarantees are made about the order in which observers are invoked. - * - * @param observer The observer to call. - * @return A unique ID identifying this observer, to that it can be removed later. - */ - public long observePeerDroppedEvents(final Consumer observer) { - checkNotNull(observer); - return peerDroppedObservers.subscribe(observer); - } - - /** - * Removes an previously added peer dropped observer. - * - * @param observerId The unique ID identifying the observer to remove. - * @return Whether the observer was located and removed. - */ - public boolean removePeerDroppedObserver(final long observerId) { - return peerDroppedObservers.unsubscribe(observerId); - } - /** * Returns the count of observers that are registered on this controller. * diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java index b08a0fae74..5f87843fcd 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java @@ -21,10 +21,7 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.DiscoveryPeer; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryEvent; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryEvent.PeerBondedEvent; -import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryEvent.PeerDroppedEvent; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryStatus; -import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerTable.EvictResult; -import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerTable.EvictResult.EvictOutcome; import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningController; @@ -137,7 +134,6 @@ public class PeerDiscoveryController { // Observers for "peer bonded" discovery events. private final Subscribers> peerBondedObservers; - private final Subscribers> peerDroppedObservers; private RecursivePeerRefreshState recursivePeerRefreshState; @@ -154,7 +150,6 @@ public PeerDiscoveryController( final PeerPermissions peerPermissions, final Optional nodePermissioningController, final Subscribers> peerBondedObservers, - final Subscribers> peerDroppedObservers, final MetricsSystem metricsSystem) { this.timerUtil = timerUtil; this.keypair = keypair; @@ -168,7 +163,6 @@ public PeerDiscoveryController( this.nodePermissioningController = nodePermissioningController; this.outboundMessageHandler = outboundMessageHandler; this.peerBondedObservers = peerBondedObservers; - this.peerDroppedObservers = peerDroppedObservers; this.discoveryProtocolLogger = new DiscoveryProtocolLogger(metricsSystem); metricsSystem.createIntegerGauge( @@ -362,27 +356,11 @@ private boolean addToPeerTable(final DiscoveryPeer peer) { return true; } - @VisibleForTesting - boolean dropFromPeerTable(final DiscoveryPeer peer) { - final EvictResult evictResult = peerTable.tryEvict(peer); - if (evictResult.getOutcome() == EvictOutcome.EVICTED) { - notifyPeerDropped(peer, System.currentTimeMillis()); - return true; - } else { - return false; - } - } - private void notifyPeerBonded(final DiscoveryPeer peer, final long now) { final PeerBondedEvent event = new PeerBondedEvent(peer, now); dispatchEvent(peerBondedObservers, event); } - private void notifyPeerDropped(final DiscoveryPeer peer, final long now) { - final PeerDroppedEvent event = new PeerDroppedEvent(peer, now); - dispatchEvent(peerDroppedObservers, event); - } - private Optional matchInteraction(final Packet packet) { final PeerInteractionState interaction = inflightInteractions.get(packet.getNodeId()); if (interaction == null || !interaction.test(packet)) { diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java index 9c7268ef2a..b07a9a67ed 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java @@ -27,7 +27,6 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.DiscoveryPeer; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryAgent; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryEvent.PeerBondedEvent; -import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryEvent.PeerDroppedEvent; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryStatus; import tech.pegasys.pantheon.ethereum.p2p.discovery.VertxPeerDiscoveryAgent; import tech.pegasys.pantheon.ethereum.p2p.network.netty.Callbacks; @@ -173,7 +172,6 @@ public class DefaultP2PNetwork implements P2PNetwork { private final LabelledMetric outboundMessagesCounter; private OptionalLong blockAddedObserverId = OptionalLong.empty(); private OptionalLong peerBondedObserverId = OptionalLong.empty(); - private OptionalLong peerDroppedObserverId = OptionalLong.empty(); private final AtomicBoolean started = new AtomicBoolean(false); private final AtomicBoolean stopped = new AtomicBoolean(false); @@ -548,8 +546,6 @@ public void start() { peerDiscoveryAgent.start(listeningPort).join(); peerBondedObserverId = OptionalLong.of(peerDiscoveryAgent.observePeerBondedEvents(handlePeerBondedEvent())); - peerDroppedObserverId = - OptionalLong.of(peerDiscoveryAgent.observePeerDroppedEvents(handlePeerDroppedEvents())); if (nodePermissioningController.isPresent()) { if (blockchain.isPresent()) { @@ -584,16 +580,6 @@ Consumer handlePeerBondedEvent() { }; } - private Consumer handlePeerDroppedEvents() { - return event -> { - final Peer peer = event.getPeer(); - getPeers().stream() - .filter(p -> p.getPeerInfo().getNodeId().equals(peer.getId())) - .findFirst() - .ifPresent(p -> p.disconnect(DisconnectReason.REQUESTED)); - }; - } - private synchronized void checkCurrentConnections() { connections .getPeerConnections() @@ -656,8 +642,6 @@ public void stop() { peerDiscoveryAgent.stop().join(); peerBondedObserverId.ifPresent(peerDiscoveryAgent::removePeerBondedObserver); peerBondedObserverId = OptionalLong.empty(); - peerDroppedObserverId.ifPresent(peerDiscoveryAgent::removePeerDroppedObserver); - peerDroppedObserverId = OptionalLong.empty(); blockchain.ifPresent(b -> blockAddedObserverId.ifPresent(b::removeObserver)); blockAddedObserverId = OptionalLong.empty(); peerDiscoveryAgent.stop().join(); diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryTimestampsTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryTimestampsTest.java index d910949754..a0ae9b2f0a 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryTimestampsTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryTimestampsTest.java @@ -66,7 +66,6 @@ public void lastSeenAndFirstDiscoveredTimestampsUpdatedOnMessage() { PeerPermissions.noop(), Optional.empty(), new Subscribers<>(), - new Subscribers<>(), new NoOpMetricsSystem()); controller.start(); diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java index 74b97d97db..c50cf975f4 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java @@ -26,7 +26,6 @@ import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; import tech.pegasys.pantheon.crypto.SECP256K1; @@ -35,10 +34,8 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.DiscoveryPeer; import tech.pegasys.pantheon.ethereum.p2p.discovery.Endpoint; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryEvent.PeerBondedEvent; -import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryEvent.PeerDroppedEvent; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryStatus; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryTestHelper; -import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerTable.EvictResult; import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissionsBlacklist; @@ -1055,27 +1052,6 @@ public void shouldNotRespondToPingFromNonWhitelistedDiscoveryPeer() { assertThat(controller.streamDiscoveredPeers()).doesNotContain(peers.get(0)); } - @Test - @SuppressWarnings({"unchecked", "rawtypes"}) - public void whenPeerIsNotEvictedDropFromTableShouldReturnFalseAndNotifyZeroObservers() { - final List peers = createPeersInLastBucket(localPeer, 1); - final DiscoveryPeer peer = peers.get(0); - final PeerTable peerTableSpy = spy(peerTable); - final Consumer peerDroppedEventConsumer = mock(Consumer.class); - final Subscribers> peerDroppedSubscribers = new Subscribers(); - peerDroppedSubscribers.subscribe(peerDroppedEventConsumer); - - doReturn(EvictResult.absent()).when(peerTableSpy).tryEvict(any()); - - controller = getControllerBuilder().peerDroppedObservers(peerDroppedSubscribers).build(); - - controller.start(); - final boolean dropped = controller.dropFromPeerTable(peer); - - assertThat(dropped).isFalse(); - verifyZeroInteractions(peerDroppedEventConsumer); - } - private static Packet mockPingPacket(final DiscoveryPeer from, final DiscoveryPeer to) { final Packet packet = mock(Packet.class); @@ -1145,7 +1121,6 @@ static class ControllerBuilder { private OutboundMessageHandler outboundMessageHandler = OutboundMessageHandler.NOOP; private static final PeerDiscoveryTestHelper helper = new PeerDiscoveryTestHelper(); private Subscribers> peerBondedObservers = new Subscribers<>(); - private Subscribers> peerDroppedObservers = new Subscribers<>(); private PeerPermissions peerPermissions = PeerPermissions.noop(); public static ControllerBuilder create() { @@ -1202,12 +1177,6 @@ ControllerBuilder peerBondedObservers(final Subscribers> observers) { - this.peerDroppedObservers = observers; - return this; - } - PeerDiscoveryController build() { checkNotNull(keypair); if (localPeer == null) { @@ -1230,7 +1199,6 @@ PeerDiscoveryController build() { peerPermissions, nodePermissioningController, peerBondedObservers, - peerDroppedObservers, new NoOpMetricsSystem())); } } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryTableRefreshTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryTableRefreshTest.java index 62802192b5..11f5726287 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryTableRefreshTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryTableRefreshTest.java @@ -67,7 +67,6 @@ public void tableRefreshSingleNode() { PeerPermissions.noop(), Optional.empty(), new Subscribers<>(), - new Subscribers<>(), new NoOpMetricsSystem())); controller.start(); From 0b3b7f46327c5fdd3c37ceac6fb819c4d96f463d Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Mon, 13 May 2019 18:15:11 -0400 Subject: [PATCH 09/19] Add PeerDiscoveryController Builder --- .../p2p/discovery/PeerDiscoveryAgent.java | 31 ++-- .../internal/OutboundMessageHandler.java | 2 +- .../internal/PeerDiscoveryController.java | 143 +++++++++++++++++- .../discovery/internal/PeerRequirement.java | 2 + .../PeerDiscoveryTimestampsTest.java | 26 ++-- .../internal/PeerDiscoveryControllerTest.java | 29 ++-- .../PeerDiscoveryTableRefreshTest.java | 27 ++-- 7 files changed, 195 insertions(+), 65 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java index 8e01329884..af3ac14315 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java @@ -14,7 +14,6 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; -import static java.util.concurrent.TimeUnit.MILLISECONDS; import static tech.pegasys.pantheon.util.bytes.BytesValue.wrapBuffer; import tech.pegasys.pantheon.crypto.SECP256K1; @@ -46,7 +45,6 @@ import java.util.OptionalInt; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CopyOnWriteArrayList; -import java.util.concurrent.TimeUnit; import java.util.function.Consumer; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -66,7 +64,6 @@ public abstract class PeerDiscoveryAgent implements DisconnectCallback { // The devp2p specification says only accept packets up to 1280, but some // clients ignore that, so we add in a little extra padding. private static final int MAX_PACKET_SIZE_BYTES = 1600; - private static final long PEER_REFRESH_INTERVAL_MS = MILLISECONDS.convert(30, TimeUnit.MINUTES); protected final List bootstrapPeers; private final List peerRequirements = new CopyOnWriteArrayList<>(); @@ -162,20 +159,20 @@ private void startController() { } private PeerDiscoveryController createController() { - return new PeerDiscoveryController( - keyPair, - advertisedPeer, - peerTable, - bootstrapPeers, - this::handleOutgoingPacket, - createTimer(), - createWorkerExecutor(), - PEER_REFRESH_INTERVAL_MS, - PeerRequirement.combine(peerRequirements), - peerPermissions, - nodePermissioningController, - peerBondedObservers, - metricsSystem); + return PeerDiscoveryController.builder() + .keypair(keyPair) + .localPeer(advertisedPeer) + .peerTable(peerTable) + .bootstrapNodes(bootstrapPeers) + .outboundMessageHandler(this::handleOutgoingPacket) + .timerUtil(createTimer()) + .workerExecutor(createWorkerExecutor()) + .peerRequirement(PeerRequirement.combine(peerRequirements)) + .peerPermissions(peerPermissions) + .nodePermissioningController(nodePermissioningController) + .peerBondedObservers(peerBondedObservers) + .metricsSystem(metricsSystem) + .build(); } protected boolean validatePacketSize(final int packetSize) { diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/OutboundMessageHandler.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/OutboundMessageHandler.java index c04085c646..0c43da994f 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/OutboundMessageHandler.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/OutboundMessageHandler.java @@ -16,7 +16,7 @@ @FunctionalInterface public interface OutboundMessageHandler { - public static OutboundMessageHandler NOOP = (peer, packet) -> {}; + OutboundMessageHandler NOOP = (peer, packet) -> {}; void send(final DiscoveryPeer toPeer, final Packet packet); } diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java index 5f87843fcd..c40df2481b 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java @@ -12,6 +12,9 @@ */ package tech.pegasys.pantheon.ethereum.p2p.discovery.internal; +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Preconditions.checkState; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; import static tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerTable.AddResult.AddOutcome; @@ -32,6 +35,7 @@ import tech.pegasys.pantheon.util.Subscribers; import tech.pegasys.pantheon.util.bytes.BytesValue; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.List; @@ -40,6 +44,7 @@ import java.util.OptionalLong; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Consumer; import java.util.function.Predicate; @@ -137,7 +142,7 @@ public class PeerDiscoveryController { private RecursivePeerRefreshState recursivePeerRefreshState; - public PeerDiscoveryController( + private PeerDiscoveryController( final KeyPair keypair, final DiscoveryPeer localPeer, final PeerTable peerTable, @@ -186,6 +191,10 @@ public PeerDiscoveryController( "type"); } + public static Builder builder() { + return new Builder(); + } + public void start() { if (!started.compareAndSet(false, true)) { throw new IllegalStateException("The peer table had already been started"); @@ -614,4 +623,136 @@ void cancelTimers() { public interface AsyncExecutor { CompletableFuture execute(Supplier action); } + + public static class Builder { + // Options with default values + private OutboundMessageHandler outboundMessageHandler = OutboundMessageHandler.NOOP; + private PeerRequirement peerRequirement = PeerRequirement.NOOP; + private PeerPermissions peerPermissions = PeerPermissions.noop(); + private long tableRefreshIntervalMs = MILLISECONDS.convert(30, TimeUnit.MINUTES); + private List bootstrapNodes = new ArrayList<>(); + private Optional nodePermissioningController = Optional.empty(); + + // Required dependencies + private KeyPair keypair; + private DiscoveryPeer localPeer; + private PeerTable peerTable; + private TimerUtil timerUtil; + private AsyncExecutor workerExecutor; + private Subscribers> peerBondedObservers = new Subscribers<>(); + private MetricsSystem metricsSystem; + + private Builder() {} + + public PeerDiscoveryController build() { + validate(); + return new PeerDiscoveryController( + keypair, + localPeer, + peerTable, + bootstrapNodes, + outboundMessageHandler, + timerUtil, + workerExecutor, + tableRefreshIntervalMs, + peerRequirement, + peerPermissions, + nodePermissioningController, + peerBondedObservers, + metricsSystem); + } + + private void validate() { + validateRequiredDependency(keypair, "KeyPair"); + validateRequiredDependency(localPeer, "LocalPeer"); + validateRequiredDependency(peerTable, "PeerTable"); + validateRequiredDependency(timerUtil, "TimerUtil"); + validateRequiredDependency(workerExecutor, "AsyncExecutor"); + validateRequiredDependency(metricsSystem, "MetricsSystem"); + validateRequiredDependency(peerBondedObservers, "PeerBondedObservers"); + } + + private void validateRequiredDependency(final Object object, final String name) { + checkState(object != null, name + " must be configured."); + } + + public Builder keypair(final KeyPair keypair) { + checkNotNull(keypair); + this.keypair = keypair; + return this; + } + + public Builder localPeer(final DiscoveryPeer localPeer) { + checkNotNull(localPeer); + this.localPeer = localPeer; + return this; + } + + public Builder peerTable(final PeerTable peerTable) { + checkNotNull(peerTable); + this.peerTable = peerTable; + return this; + } + + public Builder bootstrapNodes(final Collection bootstrapNodes) { + this.bootstrapNodes.addAll(bootstrapNodes); + return this; + } + + public Builder outboundMessageHandler(final OutboundMessageHandler outboundMessageHandler) { + checkNotNull(outboundMessageHandler); + this.outboundMessageHandler = outboundMessageHandler; + return this; + } + + public Builder timerUtil(final TimerUtil timerUtil) { + checkNotNull(timerUtil); + this.timerUtil = timerUtil; + return this; + } + + public Builder workerExecutor(final AsyncExecutor workerExecutor) { + checkNotNull(workerExecutor); + this.workerExecutor = workerExecutor; + return this; + } + + public Builder tableRefreshIntervalMs(final long tableRefreshIntervalMs) { + checkArgument(tableRefreshIntervalMs >= 0); + this.tableRefreshIntervalMs = tableRefreshIntervalMs; + return this; + } + + public Builder peerRequirement(final PeerRequirement peerRequirement) { + checkNotNull(peerRequirement); + this.peerRequirement = peerRequirement; + return this; + } + + public Builder peerPermissions(final PeerPermissions peerPermissions) { + checkNotNull(peerPermissions); + this.peerPermissions = peerPermissions; + return this; + } + + public Builder nodePermissioningController( + final Optional nodePermissioningController) { + checkNotNull(nodePermissioningController); + this.nodePermissioningController = nodePermissioningController; + return this; + } + + public Builder peerBondedObservers( + final Subscribers> peerBondedObservers) { + checkNotNull(peerBondedObservers); + this.peerBondedObservers = peerBondedObservers; + return this; + } + + public Builder metricsSystem(final MetricsSystem metricsSystem) { + checkNotNull(metricsSystem); + this.metricsSystem = metricsSystem; + return this; + } + } } diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerRequirement.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerRequirement.java index 8f21fd42cf..7398324a4b 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerRequirement.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerRequirement.java @@ -17,6 +17,8 @@ @FunctionalInterface public interface PeerRequirement { + PeerRequirement NOOP = () -> true; + boolean hasSufficientPeers(); static PeerRequirement combine(final Collection peerRequirements) { diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryTimestampsTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryTimestampsTest.java index a0ae9b2f0a..2dd15356ca 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryTimestampsTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryTimestampsTest.java @@ -20,13 +20,11 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.BlockingAsyncExecutor; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.MockPeerDiscoveryAgent; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.MockTimerUtil; -import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.OutboundMessageHandler; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.Packet; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PacketType; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerDiscoveryController; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerTable; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PingPacketData; -import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem; import tech.pegasys.pantheon.util.Subscribers; @@ -53,20 +51,16 @@ public void lastSeenAndFirstDiscoveredTimestampsUpdatedOnMessage() { final KeyPair localKeyPair = keypairs.get(0); final PeerDiscoveryController controller = - new PeerDiscoveryController( - localKeyPair, - localPeer, - new PeerTable(agent.getAdvertisedPeer().get().getId()), - Collections.emptyList(), - OutboundMessageHandler.NOOP, - new MockTimerUtil(), - new BlockingAsyncExecutor(), - TimeUnit.HOURS.toMillis(1), - () -> true, - PeerPermissions.noop(), - Optional.empty(), - new Subscribers<>(), - new NoOpMetricsSystem()); + PeerDiscoveryController.builder() + .keypair(localKeyPair) + .localPeer(localPeer) + .peerTable(new PeerTable(agent.getAdvertisedPeer().get().getId())) + .timerUtil(new MockTimerUtil()) + .workerExecutor(new BlockingAsyncExecutor()) + .tableRefreshIntervalMs(TimeUnit.HOURS.toMillis(1)) + .peerBondedObservers(new Subscribers<>()) + .metricsSystem(new NoOpMetricsSystem()) + .build(); controller.start(); final PingPacketData ping = diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java index c50cf975f4..217bf0dcd2 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java @@ -1186,20 +1186,21 @@ PeerDiscoveryController build() { peerTable = new PeerTable(localPeer.getId()); } return spy( - new PeerDiscoveryController( - keypair, - localPeer, - peerTable, - discoPeers, - outboundMessageHandler, - timerUtil, - new BlockingAsyncExecutor(), - TABLE_REFRESH_INTERVAL_MS, - PEER_REQUIREMENT, - peerPermissions, - nodePermissioningController, - peerBondedObservers, - new NoOpMetricsSystem())); + PeerDiscoveryController.builder() + .keypair(keypair) + .localPeer(localPeer) + .peerTable(peerTable) + .bootstrapNodes(discoPeers) + .outboundMessageHandler(outboundMessageHandler) + .timerUtil(timerUtil) + .workerExecutor(new BlockingAsyncExecutor()) + .tableRefreshIntervalMs(TABLE_REFRESH_INTERVAL_MS) + .peerRequirement(PEER_REQUIREMENT) + .peerPermissions(peerPermissions) + .nodePermissioningController(nodePermissioningController) + .peerBondedObservers(peerBondedObservers) + .metricsSystem(new NoOpMetricsSystem()) + .build()); } } } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryTableRefreshTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryTableRefreshTest.java index 11f5726287..db1d8cd0f1 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryTableRefreshTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryTableRefreshTest.java @@ -12,7 +12,6 @@ */ package tech.pegasys.pantheon.ethereum.p2p.discovery.internal; -import static java.util.Collections.emptyList; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.atLeast; @@ -25,7 +24,6 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.DiscoveryPeer; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryStatus; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryTestHelper; -import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem; import tech.pegasys.pantheon.util.Subscribers; import tech.pegasys.pantheon.util.bytes.BytesValue; @@ -54,20 +52,17 @@ public void tableRefreshSingleNode() { final MockTimerUtil timer = new MockTimerUtil(); final PeerDiscoveryController controller = spy( - new PeerDiscoveryController( - localKeyPair, - localPeer, - new PeerTable(localPeer.getId()), - emptyList(), - outboundMessageHandler, - timer, - new BlockingAsyncExecutor(), - 0, - () -> true, - PeerPermissions.noop(), - Optional.empty(), - new Subscribers<>(), - new NoOpMetricsSystem())); + PeerDiscoveryController.builder() + .keypair(localKeyPair) + .localPeer(localPeer) + .peerTable(new PeerTable(localPeer.getId())) + .outboundMessageHandler(outboundMessageHandler) + .timerUtil(timer) + .workerExecutor(new BlockingAsyncExecutor()) + .tableRefreshIntervalMs(0) + .peerBondedObservers(new Subscribers<>()) + .metricsSystem(new NoOpMetricsSystem()) + .build()); controller.start(); // Send a PING, so as to add a Peer in the controller. From b95a78e236a57d7ca30a878445f9d4ffdcbb69bf Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Tue, 14 May 2019 15:18:29 -0400 Subject: [PATCH 10/19] Be more selective when dropping peers from table Rather than dropping peers from the peer table for any disconnect, drop peers from the table on permissions changes, when peers are requested to be disconnected, and when peers are disconnecting for failed permissions check. --- .../p2p/discovery/PeerDiscoveryAgent.java | 24 ++++----------- .../internal/PeerDiscoveryController.java | 29 ++++++++++++++++--- .../p2p/network/DefaultP2PNetwork.java | 5 +++- .../p2p/discovery/PeerDiscoveryAgentTest.java | 23 ++++----------- 4 files changed, 41 insertions(+), 40 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java index af3ac14315..c44a5bd256 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgent.java @@ -17,20 +17,16 @@ import static tech.pegasys.pantheon.util.bytes.BytesValue.wrapBuffer; import tech.pegasys.pantheon.crypto.SECP256K1; -import tech.pegasys.pantheon.ethereum.p2p.api.DisconnectCallback; -import tech.pegasys.pantheon.ethereum.p2p.api.PeerConnection; import tech.pegasys.pantheon.ethereum.p2p.config.DiscoveryConfiguration; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryEvent.PeerBondedEvent; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.Packet; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerDiscoveryController; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerDiscoveryController.AsyncExecutor; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerRequirement; -import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerTable; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PingPacketData; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.TimerUtil; -import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeerId; +import tech.pegasys.pantheon.ethereum.p2p.peers.PeerId; import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; -import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage; import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningController; import tech.pegasys.pantheon.metrics.MetricsSystem; import tech.pegasys.pantheon.util.NetworkUtility; @@ -58,7 +54,7 @@ * The peer discovery agent is the network component that sends and receives peer discovery messages * via UDP. */ -public abstract class PeerDiscoveryAgent implements DisconnectCallback { +public abstract class PeerDiscoveryAgent { protected static final Logger LOG = LogManager.getLogger(); // The devp2p specification says only accept packets up to 1280, but some @@ -76,7 +72,6 @@ public abstract class PeerDiscoveryAgent implements DisconnectCallback { /* The keypair used to sign messages. */ protected final SECP256K1.KeyPair keyPair; private final BytesValue id; - private final PeerTable peerTable; protected final DiscoveryConfiguration config; /* This is the {@link tech.pegasys.pantheon.ethereum.p2p.Peer} object holding who we are. */ @@ -105,7 +100,6 @@ public PeerDiscoveryAgent( this.config = config; this.keyPair = keyPair; - this.peerTable = new PeerTable(keyPair.getPublicKey().getEncodedBytes(), 16); id = keyPair.getPublicKey().getEncodedBytes(); } @@ -162,7 +156,6 @@ private PeerDiscoveryController createController() { return PeerDiscoveryController.builder() .keypair(keyPair) .localPeer(advertisedPeer) - .peerTable(peerTable) .bootstrapNodes(bootstrapPeers) .outboundMessageHandler(this::handleOutgoingPacket) .timerUtil(createTimer()) @@ -234,6 +227,10 @@ public Stream streamDiscoveredPeers() { return controller.map(PeerDiscoveryController::streamDiscoveredPeers).orElse(Stream.empty()); } + public void dropPeer(final PeerId peer) { + controller.ifPresent(c -> c.dropPeer(peer)); + } + public Optional getAdvertisedPeer() { return Optional.ofNullable(advertisedPeer); } @@ -291,15 +288,6 @@ private static void validateConfiguration(final DiscoveryConfiguration config) { checkArgument(config.getBucketSize() > 0, "bucket size cannot be negative nor zero"); } - @Override - public void onDisconnect( - final PeerConnection connection, - final DisconnectMessage.DisconnectReason reason, - final boolean initiatedByPeer) { - final BytesValue nodeId = connection.getPeerInfo().getNodeId(); - peerTable.tryEvict(new DefaultPeerId(nodeId)); - } - /** * Returns the current state of the PeerDiscoveryAgent. * diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java index c40df2481b..596e019043 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java @@ -26,6 +26,7 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryEvent.PeerBondedEvent; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryStatus; import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; +import tech.pegasys.pantheon.ethereum.p2p.peers.PeerId; import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningController; import tech.pegasys.pantheon.metrics.Counter; @@ -101,7 +102,6 @@ * condition, the peer will be physically dropped (eliminated) from the table. */ public class PeerDiscoveryController { - private static final Logger LOG = LogManager.getLogger(); private static final long REFRESH_CHECK_INTERVAL_MILLIS = MILLISECONDS.convert(30, SECONDS); private static final int PEER_REFRESH_ROUND_TIMEOUT_IN_SECONDS = 5; @@ -216,6 +216,8 @@ public void start() { PEER_REFRESH_ROUND_TIMEOUT_IN_SECONDS, 100); + peerPermissions.subscribeUpdate(this::handlePermissionsUpdate); + if (nodePermissioningController.isPresent()) { // if smart contract permissioning is enabled, bond with bootnodes @@ -260,6 +262,21 @@ private boolean isPeerPermitted(final Peer remotePeer) { .orElse(true); } + private void handlePermissionsUpdate( + final boolean addRestrictions, final Optional> affectedPeers) { + if (!addRestrictions) { + // Nothing to do if permissions were relaxed + return; + } + + // If we have an explicit list of peers, drop each peer from our discovery table + affectedPeers.ifPresent(peers -> peers.forEach(this::dropPeer)); + } + + public void dropPeer(final PeerId peer) { + peerTable.tryEvict(peer); + } + /** * Handles an incoming message and processes it based on the state machine for the {@link * DiscoveryPeer}. @@ -632,20 +649,25 @@ public static class Builder { private long tableRefreshIntervalMs = MILLISECONDS.convert(30, TimeUnit.MINUTES); private List bootstrapNodes = new ArrayList<>(); private Optional nodePermissioningController = Optional.empty(); + private PeerTable peerTable; + private Subscribers> peerBondedObservers = new Subscribers<>(); // Required dependencies private KeyPair keypair; private DiscoveryPeer localPeer; - private PeerTable peerTable; private TimerUtil timerUtil; private AsyncExecutor workerExecutor; - private Subscribers> peerBondedObservers = new Subscribers<>(); private MetricsSystem metricsSystem; private Builder() {} public PeerDiscoveryController build() { validate(); + + if (peerTable == null) { + peerTable = new PeerTable(this.keypair.getPublicKey().getEncodedBytes(), 16); + } + return new PeerDiscoveryController( keypair, localPeer, @@ -665,7 +687,6 @@ public PeerDiscoveryController build() { private void validate() { validateRequiredDependency(keypair, "KeyPair"); validateRequiredDependency(localPeer, "LocalPeer"); - validateRequiredDependency(peerTable, "PeerTable"); validateRequiredDependency(timerUtil, "TimerUtil"); validateRequiredDependency(workerExecutor, "AsyncExecutor"); validateRequiredDependency(metricsSystem, "MetricsSystem"); diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java index b07a9a67ed..a6b3ca1934 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java @@ -225,7 +225,6 @@ public class DefaultP2PNetwork implements P2PNetwork { c -> c.subscribeToUpdates(this::checkCurrentConnections)); subscribeDisconnect(reputationManager); - subscribeDisconnect(peerDiscoveryAgent); subscribeDisconnect(connections); outboundMessagesCounter = @@ -350,6 +349,7 @@ protected void initChannel(final SocketChannel ch) { if (!isPeerAllowed(connection)) { connection.disconnect(DisconnectReason.UNKNOWN); + peerDiscoveryAgent.dropPeer(connection.getPeer()); return; } @@ -388,6 +388,8 @@ public boolean removeMaintainedConnectionPeer(final Peer peer) { final Optional peerConnection = connections.getConnectionForPeer(peer.getId()); peerConnection.ifPresent(pc -> pc.disconnect(DisconnectReason.REQUESTED)); + peerDiscoveryAgent.dropPeer(peer); + return removed; } @@ -587,6 +589,7 @@ private synchronized void checkCurrentConnections() { peerConnection -> { if (!isPeerAllowed(peerConnection)) { peerConnection.disconnect(DisconnectReason.REQUESTED); + peerDiscoveryAgent.dropPeer(peerConnection.getPeer()); } }); } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java index 36031fa025..0d93294970 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java @@ -15,10 +15,7 @@ import static java.util.stream.Collectors.toList; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; -import tech.pegasys.pantheon.ethereum.p2p.api.PeerConnection; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryTestHelper.AgentBuilder; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.FindNeighborsPacketData; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.MockPeerDiscoveryAgent; @@ -28,9 +25,6 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PacketType; import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissionsBlacklist; -import tech.pegasys.pantheon.ethereum.p2p.wire.PeerInfo; -import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage.DisconnectReason; -import tech.pegasys.pantheon.util.bytes.BytesValue; import tech.pegasys.pantheon.util.enode.EnodeURL; import java.util.Collections; @@ -141,18 +135,20 @@ public void neighborsPacketLimited() { } @Test - public void shouldEvictPeerOnDisconnect() { + public void shouldEvictPeerWhenPermissionsRevoked() { + final PeerPermissionsBlacklist blacklist = PeerPermissionsBlacklist.create(); final MockPeerDiscoveryAgent peerDiscoveryAgent1 = helper.startDiscoveryAgent(); peerDiscoveryAgent1.start(BROADCAST_TCP_PORT).join(); final DiscoveryPeer peer = peerDiscoveryAgent1.getAdvertisedPeer().get(); - final MockPeerDiscoveryAgent peerDiscoveryAgent2 = helper.startDiscoveryAgent(peer); + final MockPeerDiscoveryAgent peerDiscoveryAgent2 = + helper.startDiscoveryAgent( + helper.agentBuilder().peerPermissions(blacklist).bootstrapPeers(peer)); peerDiscoveryAgent2.start(BROADCAST_TCP_PORT).join(); assertThat(peerDiscoveryAgent2.streamDiscoveredPeers().collect(toList()).size()).isEqualTo(1); - final PeerConnection peerConnection = createAnonymousPeerConnection(peer.getId()); - peerDiscoveryAgent2.onDisconnect(peerConnection, DisconnectReason.REQUESTED, true); + blacklist.add(peer); assertThat(peerDiscoveryAgent2.streamDiscoveredPeers().collect(toList()).size()).isEqualTo(0); } @@ -196,11 +192,4 @@ public void shouldNotBeActiveWhenConfigIsFalse() { assertThat(agent.isActive()).isFalse(); } - - private PeerConnection createAnonymousPeerConnection(final BytesValue id) { - final PeerConnection conn = mock(PeerConnection.class); - final PeerInfo peerInfo = new PeerInfo(0, null, null, 0, id); - when(conn.getPeerInfo()).thenReturn(peerInfo); - return conn; - } } From 08ca7b3e1134f39f7a22b0d2d1fe452c5077c0ed Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Tue, 14 May 2019 16:04:31 -0400 Subject: [PATCH 11/19] Validate banned-node-ids as valid node ids --- .../tech/pegasys/pantheon/RunnerBuilder.java | 9 ++-- .../pegasys/pantheon/cli/PantheonCommand.java | 30 ++++++++++- .../pantheon/cli/CommandTestAbstract.java | 2 + .../pantheon/cli/PantheonCommandTest.java | 50 +++++++++++++------ 4 files changed, 70 insertions(+), 21 deletions(-) diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java b/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java index af19a38f95..a2a8ef56ea 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java @@ -105,7 +105,7 @@ public class RunnerBuilder { private GraphQLRpcConfiguration graphQLRpcConfiguration; private WebSocketConfiguration webSocketConfiguration; private Path dataDir; - private Collection bannedNodeIds = new ArrayList<>(); + private Collection bannedNodeIds = new ArrayList<>(); private MetricsConfiguration metricsConfiguration; private MetricsSystem metricsSystem; private Optional permissioningConfiguration = Optional.empty(); @@ -178,7 +178,7 @@ public RunnerBuilder dataDir(final Path dataDir) { return this; } - public RunnerBuilder bannedNodeIds(final Collection bannedNodeIds) { + public RunnerBuilder bannedNodeIds(final Collection bannedNodeIds) { this.bannedNodeIds.addAll(bannedNodeIds); return this; } @@ -242,10 +242,7 @@ public Runner build() { .setSupportedProtocols(subProtocols); final PeerPermissionsBlacklist bannedNodes = PeerPermissionsBlacklist.create(); - // TODO - validate and parse banned node ids as BytesValue's in {@link PantheonCommand} - bannedNodeIds.stream() - .map(n -> BytesValue.fromHexString(n, EnodeURL.NODE_ID_SIZE)) - .forEach(bannedNodes::add); + bannedNodeIds.forEach(bannedNodes::add); final List bootnodes = discoveryConfiguration.getBootnodes(); diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java b/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java index 7877dc281d..13641bca25 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java @@ -12,6 +12,7 @@ */ package tech.pegasys.pantheon.cli; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.Arrays.asList; @@ -214,7 +215,34 @@ void setBootnodes(final List values) { description = "A list of node IDs to ban from the P2P network.", split = ",", arity = "1..*") - private final Collection bannedNodeIds = new ArrayList<>(); + void setBannedNodeIds(final List values) { + try { + bannedNodeIds = + values.stream() + .filter(value -> !value.isEmpty()) + .map( + v -> { + validateNodeIdString(v); + return BytesValue.fromHexString(v, EnodeURL.NODE_ID_SIZE); + }) + .collect(Collectors.toList()); + } catch (final IllegalArgumentException e) { + throw new ParameterException( + commandLine, "Invalid ids supplied to '--banned-node-ids'. " + e.getMessage()); + } + } + + private static void validateNodeIdString(final String nodeId) { + int expectedSize = EnodeURL.NODE_ID_SIZE * 2; + if (nodeId.toLowerCase().startsWith("0x")) { + expectedSize += 2; + } + checkArgument( + nodeId.length() == expectedSize, + "Expected " + EnodeURL.NODE_ID_SIZE + " bytes in " + nodeId); + } + + private Collection bannedNodeIds = new ArrayList<>(); @Option( names = {"--sync-mode"}, diff --git a/pantheon/src/test/java/tech/pegasys/pantheon/cli/CommandTestAbstract.java b/pantheon/src/test/java/tech/pegasys/pantheon/cli/CommandTestAbstract.java index b0d35a1c8a..04ae15fde3 100644 --- a/pantheon/src/test/java/tech/pegasys/pantheon/cli/CommandTestAbstract.java +++ b/pantheon/src/test/java/tech/pegasys/pantheon/cli/CommandTestAbstract.java @@ -34,6 +34,7 @@ import tech.pegasys.pantheon.metrics.prometheus.MetricsConfiguration; import tech.pegasys.pantheon.services.kvstore.RocksDbConfiguration; import tech.pegasys.pantheon.util.BlockImporter; +import tech.pegasys.pantheon.util.bytes.BytesValue; import java.io.ByteArrayOutputStream; import java.io.File; @@ -85,6 +86,7 @@ public abstract class CommandTestAbstract { @Mock BlockImporter mockBlockImporter; @Mock Logger mockLogger; + @Captor ArgumentCaptor> bytesValueCollectionCollector; @Captor ArgumentCaptor> stringListArgumentCaptor; @Captor ArgumentCaptor pathArgumentCaptor; @Captor ArgumentCaptor fileArgumentCaptor; diff --git a/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java b/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java index cf8ecdb383..8bc3694023 100644 --- a/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java +++ b/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java @@ -60,6 +60,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -981,15 +982,6 @@ public void callingWithInvalidBootnodeAndEqualSignMustDisplayErrorAndUsage() { assertThat(commandErrorOutput.toString()).startsWith(expectedErrorOutputStart); } - @Test - public void callingWithBannedNodeidsOptionButNoValueMustDisplayErrorAndUsage() { - parseCommand("--banned-node-ids"); - assertThat(commandOutput.toString()).isEmpty(); - final String expectedErrorOutputStart = - "Missing required parameter for option '--banned-node-ids' at index 0 ()"; - assertThat(commandErrorOutput.toString()).startsWith(expectedErrorOutputStart); - } - @Test public void bootnodesOptionMustBeUsed() { parseCommand("--bootnodes", String.join(",", validENodeStrings)); @@ -1007,18 +999,48 @@ public void bootnodesOptionMustBeUsed() { @Test public void bannedNodeIdsOptionMustBeUsed() { - final String[] nodes = {"0001", "0002", "0003"}; - parseCommand("--banned-node-ids", String.join(",", nodes)); - - verify(mockRunnerBuilder).bannedNodeIds(stringListArgumentCaptor.capture()); + final BytesValue[] nodes = { + BytesValue.fromHexString( + "6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"), + BytesValue.fromHexString( + "7f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0"), + BytesValue.fromHexString( + "0x8f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0") + }; + + final String nodeIdsArg = + Arrays.asList(nodes).stream() + .map(BytesValue::toUnprefixedString) + .collect(Collectors.joining(",")); + parseCommand("--banned-node-ids", nodeIdsArg); + + verify(mockRunnerBuilder).bannedNodeIds(bytesValueCollectionCollector.capture()); verify(mockRunnerBuilder).build(); - assertThat(stringListArgumentCaptor.getValue().toArray()).isEqualTo(nodes); + assertThat(bytesValueCollectionCollector.getValue().toArray()).isEqualTo(nodes); assertThat(commandOutput.toString()).isEmpty(); assertThat(commandErrorOutput.toString()).isEmpty(); } + @Test + public void callingWithBannedNodeidsOptionButNoValueMustDisplayErrorAndUsage() { + parseCommand("--banned-node-ids"); + assertThat(commandOutput.toString()).isEmpty(); + final String expectedErrorOutputStart = + "Missing required parameter for option '--banned-node-ids' at index 0 ()"; + assertThat(commandErrorOutput.toString()).startsWith(expectedErrorOutputStart); + } + + @Test + public void callingWithBannedNodeidsOptionWithInvalidValuesMustDisplayErrorAndUsage() { + parseCommand("--banned-node-ids", "0x10,20,30"); + assertThat(commandOutput.toString()).isEmpty(); + final String expectedErrorOutputStart = + "Invalid ids supplied to '--banned-node-ids'. Expected 64 bytes in 0x10"; + assertThat(commandErrorOutput.toString()).startsWith(expectedErrorOutputStart); + } + @Test public void p2pHostAndPortOptionMustBeUsed() { From 6d63cd3485fe6bf5d63ff586486130281e7ea8b7 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Tue, 14 May 2019 17:14:03 -0400 Subject: [PATCH 12/19] Connect to peers that have not been recently contacted --- .../ethereum/p2p/discovery/DiscoveryPeer.java | 9 +++++++++ .../p2p/network/DefaultP2PNetwork.java | 8 ++++++-- .../p2p/network/DefaultP2PNetworkTest.java | 19 +++++++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/DiscoveryPeer.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/DiscoveryPeer.java index 148644d472..8185449b64 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/DiscoveryPeer.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/DiscoveryPeer.java @@ -31,6 +31,7 @@ public class DiscoveryPeer extends DefaultPeer { private long firstDiscovered = 0; private long lastContacted = 0; private long lastSeen = 0; + private long lastAttemptedConnection = 0; private DiscoveryPeer(final EnodeURL enode, final Endpoint endpoint) { super(enode); @@ -88,6 +89,14 @@ public void setLastContacted(final long lastContacted) { this.lastContacted = lastContacted; } + public long getLastAttemptedConnection() { + return lastAttemptedConnection; + } + + public void setLastAttemptedConnection(final long lastAttemptedConnection) { + this.lastAttemptedConnection = lastAttemptedConnection; + } + public long getLastSeen() { return lastSeen; } diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java index a6b3ca1934..8b73e68a43 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java @@ -53,7 +53,7 @@ import java.net.InetSocketAddress; import java.util.Arrays; import java.util.Collection; -import java.util.Collections; +import java.util.Comparator; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -411,8 +411,9 @@ void attemptPeerConnections() { streamDiscoveredPeers() .filter(peer -> peer.getStatus() == PeerDiscoveryStatus.BONDED) .filter(peer -> !isConnected(peer) && !isConnecting(peer)) + .sorted(Comparator.comparing(DiscoveryPeer::getLastAttemptedConnection)) .collect(Collectors.toList()); - Collections.shuffle(peers); + if (peers.size() == 0) { return; } @@ -463,6 +464,9 @@ public CompletableFuture connect(final Peer peer) { return connectionFuture; } + if (peer instanceof DiscoveryPeer) { + ((DiscoveryPeer) peer).setLastAttemptedConnection(System.currentTimeMillis()); + } new Bootstrap() .group(workers) .channel(NioSocketChannel.class) diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetworkTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetworkTest.java index 74d6e467b8..c14fae398d 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetworkTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetworkTest.java @@ -473,6 +473,11 @@ public void attemptPeerConnections_withSlotsAvailable() { }) .collect(Collectors.toList()); + final List highestValuePeers = peers.subList(5, 8); + // Mark as high value by lowering the lastAttemptedConnection value + peers.forEach(p -> p.setLastAttemptedConnection(100)); + highestValuePeers.forEach(p -> p.setLastAttemptedConnection(1)); + doReturn(peers.stream()).when(network).streamDiscoveredPeers(); final ArgumentCaptor peerCapture = ArgumentCaptor.forClass(DiscoveryPeer.class); doReturn(CompletableFuture.completedFuture(mock(PeerConnection.class))) @@ -482,6 +487,7 @@ public void attemptPeerConnections_withSlotsAvailable() { network.attemptPeerConnections(); verify(network, times(3)).connect(any()); assertThat(peers.containsAll(peerCapture.getAllValues())).isTrue(); + assertThat(peerCapture.getAllValues()).containsExactlyInAnyOrderElementsOf(highestValuePeers); } @Test @@ -541,6 +547,19 @@ public void connect_toNonListeningPeer() { "Attempt to connect to peer with no listening port: " + peer.getEnodeURLString()); } + @Test + public void connect_toDiscoveryPeerUpdatesStats() { + final DefaultP2PNetwork network = network(); + network.start(); + final DiscoveryPeer peer = createDiscoveryPeer(); + + assertThat(peer.getLastAttemptedConnection()).isEqualTo(0); + + final CompletableFuture result = network.connect(peer); + assertThat(result).isNotCompletedExceptionally(); + assertThat(peer.getLastAttemptedConnection()).isGreaterThan(0); + } + private DiscoveryPeer createDiscoveryPeer() { return createDiscoveryPeer(Peer.randomId(), 999); } From 650c05b4e919782cdeee99873124e9f746b901a1 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Tue, 14 May 2019 18:00:38 -0400 Subject: [PATCH 13/19] Fix PantheonCommandTest by using valid node id --- .../java/tech/pegasys/pantheon/cli/PantheonCommandTest.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java b/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java index 8bc3694023..567e486a95 100644 --- a/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java +++ b/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java @@ -882,7 +882,9 @@ public void p2pEnabledOptionValueFalseMustBeUsed() { @Test public void p2pOptionsRequiresServiceToBeEnabled() { - final String[] nodes = {"0001", "0002", "0003"}; + final String[] nodes = { + "6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0" + }; parseCommand( "--p2p-enabled", From ab9462874d006f2a98f5fc072c08b2f7862788ee Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Tue, 14 May 2019 19:09:51 -0400 Subject: [PATCH 14/19] Check permissions at discovery layer considering directionality --- .../internal/PeerDiscoveryController.java | 18 ++++-- .../internal/RecursivePeerRefreshState.java | 28 +++------ .../RecursivePeerRefreshStateTest.java | 61 ++----------------- 3 files changed, 28 insertions(+), 79 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java index 596e019043..fc896853ea 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java @@ -201,18 +201,19 @@ public void start() { } final List initialDiscoveryPeers = - bootstrapNodes.stream().filter(this::isPeerPermitted).collect(Collectors.toList()); + bootstrapNodes.stream() + .filter(this::isPeerPermittedToReceiveMessages) + .collect(Collectors.toList()); initialDiscoveryPeers.stream().forEach(peerTable::tryAdd); recursivePeerRefreshState = new RecursivePeerRefreshState( - peerPermissions, - nodePermissioningController, this::bond, this::findNodes, timerUtil, localPeer, peerTable, + this::isPeerPermittedToReceiveMessages, PEER_REFRESH_ROUND_TIMEOUT_IN_SECONDS, 100); @@ -255,13 +256,20 @@ public CompletableFuture stop() { return CompletableFuture.completedFuture(null); } - private boolean isPeerPermitted(final Peer remotePeer) { + private boolean isPeerPermittedToReceiveMessages(final Peer remotePeer) { return peerPermissions.isPermitted(remotePeer) && nodePermissioningController .map(c -> c.isPermitted(localPeer.getEnodeURL(), remotePeer.getEnodeURL())) .orElse(true); } + private boolean isPeerPermittedToSendMessages(final Peer remotePeer) { + return peerPermissions.isPermitted(remotePeer) + && nodePermissioningController + .map(c -> c.isPermitted(remotePeer.getEnodeURL(), localPeer.getEnodeURL())) + .orElse(true); + } + private void handlePermissionsUpdate( final boolean addRestrictions, final Optional> affectedPeers) { if (!addRestrictions) { @@ -296,7 +304,7 @@ public void onMessage(final Packet packet, final DiscoveryPeer sender) { return; } - if (!isPeerPermitted(sender)) { + if (!isPeerPermittedToSendMessages(sender)) { LOG.trace("Dropping packet from disallowed peer ({})", sender.getEnodeURLString()); return; } diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java index d051394728..5666fe3315 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java @@ -16,8 +16,7 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.DiscoveryPeer; import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryStatus; -import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; -import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningController; +import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; import tech.pegasys.pantheon.util.bytes.BytesValue; import java.util.List; @@ -38,8 +37,7 @@ public class RecursivePeerRefreshState { private static final Logger LOG = LogManager.getLogger(); private static final int MAX_CONCURRENT_REQUESTS = 3; private BytesValue target; - private final PeerPermissions peerPermissions; - private final Optional nodePermissioningController; + private final OutgoingPeerPermissions peerPermissions; private final PeerTable peerTable; private final DiscoveryPeer localPeer; @@ -58,22 +56,20 @@ public class RecursivePeerRefreshState { List initialPeers; RecursivePeerRefreshState( - final PeerPermissions peerPermissions, - final Optional nodePermissioningController, final BondingAgent bondingAgent, final FindNeighbourDispatcher neighborFinder, final TimerUtil timerUtil, final DiscoveryPeer localPeer, final PeerTable peerTable, + final OutgoingPeerPermissions peerPermissions, final int timeoutPeriodInSeconds, final int maxRounds) { - this.peerPermissions = peerPermissions; - this.nodePermissioningController = nodePermissioningController; this.bondingAgent = bondingAgent; this.findNeighbourDispatcher = neighborFinder; this.timerUtil = timerUtil; this.localPeer = localPeer; this.peerTable = peerTable; + this.peerPermissions = peerPermissions; this.timeoutPeriodInSeconds = timeoutPeriodInSeconds; this.maxRounds = maxRounds; } @@ -186,20 +182,11 @@ private void neighboursCancelOutstandingRequests() { private boolean satisfiesMapAdditionCriteria(final DiscoveryPeer discoPeer) { return !oneTrueMap.containsKey(discoPeer.getId()) - && isPeerPermitted(discoPeer) + && peerPermissions.isPermitted(discoPeer) && (initialPeers.contains(discoPeer) || !peerTable.get(discoPeer).isPresent()) && !discoPeer.getId().equals(localPeer.getId()); } - private Boolean isPeerPermitted(final DiscoveryPeer discoPeer) { - return peerPermissions.isPermitted(discoPeer) - && nodePermissioningController - .map( - controller -> - controller.isPermitted(localPeer.getEnodeURL(), discoPeer.getEnodeURL())) - .orElse(true); - } - void onNeighboursReceived(final DiscoveryPeer peer, final List peers) { final MetadataPeer metadataPeer = oneTrueMap.get(peer.getId()); if (metadataPeer == null) { @@ -392,4 +379,9 @@ public void cancelTimeout() { timeoutCancelled.set(true); } } + + @FunctionalInterface + public interface OutgoingPeerPermissions { + boolean isPermitted(Peer remotePeer); + } } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshStateTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshStateTest.java index c79e7314cf..6f4423ff07 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshStateTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshStateTest.java @@ -16,7 +16,6 @@ import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -28,24 +27,19 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryStatus; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.RecursivePeerRefreshState.BondingAgent; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.RecursivePeerRefreshState.FindNeighbourDispatcher; -import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; -import tech.pegasys.pantheon.ethereum.permissioning.LocalPermissioningConfiguration; -import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningController; +import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.RecursivePeerRefreshState.OutgoingPeerPermissions; import tech.pegasys.pantheon.util.bytes.BytesValue; import tech.pegasys.pantheon.util.enode.EnodeURL; -import java.nio.file.Files; -import java.nio.file.Path; import java.util.Collections; import java.util.List; -import java.util.Optional; import org.junit.Before; import org.junit.Test; public class RecursivePeerRefreshStateTest { private static final BytesValue TARGET = createId(0); - private final PeerPermissions peerPermissions = mock(PeerPermissions.class); + private final OutgoingPeerPermissions peerPermissions = mock(OutgoingPeerPermissions.class); private final BondingAgent bondingAgent = mock(BondingAgent.class); private final FindNeighbourDispatcher neighborFinder = mock(FindNeighbourDispatcher.class); private final MockTimerUtil timerUtil = new MockTimerUtil(); @@ -58,13 +52,12 @@ public class RecursivePeerRefreshStateTest { private RecursivePeerRefreshState recursivePeerRefreshState = new RecursivePeerRefreshState( - peerPermissions, - Optional.empty(), bondingAgent, neighborFinder, timerUtil, localPeer, new PeerTable(createId(999), 16), + peerPermissions, 5, 100); @@ -178,13 +171,12 @@ public void shouldStopWhenAllNodesHaveBeenQueried() { public void shouldStopWhenMaximumNumberOfRoundsReached() { recursivePeerRefreshState = new RecursivePeerRefreshState( - peerPermissions, - Optional.empty(), bondingAgent, neighborFinder, timerUtil, localPeer, new PeerTable(createId(999), 16), + peerPermissions, 5, 1); @@ -446,13 +438,12 @@ public void shouldNotBondWithNonPermittedNode() { recursivePeerRefreshState = new RecursivePeerRefreshState( - peerPermissions, - Optional.empty(), bondingAgent, neighborFinder, timerUtil, localPeer, new PeerTable(createId(999), 16), + peerPermissions, 5, 100); recursivePeerRefreshState.start(singletonList(peerA), TARGET); @@ -487,48 +478,6 @@ public void shouldNotBondWithSelf() { verify(bondingAgent, never()).performBonding(localPeer); } - @Test - public void shouldNotBondWithNodesNotPermitted() throws Exception { - final DiscoveryPeer localPeer = createPeer(999, "127.0.0.9", 9, 9); - final DiscoveryPeer peerA = createPeer(1, "127.0.0.1", 1, 1); - final DiscoveryPeer peerB = createPeer(2, "127.0.0.2", 2, 2); - - final Path tempFile = Files.createTempFile("test", "test"); - tempFile.toFile().deleteOnExit(); - final LocalPermissioningConfiguration permissioningConfiguration = - LocalPermissioningConfiguration.createDefault(); - permissioningConfiguration.setNodePermissioningConfigFilePath( - tempFile.toAbsolutePath().toString()); - - final NodePermissioningController nodeWhitelistController = - mock(NodePermissioningController.class); - when(nodeWhitelistController.isPermitted(any(), eq(peerA.getEnodeURL()))).thenReturn(true); - when(nodeWhitelistController.isPermitted(any(), eq(peerB.getEnodeURL()))).thenReturn(false); - - recursivePeerRefreshState = - new RecursivePeerRefreshState( - peerPermissions, - Optional.of(nodeWhitelistController), - bondingAgent, - neighborFinder, - timerUtil, - localPeer, - new PeerTable(createId(999), 16), - 5, - 100); - recursivePeerRefreshState.start(singletonList(peerA), TARGET); - - verify(bondingAgent).performBonding(peerA); - - completeBonding(peerA); - - verify(neighborFinder).findNeighbours(peerA, TARGET); - - recursivePeerRefreshState.onNeighboursReceived(peerA, Collections.singletonList(peerB)); - - verify(bondingAgent, never()).performBonding(peerB); - } - private static BytesValue createId(final int id) { return BytesValue.fromHexString(String.format("%0128x", id)); } From 13033b28361f0e569ed02ae043ec9c95f1144073 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Wed, 15 May 2019 11:33:25 -0400 Subject: [PATCH 15/19] Rename interface for clarity --- .../p2p/discovery/internal/RecursivePeerRefreshState.java | 6 +++--- .../discovery/internal/RecursivePeerRefreshStateTest.java | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java index 5666fe3315..15c02c882a 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java @@ -37,7 +37,7 @@ public class RecursivePeerRefreshState { private static final Logger LOG = LogManager.getLogger(); private static final int MAX_CONCURRENT_REQUESTS = 3; private BytesValue target; - private final OutgoingPeerPermissions peerPermissions; + private final OutboundDiscoveryMessagingPermissions peerPermissions; private final PeerTable peerTable; private final DiscoveryPeer localPeer; @@ -61,7 +61,7 @@ public class RecursivePeerRefreshState { final TimerUtil timerUtil, final DiscoveryPeer localPeer, final PeerTable peerTable, - final OutgoingPeerPermissions peerPermissions, + final OutboundDiscoveryMessagingPermissions peerPermissions, final int timeoutPeriodInSeconds, final int maxRounds) { this.bondingAgent = bondingAgent; @@ -381,7 +381,7 @@ public void cancelTimeout() { } @FunctionalInterface - public interface OutgoingPeerPermissions { + public interface OutboundDiscoveryMessagingPermissions { boolean isPermitted(Peer remotePeer); } } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshStateTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshStateTest.java index 6f4423ff07..35fb727fea 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshStateTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshStateTest.java @@ -27,7 +27,7 @@ import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryStatus; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.RecursivePeerRefreshState.BondingAgent; import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.RecursivePeerRefreshState.FindNeighbourDispatcher; -import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.RecursivePeerRefreshState.OutgoingPeerPermissions; +import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.RecursivePeerRefreshState.OutboundDiscoveryMessagingPermissions; import tech.pegasys.pantheon.util.bytes.BytesValue; import tech.pegasys.pantheon.util.enode.EnodeURL; @@ -39,7 +39,8 @@ public class RecursivePeerRefreshStateTest { private static final BytesValue TARGET = createId(0); - private final OutgoingPeerPermissions peerPermissions = mock(OutgoingPeerPermissions.class); + private final OutboundDiscoveryMessagingPermissions peerPermissions = + mock(OutboundDiscoveryMessagingPermissions.class); private final BondingAgent bondingAgent = mock(BondingAgent.class); private final FindNeighbourDispatcher neighborFinder = mock(FindNeighbourDispatcher.class); private final MockTimerUtil timerUtil = new MockTimerUtil(); From 0bfb1683ce6b366f791337b473b0f4ff02e7a9e7 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Wed, 15 May 2019 11:33:57 -0400 Subject: [PATCH 16/19] Validate peers before initiating connection --- .../p2p/network/DefaultP2PNetwork.java | 1 + .../p2p/network/DefaultP2PNetworkTest.java | 49 +++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java index 8b73e68a43..7206b49e7e 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetwork.java @@ -410,6 +410,7 @@ void attemptPeerConnections() { final List peers = streamDiscoveredPeers() .filter(peer -> peer.getStatus() == PeerDiscoveryStatus.BONDED) + .filter(this::isPeerAllowed) .filter(peer -> !isConnected(peer) && !isConnecting(peer)) .sorted(Comparator.comparing(DiscoveryPeer::getLastAttemptedConnection)) .collect(Collectors.toList()); diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetworkTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetworkTest.java index c14fae398d..6526b70b56 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetworkTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/network/DefaultP2PNetworkTest.java @@ -389,6 +389,7 @@ public void attemptPeerConnections_connectsToValidPeer() { final int maxPeers = 5; final DefaultP2PNetwork network = mockNetwork(() -> RlpxConfiguration.create().setMaxPeers(maxPeers)); + network.start(); doReturn(2).when(network).connectionCount(); DiscoveryPeer peer = createDiscoveryPeer(); @@ -410,6 +411,7 @@ public void attemptPeerConnections_ignoresUnbondedPeer() { final int maxPeers = 5; final DefaultP2PNetwork network = mockNetwork(() -> RlpxConfiguration.create().setMaxPeers(maxPeers)); + network.start(); doReturn(2).when(network).connectionCount(); DiscoveryPeer peer = createDiscoveryPeer(); @@ -426,6 +428,7 @@ public void attemptPeerConnections_ignoresConnectingPeer() { final int maxPeers = 5; final DefaultP2PNetwork network = mockNetwork(() -> RlpxConfiguration.create().setMaxPeers(maxPeers)); + network.start(); doReturn(2).when(network).connectionCount(); DiscoveryPeer peer = createDiscoveryPeer(); @@ -443,6 +446,7 @@ public void attemptPeerConnections_ignoresConnectedPeer() { final int maxPeers = 5; final DefaultP2PNetwork network = mockNetwork(() -> RlpxConfiguration.create().setMaxPeers(maxPeers)); + network.start(); doReturn(2).when(network).connectionCount(); DiscoveryPeer peer = createDiscoveryPeer(); @@ -460,6 +464,7 @@ public void attemptPeerConnections_withSlotsAvailable() { final int maxPeers = 5; final DefaultP2PNetwork network = mockNetwork(() -> RlpxConfiguration.create().setMaxPeers(maxPeers)); + network.start(); doReturn(2).when(network).connectionCount(); final List peers = @@ -490,11 +495,55 @@ public void attemptPeerConnections_withSlotsAvailable() { assertThat(peerCapture.getAllValues()).containsExactlyInAnyOrderElementsOf(highestValuePeers); } + @Test + public void attemptPeerConnections_withNonPermittedPeers() { + final int maxPeers = 5; + final DefaultP2PNetwork network = + mockNetwork(() -> RlpxConfiguration.create().setMaxPeers(maxPeers)); + network.start(); + + doReturn(2).when(network).connectionCount(); + final List peers = + Stream.iterate(1, n -> n + 1) + .limit(10) + .map( + (seed) -> { + DiscoveryPeer peer = createDiscoveryPeer(); + peer.setStatus(PeerDiscoveryStatus.BONDED); + return peer; + }) + .collect(Collectors.toList()); + + // Prioritize peers + final List highestValuePeers = peers.subList(5, 8); + peers.forEach(p -> p.setLastAttemptedConnection(100)); + highestValuePeers.forEach(p -> p.setLastAttemptedConnection(2)); + // Set up the highest value peer to lack permissions + DiscoveryPeer highestValueNonPermittedPeer = peers.get(0); + highestValueNonPermittedPeer.setLastAttemptedConnection(1); + when(nodePermissioningController.isPermitted(any(), any())).thenReturn(true); + when(nodePermissioningController.isPermitted( + any(), eq(highestValueNonPermittedPeer.getEnodeURL()))) + .thenReturn(false); + + doReturn(peers.stream()).when(network).streamDiscoveredPeers(); + final ArgumentCaptor peerCapture = ArgumentCaptor.forClass(DiscoveryPeer.class); + doReturn(CompletableFuture.completedFuture(mock(PeerConnection.class))) + .when(network) + .connect(peerCapture.capture()); + + network.attemptPeerConnections(); + verify(network, times(3)).connect(any()); + assertThat(peers.containsAll(peerCapture.getAllValues())).isTrue(); + assertThat(peerCapture.getAllValues()).containsExactlyInAnyOrderElementsOf(highestValuePeers); + } + @Test public void attemptPeerConnections_withNoSlotsAvailable() { final int maxPeers = 5; final DefaultP2PNetwork network = mockNetwork(() -> RlpxConfiguration.create().setMaxPeers(maxPeers)); + network.start(); doReturn(maxPeers).when(network).connectionCount(); final List peers = From 8740f74374dd3c258d2bac725ec77efc45ecdbdc Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Wed, 15 May 2019 11:57:44 -0400 Subject: [PATCH 17/19] Clean up PeerPermissions utils --- .../p2p/permissions/PeerPermissions.java | 46 +++++++++++++++---- .../p2p/permissions/PeerPermissionsTest.java | 16 +++++++ 2 files changed, 53 insertions(+), 9 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java index 535bbbb0c6..3a9d322d86 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java @@ -18,26 +18,24 @@ import java.util.Arrays; import java.util.List; import java.util.Optional; +import java.util.stream.Collectors; +import java.util.stream.Stream; public abstract class PeerPermissions { private final Subscribers updateSubscribers = new Subscribers<>(); - public static PeerPermissions noop() { - return new PeerPermissions() { + public static final PeerPermissions NOOP = new NoopPeerPermissions(); - @Override - public boolean isPermitted(final Peer peer) { - return true; - } - }; + public static PeerPermissions noop() { + return NOOP; } public static PeerPermissions combine(final PeerPermissions... permissions) { - return new CombinedPeerPermissions(Arrays.asList(permissions)); + return combine(Arrays.asList(permissions)); } public static PeerPermissions combine(final List permissions) { - return new CombinedPeerPermissions(permissions); + return CombinedPeerPermissions.create(permissions); } /** @@ -55,6 +53,13 @@ protected void dispatchUpdate( updateSubscribers.forEach(s -> s.onUpdate(permissionsRestricted, affectedPeers)); } + private static class NoopPeerPermissions extends PeerPermissions { + @Override + public boolean isPermitted(final Peer peer) { + return true; + } + } + private static class CombinedPeerPermissions extends PeerPermissions { private final List permissions; @@ -62,6 +67,29 @@ private CombinedPeerPermissions(final List permissions) { this.permissions = permissions; } + public static PeerPermissions create(final List permissions) { + final List filteredPermissions = + permissions.stream() + .flatMap( + p -> { + if (p instanceof CombinedPeerPermissions) { + return ((CombinedPeerPermissions) p).permissions.stream(); + } else { + return Stream.of(p); + } + }) + .filter(p -> !(p instanceof NoopPeerPermissions)) + .collect(Collectors.toList()); + + if (filteredPermissions.size() == 0) { + return PeerPermissions.NOOP; + } else if (filteredPermissions.size() == 1) { + return filteredPermissions.get(0); + } else { + return new CombinedPeerPermissions(filteredPermissions); + } + } + @Override public void subscribeUpdate(final PermissionsUpdateCallback callback) { for (final PeerPermissions permission : permissions) { diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsTest.java index ebf2275463..74b7472323 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissionsTest.java @@ -77,6 +77,9 @@ public void subscribeUpdate_forCombinedPermissions() { public void isPermitted_forCombinedPermissions() { final PeerPermissions allowPeers = new TestPeerPermissions(true); final PeerPermissions disallowPeers = new TestPeerPermissions(false); + final PeerPermissions noop = PeerPermissions.NOOP; + final PeerPermissions combinedPermissive = PeerPermissions.combine(noop, allowPeers); + final PeerPermissions combinedRestrictive = PeerPermissions.combine(disallowPeers, allowPeers); Peer peer = DefaultPeer.fromEnodeURL( @@ -92,6 +95,19 @@ public void isPermitted_forCombinedPermissions() { assertThat(PeerPermissions.combine(disallowPeers, disallowPeers).isPermitted(peer)).isFalse(); assertThat(PeerPermissions.combine(allowPeers, disallowPeers).isPermitted(peer)).isFalse(); assertThat(PeerPermissions.combine(allowPeers, allowPeers).isPermitted(peer)).isTrue(); + + assertThat(PeerPermissions.combine(combinedPermissive, allowPeers).isPermitted(peer)).isTrue(); + assertThat(PeerPermissions.combine(combinedPermissive, disallowPeers).isPermitted(peer)) + .isFalse(); + assertThat(PeerPermissions.combine(combinedRestrictive, allowPeers).isPermitted(peer)) + .isFalse(); + assertThat(PeerPermissions.combine(combinedRestrictive, disallowPeers).isPermitted(peer)) + .isFalse(); + assertThat(PeerPermissions.combine(combinedRestrictive).isPermitted(peer)).isFalse(); + assertThat(PeerPermissions.combine(combinedPermissive).isPermitted(peer)).isTrue(); + + assertThat(PeerPermissions.combine(noop).isPermitted(peer)).isTrue(); + assertThat(PeerPermissions.combine().isPermitted(peer)).isTrue(); } private static class TestPeerPermissions extends PeerPermissions { From 69f8d0aa9dcc6a745c781875f4ca7f7dcbf46b70 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Fri, 17 May 2019 12:05:19 -0400 Subject: [PATCH 18/19] Use ImmutableList --- .../ethereum/p2p/permissions/PeerPermissions.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java index 3a9d322d86..f728df2862 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/permissions/PeerPermissions.java @@ -18,9 +18,10 @@ import java.util.Arrays; import java.util.List; import java.util.Optional; -import java.util.stream.Collectors; import java.util.stream.Stream; +import com.google.common.collect.ImmutableList; + public abstract class PeerPermissions { private final Subscribers updateSubscribers = new Subscribers<>(); @@ -61,14 +62,14 @@ public boolean isPermitted(final Peer peer) { } private static class CombinedPeerPermissions extends PeerPermissions { - private final List permissions; + private final ImmutableList permissions; - private CombinedPeerPermissions(final List permissions) { + private CombinedPeerPermissions(final ImmutableList permissions) { this.permissions = permissions; } public static PeerPermissions create(final List permissions) { - final List filteredPermissions = + final ImmutableList filteredPermissions = permissions.stream() .flatMap( p -> { @@ -79,7 +80,7 @@ public static PeerPermissions create(final List permissions) { } }) .filter(p -> !(p instanceof NoopPeerPermissions)) - .collect(Collectors.toList()); + .collect(ImmutableList.toImmutableList()); if (filteredPermissions.size() == 0) { return PeerPermissions.NOOP; From e05836f3145bde5410d37778cb72341f2a6a5564 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Fri, 17 May 2019 12:13:26 -0400 Subject: [PATCH 19/19] Move utility method to EnodeURL --- .../pegasys/pantheon/cli/PantheonCommand.java | 17 +---------------- .../pegasys/pantheon/util/enode/EnodeURL.java | 11 +++++++++++ .../pantheon/util/enode/EnodeURLTest.java | 18 ++++++++++++++++++ 3 files changed, 30 insertions(+), 16 deletions(-) diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java b/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java index 13641bca25..9397a59077 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java @@ -12,7 +12,6 @@ */ package tech.pegasys.pantheon.cli; -import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.Arrays.asList; @@ -220,11 +219,7 @@ void setBannedNodeIds(final List values) { bannedNodeIds = values.stream() .filter(value -> !value.isEmpty()) - .map( - v -> { - validateNodeIdString(v); - return BytesValue.fromHexString(v, EnodeURL.NODE_ID_SIZE); - }) + .map(EnodeURL::parseNodeId) .collect(Collectors.toList()); } catch (final IllegalArgumentException e) { throw new ParameterException( @@ -232,16 +227,6 @@ void setBannedNodeIds(final List values) { } } - private static void validateNodeIdString(final String nodeId) { - int expectedSize = EnodeURL.NODE_ID_SIZE * 2; - if (nodeId.toLowerCase().startsWith("0x")) { - expectedSize += 2; - } - checkArgument( - nodeId.length() == expectedSize, - "Expected " + EnodeURL.NODE_ID_SIZE + " bytes in " + nodeId); - } - private Collection bannedNodeIds = new ArrayList<>(); @Option( diff --git a/util/src/main/java/tech/pegasys/pantheon/util/enode/EnodeURL.java b/util/src/main/java/tech/pegasys/pantheon/util/enode/EnodeURL.java index 2509676045..8461611dc5 100644 --- a/util/src/main/java/tech/pegasys/pantheon/util/enode/EnodeURL.java +++ b/util/src/main/java/tech/pegasys/pantheon/util/enode/EnodeURL.java @@ -133,6 +133,17 @@ public static boolean sameListeningEndpoint(final EnodeURL enodeA, final EnodeUR && Objects.equals(enodeA.listeningPort, enodeB.listeningPort); } + public static BytesValue parseNodeId(final String nodeId) { + int expectedSize = EnodeURL.NODE_ID_SIZE * 2; + if (nodeId.toLowerCase().startsWith("0x")) { + expectedSize += 2; + } + checkArgument( + nodeId.length() == expectedSize, + "Expected " + EnodeURL.NODE_ID_SIZE + " bytes in " + nodeId); + return BytesValue.fromHexString(nodeId, NODE_ID_SIZE); + } + public URI toURI() { final String uri = String.format( diff --git a/util/src/test/java/tech/pegasys/pantheon/util/enode/EnodeURLTest.java b/util/src/test/java/tech/pegasys/pantheon/util/enode/EnodeURLTest.java index e021a5bbf5..f3bb88910f 100644 --- a/util/src/test/java/tech/pegasys/pantheon/util/enode/EnodeURLTest.java +++ b/util/src/test/java/tech/pegasys/pantheon/util/enode/EnodeURLTest.java @@ -16,6 +16,8 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assertions.catchThrowable; +import tech.pegasys.pantheon.util.bytes.BytesValue; + import java.net.URI; import java.util.OptionalInt; @@ -724,4 +726,20 @@ public void sameListeningEndpoint_listeningDisabledForBoth() { assertThat(EnodeURL.sameListeningEndpoint(enodeA, enodeB)).isTrue(); } + + @Test + public void parseNodeId_invalid() { + final String invalidId = "0x10"; + assertThatThrownBy(() -> EnodeURL.parseNodeId(invalidId)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Expected 64 bytes in " + invalidId); + } + + @Test + public void parseNodeId_valid() { + final String validId = VALID_NODE_ID; + final BytesValue nodeId = EnodeURL.parseNodeId(validId); + assertThat(nodeId.size()).isEqualTo(EnodeURL.NODE_ID_SIZE); + assertThat(nodeId.toUnprefixedString()).isEqualTo(validId); + } }