Skip to content

adjustments to gas estimation #8478

Merged
macfarla merged 39 commits intobesu-eth:mainfrom
macfarla:gas-estimate-gas-price
May 1, 2025
Merged

adjustments to gas estimation #8478
macfarla merged 39 commits intobesu-eth:mainfrom
macfarla:gas-estimate-gas-price

Conversation

@macfarla
Copy link
Copy Markdown
Contributor

@macfarla macfarla commented Mar 27, 2025

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

PR description

this builds on top of the simple change in #8472

adjustments to gas estimate algorithm to match hive test and geth gas estimates

  • if tx is simple transfer, try 21,000
  • adjustment for call stipend
  • introduce tolerance ratio to know when to stop the binary search

Fixed Issue(s)

fixes #8527, #8459 and #8464
fixes 3 hive tests:

eth_createAccessList/create-al-contract-eip1559 (besu_besu-local) | ✓ | 8ms |  
eth_createAccessList/create-al-contract (besu_besu-local) | ✓ | 5ms |  
eth_estimateGas/estimate-successful-call (besu_besu-local) | ✓ | 6ms |  

rpc-compat hive test

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

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla
Copy link
Copy Markdown
Contributor Author

this is the test that's failing currently on main, and passes with this PR


eth_estimateGas/estimate-successful-call (besu_besu-local) | ✓ | 6ms |  
-- | -- | -- | --
Description:estimates a successful contract callspeconly: client response is only checked for schema validity.Details:>>  {"jsonrpc":"2.0","id":1,"method":"eth_estimateGas","params":[
{"from":"0x0102030000000000000000000000000000000000","input":"0xff01", "to":"0x17e7eedce4ac02ef114a7ed9fe6e2f33feba1667"}]}

<<  {"jsonrpc":"2.0","id":1,"result":"0x5316"}

@macfarla macfarla added the hive relating to hive tests label Mar 28, 2025
@macfarla
Copy link
Copy Markdown
Contributor Author

macfarla commented Apr 2, 2025

still WIP. accessList hive tests passing but a regression in estimateGas:

eth_estimateGas/estimate-successful-call (besu_besu-local) ✕ Fail 8ms  
Description:estimates a successful contract callspeconly: client response is only checked for schema validity.Details:>> {"jsonrpc":"2.0","id":1,"method":"eth_estimateGas","params":[{"from":"0x0102030000000000000000000000000000000000","input":"0xff01","to":"0x17e7eedce4ac02ef114a7ed9fe6e2f33feba1667"}]}<< {"jsonrpc":"2.0","id":1,"result":"0x5328"}
response differs from expected (-- client, ++ test):
{ "id": 1, "jsonrpc": "2.0", "result": -- "0x5328" ++ "0x5316"}

macfarla added 5 commits April 3, 2025 12:27
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla
Copy link
Copy Markdown
Contributor Author

macfarla commented Apr 3, 2025

with this PR, down to 23 rpc_compat failures. With this PR, 3 hive tests are fixed:

eth_createAccessList/create-al-contract-eip1559 (besu_besu-local) | ✓ | 8ms |  
eth_createAccessList/create-al-contract (besu_besu-local) | ✓ | 5ms |  
eth_estimateGas/estimate-failed-call (besu) | ✕ Fail | 10ms |  
eth_estimateGas/estimate-successful-call (besu_besu-local) | ✓ | 6ms |  

without this PR


eth_createAccessList/create-al-contract-eip1559 (besu) | ✕ Fail | 20ms |  
eth_createAccessList/create-al-contract (besu) | ✕ Fail | 11ms |  
eth_estimateGas/estimate-failed-call (besu) | ✕ Fail | 10ms |  
eth_estimateGas/estimate-successful-call (besu) | ✕ Fail | 7ms |  

macfarla added 6 commits April 8, 2025 10:42
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla macfarla requested a review from fab-10 April 15, 2025 09:45
@macfarla macfarla marked this pull request as ready for review April 15, 2025 09:46
@fab-10
Copy link
Copy Markdown
Contributor

fab-10 commented Apr 15, 2025

Great, this also fixes #8527

====================================================
Testing case 1 with input length: 32 bytes
====================================================
Estimating gas on each client...
Gas estimates:
Geth: 21696
Besu: 21696
====================================================
Testing case 2 with input length: 96 bytes
====================================================
Estimating gas on each client...
Gas estimates:
Geth: 22895
Besu: 22895
====================================================
Testing case 3 with input length: 128 bytes
====================================================
Estimating gas on each client...
Gas estimates:
Geth: 23399
Besu: 23399
====================================================
Testing case 4 with input length: 160 bytes
====================================================
Estimating gas on each client...
Gas estimates:
Geth: 23915
Besu: 23915

Copy link
Copy Markdown
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

LGTM, I just have some questions about a possible cleanup.
Then I prefer that the tolerance ratio is configurable already in this PR, so just in case someone want to keep the previous behavior.

macfarla added 11 commits April 16, 2025 14:16
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla
Copy link
Copy Markdown
Contributor Author

Apr 17 17:26:31.185 INF simulation ethereum/rpc-compat finished suites=1 tests=99 failed=22
^ confirmed hive tests still passing

I added a store to the TestDepth.sol that is used in EthEstimateGasAcceptanceTest - I think this exercises the estimate and tx execution with multiple frame depth with SSTORE, which I think shows that the new values for maxDepth and callStipend are valid.

Is there extra testing you want done eg for linea related to this change @fab-10 ?

Copy link
Copy Markdown
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

LGTM: just an optimization

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla macfarla added the doc-change-required Indicates an issue or PR that requires doc to be updated label Apr 28, 2025
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Copy link
Copy Markdown
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

LGTM, if you want code could be simplified further removing the Math::pow call since the exponent is always 1

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla macfarla merged commit 9da0289 into besu-eth:main May 1, 2025
48 checks passed
@macfarla macfarla deleted the gas-estimate-gas-price branch May 2, 2025 03:48
@alexandratran alexandratran removed the doc-change-required Indicates an issue or PR that requires doc to be updated label May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hive relating to hive tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Besu mis-estimates identity precompile

5 participants