Deprecate Xsnapsync-bft-enabled option , to be removed in future release (#7930)

* Move snapsync-bft-enabled to stable options

Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>

* Add deprecation comments:

Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>

* Changelog entry for deprecation

Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>

* spotlessApply

Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>

* Update CHANGELOG.md

Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Bhanu Pulluri <59369753+pullurib@users.noreply.github.com>

* Removed option value checking to enable snapsync for bft networks. It's now supported by default

Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>

* Add acceptance tests for QBFT sync

Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>

* Update test

Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>

* update CHANGELOG

Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>

* Update besu/src/main/java/org/hyperledger/besu/cli/options/SynchronizerOptions.java

Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Bhanu Pulluri <59369753+pullurib@users.noreply.github.com>

* add delay to let the new validator sync

Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>

* update BFT sync AT to use conditions and timeouts

Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>

---------

Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>
Signed-off-by: Bhanu Pulluri <59369753+pullurib@users.noreply.github.com>
Co-authored-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>
Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>

CHANGELOG.md

@@ -11,6 +11,7 @@
 - `--Xbonsai-limit-trie-logs-enabled` is deprecated, use `--bonsai-limit-trie-logs-enabled` instead.
 - `--Xbonsai-trie-log-pruning-enabled` is deprecated, use `--bonsai-limit-trie-logs-enabled` instead.
 - `--Xbonsai-trie-logs-pruning-window-size` is deprecated, use `--bonsai-trie-logs-pruning-window-size` instead.
+- `--Xsnapsync-bft-enabled` is deprecated and will be removed in a future release. SNAP sync is supported for BFT networks.
 - `--tx-pool-disable-locals` has been deprecated, use `--tx-pool-no-local-priority`, instead.
 - Sunsetting features - for more context on the reasoning behind the deprecation of these features, including alternative options, read [this blog post](https://www.lfdecentralizedtrust.org/blog/sunsetting-tessera-and-simplifying-hyperledger-besu)
     - Tessera privacy
@@ -168,7 +169,6 @@ This is a hotfix to address publishing besu maven artifacts.  There are no issue
   - Smart-contract-based (onchain) permissioning
   - Proof of Work consensus
   - Fast Sync
-
 ### Additions and Improvements
 - Fine tune already seen txs tracker when a tx is removed from the pool [#7755](https://github.com/hyperledger/besu/pull/7755)
 - Support for enabling and configuring TLS/mTLS in WebSocket service. [#7854](https://github.com/hyperledger/besu/pull/7854)

acceptance-tests/dsl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/blockchain/Blockchain.java

@@ -45,6 +45,10 @@ public class Blockchain {
     return new ExpectBlockNumberAbove(eth, BigInteger.valueOf(blockNumber));
   }
 
+  public Condition minimumHeight(final long blockNumber, final int timeout) {
+    return new ExpectBlockNumberAbove(eth, BigInteger.valueOf(blockNumber), timeout);
+  }
+
   public Condition reachesHeight(final BesuNode node, final int blocksAheadOfLatest) {
     return new ExpectBlockNumberAbove(eth, futureHeight(node, blocksAheadOfLatest));
   }

acceptance-tests/dsl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/node/BesuNode.java

@@ -30,6 +30,7 @@ import org.hyperledger.besu.ethereum.api.jsonrpc.websocket.WebSocketConfiguratio
 import org.hyperledger.besu.ethereum.core.MiningConfiguration;
 import org.hyperledger.besu.ethereum.core.PrivacyParameters;
 import org.hyperledger.besu.ethereum.core.Util;
+import org.hyperledger.besu.ethereum.eth.sync.SynchronizerConfiguration;
 import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolConfiguration;
 import org.hyperledger.besu.ethereum.p2p.config.NetworkingConfiguration;
 import org.hyperledger.besu.ethereum.permissioning.PermissioningConfiguration;
@@ -134,6 +135,7 @@ public class BesuNode implements NodeConfiguration, RunnableNode, AutoCloseable
   private Optional<Integer> exitCode = Optional.empty();
   private final boolean isStrictTxReplayProtectionEnabled;
   private final Map<String, String> environment;
+  private SynchronizerConfiguration synchronizerConfiguration;
   private final Optional<KeyValueStorageFactory> storageFactory;
 
   public BesuNode(
@@ -234,6 +236,7 @@ public class BesuNode implements NodeConfiguration, RunnableNode, AutoCloseable
     this.isDnsEnabled = isDnsEnabled;
     privacyParameters.ifPresent(this::setPrivacyParameters);
     this.environment = environment;
+    this.synchronizerConfiguration = SynchronizerConfiguration.builder().build(); // Default config
     LOG.info("Created BesuNode {}", this);
   }
 
@@ -855,6 +858,15 @@ public class BesuNode implements NodeConfiguration, RunnableNode, AutoCloseable
     return apiConfiguration;
   }
 
+  public SynchronizerConfiguration getSynchronizerConfiguration() {
+    return synchronizerConfiguration;
+  }
+
+  public BesuNode setSynchronizerConfiguration(final SynchronizerConfiguration config) {
+    this.synchronizerConfiguration = config;
+    return this;
+  }
+
   public Optional<KeyValueStorageFactory> getStorageFactory() {
     return storageFactory;
   }

acceptance-tests/dsl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/node/ProcessBesuNodeRunner.java

@@ -153,8 +153,20 @@ public class ProcessBesuNodeRunner implements BesuNodeRunner {
       params.add(node.getNetwork().name());
     }
 
-    params.add("--sync-mode");
+    if (node.getSynchronizerConfiguration() != null) {
-    params.add("FULL");
+
+      if (node.getSynchronizerConfiguration().getSyncMode() != null) {
+        params.add("--sync-mode");
+        params.add(node.getSynchronizerConfiguration().getSyncMode().toString());
+      }
+      params.add("--sync-min-peers");
+      params.add(Integer.toString(node.getSynchronizerConfiguration().getSyncMinimumPeerCount()));
+    } else {
+      params.add("--sync-mode");
+      params.add("FULL");
+    }
+
+    params.add("--Xsnapsync-server-enabled");
 
     params.add("--discovery-enabled");
     params.add(Boolean.toString(node.isDiscoveryEnabled()));

acceptance-tests/dsl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/node/configuration/BesuNodeConfiguration.java

@@ -23,6 +23,7 @@ import org.hyperledger.besu.ethereum.api.jsonrpc.ipc.JsonRpcIpcConfiguration;
 import org.hyperledger.besu.ethereum.api.jsonrpc.websocket.WebSocketConfiguration;
 import org.hyperledger.besu.ethereum.core.MiningConfiguration;
 import org.hyperledger.besu.ethereum.core.PrivacyParameters;
+import org.hyperledger.besu.ethereum.eth.sync.SynchronizerConfiguration;
 import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolConfiguration;
 import org.hyperledger.besu.ethereum.p2p.config.NetworkingConfiguration;
 import org.hyperledger.besu.ethereum.permissioning.PermissioningConfiguration;
@@ -73,6 +74,7 @@ public class BesuNodeConfiguration {
   private final Optional<KeyPair> keyPair;
   private final boolean strictTxReplayProtectionEnabled;
   private final Map<String, String> environment;
+  private final SynchronizerConfiguration synchronizerConfiguration;
   private final Optional<KeyValueStorageFactory> storageFactory;
 
   BesuNodeConfiguration(
@@ -111,6 +113,7 @@ public class BesuNodeConfiguration {
       final Optional<KeyPair> keyPair,
       final boolean strictTxReplayProtectionEnabled,
       final Map<String, String> environment,
+      final SynchronizerConfiguration synchronizerConfiguration,
       final Optional<KeyValueStorageFactory> storageFactory) {
     this.name = name;
     this.miningConfiguration = miningConfiguration;
@@ -147,6 +150,7 @@ public class BesuNodeConfiguration {
     this.keyPair = keyPair;
     this.strictTxReplayProtectionEnabled = strictTxReplayProtectionEnabled;
     this.environment = environment;
+    this.synchronizerConfiguration = synchronizerConfiguration;
     this.storageFactory = storageFactory;
   }
 
@@ -290,6 +294,10 @@ public class BesuNodeConfiguration {
     return environment;
   }
 
+  public SynchronizerConfiguration getSynchronizerConfiguration() {
+    return synchronizerConfiguration;
+  }
+
   public Optional<KeyValueStorageFactory> storageImplementation() {
     return storageFactory;
   }

acceptance-tests/dsl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/node/configuration/BesuNodeConfigurationBuilder.java

@@ -33,6 +33,7 @@ import org.hyperledger.besu.ethereum.core.ImmutableMiningConfiguration;
 import org.hyperledger.besu.ethereum.core.ImmutableMiningConfiguration.MutableInitValues;
 import org.hyperledger.besu.ethereum.core.MiningConfiguration;
 import org.hyperledger.besu.ethereum.core.PrivacyParameters;
+import org.hyperledger.besu.ethereum.eth.sync.SynchronizerConfiguration;
 import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolConfiguration;
 import org.hyperledger.besu.ethereum.p2p.config.NetworkingConfiguration;
 import org.hyperledger.besu.ethereum.permissioning.PermissioningConfiguration;
@@ -96,6 +97,7 @@ public class BesuNodeConfigurationBuilder {
   private Optional<KeyPair> keyPair = Optional.empty();
   private Boolean strictTxReplayProtectionEnabled = false;
   private Map<String, String> environment = new HashMap<>();
+  private SynchronizerConfiguration synchronizerConfiguration;
   private Optional<KeyValueStorageFactory> storageImplementation = Optional.empty();
 
   public BesuNodeConfigurationBuilder() {
@@ -467,7 +469,17 @@ public class BesuNodeConfigurationBuilder {
     return this;
   }
 
+  public BesuNodeConfigurationBuilder synchronizerConfiguration(
+      final SynchronizerConfiguration config) {
+    this.synchronizerConfiguration = config;
+    return this;
+  }
+
   public BesuNodeConfiguration build() {
+    if (name == null) {
+      throw new IllegalStateException("Name is required");
+    }
+
     return new BesuNodeConfiguration(
         name,
         dataPath,
@@ -504,6 +516,7 @@ public class BesuNodeConfigurationBuilder {
         keyPair,
         strictTxReplayProtectionEnabled,
         environment,
+        synchronizerConfiguration,
         storageImplementation);
   }
 }

acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/bft/BftSyncAcceptanceTest.java

@@ -0,0 +1,69 @@
+/*
+ * Copyright contributors to Besu.
+ *
+ * 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.tests.acceptance.bft;
+
+import org.hyperledger.besu.ethereum.eth.sync.SyncMode;
+import org.hyperledger.besu.ethereum.eth.sync.SynchronizerConfiguration;
+import org.hyperledger.besu.tests.acceptance.dsl.node.BesuNode;
+
+import java.util.stream.Stream;
+
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+public class BftSyncAcceptanceTest extends ParameterizedBftTestBase {
+
+  private static final int TARGET_BLOCK_HEIGHT = 70;
+
+  static Stream<Arguments> syncModeTestParameters() {
+    return Stream.of(SyncMode.FULL, SyncMode.SNAP, SyncMode.CHECKPOINT)
+        .flatMap(
+            syncMode ->
+                factoryFunctions()
+                    .map(args -> Arguments.of(args.get()[0], args.get()[1], syncMode)));
+  }
+
+  @ParameterizedTest(name = "{index}: {0} with {2} sync")
+  @MethodSource("syncModeTestParameters")
+  public void shouldSyncValidatorNode(
+      final String testName,
+      final BftAcceptanceTestParameterization nodeFactory,
+      final SyncMode syncMode)
+      throws Exception {
+    setUp(testName, nodeFactory);
+
+    // Create validator network with 4 validators
+    final BesuNode validator1 = nodeFactory.createBonsaiNodeFixedPort(besu, "validator1");
+    final BesuNode validator2 = nodeFactory.createBonsaiNodeFixedPort(besu, "validator2");
+    final BesuNode validator3 = nodeFactory.createBonsaiNodeFixedPort(besu, "validator3");
+    final BesuNode validator4 = nodeFactory.createBonsaiNodeFixedPort(besu, "validator4");
+
+    // Configure validators with specified sync mode
+    final SynchronizerConfiguration syncConfig =
+        SynchronizerConfiguration.builder().syncMode(syncMode).syncMinimumPeerCount(1).build();
+
+    validator4.setSynchronizerConfiguration(syncConfig);
+
+    // Start first three validators
+    cluster.start(validator1, validator2, validator3);
+
+    validator1.verify(blockchain.minimumHeight(TARGET_BLOCK_HEIGHT, TARGET_BLOCK_HEIGHT));
+    // Add validator4 to cluster and start
+    cluster.addNode(validator4);
+
+    validator4.verify(blockchain.minimumHeight(TARGET_BLOCK_HEIGHT, 60));
+  }
+}

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

@@ -1443,27 +1443,9 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
     validateTransactionPoolOptions();
     validateDataStorageOptions();
     validateGraphQlOptions();
-    validateConsensusSyncCompatibilityOptions();
     validatePluginOptions();
   }
 
-  private void validateConsensusSyncCompatibilityOptions() {
-    // snap and checkpoint are experimental for BFT
-    if ((genesisConfigOptionsSupplier.get().isIbftLegacy()
-            || genesisConfigOptionsSupplier.get().isIbft2()
-            || genesisConfigOptionsSupplier.get().isQbft())
-        && !unstableSynchronizerOptions.isSnapSyncBftEnabled()) {
-      final String errorSuffix = "can't be used with BFT networks";
-      if (SyncMode.CHECKPOINT.equals(syncMode)) {
-        throw new ParameterException(
-            commandLine, String.format("%s %s", "Checkpoint sync", errorSuffix));
-      }
-      if (syncMode == SyncMode.SNAP) {
-        throw new ParameterException(commandLine, String.format("%s %s", "Snap sync", errorSuffix));
-      }
-    }
-  }
-
   private void validatePluginOptions() {
     pluginsConfigurationOptions.validate(commandLine);
   }
@@ -2753,7 +2735,6 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
     }
 
     builder.setSnapServerEnabled(this.unstableSynchronizerOptions.isSnapsyncServerEnabled());
-    builder.setSnapSyncBftEnabled(this.unstableSynchronizerOptions.isSnapSyncBftEnabled());
 
     builder.setTxPoolImplementation(buildTransactionPoolConfiguration().getTxPoolImplementation());
     builder.setWorldStateUpdateMode(unstableEvmOptions.toDomainObject().worldUpdaterMode());

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

@@ -57,7 +57,6 @@ public class ConfigurationOverviewBuilder {
   private long trieLogRetentionLimit = 0;
   private Integer trieLogsPruningWindowSize = null;
   private boolean isSnapServerEnabled = false;
-  private boolean isSnapSyncBftEnabled = false;
   private TransactionPoolConfiguration.Implementation txPoolImplementation;
   private EvmConfiguration.WorldUpdaterMode worldStateUpdateMode;
   private Map<String, String> environment;
@@ -246,17 +245,6 @@ public class ConfigurationOverviewBuilder {
     return this;
   }
 
-  /**
-   * Sets snap sync BFT enabled/disabled
-   *
-   * @param snapSyncBftEnabled bool to indicate if snap sync for BFT is enabled
-   * @return the builder
-   */
-  public ConfigurationOverviewBuilder setSnapSyncBftEnabled(final boolean snapSyncBftEnabled) {
-    isSnapSyncBftEnabled = snapSyncBftEnabled;
-    return this;
-  }
-
   /**
    * Sets trie logs pruning window size
    *
@@ -386,10 +374,6 @@ public class ConfigurationOverviewBuilder {
       lines.add("Experimental Snap Sync server enabled");
     }
 
-    if (isSnapSyncBftEnabled) {
-      lines.add("Experimental Snap Sync for BFT enabled");
-    }
-
     if (isLimitTrieLogsEnabled) {
       final StringBuilder trieLogPruningString = new StringBuilder();
       trieLogPruningString

besu/src/main/java/org/hyperledger/besu/cli/options/SynchronizerOptions.java

@@ -306,12 +306,15 @@ public class SynchronizerOptions implements CLIOptions<SynchronizerConfiguration
   private Boolean checkpointPostMergeSyncEnabled =
       SynchronizerConfiguration.DEFAULT_CHECKPOINT_POST_MERGE_ENABLED;
 
+  // TODO --Xsnapsync-bft-enabled is deprecated,
+  // remove in a future release
   @CommandLine.Option(
-      names = SNAP_SYNC_BFT_ENABLED_FLAG,
+      names = SNAP_SYNC_BFT_ENABLED_FLAG, // deprecated
       hidden = true,
       paramLabel = "<Boolean>",
       arity = "0..1",
-      description = "Snap sync enabled for BFT chains (default: ${DEFAULT-VALUE})")
+      description =
+          "This option is now deprecated and ignored, and will be removed in future release. Snap sync for BFT is supported by default.")
   private Boolean snapsyncBftEnabled = SnapSyncConfiguration.DEFAULT_SNAP_SYNC_BFT_ENABLED;
 
   @CommandLine.Option(
@@ -409,7 +412,6 @@ public class SynchronizerOptions implements CLIOptions<SynchronizerConfiguration
         config.getSnapSyncConfiguration().getLocalFlatStorageCountToHealPerRequest();
     options.checkpointPostMergeSyncEnabled = config.isCheckpointPostMergeEnabled();
     options.snapsyncServerEnabled = config.getSnapSyncConfiguration().isSnapServerEnabled();
-    options.snapsyncBftEnabled = config.getSnapSyncConfiguration().isSnapSyncBftEnabled();
     options.snapTransactionIndexingEnabled =
         config.getSnapSyncConfiguration().isSnapSyncTransactionIndexingEnabled();
     return options;
@@ -444,7 +446,6 @@ public class SynchronizerOptions implements CLIOptions<SynchronizerConfiguration
             .localFlatAccountCountToHealPerRequest(snapsyncFlatAccountHealedCountPerRequest)
             .localFlatStorageCountToHealPerRequest(snapsyncFlatStorageHealedCountPerRequest)
             .isSnapServerEnabled(snapsyncServerEnabled)
-            .isSnapSyncBftEnabled(snapsyncBftEnabled)
             .isSnapSyncTransactionIndexingEnabled(snapTransactionIndexingEnabled)
             .build());
     builder.checkpointPostMergeEnabled(checkpointPostMergeSyncEnabled);
@@ -504,8 +505,6 @@ public class SynchronizerOptions implements CLIOptions<SynchronizerConfiguration
             OptionParser.format(snapsyncFlatStorageHealedCountPerRequest),
             SNAP_SERVER_ENABLED_FLAG,
             OptionParser.format(snapsyncServerEnabled),
-            SNAP_SYNC_BFT_ENABLED_FLAG,
-            OptionParser.format(snapsyncBftEnabled),
             SNAP_TRANSACTION_INDEXING_ENABLED_FLAG,
             OptionParser.format(snapTransactionIndexingEnabled));
     return value;

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/SnapSyncConfiguration.java

@@ -85,11 +85,6 @@ public class SnapSyncConfiguration {
     return DEFAULT_SNAP_SERVER_ENABLED;
   }
 
-  @Value.Default
-  public Boolean isSnapSyncBftEnabled() {
-    return DEFAULT_SNAP_SYNC_BFT_ENABLED;
-  }
-
   @Value.Default
   public Boolean isSnapSyncTransactionIndexingEnabled() {
     return DEFAULT_SNAP_SYNC_TRANSACTION_INDEXING_ENABLED;