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

add special state transition to allow for gas estimation #825

Merged
merged 2 commits into from
Jan 24, 2020

Conversation

nategraf
Copy link
Contributor

Description

Currently gas estimation does not work for accounts with low balance, as is the case for new accounts invited to Celo with an invite code. The issue is that the binary search used for gas estimation makes an assumption that if there is an error, raising the gas limit on the transaction will fix it, or that the transaction will always fails. This assumption is broken when the gas price is non-zero and the account does not have a high enough balance to pay fees for a transaction with maximum gas. In that case there is only a window of gas values that will work, which cannot be found via binary search.

It would be easy to set the gas price to zero and run the transaction, which solves the issue mentioned above, but the gas price minimum functionality prevents the transaction from running.

Alternative solutions

  1. Disable the GasPriceMinimum before calling the state transition with a selective StateDb write (Geth 1.9 provides some support for this in its API implementation)
  2. Amend vm.Contract to keep track of the high water line of gas used during program execution and replace the binary search mechanism with a single run at max tx gas.

Tested

Synced a node against integration and observed that gas estimation for a sample transaction failed before the changes and succeeds after. Also ran all e2e tests, which implicitly use gas estimation.

Related issues

Backwards compatibility

Changes are backwards compatible.

@nategraf nategraf requested a review from asaj as a code owner January 23, 2020 20:21
Copy link
Contributor

@timmoreton timmoreton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

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