Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

[PAN-3023] Add command line option for target gas limit #1917

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ public void startNode(final PantheonNode node) {
.ethProtocolConfiguration(EthProtocolConfiguration.defaultConfig())
.clock(Clock.systemUTC())
.isRevertReasonEnabled(node.isRevertReasonEnabled())
.gasLimitCalculator((gasLimit) -> gasLimit)
cfelde marked this conversation as resolved.
Show resolved Hide resolved
.build();
} catch (final IOException e) {
throw new RuntimeException("Error building PantheonController", e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,16 @@ public CliqueMinerExecutor(
final KeyPair nodeKeys,
final MiningParameters miningParams,
final AbstractBlockScheduler blockScheduler,
final EpochManager epochManager) {
final EpochManager epochManager,
final Function<Long, Long> gasLimitCalculator) {
super(
protocolContext,
executorService,
protocolSchedule,
pendingTransactions,
miningParams,
blockScheduler);
blockScheduler,
gasLimitCalculator);
this.nodeKeys = nodeKeys;
this.localAddress = Util.publicKeyToAddress(nodeKeys.getPublicKey());
this.epochManager = epochManager;
Expand Down Expand Up @@ -89,7 +91,7 @@ private CliqueBlockMiner createMiner(
pendingTransactions,
protocolContext,
protocolSchedule,
(gasLimit) -> gasLimit,
gasLimitCalculator,
nodeKeys,
minTransactionGasPrice,
header,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ public void extraDataCreatedOnEpochBlocksContainsValidators() {
proposerKeyPair,
new MiningParameters(AddressHelpers.ofValue(1), Wei.ZERO, vanityData, false),
mock(CliqueBlockScheduler.class),
new EpochManager(EPOCH_LENGTH));
new EpochManager(EPOCH_LENGTH),
(gasLimit) -> gasLimit);

// NOTE: Passing in the *parent* block, so must be 1 less than EPOCH
final BlockHeader header = blockHeaderBuilder.number(EPOCH_LENGTH - 1).buildHeader();
Expand Down Expand Up @@ -135,7 +136,8 @@ public void extraDataForNonEpochBlocksDoesNotContainValidaors() {
proposerKeyPair,
new MiningParameters(AddressHelpers.ofValue(1), Wei.ZERO, vanityData, false),
mock(CliqueBlockScheduler.class),
new EpochManager(EPOCH_LENGTH));
new EpochManager(EPOCH_LENGTH),
(gasLimit) -> gasLimit);

// Parent block was epoch, so the next block should contain no validators.
final BlockHeader header = blockHeaderBuilder.number(EPOCH_LENGTH).buildHeader();
Expand Down Expand Up @@ -172,7 +174,8 @@ public void shouldUseLatestVanityData() {
proposerKeyPair,
new MiningParameters(AddressHelpers.ofValue(1), Wei.ZERO, initialVanityData, false),
mock(CliqueBlockScheduler.class),
new EpochManager(EPOCH_LENGTH));
new EpochManager(EPOCH_LENGTH),
(gasLimit) -> gasLimit);

executor.setExtraData(modifiedVanityData);
final BytesValue extraDataBytes = executor.calculateExtraData(blockHeaderBuilder.buildHeader());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

import java.util.Optional;
import java.util.concurrent.ExecutorService;
import java.util.function.Function;

