Skip to content
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

Conversation

garyschulte
Copy link
Contributor

@garyschulte garyschulte commented Apr 22, 2024

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?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

LOG.info("Capping gasLimit to " + gasCap);
}
gasLimit = rpcGasCap;
LOG.info("Capping gasLimit to " + rpcGasCap);
Copy link
Contributor

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?

Copy link
Contributor Author

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).

Copy link
Contributor

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() {
Copy link
Contributor

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

Copy link
Contributor Author

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"

matkt and others added 2 commits April 29, 2024 14:31
Signed-off-by: Karim Taam <karim.t2am@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte garyschulte force-pushed the feature/configurable-simulation-tx-gas-limit branch from f40d66c to c3e9cf3 Compare April 29, 2024 21:31
Copy link
Contributor

@matkt matkt left a comment

Choose a reason for hiding this comment

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

Lgtm

@garyschulte garyschulte enabled auto-merge (squash) April 30, 2024 22:54
@garyschulte garyschulte merged commit afea4e0 into hyperledger:main May 1, 2024
42 checks passed
@garyschulte garyschulte deleted the feature/configurable-simulation-tx-gas-limit branch May 1, 2024 16:16
@garyschulte garyschulte added the doc-change-required Indicates an issue or PR that requires doc to be updated label May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-change-required Indicates an issue or PR that requires doc to be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants