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.

Sign up for GitHub

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

Jump to bottom

Add rpc-gas-cap via transaction simulator #6156

Merged
merged 20 commits into from
Nov 21, 2023
Merged

Add rpc-gas-cap via transaction simulator #6156

merged 20 commits into from
Nov 21, 2023

Conversation

gfukushima
Copy link
Contributor

PR description

This PR adds rpc-gas-cap directly into the transaction simulator, which seems a better approach than the one used in #6130.

Fixed Issue(s)

Fixes #6042

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 10, 2023 •
edited by gfukushima
Loading

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

@gfukushima gfukushima marked this pull request as ready for review November 12, 2023 23:41
@gfukushima gfukushima mentioned this pull request Nov 13, 2023
Closed
@gfukushima gfukushima requested a review from shemnon November 13, 2023 14:34
siladu
siladu reviewed Nov 13, 2023
View reviewed changes
Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

Looks straight forward enough, just want to understand what TransactionSimulatorTest is testing before approving.

nit: From a readability POV, I like the idea of a GasCappedTransactionSimulator class.

besu/src/main/java/org/hyperledger/besu/RunnerBuilder.java Outdated
/**
* Add Rpc gasLimit cap .
*
* @param rpcGasCap the rpc max logs range
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java
@@ -1587,6 +1587,20 @@ public void rpcMaxLogsRangeOptionMustBeUsed() {
assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
}

@Test
public void rpcGasCapOptionMustBeUsed() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Bit confused what this test name means...it's optional right? In what sense must it be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added following the pattern that a lot of other CLI options follow. Agree that the name could be better but I take that it means that the option must be used when specified and throw no error.

...m/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/methods/EthJsonRpcMethods.java
@@ -96,7 +97,8 @@ public EthJsonRpcMethods(
final TransactionPool transactionPool,
final MiningCoordinator miningCoordinator,
final Set<Capability> supportedCapabilities,
final Optional<Long> maxLogRange) {
final Optional<Long> maxLogRange,
final Optional<Long> gasCap) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was wondering if it might make sense to use Long.MAX_VALUE instead of a optional field...but also like that this same pattern as maxLogRange.

siladu
siladu approved these changes Nov 17, 2023
View reviewed changes
Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

LGTM

@gfukushima gfukushima enabled auto-merge (squash) November 21, 2023 13:36
@gfukushima gfukushima added the doc-change-required Indicates an issue or PR that requires doc to be updated label Nov 21, 2023
@gfukushima
Add changelog
4302214 
Signed-off-by: Gabriel Fukushima <[email protected]>
@gfukushima
Merge remote-tracking branch 'origin/rpcGasCap_via_transactionSimulat…
23eb8e8 
…or' into rpcGasCap_via_transactionSimulator

# Conflicts:
#	CHANGELOG.md
@gfukushima gfukushima merged commit a5d5af0 into hyperledger:main Nov 21, 2023
jflo pushed a commit to jflo/besu that referenced this pull request Dec 4, 2023
@gfukushima @jflo
Add rpc-gas-cap via transaction simulator (hyperledger#6156)
73f6b7a 
* Add `rpc-gas-cap` to enable user to cap gasLimit of certain RPC methods
Signed-off-by: Gabriel Fukushima <[email protected]>

---------

Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Justin Florentine <[email protected]>
jflo pushed a commit to jflo/besu that referenced this pull request Dec 4, 2023
@gfukushima @jflo
Add rpc-gas-cap via transaction simulator (hyperledger#6156)
1f099f7 
* Add `rpc-gas-cap` to enable user to cap gasLimit of certain RPC methods
Signed-off-by: Gabriel Fukushima <[email protected]>

---------

Signed-off-by: Gabriel Fukushima <[email protected]>
jflo pushed a commit to jflo/besu that referenced this pull request Dec 4, 2023
@gfukushima @jflo
Add rpc-gas-cap via transaction simulator (hyperledger#6156)
ae34bf9 
* Add `rpc-gas-cap` to enable user to cap gasLimit of certain RPC methods
Signed-off-by: Gabriel Fukushima <[email protected]>

---------

Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Justin Florentine <[email protected]>
@alexandratran alexandratran mentioned this pull request Dec 12, 2023
Closed
@alexandratran alexandratran removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Dec 12, 2023
gfukushima added a commit to gfukushima/besu that referenced this pull request Dec 15, 2023
@gfukushima
Add rpc-gas-cap via transaction simulator (hyperledger#6156)
5802a08 
* Add `rpc-gas-cap` to enable user to cap gasLimit of certain RPC methods
Signed-off-by: Gabriel Fukushima <[email protected]>

---------

Signed-off-by: Gabriel Fukushima <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siladu siladu siladu approved these changes

@shemnon shemnon Awaiting requested review from shemnon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature Request: eth_call gas limit (rpc.gascap)
3 participants
@gfukushima @siladu @alexandratran