feat!: move Global Fee checks to DeliverTx

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

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"]

go.sum

@@ -1023,8 +1023,6 @@ github.com/openzipkin/zipkin-go v0.1.6/go.mod h1:QgAqvLzwWbR/WpD4A3cGpPtJrZXNIiJ
 github.com/openzipkin/zipkin-go v0.2.1/go.mod h1:NaW6tEwdmWMaCDZzg8sh+IBNOxHMPnhQw8ySjnjRyN4=
 github.com/openzipkin/zipkin-go v0.2.2/go.mod h1:NaW6tEwdmWMaCDZzg8sh+IBNOxHMPnhQw8ySjnjRyN4=
 github.com/ory/dockertest v3.3.5+incompatible h1:iLLK6SQwIhcbrG783Dghaaa3WPzGc+4Emza6EbVUUGA=
-github.com/ory/dockertest/v3 v3.9.1 h1:v4dkG+dlu76goxMiTT2j8zV7s4oPPEppKT8K8p2f1kY=
-github.com/ory/dockertest/v3 v3.9.1/go.mod h1:42Ir9hmvaAPm0Mgibk6mBPi7SFvTXxEcnztDYOJ//uM=
 github.com/ory/dockertest/v3 v3.10.0 h1:4K3z2VMe8Woe++invjaTB7VRyQXQy5UY+loujO4aNE4=
 github.com/ory/dockertest/v3 v3.10.0/go.mod h1:nr57ZbRWMqfsdGdFNLHz5jjNdDb7VVFnzAeW1n5N1Lg=
 github.com/otiai10/copy v1.2.0/go.mod h1:rrF5dJ5F0t/EWSYODDu4j9/vEeYHMkc8jt0zJChqQWw=

tests/e2e/e2e_exec_test.go

@@ -442,6 +442,7 @@ func (s *IntegrationTestSuite) executeDelegate(c *chain, valIdx int, amount, val
 		fmt.Sprintf("--%s=%s", flags.FlagGas, "auto"),
 		fmt.Sprintf("--%s=%s", flags.FlagFees, delegateFees),
 		"--keyring-backend=test",
+		fmt.Sprintf("--%s=%s", flags.FlagGasAdjustment, "1.5"),
 		fmt.Sprintf("--%s=%s", flags.FlagHome, home),
 		"--output=json",
 		"-y",
@@ -471,6 +472,7 @@ func (s *IntegrationTestSuite) executeRedelegate(c *chain, valIdx int, amount, o
 		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.FlagGasAdjustment, "1.5"),
 		"--keyring-backend=test",
 		fmt.Sprintf("--%s=%s", flags.FlagHome, home),
 		"--output=json",
@@ -538,7 +540,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 +612,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))

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"
 )
 

x/globalfee/ante/antetest/fee_test.go

@@ -6,6 +6,7 @@ import (
 	cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
 	"github.com/cosmos/cosmos-sdk/testutil/testdata"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
 	ibcclienttypes "github.com/cosmos/ibc-go/v4/modules/core/02-client/types"
 	ibcchanneltypes "github.com/cosmos/ibc-go/v4/modules/core/04-channel/types"
@@ -574,23 +575,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 +756,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)
+
+	// reset decorator staking subspace
+	feeDecorator.StakingSubspace = paramtypes.Subspace{}
+
+	// check that an error is returned when staking subspace isn't set
+	_, 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))
+}

x/globalfee/ante/fee.go

@@ -57,8 +57,8 @@ 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 and global fees during simulations
+	if simulate {
 		return next(ctx, tx, simulate)
 	}
 
@@ -67,35 +67,25 @@ func (mfd FeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, ne
 	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
 	}
 
-	// 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.")
-	}
+	nonZeroCoinFeesReq, zeroCoinFeesDenomReq := getNonZeroFees(feeRequired)
 
-	nonZeroCoinFeesReq, zeroCoinFeesDenomReq := getNonZeroFees(combinedFeeRequirement)
-
-	// feeCoinsNonZeroDenom contains non-zero denominations from the combinedFeeRequirement
+	// feeCoinsNonZeroDenom contains non-zero denominations from the feeRequired
 	//
 	// feeCoinsNoZeroDenom is used to check if the fees meets the requirement imposed by nonZeroCoinFeesReq
