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

Fix estimate_gas result differs widely from the used_gas #1239

Merged
merged 7 commits into from
Nov 10, 2023

Conversation

boundless-forest
Copy link
Collaborator

We found an issue last week where the estimated gas result for some transactions was significantly higher than the actual used_gas in the receipt. See this darwinia-network/darwinia#1301 for details. After some investment, the root cause was in the proof size calculation in the record_external_operation and record_external_dynamic_opcode_cost:

  1. https://github.com/paritytech/frontier/blob/master/frame/evm/src/runner/stack.rs#L989-L994
  2. https://github.com/paritytech/frontier/blob/master/frame/evm/src/runner/stack.rs#L1083-L1090

In the case of not hitting the account metadata, the weight info recorder tried to record the create_contract_limit value, which is a very large amount 24576. This made it easy to trigger the transaction proof size limit and then out of gas. It fooled the binary research gas estimater to make the gas higher than before. Take https://crab.subscan.io/tx/0x313ba47341cdf5b6b5ce05cc2317ee6e3b285312b6a01d62fba4c39f51ab8c34 for example, the actual used_gas is 23135, but the estimated gas result over 160000~ This PR mainly fixes this issue. Beside this, the current record_external_dynamic_opcode_cost is very messy, I have rewritten it.

@boundless-forest
Copy link
Collaborator Author

I know you are doing some refactoring to the core evm and how the current frontier works. Would you mind to reviewing this when you have time. This is a bug fix and improvement. Maybe things will be different after your refactor, but before that, we need to solve this issue in the main branch. @sorpaas

@sorpaas
Copy link
Member

sorpaas commented Nov 10, 2023

Sorry. It just took me a while to review the other parts of your rewrites.

The EVM refactoring won't block anything here -- it'll still take a while (sorry I probably should allocate more time doing reviews). But indeed, at the end we'll be able to move away from the record_external_operation structure and implement a much nicer custom gasometer.

@sorpaas sorpaas merged commit 176cb34 into polkadot-evm:master Nov 10, 2023
5 checks passed
@boundless-forest boundless-forest deleted the bear-fix-out-of-gas branch November 10, 2023 05:26
icodezjb pushed a commit to btclayer2/frontier that referenced this pull request Nov 10, 2023
…t-evm#1239)

* Fix add `size_limit` to the proof_size of weight info

* Fix issues in the record dynamic opcode

* Debug test case

* Debug test case

* Fix test cases

* Fix clippy

* Sload cache item

(cherry picked from commit 176cb34)
boundless-forest added a commit to darwinia-network/frontier that referenced this pull request Nov 24, 2023
…t-evm#1239)

* Fix add `size_limit` to the proof_size of weight info

* Fix issues in the record dynamic opcode

* Debug test case

* Debug test case

* Fix test cases

* Fix clippy

* Sload cache item
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants