[PAN-2265] Expose fast-sync options on command line (#1218)

- add `--fast-sync-min-peers` hidden cli option
- add `--fast-sync-max-wait-time` hidden cli option
- update `FastSyncActions` to handle no timeout behaviour (when timeout set to 0 or no timeout set then we just trigger asynchronously the `waitPeersTask`)
- update unit tests mock related to sync config builder
- add fast sync options in `everything_config.toml` and `partial_config.toml`
- re enable PantheonCommandTest.syncModeOptionMustBeUsed
- add new flags in `noOverrideDefaultValuesIfKeyIsNotPresentInConfigFile` and `overrideDefaultValuesIfKeyIsPresentInConfigFile` tests

fix PAN-2265
Signed-off-by: Adrian Sutton <adrian.sutton@consensys.net>

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/SynchronizerConfiguration.java

@@ -27,7 +27,7 @@ public class SynchronizerConfiguration {
   private static final int DEFAULT_PIVOT_DISTANCE_FROM_HEAD = 50;
   private static final float DEFAULT_FULL_VALIDATION_RATE = .1f;
   private static final int DEFAULT_FAST_SYNC_MINIMUM_PEERS = 5;
-  private static final Duration DEFAULT_FAST_SYNC_MAXIMUM_PEER_WAIT_TIME = Duration.ofMinutes(5);
+  private static final Duration DEFAULT_FAST_SYNC_MAXIMUM_PEER_WAIT_TIME = Duration.ofSeconds(0);
   private static final int DEFAULT_WORLD_STATE_HASH_COUNT_PER_REQUEST = 384;
   private static final int DEFAULT_WORLD_STATE_REQUEST_PARALLELISM = 10;
   private static final int DEFAULT_WORLD_STATE_MAX_REQUESTS_WITHOUT_PROGRESS = 1000;

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/fastsync/FastSyncActions.java

@@ -71,8 +71,22 @@ public class FastSyncActions<C> {
             ethContext, syncConfig.getFastSyncMinimumPeerCount(), metricsSystem);
 
     final EthScheduler scheduler = ethContext.getScheduler();
+    final CompletableFuture<Void> fastSyncTask;
+    if (!syncConfig.getFastSyncMaximumPeerWaitTime().isZero()) {
+      LOG.debug(
+          "Waiting for at least {} peers, maximum wait time set to {}.",
+          syncConfig.getFastSyncMinimumPeerCount(),
+          syncConfig.getFastSyncMaximumPeerWaitTime().toString());
+      fastSyncTask =
+          scheduler.timeout(waitForPeersTask, syncConfig.getFastSyncMaximumPeerWaitTime());
+    } else {
+      LOG.debug(
+          "Waiting for at least {} peers, no maximum wait time set.",
+          syncConfig.getFastSyncMinimumPeerCount());
+      fastSyncTask = scheduler.scheduleServiceTask(waitForPeersTask);
+    }
     return exceptionallyCompose(
-            scheduler.timeout(waitForPeersTask, syncConfig.getFastSyncMaximumPeerWaitTime()),
+            fastSyncTask,
             error -> {
               if (ExceptionUtils.rootCause(error) instanceof TimeoutException) {
                 if (ethContext.getEthPeers().availablePeerCount() > 0) {

ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fastsync/FastSyncActionsTest.java

@@ -38,6 +38,7 @@ import tech.pegasys.pantheon.ethereum.mainnet.ProtocolSchedule;
 import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem;
 import tech.pegasys.pantheon.util.uint.UInt256;
 
+import java.time.Duration;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.atomic.AtomicInteger;
 
@@ -50,6 +51,7 @@ public class FastSyncActionsTest {
   private final SynchronizerConfiguration syncConfig =
       new SynchronizerConfiguration.Builder()
           .syncMode(SyncMode.FAST)
+          .fastSyncMaximumPeerWaitTime(Duration.ofMinutes(5))
           .fastSyncPivotDistance(1000)
           .build();
 

pantheon/src/main/java/tech/pegasys/pantheon/cli/DefaultCommandValues.java

@@ -55,6 +55,8 @@ public interface DefaultCommandValues {
   // Default should be FAST for the next release
   // but we use FULL for the moment as Fast is still in progress
   SyncMode DEFAULT_SYNC_MODE = SyncMode.FULL;
+  int FAST_SYNC_MAX_WAIT_TIME = 0;
+  int FAST_SYNC_MIN_PEER_COUNT = 5;
   int DEFAULT_MAX_PEERS = 25;
 
   static Path getDefaultPantheonDataPath(final Object command) {

pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java

@@ -14,6 +14,8 @@ package tech.pegasys.pantheon.cli;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Arrays.asList;
+import static tech.pegasys.pantheon.cli.CommandLineUtils.checkOptionDependencies;
 import static tech.pegasys.pantheon.cli.DefaultCommandValues.getDefaultPantheonDataPath;
 import static tech.pegasys.pantheon.cli.NetworkName.MAINNET;
 import static tech.pegasys.pantheon.ethereum.jsonrpc.JsonRpcConfiguration.DEFAULT_JSON_RPC_PORT;
@@ -72,8 +74,8 @@ import java.net.InetAddress;
 import java.net.URI;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.time.Duration;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.List;
 import java.util.Optional;
@@ -228,6 +230,22 @@ public class PantheonCommand implements DefaultCommandValues, Runnable {
           "Synchronization mode, possible values are ${COMPLETION-CANDIDATES} (default: ${DEFAULT-VALUE})")
   private final SyncMode syncMode = DEFAULT_SYNC_MODE;
 
+  @Option(
+      hidden = true,
+      names = {"--fast-sync-min-peers"},
+      paramLabel = MANDATORY_INTEGER_FORMAT_HELP,
+      description =
+          "Minimum number of peers required before starting fast sync. (default: ${DEFAULT-VALUE})")
+  private final Integer fastSyncMinPeerCount = FAST_SYNC_MIN_PEER_COUNT;
+
+  @Option(
+      hidden = true,
+      names = {"--fast-sync-max-wait-time"},
+      paramLabel = MANDATORY_INTEGER_FORMAT_HELP,
+      description =
+          "Maximum time to wait for the required number of peers before starting fast sync, expressed in seconds, 0 means no timeout (default: ${DEFAULT-VALUE})")
+  private final Integer fastSyncMaxWaitTime = FAST_SYNC_MAX_WAIT_TIME;
+
   @Option(
       names = {"--network"},
       paramLabel = MANDATORY_NETWORK_FORMAT_HELP,
@@ -590,12 +608,12 @@ public class PantheonCommand implements DefaultCommandValues, Runnable {
     }
 
     // Check that P2P options are able to work or send an error
-    CommandLineUtils.checkOptionDependencies(
+    checkOptionDependencies(
         logger,
         commandLine,
         "--p2p-enabled",
         !p2pEnabled,
-        Arrays.asList(
+        asList(
             "--bootnodes",
             "--discovery-enabled",
             "--max-peers",
@@ -603,12 +621,24 @@ public class PantheonCommand implements DefaultCommandValues, Runnable {
             "--banned-node-ids"));
 
     // Check that mining options are able to work or send an error
-    CommandLineUtils.checkOptionDependencies(
+    checkOptionDependencies(
         logger,
         commandLine,
         "--miner-enabled",
         !isMiningEnabled,
-        Arrays.asList("--miner-coinbase", "--min-gas-price", "--miner-extra-data"));
+        asList("--miner-coinbase", "--min-gas-price", "--miner-extra-data"));
+
+    // Check that fast sync options are able to work or send an error
+    if (fastSyncMaxWaitTime < 0) {
+      throw new ParameterException(
+          commandLine, "--fast-sync-max-wait-time must be greater than or equal to 0");
+    }
+    checkOptionDependencies(
+        logger,
+        commandLine,
+        "--sync-mode",
+        SyncMode.FAST.equals(syncMode),
+        asList("--fast-sync-num-peers", "--fast-sync-timeout"));
 
     //noinspection ConstantConditions
     if (isMiningEnabled && coinbase == null) {
@@ -697,12 +727,12 @@ public class PantheonCommand implements DefaultCommandValues, Runnable {
 
   private JsonRpcConfiguration jsonRpcConfiguration() {
 
-    CommandLineUtils.checkOptionDependencies(
+    checkOptionDependencies(
         logger,
         commandLine,
         "--rpc-http-enabled",
         !isRpcHttpEnabled,
-        Arrays.asList(
+        asList(
             "--rpc-http-api",
             "--rpc-http-apis",
             "--rpc-http-cors-origins",
@@ -731,12 +761,12 @@ public class PantheonCommand implements DefaultCommandValues, Runnable {
 
   private WebSocketConfiguration webSocketConfiguration() {
 
-    CommandLineUtils.checkOptionDependencies(
+    checkOptionDependencies(
         logger,
         commandLine,
         "--rpc-ws-enabled",
         !isRpcWsEnabled,
-        Arrays.asList(
+        asList(
             "--rpc-ws-api",
             "--rpc-ws-apis",
             "--rpc-ws-host",
@@ -769,19 +799,19 @@ public class PantheonCommand implements DefaultCommandValues, Runnable {
               + "time.  Please refer to CLI reference for more details about this constraint.");
     }
 
-    CommandLineUtils.checkOptionDependencies(
+    checkOptionDependencies(
         logger,
         commandLine,
         "--metrics-enabled",
         !isMetricsEnabled,
-        Arrays.asList("--metrics-host", "--metrics-port"));
+        asList("--metrics-host", "--metrics-port"));
 
-    CommandLineUtils.checkOptionDependencies(
+    checkOptionDependencies(
         logger,
         commandLine,
         "--metrics-push-enabled",
         !isMetricsPushEnabled,
-        Arrays.asList(
+        asList(
             "--metrics-push-host",
             "--metrics-push-port",
             "--metrics-push-interval",
@@ -882,13 +912,12 @@ public class PantheonCommand implements DefaultCommandValues, Runnable {
 
   private PrivacyParameters privacyParameters() throws IOException {
 
-    CommandLineUtils.checkOptionDependencies(
+    checkOptionDependencies(
         logger,
         commandLine,
         "--privacy-enabled",
         !isPrivacyEnabled,
-        Arrays.asList(
+        asList("--privacy-url", "--privacy-public-key-file", "--privacy-precompiled-address"));
-            "--privacy-url", "--privacy-public-key-file", "--privacy-precompiled-address"));
 
     final PrivacyParameters privacyParameters = PrivacyParameters.noPrivacy();
     if (isPrivacyEnabled) {
@@ -909,6 +938,8 @@ public class PantheonCommand implements DefaultCommandValues, Runnable {
   private SynchronizerConfiguration buildSyncConfig() {
     return synchronizerConfigurationBuilder
         .syncMode(syncMode)
+        .fastSyncMinimumPeerCount(fastSyncMinPeerCount)
+        .fastSyncMaximumPeerWaitTime(Duration.ofSeconds(fastSyncMaxWaitTime))
         .maxTrailingPeers(TrailingPeerRequirements.calculateMaxTrailingPeers(maxPeers))
         .build();
   }

pantheon/src/test/java/tech/pegasys/pantheon/cli/CommandTestAbstract.java

@@ -36,6 +36,7 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.PrintStream;
 import java.nio.file.Path;
+import java.time.Duration;
 import java.util.Collection;
 
 import org.apache.logging.log4j.LogManager;
@@ -110,6 +111,10 @@ public abstract class CommandTestAbstract {
 
     when(mockSyncConfBuilder.syncMode(any())).thenReturn(mockSyncConfBuilder);
     when(mockSyncConfBuilder.maxTrailingPeers(anyInt())).thenReturn(mockSyncConfBuilder);
+    when(mockSyncConfBuilder.fastSyncMinimumPeerCount(anyInt())).thenReturn(mockSyncConfBuilder);
+    when(mockSyncConfBuilder.fastSyncMaximumPeerWaitTime(any(Duration.class)))
+        .thenReturn(mockSyncConfBuilder);
+
     when(mockSyncConfBuilder.build()).thenReturn(mockSyncConf);
 
     when(mockRunnerBuilder.vertx(any())).thenReturn(mockRunnerBuilder);

pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java

@@ -59,6 +59,7 @@ import java.net.URL;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.time.Duration;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
@@ -306,8 +307,10 @@ public class PantheonCommandTest extends CommandTestAbstract {
     verify(mockControllerBuilder).homePath(eq(Paths.get("~/pantheondata").toAbsolutePath()));
     verify(mockControllerBuilder).ethNetworkConfig(eq(networkConfig));
 
-    // TODO: Re-enable as per NC-1057/NC-1681
+    verify(mockSyncConfBuilder).syncMode(ArgumentMatchers.eq(SyncMode.FAST));
-    // verify(mockSyncConfBuilder).syncMode(ArgumentMatchers.eq(SyncMode.FAST));
+    verify(mockSyncConfBuilder).fastSyncMinimumPeerCount(ArgumentMatchers.eq(13));
+    verify(mockSyncConfBuilder)
+        .fastSyncMaximumPeerWaitTime(ArgumentMatchers.eq(Duration.ofSeconds(57)));
 
     assertThat(commandOutput.toString()).isEmpty();
     assertThat(commandErrorOutput.toString()).isEmpty();
@@ -608,8 +611,10 @@ public class PantheonCommandTest extends CommandTestAbstract {
         .maxPendingTransactions(eq(PendingTransactions.MAX_PENDING_TRANSACTIONS));
     verify(mockControllerBuilder).build();
 
-    // TODO: Re-enable as per NC-1057/NC-1681
+    verify(mockSyncConfBuilder).syncMode(ArgumentMatchers.eq(SyncMode.FULL));
-    // verify(mockSyncConfBuilder).syncMode(ArgumentMatchers.eq(SyncMode.FULL));
+    verify(mockSyncConfBuilder).fastSyncMinimumPeerCount(ArgumentMatchers.eq(5));
+    verify(mockSyncConfBuilder)
+        .fastSyncMaximumPeerWaitTime(ArgumentMatchers.eq(Duration.ofSeconds(0)));
 
     assertThat(commandErrorOutput.toString()).isEmpty();
 
@@ -1035,8 +1040,8 @@ public class PantheonCommandTest extends CommandTestAbstract {
     assertThat(commandErrorOutput.toString()).isEmpty();
   }
 
-  @Ignore("Ignored as we only have one mode available for now. See NC-1057/NC-1681")
   @Test
+  @Ignore
   public void syncModeOptionMustBeUsed() {
 
     parseCommand("--sync-mode", "FAST");
@@ -1049,6 +1054,48 @@ public class PantheonCommandTest extends CommandTestAbstract {
     assertThat(commandErrorOutput.toString()).isEmpty();
   }
 
+  @Test
+  public void parsesValidFastSyncTimeoutOption() {
+
+    parseCommand("--sync-mode", "FAST", "--fast-sync-max-wait-time", "17");
+    verify(mockSyncConfBuilder).syncMode(ArgumentMatchers.eq(SyncMode.FAST));
+    verify(mockSyncConfBuilder)
+        .fastSyncMaximumPeerWaitTime(ArgumentMatchers.eq(Duration.ofSeconds(17)));
+    assertThat(commandOutput.toString()).isEmpty();
+    assertThat(commandErrorOutput.toString()).isEmpty();
+  }
+
+  @Test
+  public void parsesInvalidFastSyncTimeoutOptionShouldFail() {
+    parseCommand("--sync-mode", "FAST", "--fast-sync-max-wait-time", "-1");
+
+    verifyZeroInteractions(mockRunnerBuilder);
+
+    assertThat(commandOutput.toString()).isEmpty();
+    assertThat(commandErrorOutput.toString())
+        .contains("--fast-sync-max-wait-time must be greater than or equal to 0");
+  }
+
+  @Test
+  public void parsesValidFastSyncMinPeersOption() {
+
+    parseCommand("--sync-mode", "FAST", "--fast-sync-min-peers", "11");
+    verify(mockSyncConfBuilder).syncMode(ArgumentMatchers.eq(SyncMode.FAST));
+    verify(mockSyncConfBuilder).fastSyncMinimumPeerCount(ArgumentMatchers.eq(11));
+    assertThat(commandOutput.toString()).isEmpty();
+    assertThat(commandErrorOutput.toString()).isEmpty();
+  }
+
+  @Test
+  public void parsesInvalidFastSyncMinPeersOptionWrongFormatShouldFail() {
+
+    parseCommand("--sync-mode", "FAST", "--fast-sync-min-peers", "ten");
+    verifyZeroInteractions(mockRunnerBuilder);
+    assertThat(commandOutput.toString()).isEmpty();
+    assertThat(commandErrorOutput.toString())
+        .contains("Invalid value for option '--fast-sync-min-peers': 'ten' is not an int");
+  }
+
   @Test
   public void rpcHttpEnabledPropertyDefaultIsFalse() {
     parseCommand();

pantheon/src/test/resources/complete_config.toml

@@ -27,6 +27,8 @@ metrics-port=309
 genesis-file="~/genesis.json" # Path
 network-id=42
 sync-mode="fast"# should be FAST or FULL (or fast or full)
+fast-sync-min-peers=13
+fast-sync-max-wait-time=57
 ottoman=false # true means using ottoman testnet if genesis file uses iBFT
 
 #mining

pantheon/src/test/resources/everything_config.toml

@@ -32,6 +32,8 @@ host-whitelist=["all"]
 network="MAINNET"
 genesis-file="~/genesis.json"
 sync-mode="fast"
+fast-sync-min-peers=5
+fast-sync-max-wait-time=30
 network-id=303
 
 # JSON-RPC