-	// when feeCoins does not contain zero coins' denoms in combinedFeeRequirement
+	// 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
 	if !feeCoinsNonZeroDenom.DenomsSubsetOf(nonZeroCoinFeesReq) {
-		return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "fee is not a subset of required fees; got %s, required: %s", feeCoins, combinedFeeRequirement)
+		return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "fee is not a subset of required fees; got %s, required: %s", feeCoins, feeRequired)
 	}
 
 	// Accept zero fee transactions only if both of the following statements are true:
@@ -116,7 +106,7 @@ func (mfd FeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, ne
 	// the expected amounts.
 
 	// only check feeCoinsNoZeroDenom has coins IsAnyGTE than nonZeroCoinFeesReq
-	// when feeCoins does not contain denoms of zero denoms in combinedFeeRequirement
+	// when feeCoins does not contain denoms of zero denoms in feeRequired
 	if !allowedToBypassMinFee && len(feeCoinsZeroDenom) == 0 {
 		// special case: when feeCoins=[] and there is zero coin in fee requirement
 		if len(feeCoins) == 0 && len(zeroCoinFeesDenomReq) != 0 {
@@ -132,13 +122,39 @@ func (mfd FeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, ne
 		// 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, combinedFeeRequirement)
+			return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins, feeRequired)
 		}
 	}
 
 	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.
@@ -171,6 +187,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 == "" {
@@ -181,11 +198,15 @@ 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)
+	// prevent the getter above to panic
+	// when the staking subspace isn't set
+	if !mfd.StakingSubspace.HasKeyTable() {
+		return ""
 	}
 
+	var bondDenom string
+	mfd.StakingSubspace.Get(ctx, stakingtypes.KeyBondDenom, &bondDenom)
+
 	return bondDenom
 }
 

x/globalfee/ante/fee_utils.go

@@ -2,6 +2,7 @@ package ante
 
 import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 )
 
 // ContainZeroCoins returns true if the given coins are empty or contain zero coins,
@@ -23,14 +24,16 @@ func ContainZeroCoins(coins sdk.Coins) bool {
 // Both globalFees and minGasPrices must be valid, but CombinedFeeRequirement
 // does not validate them, so it may return 0denom.
 // if globalfee is empty, CombinedFeeRequirement return sdk.Coins{}
-func CombinedFeeRequirement(globalFees, minGasPrices sdk.Coins) sdk.Coins {
+func CombinedFeeRequirement(globalFees, minGasPrices sdk.Coins) (sdk.Coins, error) {
+	// global fees should never be empty
+	// since it has a default value using the staking module's bond denom
+	if len(globalFees) == 0 {
+		return sdk.Coins{}, sdkerrors.Wrapf(sdkerrors.ErrNotFound, "global fee cannot be empty")
+	}
+
 	// empty min_gas_price
 	if len(minGasPrices) == 0 {
-		return globalFees
-	}
-	// empty global fee is not possible if we set default global fee
-	if len(globalFees) == 0 && len(minGasPrices) != 0 {
-		return sdk.Coins{}
+		return globalFees, nil
 	}
 
 	// if min_gas_price denom is in globalfee, and the amount is higher than globalfee, add min_gas_price to allFees
@@ -45,7 +48,7 @@ func CombinedFeeRequirement(globalFees, minGasPrices sdk.Coins) sdk.Coins {
 		}
 	}
 
-	return allFees.Sort()
+	return allFees.Sort(), nil
 }
 
 // Find replaces the functionality of Coins.Find from SDK v0.46.x

x/globalfee/ante/fee_utils_test.go

@@ -81,16 +81,11 @@ func TestCombinedFeeRequirement(t *testing.T) {
 		c        sdk.Coins
 		combined sdk.Coins
 	}{
-		"global fee empty, min fee empty, combined fee empty": {
+		"global fee invalid, return combined fee empty and non-nil error": {
 			cGlobal:  coinsEmpty,
 			c:        coinsEmpty,
 			combined: coinsEmpty,
 		},
-		"global fee empty, min fee nonempty, combined fee empty": {
-			cGlobal:  coinsEmpty,
-			c:        coinsNonEmpty,
-			combined: coinsEmpty,
-		},
 		"global fee nonempty, min fee empty, combined fee = global fee": {
 			cGlobal:  coinsNonEmpty,
 			c:        coinsNonEmpty,
@@ -155,7 +150,12 @@ func TestCombinedFeeRequirement(t *testing.T) {
 
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
-			allFees := CombinedFeeRequirement(test.cGlobal, test.c)
+			allFees, err := CombinedFeeRequirement(test.cGlobal, test.c)
+			if len(test.cGlobal) == 0 {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
 			require.Equal(t, test.combined, allFees)
 		})
 	}