From a024bf4dbd4c9d9bec90bbd4ef8ab24570e7d79d Mon Sep 17 00:00:00 2001 From: mbaxter Date: Tue, 16 Apr 2019 00:00:12 -0400 Subject: [PATCH] [PAN-2560] Consolidate p2p node info methods (#1288) --- .../ethereum/eth/transactions/TestNode.java | 3 +- .../ethereum/jsonrpc/JsonRpcHttpService.java | 2 +- .../internal/methods/AdminModifyPeer.java | 7 -- .../internal/methods/AdminNodeInfo.java | 87 ++++++++----------- .../jsonrpc/internal/methods/AdminPeers.java | 7 -- .../jsonrpc/internal/methods/NetEnode.java | 4 +- .../internal/response/JsonRpcError.java | 2 +- .../internal/methods/AdminNodeInfoTest.java | 42 +++++++-- .../internal/methods/NetEnodeTest.java | 6 +- .../ethereum/p2p/testing/MockNetwork.java | 19 ++-- .../pantheon/ethereum/p2p/NoopP2PNetwork.java | 16 ++-- .../pantheon/ethereum/p2p/api/P2PNetwork.java | 15 +--- .../ethereum/p2p/netty/NettyP2PNetwork.java | 23 +++-- .../p2p/NetworkingServiceLifecycleTest.java | 18 ++-- .../p2p/netty/NettyP2PNetworkTest.java | 70 +++++++++------ .../java/tech/pegasys/pantheon/Runner.java | 25 +++--- .../tech/pegasys/pantheon/RunnerTest.java | 7 +- .../pegasys/pantheon/util/enode/EnodeURL.java | 8 ++ .../pantheon/util/enode/EnodeURLTest.java | 30 +++++++ 19 files changed, 213 insertions(+), 178 deletions(-) diff --git a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TestNode.java b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TestNode.java index 3ebad80906..a35cf5a7ec 100644 --- a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TestNode.java +++ b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/TestNode.java @@ -138,7 +138,6 @@ public TestNode( .metricsSystem(new NoOpMetricsSystem()) .build(); network = networkRunner.getNetwork(); - this.port = network.getLocalPeerInfo().getPort(); network.subscribeDisconnect( (connection, reason, initiatedByPeer) -> disconnections.put(connection, reason)); @@ -157,7 +156,7 @@ public TestNode( metricsSystem, syncState); networkRunner.start(); - + this.port = network.getLocalEnode().get().getListeningPort(); selfPeer = new DefaultPeer(id(), endpoint()); } diff --git a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/JsonRpcHttpService.java b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/JsonRpcHttpService.java index 47a9d7fde5..29528c4c35 100644 --- a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/JsonRpcHttpService.java +++ b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/JsonRpcHttpService.java @@ -455,7 +455,7 @@ private JsonRpcResponse process(final JsonObject requestJson, final Optional response = new HashMap<>(); + final Map ports = new HashMap<>(); - try { - final Map response = new HashMap<>(); - final Map ports = new HashMap<>(); + if (!peerNetwork.isP2pEnabled()) { + return new JsonRpcErrorResponse(req.getId(), JsonRpcError.P2P_DISABLED); + } + final Optional maybeEnode = peerNetwork.getLocalEnode(); + if (!maybeEnode.isPresent()) { + return new JsonRpcErrorResponse(req.getId(), JsonRpcError.P2P_NETWORK_NOT_RUNNING); + } + final EnodeURL enode = maybeEnode.get(); - final PeerInfo peerInfo = peerNetwork.getLocalPeerInfo(); - final BytesValue nodeId = peerInfo.getNodeId(); - peerNetwork - .getAdvertisedPeer() - .ifPresent( - advertisedPeer -> { - response.put("enode", advertisedPeer.getEnodeURLString()); - ports.put("discovery", advertisedPeer.getEndpoint().getUdpPort()); - response.put("ip", advertisedPeer.getEndpoint().getHost()); - response.put( - "listenAddr", - advertisedPeer.getEndpoint().getHost() + ":" + peerInfo.getPort()); - }); - response.put("id", nodeId.toString().substring(2)); - response.put("name", clientVersion); + final BytesValue nodeId = enode.getNodeId(); + response.put("enode", enode.toString()); + ports.put("discovery", enode.getEffectiveDiscoveryPort()); + response.put("ip", enode.getIp()); + response.put("listenAddr", enode.getIp() + ":" + enode.getListeningPort()); + response.put("id", nodeId.toUnprefixedString()); + response.put("name", clientVersion); - ports.put("listener", peerInfo.getPort()); - response.put("ports", ports); + ports.put("listener", enode.getListeningPort()); + response.put("ports", ports); - final ChainHead chainHead = blockchainQueries.getBlockchain().getChainHead(); - response.put( - "protocols", - ImmutableMap.of( - "eth", - ImmutableMap.of( - "config", - genesisConfigOptions.asMap(), - "difficulty", - chainHead.getTotalDifficulty().toLong(), - "genesis", - blockchainQueries.getBlockHashByNumber(0).get().toString(), - "head", - chainHead.getHash().toString(), - "network", - networkId))); + final ChainHead chainHead = blockchainQueries.getBlockchain().getChainHead(); + response.put( + "protocols", + ImmutableMap.of( + "eth", + ImmutableMap.of( + "config", + genesisConfigOptions.asMap(), + "difficulty", + chainHead.getTotalDifficulty().toLong(), + "genesis", + blockchainQueries.getBlockHashByNumber(0).get().toString(), + "head", + chainHead.getHash().toString(), + "network", + networkId))); - return new JsonRpcSuccessResponse(req.getId(), response); - } catch (final P2pDisabledException e) { - return new JsonRpcErrorResponse(req.getId(), JsonRpcError.P2P_DISABLED); - } catch (final Exception e) { - LOG.error("Error processing request: " + req, e); - throw e; - } + return new JsonRpcSuccessResponse(req.getId(), response); } } diff --git a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/AdminPeers.java b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/AdminPeers.java index 4d9a77d566..11e70a6a74 100644 --- a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/AdminPeers.java +++ b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/AdminPeers.java @@ -24,11 +24,7 @@ import java.util.List; import java.util.stream.Collectors; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; - public class AdminPeers implements JsonRpcMethod { - private static final Logger LOG = LogManager.getLogger(); private final P2PNetwork peerDiscoveryAgent; public AdminPeers(final P2PNetwork peerDiscoveryAgent) { @@ -50,9 +46,6 @@ public JsonRpcResponse response(final JsonRpcRequest req) { return result; } catch (P2pDisabledException e) { return new JsonRpcErrorResponse(req.getId(), JsonRpcError.P2P_DISABLED); - } catch (final Exception e) { - LOG.error("Error processing request: " + req, e); - throw e; } } } diff --git a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/NetEnode.java b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/NetEnode.java index 5006644422..0d5bc107c7 100644 --- a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/NetEnode.java +++ b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/NetEnode.java @@ -41,7 +41,7 @@ public JsonRpcResponse response(final JsonRpcRequest req) { return p2pDisabledResponse(req); } - final Optional enodeURL = p2pNetwork.getSelfEnodeURL(); + final Optional enodeURL = p2pNetwork.getLocalEnode(); if (!enodeURL.isPresent()) { return enodeUrlNotAvailable(req); } @@ -54,6 +54,6 @@ private JsonRpcErrorResponse p2pDisabledResponse(final JsonRpcRequest req) { } private JsonRpcErrorResponse enodeUrlNotAvailable(final JsonRpcRequest req) { - return new JsonRpcErrorResponse(req.getId(), JsonRpcError.ENODE_NOT_AVAILABLE); + return new JsonRpcErrorResponse(req.getId(), JsonRpcError.P2P_NETWORK_NOT_RUNNING); } } diff --git a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/response/JsonRpcError.java b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/response/JsonRpcError.java index 6c73ba1d2e..eb0768e364 100644 --- a/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/response/JsonRpcError.java +++ b/ethereum/jsonrpc/src/main/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/response/JsonRpcError.java @@ -28,7 +28,7 @@ public enum JsonRpcError { // P2P related errors P2P_DISABLED(-32000, "P2P has been disabled. This functionality is not available"), - ENODE_NOT_AVAILABLE(-32000, "Enode URL not available"), + P2P_NETWORK_NOT_RUNNING(-32000, "P2P network is not running"), // Filter & Subscription Errors FILTER_NOT_FOUND(-32000, "Filter not found"), diff --git a/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/AdminNodeInfoTest.java b/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/AdminNodeInfoTest.java index 3a61914e7c..cb4c23486c 100644 --- a/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/AdminNodeInfoTest.java +++ b/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/AdminNodeInfoTest.java @@ -14,7 +14,6 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.anyLong; -import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.when; import tech.pegasys.pantheon.config.GenesisConfigOptions; @@ -24,14 +23,15 @@ import tech.pegasys.pantheon.ethereum.core.Hash; import tech.pegasys.pantheon.ethereum.jsonrpc.internal.JsonRpcRequest; import tech.pegasys.pantheon.ethereum.jsonrpc.internal.queries.BlockchainQueries; +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.api.P2PNetwork; import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeer; -import tech.pegasys.pantheon.ethereum.p2p.wire.PeerInfo; import tech.pegasys.pantheon.util.bytes.BytesValue; import tech.pegasys.pantheon.util.uint.UInt256; -import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.Optional; @@ -55,7 +55,6 @@ public class AdminNodeInfoTest { private final BytesValue nodeId = BytesValue.fromHexString( "0x0f1b319e32017c3fcb221841f0f978701b4e9513fe6a567a2db43d43381a9c7e3dfe7cae13cbc2f56943400bacaf9082576ab087cd51983b17d729ae796f6807"); - private final PeerInfo localPeer = new PeerInfo(5, "0x0", Collections.emptyList(), 30303, nodeId); private final ChainHead testChainHead = new ChainHead(Hash.EMPTY, UInt256.ONE); private final GenesisConfigOptions genesisConfigOptions = new StubGenesisConfigOptions().chainId(2019); @@ -63,8 +62,6 @@ public class AdminNodeInfoTest { @Before public void setup() { - when(p2pNetwork.getLocalPeerInfo()).thenReturn(localPeer); - doReturn(Optional.of(this.defaultPeer)).when(p2pNetwork).getAdvertisedPeer(); when(blockchainQueries.getBlockchain()).thenReturn(blockchain); when(blockchainQueries.getBlockHashByNumber(anyLong())).thenReturn(Optional.of(Hash.EMPTY)); when(blockchain.getChainHead()).thenReturn(testChainHead); @@ -76,9 +73,10 @@ public void setup() { @Test public void shouldReturnCorrectResult() { + when(p2pNetwork.isP2pEnabled()).thenReturn(true); + when(p2pNetwork.getLocalEnode()).thenReturn(Optional.of(defaultPeer.getEnodeURL())); final JsonRpcRequest request = adminNodeInfo(); - final JsonRpcSuccessResponse actual = (JsonRpcSuccessResponse) method.response(request); final Map expected = new HashMap<>(); expected.put( "enode", @@ -106,9 +104,39 @@ public void shouldReturnCorrectResult() { "network", 2018))); + final JsonRpcResponse response = method.response(request); + assertThat(response).isInstanceOf(JsonRpcSuccessResponse.class); + final JsonRpcSuccessResponse actual = (JsonRpcSuccessResponse) response; assertThat(actual.getResult()).isEqualTo(expected); } + @Test + public void returnsErrorWhenP2PDisabled() { + when(p2pNetwork.isP2pEnabled()).thenReturn(false); + final JsonRpcRequest request = adminNodeInfo(); + + final JsonRpcResponse expectedResponse = + new JsonRpcErrorResponse(request.getId(), JsonRpcError.P2P_DISABLED); + + final JsonRpcResponse response = method.response(request); + assertThat(response).isInstanceOf(JsonRpcErrorResponse.class); + assertThat(response).isEqualToComparingFieldByField(expectedResponse); + } + + @Test + public void returnsErrorWhenP2PNotReady() { + when(p2pNetwork.isP2pEnabled()).thenReturn(true); + when(p2pNetwork.getLocalEnode()).thenReturn(Optional.empty()); + final JsonRpcRequest request = adminNodeInfo(); + + final JsonRpcResponse expectedResponse = + new JsonRpcErrorResponse(request.getId(), JsonRpcError.P2P_NETWORK_NOT_RUNNING); + + final JsonRpcResponse response = method.response(request); + assertThat(response).isInstanceOf(JsonRpcErrorResponse.class); + assertThat(response).isEqualToComparingFieldByField(expectedResponse); + } + private JsonRpcRequest adminNodeInfo() { return new JsonRpcRequest("2.0", "admin_nodeInfo", new Object[] {}); } diff --git a/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/NetEnodeTest.java b/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/NetEnodeTest.java index 1efd346e44..7b2338d2ba 100644 --- a/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/NetEnodeTest.java +++ b/ethereum/jsonrpc/src/test/java/tech/pegasys/pantheon/ethereum/jsonrpc/internal/methods/NetEnodeTest.java @@ -61,7 +61,7 @@ public void shouldReturnExpectedMethodName() { @Test public void shouldReturnEnode() { when(p2PNetwork.isP2pEnabled()).thenReturn(true); - doReturn(enodeURL).when(p2PNetwork).getSelfEnodeURL(); + doReturn(enodeURL).when(p2PNetwork).getLocalEnode(); final JsonRpcRequest request = netEnodeRequest(); final JsonRpcResponse expectedResponse = @@ -84,11 +84,11 @@ public void shouldReturnErrorWhenP2pDisabled() { @Test public void shouldReturnErrorWhenP2PEnabledButNoEnodeFound() { when(p2PNetwork.isP2pEnabled()).thenReturn(true); - doReturn(Optional.empty()).when(p2PNetwork).getSelfEnodeURL(); + doReturn(Optional.empty()).when(p2PNetwork).getLocalEnode(); final JsonRpcRequest request = netEnodeRequest(); final JsonRpcResponse expectedResponse = - new JsonRpcErrorResponse(request.getId(), JsonRpcError.ENODE_NOT_AVAILABLE); + new JsonRpcErrorResponse(request.getId(), JsonRpcError.P2P_NETWORK_NOT_RUNNING); assertThat(method.response(request)).isEqualToComparingFieldByField(expectedResponse); } diff --git a/ethereum/mock-p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/testing/MockNetwork.java b/ethereum/mock-p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/testing/MockNetwork.java index c4b9766f55..ff576361aa 100644 --- a/ethereum/mock-p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/testing/MockNetwork.java +++ b/ethereum/mock-p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/testing/MockNetwork.java @@ -17,7 +17,6 @@ import tech.pegasys.pantheon.ethereum.p2p.api.MessageData; import tech.pegasys.pantheon.ethereum.p2p.api.P2PNetwork; import tech.pegasys.pantheon.ethereum.p2p.api.PeerConnection; -import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeer; import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; import tech.pegasys.pantheon.ethereum.p2p.wire.Capability; import tech.pegasys.pantheon.ethereum.p2p.wire.DefaultMessage; @@ -100,7 +99,7 @@ private void disconnect( } } - private final class MockP2PNetwork implements P2PNetwork { + private static final class MockP2PNetwork implements P2PNetwork { private final MockNetwork network; @@ -178,11 +177,6 @@ public void stop() {} @Override public void awaitStop() {} - @Override - public Optional getAdvertisedPeer() { - return Optional.of(new DefaultPeer(self.getId(), "127.0.0.1", 0, 0)); - } - @Override public void start() {} @@ -190,23 +184,22 @@ public void start() {} public void close() {} @Override - public PeerInfo getLocalPeerInfo() { - return new PeerInfo( - 5, self.getId().toString(), new ArrayList<>(capabilities), 0, self.getId()); + public boolean isListening() { + return true; } @Override - public boolean isListening() { + public boolean isP2pEnabled() { return true; } @Override - public boolean isP2pEnabled() { + public boolean isDiscoveryEnabled() { return true; } @Override - public Optional getSelfEnodeURL() { + public Optional getLocalEnode() { return Optional.empty(); } } diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/NoopP2PNetwork.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/NoopP2PNetwork.java index 97bd2c0361..0ea9d37b97 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/NoopP2PNetwork.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/NoopP2PNetwork.java @@ -18,7 +18,6 @@ import tech.pegasys.pantheon.ethereum.p2p.api.PeerConnection; import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; import tech.pegasys.pantheon.ethereum.p2p.wire.Capability; -import tech.pegasys.pantheon.ethereum.p2p.wire.PeerInfo; import tech.pegasys.pantheon.util.enode.EnodeURL; import java.io.IOException; @@ -64,27 +63,22 @@ public void stop() {} public void awaitStop() {} @Override - public Optional getAdvertisedPeer() { - return Optional.empty(); - } - - @Override - public PeerInfo getLocalPeerInfo() { - throw new P2pDisabledException("P2P networking disabled. Local peer info unavailable."); + public boolean isListening() { + return false; } @Override - public boolean isListening() { + public boolean isP2pEnabled() { return false; } @Override - public boolean isP2pEnabled() { + public boolean isDiscoveryEnabled() { return false; } @Override - public Optional getSelfEnodeURL() { + public Optional getLocalEnode() { return Optional.empty(); } 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 df7bb6791c..d1989ac30a 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 @@ -14,7 +14,6 @@ import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; import tech.pegasys.pantheon.ethereum.p2p.wire.Capability; -import tech.pegasys.pantheon.ethereum.p2p.wire.PeerInfo; import tech.pegasys.pantheon.util.enode.EnodeURL; import java.io.Closeable; @@ -92,15 +91,6 @@ public interface P2PNetwork extends Closeable { /** Blocks until the P2P network layer has stopped. */ void awaitStop(); - Optional getAdvertisedPeer(); - - /** - * Returns {@link PeerInfo} object for this node - * - * @return the PeerInfo for this node. - */ - PeerInfo getLocalPeerInfo(); - /** * Checks if the node is listening for network connections * @@ -115,11 +105,14 @@ public interface P2PNetwork extends Closeable { */ boolean isP2pEnabled(); + /** @return Return true if peer discovery is enabled. */ + boolean isDiscoveryEnabled(); + /** * Returns the EnodeURL used to identify this peer in the network. * * @return the enodeURL associated with this node if P2P has been enabled. Returns empty * otherwise. */ - Optional getSelfEnodeURL(); + Optional getLocalEnode(); } diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/netty/NettyP2PNetwork.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/netty/NettyP2PNetwork.java index ed39ee5d88..7faee64be3 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/netty/NettyP2PNetwork.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/netty/NettyP2PNetwork.java @@ -145,6 +145,7 @@ public class NettyP2PNetwork implements P2PNetwork { private final PeerDiscoveryAgent peerDiscoveryAgent; private final PeerBlacklist peerBlacklist; + private final NetworkingConfiguration config; private OptionalLong peerBondedObserverId = OptionalLong.empty(); private OptionalLong peerDroppedObserverId = OptionalLong.empty(); @@ -174,7 +175,7 @@ public class NettyP2PNetwork implements P2PNetwork { private final String advertisedHost; - private EnodeURL ourEnodeURL; + private volatile EnodeURL ourEnodeURL; private final Optional nodePermissioningController; private final Optional blockchain; @@ -229,6 +230,7 @@ public NettyP2PNetwork( final Optional nodePermissioningController, final Blockchain blockchain) { + this.config = config; maxPeers = config.getRlpx().getMaxPeers(); connections = new PeerConnectionRegistry(metricsSystem); this.peerBlacklist = peerBlacklist; @@ -631,7 +633,7 @@ private boolean isPeerConnectionAllowed(final PeerConnection peerConnection) { return nodePermissioningController .map( c -> { - final EnodeURL localPeerEnodeURL = getSelfEnodeURL().orElse(buildSelfEnodeURL()); + final EnodeURL localPeerEnodeURL = getLocalEnode().orElse(buildSelfEnodeURL()); final EnodeURL remotePeerEnodeURL = peerConnection.getRemoteEnode(); return c.isPermitted(localPeerEnodeURL, remotePeerEnodeURL); }) @@ -697,16 +699,6 @@ public Stream getDiscoveryPeers() { return peerDiscoveryAgent.getPeers(); } - @Override - public Optional getAdvertisedPeer() { - return peerDiscoveryAgent.getAdvertisedPeer(); - } - - @Override - public PeerInfo getLocalPeerInfo() { - return ourPeerInfo; - } - @Override public boolean isListening() { return peerDiscoveryAgent.isActive(); @@ -718,7 +710,12 @@ public boolean isP2pEnabled() { } @Override - public Optional getSelfEnodeURL() { + public boolean isDiscoveryEnabled() { + return config.getDiscovery().isActive(); + } + + @Override + public Optional getLocalEnode() { return Optional.ofNullable(ourEnodeURL); } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/NetworkingServiceLifecycleTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/NetworkingServiceLifecycleTest.java index 5750d16f5c..f869b59999 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/NetworkingServiceLifecycleTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/NetworkingServiceLifecycleTest.java @@ -27,10 +27,10 @@ import tech.pegasys.pantheon.ethereum.p2p.netty.NettyP2PNetwork; import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem; +import tech.pegasys.pantheon.util.enode.EnodeURL; import java.io.IOException; import java.util.Optional; -import java.util.OptionalInt; import io.vertx.core.Vertx; import org.assertj.core.api.Assertions; @@ -61,15 +61,13 @@ public void createP2PNetwork() { Optional.empty(), Optional.empty())) { service.start(); - final int udpPort = service.getAdvertisedPeer().get().getEndpoint().getUdpPort(); - final OptionalInt tcpPort = service.getAdvertisedPeer().get().getEndpoint().getTcpPort(); + final EnodeURL enode = service.getLocalEnode().get(); + final int udpPort = enode.getEffectiveDiscoveryPort(); + final int tcpPort = enode.getListeningPort(); - assertEquals( - config.getDiscovery().getAdvertisedHost(), - service.getAdvertisedPeer().get().getEndpoint().getHost()); + assertEquals(config.getDiscovery().getAdvertisedHost(), enode.getIp()); assertThat(udpPort).isNotZero(); - assertThat(tcpPort).isPresent(); - assertThat(tcpPort.getAsInt()).isNotZero(); + assertThat(tcpPort).isNotZero(); assertThat(service.getDiscoveryPeers()).hasSize(0); } } @@ -213,9 +211,7 @@ public void startDiscoveryPortInUse() { Optional.empty())) { service1.start(); final NetworkingConfiguration config = configWithRandomPorts(); - config - .getDiscovery() - .setBindPort(service1.getAdvertisedPeer().get().getEndpoint().getUdpPort()); + config.getDiscovery().setBindPort(service1.getLocalEnode().get().getEffectiveDiscoveryPort()); try (final NettyP2PNetwork service2 = new NettyP2PNetwork( vertx, diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/netty/NettyP2PNetworkTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/netty/NettyP2PNetworkTest.java index a648cc5bf5..6a5251d2b1 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/netty/NettyP2PNetworkTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/netty/NettyP2PNetworkTest.java @@ -141,10 +141,12 @@ public void handshaking() throws Exception { Optional.empty(), Optional.empty())) { - final int listenPort = listener.getLocalPeerInfo().getPort(); listener.start(); connector.start(); - final BytesValue listenId = listenKp.getPublicKey().getEncodedBytes(); + final EnodeURL listenerEnode = listener.getLocalEnode().get(); + final BytesValue listenId = listenerEnode.getNodeId(); + final int listenPort = listenerEnode.getListeningPort(); + assertThat( connector .connect( @@ -194,10 +196,13 @@ public void preventMultipleConnections() throws Exception { new NoOpMetricsSystem(), Optional.empty(), Optional.empty())) { - final int listenPort = listener.getLocalPeerInfo().getPort(); + listener.start(); connector.start(); - final BytesValue listenId = listenKp.getPublicKey().getEncodedBytes(); + final EnodeURL listenerEnode = listener.getLocalEnode().get(); + final BytesValue listenId = listenerEnode.getNodeId(); + final int listenPort = listenerEnode.getListeningPort(); + assertThat( connector .connect( @@ -277,11 +282,13 @@ public void limitMaxPeers() throws Exception { Optional.empty(), Optional.empty())) { - final int listenPort = listener.getLocalPeerInfo().getPort(); // Setup listener and first connection listener.start(); connector1.start(); - final BytesValue listenId = listenKp.getPublicKey().getEncodedBytes(); + final EnodeURL listenerEnode = listener.getLocalEnode().get(); + final BytesValue listenId = listenerEnode.getNodeId(); + final int listenPort = listenerEnode.getListeningPort(); + final Peer listeningPeer = new DefaultPeer( listenId, @@ -355,10 +362,12 @@ public void rejectPeerWithNoSharedCaps() throws Exception { new NoOpMetricsSystem(), Optional.empty(), Optional.empty())) { - final int listenPort = listener.getLocalPeerInfo().getPort(); + listener.start(); connector.start(); - final BytesValue listenId = listenKp.getPublicKey().getEncodedBytes(); + final EnodeURL listenerEnode = listener.getLocalEnode().get(); + final BytesValue listenId = listenerEnode.getNodeId(); + final int listenPort = listenerEnode.getListeningPort(); final Peer listenerPeer = new DefaultPeer( @@ -377,8 +386,6 @@ public void rejectIncomingConnectionFromBlacklistedPeer() throws Exception { final DiscoveryConfiguration noDiscovery = DiscoveryConfiguration.create().setActive(false); final SECP256K1.KeyPair localKp = SECP256K1.KeyPair.generate(); final SECP256K1.KeyPair remoteKp = SECP256K1.KeyPair.generate(); - final BytesValue localId = localKp.getPublicKey().getEncodedBytes(); - final BytesValue remoteId = remoteKp.getPublicKey().getEncodedBytes(); final PeerBlacklist localBlacklist = new PeerBlacklist(); final PeerBlacklist remoteBlacklist = new PeerBlacklist(); @@ -410,23 +417,33 @@ public void rejectIncomingConnectionFromBlacklistedPeer() throws Exception { new NoOpMetricsSystem(), Optional.empty(), Optional.empty())) { - final int localListenPort = localNetwork.getLocalPeerInfo().getPort(); - final int remoteListenPort = remoteNetwork.getLocalPeerInfo().getPort(); + + localNetwork.start(); + remoteNetwork.start(); + + final EnodeURL localEnode = localNetwork.getLocalEnode().get(); + final BytesValue localId = localEnode.getNodeId(); + final int localPort = localEnode.getListeningPort(); + + final EnodeURL remoteEnode = remoteNetwork.getLocalEnode().get(); + final BytesValue remoteId = remoteEnode.getNodeId(); + final int remotePort = remoteEnode.getListeningPort(); + final Peer localPeer = new DefaultPeer( localId, new Endpoint( InetAddress.getLoopbackAddress().getHostAddress(), - localListenPort, - OptionalInt.of(localListenPort))); + localPort, + OptionalInt.of(localPort))); final Peer remotePeer = new DefaultPeer( remoteId, new Endpoint( InetAddress.getLoopbackAddress().getHostAddress(), - remoteListenPort, - OptionalInt.of(remoteListenPort))); + remotePort, + OptionalInt.of(remotePort))); // Blacklist the remote peer localBlacklist.add(remotePeer); @@ -460,7 +477,6 @@ public void rejectIncomingConnectionFromNonWhitelistedPeer() throws Exception { final DiscoveryConfiguration noDiscovery = DiscoveryConfiguration.create().setActive(false); final SECP256K1.KeyPair localKp = SECP256K1.KeyPair.generate(); final SECP256K1.KeyPair remoteKp = SECP256K1.KeyPair.generate(); - final BytesValue localId = localKp.getPublicKey().getEncodedBytes(); final PeerBlacklist localBlacklist = new PeerBlacklist(); final PeerBlacklist remoteBlacklist = new PeerBlacklist(); final LocalPermissioningConfiguration config = LocalPermissioningConfiguration.createDefault(); @@ -505,17 +521,21 @@ public void rejectIncomingConnectionFromNonWhitelistedPeer() throws Exception { new NoOpMetricsSystem(), Optional.empty(), Optional.empty())) { - final int localListenPort = localNetwork.getLocalPeerInfo().getPort(); + + localNetwork.start(); + remoteNetwork.start(); + + final EnodeURL localEnode = localNetwork.getLocalEnode().get(); + final BytesValue localId = localEnode.getNodeId(); + final int localPort = localEnode.getListeningPort(); + final Peer localPeer = new DefaultPeer( localId, new Endpoint( InetAddress.getLoopbackAddress().getHostAddress(), - localListenPort, - OptionalInt.of(localListenPort))); - - localNetwork.start(); - remoteNetwork.start(); + localPort, + OptionalInt.of(localPort))); // Setup disconnect listener final CompletableFuture peerFuture = new CompletableFuture<>(); @@ -802,7 +822,7 @@ public void removePeerReturnsFalseIfNotInMaintainedListButDisconnectsPeer() { public void beforeStartingNetworkEnodeURLShouldNotBePresent() { final NettyP2PNetwork nettyP2PNetwork = mockNettyP2PNetwork(); - assertThat(nettyP2PNetwork.getSelfEnodeURL()).isNotPresent(); + assertThat(nettyP2PNetwork.getLocalEnode()).isNotPresent(); } @Test @@ -810,7 +830,7 @@ public void afterStartingNetworkEnodeURLShouldBePresent() { final NettyP2PNetwork nettyP2PNetwork = mockNettyP2PNetwork(); nettyP2PNetwork.start(); - assertThat(nettyP2PNetwork.getSelfEnodeURL()).isPresent(); + assertThat(nettyP2PNetwork.getLocalEnode()).isPresent(); } @Test diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/Runner.java b/pantheon/src/main/java/tech/pegasys/pantheon/Runner.java index b4a5b19f66..b344fe1ffd 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/Runner.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/Runner.java @@ -16,9 +16,9 @@ import tech.pegasys.pantheon.ethereum.jsonrpc.JsonRpcHttpService; import tech.pegasys.pantheon.ethereum.jsonrpc.websocket.WebSocketService; import tech.pegasys.pantheon.ethereum.p2p.NetworkRunner; -import tech.pegasys.pantheon.ethereum.p2p.peers.Endpoint; -import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; +import tech.pegasys.pantheon.ethereum.p2p.api.P2PNetwork; import tech.pegasys.pantheon.metrics.prometheus.MetricsService; +import tech.pegasys.pantheon.util.enode.EnodeURL; import java.io.File; import java.io.FileOutputStream; @@ -30,6 +30,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import com.google.common.annotations.VisibleForTesting; import io.vertx.core.Vertx; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -143,18 +144,19 @@ private void waitForServiceToStart( private void writePantheonPortsToFile() { final Properties properties = new Properties(); - + final P2PNetwork network = networkRunner.getNetwork(); if (networkRunner.getNetwork().isP2pEnabled()) { networkRunner .getNetwork() - .getAdvertisedPeer() + .getLocalEnode() .ifPresent( - advertisedPeer -> { - final Endpoint endpoint = advertisedPeer.getEndpoint(); - properties.setProperty("discovery", String.valueOf(endpoint.getUdpPort())); + enode -> { + if (network.isDiscoveryEnabled()) { + properties.setProperty( + "discovery", String.valueOf(enode.getEffectiveDiscoveryPort())); + } + properties.setProperty("p2p", String.valueOf(enode.getListeningPort())); }); - final int tcpPort = networkRunner.getNetwork().getLocalPeerInfo().getPort(); - properties.setProperty("p2p", String.valueOf(tcpPort)); } if (getJsonRpcPort().isPresent()) { @@ -195,7 +197,8 @@ public Optional getMetricsPort() { } } - public Optional getAdvertisedPeer() { - return networkRunner.getNetwork().getAdvertisedPeer(); + @VisibleForTesting + Optional getLocalEnode() { + return networkRunner.getNetwork().getLocalEnode(); } } diff --git a/pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java b/pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java index 258229cb20..18c2f09eef 100644 --- a/pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java +++ b/pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java @@ -40,7 +40,6 @@ import tech.pegasys.pantheon.ethereum.mainnet.MainnetProtocolSchedule; import tech.pegasys.pantheon.ethereum.mainnet.ProtocolSchedule; import tech.pegasys.pantheon.ethereum.mainnet.ProtocolSpec; -import tech.pegasys.pantheon.ethereum.p2p.peers.Peer; import tech.pegasys.pantheon.ethereum.storage.StorageProvider; import tech.pegasys.pantheon.ethereum.storage.keyvalue.RocksDbStorageProvider; import tech.pegasys.pantheon.metrics.MetricsSystem; @@ -48,11 +47,11 @@ import tech.pegasys.pantheon.metrics.prometheus.MetricsConfiguration; import tech.pegasys.pantheon.services.kvstore.RocksDbConfiguration; import tech.pegasys.pantheon.testutil.TestClock; +import tech.pegasys.pantheon.util.enode.EnodeURL; import tech.pegasys.pantheon.util.uint.UInt256; import java.io.IOException; import java.net.InetAddress; -import java.net.URI; import java.nio.file.Path; import java.time.Duration; import java.util.Collections; @@ -193,12 +192,12 @@ private void syncFromGenesis(final SyncMode mode) throws Exception { noOpMetricsSystem, TestClock.fixed(), PendingTransactions.MAX_PENDING_TRANSACTIONS); - final Peer advertisedPeer = runnerAhead.getAdvertisedPeer().get(); + final EnodeURL enode = runnerAhead.getLocalEnode().get(); final EthNetworkConfig behindEthNetworkConfiguration = new EthNetworkConfig( EthNetworkConfig.jsonConfig(DEV), DEV_NETWORK_ID, - Collections.singletonList(URI.create(advertisedPeer.getEnodeURLString()))); + Collections.singletonList(enode.toURI())); runnerBehind = runnerBuilder .pantheonController(controllerBehind) diff --git a/util/src/main/java/tech/pegasys/pantheon/util/enode/EnodeURL.java b/util/src/main/java/tech/pegasys/pantheon/util/enode/EnodeURL.java index 06e7b2c134..2a97f0072a 100644 --- a/util/src/main/java/tech/pegasys/pantheon/util/enode/EnodeURL.java +++ b/util/src/main/java/tech/pegasys/pantheon/util/enode/EnodeURL.java @@ -164,6 +164,14 @@ public OptionalInt getDiscoveryPort() { return discoveryPort; } + /** + * @return Returns the discovery port if explicitly specified, otherwise returns the listening + * port. + */ + public int getEffectiveDiscoveryPort() { + return discoveryPort.orElse(listeningPort); + } + public boolean sameEndpoint(final EnodeURL enode) { return Objects.equal(nodeId, enode.nodeId) && Objects.equal(ip, enode.ip) diff --git a/util/src/test/java/tech/pegasys/pantheon/util/enode/EnodeURLTest.java b/util/src/test/java/tech/pegasys/pantheon/util/enode/EnodeURLTest.java index a724df42b4..6d93396866 100644 --- a/util/src/test/java/tech/pegasys/pantheon/util/enode/EnodeURLTest.java +++ b/util/src/test/java/tech/pegasys/pantheon/util/enode/EnodeURLTest.java @@ -275,4 +275,34 @@ public void toURI_WithoutDiscoveryPortCreateExpectedURI() { assertThat(createdURI).isEqualTo(expectedURI); } + + @Test + public void getEffectiveDiscoveryPort_withMatchingDiscoveryAndListeningPorts() { + final EnodeURL enode = + EnodeURL.builder() + .nodeId(VALID_NODE_ID) + .ipAddress(IPV4_ADDRESS) + .listeningPort(P2P_PORT) + .discoveryPort(OptionalInt.of(P2P_PORT)) + .build(); + assertThat(enode.getListeningPort()).isEqualTo(P2P_PORT); + // A discovery port matching the listening port should not be explicitly specified + assertThat(enode.getDiscoveryPort()).isEmpty(); + assertThat(enode.getEffectiveDiscoveryPort()).isEqualTo(P2P_PORT); + } + + @Test + public void getEffectiveDiscoveryPort_withDistinctDiscoveryAndListeningPorts() { + final EnodeURL enode = + EnodeURL.builder() + .nodeId(VALID_NODE_ID) + .ipAddress(IPV4_ADDRESS) + .listeningPort(P2P_PORT) + .discoveryPort(OptionalInt.of(DISCOVERY_PORT)) + .build(); + assertThat(enode.getListeningPort()).isEqualTo(P2P_PORT); + // A discovery port matching the listening port should not be explicitly specified + assertThat(enode.getDiscoveryPort()).isEqualTo(OptionalInt.of(DISCOVERY_PORT)); + assertThat(enode.getEffectiveDiscoveryPort()).isEqualTo(DISCOVERY_PORT); + } }