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

Refactor: fee antehandler #2228

Closed
5 tasks
yaruwangway opened this issue Feb 20, 2023 · 3 comments
Closed
5 tasks

Refactor: fee antehandler #2228

yaruwangway opened this issue Feb 20, 2023 · 3 comments
Assignees
Labels
type: tech-debt Slows down development in the long run

Comments

@yaruwangway
Copy link
Contributor

yaruwangway commented Feb 20, 2023

Summary

Gaia fee antehandler introduction

Gaia fee antehandler takes min_gas_price, Global fee into a combined fee requirement. The combined fee:

  1. takes the higher fees between min_gas_price and global fee if min_gas_price and global_fee have the same denom. Otherwise, take the global fee.
  2. consider the global fee denom as required denom even if global fee is zero coin.

For point 2, present gaia fee antehandler allows zero coins globalfee, for example, globalfee=[0stake] means the chain does not want to charge transaction fees, but if there is volunteer paid fee, it should be in the denom of stake.

Concerns

Globalfee is sdk.DecCoins type, but cosmos-sdk sdk.DecCoins are sanitized to remove zero coins in it, so several methods from sdk.DecCoins are over-written in gaia fee antehandler.
Even though present fee antehandler logic works, allowing zero coins in global fee makes the fee check logic in antehandler complicated and difficult to maintain.

Type

Code Debt
Design Debt

Impact

Simplify gaia fee antehandler, the present logic is complex and hard to maintain, need to maintain extra methods that overwritten sdk's methods.

### Potential state-breaking
For the hub, this refactor means users cannot propose zero coins in globalfee through gov proposal. If global fee is empty, the bond_denom will be used to check the paid fee denoms. This is potential state-breaking if in param store, globalfee params already contain zero coins (assume someone proposed zero coins as globalfee before this refactor). Please see below solutions, step4: upgrade handler, for how to deal with this scenarios.

Proposed Solution

Proposal:

## proposal 1:
Globalfee does not allow zero coins, this means globalfee=[0stake] is invalid setup.
- If the chain does not want to charge fees (above mentioned point 2), it can set globalfee=[], if globalfee empty, we can use bonding denom to check the denoms of the global fee.
- If the chain allows any other denoms as paid fee denom, it has to set a value in global fee, for example, globalfee=[0.0001stake]

This can simplify the logic, but disables one possibility:
bonding denom = uatom, but the chain want to set 0stake as globalfee (the chain does not charge tx fee, but tx still pays, it has to be stake (not a bonding denom of the chain.) )

So in summary:
after refactor, globalfee does not allow zero coins.
- if chain want to set globalfee=[0uatom] (uatom is bond-denom), it can set globalfee=[]
- if chain want to set globalfee=[0stake] (uatom is bond-denom), it has to setup as globalfee=[x stake], (x > 0)

Proposal 2:

globalfee can contain zero coins, in the fee check antehandler: split the combinedFeeRequirement (get a combined fee requirement from local fee(min-gas-prices in app.toml) and global fee) into zero and non-zero coins:
combinedFeeRequirement = nonZeroCoinFeesReq + zeroCoinFeesDenomReq

split the paidfee according to the nonZeroCoinFeesReq and zeroCoinFeesDenomReq denoms:
paidfee= feeCoinsNoZeroDenom + feeCoinsZeroDenom

steps:

  1. check paidfee denoms
  2. if bypass-msg, tx pass
  3. if not bypass-msg, if paidfee contains feeCoinsZeroDenom, tx pass.
  4. if not bypass-msg, if paidfee does not contain feeCoinsZeroDenom, check feeCoinsNoZeroDenom 's amount IsAnyGTE than nonZeroCoinFeesReq
  5. specical case: when paidfee=[], and there is zero coins in combinedFeeRequirement, tx pass.

example:

