Skip to content

Commit

Permalink
Fix poa-block-txs-selection-max-time option that was inadvertently …
Browse files Browse the repository at this point in the history 
…reset to its default after being configured (hyperledger#6444)


Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
  • Loading branch information
@fab-10
fab-10 authored Jan 25, 2024
1 parent 6ac419b commit 16016ec
Show file tree
Hide file tree
Showing 8 changed files with 119 additions and 52 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
31 changes: 16 additions & 15 deletions besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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(
() ->
Expand Down
60 changes: 39 additions & 21 deletions besu/src/main/java/org/hyperledger/besu/cli/options/MiningOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -188,6 +189,8 @@ static class Unstable {
DEFAULT_POS_BLOCK_CREATION_REPETITION_MIN_DURATION;
}

private OptionalInt maybeGenesisBlockPeriodSeconds;

private MiningOptions() {}

/**
Expand All @@ -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.
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down
4 changes: 3 additions & 1 deletion besu/src/main/java/org/hyperledger/besu/cli/util/TomlConfigurationDefaultProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
27 changes: 25 additions & 2 deletions besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -561,6 +579,11 @@ protected Vertx createVertx(final VertxOptions vertxOptions) {
return vertx;
}

@Override
public GenesisConfigOptions getActualGenesisConfigOptions() {
return super.getActualGenesisConfigOptions();
}

public CommandSpec getSpec() {
return spec;
}
Expand Down
17 changes: 12 additions & 5 deletions besu/src/test/java/org/hyperledger/besu/cli/options/AbstractCLIOptionsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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() {
Expand Down
27 changes: 21 additions & 6 deletions besu/src/test/java/org/hyperledger/besu/cli/options/MiningOptionsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
}
Expand Down Expand Up @@ -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"};
}
}
4 changes: 2 additions & 2 deletions besu/src/test/java/org/hyperledger/besu/cli/options/SynchronizerOptionsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 16016ec

Please sign in to comment.