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

Conversation

cfelde
Copy link

@cfelde cfelde commented Sep 9, 2019

PR description

Adds the --target-gas-limit command line option.

Gradually changes the block gas limit towards this target when option enabled.

Fixed Issue(s)

https://pegasys1.atlassian.net/browse/PAN-3023

@CLAassistant
Copy link

CLAassistant commented Sep 9, 2019

CLA assistant check
All committers have signed the CLA.

@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

@NicolasMassart NicolasMassart added the api Related to public APIs label Sep 9, 2019
@NicolasMassart NicolasMassart self-assigned this Sep 9, 2019
@NicolasMassart NicolasMassart added the enhancement New feature or request label Sep 9, 2019
@NicolasMassart NicolasMassart removed the enhancement New feature or request label Sep 9, 2019
Copy link
Contributor

@shemnon shemnon left a comment

Choose a reason for hiding this comment

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

Can we get a test that exercises the 1/1024th gas adjustment? tech.pegasys.pantheon.ethereum.blockcreation.EthHashBlockCreatorTest is probably a good place.

@@ -84,6 +85,7 @@
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..

@cfelde
Copy link
Author

cfelde commented Sep 10, 2019

Great feedback, I'll start working through this on Thursday when back at my computer.

# Conflicts:
#	acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/ThreadPantheonNodeRunner.java
#	pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java
#	pantheon/src/main/java/tech/pegasys/pantheon/controller/PantheonControllerBuilder.java
#	pantheon/src/test/java/tech/pegasys/pantheon/cli/CommandTestAbstract.java
#	pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java
#	pantheon/src/test/resources/everything_config.toml
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api Related to public APIs
Projects
None yet
4 participants