refactor: bypass-msgs (#2218)

* refactor: maxBypassMinFeeMsgGasUsage -> maxTotalBypassMinFeeMsgGasUsage * add more bypass-msg-types to default bypass-msg * docs: update bypass-msg in globalfee.md * test: add tests for bypass-msg * refactor: containsOnlyBypassMinFeeMsgs * docs: update changelog * Apply suggestions from code review Co-authored-by: MSalopek <[email protected]> * docs: update comments on bypass-min-fee-msgs * Update docs/modules/globalfee.md * workflow: increase upgrade test timeout * workflow: increase upgrade test timeout * workflow: use latest cosmovisor in upgrade test * fix typo * fix: upgrade test * fix: cosmovisor start in upgrade test * change timeout for upgrade test in workflow --------- Co-authored-by: MSalopek <[email protected]>
cosmos · Mar 17, 2023 · 483fb4d · 483fb4d
1 parent 53a07ee
commit 483fb4d
Show file tree

Hide file tree

Showing 10 changed files with 82 additions and 34 deletions.
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -45,7 +45,7 @@ jobs:
           go-version: 1.18
       - name: test & coverage report creation
         run: |
-          go test -v -coverprofile=coverage.txt -covermode=atomic -coverpkg=./... $(go list ./... | grep -v -e '/tests/e2e') 
+          go test -v -coverprofile=coverage.txt -covermode=atomic -coverpkg=./... $(go list ./... | grep -v -e '/tests/e2e')
       - name: filter non-testable files
         run: |
           excludelist="$(find ./ -type f -name '*.go' | xargs grep -l 'DONTCOVER')"
@@ -128,7 +128,7 @@ jobs:
             go.sum
       - name: Install GaiaV8
         run: |
-          git checkout v8.0.0-rc3
+          git checkout v8.0.0
           make build
           cp ./build/gaiad ./build/gaiad8
         if: env.GIT_DIFF
@@ -140,7 +140,7 @@ jobs:
         if: env.GIT_DIFF
       - name: Install Cosmovisor
         run: |
-          go install github.com/cosmos/cosmos-sdk/cosmovisor/cmd/cosmovisor@v1.0.0
+          go install github.com/cosmos/cosmos-sdk/cosmovisor/cmd/cosmovisor@latest
         if: env.GIT_DIFF
       - name: Start GaiaV8
         run: |

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -35,6 +35,9 @@ Ref: https://keepachangelog.com/en/1.0.0/
 # Changelog
 
 ## [Unreleased]
+* 
+* (feat) Add two more msg types `/ibc.core.channel.v1.MsgTimeout` and `/ibc.core.channel.v1.MsgTimeoutOnClose` to default `bypass-min-fee-msg-types`.
+* (feat) Change the bypassing gas usage criteria. Instead of requiring 200,000 gas per `bypass-min-fee-msg`, we will now allow a maximum total usage of 1,000,000 gas for all bypassed messages in a transaction. Note that all messages in the transaction must be the `bypass-min-fee-msg-types` for the bypass min fee to take effect, otherwise, fee payment will still apply.
 
 ## [v9.0.1] - 2023-03-09
 

diff --git a/ante/ante.go b/ante/ante.go
@@ -53,11 +53,12 @@ func NewAnteHandler(opts HandlerOptions) (sdk.AnteHandler, error) {
 		sigGasConsumer = ante.DefaultSigVerificationGasConsumer
 	}
 
-	// maxBypassMinFeeMsgGasUsage is the maximum gas usage per message
-	// so that a transaction that contains only message types that can
-	// bypass the minimum fee can be accepted with a zero fee.
+	// maxTotalBypassMinFeeMsgGasUsage is the allowed maximum gas usage
+	// for all the bypass msgs in a transactions.
+	// A transaction that contains only bypass message types and the gas usage does not
+	// exceed maxTotalBypassMinFeeMsgGasUsage can be accepted with a zero fee.
 	// For details, see gaiafeeante.NewFeeDecorator()
-	var maxBypassMinFeeMsgGasUsage uint64 = 200_000
+	var maxTotalBypassMinFeeMsgGasUsage uint64 = 1_000_000
 
 	anteDecorators := []sdk.AnteDecorator{
 		ante.NewSetUpContextDecorator(), // outermost AnteDecorator. SetUpContext must be called first
@@ -67,7 +68,7 @@ func NewAnteHandler(opts HandlerOptions) (sdk.AnteHandler, error) {
 		ante.NewValidateMemoDecorator(opts.AccountKeeper),
 		ante.NewConsumeGasForTxSizeDecorator(opts.AccountKeeper),
 		NewGovPreventSpamDecorator(opts.Codec, opts.GovKeeper),
-		gaiafeeante.NewFeeDecorator(opts.BypassMinFeeMsgTypes, opts.GlobalFeeSubspace, opts.StakingSubspace, maxBypassMinFeeMsgGasUsage),
+		gaiafeeante.NewFeeDecorator(opts.BypassMinFeeMsgTypes, opts.GlobalFeeSubspace, opts.StakingSubspace, maxTotalBypassMinFeeMsgGasUsage),
 
 		ante.NewDeductFeeDecorator(opts.AccountKeeper, opts.BankKeeper, opts.FeegrantKeeper),
 		ante.NewSetPubKeyDecorator(opts.AccountKeeper), // SetPubKeyDecorator must be called before all signature verification decorators

diff --git a/app/app.go b/app/app.go
@@ -236,6 +236,8 @@ func GetDefaultBypassFeeMessages() []string {
 		sdk.MsgTypeURL(&ibcchanneltypes.MsgRecvPacket{}),
 		sdk.MsgTypeURL(&ibcchanneltypes.MsgAcknowledgement{}),
 		sdk.MsgTypeURL(&ibcclienttypes.MsgUpdateClient{}),
+		sdk.MsgTypeURL(&ibcchanneltypes.MsgTimeout{}),
+		sdk.MsgTypeURL(&ibcchanneltypes.MsgTimeoutOnClose{}),
 	}
 }
 

diff --git a/contrib/scripts/run-gaia-v8.sh b/contrib/scripts/run-gaia-v8.sh
@@ -67,5 +67,5 @@ perl -i~ -0777 -pe 's/# Enable defines if the API server should be enabled.
 enable = false/# Enable defines if the API server should be enabled.
 enable = true/g' $NODE_HOME/config/app.toml
 
-$COSMOVISOR start --home $NODE_HOME --x-crisis-skip-assert-invariants
+$COSMOVISOR run start --home $NODE_HOME --x-crisis-skip-assert-invariants
 
diff --git a/contrib/scripts/run-upgrade-commands.sh b/contrib/scripts/run-upgrade-commands.sh
@@ -31,7 +31,7 @@ if test -f "$BINARY"; then
 
 
   key=$($BINARY keys show val --home $NODE_HOME)
-  if [ key == "" ]; then
+  if [ -z "$key" ]; then
     echo $USER_MNEMONIC | $BINARY --home $NODE_HOME keys add val --recover --keyring-backend=test
   fi
 

diff --git a/docs/modules/globalfee.md b/docs/modules/globalfee.md
@@ -51,14 +51,15 @@ The `minimum-gas-prices` config parameter allows node operators to impose additi
 Bypass messages are messages that are exempt from paying fees. The above global fees and `minimum-gas-prices` checks do not apply for transactions that satisfy the following conditions: 
 
 - Contains only bypass message types, i.e., bypass transactions.
-- The total gas used is less than or equal to `len(messages) * MaxBypassMinFeeMsgGasUsage`. Note: the current `MaxBypassMinFeeMsgGasUsage` is set to `200,000`.
+- The total gas used is less than or equal to `MaxTotalBypassMinFeeMsgGasUsage`. Note: the current `MaxTotalBypassMinFeeMsgGasUsage` is set to `1,000,000`.
 - In case of non-zero transaction fees, the denom has to be a subset of denoms defined in the global fees list.
 
 Node operators can configure `bypass-min-fee-msg-types` in `config/app.toml`.
 
-Nodes created using Gaiad `v7.0.2` or later use `["/ibc.core.channel.v1.MsgRecvPacket", "/ibc.core.channel.v1.MsgAcknowledgement","/ibc.applications.transfer.v1.MsgTransfer"]` as defaults. Nodes with `bypass-min-fee-msg-types = []` or missing this field in `app.toml` also use default bypass message types.
-
-Nodes created using Gaiad `v7.0.1` or earlier do not have `bypass-min-fee-msg-types` configured in `config/app.toml` - they are also using default values. The `bypass-min-fee-msg-types` config option can be added to `config/app.toml` before the `[telemetry]` field.
+- Nodes created using Gaiad `v7.0.2` or `v9.0.x` use `["/ibc.core.channel.v1.MsgRecvPacket", "/ibc.core.channel.v1.MsgAcknowledgement","/ibc.applications.transfer.v1.MsgTransfer"]` as defaults. 
+- Nodes created using Gaiad `v10.0.x` or later use `["/ibc.core.channel.v1.MsgRecvPacket", "/ibc.core.channel.v1.MsgAcknowledgement","/ibc.applications.transfer.v1.MsgTransfer", "/ibc.core.channel.v1.MsgTimeout", "/ibc.core.channel.v1.MsgTimeoutOnClose"]` as defaults. 
+- Node Nodes with `bypass-min-fee-msg-types = []` or missing this field in `app.toml` also use default bypass message types.
+- Nodes created using Gaiad `v7.0.1` and `v7.0.0` do not have `bypass-min-fee-msg-types` configured in `config/app.toml` - they are also using same default values as in `v7.0.2`. The `bypass-min-fee-msg-types` config option can be added to `config/app.toml` before the `[telemetry]` field.
 
 An example of `bypass-min-fee-msg-types` in `app.toml`:
 
@@ -72,7 +73,7 @@ An example of `bypass-min-fee-msg-types` in `app.toml`:
 #
 # Example:
 # ["/ibc.core.channel.v1.MsgRecvPacket", "/ibc.core.channel.v1.MsgAcknowledgement", ...]
-bypass-min-fee-msg-types = ["/ibc.core.channel.v1.MsgRecvPacket", "/ibc.core.channel.v1.MsgAcknowledgement","/ibc.applications.transfer.v1.MsgTransfer"]
+bypass-min-fee-msg-types = ["/ibc.core.channel.v1.MsgRecvPacket", "/ibc.core.channel.v1.MsgAcknowledgement","/ibc.applications.transfer.v1.MsgTransfer", "/ibc.core.channel.v1.MsgTimeout", "/ibc.core.channel.v1.MsgTimeoutOnClose"]
 ```
 
 

diff --git a/x/globalfee/ante/antetest/fee_test.go b/x/globalfee/ante/antetest/fee_test.go
@@ -468,8 +468,7 @@ func (s *IntegrationTestSuite) TestGlobalFeeMinimumGasFeeAnteHandler() {
 			txCheck:         true,
 			expErr:          false,
 		},
-		// test bypass msg
-		"msg type ibc, zero fee in globalfee denom": {
+		"bypass msg type: ibc.core.channel.v1.MsgRecvPacket": {
 			minGasPrice:     minGasPrice,
 			globalFeeParams: globalfeeParamsLow,
 			gasPrice:        sdk.NewCoins(sdk.NewCoin("uatom", sdk.ZeroInt())),
@@ -479,6 +478,46 @@ func (s *IntegrationTestSuite) TestGlobalFeeMinimumGasFeeAnteHandler() {
 			txCheck: true,
 			expErr:  false,
 		},
+		"bypass msg type: ibc.core.channel.v1.MsgTimeout": {
+			minGasPrice:     minGasPrice,
+			globalFeeParams: globalfeeParamsLow,
+			gasPrice:        sdk.NewCoins(sdk.NewCoin("uatom", sdk.ZeroInt())),
+			gasLimit:        newTestGasLimit(),
+			txMsg: ibcchanneltypes.NewMsgTimeout(
+				ibcchanneltypes.Packet{}, 1, nil, ibcclienttypes.Height{}, ""),
+			txCheck: true,
+			expErr:  false,
+		},
+		"bypass msg type: ibc.core.channel.v1.MsgTimeoutOnClose": {
+			minGasPrice:     minGasPrice,
+			globalFeeParams: globalfeeParamsLow,
+			gasPrice:        sdk.NewCoins(sdk.NewCoin("uatom", sdk.ZeroInt())),
+			gasLimit:        newTestGasLimit(),
+			txMsg: ibcchanneltypes.NewMsgTimeout(
+				ibcchanneltypes.Packet{}, 2, nil, ibcclienttypes.Height{}, ""),
+			txCheck: true,
+			expErr:  false,
+		},
+		"bypass msg gas usage exceeds maxTotalBypassMinFeeMsgGasUsage": {
+			minGasPrice:     minGasPrice,
+			globalFeeParams: globalfeeParamsLow,
+			gasPrice:        sdk.NewCoins(sdk.NewCoin("uatom", sdk.ZeroInt())),
+			gasLimit:        2 * newTestMaxTotalBypassMinFeeMsgGasUsage(),
+			txMsg: ibcchanneltypes.NewMsgTimeout(
+				ibcchanneltypes.Packet{}, 2, nil, ibcclienttypes.Height{}, ""),
+			txCheck: true,
+			expErr:  true,
+		},
+		"bypass msg gas usage equals to maxTotalBypassMinFeeMsgGasUsage": {
+			minGasPrice:     minGasPrice,
+			globalFeeParams: globalfeeParamsLow,
+			gasPrice:        sdk.NewCoins(sdk.NewCoin("uatom", sdk.ZeroInt())),
+			gasLimit:        newTestMaxTotalBypassMinFeeMsgGasUsage(),
+			txMsg: ibcchanneltypes.NewMsgTimeout(
+				ibcchanneltypes.Packet{}, 3, nil, ibcclienttypes.Height{}, ""),
+			txCheck: true,
+			expErr:  false,
+		},
 		"msg type ibc, zero fee not in globalfee denom": {
 			minGasPrice:     minGasPrice,
 			globalFeeParams: globalfeeParamsLow,
@@ -573,9 +612,8 @@ func (s *IntegrationTestSuite) TestGlobalFeeMinimumGasFeeAnteHandler() {
 			stakingParam.BondDenom = "uatom"
 			stakingSubspace := s.SetupTestStakingSubspace(stakingParam)
 			// setup antehandler
-			mfd := gaiafeeante.NewFeeDecorator(gaiaapp.GetDefaultBypassFeeMessages(), globalfeeSubspace, stakingSubspace, newTestGasLimit())
+			mfd := gaiafeeante.NewFeeDecorator(gaiaapp.GetDefaultBypassFeeMessages(), globalfeeSubspace, stakingSubspace, newTestMaxTotalBypassMinFeeMsgGasUsage())
 			antehandler := sdk.ChainAnteDecorators(mfd)
-
 			s.Require().NoError(s.txBuilder.SetMsgs(testCase.txMsg))
 			s.txBuilder.SetFeeAmount(testCase.gasPrice)
 			s.txBuilder.SetGasLimit(testCase.gasLimit)
@@ -597,3 +635,7 @@ func (s *IntegrationTestSuite) TestGlobalFeeMinimumGasFeeAnteHandler() {
 func newTestGasLimit() uint64 {
 	return 200000
 }
+
+func newTestMaxTotalBypassMinFeeMsgGasUsage() uint64 {
+	return 1000000
+}
diff --git a/x/globalfee/ante/fee.go b/x/globalfee/ante/fee.go
@@ -26,13 +26,13 @@ import (
 var _ sdk.AnteDecorator = FeeDecorator{}
 
 type FeeDecorator struct {
-	BypassMinFeeMsgTypes       []string
-	GlobalMinFee               globalfee.ParamSource
-	StakingSubspace            paramtypes.Subspace
-	MaxBypassMinFeeMsgGasUsage uint64
+	BypassMinFeeMsgTypes            []string
+	GlobalMinFee                    globalfee.ParamSource
+	StakingSubspace                 paramtypes.Subspace
+	MaxTotalBypassMinFeeMsgGasUsage uint64
 }
 
-func NewFeeDecorator(bypassMsgTypes []string, globalfeeSubspace, stakingSubspace paramtypes.Subspace, maxBypassMinFeeMsgGasUsage uint64) FeeDecorator {
+func NewFeeDecorator(bypassMsgTypes []string, globalfeeSubspace, stakingSubspace paramtypes.Subspace, maxTotalBypassMinFeeMsgGasUsage uint64) FeeDecorator {
 	if !globalfeeSubspace.HasKeyTable() {
 		panic("global fee paramspace was not set up via module")
 	}
@@ -42,10 +42,10 @@ func NewFeeDecorator(bypassMsgTypes []string, globalfeeSubspace, stakingSubspace
 	}
 
 	return FeeDecorator{
-		BypassMinFeeMsgTypes:       bypassMsgTypes,
-		GlobalMinFee:               globalfeeSubspace,
-		StakingSubspace:            stakingSubspace,
-		MaxBypassMinFeeMsgGasUsage: maxBypassMinFeeMsgGasUsage,
+		BypassMinFeeMsgTypes:            bypassMsgTypes,
+		GlobalMinFee:                    globalfeeSubspace,
+		StakingSubspace:                 stakingSubspace,
+		MaxTotalBypassMinFeeMsgGasUsage: maxTotalBypassMinFeeMsgGasUsage,
 	}
 }
 
@@ -62,12 +62,11 @@ func (mfd FeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, ne
 	// Accept zero fee transactions only if both of the following statements are true:
 	// 	- the tx contains only message types that can bypass the minimum fee,
 	//	see BypassMinFeeMsgTypes;
-	//	- the total gas limit per message does not exceed MaxBypassMinFeeMsgGasUsage,
-	//	i.e., totalGas <= len(msgs) * MaxBypassMinFeeMsgGasUsage
+	//	- 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.
-	containsOnlyBypassMinFeeMsgs := mfd.bypassMinFeeMsgs(msgs)
-	doesNotExceedMaxGasUsage := gas <= uint64(len(msgs))*mfd.MaxBypassMinFeeMsgGasUsage
-	allowedToBypassMinFee := containsOnlyBypassMinFeeMsgs && doesNotExceedMaxGasUsage
+	doesNotExceedMaxGasUsage := gas <= mfd.MaxTotalBypassMinFeeMsgGasUsage
+	allowedToBypassMinFee := mfd.containsOnlyBypassMinFeeMsgs(msgs) && doesNotExceedMaxGasUsage
 
 	var allFees sdk.Coins
 	requiredGlobalFees, err := mfd.getGlobalFee(ctx, feeTx)

diff --git a/x/globalfee/ante/fee_utils.go b/x/globalfee/ante/fee_utils.go
@@ -27,7 +27,7 @@ func getMinGasPrice(ctx sdk.Context, feeTx sdk.FeeTx) sdk.Coins {
 	return requiredFees.Sort()
 }
 
-func (mfd FeeDecorator) bypassMinFeeMsgs(msgs []sdk.Msg) bool {
+func (mfd FeeDecorator) containsOnlyBypassMinFeeMsgs(msgs []sdk.Msg) bool {
 	for _, msg := range msgs {
 		if tmstrings.StringInSlice(sdk.MsgTypeURL(msg), mfd.BypassMinFeeMsgTypes) {
 			continue