From 45fc6b7d786b101557fc4926b86799cf79008f5a Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Mon, 29 Apr 2019 22:02:14 -0400 Subject: [PATCH 01/12] Create a LocalNode class for holding information about the local node --- .../pegasys/pantheon/util/Subscribers.java | 35 ++++++++ .../pantheon/util/enode/DefaultLocalNode.java | 65 ++++++++++++++ .../pantheon/util/enode/LocalNode.java | 54 ++++++++++++ .../pantheon/util/enode/MutableLocalNode.java | 27 ++++++ .../pantheon/util/SubscribersTest.java | 15 ++++ .../util/enode/DefaultLocalNodeTest.java | 88 +++++++++++++++++++ 6 files changed, 284 insertions(+) create mode 100644 util/src/main/java/tech/pegasys/pantheon/util/enode/DefaultLocalNode.java create mode 100644 util/src/main/java/tech/pegasys/pantheon/util/enode/LocalNode.java create mode 100644 util/src/main/java/tech/pegasys/pantheon/util/enode/MutableLocalNode.java create mode 100644 util/src/test/java/tech/pegasys/pantheon/util/enode/DefaultLocalNodeTest.java diff --git a/util/src/main/java/tech/pegasys/pantheon/util/Subscribers.java b/util/src/main/java/tech/pegasys/pantheon/util/Subscribers.java index e06a815f70..a984da39fe 100644 --- a/util/src/main/java/tech/pegasys/pantheon/util/Subscribers.java +++ b/util/src/main/java/tech/pegasys/pantheon/util/Subscribers.java @@ -39,9 +39,16 @@ */ public class Subscribers { + public static Subscribers NONE = new EmptySubscribers<>(); + private final AtomicLong subscriberId = new AtomicLong(); private final Map subscribers = new ConcurrentHashMap<>(); + @SuppressWarnings("unchecked") + public static Subscribers none() { + return (Subscribers) NONE; + } + /** * Add a subscriber to the list. * @@ -86,4 +93,32 @@ public void forEach(final Consumer action) { public int getSubscriberCount() { return subscribers.size(); } + + /** Remove all subscribers */ + public void clear() { + subscribers.clear(); + } + + private static class EmptySubscribers extends Subscribers { + + @Override + public long subscribe(final T subscriber) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean unsubscribe(final long subscriberId) { + return false; + } + + @Override + public void forEach(final Consumer action) { + return; + } + + @Override + public int getSubscriberCount() { + return 0; + } + } } diff --git a/util/src/main/java/tech/pegasys/pantheon/util/enode/DefaultLocalNode.java b/util/src/main/java/tech/pegasys/pantheon/util/enode/DefaultLocalNode.java new file mode 100644 index 0000000000..73cc73923b --- /dev/null +++ b/util/src/main/java/tech/pegasys/pantheon/util/enode/DefaultLocalNode.java @@ -0,0 +1,65 @@ +/* + * 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.enode; + +import tech.pegasys.pantheon.util.Subscribers; + +import java.util.Optional; + +class DefaultLocalNode implements MutableLocalNode { + + private Optional enode = Optional.empty(); + private Subscribers readySubscribers = new Subscribers<>(); + + private DefaultLocalNode() {} + + public static DefaultLocalNode create() { + return new DefaultLocalNode(); + } + + @Override + public void setEnode(final EnodeURL enode) throws NodeAlreadySetException { + if (this.enode.isPresent()) { + throw new NodeAlreadySetException("Attempt to set already initialized local node"); + } + this.enode = Optional.of(enode); + dispatchReady(enode); + } + + @Override + public EnodeURL getEnode() throws NodeNotReadyException { + if (!enode.isPresent()) { + throw new NodeNotReadyException("Attempt to access local enode before local node is ready."); + } + return enode.get(); + } + + @Override + public boolean isReady() { + return enode.isPresent(); + } + + @Override + public synchronized void subscribeReady(final ReadyCallback callback) { + if (isReady()) { + callback.onReady(enode.get()); + } else { + readySubscribers.subscribe(callback); + } + } + + private synchronized void dispatchReady(final EnodeURL localNode) { + readySubscribers.forEach(c -> c.onReady(localNode)); + readySubscribers.clear(); + } +} diff --git a/util/src/main/java/tech/pegasys/pantheon/util/enode/LocalNode.java b/util/src/main/java/tech/pegasys/pantheon/util/enode/LocalNode.java new file mode 100644 index 0000000000..936f7902f1 --- /dev/null +++ b/util/src/main/java/tech/pegasys/pantheon/util/enode/LocalNode.java @@ -0,0 +1,54 @@ +/* + * 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.enode; + +public interface LocalNode { + + static LocalNode create() { + return DefaultLocalNode.create(); + } + + /** + * While node is initializing, an empty value will be returned. Once this node is up and running, + * a {@link EnodeURL} object corresponding to this node will be returned. + * + * @return The {@link EnodeURL} representation associated with this node. + */ + EnodeURL getEnode() throws NodeNotReadyException; + + /** + * @return True if the local node is up and running and has an available {@link EnodeURL} + * representation. + */ + boolean isReady(); + + /** + * When this node is up and running with a valid {@link EnodeURL} representation, the given + * callback will be invoked. If the callback is added after this node is ready, it is invoked + * immediately. + * + * @param callback The callback to run against the {@link EnodeURL} representing the local node, + * when the local node is ready. + */ + void subscribeReady(ReadyCallback callback); + + interface ReadyCallback { + void onReady(EnodeURL localNode); + } + + class NodeNotReadyException extends RuntimeException { + public NodeNotReadyException(final String message) { + super(message); + } + } +} diff --git a/util/src/main/java/tech/pegasys/pantheon/util/enode/MutableLocalNode.java b/util/src/main/java/tech/pegasys/pantheon/util/enode/MutableLocalNode.java new file mode 100644 index 0000000000..0f2af0fea2 --- /dev/null +++ b/util/src/main/java/tech/pegasys/pantheon/util/enode/MutableLocalNode.java @@ -0,0 +1,27 @@ +/* + * 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.enode; + +public interface MutableLocalNode extends LocalNode { + static MutableLocalNode create() { + return DefaultLocalNode.create(); + } + + void setEnode(EnodeURL enode) throws NodeAlreadySetException; + + class NodeAlreadySetException extends RuntimeException { + public NodeAlreadySetException(final String message) { + super(message); + } + } +} diff --git a/util/src/test/java/tech/pegasys/pantheon/util/SubscribersTest.java b/util/src/test/java/tech/pegasys/pantheon/util/SubscribersTest.java index fa70ee0dd3..b1105eef3d 100644 --- a/util/src/test/java/tech/pegasys/pantheon/util/SubscribersTest.java +++ b/util/src/test/java/tech/pegasys/pantheon/util/SubscribersTest.java @@ -46,6 +46,21 @@ public void shouldRemoveSubscriber() { verify(subscriber2).run(); } + @Test + public void shouldClearSubscriber() { + subscribers.subscribe(subscriber1); + subscribers.subscribe(subscriber2); + assertThat(subscribers.getSubscriberCount()).isEqualTo(2); + + subscribers.clear(); + + assertThat(subscribers.getSubscriberCount()).isEqualTo(0); + + subscribers.forEach(Runnable::run); + verifyZeroInteractions(subscriber1); + verifyZeroInteractions(subscriber2); + } + @Test public void shouldTrackMultipleSubscribers() { final Runnable subscriber3 = mock(Runnable.class); diff --git a/util/src/test/java/tech/pegasys/pantheon/util/enode/DefaultLocalNodeTest.java b/util/src/test/java/tech/pegasys/pantheon/util/enode/DefaultLocalNodeTest.java new file mode 100644 index 0000000000..51c4409845 --- /dev/null +++ b/util/src/test/java/tech/pegasys/pantheon/util/enode/DefaultLocalNodeTest.java @@ -0,0 +1,88 @@ +/* + * 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.enode; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Java6Assertions.assertThatThrownBy; + +import tech.pegasys.pantheon.util.bytes.BytesValue; +import tech.pegasys.pantheon.util.enode.LocalNode.NodeNotReadyException; + +import java.util.concurrent.atomic.AtomicReference; + +import org.junit.Test; + +public class DefaultLocalNodeTest { + private final EnodeURL enode = + EnodeURL.builder() + .ipAddress("127.0.0.1") + .listeningPort(30303) + .nodeId(BytesValue.of(new byte[64])) + .build(); + + @Test + public void create() { + final LocalNode localNode = DefaultLocalNode.create(); + assertThat(localNode.isReady()).isFalse(); + assertThatThrownBy(localNode::getEnode).isInstanceOf(NodeNotReadyException.class); + } + + @Test + public void setEnode() { + final MutableLocalNode localNode = DefaultLocalNode.create(); + localNode.setEnode(enode); + + assertThat(localNode.isReady()).isTrue(); + final EnodeURL enodeValue = localNode.getEnode(); + assertThat(enodeValue).isEqualTo(enode); + } + + @Test + public void subscribeReady_beforeReady() { + AtomicReference localEnode = new AtomicReference<>(null); + final MutableLocalNode localNode = DefaultLocalNode.create(); + localNode.subscribeReady(localEnode::set); + + assertThat(localEnode.get()).isNull(); + + localNode.setEnode(enode); + assertThat(localEnode.get()).isEqualTo(enode); + } + + @Test + public void subscribeReady_afterReady() { + AtomicReference localEnode = new AtomicReference<>(null); + final MutableLocalNode localNode = DefaultLocalNode.create(); + localNode.setEnode(enode); + + localNode.subscribeReady(localEnode::set); + assertThat(localEnode.get()).isEqualTo(enode); + } + + @Test + public void subscribeReady_beforeAndAfterReady() { + final MutableLocalNode localNode = DefaultLocalNode.create(); + + AtomicReference subscriberA = new AtomicReference<>(null); + AtomicReference subscriberB = new AtomicReference<>(null); + + localNode.subscribeReady(subscriberA::set); + assertThat(subscriberA.get()).isNull(); + + localNode.setEnode(enode); + + localNode.subscribeReady(subscriberB::set); + assertThat(subscriberA.get()).isEqualTo(enode); + assertThat(subscriberB.get()).isEqualTo(enode); + } +} From 4f86e9a279551f9b0b307b6ca2b53007087242de Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Mon, 29 Apr 2019 22:18:34 -0400 Subject: [PATCH 02/12] Use LocalNode within DefaultP2PNetwork --- .../p2p/network/DefaultP2PNetwork.java | 95 +++++++++++-------- .../ethereum/p2p/peers/PeerBlacklist.java | 2 +- 2 files changed, 55 insertions(+), 42 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 f4ae8474e9..6ac0d277f4 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,6 +53,7 @@ import tech.pegasys.pantheon.util.Subscribers; import tech.pegasys.pantheon.util.bytes.BytesValue; import tech.pegasys.pantheon.util.enode.EnodeURL; +import tech.pegasys.pantheon.util.enode.MutableLocalNode; import java.net.InetSocketAddress; import java.util.Arrays; @@ -182,7 +183,7 @@ public class DefaultP2PNetwork implements P2PNetwork { private final String advertisedHost; - private volatile EnodeURL ourEnodeURL; + private final MutableLocalNode localNode; private final Optional nodePermissioningController; private final Optional blockchain; @@ -205,6 +206,7 @@ public class DefaultP2PNetwork implements P2PNetwork { * @param blockchain The blockchain to subscribe to BlockAddedEvents. */ DefaultP2PNetwork( + final MutableLocalNode localNode, final PeerDiscoveryAgent peerDiscoveryAgent, final SECP256K1.KeyPair keyPair, final NetworkingConfiguration config, @@ -214,6 +216,7 @@ public class DefaultP2PNetwork implements P2PNetwork { final Optional nodePermissioningController, final Blockchain blockchain) { + this.localNode = localNode; this.config = config; maxPeers = config.getRlpx().getMaxPeers(); connections = new PeerConnectionRegistry(metricsSystem); @@ -346,7 +349,7 @@ protected void initChannel(final SocketChannel ch) { return; } - if (!isPeerConnectionAllowed(connection)) { + if (!isPeerAllowed(connection)) { connection.disconnect(DisconnectReason.UNKNOWN); return; } @@ -549,8 +552,7 @@ public void start() { } } - this.ourEnodeURL = buildSelfEnodeURL(); - LOG.info("Enode URL {}", ourEnodeURL.toString()); + setLocalNode(); peerConnectionScheduler.scheduleWithFixedDelay( this::checkMaintainedConnectionPeers, 60, 60, TimeUnit.SECONDS); @@ -584,7 +586,7 @@ private synchronized void handleBlockAddedEvent( .getPeerConnections() .forEach( peerConnection -> { - if (!isPeerConnectionAllowed(peerConnection)) { + if (!isPeerAllowed(peerConnection)) { peerConnection.disconnect(DisconnectReason.REQUESTED); } }); @@ -595,38 +597,33 @@ private synchronized void checkCurrentConnections() { .getPeerConnections() .forEach( peerConnection -> { - if (!isPeerConnectionAllowed(peerConnection)) { + if (!isPeerAllowed(peerConnection)) { peerConnection.disconnect(DisconnectReason.REQUESTED); } }); } - private boolean isPeerConnectionAllowed(final PeerConnection peerConnection) { - if (peerBlacklist.contains(peerConnection)) { - return false; - } - - LOG.trace( - "Checking if connection with peer {} is permitted", - peerConnection.getPeerInfo().getNodeId()); - - return nodePermissioningController - .map( - c -> { - final EnodeURL localPeerEnodeURL = getLocalEnode().orElse(buildSelfEnodeURL()); - final EnodeURL remotePeerEnodeURL = peerConnection.getRemoteEnode(); - return c.isPermitted(localPeerEnodeURL, remotePeerEnodeURL); - }) - .orElse(true); + private boolean isPeerAllowed(final PeerConnection conn) { + return isPeerAllowed(conn.getRemoteEnode()); } private boolean isPeerAllowed(final Peer peer) { - if (peerBlacklist.contains(peer)) { + return isPeerAllowed(peer.getEnodeURL()); + } + + private boolean isPeerAllowed(final EnodeURL enode) { + if (peerBlacklist.contains(enode.getNodeId())) { + return false; + } + + Optional maybeEnode = getLocalEnode(); + if (!maybeEnode.isPresent()) { + // If local enode isn't yet available we can't evaluate permissions return false; } return nodePermissioningController - .map(c -> c.isPermitted(ourEnodeURL, peer.getEnodeURL())) + .map(c -> c.isPermitted(maybeEnode.get(), enode)) .orElse(true); } @@ -691,10 +688,13 @@ public boolean isDiscoveryEnabled() { @Override public Optional getLocalEnode() { - return Optional.ofNullable(ourEnodeURL); + if (!localNode.isReady()) { + return Optional.empty(); + } + return Optional.of(localNode.getEnode()); } - private EnodeURL buildSelfEnodeURL() { + private void setLocalNode() { final BytesValue nodeId = ourPeerInfo.getNodeId(); final int listeningPort = ourPeerInfo.getPort(); final OptionalInt discoveryPort = @@ -704,12 +704,16 @@ private EnodeURL buildSelfEnodeURL() { .filter(port -> port.getAsInt() != listeningPort) .orElse(OptionalInt.empty()); - return EnodeURL.builder() - .nodeId(nodeId) - .ipAddress(advertisedHost) - .listeningPort(listeningPort) - .discoveryPort(discoveryPort) - .build(); + final EnodeURL localEnode = + EnodeURL.builder() + .nodeId(nodeId) + .ipAddress(advertisedHost) + .listeningPort(listeningPort) + .discoveryPort(discoveryPort) + .build(); + + LOG.info("Enode URL {}", localEnode.toString()); + localNode.setEnode(localEnode); } private void onConnectionEstablished(final PeerConnection connection) { @@ -719,14 +723,15 @@ private void onConnectionEstablished(final PeerConnection connection) { public static class Builder { - protected PeerDiscoveryAgent peerDiscoveryAgent; - protected KeyPair keyPair; - protected NetworkingConfiguration config = NetworkingConfiguration.create(); - protected List supportedCapabilities; - protected PeerBlacklist peerBlacklist; - protected MetricsSystem metricsSystem; - protected Optional nodePermissioningController = Optional.empty(); - protected Blockchain blockchain = null; + private MutableLocalNode localNode = MutableLocalNode.create(); + private PeerDiscoveryAgent peerDiscoveryAgent; + private KeyPair keyPair; + private NetworkingConfiguration config = NetworkingConfiguration.create(); + private List supportedCapabilities; + private PeerBlacklist peerBlacklist; + private MetricsSystem metricsSystem; + private Optional nodePermissioningController = Optional.empty(); + private Blockchain blockchain = null; private Vertx vertx; private Optional nodeLocalConfigPermissioningController = Optional.empty(); @@ -740,6 +745,7 @@ private P2PNetwork doBuild() { peerDiscoveryAgent = peerDiscoveryAgent == null ? createDiscoveryAgent() : peerDiscoveryAgent; return new DefaultP2PNetwork( + localNode, peerDiscoveryAgent, keyPair, config, @@ -751,6 +757,7 @@ private P2PNetwork doBuild() { } private void validate() { + checkState(localNode != null, "LocalNode must be set."); checkState(keyPair != null, "KeyPair must be set."); checkState(config != null, "NetworkingConfiguration must be set."); checkState( @@ -779,6 +786,12 @@ private PeerDiscoveryAgent createDiscoveryAgent() { metricsSystem); } + public Builder localNode(final MutableLocalNode localNode) { + checkNotNull(localNode); + this.localNode = localNode; + return this; + } + public Builder vertx(final Vertx vertx) { checkNotNull(vertx); this.vertx = vertx; 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 index d997f1dae3..9a84be454f 100644 --- 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 @@ -79,7 +79,7 @@ public PeerBlacklist() { this(DEFAULT_BLACKLIST_CAP, Collections.emptySet()); } - private boolean contains(final BytesValue nodeId) { + public boolean contains(final BytesValue nodeId) { return blacklistedNodeIds.contains(nodeId) || bannedNodeIds.contains(nodeId); } From 19cd5f027b7cc7c5a17127dd5b74d03e323c1044 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Mon, 29 Apr 2019 20:33:26 -0400 Subject: [PATCH 03/12] Permissions check gates connection, not addition to maintained list Peers can be added to the maintained peer list before the network is fully started and permissions can be properly checked. So, for consistency, don't gate addition to the maintained peer list using permissions. Just don't connect to peers who are currently not allowed. --- .../pantheon/ethereum/p2p/api/P2PNetwork.java | 16 ++++---- .../p2p/network/DefaultP2PNetwork.java | 40 +++++++++---------- .../p2p/network/DefaultP2PNetworkTest.java | 32 ++++++++------- 3 files changed, 44 insertions(+), 44 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/api/P2PNetwork.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/api/P2PNetwork.java index 03250ae1fc..aee2c95e4f 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/api/P2PNetwork.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/api/P2PNetwork.java @@ -77,21 +77,23 @@ public interface P2PNetwork extends Closeable { void subscribeDisconnect(DisconnectCallback consumer); /** - * Adds a {@link Peer} to a list indicating efforts should be made to always stay connected to it + * Adds a {@link Peer} to a list indicating efforts should be made to always stay connected + * regardless of maxPeer limits. Non-permitted peers may be added to this list, but will not + * actually be connected to as long as they are prohibited. * * @param peer The peer that should be connected to - * @return boolean representing whether or not the peer has been added to the list or was already - * on it + * @return boolean representing whether or not the peer has been added to the list, false is + * returned if the peer was already on the list */ boolean addMaintainConnectionPeer(final Peer peer); /** - * Removes a {@link Peer} from a list indicating any existing efforts to connect to a given peer - * should be removed, and if connected, the peer should be disconnected + * Disconnect and remove the given {@link Peer} from the maintained peer list. Peer is + * disconnected even if it is not in the maintained peer list. See {@link + * #addMaintainConnectionPeer(Peer)} for details on the maintained peer list. * * @param peer The peer to which connections are not longer required - * @return boolean representing whether or not the peer has been disconnected, or if it was not - * currently connected. + * @return boolean representing whether the peer was removed from the maintained peer list */ boolean removeMaintainedConnectionPeer(final Peer peer); 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 6ac0d277f4..adaa865cc6 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 @@ -19,8 +19,6 @@ 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.ConnectingToLocalNodeException; -import tech.pegasys.pantheon.ethereum.p2p.PeerNotPermittedException; import tech.pegasys.pantheon.ethereum.p2p.api.DisconnectCallback; import tech.pegasys.pantheon.ethereum.p2p.api.Message; import tech.pegasys.pantheon.ethereum.p2p.api.P2PNetwork; @@ -365,21 +363,13 @@ protected void initChannel(final SocketChannel ch) { @Override public boolean addMaintainConnectionPeer(final Peer peer) { - if (!isPeerAllowed(peer)) { - throw new PeerNotPermittedException(); - } - - if (peer.getId().equals(ourPeerInfo.getNodeId())) { - throw new ConnectingToLocalNodeException(); - } - final boolean added = peerMaintainConnectionList.add(peer); - if (added) { + if (isPeerAllowed(peer) && !isConnectingOrConnected(peer)) { + // Connect immediately if appropriate connect(peer); - return true; - } else { - return false; } + + return added; } @Override @@ -397,12 +387,11 @@ public boolean removeMaintainedConnectionPeer(final Peer peer) { return removed; } - public void checkMaintainedConnectionPeers() { - for (final Peer peer : peerMaintainConnectionList) { - if (!(isConnecting(peer) || isConnected(peer))) { - connect(peer); - } - } + void checkMaintainedConnectionPeers() { + peerMaintainConnectionList.stream() + .filter(p -> !isConnectingOrConnected(p)) + .filter(this::isPeerAllowed) + .forEach(this::connect); } @VisibleForTesting @@ -555,7 +544,7 @@ public void start() { setLocalNode(); peerConnectionScheduler.scheduleWithFixedDelay( - this::checkMaintainedConnectionPeers, 60, 60, TimeUnit.SECONDS); + this::checkMaintainedConnectionPeers, 2, 60, TimeUnit.SECONDS); peerConnectionScheduler.scheduleWithFixedDelay( this::attemptPeerConnections, 30, 30, TimeUnit.SECONDS); } @@ -615,13 +604,16 @@ private boolean isPeerAllowed(final EnodeURL enode) { if (peerBlacklist.contains(enode.getNodeId())) { return false; } + if (enode.getNodeId().equals(ourPeerInfo.getNodeId())) { + // Peer matches our node id + return false; + } Optional maybeEnode = getLocalEnode(); if (!maybeEnode.isPresent()) { // If local enode isn't yet available we can't evaluate permissions return false; } - return nodePermissioningController .map(c -> c.isPermitted(maybeEnode.get(), enode)) .orElse(true); @@ -637,6 +629,10 @@ boolean isConnected(final Peer peer) { return connections.isAlreadyConnected(peer.getId()); } + private boolean isConnectingOrConnected(final Peer peer) { + return isConnected(peer) || isConnecting(peer); + } + @Override public void stop() { sendClientQuittingToPeers(); 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 7fc9e80e3a..48e19a7b84 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 @@ -17,6 +17,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; @@ -103,17 +104,19 @@ public void closeVertx() { @Test public void addingMaintainedNetworkPeerStartsConnection() { final DefaultP2PNetwork network = mockNetwork(); + network.start(); final Peer peer = mockPeer(); assertThat(network.addMaintainConnectionPeer(peer)).isTrue(); assertThat(network.peerMaintainConnectionList).contains(peer); - verify(network, times(1)).connect(peer); + verify(network, atLeast(1)).connect(peer); } @Test public void addingRepeatMaintainedPeersReturnsFalse() { final P2PNetwork network = network(); + network.start(); final Peer peer = mockPeer(); assertThat(network.addMaintainConnectionPeer(peer)).isTrue(); assertThat(network.addMaintainConnectionPeer(peer)).isFalse(); @@ -122,11 +125,13 @@ public void addingRepeatMaintainedPeersReturnsFalse() { @Test public void checkMaintainedConnectionPeersTriesToConnect() { final DefaultP2PNetwork network = mockNetwork(); + network.start(); + final Peer peer = mockPeer(); network.peerMaintainConnectionList.add(peer); network.checkMaintainedConnectionPeers(); - verify(network, times(1)).connect(peer); + verify(network, atLeast(1)).connect(peer); } @Test @@ -143,24 +148,21 @@ public void checkMaintainedConnectionPeersDoesntReconnectPendingPeers() { @Test public void checkMaintainedConnectionPeersDoesntReconnectConnectedPeers() { final DefaultP2PNetwork network = spy(network()); + network.start(); final Peer peer = mockPeer(); + + // Connect to Peer verify(network, never()).connect(peer); - assertThat(network.addMaintainConnectionPeer(peer)).isTrue(); + network.connect(peer); verify(network, times(1)).connect(peer); - { - final CompletableFuture connection; - connection = network.pendingConnections.remove(peer); - assertThat(connection).isNotNull(); - assertThat(connection.cancel(true)).isTrue(); - } + // Add peer to maintained list + assertThat(network.addMaintainConnectionPeer(peer)).isTrue(); + verify(network, times(1)).connect(peer); - { - final PeerConnection peerConnection = mockPeerConnection(peer.getId()); - network.connections.registerConnection(peerConnection); - network.checkMaintainedConnectionPeers(); - verify(network, times(1)).connect(peer); - } + // Check maintained connections + network.checkMaintainedConnectionPeers(); + verify(network, times(1)).connect(peer); } @Test From c97239a077e96c4de57263707611049a46bf5448 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Tue, 30 Apr 2019 14:05:29 -0400 Subject: [PATCH 04/12] P2PNetwork should only be started once --- .../eth/transactions/TestNodeList.java | 6 --- .../TransactionPoolPropagationTest.java | 1 - .../p2p/network/DefaultP2PNetwork.java | 49 +++++++++++-------- 3 files changed, 28 insertions(+), 28 deletions(-) diff --git a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TestNodeList.java b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TestNodeList.java index 60ebbb090c..372ff65566 100644 --- a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TestNodeList.java +++ b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TestNodeList.java @@ -59,12 +59,6 @@ public TestNode create( return node; } - public void startNetworks() { - for (final TestNode node : nodes) { - node.network.start(); - } - } - public void connectAndAssertAll() throws InterruptedException, ExecutionException, TimeoutException { for (int i = 0; i < nodes.size(); i++) { diff --git a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TransactionPoolPropagationTest.java b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TransactionPoolPropagationTest.java index db3989d57d..de8856cd14 100644 --- a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TransactionPoolPropagationTest.java +++ b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TransactionPoolPropagationTest.java @@ -47,7 +47,6 @@ public void tearDown() { /** Helper to do common setup tasks. */ private void initTest(final TestNodeList txNodes) throws Exception { - txNodes.startNetworks(); txNodes.connectAndAssertAll(); txNodes.logPeerConnections(); txNodes.assertPeerCounts(); 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 adaa865cc6..9e3ffa4e70 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 @@ -70,6 +70,7 @@ import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Consumer; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -187,6 +188,8 @@ public class DefaultP2PNetwork implements P2PNetwork { private final Optional blockchain; private OptionalLong blockAddedObserverId = OptionalLong.empty(); + private final AtomicBoolean started = new AtomicBoolean(false); + /** * Creates a peer networking service for production purposes. * @@ -521,32 +524,36 @@ public void subscribeDisconnect(final DisconnectCallback callback) { @Override public void start() { - peerDiscoveryAgent.start(ourPeerInfo.getPort()).join(); - peerBondedObserverId = - OptionalLong.of(peerDiscoveryAgent.observePeerBondedEvents(handlePeerBondedEvent())); - peerDroppedObserverId = - OptionalLong.of(peerDiscoveryAgent.observePeerDroppedEvents(handlePeerDroppedEvents())); - - if (nodePermissioningController.isPresent()) { - if (blockchain.isPresent()) { - synchronized (this) { - if (!blockAddedObserverId.isPresent()) { - blockAddedObserverId = - OptionalLong.of(blockchain.get().observeBlockAdded(this::handleBlockAddedEvent)); + if (started.compareAndSet(false, true)) { + peerDiscoveryAgent.start(ourPeerInfo.getPort()).join(); + peerBondedObserverId = + OptionalLong.of(peerDiscoveryAgent.observePeerBondedEvents(handlePeerBondedEvent())); + peerDroppedObserverId = + OptionalLong.of(peerDiscoveryAgent.observePeerDroppedEvents(handlePeerDroppedEvents())); + + if (nodePermissioningController.isPresent()) { + if (blockchain.isPresent()) { + synchronized (this) { + if (!blockAddedObserverId.isPresent()) { + blockAddedObserverId = + OptionalLong.of(blockchain.get().observeBlockAdded(this::handleBlockAddedEvent)); + } } + } else { + throw new IllegalStateException( + "Network permissioning needs to listen to BlockAddedEvents. Blockchain can't be null."); } - } else { - throw new IllegalStateException( - "Network permissioning needs to listen to BlockAddedEvents. Blockchain can't be null."); } - } - setLocalNode(); + setLocalNode(); - peerConnectionScheduler.scheduleWithFixedDelay( - this::checkMaintainedConnectionPeers, 2, 60, TimeUnit.SECONDS); - peerConnectionScheduler.scheduleWithFixedDelay( - this::attemptPeerConnections, 30, 30, TimeUnit.SECONDS); + peerConnectionScheduler.scheduleWithFixedDelay( + this::checkMaintainedConnectionPeers, 2, 60, TimeUnit.SECONDS); + peerConnectionScheduler.scheduleWithFixedDelay( + this::attemptPeerConnections, 30, 30, TimeUnit.SECONDS); + } else { + LOG.warn("Attempted to start an already started P2PNetwork"); + } } @VisibleForTesting From 27e1d2d865e9a62613ce956107d6ffaf270a9fff Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Tue, 30 Apr 2019 14:54:55 -0400 Subject: [PATCH 05/12] Don't construct local enode representation in RunnerBuilder --- ...nsufficientPeersPermissioningProvider.java | 20 +++++++---- ...ficientPeersPermissioningProviderTest.java | 35 +++++++++++++++---- .../internal/PeerDiscoveryControllerTest.java | 6 ++-- .../ethereum/p2p/network/P2PNetworkTest.java | 3 +- ...odeLocalConfigPermissioningController.java | 18 +++++----- .../NodePermissioningControllerFactory.java | 5 +-- ...ocalConfigPermissioningControllerTest.java | 21 ++++++----- ...odePermissioningControllerFactoryTest.java | 12 ++++--- .../tech/pegasys/pantheon/RunnerBuilder.java | 23 ++++++------ .../pantheon/util/enode/LocalNode.java | 6 ++++ 10 files changed, 92 insertions(+), 57 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProvider.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProvider.java index 4ef2897f33..956037f0e4 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProvider.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProvider.java @@ -18,6 +18,7 @@ import tech.pegasys.pantheon.ethereum.permissioning.node.ContextualNodePermissioningProvider; import tech.pegasys.pantheon.util.Subscribers; import tech.pegasys.pantheon.util.enode.EnodeURL; +import tech.pegasys.pantheon.util.enode.LocalNode; import java.util.Collection; import java.util.Optional; @@ -27,7 +28,7 @@ * bootnodes */ public class InsufficientPeersPermissioningProvider implements ContextualNodePermissioningProvider { - private final EnodeURL selfEnode; + private final LocalNode localNode; private final P2PNetwork p2pNetwork; private final Collection bootnodeEnodes; private long nonBootnodePeerConnections; @@ -37,14 +38,14 @@ public class InsufficientPeersPermissioningProvider implements ContextualNodePer * Creates the provider observing the provided p2p network * * @param p2pNetwork the p2p network to observe - * @param selfEnode the advertised enode address of this node + * @param localNode An object representing the locally running node * @param bootnodeEnodes the bootnodes that this node is configured to connection to */ public InsufficientPeersPermissioningProvider( final P2PNetwork p2pNetwork, - final EnodeURL selfEnode, + final LocalNode localNode, final Collection bootnodeEnodes) { - this.selfEnode = selfEnode; + this.localNode = localNode; this.p2pNetwork = p2pNetwork; this.bootnodeEnodes = bootnodeEnodes; this.nonBootnodePeerConnections = countP2PNetworkNonBootnodeConnections(); @@ -66,15 +67,20 @@ public Optional isPermitted( final EnodeURL sourceEnode, final EnodeURL destinationEnode) { if (nonBootnodePeerConnections > 0) { return Optional.empty(); - } else if (checkEnode(sourceEnode) && checkEnode(destinationEnode)) { + } else if (!localNode.isReady()) { + // The local node is not yet ready, so we can't validate enodes yet + return Optional.empty(); + } else if (checkEnode(localNode.getEnode(), sourceEnode) + && checkEnode(localNode.getEnode(), destinationEnode)) { return Optional.of(true); } else { return Optional.empty(); } } - private boolean checkEnode(final EnodeURL enode) { - return (enode.sameEndpoint(selfEnode) || bootnodeEnodes.stream().anyMatch(enode::sameEndpoint)); + private boolean checkEnode(final EnodeURL localEnode, final EnodeURL enode) { + return (enode.sameEndpoint(localEnode) + || bootnodeEnodes.stream().anyMatch(enode::sameEndpoint)); } private void handleConnect(final PeerConnection peerConnection) { diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProviderTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProviderTest.java index 183e4f85c6..3daf633b2e 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProviderTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProviderTest.java @@ -22,6 +22,7 @@ import tech.pegasys.pantheon.ethereum.p2p.api.P2PNetwork; import tech.pegasys.pantheon.ethereum.p2p.api.PeerConnection; import tech.pegasys.pantheon.util.enode.EnodeURL; +import tech.pegasys.pantheon.util.enode.LocalNode; import java.util.Arrays; import java.util.Collection; @@ -60,7 +61,8 @@ public void noResultWhenNoBootnodes() { when(p2pNetwork.getPeers()).thenReturn(Collections.emptyList()); final InsufficientPeersPermissioningProvider provider = - new InsufficientPeersPermissioningProvider(p2pNetwork, SELF_ENODE, bootnodes); + new InsufficientPeersPermissioningProvider( + p2pNetwork, LocalNode.create(SELF_ENODE), bootnodes); assertThat(provider.isPermitted(SELF_ENODE, ENODE_2)).isEmpty(); } @@ -74,7 +76,8 @@ public void noResultWhenOtherConnections() { final Collection bootnodes = Collections.singletonList(ENODE_2); final InsufficientPeersPermissioningProvider provider = - new InsufficientPeersPermissioningProvider(p2pNetwork, SELF_ENODE, bootnodes); + new InsufficientPeersPermissioningProvider( + p2pNetwork, LocalNode.create(SELF_ENODE), bootnodes); assertThat(provider.isPermitted(SELF_ENODE, ENODE_3)).isEmpty(); assertThat(provider.isPermitted(SELF_ENODE, ENODE_2)).isEmpty(); @@ -87,12 +90,26 @@ public void allowsConnectionIfBootnodeAndNoConnections() { when(p2pNetwork.getPeers()).thenReturn(Collections.emptyList()); final InsufficientPeersPermissioningProvider provider = - new InsufficientPeersPermissioningProvider(p2pNetwork, SELF_ENODE, bootnodes); + new InsufficientPeersPermissioningProvider( + p2pNetwork, LocalNode.create(SELF_ENODE), bootnodes); assertThat(provider.isPermitted(SELF_ENODE, ENODE_2)).contains(true); assertThat(provider.isPermitted(SELF_ENODE, ENODE_3)).isEmpty(); } + @Test + public void noResultWhenLocalNodeNotReady() { + final Collection bootnodes = Collections.singletonList(ENODE_2); + + when(p2pNetwork.getPeers()).thenReturn(Collections.emptyList()); + + final InsufficientPeersPermissioningProvider provider = + new InsufficientPeersPermissioningProvider(p2pNetwork, LocalNode.create(), bootnodes); + + assertThat(provider.isPermitted(SELF_ENODE, ENODE_2)).isEmpty(); + assertThat(provider.isPermitted(SELF_ENODE, ENODE_3)).isEmpty(); + } + @Test public void allowsConnectionIfBootnodeAndOnlyBootnodesConnected() { final Collection bootnodes = Collections.singletonList(ENODE_2); @@ -102,7 +119,8 @@ public void allowsConnectionIfBootnodeAndOnlyBootnodesConnected() { when(p2pNetwork.getPeers()).thenReturn(Collections.singletonList(bootnodeMatchPeerConnection)); final InsufficientPeersPermissioningProvider provider = - new InsufficientPeersPermissioningProvider(p2pNetwork, SELF_ENODE, bootnodes); + new InsufficientPeersPermissioningProvider( + p2pNetwork, LocalNode.create(SELF_ENODE), bootnodes); assertThat(provider.isPermitted(SELF_ENODE, ENODE_2)).contains(true); assertThat(provider.isPermitted(SELF_ENODE, ENODE_3)).isEmpty(); @@ -125,7 +143,8 @@ public void firesUpdateWhenDisconnectLastNonBootnode() { when(p2pNetwork.getPeers()).thenReturn(pcs); final InsufficientPeersPermissioningProvider provider = - new InsufficientPeersPermissioningProvider(p2pNetwork, SELF_ENODE, bootnodes); + new InsufficientPeersPermissioningProvider( + p2pNetwork, LocalNode.create(SELF_ENODE), bootnodes); final ArgumentCaptor callbackCaptor = ArgumentCaptor.forClass(DisconnectCallback.class); @@ -152,7 +171,8 @@ public void firesUpdateWhenNonBootnodeConnects() { when(p2pNetwork.getPeers()).thenReturn(pcs); final InsufficientPeersPermissioningProvider provider = - new InsufficientPeersPermissioningProvider(p2pNetwork, SELF_ENODE, bootnodes); + new InsufficientPeersPermissioningProvider( + p2pNetwork, LocalNode.create(SELF_ENODE), bootnodes); @SuppressWarnings("unchecked") final ArgumentCaptor> callbackCaptor = @@ -185,7 +205,8 @@ public void firesUpdateWhenGettingAndLosingConnection() { when(p2pNetwork.getPeers()).thenReturn(pcs); final InsufficientPeersPermissioningProvider provider = - new InsufficientPeersPermissioningProvider(p2pNetwork, SELF_ENODE, bootnodes); + new InsufficientPeersPermissioningProvider( + p2pNetwork, LocalNode.create(SELF_ENODE), bootnodes); @SuppressWarnings("unchecked") final ArgumentCaptor> connectCallbackCaptor = 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 5e11023b3b..29ffa6ae99 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 @@ -1074,7 +1074,8 @@ public void whenObservingNodeWhitelistAndNodeIsRemovedShouldEvictPeerFromPeerTab final URI peerURI = URI.create(peer.getEnodeURLString()); config.setNodeWhitelist(Lists.newArrayList(peerURI)); final NodeLocalConfigPermissioningController nodeLocalConfigPermissioningController = - new NodeLocalConfigPermissioningController(config, Collections.emptyList(), selfEnode); + new NodeLocalConfigPermissioningController( + config, Collections.emptyList(), selfEnode.getNodeId()); controller = getControllerBuilder() @@ -1101,7 +1102,8 @@ public void whenObservingNodeWhitelistAndNodeIsRemovedShouldNotifyPeerDroppedObs final URI peerURI = URI.create(peer.getEnodeURLString()); config.setNodeWhitelist(Lists.newArrayList(peerURI)); final NodeLocalConfigPermissioningController nodeLocalConfigPermissioningController = - new NodeLocalConfigPermissioningController(config, Collections.emptyList(), selfEnode); + new NodeLocalConfigPermissioningController( + config, Collections.emptyList(), selfEnode.getNodeId()); final Consumer peerDroppedEventConsumer = mock(Consumer.class); final Subscribers> peerDroppedSubscribers = new Subscribers(); 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 65089adab4..4c96180227 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 @@ -319,7 +319,8 @@ public void rejectIncomingConnectionFromNonWhitelistedPeer() throws Exception { config.setNodePermissioningConfigFilePath(tempFile.toAbsolutePath().toString()); final NodeLocalConfigPermissioningController localWhitelistController = - new NodeLocalConfigPermissioningController(config, Collections.emptyList(), selfEnode); + new NodeLocalConfigPermissioningController( + config, Collections.emptyList(), selfEnode.getNodeId()); // turn on whitelisting by adding a different node NOT remote node localWhitelistController.addNode( EnodeURL.builder().ipAddress("127.0.0.1").nodeId(Peer.randomId()).build()); diff --git a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningController.java b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningController.java index d84310f34d..ca5582d792 100644 --- a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningController.java +++ b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningController.java @@ -15,6 +15,7 @@ import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningProvider; import tech.pegasys.pantheon.ethereum.permissioning.node.NodeWhitelistUpdatedEvent; import tech.pegasys.pantheon.util.Subscribers; +import tech.pegasys.pantheon.util.bytes.BytesValue; import tech.pegasys.pantheon.util.enode.EnodeURL; import java.io.IOException; @@ -24,6 +25,7 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.Objects; import java.util.Optional; import java.util.function.Consumer; import java.util.stream.Collectors; @@ -38,7 +40,7 @@ public class NodeLocalConfigPermissioningController implements NodePermissioning private LocalPermissioningConfiguration configuration; private final List fixedNodes; - private final EnodeURL selfEnode; + private final BytesValue localNodeId; private final List nodesWhitelist = new ArrayList<>(); private final WhitelistPersistor whitelistPersistor; private final Subscribers> nodeWhitelistUpdatedObservers = @@ -47,22 +49,22 @@ public class NodeLocalConfigPermissioningController implements NodePermissioning public NodeLocalConfigPermissioningController( final LocalPermissioningConfiguration permissioningConfiguration, final List fixedNodes, - final EnodeURL selfEnode) { + final BytesValue localNodeId) { this( permissioningConfiguration, fixedNodes, - selfEnode, + localNodeId, new WhitelistPersistor(permissioningConfiguration.getNodePermissioningConfigFilePath())); } public NodeLocalConfigPermissioningController( final LocalPermissioningConfiguration configuration, final List fixedNodes, - final EnodeURL selfEnode, + final BytesValue localNodeId, final WhitelistPersistor whitelistPersistor) { this.configuration = configuration; this.fixedNodes = fixedNodes; - this.selfEnode = selfEnode; + this.localNodeId = localNodeId; this.whitelistPersistor = whitelistPersistor; readNodesFromConfig(configuration); } @@ -197,10 +199,6 @@ private Collection peerToEnodeURI(final Collection peers) { return peers.parallelStream().map(EnodeURL::toString).collect(Collectors.toList()); } - private boolean checkSelfEnode(final EnodeURL node) { - return selfEnode.getNodeId().equals(node.getNodeId()); - } - private boolean compareEnodes(final EnodeURL nodeA, final EnodeURL nodeB) { boolean idsMatch = nodeA.getNodeId().equals(nodeB.getNodeId()); boolean hostsMatch = nodeA.getIp().equals(nodeB.getIp()); @@ -219,7 +217,7 @@ public boolean isPermitted(final String enodeURL) { } public boolean isPermitted(final EnodeURL node) { - if (checkSelfEnode(node)) { + if (Objects.equals(localNodeId, node.getNodeId())) { return true; } return nodesWhitelist.stream().anyMatch(p -> compareEnodes(p, node)); diff --git a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/NodePermissioningControllerFactory.java b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/NodePermissioningControllerFactory.java index 16b70b8551..e1506e3e38 100644 --- a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/NodePermissioningControllerFactory.java +++ b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/NodePermissioningControllerFactory.java @@ -17,6 +17,7 @@ import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningProvider; import tech.pegasys.pantheon.ethereum.permissioning.node.provider.SyncStatusNodePermissioningProvider; import tech.pegasys.pantheon.ethereum.transaction.TransactionSimulator; +import tech.pegasys.pantheon.util.bytes.BytesValue; import tech.pegasys.pantheon.util.enode.EnodeURL; import java.util.ArrayList; @@ -30,7 +31,7 @@ public NodePermissioningController create( final PermissioningConfiguration permissioningConfiguration, final Synchronizer synchronizer, final Collection fixedNodes, - final EnodeURL selfEnode, + final BytesValue localNodeId, final TransactionSimulator transactionSimulator) { Optional syncStatusProviderOptional; @@ -42,7 +43,7 @@ public NodePermissioningController create( if (localPermissioningConfiguration.isNodeWhitelistEnabled()) { NodeLocalConfigPermissioningController localProvider = new NodeLocalConfigPermissioningController( - localPermissioningConfiguration, new ArrayList<>(fixedNodes), selfEnode); + localPermissioningConfiguration, new ArrayList<>(fixedNodes), localNodeId); providers.add(localProvider); } } diff --git a/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningControllerTest.java b/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningControllerTest.java index d621dcf856..f628687780 100644 --- a/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningControllerTest.java +++ b/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/NodeLocalConfigPermissioningControllerTest.java @@ -58,8 +58,9 @@ public class NodeLocalConfigPermissioningControllerTest { "enode://6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0@192.168.0.10:4567"; private final String enode2 = "enode://5f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0@192.168.0.10:4567"; - private final String selfEnode = - "enode://5f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0@192.168.0.10:1111"; + private final EnodeURL selfEnode = + EnodeURL.fromString( + "enode://5f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0@192.168.0.10:1111"); @Before public void setUp() { @@ -68,7 +69,7 @@ public void setUp() { new NodeLocalConfigPermissioningController( LocalPermissioningConfiguration.createDefault(), bootnodesList, - EnodeURL.fromString(selfEnode), + selfEnode.getNodeId(), whitelistPersistor); } @@ -219,10 +220,8 @@ public void whenCheckingIfNodeIsPermittedDiscoveryPortShouldBeConsideredIfPresen @Test public void whenCheckingIfNodeIsPermittedOrderDoesNotMatter() { controller.addNodes(Arrays.asList(enode1)); - assertThat(controller.isPermitted(EnodeURL.fromString(enode1), EnodeURL.fromString(selfEnode))) - .isTrue(); - assertThat(controller.isPermitted(EnodeURL.fromString(selfEnode), EnodeURL.fromString(enode1))) - .isTrue(); + assertThat(controller.isPermitted(EnodeURL.fromString(enode1), selfEnode)).isTrue(); + assertThat(controller.isPermitted(selfEnode, EnodeURL.fromString(enode1))).isTrue(); } @Test @@ -262,7 +261,7 @@ public void reloadNodeWhitelistWithValidConfigFileShouldUpdateWhitelist() throws .thenReturn(Arrays.asList(URI.create(expectedEnodeURL))); controller = new NodeLocalConfigPermissioningController( - permissioningConfig, bootnodesList, EnodeURL.fromString(selfEnode)); + permissioningConfig, bootnodesList, selfEnode.getNodeId()); controller.reload(); @@ -282,7 +281,7 @@ public void reloadNodeWhitelistWithErrorReadingConfigFileShouldKeepOldWhitelist( .thenReturn(Arrays.asList(URI.create(expectedEnodeURI))); controller = new NodeLocalConfigPermissioningController( - permissioningConfig, bootnodesList, EnodeURL.fromString(selfEnode)); + permissioningConfig, bootnodesList, selfEnode.getNodeId()); final Throwable thrown = catchThrowable(() -> controller.reload()); @@ -381,7 +380,7 @@ public void whenReloadingWhitelistShouldNotifyWhitelistModifiedSubscribers() thr when(permissioningConfig.getNodeWhitelist()).thenReturn(Arrays.asList(URI.create(enode1))); controller = new NodeLocalConfigPermissioningController( - permissioningConfig, bootnodesList, EnodeURL.fromString(selfEnode)); + permissioningConfig, bootnodesList, selfEnode.getNodeId()); controller.subscribeToListUpdatedEvent(consumer); controller.reload(); @@ -404,7 +403,7 @@ public void whenReloadingWhitelistAndNothingChangesShouldNotNotifyWhitelistModif when(permissioningConfig.getNodeWhitelist()).thenReturn(Arrays.asList(URI.create(enode1))); controller = new NodeLocalConfigPermissioningController( - permissioningConfig, bootnodesList, EnodeURL.fromString(selfEnode)); + permissioningConfig, bootnodesList, selfEnode.getNodeId()); controller.subscribeToListUpdatedEvent(consumer); controller.reload(); diff --git a/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningControllerFactoryTest.java b/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningControllerFactoryTest.java index ea27cd2b30..770b0dcf46 100644 --- a/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningControllerFactoryTest.java +++ b/ethereum/permissioning/src/test/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningControllerFactoryTest.java @@ -54,7 +54,8 @@ public void testCreateWithNeitherPermissioningEnabled() { config = new PermissioningConfiguration(Optional.empty(), Optional.empty()); NodePermissioningControllerFactory factory = new NodePermissioningControllerFactory(); NodePermissioningController controller = - factory.create(config, synchronizer, bootnodes, selfEnode, transactionSimulator); + factory.create( + config, synchronizer, bootnodes, selfEnode.getNodeId(), transactionSimulator); List providers = controller.getProviders(); assertThat(providers.size()).isEqualTo(0); @@ -73,7 +74,8 @@ public void testCreateWithSmartContractNodePermissioningEnabledOnly() { NodePermissioningControllerFactory factory = new NodePermissioningControllerFactory(); NodePermissioningController controller = - factory.create(config, synchronizer, bootnodes, selfEnode, transactionSimulator); + factory.create( + config, synchronizer, bootnodes, selfEnode.getNodeId(), transactionSimulator); List providers = controller.getProviders(); assertThat(providers.size()).isEqualTo(1); @@ -93,7 +95,8 @@ public void testCreateWithLocalNodePermissioningEnabledOnly() { NodePermissioningControllerFactory factory = new NodePermissioningControllerFactory(); NodePermissioningController controller = - factory.create(config, synchronizer, bootnodes, selfEnode, transactionSimulator); + factory.create( + config, synchronizer, bootnodes, selfEnode.getNodeId(), transactionSimulator); List providers = controller.getProviders(); assertThat(providers.size()).isEqualTo(1); @@ -120,7 +123,8 @@ public void testCreateWithLocalNodeAndSmartContractPermissioningEnabled() { NodePermissioningControllerFactory factory = new NodePermissioningControllerFactory(); NodePermissioningController controller = - factory.create(config, synchronizer, bootnodes, selfEnode, transactionSimulator); + factory.create( + config, synchronizer, bootnodes, selfEnode.getNodeId(), transactionSimulator); List providers = controller.getProviders(); assertThat(providers.size()).isEqualTo(2); diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java b/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java index d4661c5ad4..49b06a21ee 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java @@ -71,6 +71,7 @@ import tech.pegasys.pantheon.metrics.prometheus.MetricsService; import tech.pegasys.pantheon.util.bytes.BytesValue; import tech.pegasys.pantheon.util.enode.EnodeURL; +import tech.pegasys.pantheon.util.enode.MutableLocalNode; import java.net.URI; import java.nio.file.Path; @@ -106,15 +107,6 @@ public class RunnerBuilder { private Optional permissioningConfiguration = Optional.empty(); private Collection staticNodes = Collections.emptyList(); - private EnodeURL getSelfEnode() { - BytesValue nodeId = pantheonController.getLocalNodeKeyPair().getPublicKey().getEncodedBytes(); - return EnodeURL.builder() - .nodeId(nodeId) - .ipAddress(p2pAdvertisedHost) - .listeningPort(p2pListenPort) - .build(); - } - public RunnerBuilder vertx(final Vertx vertx) { this.vertx = vertx; return this; @@ -257,8 +249,10 @@ public Runner build() { new TransactionSimulator( context.getBlockchain(), context.getWorldStateArchive(), protocolSchedule); + BytesValue localNodeId = keyPair.getPublicKey().getEncodedBytes(); final Optional nodePermissioningController = - buildNodePermissioningController(bootnodesAsEnodeURLs, synchronizer, transactionSimulator); + buildNodePermissioningController( + bootnodesAsEnodeURLs, synchronizer, transactionSimulator, localNodeId); final Optional nodeWhitelistController = nodePermissioningController @@ -269,10 +263,12 @@ public Runner build() { .findFirst()) .map(n -> (NodeLocalConfigPermissioningController) n); + final MutableLocalNode localNode = MutableLocalNode.create(); NetworkBuilder inactiveNetwork = (caps) -> new NoopP2PNetwork(); NetworkBuilder activeNetwork = (caps) -> DefaultP2PNetwork.builder() + .localNode(localNode) .vertx(vertx) .keyPair(keyPair) .nodeLocalConfigPermissioningController(nodeWhitelistController) @@ -296,7 +292,7 @@ public Runner build() { n -> n.setInsufficientPeersPermissioningProvider( new InsufficientPeersPermissioningProvider( - networkRunner.getNetwork(), getSelfEnode(), bootnodesAsEnodeURLs))); + networkRunner.getNetwork(), localNode, bootnodesAsEnodeURLs))); final TransactionPool transactionPool = pantheonController.getTransactionPool(); final MiningCoordinator miningCoordinator = pantheonController.getMiningCoordinator(); @@ -409,12 +405,13 @@ public Runner build() { private Optional buildNodePermissioningController( final List bootnodesAsEnodeURLs, final Synchronizer synchronizer, - final TransactionSimulator transactionSimulator) { + final TransactionSimulator transactionSimulator, + final BytesValue localNodeId) { Collection fixedNodes = getFixedNodes(bootnodesAsEnodeURLs, staticNodes); return permissioningConfiguration.map( config -> new NodePermissioningControllerFactory() - .create(config, synchronizer, fixedNodes, getSelfEnode(), transactionSimulator)); + .create(config, synchronizer, fixedNodes, localNodeId, transactionSimulator)); } @VisibleForTesting diff --git a/util/src/main/java/tech/pegasys/pantheon/util/enode/LocalNode.java b/util/src/main/java/tech/pegasys/pantheon/util/enode/LocalNode.java index 936f7902f1..bd3cdf121f 100644 --- a/util/src/main/java/tech/pegasys/pantheon/util/enode/LocalNode.java +++ b/util/src/main/java/tech/pegasys/pantheon/util/enode/LocalNode.java @@ -18,6 +18,12 @@ static LocalNode create() { return DefaultLocalNode.create(); } + static LocalNode create(final EnodeURL enode) { + DefaultLocalNode localNode = DefaultLocalNode.create(); + localNode.setEnode(enode); + return localNode; + } + /** * While node is initializing, an empty value will be returned. Once this node is up and running, * a {@link EnodeURL} object corresponding to this node will be returned. From e95d0681450b12df36102d16c1b99c02ea2aca2f Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Tue, 30 Apr 2019 15:10:57 -0400 Subject: [PATCH 06/12] Remove LocalNode in favor of a plain Optional --- ...nsufficientPeersPermissioningProvider.java | 17 ++-- .../p2p/network/DefaultP2PNetwork.java | 25 ++---- ...ficientPeersPermissioningProviderTest.java | 18 ++-- .../tech/pegasys/pantheon/RunnerBuilder.java | 6 +- .../pantheon/util/enode/DefaultLocalNode.java | 65 -------------- .../pantheon/util/enode/LocalNode.java | 60 ------------- .../pantheon/util/enode/MutableLocalNode.java | 27 ------ .../util/enode/DefaultLocalNodeTest.java | 88 ------------------- 8 files changed, 27 insertions(+), 279 deletions(-) delete mode 100644 util/src/main/java/tech/pegasys/pantheon/util/enode/DefaultLocalNode.java delete mode 100644 util/src/main/java/tech/pegasys/pantheon/util/enode/LocalNode.java delete mode 100644 util/src/main/java/tech/pegasys/pantheon/util/enode/MutableLocalNode.java delete mode 100644 util/src/test/java/tech/pegasys/pantheon/util/enode/DefaultLocalNodeTest.java diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProvider.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProvider.java index 956037f0e4..c172c28ba1 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProvider.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProvider.java @@ -18,17 +18,17 @@ import tech.pegasys.pantheon.ethereum.permissioning.node.ContextualNodePermissioningProvider; import tech.pegasys.pantheon.util.Subscribers; import tech.pegasys.pantheon.util.enode.EnodeURL; -import tech.pegasys.pantheon.util.enode.LocalNode; import java.util.Collection; import java.util.Optional; +import java.util.function.Supplier; /** * A permissioning provider that only provides an answer when we have no peers outside of our * bootnodes */ public class InsufficientPeersPermissioningProvider implements ContextualNodePermissioningProvider { - private final LocalNode localNode; + private final Supplier> selfEnode; private final P2PNetwork p2pNetwork; private final Collection bootnodeEnodes; private long nonBootnodePeerConnections; @@ -38,14 +38,14 @@ public class InsufficientPeersPermissioningProvider implements ContextualNodePer * Creates the provider observing the provided p2p network * * @param p2pNetwork the p2p network to observe - * @param localNode An object representing the locally running node + * @param selfEnode A supplier that provides a represention the locally running node, if available * @param bootnodeEnodes the bootnodes that this node is configured to connection to */ public InsufficientPeersPermissioningProvider( final P2PNetwork p2pNetwork, - final LocalNode localNode, + final Supplier> selfEnode, final Collection bootnodeEnodes) { - this.localNode = localNode; + this.selfEnode = selfEnode; this.p2pNetwork = p2pNetwork; this.bootnodeEnodes = bootnodeEnodes; this.nonBootnodePeerConnections = countP2PNetworkNonBootnodeConnections(); @@ -65,13 +65,14 @@ private long countP2PNetworkNonBootnodeConnections() { @Override public Optional isPermitted( final EnodeURL sourceEnode, final EnodeURL destinationEnode) { + Optional maybeSelfEnode = selfEnode.get(); if (nonBootnodePeerConnections > 0) { return Optional.empty(); - } else if (!localNode.isReady()) { + } else if (!maybeSelfEnode.isPresent()) { // The local node is not yet ready, so we can't validate enodes yet return Optional.empty(); - } else if (checkEnode(localNode.getEnode(), sourceEnode) - && checkEnode(localNode.getEnode(), destinationEnode)) { + } else if (checkEnode(maybeSelfEnode.get(), sourceEnode) + && checkEnode(maybeSelfEnode.get(), destinationEnode)) { return Optional.of(true); } else { return Optional.empty(); 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 9e3ffa4e70..4bbbd30487 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 @@ -51,7 +51,6 @@ import tech.pegasys.pantheon.util.Subscribers; import tech.pegasys.pantheon.util.bytes.BytesValue; import tech.pegasys.pantheon.util.enode.EnodeURL; -import tech.pegasys.pantheon.util.enode.MutableLocalNode; import java.net.InetSocketAddress; import java.util.Arrays; @@ -182,7 +181,7 @@ public class DefaultP2PNetwork implements P2PNetwork { private final String advertisedHost; - private final MutableLocalNode localNode; + private volatile Optional localEnode = Optional.empty(); private final Optional nodePermissioningController; private final Optional blockchain; @@ -207,7 +206,6 @@ public class DefaultP2PNetwork implements P2PNetwork { * @param blockchain The blockchain to subscribe to BlockAddedEvents. */ DefaultP2PNetwork( - final MutableLocalNode localNode, final PeerDiscoveryAgent peerDiscoveryAgent, final SECP256K1.KeyPair keyPair, final NetworkingConfiguration config, @@ -217,7 +215,6 @@ public class DefaultP2PNetwork implements P2PNetwork { final Optional nodePermissioningController, final Blockchain blockchain) { - this.localNode = localNode; this.config = config; maxPeers = config.getRlpx().getMaxPeers(); connections = new PeerConnectionRegistry(metricsSystem); @@ -691,13 +688,14 @@ public boolean isDiscoveryEnabled() { @Override public Optional getLocalEnode() { - if (!localNode.isReady()) { - return Optional.empty(); - } - return Optional.of(localNode.getEnode()); + return localEnode; } private void setLocalNode() { + if (localEnode.isPresent()) { + return; + } + final BytesValue nodeId = ourPeerInfo.getNodeId(); final int listeningPort = ourPeerInfo.getPort(); final OptionalInt discoveryPort = @@ -716,7 +714,7 @@ private void setLocalNode() { .build(); LOG.info("Enode URL {}", localEnode.toString()); - localNode.setEnode(localEnode); + this.localEnode = Optional.of(localEnode); } private void onConnectionEstablished(final PeerConnection connection) { @@ -726,7 +724,6 @@ private void onConnectionEstablished(final PeerConnection connection) { public static class Builder { - private MutableLocalNode localNode = MutableLocalNode.create(); private PeerDiscoveryAgent peerDiscoveryAgent; private KeyPair keyPair; private NetworkingConfiguration config = NetworkingConfiguration.create(); @@ -748,7 +745,6 @@ private P2PNetwork doBuild() { peerDiscoveryAgent = peerDiscoveryAgent == null ? createDiscoveryAgent() : peerDiscoveryAgent; return new DefaultP2PNetwork( - localNode, peerDiscoveryAgent, keyPair, config, @@ -760,7 +756,6 @@ private P2PNetwork doBuild() { } private void validate() { - checkState(localNode != null, "LocalNode must be set."); checkState(keyPair != null, "KeyPair must be set."); checkState(config != null, "NetworkingConfiguration must be set."); checkState( @@ -789,12 +784,6 @@ private PeerDiscoveryAgent createDiscoveryAgent() { metricsSystem); } - public Builder localNode(final MutableLocalNode localNode) { - checkNotNull(localNode); - this.localNode = localNode; - return this; - } - public Builder vertx(final Vertx vertx) { checkNotNull(vertx); this.vertx = vertx; diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProviderTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProviderTest.java index 3daf633b2e..0cced86e6a 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProviderTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProviderTest.java @@ -22,11 +22,11 @@ import tech.pegasys.pantheon.ethereum.p2p.api.P2PNetwork; import tech.pegasys.pantheon.ethereum.p2p.api.PeerConnection; import tech.pegasys.pantheon.util.enode.EnodeURL; -import tech.pegasys.pantheon.util.enode.LocalNode; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.Optional; import java.util.function.Consumer; import org.junit.Test; @@ -62,7 +62,7 @@ public void noResultWhenNoBootnodes() { final InsufficientPeersPermissioningProvider provider = new InsufficientPeersPermissioningProvider( - p2pNetwork, LocalNode.create(SELF_ENODE), bootnodes); + p2pNetwork, () -> Optional.of(SELF_ENODE), bootnodes); assertThat(provider.isPermitted(SELF_ENODE, ENODE_2)).isEmpty(); } @@ -77,7 +77,7 @@ public void noResultWhenOtherConnections() { final InsufficientPeersPermissioningProvider provider = new InsufficientPeersPermissioningProvider( - p2pNetwork, LocalNode.create(SELF_ENODE), bootnodes); + p2pNetwork, () -> Optional.of(SELF_ENODE), bootnodes); assertThat(provider.isPermitted(SELF_ENODE, ENODE_3)).isEmpty(); assertThat(provider.isPermitted(SELF_ENODE, ENODE_2)).isEmpty(); @@ -91,7 +91,7 @@ public void allowsConnectionIfBootnodeAndNoConnections() { final InsufficientPeersPermissioningProvider provider = new InsufficientPeersPermissioningProvider( - p2pNetwork, LocalNode.create(SELF_ENODE), bootnodes); + p2pNetwork, () -> Optional.of(SELF_ENODE), bootnodes); assertThat(provider.isPermitted(SELF_ENODE, ENODE_2)).contains(true); assertThat(provider.isPermitted(SELF_ENODE, ENODE_3)).isEmpty(); @@ -104,7 +104,7 @@ public void noResultWhenLocalNodeNotReady() { when(p2pNetwork.getPeers()).thenReturn(Collections.emptyList()); final InsufficientPeersPermissioningProvider provider = - new InsufficientPeersPermissioningProvider(p2pNetwork, LocalNode.create(), bootnodes); + new InsufficientPeersPermissioningProvider(p2pNetwork, Optional::empty, bootnodes); assertThat(provider.isPermitted(SELF_ENODE, ENODE_2)).isEmpty(); assertThat(provider.isPermitted(SELF_ENODE, ENODE_3)).isEmpty(); @@ -120,7 +120,7 @@ public void allowsConnectionIfBootnodeAndOnlyBootnodesConnected() { final InsufficientPeersPermissioningProvider provider = new InsufficientPeersPermissioningProvider( - p2pNetwork, LocalNode.create(SELF_ENODE), bootnodes); + p2pNetwork, () -> Optional.of(SELF_ENODE), bootnodes); assertThat(provider.isPermitted(SELF_ENODE, ENODE_2)).contains(true); assertThat(provider.isPermitted(SELF_ENODE, ENODE_3)).isEmpty(); @@ -144,7 +144,7 @@ public void firesUpdateWhenDisconnectLastNonBootnode() { final InsufficientPeersPermissioningProvider provider = new InsufficientPeersPermissioningProvider( - p2pNetwork, LocalNode.create(SELF_ENODE), bootnodes); + p2pNetwork, () -> Optional.of(SELF_ENODE), bootnodes); final ArgumentCaptor callbackCaptor = ArgumentCaptor.forClass(DisconnectCallback.class); @@ -172,7 +172,7 @@ public void firesUpdateWhenNonBootnodeConnects() { final InsufficientPeersPermissioningProvider provider = new InsufficientPeersPermissioningProvider( - p2pNetwork, LocalNode.create(SELF_ENODE), bootnodes); + p2pNetwork, () -> Optional.of(SELF_ENODE), bootnodes); @SuppressWarnings("unchecked") final ArgumentCaptor> callbackCaptor = @@ -206,7 +206,7 @@ public void firesUpdateWhenGettingAndLosingConnection() { final InsufficientPeersPermissioningProvider provider = new InsufficientPeersPermissioningProvider( - p2pNetwork, LocalNode.create(SELF_ENODE), bootnodes); + p2pNetwork, () -> Optional.of(SELF_ENODE), bootnodes); @SuppressWarnings("unchecked") final ArgumentCaptor> connectCallbackCaptor = diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java b/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java index 49b06a21ee..836ec3015b 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java @@ -71,7 +71,6 @@ import tech.pegasys.pantheon.metrics.prometheus.MetricsService; import tech.pegasys.pantheon.util.bytes.BytesValue; import tech.pegasys.pantheon.util.enode.EnodeURL; -import tech.pegasys.pantheon.util.enode.MutableLocalNode; import java.net.URI; import java.nio.file.Path; @@ -263,12 +262,10 @@ public Runner build() { .findFirst()) .map(n -> (NodeLocalConfigPermissioningController) n); - final MutableLocalNode localNode = MutableLocalNode.create(); NetworkBuilder inactiveNetwork = (caps) -> new NoopP2PNetwork(); NetworkBuilder activeNetwork = (caps) -> DefaultP2PNetwork.builder() - .localNode(localNode) .vertx(vertx) .keyPair(keyPair) .nodeLocalConfigPermissioningController(nodeWhitelistController) @@ -288,11 +285,12 @@ public Runner build() { .metricsSystem(metricsSystem) .build(); + final P2PNetwork network = networkRunner.getNetwork(); nodePermissioningController.ifPresent( n -> n.setInsufficientPeersPermissioningProvider( new InsufficientPeersPermissioningProvider( - networkRunner.getNetwork(), localNode, bootnodesAsEnodeURLs))); + networkRunner.getNetwork(), network::getLocalEnode, bootnodesAsEnodeURLs))); final TransactionPool transactionPool = pantheonController.getTransactionPool(); final MiningCoordinator miningCoordinator = pantheonController.getMiningCoordinator(); diff --git a/util/src/main/java/tech/pegasys/pantheon/util/enode/DefaultLocalNode.java b/util/src/main/java/tech/pegasys/pantheon/util/enode/DefaultLocalNode.java deleted file mode 100644 index 73cc73923b..0000000000 --- a/util/src/main/java/tech/pegasys/pantheon/util/enode/DefaultLocalNode.java +++ /dev/null @@ -1,65 +0,0 @@ -/* - * 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.enode; - -import tech.pegasys.pantheon.util.Subscribers; - -import java.util.Optional; - -class DefaultLocalNode implements MutableLocalNode { - - private Optional enode = Optional.empty(); - private Subscribers readySubscribers = new Subscribers<>(); - - private DefaultLocalNode() {} - - public static DefaultLocalNode create() { - return new DefaultLocalNode(); - } - - @Override - public void setEnode(final EnodeURL enode) throws NodeAlreadySetException { - if (this.enode.isPresent()) { - throw new NodeAlreadySetException("Attempt to set already initialized local node"); - } - this.enode = Optional.of(enode); - dispatchReady(enode); - } - - @Override - public EnodeURL getEnode() throws NodeNotReadyException { - if (!enode.isPresent()) { - throw new NodeNotReadyException("Attempt to access local enode before local node is ready."); - } - return enode.get(); - } - - @Override - public boolean isReady() { - return enode.isPresent(); - } - - @Override - public synchronized void subscribeReady(final ReadyCallback callback) { - if (isReady()) { - callback.onReady(enode.get()); - } else { - readySubscribers.subscribe(callback); - } - } - - private synchronized void dispatchReady(final EnodeURL localNode) { - readySubscribers.forEach(c -> c.onReady(localNode)); - readySubscribers.clear(); - } -} diff --git a/util/src/main/java/tech/pegasys/pantheon/util/enode/LocalNode.java b/util/src/main/java/tech/pegasys/pantheon/util/enode/LocalNode.java deleted file mode 100644 index bd3cdf121f..0000000000 --- a/util/src/main/java/tech/pegasys/pantheon/util/enode/LocalNode.java +++ /dev/null @@ -1,60 +0,0 @@ -/* - * 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.enode; - -public interface LocalNode { - - static LocalNode create() { - return DefaultLocalNode.create(); - } - - static LocalNode create(final EnodeURL enode) { - DefaultLocalNode localNode = DefaultLocalNode.create(); - localNode.setEnode(enode); - return localNode; - } - - /** - * While node is initializing, an empty value will be returned. Once this node is up and running, - * a {@link EnodeURL} object corresponding to this node will be returned. - * - * @return The {@link EnodeURL} representation associated with this node. - */ - EnodeURL getEnode() throws NodeNotReadyException; - - /** - * @return True if the local node is up and running and has an available {@link EnodeURL} - * representation. - */ - boolean isReady(); - - /** - * When this node is up and running with a valid {@link EnodeURL} representation, the given - * callback will be invoked. If the callback is added after this node is ready, it is invoked - * immediately. - * - * @param callback The callback to run against the {@link EnodeURL} representing the local node, - * when the local node is ready. - */ - void subscribeReady(ReadyCallback callback); - - interface ReadyCallback { - void onReady(EnodeURL localNode); - } - - class NodeNotReadyException extends RuntimeException { - public NodeNotReadyException(final String message) { - super(message); - } - } -} diff --git a/util/src/main/java/tech/pegasys/pantheon/util/enode/MutableLocalNode.java b/util/src/main/java/tech/pegasys/pantheon/util/enode/MutableLocalNode.java deleted file mode 100644 index 0f2af0fea2..0000000000 --- a/util/src/main/java/tech/pegasys/pantheon/util/enode/MutableLocalNode.java +++ /dev/null @@ -1,27 +0,0 @@ -/* - * 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.enode; - -public interface MutableLocalNode extends LocalNode { - static MutableLocalNode create() { - return DefaultLocalNode.create(); - } - - void setEnode(EnodeURL enode) throws NodeAlreadySetException; - - class NodeAlreadySetException extends RuntimeException { - public NodeAlreadySetException(final String message) { - super(message); - } - } -} diff --git a/util/src/test/java/tech/pegasys/pantheon/util/enode/DefaultLocalNodeTest.java b/util/src/test/java/tech/pegasys/pantheon/util/enode/DefaultLocalNodeTest.java deleted file mode 100644 index 51c4409845..0000000000 --- a/util/src/test/java/tech/pegasys/pantheon/util/enode/DefaultLocalNodeTest.java +++ /dev/null @@ -1,88 +0,0 @@ -/* - * 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.enode; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Java6Assertions.assertThatThrownBy; - -import tech.pegasys.pantheon.util.bytes.BytesValue; -import tech.pegasys.pantheon.util.enode.LocalNode.NodeNotReadyException; - -import java.util.concurrent.atomic.AtomicReference; - -import org.junit.Test; - -public class DefaultLocalNodeTest { - private final EnodeURL enode = - EnodeURL.builder() - .ipAddress("127.0.0.1") - .listeningPort(30303) - .nodeId(BytesValue.of(new byte[64])) - .build(); - - @Test - public void create() { - final LocalNode localNode = DefaultLocalNode.create(); - assertThat(localNode.isReady()).isFalse(); - assertThatThrownBy(localNode::getEnode).isInstanceOf(NodeNotReadyException.class); - } - - @Test - public void setEnode() { - final MutableLocalNode localNode = DefaultLocalNode.create(); - localNode.setEnode(enode); - - assertThat(localNode.isReady()).isTrue(); - final EnodeURL enodeValue = localNode.getEnode(); - assertThat(enodeValue).isEqualTo(enode); - } - - @Test - public void subscribeReady_beforeReady() { - AtomicReference localEnode = new AtomicReference<>(null); - final MutableLocalNode localNode = DefaultLocalNode.create(); - localNode.subscribeReady(localEnode::set); - - assertThat(localEnode.get()).isNull(); - - localNode.setEnode(enode); - assertThat(localEnode.get()).isEqualTo(enode); - } - - @Test - public void subscribeReady_afterReady() { - AtomicReference localEnode = new AtomicReference<>(null); - final MutableLocalNode localNode = DefaultLocalNode.create(); - localNode.setEnode(enode); - - localNode.subscribeReady(localEnode::set); - assertThat(localEnode.get()).isEqualTo(enode); - } - - @Test - public void subscribeReady_beforeAndAfterReady() { - final MutableLocalNode localNode = DefaultLocalNode.create(); - - AtomicReference subscriberA = new AtomicReference<>(null); - AtomicReference subscriberB = new AtomicReference<>(null); - - localNode.subscribeReady(subscriberA::set); - assertThat(subscriberA.get()).isNull(); - - localNode.setEnode(enode); - - localNode.subscribeReady(subscriberB::set); - assertThat(subscriberA.get()).isEqualTo(enode); - assertThat(subscriberB.get()).isEqualTo(enode); - } -} From eab46621552c9097b6bc0e50c532b45661e0ebb6 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Tue, 30 Apr 2019 16:01:15 -0400 Subject: [PATCH 07/12] Test that we don't connect to disallowed maintained peers --- .../p2p/network/DefaultP2PNetworkTest.java | 14 ++++++++++++++ 1 file changed, 14 insertions(+) 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 48e19a7b84..146757e61e 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 @@ -134,6 +134,20 @@ public void checkMaintainedConnectionPeersTriesToConnect() { verify(network, atLeast(1)).connect(peer); } + @Test + public void checkMaintainedConnectionPeersDoesNotConnectToDisallowedPeer() { + final DefaultP2PNetwork network = mockNetwork(); + network.start(); + + // Add peer that is not permitted + final Peer peer = mockPeer(); + lenient().when(nodePermissioningController.isPermitted(any(), any())).thenReturn(false); + network.peerMaintainConnectionList.add(peer); + + network.checkMaintainedConnectionPeers(); + verify(network, never()).connect(peer); + } + @Test public void checkMaintainedConnectionPeersDoesntReconnectPendingPeers() { final DefaultP2PNetwork network = mockNetwork(); From d4c8f2ecde9fb1cbf52730fb4b9231cfe4ca8a4b Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Tue, 30 Apr 2019 16:11:04 -0400 Subject: [PATCH 08/12] Use more precise times over atLeast --- .../ethereum/p2p/InsufficientPeersPermissioningProvider.java | 3 ++- .../pantheon/ethereum/p2p/network/DefaultP2PNetworkTest.java | 5 ++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProvider.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProvider.java index c172c28ba1..c6d02f3d6b 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProvider.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/InsufficientPeersPermissioningProvider.java @@ -38,7 +38,8 @@ public class InsufficientPeersPermissioningProvider implements ContextualNodePer * Creates the provider observing the provided p2p network * * @param p2pNetwork the p2p network to observe - * @param selfEnode A supplier that provides a represention the locally running node, if available + * @param selfEnode A supplier that provides a representation of the locally running node, if + * available * @param bootnodeEnodes the bootnodes that this node is configured to connection to */ public InsufficientPeersPermissioningProvider( 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 146757e61e..e25484835c 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 @@ -17,7 +17,6 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; @@ -110,7 +109,7 @@ public void addingMaintainedNetworkPeerStartsConnection() { assertThat(network.addMaintainConnectionPeer(peer)).isTrue(); assertThat(network.peerMaintainConnectionList).contains(peer); - verify(network, atLeast(1)).connect(peer); + verify(network, times(1)).connect(peer); } @Test @@ -131,7 +130,7 @@ public void checkMaintainedConnectionPeersTriesToConnect() { network.peerMaintainConnectionList.add(peer); network.checkMaintainedConnectionPeers(); - verify(network, atLeast(1)).connect(peer); + verify(network, times(1)).connect(peer); } @Test From 98e7be163495690425458df0f61d3bb946751738 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Tue, 30 Apr 2019 16:15:10 -0400 Subject: [PATCH 09/12] Fix method name --- .../pantheon/ethereum/p2p/network/DefaultP2PNetwork.java | 4 ++-- 1 file changed, 2 insertions(+), 2 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 4bbbd30487..f31d07916c 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 @@ -542,7 +542,7 @@ public void start() { } } - setLocalNode(); + setLocalEnode(); peerConnectionScheduler.scheduleWithFixedDelay( this::checkMaintainedConnectionPeers, 2, 60, TimeUnit.SECONDS); @@ -691,7 +691,7 @@ public Optional getLocalEnode() { return localEnode; } - private void setLocalNode() { + private void setLocalEnode() { if (localEnode.isPresent()) { return; } From 7335b49ae7c1f4ab19f669dc6aa24f99e4ffd31f Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Tue, 30 Apr 2019 17:26:47 -0400 Subject: [PATCH 10/12] Cut unused code --- .../pegasys/pantheon/util/Subscribers.java | 35 ------------------- .../pantheon/util/SubscribersTest.java | 15 -------- 2 files changed, 50 deletions(-) diff --git a/util/src/main/java/tech/pegasys/pantheon/util/Subscribers.java b/util/src/main/java/tech/pegasys/pantheon/util/Subscribers.java index a984da39fe..e06a815f70 100644 --- a/util/src/main/java/tech/pegasys/pantheon/util/Subscribers.java +++ b/util/src/main/java/tech/pegasys/pantheon/util/Subscribers.java @@ -39,16 +39,9 @@ */ public class Subscribers { - public static Subscribers NONE = new EmptySubscribers<>(); - private final AtomicLong subscriberId = new AtomicLong(); private final Map subscribers = new ConcurrentHashMap<>(); - @SuppressWarnings("unchecked") - public static Subscribers none() { - return (Subscribers) NONE; - } - /** * Add a subscriber to the list. * @@ -93,32 +86,4 @@ public void forEach(final Consumer action) { public int getSubscriberCount() { return subscribers.size(); } - - /** Remove all subscribers */ - public void clear() { - subscribers.clear(); - } - - private static class EmptySubscribers extends Subscribers { - - @Override - public long subscribe(final T subscriber) { - throw new UnsupportedOperationException(); - } - - @Override - public boolean unsubscribe(final long subscriberId) { - return false; - } - - @Override - public void forEach(final Consumer action) { - return; - } - - @Override - public int getSubscriberCount() { - return 0; - } - } } diff --git a/util/src/test/java/tech/pegasys/pantheon/util/SubscribersTest.java b/util/src/test/java/tech/pegasys/pantheon/util/SubscribersTest.java index b1105eef3d..fa70ee0dd3 100644 --- a/util/src/test/java/tech/pegasys/pantheon/util/SubscribersTest.java +++ b/util/src/test/java/tech/pegasys/pantheon/util/SubscribersTest.java @@ -46,21 +46,6 @@ public void shouldRemoveSubscriber() { verify(subscriber2).run(); } - @Test - public void shouldClearSubscriber() { - subscribers.subscribe(subscriber1); - subscribers.subscribe(subscriber2); - assertThat(subscribers.getSubscriberCount()).isEqualTo(2); - - subscribers.clear(); - - assertThat(subscribers.getSubscriberCount()).isEqualTo(0); - - subscribers.forEach(Runnable::run); - verifyZeroInteractions(subscriber1); - verifyZeroInteractions(subscriber2); - } - @Test public void shouldTrackMultipleSubscribers() { final Runnable subscriber3 = mock(Runnable.class); From c3c25bdb11d2f3b8ef428eaef14eb649c619df37 Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Tue, 30 Apr 2019 17:45:08 -0400 Subject: [PATCH 11/12] Code review cleanup --- .../p2p/network/DefaultP2PNetwork.java | 52 +++++++++---------- 1 file changed, 26 insertions(+), 26 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 f31d07916c..29a2e8d7f9 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 @@ -521,36 +521,36 @@ public void subscribeDisconnect(final DisconnectCallback callback) { @Override public void start() { - if (started.compareAndSet(false, true)) { - peerDiscoveryAgent.start(ourPeerInfo.getPort()).join(); - peerBondedObserverId = - OptionalLong.of(peerDiscoveryAgent.observePeerBondedEvents(handlePeerBondedEvent())); - peerDroppedObserverId = - OptionalLong.of(peerDiscoveryAgent.observePeerDroppedEvents(handlePeerDroppedEvents())); - - if (nodePermissioningController.isPresent()) { - if (blockchain.isPresent()) { - synchronized (this) { - if (!blockAddedObserverId.isPresent()) { - blockAddedObserverId = - OptionalLong.of(blockchain.get().observeBlockAdded(this::handleBlockAddedEvent)); - } + if (!started.compareAndSet(false, true)) { + LOG.warn("Attempted to start an already started " + getClass().getSimpleName()); + } + + peerDiscoveryAgent.start(ourPeerInfo.getPort()).join(); + peerBondedObserverId = + OptionalLong.of(peerDiscoveryAgent.observePeerBondedEvents(handlePeerBondedEvent())); + peerDroppedObserverId = + OptionalLong.of(peerDiscoveryAgent.observePeerDroppedEvents(handlePeerDroppedEvents())); + + if (nodePermissioningController.isPresent()) { + if (blockchain.isPresent()) { + synchronized (this) { + if (!blockAddedObserverId.isPresent()) { + blockAddedObserverId = + OptionalLong.of(blockchain.get().observeBlockAdded(this::handleBlockAddedEvent)); } - } else { - throw new IllegalStateException( - "Network permissioning needs to listen to BlockAddedEvents. Blockchain can't be null."); } + } else { + throw new IllegalStateException( + "Network permissioning needs to listen to BlockAddedEvents. Blockchain can't be null."); } + } - setLocalEnode(); + createLocalEnode(); - peerConnectionScheduler.scheduleWithFixedDelay( - this::checkMaintainedConnectionPeers, 2, 60, TimeUnit.SECONDS); - peerConnectionScheduler.scheduleWithFixedDelay( - this::attemptPeerConnections, 30, 30, TimeUnit.SECONDS); - } else { - LOG.warn("Attempted to start an already started P2PNetwork"); - } + peerConnectionScheduler.scheduleWithFixedDelay( + this::checkMaintainedConnectionPeers, 2, 60, TimeUnit.SECONDS); + peerConnectionScheduler.scheduleWithFixedDelay( + this::attemptPeerConnections, 30, 30, TimeUnit.SECONDS); } @VisibleForTesting @@ -691,7 +691,7 @@ public Optional getLocalEnode() { return localEnode; } - private void setLocalEnode() { + private void createLocalEnode() { if (localEnode.isPresent()) { return; } From b286f7d854eaa3fd4d112561bd36bf87d90625ef Mon Sep 17 00:00:00 2001 From: Meredith Baxter Date: Tue, 30 Apr 2019 21:48:36 -0400 Subject: [PATCH 12/12] Remove dead code --- .../internal/methods/AdminAddPeer.java | 21 ++++---------- .../internal/methods/AdminAddPeerTest.java | 29 ------------------- .../p2p/ConnectingToLocalNodeException.java | 23 --------------- .../p2p/PeerNotPermittedException.java | 23 --------------- .../tech/pegasys/pantheon/RunnerBuilder.java | 13 ++------- 5 files changed, 8 insertions(+), 101 deletions(-) delete mode 100644 ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/ConnectingToLocalNodeException.java delete mode 100644 ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/PeerNotPermittedException.java diff --git a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/AdminAddPeer.java b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/AdminAddPeer.java index eef58fa12d..57592224e1 100644 --- a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/AdminAddPeer.java +++ b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/AdminAddPeer.java @@ -13,12 +13,8 @@ package tech.pegasys.pantheon.ethereum.jsonrpc.internal.methods; import tech.pegasys.pantheon.ethereum.jsonrpc.internal.parameters.JsonRpcParameter; -import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcError; -import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcErrorResponse; import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcResponse; import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcSuccessResponse; -import tech.pegasys.pantheon.ethereum.p2p.ConnectingToLocalNodeException; -import tech.pegasys.pantheon.ethereum.p2p.PeerNotPermittedException; import tech.pegasys.pantheon.ethereum.p2p.api.P2PNetwork; import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeer; import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; @@ -42,17 +38,10 @@ public String getName() { @Override protected JsonRpcResponse performOperation(final Object id, final String enode) { - try { - LOG.debug("Adding ({}) to peers", enode); - final EnodeURL enodeURL = EnodeURL.fromString(enode); - final Peer peer = DefaultPeer.fromEnodeURL(enodeURL); - boolean addedToNetwork = peerNetwork.addMaintainConnectionPeer(peer); - return new JsonRpcSuccessResponse(id, addedToNetwork); - } catch (final PeerNotPermittedException e) { - return new JsonRpcErrorResponse( - id, JsonRpcError.NON_PERMITTED_NODE_CANNOT_BE_ADDED_AS_A_PEER); - } catch (final ConnectingToLocalNodeException e) { - return new JsonRpcErrorResponse(id, JsonRpcError.CANT_CONNECT_TO_LOCAL_PEER); - } + LOG.debug("Adding ({}) to peers", enode); + final EnodeURL enodeURL = EnodeURL.fromString(enode); + final Peer peer = DefaultPeer.fromEnodeURL(enodeURL); + boolean addedToNetwork = peerNetwork.addMaintainConnectionPeer(peer); + return new JsonRpcSuccessResponse(id, addedToNetwork); } } diff --git a/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/AdminAddPeerTest.java b/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/AdminAddPeerTest.java index 46a467b806..79dac1b961 100644 --- a/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/AdminAddPeerTest.java +++ b/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/AdminAddPeerTest.java @@ -22,9 +22,7 @@ import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcErrorResponse; import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcResponse; import tech.pegasys.pantheon.ethereum.jsonrpc.internal.response.JsonRpcSuccessResponse; -import tech.pegasys.pantheon.ethereum.p2p.ConnectingToLocalNodeException; import tech.pegasys.pantheon.ethereum.p2p.P2pDisabledException; -import tech.pegasys.pantheon.ethereum.p2p.PeerNotPermittedException; import tech.pegasys.pantheon.ethereum.p2p.api.P2PNetwork; import org.junit.Before; @@ -151,31 +149,4 @@ public void requestReturnsErrorWhenP2pDisabled() { assertThat(actualResponse).isEqualToComparingFieldByField(expectedResponse); } - - @Test - public void requestReturnsErrorWhenPeerNotWhitelisted() { - when(p2pNetwork.addMaintainConnectionPeer(any())).thenThrow(new PeerNotPermittedException()); - - final JsonRpcResponse expectedResponse = - new JsonRpcErrorResponse( - validRequest.getId(), JsonRpcError.NON_PERMITTED_NODE_CANNOT_BE_ADDED_AS_A_PEER); - - final JsonRpcResponse actualResponse = method.response(validRequest); - - assertThat(actualResponse).isEqualToComparingFieldByField(expectedResponse); - } - - @Test - public void - p2pNetworkThrowsConnectingToLocalNodeExceptionReturnsCantConnectToLocalNodeJsonError() { - when(p2pNetwork.addMaintainConnectionPeer(any())) - .thenThrow(new ConnectingToLocalNodeException()); - - final JsonRpcResponse expectedResponse = - new JsonRpcErrorResponse(validRequest.getId(), JsonRpcError.CANT_CONNECT_TO_LOCAL_PEER); - - final JsonRpcResponse actualResponse = method.response(validRequest); - - assertThat(actualResponse).isEqualToComparingFieldByField(expectedResponse); - } } diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/ConnectingToLocalNodeException.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/ConnectingToLocalNodeException.java deleted file mode 100644 index 96a898ca3a..0000000000 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/ConnectingToLocalNodeException.java +++ /dev/null @@ -1,23 +0,0 @@ -/* - * 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; - -public class ConnectingToLocalNodeException extends RuntimeException { - public ConnectingToLocalNodeException(final String message) { - super(message); - } - - public ConnectingToLocalNodeException() { - super("Cannot add the local node as a peer connection"); - } -} diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/PeerNotPermittedException.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/PeerNotPermittedException.java deleted file mode 100644 index c14955f605..0000000000 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/PeerNotPermittedException.java +++ /dev/null @@ -1,23 +0,0 @@ -/* - * 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; - -public class PeerNotPermittedException extends RuntimeException { - public PeerNotPermittedException(final String message) { - super(message); - } - - public PeerNotPermittedException() { - super("Cannot add a peer that is not permitted"); - } -} diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java b/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java index 836ec3015b..eb0de4def9 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java @@ -41,7 +41,6 @@ import tech.pegasys.pantheon.ethereum.jsonrpc.websocket.subscription.pending.PendingTransactionSubscriptionService; import tech.pegasys.pantheon.ethereum.jsonrpc.websocket.subscription.syncing.SyncingSubscriptionService; import tech.pegasys.pantheon.ethereum.mainnet.ProtocolSchedule; -import tech.pegasys.pantheon.ethereum.p2p.ConnectingToLocalNodeException; import tech.pegasys.pantheon.ethereum.p2p.InsufficientPeersPermissioningProvider; import tech.pegasys.pantheon.ethereum.p2p.NetworkRunner; import tech.pegasys.pantheon.ethereum.p2p.NetworkRunner.NetworkBuilder; @@ -54,7 +53,6 @@ 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.Peer; import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; import tech.pegasys.pantheon.ethereum.p2p.wire.Capability; import tech.pegasys.pantheon.ethereum.p2p.wire.SubProtocol; @@ -310,14 +308,9 @@ public Runner build() { final P2PNetwork peerNetwork = networkRunner.getNetwork(); - staticNodes.forEach( - enodeURL -> { - final Peer peer = DefaultPeer.fromEnodeURL(enodeURL); - try { - peerNetwork.addMaintainConnectionPeer(peer); - } catch (ConnectingToLocalNodeException ex) { - } - }); + staticNodes.stream() + .map(DefaultPeer::fromEnodeURL) + .forEach(peerNetwork::addMaintainConnectionPeer); Optional jsonRpcHttpService = Optional.empty(); if (jsonRpcConfiguration.isEnabled()) {