Skip to content

estimate gas - don't override gasPrice if not set #8472

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
Merged
macfarla merged 4 commits into from
Mar 29, 2025
Merged

estimate gas - don't override gasPrice if not set #8472

macfarla merged 4 commits into from
Mar 29, 2025

Conversation

@macfarla
Copy link
Contributor

@macfarla macfarla commented Mar 27, 2025

Signed-off-by: Sally MacFarlane macfarla.github@gmail.com

PR description

If not set in call params, don't override gasPrice. TransactionSimulator does the work of deciding whether it is set and whether it needs overriding.

Fixed Issue(s)

This was discovered by looking at rpc-compat hive tests but doesn't actually fix any hive tests directly.
What is fixed by this is transactions will be detected as EIP1559 if no gas price set.

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:

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

macfarla added 2 commits March 27, 2025 10:40
@macfarla
don't override gasPrice
c629d41 
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla
Merge branch 'main' of github.com:hyperledger/besu into gas-estimate-…
db73d35 
…dont-override-gas-price
@macfarla macfarla added RPC hive relating to hive tests labels Mar 27, 2025
@macfarla macfarla requested a review from fab-10 March 27, 2025 00:51
@macfarla
Copy link
Contributor Author

macfarla commented Mar 27, 2025

before this PR - note tx type is FRONTIER

2025-03-26 00:50:05.997+00:00 | vert.x-worker-thread-6 | TRACE | AbstractJsonRpcExecutor | {"jsonrpc":"2.0","id":1,"method":"eth_estimateGas","params":[{"from":"0x0102030000000000000000000000000000000000","input":"0xff01","to":"0x17e7eedce4ac02ef114a7ed9fe6e2f33feba1667"}]}
2025-03-26 00:50:05.998+00:00 | vert.x-worker-thread-6 | TRACE | JsonRpcExecutor | JSON-RPC request -> eth_estimateGas [{"from":"0x0102030000000000000000000000000000000000","input":"0xff01","to":"0x17e7eedce4ac02ef114a7ed9fe6e2f33feba1667"}]
2025-03-26 00:50:05.998+00:00 | vert.x-worker-thread-6 | DEBUG | EthEstimateGas | Processing transaction with params: CallParameter{chainId=Optional.empty, from=0x0102030000000000000000000000000000000000, to=0x17e7eedce4ac02ef114a7ed9fe6e2f33feba1667, gasLimit=-1, maxPriorityFeePerGas=N/A, maxFeePerGas=N/A, maxFeePerBlobGas=N/A, gasPrice=N/A, value=N/A, payloadSize=2, accessListSize=Optional.empty, blobVersionedHashesSize=Optional.empty, nonce=N/A}
2025-03-26 00:50:05.998+00:00 | vert.x-worker-thread-6 | TRACE | TransactionSimulator | User provided gas limit 75398208 is bigger than the value of rpc-gas-cap 50000000, setting simulation gas cap to the latter
2025-03-26 00:50:05.998+00:00 | vert.x-worker-thread-6 | TRACE | MainnetTransactionProcessor | Starting execution of MessageCall{type=FRONTIER, nonce=0, gasPrice=0 wei, gasLimit=50000000, to=0x17e7eedce4ac02ef114a7ed9fe6e2f33feba1667, value=0x0000000000000000000000000000000000000000000000000000000000000000, sig=Signature{r=57896044618658097711785492504343953926418782139537452191302581570759080747168, s=57896044618658097711785492504343953926418782139537452191302581570759080747168, recId=0}, payload=0xff01}

after this PR - EIP1559

2025-03-27 00:09:15.449+00:00 | vert.x-worker-thread-6 | TRACE | AbstractJsonRpcExecutor | {"jsonrpc":"2.0","id":1,"method":"eth_estimateGas","params":[{"from":"0x0102030000000000000000000000000000000000","input":"0xff01","to":"0x17e7eedce4ac02ef114a7ed9fe6e2f33feba1667"}]}
2025-03-27 00:09:15.450+00:00 | vert.x-worker-thread-6 | TRACE | JsonRpcExecutor | JSON-RPC request -> eth_estimateGas [{"from":"0x0102030000000000000000000000000000000000","input":"0xff01","to":"0x17e7eedce4ac02ef114a7ed9fe6e2f33feba1667"}]
2025-03-27 00:09:15.450+00:00 | vert.x-worker-thread-6 | DEBUG | EthEstimateGas | Processing transaction with params: CallParameter{chainId=Optional.empty, from=0x0102030000000000000000000000000000000000, to=0x17e7eedce4ac02ef114a7ed9fe6e2f33feba1667, gasLimit=-1, maxPriorityFeePerGas=N/A, maxFeePerGas=N/A, maxFeePerBlobGas=N/A, gasPrice=N/A, value=N/A, payloadSize=2, accessListSize=Optional.empty, blobVersionedHashesSize=Optional.empty, nonce=N/A}
2025-03-27 00:09:15.450+00:00 | vert.x-worker-thread-6 | TRACE | TransactionSimulator | User provided gas limit 75398208 is bigger than the value of rpc-gas-cap 50000000, setting simulation gas cap to the latter
2025-03-27 00:09:15.450+00:00 | vert.x-worker-thread-6 | TRACE | MainnetTransactionProcessor | Starting execution of MessageCall{type=EIP1559, nonce=0, gasPrice=0 wei, maxPriorityFeePerGas=0 wei, maxFeePerGas=0 wei, gasLimit=50000000, to=0x17e7eedce4ac02ef114a7ed9fe6e2f33feba1667, value=0x0000000000000000000000000000000000000000000000000000000000000000, sig=Signature{r=57896044618658097711785492504343953926418782139537452191302581570759080747168,

@macfarla
Copy link
Contributor Author

macfarla commented Mar 27, 2025

kudos @Gabriel-Trintinalia for pinpointing this

@macfarla
Copy link
Contributor Author

macfarla commented Mar 27, 2025

TODO fix the EthEstimateGas tests - mocking is now broken

@macfarla macfarla marked this pull request as draft March 27, 2025 08:25
@macfarla macfarla mentioned this pull request Mar 27, 2025
Merged
9 tasks
macfarla added 2 commits March 29, 2025 10:17
@macfarla
Merge branch 'main' of github.com:hyperledger/besu into gas-estimate-…
abf66eb 
…dont-override-gas-price
@macfarla
fix tests
093d167 
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla macfarla marked this pull request as ready for review March 29, 2025 00:56
@macfarla macfarla merged commit f54df36 into hyperledger:main Mar 29, 2025
43 checks passed
@macfarla macfarla mentioned this pull request Apr 2, 2025
Merged
9 tasks
@macfarla macfarla deleted the gas-estimate-dont-override-gas-price branch April 14, 2025 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fab-10 fab-10 fab-10 approved these changes

Assignees

No one assigned

Labels

hive relating to hive tests RPC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@macfarla @fab-10