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

execute fee check logic in deliverTX #2383

Closed
5 tasks
yaruwangway opened this issue Apr 6, 2023 · 0 comments · Fixed by #2447
Closed
5 tasks

execute fee check logic in deliverTX #2383

yaruwangway opened this issue Apr 6, 2023 · 0 comments · Fixed by #2447
Assignees
Labels
state-machine-breaking State machine breaking changes (impacts consensus). status: waiting-triage This issue/PR has not yet been triaged by the team. type: feature-request New feature or request improvement

Comments

@yaruwangway
Copy link
Contributor

yaruwangway commented Apr 6, 2023

Summary

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

if msgs are bypass msgs and the bypass msgs gas usage is within bypass gas limit.
if the paid fee is in the denom required.
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.

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

In order to make deliverTx deterministic, the deliverTX checks the fee against global fee, while checkTX is checking fees against combinedFees (global fee and local min-gas-price in app.toml).

This means:

  • local min-gas-price higher than global fee, if paidfee can pass CheckTx, it can also pass DeliverTX.
  • local min-gas-price higher than global fee, if paidfee cannot pass CheckTx, it get rejected.
  • local min-gas-price lower than global fee, if paidfee can pass CheckTx, it can also pass DeliverTX.
  • local min-gas-price lower than global fee, if paidfee cannot pass CheckTx, it get rejected.

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

Problem Definition

Proposal

  • remove the checkTX condition here so that the fee check will run both in CheckTX and DeliverTX.
  • if CheckTX, check paid fees against combinedFees, if DeliverTx, check paid fees against globalfees, eeCoinsNonZeroDenom.IsAnyGTE(nonZeroCoinGlobalFeeReq) .
	nonZeroCoinGlobalFeeReq, _ := splitFees(requiredGlobalFees)
	if ctx.IsCheckTx() {
		if !feeCoinsNoZeroDenom.DenomsSubsetOf(nonZeroCoinFeesReq) {
			return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "fee is not a subset of required fees; got %s, required: %s", feeCoins, combinedFeeRequirement)
		}
	} else {
		// deliverTx
		if !feeCoinsNoZeroDenom.DenomsSubsetOf(nonZeroCoinGlobalFeeReq) {
			return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "fee is not a subset of required fees; got %s, required: %s", feeCoins, combinedFeeRequirement)
		}
	}
	

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 type: feature-request New feature or request improvement status: waiting-triage This issue/PR has not yet been triaged by the team. labels Apr 6, 2023
@github-project-automation github-project-automation bot moved this to 🩹 Triage in Cosmos Hub Apr 6, 2023
@yaruwangway yaruwangway changed the title Feat: !(state-breaking) execute fee check logic in deliverTX !Feat: execute fee check logic in deliverTX Apr 6, 2023
@yaruwangway yaruwangway changed the title !Feat: execute fee check logic in deliverTX Feat: execute fee check logic in deliverTX Apr 6, 2023
@sainoe sainoe self-assigned this Apr 20, 2023
@sainoe sainoe changed the title Feat: execute fee check logic in deliverTX execute fee check logic in deliverTX Apr 20, 2023
@sainoe sainoe moved this from 🩹 Triage to 📥 Todo in Cosmos Hub Apr 20, 2023
@mpoke mpoke moved this from 📥 Todo to 🏗 In progress in Cosmos Hub Apr 25, 2023
@sainoe sainoe moved this from 🏗 In progress to 👀 In review in Cosmos Hub May 1, 2023
@mmulji-ic mmulji-ic added the state-machine-breaking State machine breaking changes (impacts consensus). label May 2, 2023
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Cosmos Hub May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state-machine-breaking State machine breaking changes (impacts consensus). status: waiting-triage This issue/PR has not yet been triaged by the team. type: feature-request New feature or request improvement
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

3 participants