Fix estimate_gas execution doesn't match the actual execution
#1257
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@sorpaas Please take a review. |
sorpaas
approved these changes
Dec 13, 2023
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
This was referenced Apr 24, 2024
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
dnjscksdn98
added a commit
to bifrost-platform/bifrost-frontier
that referenced
this pull request
Nov 29, 2024
dnjscksdn98
added a commit
to bifrost-platform/bifrost-frontier
that referenced
this pull request
Nov 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
proof_size_base_costcalculation of the TransactionData in the runtime call or create is different from the one in thepallet-ethereum. In the runtime call, we create an instance by callingTransactionData::newwhile in the ethereum pallet, wetransaction.into(). There is a subtle difference when calculating theproof_size_base_cost. This difference can lead to the wrong estimate_gas result and cause an OutOfGas issue. I fixed this by taking theproof_size_base_costfrom the TransactionData and unifying the proof base size calculation.fee_detailsin theestimate_gasfunction is problematic in the case of absence ofgas_priceorrequest_max_fee,request_priorityin 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 thefee_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