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

EthEstimateGas: Estimation fails on recursive calls #10041

Closed
8 of 18 tasks
Stebalien opened this issue Jan 17, 2023 · 1 comment
Closed
8 of 18 tasks

EthEstimateGas: Estimation fails on recursive calls #10041

Stebalien opened this issue Jan 17, 2023 · 1 comment
Assignees
Labels
kind/bug Kind: Bug

Comments

@Stebalien
Copy link
Member

Checklist

  • This is not a security-related bug/issue. If it is, please follow please follow the security policy.
  • This is not a question or a support request. If you have any lotus related questions, please ask in the lotus forum.
  • This is not a new feature request. If it is, please file a feature request instead.
  • This is not an enhancement request. If it is, please file a improvement suggestion instead.
  • I have searched on the issue tracker and the lotus forum, and there is no existing related issue or discussion.
  • I am running the Latest release, or the most recent RC(release canadiate) for the upcoming release or the dev branch(master), or have an issue updating to any of these.
  • I did not make any code changes to lotus.

Lotus component

  • lotus daemon - chain sync
  • lotus miner - mining and block production
  • lotus miner/worker - sealing
  • lotus miner - proving(WindowPoSt)
  • lotus miner/market - storage deal
  • lotus miner/market - retrieval deal
  • lotus miner/market - data transfer
  • lotus client
  • lotus JSON-RPC API
  • lotus message management (mpool)
  • Other

Lotus Version

release/v1.20.0

Describe the Bug

EthEstimateGas uses Filecoin's gas estimation logic, which doesn't take into account the fact that the EVM reserves 1/64 of the remaining gas per sub-call. Ethereum clients usually estimate gas by performing a binary search.

We should change EthEstimateGas to:

  1. Estimate the lower-bound on gas usage the normal way.
  2. Perform an exponential search for the actual gas limit. I.e.:
    1. Run the message with the estimated gas.
    2. If it fails, increase by 100k.
      1. If that succeeds, decrease by 50k.
      2. ...
    3. If that fails, increase by 200k.

We could consider running this same algorithm inside GasEstimateMessageGas instead as, in the expected case, it should only cause us to execute the message one additional time to confirm the gas estimation.

Logging Information

-

Repo Steps

No response

@arajasek
Copy link
Contributor

I think Steb's proposal is reasonable here. I'd definitely want to only do this in the Ethereum API, because it's very specific to that.

As discussed in sync, the correctest way to do this would be to expand gas tracing to track this info (track the minimum needed gas at each subcall, and bubble that information up). This is very much nice-to-have, though.

@jennijuju jennijuju moved this to 🌟 In Scope in Network v18 Jan 19, 2023
@jennijuju jennijuju moved this from 🌟 In Scope to 🏗 In progress in Network v18 Jan 19, 2023
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Network v18 Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Kind: Bug
Projects
No open projects
Status: Done
Development

No branches or pull requests

4 participants