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

Tx Gas Estimation Improvement #4938

Open
2 of 4 tasks
alexanderbez opened this issue Aug 21, 2019 · 10 comments
Open
2 of 4 tasks

Tx Gas Estimation Improvement #4938

alexanderbez opened this issue Aug 21, 2019 · 10 comments

Comments

@alexanderbez
Copy link
Contributor

alexanderbez commented Aug 21, 2019

Summary

If you provide "gas": "auto" when submitting a tx, the application will automatically estimate gas for you via runTxModeSimulate before submitting it with that gas estimate.

However, this estimate is not always entirely accurate for the following reasons:

  1. Estimation does not take into consideration state writes
// Safety check: don't write the cache state unless we're in DeliverTx.
if mode != runTxModeDeliver {
	return result
}

// only update state if all messages pass
if result.IsOK() {
	msCache.Write() // <= this is where I believe most of the difference comes from
}
  1. State mutations may have occurred between gas estimation and tx execution which could cause a slight deviation from the estimate. This is exacerbated if the time is increased between estimation and broadcasting/execution.

As a result, the gas-adjustment option exists to multiply this estimate by some factor to account for the delta.

However, the gas-adjustment can become cumbersome for clients and may have to constantly change because of (1) requiring them to constantly adjust with new values (or use very large gas and pay the fees to avoid headache).

Proposal

There is not much we can do about (2) hence I propose we keep the gas-adjustment option. However, I propose we can remove (1) and make estimation nearly exact. I believe this can easily be done by introducing a third volatile yet ephemeral state in BaseApp -- simulateState.

During runTxModeSimulate, we create this simulateState based off of the checkState and cache-wrap it. This will allow us to call Write() and get a nearly true estimation. In addition, this is pretty trivial to implement.

This is mainly a brain dump and I'm not 100% sure this works but I don't immediately see a reason why it should not work.

/cc @faboweb


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@tnachen
Copy link
Contributor

tnachen commented Aug 23, 2019

If we're not sure if this is what we want to do (braindump as you mentioned), is this still set to be in 0.38?

@alexanderbez
Copy link
Contributor Author

@tnachen yes, at the very least we should explore if this is actually a feasible approach for 0.38. Being that this is pretty trivial to implement and becoming more of a huge burden for clients, I think we should get this in for 0.38.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@ethanfrey
Copy link
Contributor

I tried, but this approach doesn't seem to fix the issue.

There also seems to be quite some caching in the sdk, and a different order of operations can yield different gas costs (both for simulate as well as deliver)

@alessio alessio removed this from the 0.39.0 [launchpad] milestone Jul 10, 2020
@clevinson clevinson added this to the v0.42 milestone Jan 7, 2021
@xwartz
Copy link
Contributor

xwartz commented Mar 1, 2021

Any updates?

@yihuang
Copy link
Collaborator

yihuang commented Jun 11, 2021

// only update state if all messages pass
if result.IsOK() {
	msCache.Write() // <= this is where I believe most of the difference comes from
}

Isn't the Write don't have access to gas meter, hence don't change gas?

@alexanderbez
Copy link
Contributor Author

No it does, but we don't write on simulation. Also, @ethanfrey claimed this provided no benefit, so perhaps my intuition was wrong all along.

@jkilpatr
Copy link

Bumping this issue. I'm seeing gas estimates that are ~17% off trying to estimate gas for Gravity bridge.

@ethanfrey
Copy link
Contributor

17% is good. Most people use 1.2 gas multiplier and raise it if the estimates are off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 📋 Backlog
Development

No branches or pull requests

11 participants