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 execution doesn't match the actual execution #1257

Merged
merged 6 commits into from
Dec 13, 2023

Conversation

boundless-forest
Copy link
Collaborator

We have found that there are some tricky OutOfGas issues in the Darwinia networks. After some digging out, I found two problems that exist in the current implementation.

  1. The proof_size_base_cost calculation of the TransactionData in the runtime call or create is different from the one in the pallet-ethereum. In the runtime call, we create an instance by calling TransactionData::new while in the ethereum pallet, we transaction.into(). There is a subtle difference when calculating the proof_size_base_cost. This difference can lead to the wrong estimate_gas result and cause an OutOfGas issue. I fixed this by taking the proof_size_base_cost from the TransactionData and unifying the proof base size calculation.
  2. The current fee_details in the estimate_gas function is problematic in the case of absence of gas_price or request_max_fee, request_priority in the estimate gas call request. If the estimate call request doesn't include the price fields, it will pass None to the runtime call, leading to a mismatch TransactionData proof_size_base(less), and going to the problem 1 above. If the proof size is plays a key role in the transaction gas estimate, it's problematic. The estimated gas is less than the actual execution gas. I fixed this by give U256::zero instead of None in the fee_details. Some of the code is similar to the Geth. https://github.com/scroll-tech/go-ethereum/blob/38a3a9c9198ce54513506ddd93d03e6d99618bad/accounts/abi/bind/backends/simulated.go#L499-L508

@boundless-forest
Copy link
Collaborator Author

@sorpaas Please take a review.

@sorpaas sorpaas merged commit ded76d0 into polkadot-evm:master Dec 13, 2023
5 checks passed
@boundless-forest boundless-forest deleted the bear-fix-estimate-gas branch December 13, 2023 07:28
boundless-forest added a commit to darwinia-network/frontier that referenced this pull request Dec 14, 2023
…adot-evm#1257)

* Ensure the actual executed proof base size is the same as the estimate approach

* Add tests

* Fix estimate-gas bug

* Fix the broken ts test, very tricky

* Rewrite the total fee per gas part and code clean

* Fix clippy
AurevoirXavier pushed a commit to darwinia-network/frontier that referenced this pull request Dec 14, 2023
…adot-evm#1257) (#18)

* Ensure the actual executed proof base size is the same as the estimate approach

* Add tests

* Fix estimate-gas bug

* Fix the broken ts test, very tricky

* Rewrite the total fee per gas part and code clean

* Fix clippy
frank0528 pushed a commit to DeepBrainChain/DBC-EVM that referenced this pull request Sep 10, 2024
…adot-evm#1257)

* Ensure the actual executed proof base size is the same as the estimate approach

* Add tests

* Fix estimate-gas bug

* Fix the broken ts test, very tricky

* Rewrite the total fee per gas part and code clean

* Fix clippy
frank0528 pushed a commit to DeepBrainChain/DBC-EVM that referenced this pull request Sep 24, 2024
…adot-evm#1257)

* Ensure the actual executed proof base size is the same as the estimate approach

* Add tests

* Fix estimate-gas bug

* Fix the broken ts test, very tricky

* Rewrite the total fee per gas part and code clean

* Fix clippy
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