Make sure DNS discovery URLs are never present if using a genesis file (

hyperledger#1635)

* Make sure DNS discovery URLs are never present if using a genesis file

Signed-off-by: Antoine Toulme <antoine@lunar-ocean.com>

* Add tests to make sure we cover extensively the genesis file vs named network use cases

Signed-off-by: Antoine Toulme <antoine@lunar-ocean.com>

besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java

@@ -2257,6 +2257,7 @@ private EthNetworkConfig updateNetworkConfig(final NetworkName network) {
         // than a useless one that may make user think that it can work when it can't.
         builder.setBootNodes(new ArrayList<>());
       }
+      builder.setDnsDiscoveryUrl(null);
     }
 
     if (networkId != null) {

besu/src/main/java/org/hyperledger/besu/cli/config/EthNetworkConfig.java

@@ -205,7 +205,7 @@ public static String jsonConfig(final NetworkName network) {
 
   public static class Builder {
 
-    private final String dnsDiscoveryUrl;
+    private String dnsDiscoveryUrl;
     private String genesisConfig;
     private BigInteger networkId;
     private List<EnodeURL> bootNodes;
@@ -232,6 +232,11 @@ public Builder setBootNodes(final List<EnodeURL> bootNodes) {
       return this;
     }
 
+    public Builder setDnsDiscoveryUrl(final String dnsDiscoveryUrl) {
+      this.dnsDiscoveryUrl = dnsDiscoveryUrl;
+      return this;
+    }
+
     public EthNetworkConfig build() {
       return new EthNetworkConfig(genesisConfig, networkId, bootNodes, dnsDiscoveryUrl);
     }

besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java

@@ -30,8 +30,12 @@
 import static org.hyperledger.besu.ethereum.api.jsonrpc.RpcApis.NET;
 import static org.hyperledger.besu.ethereum.api.jsonrpc.RpcApis.PERM;
 import static org.hyperledger.besu.ethereum.api.jsonrpc.RpcApis.WEB3;
+import static org.hyperledger.besu.ethereum.p2p.config.DiscoveryConfiguration.GOERLI_BOOTSTRAP_NODES;
+import static org.hyperledger.besu.ethereum.p2p.config.DiscoveryConfiguration.GOERLI_DISCOVERY_URL;
 import static org.hyperledger.besu.ethereum.p2p.config.DiscoveryConfiguration.MAINNET_BOOTSTRAP_NODES;
 import static org.hyperledger.besu.ethereum.p2p.config.DiscoveryConfiguration.MAINNET_DISCOVERY_URL;
+import static org.hyperledger.besu.ethereum.p2p.config.DiscoveryConfiguration.RINKEBY_BOOTSTRAP_NODES;
+import static org.hyperledger.besu.ethereum.p2p.config.DiscoveryConfiguration.RINKEBY_DISCOVERY_URL;
 import static org.hyperledger.besu.nat.kubernetes.KubernetesNatManager.DEFAULT_BESU_SERVICE_NAME_FILTER;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
@@ -345,6 +349,7 @@ public void overrideDefaultValuesIfKeyIsPresentInConfigFile() throws IOException
             .setNetworkId(BigInteger.valueOf(42))
             .setGenesisConfig(encodeJsonGenesis(GENESIS_VALID_JSON))
             .setBootNodes(nodes)
+            .setDnsDiscoveryUrl(null)
             .build();
     verify(mockControllerBuilder).dataDirectory(eq(dataFolder.toPath()));
     verify(mockControllerBuilderFactory).fromEthNetworkConfig(eq(networkConfig), any());
@@ -891,6 +896,72 @@ public void genesisPathOptionMustBeUsed() throws Exception {
     assertThat(commandErrorOutput.toString()).isEmpty();
   }
 
+  @Test
+  public void testGenesisPathEthOptions() throws Exception {
+    final Path genesisFile = createFakeGenesisFile(GENESIS_VALID_JSON);
+
+    final ArgumentCaptor<EthNetworkConfig> networkArg =
+        ArgumentCaptor.forClass(EthNetworkConfig.class);
+
+    parseCommand("--genesis-file", genesisFile.toString());
+
+    verify(mockControllerBuilderFactory).fromEthNetworkConfig(networkArg.capture(), any());
+    verify(mockControllerBuilder).build();
+
+    EthNetworkConfig config = networkArg.getValue();
+    assertThat(config.getBootNodes()).isEmpty();
+    assertThat(config.getDnsDiscoveryUrl()).isNull();
+    assertThat(config.getNetworkId()).isEqualTo(BigInteger.valueOf(3141592));
+  }
+
+  @Test
+  public void testGenesisPathMainnetEthConfig() throws Exception {
+    final ArgumentCaptor<EthNetworkConfig> networkArg =
+        ArgumentCaptor.forClass(EthNetworkConfig.class);
+
+    parseCommand("--network", "mainnet");
+
+    verify(mockControllerBuilderFactory).fromEthNetworkConfig(networkArg.capture(), any());
+    verify(mockControllerBuilder).build();
+
+    EthNetworkConfig config = networkArg.getValue();
+    assertThat(config.getBootNodes()).isEqualTo(MAINNET_BOOTSTRAP_NODES);
+    assertThat(config.getDnsDiscoveryUrl()).isEqualTo(MAINNET_DISCOVERY_URL);
+    assertThat(config.getNetworkId()).isEqualTo(BigInteger.valueOf(1));
+  }
+
+  @Test
+  public void testGenesisPathGoerliEthConfig() throws Exception {
+    final ArgumentCaptor<EthNetworkConfig> networkArg =
+        ArgumentCaptor.forClass(EthNetworkConfig.class);
+
+    parseCommand("--network", "goerli");
+
+    verify(mockControllerBuilderFactory).fromEthNetworkConfig(networkArg.capture(), any());
+    verify(mockControllerBuilder).build();
+
+    EthNetworkConfig config = networkArg.getValue();
+    assertThat(config.getBootNodes()).isEqualTo(GOERLI_BOOTSTRAP_NODES);
+    assertThat(config.getDnsDiscoveryUrl()).isEqualTo(GOERLI_DISCOVERY_URL);
+    assertThat(config.getNetworkId()).isEqualTo(BigInteger.valueOf(5));
+  }
+
+  @Test
+  public void testGenesisPathRinkebyEthConfig() throws Exception {
+    final ArgumentCaptor<EthNetworkConfig> networkArg =
+        ArgumentCaptor.forClass(EthNetworkConfig.class);
+
+    parseCommand("--network", "rinkeby");
+
+    verify(mockControllerBuilderFactory).fromEthNetworkConfig(networkArg.capture(), any());
+    verify(mockControllerBuilder).build();
+
+    EthNetworkConfig config = networkArg.getValue();
+    assertThat(config.getBootNodes()).isEqualTo(RINKEBY_BOOTSTRAP_NODES);
+    assertThat(config.getDnsDiscoveryUrl()).isEqualTo(RINKEBY_DISCOVERY_URL);
+    assertThat(config.getNetworkId()).isEqualTo(BigInteger.valueOf(4));
+  }
+
   @Test
   public void genesisAndNetworkMustNotBeUsedTogether() throws Exception {
     final Path genesisFile = createFakeGenesisFile(GENESIS_VALID_JSON);

besu/src/test/java/org/hyperledger/besu/cli/config/EthNetworkConfigTest.java

@@ -0,0 +1,76 @@
+/*
+ * Copyright 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.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package org.hyperledger.besu.cli.config;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.hyperledger.besu.cli.config.NetworkName.MAINNET;
+import static org.hyperledger.besu.ethereum.p2p.config.DiscoveryConfiguration.GOERLI_BOOTSTRAP_NODES;
+import static org.hyperledger.besu.ethereum.p2p.config.DiscoveryConfiguration.GOERLI_DISCOVERY_URL;
+import static org.hyperledger.besu.ethereum.p2p.config.DiscoveryConfiguration.MAINNET_BOOTSTRAP_NODES;
+import static org.hyperledger.besu.ethereum.p2p.config.DiscoveryConfiguration.MAINNET_DISCOVERY_URL;
+import static org.hyperledger.besu.ethereum.p2p.config.DiscoveryConfiguration.RINKEBY_BOOTSTRAP_NODES;
+import static org.hyperledger.besu.ethereum.p2p.config.DiscoveryConfiguration.RINKEBY_DISCOVERY_URL;
+
+import java.math.BigInteger;
+
+import org.junit.Test;
+
+public class EthNetworkConfigTest {
+
+  @Test
+  public void testDefaultMainnetConfig() {
+    EthNetworkConfig config = EthNetworkConfig.getNetworkConfig(NetworkName.MAINNET);
+    assertThat(config.getDnsDiscoveryUrl()).isEqualTo(MAINNET_DISCOVERY_URL);
+    assertThat(config.getBootNodes()).isEqualTo(MAINNET_BOOTSTRAP_NODES);
+    assertThat(config.getNetworkId()).isEqualTo(BigInteger.ONE);
+  }
+
+  @Test
+  public void testDefaultRinkebyConfig() {
+    EthNetworkConfig config = EthNetworkConfig.getNetworkConfig(NetworkName.RINKEBY);
+    assertThat(config.getDnsDiscoveryUrl()).isEqualTo(RINKEBY_DISCOVERY_URL);
+    assertThat(config.getBootNodes()).isEqualTo(RINKEBY_BOOTSTRAP_NODES);
+    assertThat(config.getNetworkId()).isEqualTo(BigInteger.valueOf(4));
+  }
+
+  @Test
+  public void testDefaultGoerliConfig() {
+    EthNetworkConfig config = EthNetworkConfig.getNetworkConfig(NetworkName.GOERLI);
+    assertThat(config.getDnsDiscoveryUrl()).isEqualTo(GOERLI_DISCOVERY_URL);
+    assertThat(config.getBootNodes()).isEqualTo(GOERLI_BOOTSTRAP_NODES);
+    assertThat(config.getNetworkId()).isEqualTo(BigInteger.valueOf(5));
+  }
+
+  @Test
+  public void testDefaultDevConfig() {
+    EthNetworkConfig config = EthNetworkConfig.getNetworkConfig(NetworkName.DEV);
+    assertThat(config.getDnsDiscoveryUrl()).isNull();
+    assertThat(config.getBootNodes()).isEmpty();
+    assertThat(config.getNetworkId()).isEqualTo(BigInteger.valueOf(2018));
+  }
+
+  @Test
+  public void testBuilderWithNetworkId() {
+    EthNetworkConfig config =
+        new EthNetworkConfig.Builder(EthNetworkConfig.getNetworkConfig(MAINNET))
+            .setNetworkId(BigInteger.valueOf(42))
+            .setGenesisConfig("{\"config\":{\"chainId\":\"1234567\"}")
+            .build();
+    assertThat(config.getGenesisConfig()).isEqualTo("{\"config\":{\"chainId\":\"1234567\"}");
+    assertThat(config.getDnsDiscoveryUrl()).isNotNull();
+    assertThat(config.getBootNodes()).isNotEmpty();
+    assertThat(config.getNetworkId()).isEqualTo(BigInteger.valueOf(42));
+  }
+}