globalfee=[0uatom,1stake,1photon], min-gas-prices=[] in app.toml.
combinedFeeRequirement=[0uatom,1stake,1photon].
nonZeroCoinFeesReq=[1stake,1photon], zeroCoinFeesReq=[0uatom].

  1. paidfee=[1uatom,3quark],
    fail in step 1.
  2. paidfee=[1uatom,2photon],
    pass denom checks, then split paid fee,feeCoinsZeroDenom=[1uatom], feeCoinsNoZeroDenom=[2photon],
    feeCoinsZeroDenom is not empty, direct pass tx.
  3. paidfee=[2photon]
    pass denom checks, then split paid fee,feeCoinsZeroDenom=[], feeCoinsNoZeroDenom=[2photon],
    feeCoinsZeroDenom empty, check feeCoinsNoZeroDenom IsAnyGTE than nonZeroCoinFeesReq, pass
  4. paidfee=[]
    pass denoms checks, then split paid fee,feeCoinsZeroDenom=[], feeCoinsNoZeroDenom=[], feeCoinsZeroDenom empty, if directly check feeCoinsNoZeroDenom IsAnyGTE than nonZeroCoinFeesReq, this tx will fail, so check if combinedFeeRequirement is also empty first before check IsAnyGTE
  5. paidfee=[0.5photon],
    pass denom checks, then split paid fee,feeCoinsZeroDenom=[], feeCoinsNoZeroDenom=[0.5photon],
    feeCoinsZeroDenom empty, check feeCoinsNoZeroDenom IsAnyGTE than nonZeroCoinFeesReq fail.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
  • Is a spike necessary to map out how the issue should be approached?
@yaruwangway yaruwangway added this to the Gaia v10.0.0 milestone Feb 20, 2023
@yaruwangway yaruwangway added the type: tech-debt Slows down development in the long run label Feb 21, 2023
@yaruwangway yaruwangway self-assigned this Feb 22, 2023
@yaruwangway yaruwangway modified the milestones: Gaia v10.0.0, Bump SDK to 0.47 Feb 23, 2023
@github-project-automation github-project-automation bot moved this to 🩹 Triage in Cosmos Hub Feb 23, 2023
@yaruwangway
Copy link
Contributor Author

yaruwangway commented Mar 7, 2023

updates:
present gaia fee antehandler checks the fees only in checkTx. The fee antehandler checks:

  1. if bypass the msg and the bypass msgs gas usage is within bypass gas limit.
  2. if the paid fee is in the denom required.
  3. if paidFee amount >= required Fee amount (required fee takes the higher fee between mininum-gas-price in app.toml and global fee).

The above fee checks can prevent malicious tx submitters, but it cannot prevent malicious validator. Block proposer can change the fee antehandler and propose the tx that does not pass fee checks in fee antehandler, since present fee antehandler only checks in checkTX, other validators will not re-check the txs in the proposed block again.

The solution is to do fee check logic in deliverTx as well, this requires determinism of the fee check result, therefore, the fee check in deliverTx will:

  1. the paid fee has to be higher than the global fee (not the combined fee from min-gas-price and globalfee! min-gas-price is non-deterministic, each node can setup its own min-gas-price in app.toml)
  2. denom check also follows global fee denoms
  3. the bypass msg and the MaxTotalBypassMinFeeMsgGasUsage has to be moved from app.toml to params store so that the bypass-msg and MaxTotalBypassMinFeeMsgGasUsage checks are deterministic.

Please note, the above mentioned refactor is Consensus breaking !!!

@mpoke mpoke removed this from the Gaia v10.0.0 milestone Mar 8, 2023
@mpoke mpoke moved this from 🩹 Triage to 📥 Todo in Cosmos Hub Mar 9, 2023
@yaruwangway
Copy link
Contributor Author

yaruwangway commented Mar 15, 2023

updates:

For global fees, it still can contain zero and non-zero coins, when use global fees in fee antehandler, split it into two groups: zero and non-zero coins.

  • For non-zero coins, use sdk.Coins methods DenomsSubsetOf, and IsAnyGTE rather than overwrite them.
  • For the globalfee zero coins, write logics to check the denoms.

cc @sainoe @mpoke

@yaruwangway yaruwangway moved this from 📥 Todo to 🏗 In progress in Cosmos Hub Mar 30, 2023
@sainoe sainoe moved this from 🏗 In progress to 👀 In review in Cosmos Hub Apr 3, 2023
@sainoe sainoe moved this from 👀 In review to 🏗 In progress in Cosmos Hub Apr 3, 2023
@mpoke mpoke moved this from 🏗 In progress to 👀 In review in Cosmos Hub Apr 4, 2023
@mpoke mpoke unassigned sainoe Apr 4, 2023
@mpoke mpoke assigned sainoe and unassigned yaruwangway Apr 12, 2023
@sainoe
Copy link
Contributor

sainoe commented Apr 20, 2023

closed by #2327

@sainoe sainoe closed this as completed Apr 20, 2023
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Cosmos Hub Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: tech-debt Slows down development in the long run
Projects
Status: ✅ Done
Development

No branches or pull requests

3 participants