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

Fixes #251 better way for dynamic gas estimation #252

Merged
merged 0 commits into from
Feb 24, 2021

Conversation

werlandy
Copy link
Contributor

@werlandy werlandy commented Dec 22, 2020

Fixes #251

Currently Frontier's gas estimation rpc call returns the actual gas used,
however gas estimation on Ethereum is a complex problem,
sometime we need to give a higher gas limit than the actual used,
otherwise the transaction may fail.

We provide a test case. There are two contracts,
and contract 2 will invoke method of contract 1 to update its state.
Using frontier, the estimated gas is 43603, however it needs a minimum gas limit of 43879.

We use a binary search based dynamic gas estimation strategy to solve the above issue,
and the strategy is also used by go-etherem
(ethereum/go-ethereum@682875a)

@werlandy werlandy requested a review from sorpaas as a code owner December 22, 2020 04:49
@tgmichel
Copy link
Contributor

#229 covers this same functionality. The idea there was calling the EVM directly at rpc-level, without spawning a runtime instance per binary search iteration.

As per @sorpaas suggestion, we closed it (temporarily) and go back to it once we add schema versioning for the pallet-evm. Similar to what @JoshOrndorff is in #199 for pallet-ethereum.

I leave up to him to decide what to do with this one. (Also sorry for the inconvenience).

@pangwa
Copy link

pangwa commented Dec 23, 2020

#229 covers this same functionality. The idea there was calling the EVM directly at rpc-level, without spawning a runtime instance per binary search iteration.

As per @sorpaas suggestion, we closed it (temporarily) and go back to it once we add schema versioning for the pallet-evm. Similar to what @JoshOrndorff is in #199 for pallet-ethereum.

I leave up to him to decide what to do with this one. (Also sorry for the inconvenience).

It's fine. #229 looks pretty complex comparing to the goeth implementation. It's less risky using the simple one ;-)

@tgmichel
Copy link
Contributor

It's fine. #229 looks pretty complex comparing to the goeth implementation. It's less risky using the simple one ;-)

Starting https://github.com/paritytech/frontier/pull/229/files#diff-f5b58703c6605ce51568c53912a42386c530956f2f22e15c0a4bcaa4bdfbdf65R110 , uses the same approach as in the goeth implementation. The rest is a draft for interacting with the EVM directly at rpc-level.

@sorpaas
Copy link
Member

sorpaas commented Jan 5, 2021

@werlandy Just wonder if you can try rust-ethereum/evm@5902295 and see whether it fixes the problem?

I think we'd want to include binary search as an option for the RPC, but in the mean time I want to treat all estimation failures as bugs.

@werlandy
Copy link
Contributor Author

werlandy commented Jan 5, 2021 via email

@sorpaas
Copy link
Member

sorpaas commented Jan 7, 2021

@werlandy The code is okay, but the naming right now is not.

The main issue is that we can't randomly introduce a new RPC method in the eth_ namespace. They're designed not with client extensions but go through the EIP process.

Would you mind do the following:

  • Create an rpc_binary_search_estimate rust feature (you can decide the name).
  • Make the code so that when the feature is enabled, use your new code. Otherwise, use the old code.

@werlandy
Copy link
Contributor Author

werlandy commented Jan 7, 2021 via email

@werlandy
Copy link
Contributor Author

werlandy commented Jan 8, 2021

@sorpaas As suggested, I've added the "rpc_binary_search_estimate" feature, and code was well tested by different cases. You can decide whether to enable the feature in node template later.

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

Successfully merging this pull request may close these issues.

Enhance gas estimation for inner contract method call.
4 participants