From 16016ece0a2f8abe733e832c53f49c591549762a Mon Sep 17 00:00:00 2001 From: Fabio Di Fabio Date: Thu, 25 Jan 2024 10:38:27 +0100 Subject: [PATCH] Fix `poa-block-txs-selection-max-time` option that was inadvertently reset to its default after being configured (#6444) Signed-off-by: Fabio Di Fabio --- CHANGELOG.md | 1 + .../org/hyperledger/besu/cli/BesuCommand.java | 31 +++++----- .../besu/cli/options/MiningOptions.java | 60 ++++++++++++------- .../TomlConfigurationDefaultProvider.java | 4 +- .../besu/cli/CommandTestAbstract.java | 27 ++++++++- .../cli/options/AbstractCLIOptionsTest.java | 17 ++++-- .../besu/cli/options/MiningOptionsTest.java | 27 +++++++-- .../cli/options/SynchronizerOptionsTest.java | 4 +- 8 files changed, 119 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f966114ed92..57416d07852 100644 --- 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 index d30bb421470..b9a137cbc17 100644 --- 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 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 index 55197b8e826..2e5a239f16d 100644 --- 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 index 4928d3f7ff6..a346e260d13 100644 --- 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 index 2c5f68be8c5..1609758b966 100644 --- 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 index 92b190d74bf..21d8baf9ee9 100644 --- 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,10 +85,10 @@ 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); } @@ -93,8 +96,12 @@ public void defaultValues() { protected abstract D createCustomizedDomainObject(); - protected List getFieldsWithComputedDefaults() { - return Collections.emptyList(); + protected String[] getFieldsWithComputedDefaults() { + return new String[0]; + } + + protected String[] getNonOptionFields() { + return new String[0]; } protected List 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 index 4b1fdddb53a..74b696cdf9a 100644 --- 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 index 3b1a9e6839a..9585fadf0d8 100644 --- 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 getFieldsWithComputedDefaults() { - return Arrays.asList("maxTrailingPeers", "computationParallelism"); + protected String[] getFieldsWithComputedDefaults() { + return new String[] {"maxTrailingPeers", "computationParallelism"}; } @Override