public abstract class AbstractMinerExecutor<
C, M extends BlockMiner<C, ? extends AbstractBlockCreator<C>>> {
Expand All @@ -34,6 +35,7 @@ public abstract class AbstractMinerExecutor<
protected final ProtocolSchedule<C> protocolSchedule;
protected final PendingTransactions pendingTransactions;
protected final AbstractBlockScheduler blockScheduler;
protected final Function<Long, Long> gasLimitCalculator;

protected volatile BytesValue extraData;
protected volatile Wei minTransactionGasPrice;
Expand All @@ -44,14 +46,16 @@ public AbstractMinerExecutor(
final ProtocolSchedule<C> protocolSchedule,
final PendingTransactions pendingTransactions,
final MiningParameters miningParams,
final AbstractBlockScheduler blockScheduler) {
final AbstractBlockScheduler blockScheduler,
final Function<Long, Long> gasLimitCalculator) {
this.protocolContext = protocolContext;
this.executorService = executorService;
this.protocolSchedule = protocolSchedule;
this.pendingTransactions = pendingTransactions;
this.extraData = miningParams.getExtraData();
this.minTransactionGasPrice = miningParams.getMinTransactionGasPrice();
this.blockScheduler = blockScheduler;
this.gasLimitCalculator = gasLimitCalculator;
}

public abstract M startAsyncMining(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,16 @@ public EthHashMinerExecutor(
final ProtocolSchedule<Void> protocolSchedule,
final PendingTransactions pendingTransactions,
final MiningParameters miningParams,
final AbstractBlockScheduler blockScheduler) {
final AbstractBlockScheduler blockScheduler,
final Function<Long, Long> gasLimitCalculator) {
super(
protocolContext,
executorService,
protocolSchedule,
pendingTransactions,
miningParams,
blockScheduler);
blockScheduler,
gasLimitCalculator);
this.coinbase = miningParams.getCoinbase();
}

Expand Down Expand Up @@ -77,7 +79,7 @@ private EthHashBlockMiner createMiner(
pendingTransactions,
protocolContext,
protocolSchedule,
(gasLimit) -> gasLimit,
gasLimitCalculator,
solver,
minTransactionGasPrice,
parentHeader);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ public void startingMiningWithoutCoinbaseThrowsException() {
null,
pendingTransactions,
miningParameters,
new DefaultBlockScheduler(1, 10, TestClock.fixed()));
new DefaultBlockScheduler(1, 10, TestClock.fixed()),
(gasLimit) -> gasLimit);

assertThatExceptionOfType(CoinbaseNotSetException.class)
.isThrownBy(() -> executor.startAsyncMining(Subscribers.create(), null))
Expand All @@ -74,7 +75,8 @@ public void settingCoinbaseToNullThrowsException() {
null,
pendingTransactions,
miningParameters,
new DefaultBlockScheduler(1, 10, TestClock.fixed()));
new DefaultBlockScheduler(1, 10, TestClock.fixed()),
(gasLimit) -> gasLimit);

assertThatExceptionOfType(IllegalArgumentException.class)
.isThrownBy(() -> executor.setCoinbase(null))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Collectors;

Expand Down Expand Up @@ -633,6 +634,12 @@ void setBannedNodeIds(final List<String> values) {
"The name of a file containing the private key used to sign privacy marker transactions. If unset, each will be signed with a random key.")
private final Path privacyMarkerTransactionSigningKeyPath = null;

