New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow API configuration to override supplied gasLimit #6978
Allow API configuration to override supplied gasLimit #6978
Conversation
LOG.info("Capping gasLimit to " + gasCap); | ||
} | ||
gasLimit = rpcGasCap; | ||
LOG.info("Capping gasLimit to " + rpcGasCap); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so no longer comparing gasCap to gasLimit, just if > zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it is essentially an override value from the cli. @matkt can speak to the specifics about why we would raise the cap beyond the requested limit, but I think it has something to do with simulating transactions that will exceed the anticipated gas, but not exceed some predefined hard cap (DoS vector).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it is need in order to allow user setting a high gasLimit for ethcall
@@ -565,8 +565,9 @@ public void shouldCapGasLimitWhenOriginalTransactionExceedsGasCap() { | |||
} | |||
|
|||
@Test | |||
public void shouldKeepOriginalGasLimitWhenCapIsHigherThanOriginalValue() { | |||
// generate a transaction with a gas limit that is lower than the gas cap | |||
public void shouldUseRpcGasCapWhenCapIsHigherThanGasLimit() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if cap is lower than gasLimit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, this test can really just be renamed to something like "when present rpc gas cap supersedes request gas limit"
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
f40d66c
to
c3e9cf3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
PR description
Allow API configuration to override supplied gasLimit. Originally this commit was part of #6641. Moved here for visibility and easier review.
Fixed Issue(s)
Thanks for sending a pull request! Have you done the following?
doc-change-required
label to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew build
./gradlew acceptanceTest
./gradlew integrationTest
./gradlew ethereum:referenceTests:referenceTests