feat!: move Global Fee checks to DeliverTx (#2447)

* add fee check logic to DeliverTx * add uts * refactor combinedFee * update ut test * update uts * save * print global fee * lower fees in e2e tests * fix e2e test tx flags * remove debug logs * update changelog * nits * refactor GetTxFeeRequired * nits * update docs * update changelog * remove debugging setup * nits * nits * nits * nits * Update x/globalfee/ante/antetest/fee_test.go * Update x/globalfee/ante/antetest/fee_test.go * fix changelog * Update x/globalfee/ante/fee.go Co-authored-by: yaruwangway <[email protected]> * fix flag fees in e2e tests * refactor: refactor fee check for readability * chore: fix typo * chore: update comments * refactor: change map[string]bool to map[string]struct{} * fix linter * add key check * remove unnecessary condition and improve test coverage * Update x/globalfee/ante/antetest/fee_test.go --------- Co-authored-by: yaruwangway <[email protected]> Co-authored-by: Yaru Wang <[email protected]>
cosmos · May 10, 2023 · 79f5b83 · 79f5b83
1 parent 7b73e42
commit 79f5b83
Show file tree

Hide file tree

Showing 9 changed files with 195 additions and 100 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -38,10 +38,11 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Improvements
 * (test) [#2440](https://github.com/cosmos/gaia/pull/2440) Add vulncheck to nightly builds
+* (gaia) [#2442](https://github.com/cosmos/gaia/pull/2442) Bump [Interchain-Security](https://github.com/cosmos/interchain-security) to [v1.1.1](https://github.com/cosmos/interchain-security/tree/v1.1.1).
 
 ### State Machine Breaking
 * (gaia) Bump Golang prerequisite from 1.18 to 1.20 see (https://go.dev/blog/go1.20) for details.
-* (gaia) [#2442](https://github.com/cosmos/gaia/pull/2442) Bump [Interchain-Security](https://github.com/cosmos/interchain-security) to [v1.1.1](https://github.com/cosmos/interchain-security/tree/v1.1.1).
+* (feat!) [#2447] Update Global Fee's AnteHandler to check tx fees against the network min gas prices in DeliverTx mode.
 
 
 ## [v9.0.3] - 2023-04-19

diff --git a/e2e.Dockerfile b/e2e.Dockerfile
@@ -15,6 +15,6 @@ FROM cgr.dev/chainguard/static:$IMG_TAG
 ARG IMG_TAG
 COPY --from=gaiad-builder /go/bin/gaiad /usr/local/bin/
 EXPOSE 26656 26657 1317 9090
-USER "nonroot"
+USER nonroot
 
 ENTRYPOINT ["gaiad", "start"]
diff --git a/tests/e2e/e2e_exec_test.go b/tests/e2e/e2e_exec_test.go
@@ -439,8 +439,7 @@ func (s *IntegrationTestSuite) executeDelegate(c *chain, valIdx int, amount, val
 		amount,
 		fmt.Sprintf("--%s=%s", flags.FlagFrom, delegatorAddr),
 		fmt.Sprintf("--%s=%s", flags.FlagChainID, c.id),
-		fmt.Sprintf("--%s=%s", flags.FlagGas, "auto"),
-		fmt.Sprintf("--%s=%s", flags.FlagFees, delegateFees),
+		fmt.Sprintf("--%s=%s", flags.FlagGasPrices, delegateFees),
 		"--keyring-backend=test",
 		fmt.Sprintf("--%s=%s", flags.FlagHome, home),
 		"--output=json",
@@ -469,8 +468,8 @@ func (s *IntegrationTestSuite) executeRedelegate(c *chain, valIdx int, amount, o
 		amount,
 		fmt.Sprintf("--%s=%s", flags.FlagFrom, delegatorAddr),
 		fmt.Sprintf("--%s=%s", flags.FlagChainID, c.id),
-		fmt.Sprintf("--%s=%s", flags.FlagGas, "auto"),
-		fmt.Sprintf("--%s=%s", flags.FlagFees, delegateFees),
+		fmt.Sprintf("--%s=%s", flags.FlagGas, "300000"), // default 200000 isn't enough
+		fmt.Sprintf("--%s=%s", flags.FlagGasPrices, delegateFees),
 		"--keyring-backend=test",
 		fmt.Sprintf("--%s=%s", flags.FlagHome, home),
 		"--output=json",
@@ -538,7 +537,7 @@ func (s *IntegrationTestSuite) execSetWithdrawAddress(
 		"set-withdraw-addr",
 		newWithdrawalAddress,
 		fmt.Sprintf("--%s=%s", flags.FlagFrom, delegatorAddress),
-		fmt.Sprintf("--%s=%s", flags.FlagGasPrices, fees),
+		fmt.Sprintf("--%s=%s", flags.FlagFees, fees),
 		fmt.Sprintf("--%s=%s", flags.FlagChainID, c.id),
 		fmt.Sprintf("--%s=%s", flags.FlagHome, homePath),
 		"--keyring-backend=test",
@@ -610,6 +609,7 @@ func (s *IntegrationTestSuite) executeGaiaTxCommand(ctx context.Context, c *chai
 
 	stdOut := outBuf.Bytes()
 	stdErr := errBuf.Bytes()
+
 	if !validation(stdOut, stdErr) {
 		s.Require().FailNowf("Exec validation failed", "stdout: %s, stderr: %s",
 			string(stdOut), string(stdErr))

diff --git a/tests/e2e/e2e_staking_test.go b/tests/e2e/e2e_staking_test.go
@@ -20,7 +20,7 @@ func (s *IntegrationTestSuite) testStaking() {
 
 	delegatorAddress := s.chainA.genesisAccounts[2].keyInfo.GetAddress().String()
 
-	fees := sdk.NewCoin(uatomDenom, sdk.NewInt(10))
+	fees := sdk.NewCoin(uatomDenom, sdk.NewInt(1))
 
 	delegationAmount := sdk.NewInt(500000000)
 	delegation := sdk.NewCoin(uatomDenom, delegationAmount) // 500 atom

diff --git a/tests/e2e/e2e_vesting_test.go b/tests/e2e/e2e_vesting_test.go
@@ -7,6 +7,7 @@ import (
 	"time"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
+
 	"github.com/cosmos/gaia/v9/x/globalfee/ante"
 )
 
@@ -37,7 +38,7 @@ var (
 	vestingAmount           = sdk.NewCoin(uatomDenom, sdk.NewInt(350000))
 	vestingBalance          = sdk.NewCoins(vestingAmountVested).Add(vestingAmount)
 	vestingDelegationAmount = sdk.NewCoin(uatomDenom, sdk.NewInt(500000000))
-	vestingDelegationFees   = sdk.NewCoin(uatomDenom, sdk.NewInt(10))
+	vestingDelegationFees   = sdk.NewCoin(uatomDenom, sdk.NewInt(1))
 )
 
 func (s *IntegrationTestSuite) testDelayedVestingAccount(api string) {

diff --git a/x/globalfee/ante/antetest/fee_test.go b/x/globalfee/ante/antetest/fee_test.go
@@ -574,23 +574,32 @@ func (s *IntegrationTestSuite) TestGlobalFeeMinimumGasFeeAnteHandler() {
 			txCheck:         true,
 			expErr:          true,
 		},
-		"disable checkTx: no fee check. min_gas_price is low, global fee is low, tx fee is zero": {
+		"disable checkTx: min_gas_price is medium, global fee is low, tx fee is low": {
 			minGasPrice:     minGasPrice,
 			globalFeeParams: globalfeeParamsLow,
-			gasPrice:        sdk.NewCoins(sdk.NewCoin("uatom", sdk.ZeroInt())),
+			gasPrice:        sdk.NewCoins(sdk.NewCoin("uatom", lowFeeAmt)),
 			gasLimit:        testGasLimit,
 			txMsg:           testdata.NewTestMsg(addr1),
 			txCheck:         false,
 			expErr:          false,
 		},
-		"disable checkTx: no fee check. min_gas_price is low, global fee is low, tx fee's denom is not in global fees denoms set": {
+		"disable checkTx: min_gas_price is medium, global fee is low, tx is zero": {
+			minGasPrice:     minGasPrice,
+			globalFeeParams: globalfeeParamsLow,
+			gasPrice:        sdk.NewCoins(sdk.NewCoin("uatom", sdk.ZeroInt())),
+			gasLimit:        testGasLimit,
+			txMsg:           testdata.NewTestMsg(addr1),
+			txCheck:         false,
+			expErr:          true,
+		},
+		"disable checkTx: min_gas_price is low, global fee is low, tx fee's denom is not in global fees denoms set": {
 			minGasPrice:     minGasPrice,
 			globalFeeParams: globalfeeParamsLow,
 			gasPrice:        sdk.NewCoins(sdk.NewCoin("quark", sdk.ZeroInt())),
 			gasLimit:        testGasLimit,
 			txMsg:           testdata.NewTestMsg(addr1),
 			txCheck:         false,
-			expErr:          false,
+			expErr:          true,
 		},
 	}
 	for name, tc := range testCases {
@@ -746,3 +755,55 @@ func (s *IntegrationTestSuite) TestContainsOnlyBypassMinFeeMsgs() {
 		})
 	}
 }
+
+func (s *IntegrationTestSuite) TestGetTxFeeRequired() {
+	// create global fee params
+	globalfeeParamsEmpty := &globfeetypes.Params{MinimumGasPrices: []sdk.DecCoin{}}
+
+	// setup tests with default global fee i.e. "0uatom" and empty local min gas prices
+	feeDecorator, _ := s.SetupTestGlobalFeeStoreAndMinGasPrice([]sdk.DecCoin{}, globalfeeParamsEmpty)
+
+	// set a subspace that doesn't have the stakingtypes.KeyBondDenom key registred
+	feeDecorator.StakingSubspace = s.app.GetSubspace(globfeetypes.ModuleName)
+
+	// check that an error is returned when staking bond denom is empty
+	_, err := feeDecorator.GetTxFeeRequired(s.ctx, nil)
+	s.Require().Equal(err.Error(), "empty staking bond denomination")
+
+	// set non-zero local min gas prices
+	localMinGasPrices := sdk.NewCoins(sdk.NewCoin("uatom", sdk.NewInt(1)))
+
+	// setup tests with non-empty local min gas prices
+	feeDecorator, _ = s.SetupTestGlobalFeeStoreAndMinGasPrice(
+		sdk.NewDecCoinsFromCoins(localMinGasPrices...),
+		globalfeeParamsEmpty,
+	)
+
+	// mock tx data
+	s.txBuilder = s.clientCtx.TxConfig.NewTxBuilder()
+	priv1, _, addr1 := testdata.KeyTestPubAddr()
+	privs, accNums, accSeqs := []cryptotypes.PrivKey{priv1}, []uint64{0}, []uint64{0}
+
+	s.Require().NoError(s.txBuilder.SetMsgs(testdata.NewTestMsg(addr1)))
+	s.txBuilder.SetFeeAmount(sdk.NewCoins(sdk.NewCoin("uatom", sdk.ZeroInt())))
+
+	s.txBuilder.SetGasLimit(uint64(1))
+	tx, err := s.CreateTestTx(privs, accNums, accSeqs, s.ctx.ChainID())
+	s.Require().NoError(err)
+
+	// check that the required fees returned in CheckTx mode are equal to
+	// local min gas prices since they're greater than the default global fee values.
+	s.Require().True(s.ctx.IsCheckTx())
+	res, err := feeDecorator.GetTxFeeRequired(s.ctx, tx)
+	s.Require().True(res.IsEqual(localMinGasPrices))
+	s.Require().NoError(err)
+
+	// check that the global fee is returned in DeliverTx mode.
+	globalFee, err := feeDecorator.GetGlobalFee(s.ctx, tx)
+	s.Require().NoError(err)
+
+	ctx := s.ctx.WithIsCheckTx(false)
+	res, err = feeDecorator.GetTxFeeRequired(ctx, tx)
+	s.Require().NoError(err)
+	s.Require().True(res.IsEqual(globalFee))
+}
diff --git a/x/globalfee/ante/fee.go b/x/globalfee/ante/fee.go
@@ -57,93 +57,120 @@ func (mfd FeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, ne
 		return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must implement the sdk.FeeTx interface")
 	}
 
-	// Only check for minimum fees and global fee if the execution mode is CheckTx
-	if !ctx.IsCheckTx() || simulate {
+	// Do not check minimum-gas-prices and global fees during simulations
+	if simulate {
 		return next(ctx, tx, simulate)
 	}
 
-	// Sort fee tx's coins, zero coins in feeCoins are already removed
-	feeCoins := feeTx.GetFee().Sort()
-	gas := feeTx.GetGas()
-	msgs := feeTx.GetMsgs()
-
-	// Get required Global Fee
-	requiredGlobalFees, err := mfd.GetGlobalFee(ctx, feeTx)
+	// Get the required fees according to the CheckTx or DeliverTx modes
+	feeRequired, err := mfd.GetTxFeeRequired(ctx, feeTx)
 	if err != nil {
 		return ctx, err
 	}
 
 	// reject the transaction early if the feeCoins have more denoms than the fee requirement
-	if feeCoins.Len() > requiredGlobalFees.Len() {
-		return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidCoins, "fee is not a subset of required fees; got %s, required: %s", feeCoins.String(), requiredGlobalFees.String())
+	// feeRequired cannot be be empty
+	if feeTx.GetFee().Len() > feeRequired.Len() {
+		return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidCoins, "fee is not a subset of required fees; got %s, required: %s", feeTx.GetFee().String(), feeRequired.String())
 	}
 
-	// Get local minimum-gas-prices
-	localFees := GetMinGasPrice(ctx, int64(feeTx.GetGas()))
-
-	// CombinedFeeRequirement should never be empty since
-	// global fee is set to its default value, i.e. 0uatom, if empty
-	combinedFeeRequirement := CombinedFeeRequirement(requiredGlobalFees, localFees)
-	if len(combinedFeeRequirement) == 0 {
-		return ctx, sdkerrors.Wrapf(sdkerrors.ErrNotFound, "required fees are not setup.")
-	}
+	// Sort fee tx's coins, zero coins in feeCoins are already removed
+	feeCoins := feeTx.GetFee().Sort()
+	gas := feeTx.GetGas()
+	msgs := feeTx.GetMsgs()
 
-	nonZeroCoinFeesReq, zeroCoinFeesDenomReq := getNonZeroFees(combinedFeeRequirement)
+	// split feeRequired into zero and non-zero coins(nonZeroCoinFeesReq, zeroCoinFeesDenomReq), split feeCoins according to
+	// nonZeroCoinFeesReq, zeroCoinFeesDenomReq,
+	// so that feeCoins can be checked separately against them.
+	nonZeroCoinFeesReq, zeroCoinFeesDenomReq := getNonZeroFees(feeRequired)
 
-	// feeCoinsNonZeroDenom contains non-zero denominations from the combinedFeeRequirement
-	//
-	// feeCoinsNoZeroDenom is used to check if the fees meets the requirement imposed by nonZeroCoinFeesReq
-	// when feeCoins does not contain zero coins' denoms in combinedFeeRequirement
+	// feeCoinsNonZeroDenom contains non-zero denominations from the feeRequired
+	// feeCoinsNonZeroDenom is used to check if the fees meets the requirement imposed by nonZeroCoinFeesReq
+	// when feeCoins does not contain zero coins' denoms in feeRequired
 	feeCoinsNonZeroDenom, feeCoinsZeroDenom := splitCoinsByDenoms(feeCoins, zeroCoinFeesDenomReq)
 
 	// Check that the fees are in expected denominations.
-	// if feeCoinsNoZeroDenom=[], DenomsSubsetOf returns true
-	// if feeCoinsNoZeroDenom is not empty, but nonZeroCoinFeesReq empty, return false
+	// according to splitCoinsByDenoms(), feeCoinsZeroDenom must be in denom subset of zeroCoinFeesDenomReq.
+	// check if feeCoinsNonZeroDenom is a subset of nonZeroCoinFeesReq.
+	// special case: if feeCoinsNonZeroDenom=[], DenomsSubsetOf returns true
+	// special case: if feeCoinsNonZeroDenom is not empty, but nonZeroCoinFeesReq empty, return false
 	if !feeCoinsNonZeroDenom.DenomsSubsetOf(nonZeroCoinFeesReq) {
-		return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidCoins, "fee is not a subset of required fees; got %s, required: %s", feeCoins.String(), combinedFeeRequirement.String())
+		return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "fee is not a subset of required fees; got %s, required: %s", feeCoins.String(), feeRequired.String())
 	}
 
-	// Accept zero fee transactions only if both of the following statements are true:
+	// If the feeCoins pass the denoms check, check they are bypass-msg types.
 	//
+	// Bypass min fee requires:
 	// 	- the tx contains only message types that can bypass the minimum fee,
 	//	see BypassMinFeeMsgTypes;
 	//	- the total gas limit per message does not exceed MaxTotalBypassMinFeeMsgGasUsage,
 	//	i.e., totalGas <=  MaxTotalBypassMinFeeMsgGasUsage
-	//
 	// Otherwise, minimum fees and global fees are checked to prevent spam.
 	doesNotExceedMaxGasUsage := gas <= mfd.MaxTotalBypassMinFeeMsgGasUsage
 	allowedToBypassMinFee := mfd.ContainsOnlyBypassMinFeeMsgs(msgs) && doesNotExceedMaxGasUsage
+	if allowedToBypassMinFee {
+		return next(ctx, tx, simulate)
+	}
 
-	// Either the transaction contains at least one message of a type
-	// that cannot bypass the minimum fee or the total gas limit exceeds
-	// the imposed threshold. As a result, besides check the fees are in
-	// expected denominations, check the amounts are greater or equal than
-	// the expected amounts.
-
-	// only check feeCoinsNoZeroDenom has coins IsAnyGTE than nonZeroCoinFeesReq
-	// when feeCoins does not contain denoms of zero denoms in combinedFeeRequirement
-	if !allowedToBypassMinFee && len(feeCoinsZeroDenom) == 0 {
-		// special case: when feeCoins=[] and there is zero coin in fee requirement
-		if len(feeCoins) == 0 && len(zeroCoinFeesDenomReq) != 0 {
+	// if the msg does not satisfy bypass condition and the feeCoins denoms are subset of feeRequired,
+	// check the feeCoins amount against feeRequired
+	//
+	// when feeCoins=[]
+	// special case: and there is zero coin in fee requirement, pass,
+	// otherwise, err
+	if len(feeCoins) == 0 {
+		if len(zeroCoinFeesDenomReq) != 0 {
 			return next(ctx, tx, simulate)
 		}
+		return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins.String(), feeRequired.String())
+	}
 
-		// Check that the amounts of the fees are greater or equal than
-		// the expected amounts, i.e., at least one feeCoin amount must
-		// be greater or equal to one of the combined required fees.
+	// when feeCoins != []
+	// special case: if TX has at least one of the zeroCoinFeesDenomReq, then it should pass
+	if len(feeCoinsZeroDenom) > 0 {
+		return next(ctx, tx, simulate)
+	}
 
-		// if feeCoinsNoZeroDenom=[], return false
-		// if nonZeroCoinFeesReq=[], return false (this situation should not happen
-		// because when nonZeroCoinFeesReq empty, and DenomsSubsetOf check passed,
-		// the tx should already passed before)
-		if !feeCoinsNonZeroDenom.IsAnyGTE(nonZeroCoinFeesReq) {
-			return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins.String(), combinedFeeRequirement.String())
-		}
+	// After all the checks, the tx is confirmed:
+	// feeCoins denoms subset off feeRequired
+	// Not bypass
+	// feeCoins != []
+	// Not contain zeroCoinFeesDenomReq's denoms
+	//
+	// check if the feeCoins's feeCoinsNonZeroDenom part has coins' amount higher/equal to nonZeroCoinFeesReq
+	if !feeCoinsNonZeroDenom.IsAnyGTE(nonZeroCoinFeesReq) {
+		return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins.String(), feeRequired.String())
 	}
 
 	return next(ctx, tx, simulate)
 }
 
+// GetTxFeeRequired returns the required fees for the given FeeTx.
+// In case the FeeTx's mode is CheckTx, it returns the combined requirements
+// of local min gas prices and global fees. Otherwise, in DeliverTx, it returns the global fee.
+func (mfd FeeDecorator) GetTxFeeRequired(ctx sdk.Context, tx sdk.FeeTx) (sdk.Coins, error) {
+	// Get required global fee min gas prices
+	// Note that it should never be empty since its default value is set to coin={"StakingBondDenom", 0}
+	globalFees, err := mfd.GetGlobalFee(ctx, tx)
+	if err != nil {
+		return sdk.Coins{}, err
+	}
+
+	// In DeliverTx, the global fee min gas prices are the only tx fee requirements.
+	if !ctx.IsCheckTx() {
+		return globalFees, nil
+	}
+
+	// In CheckTx mode, the local and global fee min gas prices are combined
+	// to form the tx fee requirements
+
+	// Get local minimum-gas-prices
+	localFees := GetMinGasPrice(ctx, int64(tx.GetGas()))
+
+	// Return combined fee requirements
+	return CombinedFeeRequirement(globalFees, localFees)
+}
+
 // GetGlobalFee returns the global fees for a given fee tx's gas
 // (might also return 0denom if globalMinGasPrice is 0)
 // sorted in ascending order.
@@ -176,6 +203,7 @@ func (mfd FeeDecorator) GetGlobalFee(ctx sdk.Context, feeTx sdk.FeeTx) (sdk.Coin
 	return requiredGlobalFees.Sort(), nil
 }
 
+// DefaultZeroGlobalFee returns a zero coin with the staking module bond denom
 func (mfd FeeDecorator) DefaultZeroGlobalFee(ctx sdk.Context) ([]sdk.DecCoin, error) {
 	bondDenom := mfd.getBondDenom(ctx)
 	if bondDenom == "" {
@@ -187,6 +215,7 @@ func (mfd FeeDecorator) DefaultZeroGlobalFee(ctx sdk.Context) ([]sdk.DecCoin, er
 
 func (mfd FeeDecorator) getBondDenom(ctx sdk.Context) string {
 	var bondDenom string
+
 	if mfd.StakingSubspace.Has(ctx, stakingtypes.KeyBondDenom) {
 		mfd.StakingSubspace.Get(ctx, stakingtypes.KeyBondDenom, &bondDenom)
 	}