@Option(
names = {"--target-gas-limit"},
description =
"Sets target gas limit per block. If set each blocks gas limit will approach this setting over time if the current gas limit is different.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MadelineMurray for help description text review

private final Long targetGasLimit = null;

@Option(
names = {"--tx-pool-max-size"},
paramLabel = MANDATORY_INTEGER_FORMAT_HELP,
Expand All @@ -657,6 +664,7 @@ void setBannedNodeIds(final List<String> values) {
private MetricsConfiguration metricsConfiguration;
private Optional<PermissioningConfiguration> permissioningConfiguration;
private Collection<EnodeURL> staticNodes;
private Function<Long, Long> gasLimitCalculator;
private PantheonController<?> pantheonController;

private final Supplier<ObservableMetricsSystem> metricsSystem =
Expand Down Expand Up @@ -879,6 +887,7 @@ private PantheonCommand configure() throws Exception {
webSocketConfiguration = webSocketConfiguration();
permissioningConfiguration = permissioningConfiguration();
staticNodes = loadStaticNodes();
gasLimitCalculator = gasLimitCalculator();
logger.info("Connecting to {} static nodes.", staticNodes.size());
logger.trace("Static Nodes = {}", staticNodes);
final List<URI> enodeURIs =
Expand Down Expand Up @@ -946,7 +955,8 @@ public PantheonControllerBuilder<?> getControllerBuilder() {
.clock(Clock.systemUTC())
.isRevertReasonEnabled(isRevertReasonEnabled)
.isPruningEnabled(isPruningEnabled)
.pruningConfiguration(buildPruningConfiguration());
.pruningConfiguration(buildPruningConfiguration())
.gasLimitCalculator(gasLimitCalculator);
} catch (final IOException e) {
throw new ExecutionException(this.commandLine, "Invalid path", e);
}
Expand Down Expand Up @@ -1536,6 +1546,31 @@ private Set<EnodeURL> loadStaticNodes() throws IOException {
return StaticNodesParser.fromPath(staticNodesPath);
}

private Function<Long, Long> gasLimitCalculator() {
cfelde marked this conversation as resolved.
Show resolved Hide resolved
final long targetGasLimit = this.targetGasLimit == null ? 0L : this.targetGasLimit;
final long adjustmentFactor = 1024L;

if (targetGasLimit > 0L) {
return (gasLimit) -> {
long newGasLimit;

if (targetGasLimit > gasLimit) {
newGasLimit = Math.min(targetGasLimit, gasLimit + adjustmentFactor);
} else if (targetGasLimit < gasLimit) {
newGasLimit = Math.max(targetGasLimit, gasLimit - adjustmentFactor);
} else {
return gasLimit;
}

logger.debug("Adjusting block gas limit from {} to {}", gasLimit, newGasLimit);

return newGasLimit;
};
} else {
return gasLimit -> gasLimit;
}
}

public PantheonExceptionHandler exceptionHandler() {
return new PantheonExceptionHandler(this::getLogLevel);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ protected MiningCoordinator createMiningCoordinator(
protocolContext.getConsensusState().getVoteTallyCache(),
localAddress,
secondsBetweenBlocks),
epochManager);
epochManager,
gasLimitCalculator);
final CliqueMiningCoordinator miningCoordinator =
new CliqueMiningCoordinator(
protocolContext.getBlockchain(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ protected MiningCoordinator createMiningCoordinator(

final IbftBlockCreatorFactory blockCreatorFactory =
new IbftBlockCreatorFactory(
(gasLimit) -> gasLimit,
gasLimitCalculator,
transactionPool.getPendingTransactions(),
protocolContext,
protocolSchedule,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ protected MiningCoordinator createMiningCoordinator(
new DefaultBlockScheduler(
MainnetBlockHeaderValidator.MINIMUM_SECONDS_SINCE_PARENT,
MainnetBlockHeaderValidator.TIMESTAMP_TOLERANCE_S,
clock));
clock),
gasLimitCalculator);

final EthHashMiningCoordinator miningCoordinator =
new EthHashMiningCoordinator(protocolContext.getBlockchain(), executor, syncState);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
import java.util.Optional;
import java.util.OptionalLong;
import java.util.concurrent.Executors;
import java.util.function.Function;

import com.google.common.util.concurrent.ThreadFactoryBuilder;
import org.apache.logging.log4j.LogManager;
Expand All @@ -84,6 +85,7 @@ public abstract class PantheonControllerBuilder<C> {
protected Clock clock;
protected KeyPair nodeKeys;
protected boolean isRevertReasonEnabled;
protected Function<Long, Long> gasLimitCalculator;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a type Function field I think we should consider a special case GasLimitCalculator functional interface. It will make it more obvious what the type does. Here and the other places in the code we us it (the compiler errors should tell you where, one advantage of a strongly typed language).

It will also give a home to the eth spec standard 'no more than 1/1024th per block` adjustment code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standalone replacement for GasLimitCalculator is optional, but we do need to find a better home for the calculator in PantheonCommand

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing that maybe argues in favor of a standalone GasLimitCalculator is what would be needed to have PAN-3031 (Add JSON-RPC method to set target block gas limit) later on. I'll look at some as well..

private StorageProvider storageProvider;
private final List<Runnable> shutdownActions = new ArrayList<>();
private RocksDbConfiguration rocksDbConfiguration;
Expand Down Expand Up @@ -181,6 +183,12 @@ public PantheonControllerBuilder<C> pruningConfiguration(
return this;
}

public PantheonControllerBuilder<C> gasLimitCalculator(
final Function<Long, Long> gasLimitCalculator) {
this.gasLimitCalculator = gasLimitCalculator;
return this;
}

public PantheonController<C> build() throws IOException {
checkNotNull(genesisConfig, "Missing genesis config");
checkNotNull(syncConfig, "Missing sync config");
Expand All @@ -193,6 +201,7 @@ public PantheonController<C> build() throws IOException {
checkNotNull(clock, "Mising clock");
checkNotNull(transactionPoolConfiguration, "Missing transaction pool configuration");
checkNotNull(nodeKeys, "Missing node keys");
checkNotNull(gasLimitCalculator, "Missing gas limit calculator");
checkArgument(
storageProvider != null || rocksDbConfiguration != null,
"Must supply either a storage provider or RocksDB configuration");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ public void privacyPrecompiled() throws IOException {
.clock(TestClock.fixed())
.privacyParameters(privacyParameters)
.transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
.gasLimitCalculator(gasLimit -> gasLimit)
.build();

final Address privacyContractAddress = Address.privacyPrecompiled(ADDRESS);
Expand Down
3 changes: 3 additions & 0 deletions pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ private void syncFromGenesis(final SyncMode mode) throws Exception {
.clock(TestClock.fixed())
.transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
.storageProvider(createKeyValueStorageProvider(dbAhead))
.gasLimitCalculator(gasLimit -> gasLimit)
.build()) {
setupState(blockCount, controller.getProtocolSchedule(), controller.getProtocolContext());
}
Expand All @@ -154,6 +155,7 @@ private void syncFromGenesis(final SyncMode mode) throws Exception {
.clock(TestClock.fixed())
.transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
.storageProvider(createKeyValueStorageProvider(dbAhead))
.gasLimitCalculator(gasLimit -> gasLimit)
.build();
final String listenHost = InetAddress.getLoopbackAddress().getHostAddress();
final JsonRpcConfiguration aheadJsonRpcConfiguration = jsonRpcConfiguration();
Expand Down Expand Up @@ -212,6 +214,7 @@ private void syncFromGenesis(final SyncMode mode) throws Exception {
.privacyParameters(PrivacyParameters.DEFAULT)
.clock(TestClock.fixed())
.transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
.gasLimitCalculator(gasLimit -> gasLimit)
.build();
final EnodeURL enode = runnerAhead.getLocalEnode().get();
final EthNetworkConfig behindEthNetworkConfiguration =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ private static PantheonController<?> createController() throws IOException {
.dataDirectory(dataDir)
.clock(TestClock.fixed())
.transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
.gasLimitCalculator(gasLimit -> gasLimit)
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,7 @@ protected PantheonController<?> createController(final GenesisConfigFile genesis
.dataDirectory(dataDir)
.clock(TestClock.fixed())
.transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
.gasLimitCalculator(gasLimit -> gasLimit)
.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ public void blockImport() throws IOException {
.dataDirectory(dataDir)
.clock(TestClock.fixed())
.transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
.gasLimitCalculator(gasLimit -> gasLimit)
.build();
final RlpBlockImporter.ImportResult result =
rlpBlockImporter.importBlockchain(source, targetController);
Expand Down Expand Up @@ -105,6 +106,7 @@ public void ibftImport() throws IOException {
.dataDirectory(dataDir)
.clock(TestClock.fixed())
.transactionPoolConfiguration(TransactionPoolConfiguration.builder().build())
.gasLimitCalculator(gasLimit -> gasLimit)
.build();
final RlpBlockImporter.ImportResult result =
rlpBlockImporter.importBlockchain(source, controller);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ public void initMocks() throws Exception {
when(mockControllerBuilder.isRevertReasonEnabled(false)).thenReturn(mockControllerBuilder);
when(mockControllerBuilder.isPruningEnabled(anyBoolean())).thenReturn(mockControllerBuilder);
when(mockControllerBuilder.pruningConfiguration(any())).thenReturn(mockControllerBuilder);
when(mockControllerBuilder.gasLimitCalculator(any())).thenReturn(mockControllerBuilder);

// doReturn used because of generic PantheonController
doReturn(mockController).when(mockControllerBuilder).build();
Expand Down
Loading