Feature/keep peers on dns fail (#3057)

* add behavior to not drop peers on failed lookup, add test coverage
* add dns discovery server override cli flag

Signed-off-by: garyschulte <garyschulte@gmail.com>

besu/src/main/java/org/hyperledger/besu/cli/options/unstable/NetworkingOptions.java

@@ -20,6 +20,7 @@ import org.hyperledger.besu.ethereum.p2p.config.NetworkingConfiguration;
 
 import java.util.Arrays;
 import java.util.List;
+import java.util.Optional;
 
 import picocli.CommandLine;
 
@@ -28,6 +29,7 @@ public class NetworkingOptions implements CLIOptions<NetworkingConfiguration> {
       "--Xp2p-initiate-connections-frequency";
   private final String CHECK_MAINTAINED_CONNECTIONS_FREQUENCY_FLAG =
       "--Xp2p-check-maintained-connections-frequency";
+  private final String DNS_DISCOVERY_SERVER_OVERRIDE_FLAG = "--Xp2p-dns-discovery-server";
 
   @CommandLine.Option(
       names = INITIATE_CONNECTIONS_FREQUENCY_FLAG,
@@ -49,6 +51,14 @@ public class NetworkingOptions implements CLIOptions<NetworkingConfiguration> {
   private int checkMaintainedConnectionsFrequencySec =
       NetworkingConfiguration.DEFAULT_CHECK_MAINTAINED_CONNECTIONS_FREQUENCY_SEC;
 
+  @CommandLine.Option(
+      names = DNS_DISCOVERY_SERVER_OVERRIDE_FLAG,
+      hidden = true,
+      defaultValue = "",
+      description =
+          "DNS server host to use for doing DNS Discovery of peers, rather than the machine's configured DNS server")
+  private String dnsDiscoveryServerOverride = null;
+
   private NetworkingOptions() {}
 
   public static NetworkingOptions create() {
@@ -61,6 +71,8 @@ public class NetworkingOptions implements CLIOptions<NetworkingConfiguration> {
         networkingConfig.getCheckMaintainedConnectionsFrequencySec();
     cliOptions.initiateConnectionsFrequencySec =
         networkingConfig.getInitiateConnectionsFrequencySec();
+    cliOptions.dnsDiscoveryServerOverride =
+        networkingConfig.getDnsDiscoveryServerOverride().orElse("");
     return cliOptions;
   }
 
@@ -69,6 +81,8 @@ public class NetworkingOptions implements CLIOptions<NetworkingConfiguration> {
     NetworkingConfiguration config = NetworkingConfiguration.create();
     config.setCheckMaintainedConnectionsFrequency(checkMaintainedConnectionsFrequencySec);
     config.setInitiateConnectionsFrequency(initiateConnectionsFrequencySec);
+    config.setDnsDiscoveryServerOverride(
+        Optional.of(dnsDiscoveryServerOverride).filter(z -> !z.isBlank()).orElse(null));
     return config;
   }
 
@@ -78,6 +92,8 @@ public class NetworkingOptions implements CLIOptions<NetworkingConfiguration> {
         CHECK_MAINTAINED_CONNECTIONS_FREQUENCY_FLAG,
         OptionParser.format(checkMaintainedConnectionsFrequencySec),
         INITIATE_CONNECTIONS_FREQUENCY_FLAG,
-        OptionParser.format(initiateConnectionsFrequencySec));
+        OptionParser.format(initiateConnectionsFrequencySec),
+        DNS_DISCOVERY_SERVER_OVERRIDE_FLAG,
+        dnsDiscoveryServerOverride);
   }
 }

besu/src/test/java/org/hyperledger/besu/cli/options/NetworkingOptionsTest.java

@@ -72,6 +72,31 @@ public class NetworkingOptionsTest
     assertThat(commandOutput.toString()).isEmpty();
   }
 
+  @Test
+  public void checkDnsServerOverrideFlag_isSet() {
+    final TestBesuCommand cmd = parseCommand("--Xp2p-dns-discovery-server", "localhost");
+
+    final NetworkingOptions options = cmd.getNetworkingOptions();
+    final NetworkingConfiguration networkingConfig = options.toDomainObject();
+    assertThat(networkingConfig.getDnsDiscoveryServerOverride()).isPresent();
+    assertThat(networkingConfig.getDnsDiscoveryServerOverride().get()).isEqualTo("localhost");
+
+    assertThat(commandErrorOutput.toString()).isEmpty();
+    assertThat(commandOutput.toString()).isEmpty();
+  }
+
+  @Test
+  public void checkDnsServerOverrideFlag_isNotSet() {
+    final TestBesuCommand cmd = parseCommand();
+
+    final NetworkingOptions options = cmd.getNetworkingOptions();
+    final NetworkingConfiguration networkingConfig = options.toDomainObject();
+    assertThat(networkingConfig.getDnsDiscoveryServerOverride()).isEmpty();
+
+    assertThat(commandErrorOutput.toString()).isEmpty();
+    assertThat(commandOutput.toString()).isEmpty();
+  }
+
   @Override
   NetworkingConfiguration createDefaultDomainObject() {
     return NetworkingConfiguration.create();

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/config/NetworkingConfiguration.java

@@ -17,6 +17,7 @@ package org.hyperledger.besu.ethereum.p2p.config;
 import static com.google.common.base.Preconditions.checkArgument;
 
 import java.util.Objects;
+import java.util.Optional;
 
 public class NetworkingConfiguration {
   public static final int DEFAULT_INITIATE_CONNECTIONS_FREQUENCY_SEC = 30;
@@ -27,6 +28,7 @@ public class NetworkingConfiguration {
   private int initiateConnectionsFrequencySec = DEFAULT_INITIATE_CONNECTIONS_FREQUENCY_SEC;
   private int checkMaintainedConnectionsFrequencySec =
       DEFAULT_CHECK_MAINTAINED_CONNECTIONS_FREQUENCY_SEC;
+  private String dnsDiscoveryServerOverride = null;
 
   public static NetworkingConfiguration create() {
     return new NetworkingConfiguration();
@@ -65,6 +67,16 @@ public class NetworkingConfiguration {
     return checkMaintainedConnectionsFrequencySec;
   }
 
+  public NetworkingConfiguration setDnsDiscoveryServerOverride(
+      final String dnsDiscoveryServerOverride) {
+    this.dnsDiscoveryServerOverride = dnsDiscoveryServerOverride;
+    return this;
+  }
+
+  public Optional<String> getDnsDiscoveryServerOverride() {
+    return Optional.ofNullable(dnsDiscoveryServerOverride);
+  }
+
   public NetworkingConfiguration setCheckMaintainedConnectionsFrequency(
       final int checkMaintainedConnectionsFrequency) {
     checkArgument(checkMaintainedConnectionsFrequency > 0);

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java

@@ -77,6 +77,7 @@ import org.apache.logging.log4j.Logger;
 import org.apache.tuweni.bytes.Bytes;
 import org.apache.tuweni.devp2p.EthereumNodeRecord;
 import org.apache.tuweni.discovery.DNSDaemon;
+import org.apache.tuweni.discovery.DNSDaemonListener;
 
 /**
  * The peer network service (defunct PeerNetworkingService) is the entrypoint to the peer-to-peer
@@ -143,10 +144,10 @@ public class DefaultP2PNetwork implements P2PNetwork {
   private final AtomicBoolean stopped = new AtomicBoolean(false);
   private final CountDownLatch shutdownLatch = new CountDownLatch(2);
   private final Duration shutdownTimeout = Duration.ofMinutes(1);
-
-  private final AtomicReference<List<DiscoveryPeer>> dnsPeers = new AtomicReference<>();
   private DNSDaemon dnsDaemon;
 
+  @VisibleForTesting final AtomicReference<List<DiscoveryPeer>> dnsPeers = new AtomicReference<>();
+
   /**
    * Creates a peer networking service for production purposes.
    *
@@ -203,29 +204,27 @@ public class DefaultP2PNetwork implements P2PNetwork {
     final String address = config.getDiscovery().getAdvertisedHost();
     final int configuredDiscoveryPort = config.getDiscovery().getBindPort();
     final int configuredRlpxPort = config.getRlpx().getBindPort();
-    if (config.getDiscovery().getDNSDiscoveryURL() != null) {
+
-      LOG.info("Starting DNS discovery with URL {}", config.getDiscovery().getDNSDiscoveryURL());
+    Optional.ofNullable(config.getDiscovery().getDNSDiscoveryURL())
-      dnsDaemon =
+        .ifPresent(
-          new DNSDaemon(
+            disco -> {
-              config.getDiscovery().getDNSDiscoveryURL(),
+              LOG.info("Starting DNS discovery with URL {}", disco);
-              (seq, records) -> {
+              config
-                List<DiscoveryPeer> peers = new ArrayList<>();
+                  .getDnsDiscoveryServerOverride()
-                for (EthereumNodeRecord enr : records) {
+                  .ifPresent(
-                  EnodeURL enodeURL =
+                      dnsServer ->
-                      EnodeURLImpl.builder()
+                          LOG.info(
-                          .ipAddress(enr.ip())
+                              "Starting DNS discovery with DNS Server override {}", dnsServer));
-                          .nodeId(enr.publicKey().bytes())
+
-                          .discoveryPort(Optional.ofNullable(enr.udp()))
+              dnsDaemon =
-                          .listeningPort(Optional.ofNullable(enr.tcp()))
+                  new DNSDaemon(
-                          .build();
+                      disco,
-                  DiscoveryPeer peer = DiscoveryPeer.fromEnode(enodeURL);
+                      createDaemonListener(),
-                  peers.add(peer);
+                      0L,
-                  rlpxAgent.connect(peer);
+                      60000L,
-                }
+                      config.getDnsDiscoveryServerOverride().orElse(null));
-                dnsPeers.set(peers);
+              dnsDaemon.start();
-              });
+            });
-    }
-    getDnsDaemon().ifPresent(DNSDaemon::start);
 
     final int listeningPort = rlpxAgent.start().join();
     final int discoveryPort =
@@ -325,6 +324,29 @@ public class DefaultP2PNetwork implements P2PNetwork {
     return Optional.ofNullable(dnsDaemon);
   }
 
+  @VisibleForTesting
+  DNSDaemonListener createDaemonListener() {
+    return (seq, records) -> {
+      List<DiscoveryPeer> peers = new ArrayList<>();
+      for (EthereumNodeRecord enr : records) {
+        EnodeURL enodeURL =
+            EnodeURLImpl.builder()
+                .ipAddress(enr.ip())
+                .nodeId(enr.publicKey().bytes())
+                .discoveryPort(Optional.ofNullable(enr.udp()))
+                .listeningPort(Optional.ofNullable(enr.tcp()))
+                .build();
+        DiscoveryPeer peer = DiscoveryPeer.fromEnode(enodeURL);
+        peers.add(peer);
+        rlpxAgent.connect(peer);
+      }
+      // only replace dnsPeers if the lookup was successful:
+      if (!peers.isEmpty()) {
+        dnsPeers.set(peers);
+      }
+    };
+  }
+
   @VisibleForTesting
   void checkMaintainedConnectionPeers() {
     if (!localNode.isReady()) {

ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetworkTest.java

@@ -18,6 +18,7 @@ import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
@@ -52,6 +53,7 @@ import org.hyperledger.besu.nat.NatService;
 import org.hyperledger.besu.nat.core.domain.NetworkProtocol;
 import org.hyperledger.besu.nat.upnp.UpnpNatManager;
 
+import java.net.InetAddress;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
@@ -61,6 +63,10 @@ import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import org.apache.tuweni.bytes.Bytes;
+import org.apache.tuweni.bytes.Bytes32;
+import org.apache.tuweni.crypto.SECP256K1;
+import org.apache.tuweni.devp2p.EthereumNodeRecord;
+import org.apache.tuweni.discovery.DNSDaemonListener;
 import org.assertj.core.api.Assertions;
 import org.junit.Before;
 import org.junit.Test;
@@ -72,8 +78,11 @@ import org.mockito.junit.MockitoJUnitRunner;
 
 @RunWith(MockitoJUnitRunner.StrictStubs.class)
 public final class DefaultP2PNetworkTest {
-
   final MaintainedPeers maintainedPeers = new MaintainedPeers();
+  final SECP256K1.SecretKey mockKey =
+      SECP256K1.SecretKey.fromBytes(
+          Bytes32.fromHexString(
+              "0x8f2a55949038a9610f50fb23b5883af3b4ecb3c3bb792cbcefbd1542c692be63"));
   @Mock PeerDiscoveryAgent discoveryAgent;
   @Mock RlpxAgent rlpxAgent;
 
@@ -334,18 +343,15 @@ public final class DefaultP2PNetworkTest {
   }
 
   @Test
-  public void shouldNotStartDnsDiscoveryWhenDNSURLIsNotConfigured() {
+  public void shouldNotStartDnsDiscoveryWhenDnsURLIsNotConfigured() {
-    // spy on DefaultP2PNetwork
+    DefaultP2PNetwork testClass = network();
-    DefaultP2PNetwork testClass = spy(network());
-
     testClass.start();
-    // ensure we called getDnsDaemon during start, and that it is NOT present:
+    // ensure DnsDaemon is NOT present:
-    verify(testClass, times(1)).getDnsDaemon();
     assertThat(testClass.getDnsDaemon()).isNotPresent();
   }
 
   @Test
-  public void shouldStartDnsDiscoveryWhenDNSURLIsNotConfigured() {
+  public void shouldStartDnsDiscoveryWhenDnsURLIsConfigured() {
     // create a discovery config with a dns config
     DiscoveryConfiguration disco =
         DiscoveryConfiguration.create().setDnsDiscoveryURL("enrtree://mock@localhost");
@@ -355,12 +361,58 @@ public final class DefaultP2PNetworkTest {
         when(spy(config).getDiscovery()).thenReturn(disco).getMock();
 
     // spy on DefaultP2PNetwork
-    DefaultP2PNetwork testClass = spy((DefaultP2PNetwork) builder().config(dnsConfig).build());
+    DefaultP2PNetwork testClass = (DefaultP2PNetwork) builder().config(dnsConfig).build();
+
+    testClass.start();
+    assertThat(testClass.getDnsDaemon()).isPresent();
+  }
+
+  @Test
+  public void shouldUseDnsServerOverrideIfPresent() {
+    // create a discovery config with a dns config
+    DiscoveryConfiguration disco =
+        DiscoveryConfiguration.create().setDnsDiscoveryURL("enrtree://mock@localhost");
+
+    // spy on config to return dns discovery config:
+    NetworkingConfiguration dnsConfig = spy(config);
+    doReturn(disco).when(dnsConfig).getDiscovery();
+    doReturn(Optional.of("localhost")).when(dnsConfig).getDnsDiscoveryServerOverride();
 
+    DefaultP2PNetwork testClass = (DefaultP2PNetwork) builder().config(dnsConfig).build();
     testClass.start();
-    // ensure we called getDnsDaemon during start, and that it is present:
+
-    verify(testClass, times(1)).getDnsDaemon();
+    // ensure we used the dns server override config when building DNSDaemon:
     assertThat(testClass.getDnsDaemon()).isPresent();
+    verify(dnsConfig, times(2)).getDnsDiscoveryServerOverride();
+  }
+
+  @Test
+  public void shouldNotDropDnsHostsOnEmptyLookup() {
+    DefaultP2PNetwork network = network();
+    DNSDaemonListener listenerUnderTest = network.createDaemonListener();
+
+    // assert no entries prior to lookup
+    assertThat(network.dnsPeers.get()).isNull();
+
+    // simulate successful lookup of 1 peer
+    listenerUnderTest.newRecords(
+        1,
+        List.of(
+            EthereumNodeRecord.create(
+                SECP256K1.KeyPair.fromSecretKey(mockKey),
+                1L,
+                null,
+                null,
+                InetAddress.getLoopbackAddress(),
+                30303,
+                30303)));
+    assertThat(network.dnsPeers.get()).isNotEmpty();
+    assertThat(network.dnsPeers.get().size()).isEqualTo(1);
+
+    // simulate failed lookup empty list
+    listenerUnderTest.newRecords(2, Collections.emptyList());
+    assertThat(network.dnsPeers.get()).isNotEmpty();
+    assertThat(network.dnsPeers.get().size()).isEqualTo(1);
   }
 
   private DefaultP2PNetwork network() {