diff --git a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java index dcd31b7e4f1..4efc1267520 100644 --- a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java +++ b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java @@ -189,12 +189,22 @@ public void start() { return; } + final int configuredDiscoveryPort = config.getDiscovery().getBindPort(); + final int configuredRlpxPort = config.getRlpx().getBindPort(); + + final int listeningPort = rlpxAgent.start().join(); + final int discoveryPort = + peerDiscoveryAgent + .start( + (configuredDiscoveryPort == 0 && configuredRlpxPort == 0) + ? listeningPort + : configuredDiscoveryPort) + .join(); + if (natManager.isPresent()) { - this.configureNatEnvironment(); + this.configureNatEnvironment(listeningPort, discoveryPort); } - final int listeningPort = rlpxAgent.start().join(); - final int discoveryPort = peerDiscoveryAgent.start(listeningPort).join(); setLocalNode(listeningPort, discoveryPort); peerBondedObserverId = @@ -362,8 +372,9 @@ private void setLocalNode(final int listeningPort, final int discoveryPort) { localNode.setEnode(localEnode); } - private void configureNatEnvironment() { - CompletableFuture natQueryFuture = this.natManager.get().queryExternalIPAddress(); + private void configureNatEnvironment(final int listeningPort, final int discoveryPort) { + final CompletableFuture natQueryFuture = + this.natManager.orElseThrow().queryExternalIPAddress(); String externalAddress = null; try { final int timeoutSeconds = 60; @@ -379,19 +390,15 @@ private void configureNatEnvironment() { LOG.info("External IP detected: " + externalAddress); this.natManager .get() - .requestPortForward( - this.config.getDiscovery().getBindPort(), - UpnpNatManager.Protocol.UDP, - "besu-discovery"); + .requestPortForward(discoveryPort, UpnpNatManager.Protocol.UDP, "besu-discovery"); this.natManager .get() - .requestPortForward( - this.config.getRlpx().getBindPort(), UpnpNatManager.Protocol.TCP, "besu-rlpx"); + .requestPortForward(listeningPort, UpnpNatManager.Protocol.TCP, "besu-rlpx"); } else { LOG.info("No external IP detected within timeout."); } - } catch (Exception e) { + } catch (final Exception e) { LOG.error("Error configuring NAT environment", e); } natExternalAddress = Optional.ofNullable(externalAddress); diff --git a/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetworkTest.java b/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetworkTest.java index 4dc6098dc38..92aed4f54e7 100644 --- a/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetworkTest.java +++ b/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetworkTest.java @@ -90,7 +90,9 @@ public void before() { lenient().when(rlpxAgent.stop()).thenReturn(CompletableFuture.completedFuture(null)); lenient() .when(discoveryAgent.start(anyInt())) - .thenReturn(CompletableFuture.completedFuture(30303)); + .thenAnswer( + invocation -> + CompletableFuture.completedFuture(invocation.getArgument(0, Integer.class))); lenient().when(discoveryAgent.stop()).thenReturn(CompletableFuture.completedFuture(null)); lenient() .when(discoveryAgent.observePeerBondedEvents(discoverySubscriberCaptor.capture())) diff --git a/nat/src/main/java/org/hyperledger/besu/nat/upnp/UpnpNatManager.java b/nat/src/main/java/org/hyperledger/besu/nat/upnp/UpnpNatManager.java index 4c6da4305c3..e7c8db8b062 100644 --- a/nat/src/main/java/org/hyperledger/besu/nat/upnp/UpnpNatManager.java +++ b/nat/src/main/java/org/hyperledger/besu/nat/upnp/UpnpNatManager.java @@ -14,6 +14,9 @@ */ package org.hyperledger.besu.nat.upnp; +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkState; + import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -38,10 +41,8 @@ import org.jupnp.registry.Registry; import org.jupnp.registry.RegistryListener; import org.jupnp.support.igd.callback.GetExternalIP; -import org.jupnp.support.igd.callback.GetStatusInfo; import org.jupnp.support.igd.callback.PortMappingAdd; import org.jupnp.support.igd.callback.PortMappingDelete; -import org.jupnp.support.model.Connection; import org.jupnp.support.model.PortMapping; /** @@ -178,23 +179,6 @@ synchronized CompletableFuture getWANIPConnectionService() { return getService(SERVICE_TYPE_WAN_IP_CONNECTION); } - /** - * Get the local address on which we discovered our external IP address. - * - *

This can be useful to distinguish which network interface the external address was - * discovered on. - * - * @return the local address on which our GetExternalIP was discovered on, or an empty value if no - * GetExternalIP query has been performed successfully. - */ - public Optional getDiscoveredOnLocalAddress() { - if (!started) { - throw new IllegalStateException( - "Cannot call getDiscoveredOnLocalAddress() when in stopped state"); - } - return this.discoveredOnLocalAddress; - } - /** * Returns a CompletableFuture that will wait for the given service type to be discovered. No new * query will be performed, and if the service has already been discovered, the future will @@ -225,8 +209,6 @@ public synchronized CompletableFuture queryExternalIPAddress() { * *

Note that this is not synchronized, as it is expected to be called within an * already-synchronized context ({@link #start()}). - * - * @return A CompletableFuture that can be used to query the result (or error). */ private void initiateExternalIpQuery() { discoverService(SERVICE_TYPE_WAN_IP_CONNECTION) @@ -285,46 +267,6 @@ public void failure( }); } - /** - * Sends a UPnP request to the discovered IGD to request status info. - * - * @return A CompletableFuture that can be used to query the result (or error). - */ - public CompletableFuture queryStatusInfo() { - if (!started) { - throw new IllegalStateException("Cannot call queryStatusInfo() when in stopped state"); - } - final CompletableFuture upnpQueryFuture = new CompletableFuture<>(); - - return discoverService(SERVICE_TYPE_WAN_IP_CONNECTION) - .thenCompose( - service -> { - GetStatusInfo callback = - new GetStatusInfo(service) { - @Override - public void success(final Connection.StatusInfo statusInfo) { - upnpQueryFuture.complete(statusInfo); - } - - /** - * Because the underlying jupnp library omits generics info in this method - * signature, we must too when we override it. - */ - @Override - @SuppressWarnings("rawtypes") - public void failure( - final ActionInvocation invocation, - final UpnpResponse operation, - final String msg) { - upnpQueryFuture.completeExceptionally(new Exception(msg)); - } - }; - upnpService.getControlPoint().execute(callback); - - return upnpQueryFuture; - }); - } - /** * Convenience function to call {@link #requestPortForward(PortMapping)} with the following * defaults: @@ -337,12 +279,11 @@ public void failure( * @param port is the port to be used for both internal and external port values * @param protocol is either UDP or TCP * @param description is a free-form description, often displayed in router UIs - * @return A CompletableFuture which will provide the results of the request */ - public CompletableFuture requestPortForward( + public void requestPortForward( final int port, final Protocol protocol, final String description) { - return this.requestPortForward( + this.requestPortForward( new PortMapping( true, new UnsignedIntegerFourBytes(0), @@ -354,45 +295,6 @@ public CompletableFuture requestPortForward( description)); } - /** - * Convenience function to avoid use of PortMapping object. Takes the same arguments as are in a - * PortMapping object and constructs such an object for the caller. - * - *

This method chains to the {@link #requestPortForward(PortMapping)} method. - * - * @param enabled specifies whether or not the PortMapping is enabled - * @param leaseDurationSeconds is the duration of the PortMapping, in seconds - * @param remoteHost is a domain name or IP address used to filter which remote source this - * forwarding can apply to - * @param externalPort is the source port (the port visible to the Internet) - * @param internalPort is the destination port (the port to be forwarded to) - * @param internalClient is the destination host on the local LAN - * @param protocol is either UDP or TCP - * @param description is a free-form description, often displayed in router UIs - * @return A CompletableFuture which will provide the results of the request - */ - public CompletableFuture requestPortForward( - final boolean enabled, - final int leaseDurationSeconds, - final String remoteHost, - final int externalPort, - final int internalPort, - final String internalClient, - final Protocol protocol, - final String description) { - - return this.requestPortForward( - new PortMapping( - enabled, - new UnsignedIntegerFourBytes(leaseDurationSeconds), - remoteHost, - new UnsignedIntegerTwoBytes(externalPort), - new UnsignedIntegerTwoBytes(internalPort), - internalClient, - toJupnpProtocol(protocol), - description)); - } - /** * Sends a UPnP request to the discovered IGD to request a port forward. * @@ -400,9 +302,9 @@ public CompletableFuture requestPortForward( * @return A CompletableFuture that can be used to query the result (or error). */ private CompletableFuture requestPortForward(final PortMapping portMapping) { - if (!started) { - throw new IllegalStateException("Cannot call requestPortForward() when in stopped state"); - } + checkArgument( + portMapping.getInternalPort().getValue() != 0, "Cannot map to internal port zero."); + checkState(started, "Cannot call requestPortForward() when in stopped state"); CompletableFuture upnpQueryFuture = new CompletableFuture<>(); diff --git a/nat/src/test/java/org/hyperledger/besu/nat/upnp/UpnpNatManagerTest.java b/nat/src/test/java/org/hyperledger/besu/nat/upnp/UpnpNatManagerTest.java index 7843d2381cf..f39940c28ee 100644 --- a/nat/src/test/java/org/hyperledger/besu/nat/upnp/UpnpNatManagerTest.java +++ b/nat/src/test/java/org/hyperledger/besu/nat/upnp/UpnpNatManagerTest.java @@ -100,33 +100,21 @@ public void startDoesNothingWhenAlreadyStarted() throws Exception { } @Test - public void getDiscoveredOnLocalAddressThrowsWhenCalledBeforeStart() throws Exception { - - assertThatThrownBy( - () -> { - upnpManager.getDiscoveredOnLocalAddress(); - }) - .isInstanceOf(IllegalStateException.class); - } - - @Test - public void queryStatusInfoThrowsWhenCalledBeforeStart() throws Exception { + public void requestPortForwardThrowsWhenCalledBeforeStart() throws Exception { assertThatThrownBy( () -> { - upnpManager.queryStatusInfo(); + upnpManager.requestPortForward(80, UpnpNatManager.Protocol.TCP, ""); }) .isInstanceOf(IllegalStateException.class); } @Test - public void requestPortForwardThrowsWhenCalledBeforeStart() throws Exception { + public void requestPortForwardThrowsWhenPortIsZero() { + upnpManager.start(); - assertThatThrownBy( - () -> { - upnpManager.requestPortForward(0, UpnpNatManager.Protocol.TCP, ""); - }) - .isInstanceOf(IllegalStateException.class); + assertThatThrownBy(() -> upnpManager.requestPortForward(0, UpnpNatManager.Protocol.TCP, "")) + .isInstanceOf(IllegalArgumentException.class); } @Test