hyperledger · fab-10 · Jan 25, 2024 · Jan 22, 2024 · Jan 23, 2024 · Jan 25, 2024
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -14,6 +14,7 @@
 
 ### Bug fixes
 - Fix the way an advertised host configured with `--p2p-host` is treated when communicating with the originator of a PING packet [#6225](https://github.com/hyperledger/besu/pull/6225)
+- Fix `poa-block-txs-selection-max-time` option that was inadvertently reset to its default after being configured [#6444](https://github.com/hyperledger/besu/pull/6444)
 
 ### Download Links
 

diff --git a/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java b/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java
@@ -124,7 +124,6 @@
 import org.hyperledger.besu.ethereum.api.tls.TlsClientAuthConfiguration;
 import org.hyperledger.besu.ethereum.api.tls.TlsConfiguration;
 import org.hyperledger.besu.ethereum.chain.Blockchain;
-import org.hyperledger.besu.ethereum.core.ImmutableMiningParameters;
 import org.hyperledger.besu.ethereum.core.MiningParameters;
 import org.hyperledger.besu.ethereum.core.PrivacyParameters;
 import org.hyperledger.besu.ethereum.eth.sync.SyncMode;
@@ -225,6 +224,7 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.OptionalInt;
 import java.util.Set;
 import java.util.TreeMap;
 import java.util.function.Function;
@@ -2758,32 +2758,28 @@ private TransactionPoolConfiguration buildTransactionPoolConfiguration() {
 
   private MiningParameters getMiningParameters() {
     if (miningParameters == null) {
-      final var miningParametersBuilder =
-          ImmutableMiningParameters.builder().from(miningOptions.toDomainObject());
-      final var actualGenesisOptions = getActualGenesisConfigOptions();
-      if (actualGenesisOptions.isPoa()) {
-        miningParametersBuilder.genesisBlockPeriodSeconds(
-            getGenesisBlockPeriodSeconds(actualGenesisOptions));
-      }
-      miningParameters = miningParametersBuilder.build();
+      miningOptions.setGenesisBlockPeriodSeconds(
+          getGenesisBlockPeriodSeconds(getActualGenesisConfigOptions()));
+      miningParameters = miningOptions.toDomainObject();
     }
     return miningParameters;
   }
 
-  private int getGenesisBlockPeriodSeconds(final GenesisConfigOptions genesisConfigOptions) {
+  private OptionalInt getGenesisBlockPeriodSeconds(
+      final GenesisConfigOptions genesisConfigOptions) {
     if (genesisConfigOptions.isClique()) {
-      return genesisConfigOptions.getCliqueConfigOptions().getBlockPeriodSeconds();
+      return OptionalInt.of(genesisConfigOptions.getCliqueConfigOptions().getBlockPeriodSeconds());
     }
 
     if (genesisConfigOptions.isIbft2()) {
-      return genesisConfigOptions.getBftConfigOptions().getBlockPeriodSeconds();
+      return OptionalInt.of(genesisConfigOptions.getBftConfigOptions().getBlockPeriodSeconds());
     }
 
     if (genesisConfigOptions.isQbft()) {
-      return genesisConfigOptions.getQbftConfigOptions().getBlockPeriodSeconds();
+      return OptionalInt.of(genesisConfigOptions.getQbftConfigOptions().getBlockPeriodSeconds());
     }
 
-    throw new IllegalArgumentException("Should only be called for a PoA network");
+    return OptionalInt.empty();
   }
 
   private boolean isPruningEnabled() {
@@ -3246,7 +3242,12 @@ private Optional<String> getEcCurveFromGenesisFile() {
     return genesisConfigOptions.getEcCurve();
   }
 
-  private GenesisConfigOptions getActualGenesisConfigOptions() {
+  /**
+   * Return the genesis config options after applying any specified config overrides
+   *
+   * @return the genesis config options after applying any specified config overrides
+   */
+  protected GenesisConfigOptions getActualGenesisConfigOptions() {
     return Optional.ofNullable(genesisConfigOptions)
         .orElseGet(
             () ->

diff --git a/besu/src/main/java/org/hyperledger/besu/cli/options/MiningOptions.java b/besu/src/main/java/org/hyperledger/besu/cli/options/MiningOptions.java
@@ -40,6 +40,7 @@
 import org.hyperledger.besu.util.number.PositiveNumber;
 
 import java.util.List;
+import java.util.OptionalInt;
 
 import org.apache.tuweni.bytes.Bytes;
 import org.slf4j.Logger;
@@ -188,6 +189,8 @@ static class Unstable {
         DEFAULT_POS_BLOCK_CREATION_REPETITION_MIN_DURATION;
   }
 
+  private OptionalInt maybeGenesisBlockPeriodSeconds;
+
   private MiningOptions() {}
 
   /**
@@ -199,6 +202,16 @@ public static MiningOptions create() {
     return new MiningOptions();
   }
 
+  /**
+   * Set the optional genesis block period per seconds
+   *
+   * @param genesisBlockPeriodSeconds if the network is PoA then the block period in seconds
+   *     specified in the genesis file, otherwise empty.
+   */
+  public void setGenesisBlockPeriodSeconds(final OptionalInt genesisBlockPeriodSeconds) {
+    maybeGenesisBlockPeriodSeconds = genesisBlockPeriodSeconds;
+  }
+
   /**
    * Validate that there are no inconsistencies in the specified options. For example that the
    * options are valid for the selected implementation.
@@ -285,6 +298,7 @@ public void validate(
 
   static MiningOptions fromConfig(final MiningParameters miningParameters) {
     final MiningOptions miningOptions = MiningOptions.create();
+    miningOptions.setGenesisBlockPeriodSeconds(miningParameters.getGenesisBlockPeriodSeconds());
     miningOptions.isMiningEnabled = miningParameters.isMiningEnabled();
     miningOptions.iStratumMiningEnabled = miningParameters.isStratumMiningEnabled();
     miningOptions.stratumNetworkInterface = miningParameters.getStratumNetworkInterface();
@@ -319,6 +333,11 @@ static MiningOptions fromConfig(final MiningParameters miningParameters) {
 
   @Override
   public MiningParameters toDomainObject() {
+    if (maybeGenesisBlockPeriodSeconds == null) {
+      throw new IllegalStateException(
+          "genesisBlockPeriodSeconds must be set before using this object");
+    }
+
     final var updatableInitValuesBuilder =
         MutableInitValues.builder()
             .isMiningEnabled(isMiningEnabled)
@@ -334,27 +353,26 @@ public MiningParameters toDomainObject() {
       updatableInitValuesBuilder.coinbase(coinbase);
     }
 
-    final var miningParametersBuilder =
-        ImmutableMiningParameters.builder()
-            .mutableInitValues(updatableInitValuesBuilder.build())
-            .isStratumMiningEnabled(iStratumMiningEnabled)
-            .stratumNetworkInterface(stratumNetworkInterface)
-            .stratumPort(stratumPort)
-            .nonPoaBlockTxsSelectionMaxTime(nonPoaBlockTxsSelectionMaxTime)
-            .poaBlockTxsSelectionMaxTime(poaBlockTxsSelectionMaxTime)
-            .unstable(
-                ImmutableMiningParameters.Unstable.builder()
-                    .remoteSealersLimit(unstableOptions.remoteSealersLimit)
-                    .remoteSealersTimeToLive(unstableOptions.remoteSealersTimeToLive)
-                    .powJobTimeToLive(unstableOptions.powJobTimeToLive)
-                    .maxOmmerDepth(unstableOptions.maxOmmersDepth)
-                    .stratumExtranonce(unstableOptions.stratumExtranonce)
-                    .posBlockCreationMaxTime(unstableOptions.posBlockCreationMaxTime)
-                    .posBlockCreationRepetitionMinDuration(
-                        unstableOptions.posBlockCreationRepetitionMinDuration)
-                    .build());
-
-    return miningParametersBuilder.build();
+    return ImmutableMiningParameters.builder()
+        .genesisBlockPeriodSeconds(maybeGenesisBlockPeriodSeconds)
+        .mutableInitValues(updatableInitValuesBuilder.build())
+        .isStratumMiningEnabled(iStratumMiningEnabled)
+        .stratumNetworkInterface(stratumNetworkInterface)
+        .stratumPort(stratumPort)
+        .nonPoaBlockTxsSelectionMaxTime(nonPoaBlockTxsSelectionMaxTime)
+        .poaBlockTxsSelectionMaxTime(poaBlockTxsSelectionMaxTime)
+        .unstable(
+            ImmutableMiningParameters.Unstable.builder()
+                .remoteSealersLimit(unstableOptions.remoteSealersLimit)
+                .remoteSealersTimeToLive(unstableOptions.remoteSealersTimeToLive)
+                .powJobTimeToLive(unstableOptions.powJobTimeToLive)
+                .maxOmmerDepth(unstableOptions.maxOmmersDepth)
+                .stratumExtranonce(unstableOptions.stratumExtranonce)
+                .posBlockCreationMaxTime(unstableOptions.posBlockCreationMaxTime)
+                .posBlockCreationRepetitionMinDuration(
+                    unstableOptions.posBlockCreationRepetitionMinDuration)
+                .build())
+        .build();
   }
 
   @Override

diff --git a/besu/src/main/java/org/hyperledger/besu/cli/util/TomlConfigurationDefaultProvider.java b/besu/src/main/java/org/hyperledger/besu/cli/util/TomlConfigurationDefaultProvider.java
@@ -17,6 +17,7 @@
 import org.hyperledger.besu.datatypes.Wei;
 import org.hyperledger.besu.util.number.Fraction;
 import org.hyperledger.besu.util.number.Percentage;
+import org.hyperledger.besu.util.number.PositiveNumber;
 
 import java.io.File;
 import java.io.FileInputStream;
@@ -129,7 +130,8 @@ private boolean isNumericType(final Class<?> type) {
         || type.equals(Float.class)
         || type.equals(float.class)
         || type.equals(Percentage.class)
-        || type.equals(Fraction.class);
+        || type.equals(Fraction.class)
+        || type.equals(PositiveNumber.class);
   }
 
   private String getEntryAsString(final OptionSpec spec) {

diff --git a/besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java b/besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java
@@ -44,6 +44,7 @@
 import org.hyperledger.besu.cli.options.unstable.NetworkingOptions;
 import org.hyperledger.besu.cli.options.unstable.SynchronizerOptions;
 import org.hyperledger.besu.components.BesuComponent;
+import org.hyperledger.besu.config.GenesisConfigOptions;
 import org.hyperledger.besu.consensus.qbft.pki.PkiBlockCreationConfiguration;
 import org.hyperledger.besu.consensus.qbft.pki.PkiBlockCreationConfigurationProvider;
 import org.hyperledger.besu.controller.BesuController;
@@ -133,20 +134,37 @@
 @ExtendWith(MockitoExtension.class)
 public abstract class CommandTestAbstract {
   private static final Logger TEST_LOGGER = LoggerFactory.getLogger(CommandTestAbstract.class);
+
+  protected static final int POA_BLOCK_PERIOD_SECONDS = 5;
   protected static final JsonObject VALID_GENESIS_QBFT_POST_LONDON =
       (new JsonObject())
           .put(
               "config",
               new JsonObject()
                   .put("londonBlock", 0)
-                  .put("qbft", new JsonObject().put("blockperiodseconds", 5)));
+                  .put(
+                      "qbft",
+                      new JsonObject().put("blockperiodseconds", POA_BLOCK_PERIOD_SECONDS)));
   protected static final JsonObject VALID_GENESIS_IBFT2_POST_LONDON =
       (new JsonObject())
           .put(
               "config",
               new JsonObject()
                   .put("londonBlock", 0)
-                  .put("ibft2", new JsonObject().put("blockperiodseconds", 5)));
+                  .put(
+                      "ibft2",
+                      new JsonObject().put("blockperiodseconds", POA_BLOCK_PERIOD_SECONDS)));
+
+  protected static final JsonObject VALID_GENESIS_CLIQUE_POST_LONDON =
+      (new JsonObject())
+          .put(
+              "config",
+              new JsonObject()
+                  .put("londonBlock", 0)
+                  .put(
+                      "clique",
+                      new JsonObject().put("blockperiodseconds", POA_BLOCK_PERIOD_SECONDS)));
+
   protected final PrintStream originalOut = System.out;
   protected final PrintStream originalErr = System.err;
   protected final ByteArrayOutputStream commandOutput = new ByteArrayOutputStream();
@@ -561,6 +579,11 @@ protected Vertx createVertx(final VertxOptions vertxOptions) {
       return vertx;
     }
 
+    @Override
+    public GenesisConfigOptions getActualGenesisConfigOptions() {
+      return super.getActualGenesisConfigOptions();
+    }
+
     public CommandSpec getSpec() {
       return spec;
     }

diff --git a/besu/src/test/java/org/hyperledger/besu/cli/options/AbstractCLIOptionsTest.java b/besu/src/test/java/org/hyperledger/besu/cli/options/AbstractCLIOptionsTest.java
@@ -67,7 +67,10 @@ private void getCLIOptions(final D domainObject) {
     final TestBesuCommand cmd = parseCommand(cliOptions);
     final T optionsFromCommand = getOptionsFromBesuCommand(cmd);
 
-    assertThat(optionsFromCommand).usingRecursiveComparison().isEqualTo(options);
+    assertThat(optionsFromCommand)
+        .usingRecursiveComparison()
+        .ignoringFields(getNonOptionFields())
+        .isEqualTo(options);
 
     assertThat(commandOutput.toString(UTF_8)).isEmpty();
     assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
@@ -82,19 +85,23 @@ public void defaultValues() {
     final T optionsFromCommand = getOptionsFromBesuCommand(cmd);
 
     // Check default values supplied by CLI match expected default values
-    final String[] fieldsToIgnore = getFieldsWithComputedDefaults().toArray(new String[0]);
     assertThat(optionsFromCommand)
         .usingRecursiveComparison()
-        .ignoringFields(fieldsToIgnore)
+        .ignoringFields(getFieldsWithComputedDefaults())
+        .ignoringFields(getNonOptionFields())
         .isEqualTo(defaultOptions);
   }
 
   protected abstract D createDefaultDomainObject();
 
   protected abstract D createCustomizedDomainObject();
 
-  protected List<String> getFieldsWithComputedDefaults() {
-    return Collections.emptyList();
+  protected String[] getFieldsWithComputedDefaults() {
+    return new String[0];
+  }
+
+  protected String[] getNonOptionFields() {
+    return new String[0];
   }
 
   protected List<String> getFieldsToIgnore() {

diff --git a/besu/src/test/java/org/hyperledger/besu/cli/options/MiningOptionsTest.java b/besu/src/test/java/org/hyperledger/besu/cli/options/MiningOptionsTest.java
@@ -32,7 +32,9 @@
 
 import java.io.IOException;
 import java.nio.file.Path;
+import java.time.Duration;
 import java.util.Optional;
+import java.util.OptionalInt;
 
 import org.apache.tuweni.bytes.Bytes;
 import org.junit.jupiter.api.Test;
@@ -361,13 +363,16 @@ public void poaBlockTxsSelectionMaxTimeOption() throws IOException {
 
   @Test
   public void poaBlockTxsSelectionMaxTimeOptionOver100Percent() throws IOException {
-    final Path genesisFileIBFT2 = createFakeGenesisFile(VALID_GENESIS_IBFT2_POST_LONDON);
+    final Path genesisFileClique = createFakeGenesisFile(VALID_GENESIS_CLIQUE_POST_LONDON);
     internalTestSuccess(
-        miningParams ->
-            assertThat(miningParams.getPoaBlockTxsSelectionMaxTime())
-                .isEqualTo(PositiveNumber.fromInt(200)),
+        miningParams -> {
+          assertThat(miningParams.getPoaBlockTxsSelectionMaxTime())
+              .isEqualTo(PositiveNumber.fromInt(200));
+          assertThat(miningParams.getBlockTxsSelectionMaxTime())
+              .isEqualTo(Duration.ofSeconds(POA_BLOCK_PERIOD_SECONDS * 2).toMillis());
+        },
         "--genesis-file",
-        genesisFileIBFT2.toString(),
+        genesisFileClique.toString(),
         "--poa-block-txs-selection-max-time",
         "200");
   }
@@ -407,6 +412,16 @@ protected MiningOptions optionsFromDomainObject(final MiningParameters domainObj
 
   @Override
   protected MiningOptions getOptionsFromBesuCommand(final TestBesuCommand besuCommand) {
-    return besuCommand.getMiningOptions();
+    final var miningOptions = besuCommand.getMiningOptions();
+    miningOptions.setGenesisBlockPeriodSeconds(
+        besuCommand.getActualGenesisConfigOptions().isPoa()
+            ? OptionalInt.of(POA_BLOCK_PERIOD_SECONDS)
+            : OptionalInt.empty());
+    return miningOptions;
+  }
+
+  @Override
+  protected String[] getNonOptionFields() {
+    return new String[] {"maybeGenesisBlockPeriodSeconds"};
   }
 }
diff --git a/besu/src/test/java/org/hyperledger/besu/cli/options/SynchronizerOptionsTest.java b/besu/src/test/java/org/hyperledger/besu/cli/options/SynchronizerOptionsTest.java
@@ -82,8 +82,8 @@ protected SynchronizerConfiguration.Builder createCustomizedDomainObject() {
   }
 
   @Override
-  protected List<String> getFieldsWithComputedDefaults() {
-    return Arrays.asList("maxTrailingPeers", "computationParallelism");
+  protected String[] getFieldsWithComputedDefaults() {
+    return new String[] {"maxTrailingPeers", "computationParallelism"};
   }
 
   @Override