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

More robust gas pricing #6555

Closed
4 tasks
ethanfrey opened this issue Jun 30, 2020 · 2 comments
Closed
4 tasks

More robust gas pricing #6555

ethanfrey opened this issue Jun 30, 2020 · 2 comments

Comments

@ethanfrey
Copy link
Contributor

ethanfrey commented Jun 30, 2020

Summary

Transaction fees are currently defined as Coins to pay and a gas limit. The ratio is used to provide a minimum price filter (eg. uatom/gas). However, the entire fee is deducted at the start of the transaction regardless of how much gas is used. Since there is no good way to estimate the actual cost for many transactions, we often have to multiply estimates by 2-4x to ensure it doesn't error on out of gas.

A cleaner way would be to only deduct a portion of the fee based on gas used (or all if it errors). Thus, I can easily offer to pay 0.25 uatom/gas for up to 200_000 gas by placing 50000uatom in the fee, but knowing maybe only 5000-10000uatom will be charged if this tx is simple.

Problem Definition

Many clients have this issue with gas estimation. A few days ago this came up again in discord: https://discordapp.com/channels/669268347736686612/669274997264613376/727184290818949190 explaining how Ledger Live provides the gas estimate:

@okwme

Is this how all the wallets are dong this? Are the updates in 0.38 and 0.39 going to improve this process at all?

Ledger Live:

For the network fees before any transaction is sent :

  • The gas estimation for a transaction provided by gaia node does not simulate the gas cost of reading and writing to the tendermint database.
  • Therefore we have to apply a gas_amplifier  factor to the estimation gaia gives us back, hoping that it will cover the storage costs.
  • We tuned this amplifier to have all transactions passing (we had lower values at the beginning, but internal testing triggered a lot of lost funds because of OutOfGas  errors)
  • After this (safely high) GasWanted  value is computed, we apply the anti-spam multiplier 0.25 uatom/gas  and that's the "Network fees" value that you see

It seems an alternate solution - having very precise gas estimates - was discussed some time ago for the 0.38 release, but never made it in: #4938 but comments show this was a pain point.

Proposal

Well, if only these ante decorators were actually decorators (and ran both before and after the tx), this would be easy to cleanly add.

However, baring major architecture changes, I would propose a 3 step process (2 of which exist now):

  1. MempoolFeeDecorator enforces local min-fee as it currently does
  2. DeductFeeDecorator will deduct the maximum possible fees as it does now.
  3. Add a new check at the end of runTx that on tx success if there is eg. > 10k gas left on the gas meter (enough to pay for the refund), then calculate the fee to be refunded and transfer it back to the signer:
gasWanted := ctx.GasMeter().Limit()
gasUsed := ctx.GasMeter().GasConsumed()
// TODO: use BigDec here and properly multiply the coins with it
ratio := (gasWanted - gasUsed / gasWanted) 
refund = tx.GetFee() * ratio

Ideally all this would be not in runTx, but in some "ante cleanup handler".

I once tried to do something similar, charging a minFee (anti-spam) on error and the full fee ("product fee" on success). This code just gives an idea of how to ensure fee on entry and possibly charge a different one on exit.


For Admin Use

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

ethanfrey commented Jun 30, 2020

After reading more issues, I think this is better discussed in #2150

Closing this issue, but leaving it as a +1

@hxrts
Copy link
Contributor

hxrts commented Jan 6, 2021

I think Ethan forgot to close. Doing so now as there are 2 open issues that cover on this topic.

#4938
#2150

@hxrts hxrts closed this as completed Jan 6, 2021
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

No branches or pull requests

2 participants