From b26588a5223ab95daa8fec71ec8424446c900ea4 Mon Sep 17 00:00:00 2001 From: Yaru Wang Date: Fri, 17 Feb 2023 13:07:03 +0100 Subject: [PATCH 1/8] refactor: maxBypassMinFeeMsgGasUsage -> maxTotalBypassMinFeeMsgGasUsage --- ante/ante.go | 4 ++-- x/globalfee/ante/fee.go | 24 ++++++++++++------------ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/ante/ante.go b/ante/ante.go index f1e1ce391da..15a097ccd43 100644 --- a/ante/ante.go +++ b/ante/ante.go @@ -50,7 +50,7 @@ func NewAnteHandler(opts HandlerOptions) (sdk.AnteHandler, error) { // so that a transaction that contains only message types that can // bypass the minimum fee 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 @@ -59,7 +59,7 @@ func NewAnteHandler(opts HandlerOptions) (sdk.AnteHandler, error) { ante.NewTxTimeoutHeightDecorator(), ante.NewValidateMemoDecorator(opts.AccountKeeper), ante.NewConsumeGasForTxSizeDecorator(opts.AccountKeeper), - 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/x/globalfee/ante/fee.go b/x/globalfee/ante/fee.go index 650acb2f779..832743b21c4 100644 --- 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,11 +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 + doesNotExceedMaxGasUsage := gas <= mfd.MaxTotalBypassMinFeeMsgGasUsage allowedToBypassMinFee := containsOnlyBypassMinFeeMsgs && doesNotExceedMaxGasUsage var allFees sdk.Coins From 9ace1a45f6b3bec9d8e2836e94d104f8a3e70cc5 Mon Sep 17 00:00:00 2001 From: Yaru Wang Date: Fri, 17 Feb 2023 13:21:11 +0100 Subject: [PATCH 2/8] add more bypass-msg-types to default bypass-msg --- app/app.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/app.go b/app/app.go index 78455444069..dbbbbb4c491 100644 --- a/app/app.go +++ b/app/app.go @@ -234,6 +234,8 @@ func GetDefaultBypassFeeMessages() []string { sdk.MsgTypeURL(&ibcchanneltypes.MsgRecvPacket{}), sdk.MsgTypeURL(&ibcchanneltypes.MsgAcknowledgement{}), sdk.MsgTypeURL(&ibcclienttypes.MsgUpdateClient{}), + sdk.MsgTypeURL(&ibcchanneltypes.MsgTimeout{}), + sdk.MsgTypeURL(&ibcchanneltypes.MsgTimeoutOnClose{}), } } From 34459c959d165c53268922262ab1116b969659be Mon Sep 17 00:00:00 2001 From: Yaru Wang Date: Fri, 17 Feb 2023 13:35:19 +0100 Subject: [PATCH 3/8] docs: update bypass-msg in globalfee.md --- docs/modules/globalfee.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/docs/modules/globalfee.md b/docs/modules/globalfee.md index b8853c0b96b..b69a1fe917d 100644 --- 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 later use `["/ibc.core.channel.v1.MsgRecvPacket", "/ibc.core.channel.v1.MsgAcknowledgement","/ibc.applications.transfer.v1.MsgTransfer"]` as defaults. +- Nodes created using Gaiad `v9.0.1` 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"] ``` From ed407c0572f0b78fc9f234bad9e41bd83315f6ea Mon Sep 17 00:00:00 2001 From: Yaru Wang Date: Fri, 17 Feb 2023 14:27:35 +0100 Subject: [PATCH 4/8] test: add tests for bypass-msg --- x/globalfee/ante/antetest/fee_test.go | 50 ++++++++++++++++++++++++--- 1 file changed, 46 insertions(+), 4 deletions(-) diff --git a/x/globalfee/ante/antetest/fee_test.go b/x/globalfee/ante/antetest/fee_test.go index e951ac0a288..2cb1aeffdca 100644 --- 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 +} From 3654bbf8a03b51a15c7c710a15b04265f3e8da22 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Tue, 21 Feb 2023 14:47:22 +0100 Subject: [PATCH 5/8] - Update doc to follow Go conventions - Improve code readability --- docs/modules/globalfee.md | 5 +- x/globalfee/ante/fee.go | 99 +++++++++++++++++++++-------------- x/globalfee/ante/fee_utils.go | 63 +++++++--------------- 3 files changed, 82 insertions(+), 85 deletions(-) diff --git a/docs/modules/globalfee.md b/docs/modules/globalfee.md index b8853c0b96b..78452c4969b 100644 --- a/docs/modules/globalfee.md +++ b/docs/modules/globalfee.md @@ -200,10 +200,9 @@ Note that the required amount of `uatom` in globalfee is overwritten by the amou ### Case 7 -**Setting:** globalfee=[0.1uatom], minimum-gas-prices=0.2uatom,1stake, gas=200000, bypass-min-fee-msg-types = ["/cosmos.distribution.v1beta1.MsgWithdrawDelegatorReward"] +**Setting:** globalfee=[0.1uatom], minimum-gas-prices=[0.2uatom, 1stake], gas=200000, bypass-min-fee-msg-types = ["/cosmos.distribution.v1beta1.MsgWithdrawDelegatorReward"] -Note that the required amount of `uatom` in globalfee is overwritten by the amount in minimum-gas-prices. -Also, the `1stake` in minimum-gas-prices is ignored. +Note that the required amounts of `uatom` and `stake` in minimum-gas-prices is are ignored. - msg withdraw-all-rewards with paidfee="", `pass` - msg withdraw-all-rewards with paidfee="200000 * 0.05uatom", `pass` diff --git a/x/globalfee/ante/fee.go b/x/globalfee/ante/fee.go index 650acb2f779..a919a726ac0 100644 --- a/x/globalfee/ante/fee.go +++ b/x/globalfee/ante/fee.go @@ -10,9 +10,10 @@ import ( "github.com/cosmos/gaia/v9/x/globalfee/types" "github.com/cosmos/gaia/v9/x/globalfee" + tmstrings "github.com/tendermint/tendermint/libs/strings" ) -// FeeWithBypassDecorator will check if the transaction's fee is at least as large +// FeeWithBypassDecorator checks if the transaction's fee is at least as large // as the local validator's minimum gasFee (defined in validator config) and global fee, and the fee denom should be in the global fees' denoms. // // If fee is too low, decorator returns error and tx is rejected from mempool. @@ -49,59 +50,43 @@ func NewFeeDecorator(bypassMsgTypes []string, globalfeeSubspace, stakingSubspace } } +// AnteHandle implements the AnteDecorator interface func (mfd FeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) { // please note: after parsing feeflag, the zero fees are removed already feeTx, ok := tx.(sdk.FeeTx) if !ok { return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx") } + + // Only check for minimum fees and global fee if the execution mode is CheckTx + if !ctx.IsCheckTx() || simulate { + return next(ctx, tx, simulate) + } + + // sort fee tx's coins feeCoins := feeTx.GetFee().Sort() gas := feeTx.GetGas() msgs := feeTx.GetMsgs() + // Get required Global Fee and min gas price + requiredGlobalFees, err := mfd.getGlobalFee(ctx, feeTx) + if err != nil { + panic(err) + } + requiredFees := getMinGasPrice(ctx, feeTx) + // 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 + // // 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 - - var allFees sdk.Coins - requiredGlobalFees, err := mfd.getGlobalFee(ctx, feeTx) - if err != nil { - panic(err) - } - requiredFees := getMinGasPrice(ctx, feeTx) - - // Only check for minimum fees and global fee if the execution mode is CheckTx - if !ctx.IsCheckTx() || simulate { - return next(ctx, tx, simulate) - } - - if !allowedToBypassMinFee { - // Either the transaction contains at least on message of a type - // that cannot bypass the minimum fee or the total gas limit exceeds - // the imposed threshold. As a result, check that the fees are in - // expected denominations and the amounts are greater or equal than - // the expected amounts. + allowedToBypassMinFee := mfd.containsOnlyBypassMinFeeMsgs(msgs) && doesNotExceedMaxGasUsage - allFees = CombinedFeeRequirement(requiredGlobalFees, requiredFees) - - // Check that the fees are in expected denominations. Note that a zero fee - // is accepted if the global fee has an entry with a zero amount, e.g., 0uatoms. - if !DenomsSubsetOfIncludingZero(feeCoins, allFees) { - return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "fee is not a subset of required fees; got %s, required: %s", feeCoins, allFees) - } - // 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. - if !IsAnyGTEIncludingZero(feeCoins, allFees) { - return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins, allFees) - } - } else { + if allowedToBypassMinFee { // Transactions with zero fees are accepted if len(feeCoins) == 0 { return next(ctx, tx, simulate) @@ -113,10 +98,32 @@ func (mfd FeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, ne } } + // Either the transaction contains at least on message of a type + // that cannot bypass the minimum fee or the total gas limit exceeds + // the imposed threshold. As a result, check that the fees are in + // expected denominations and the amounts are greater or equal than + // the expected amounts. + + allFees := CombinedFeeRequirement(requiredGlobalFees, requiredFees) + + // Check that the fees are in expected denominations. Note that a zero fee + // is accepted if the global fee has an entry with a zero amount, e.g., 0uatoms. + if !DenomsSubsetOfIncludingZero(feeCoins, allFees) { + return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "fee is not a subset of required fees; got %s, required: %s", feeCoins, allFees) + } + // 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. + if !IsAnyGTEIncludingZero(feeCoins, allFees) { + return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins, allFees) + } + return next(ctx, tx, simulate) } -// ParamStoreKeyMinGasPrices type require coins sorted. getGlobalFee will also return sorted coins (might return 0denom if globalMinGasPrice is 0) +// getGlobalFee returns the global fees for a given fee tx's gas (might also returns 0denom if globalMinGasPrice is 0) +// sorted in ascending order. +// Note that ParamStoreKeyMinGasPrices type requires coins sorted. func (mfd FeeDecorator) getGlobalFee(ctx sdk.Context, feeTx sdk.FeeTx) (sdk.Coins, error) { var ( globalMinGasPrices sdk.DecCoins @@ -129,6 +136,9 @@ func (mfd FeeDecorator) getGlobalFee(ctx sdk.Context, feeTx sdk.FeeTx) (sdk.Coin // global fee is empty set, set global fee to 0uatom if len(globalMinGasPrices) == 0 { globalMinGasPrices, err = mfd.DefaultZeroGlobalFee(ctx) + if err != nil { + return sdk.Coins{}, err + } } requiredGlobalFees := make(sdk.Coins, len(globalMinGasPrices)) // Determine the required fees by multiplying each required minimum gas @@ -139,7 +149,7 @@ func (mfd FeeDecorator) getGlobalFee(ctx sdk.Context, feeTx sdk.FeeTx) (sdk.Coin requiredGlobalFees[i] = sdk.NewCoin(gp.Denom, fee.Ceil().RoundInt()) } - return requiredGlobalFees.Sort(), err + return requiredGlobalFees.Sort(), nil } func (mfd FeeDecorator) DefaultZeroGlobalFee(ctx sdk.Context) ([]sdk.DecCoin, error) { @@ -159,3 +169,16 @@ func (mfd FeeDecorator) getBondDenom(ctx sdk.Context) string { return bondDenom } + +// containsOnlyBypassMinFeeMsgs returns true if all the given msgs type are listed +// in the BypassMinFeeMsgTypes of the FeeDecorator. +func (mfd FeeDecorator) containsOnlyBypassMinFeeMsgs(msgs []sdk.Msg) bool { + for _, msg := range msgs { + if tmstrings.StringInSlice(sdk.MsgTypeURL(msg), mfd.BypassMinFeeMsgTypes) { + continue + } + return false + } + + return true +} diff --git a/x/globalfee/ante/fee_utils.go b/x/globalfee/ante/fee_utils.go index c5b353a888b..891b5b8d5b0 100644 --- a/x/globalfee/ante/fee_utils.go +++ b/x/globalfee/ante/fee_utils.go @@ -1,13 +1,11 @@ package ante import ( - "math" - sdk "github.com/cosmos/cosmos-sdk/types" - tmstrings "github.com/tendermint/tendermint/libs/strings" ) -// getMinGasPrice will also return sorted coins +// getMinGasPrice returns the validator's minimum gas prices +// for a given fee tx's gas sorted in ascending order func getMinGasPrice(ctx sdk.Context, feeTx sdk.FeeTx) sdk.Coins { minGasPrices := ctx.MinGasPrices() gas := feeTx.GetGas() @@ -27,22 +25,14 @@ func getMinGasPrice(ctx sdk.Context, feeTx sdk.FeeTx) sdk.Coins { return requiredFees.Sort() } -func (mfd FeeDecorator) bypassMinFeeMsgs(msgs []sdk.Msg) bool { - for _, msg := range msgs { - if tmstrings.StringInSlice(sdk.MsgTypeURL(msg), mfd.BypassMinFeeMsgTypes) { - continue - } - return false - } - - return true -} - -// DenomsSubsetOfIncludingZero and IsAnyGTEIncludingZero are similar to DenomsSubsetOf and IsAnyGTE in sdk. Since we allow zero coins in global fee(zero coins means the chain does not want to set a global fee but still want to define the fee's denom) +// DenomsSubsetOfIncludingZero and IsAnyGTEIncludingZero are similar to DenomsSubsetOf and IsAnyGTE in sdk. +// Since we allow zero coins in global fee(zero coins means the chain does not want to set a global fee but still want to define the fee's denom) // -// overwrite DenomsSubsetOfIncludingZero from sdk, to allow zero amt coins in superset. e.g. 1stake is DenomsSubsetOfIncludingZero 0stake. [] is the DenomsSubsetOfIncludingZero of [0stake] but not [1stake]. +// DenomsSubsetOfIncludingZero overwrites DenomsSubsetOf from sdk, to allow zero amt coins in superset. e.g. +// e.g. [1stake] is the DenomsSubsetOfIncludingZero of [0stake] and +// [] is the DenomsSubsetOfIncludingZero of [0stake] but not [1stake]. // DenomsSubsetOfIncludingZero returns true if coins's denoms is subset of coinsB's denoms. -// if coins is empty set, empty set is any sets' subset +// If coins is empty set, empty set is any sets' subset func DenomsSubsetOfIncludingZero(coins, coinsB sdk.Coins) bool { // more denoms in B than in receiver if len(coins) > len(coinsB) { @@ -67,9 +57,9 @@ func DenomsSubsetOfIncludingZero(coins, coinsB sdk.Coins) bool { return true } -// overwrite the IsAnyGTEIncludingZero from sdk to allow zero coins in coins and coinsB. -// IsAnyGTEIncludingZero returns true if coins contain at least one denom that is present at a greater or equal amount in coinsB; it returns false otherwise. -// if CoinsB is emptyset, no coins sets are IsAnyGTEIncludingZero coinsB unless coins is also empty set. +// IsAnyGTEIncludingZero overwrites the IsAnyGTE from sdk to allow zero coins in coins and coinsB. +// IsAnyGTEIncludingZero returns true if coins contain at least one denom that is present at a greater or equal amount in coinsB; +// it returns false otherwise. If CoinsB is emptyset, no coins sets are IsAnyGTEIncludingZero coinsB unless coins is also empty set. // NOTE: IsAnyGTEIncludingZero operates under the invariant that both coin sets are sorted by denoms. // contract !!!! coins must be DenomsSubsetOfIncludingZero of coinsB func IsAnyGTEIncludingZero(coins, coinsB sdk.Coins) bool { @@ -103,13 +93,13 @@ func IsAnyGTEIncludingZero(coins, coinsB sdk.Coins) bool { return false } -// return true if coinsB is empty or contains zero coins, -// CoinsB must be validate coins !!! -func ContainZeroCoins(coinsB sdk.Coins) bool { - if len(coinsB) == 0 { +// ContainZeroCoins returns true if the given coins is empty or contains zero coins, +// Note that the coins denoms must be validated, see sdk.ValidateDenom +func ContainZeroCoins(coins sdk.Coins) bool { + if len(coins) == 0 { return true } - for _, coin := range coinsB { + for _, coin := range coins { if coin.IsZero() { return true } @@ -118,7 +108,9 @@ func ContainZeroCoins(coinsB sdk.Coins) bool { return false } -// CombinedFeeRequirement will combine the global fee and min_gas_price. Both globalFees and minGasPrices must be valid, but CombinedFeeRequirement does not validate them, so it may return 0denom. +// CombinedFeeRequirement returns the global fee and min_gas_price combined and sorted. +// Both globalFees and minGasPrices must be valid, but CombinedFeeRequirement +// does not validate them, so it may return 0denom. func CombinedFeeRequirement(globalFees, minGasPrices sdk.Coins) sdk.Coins { // empty min_gas_price if len(minGasPrices) == 0 { @@ -144,23 +136,6 @@ func CombinedFeeRequirement(globalFees, minGasPrices sdk.Coins) sdk.Coins { return allFees.Sort() } -// getTxPriority returns a naive tx priority based on the amount of the smallest denomination of the fee -// provided in a transaction. -func GetTxPriority(fee sdk.Coins) int64 { - var priority int64 - for _, c := range fee { - p := int64(math.MaxInt64) - if c.Amount.IsInt64() { - p = c.Amount.Int64() - } - if priority == 0 || p < priority { - priority = p - } - } - - return priority -} - // Find replaces the functionality of Coins.Find from SDK v0.46.x func Find(coins sdk.Coins, denom string) (bool, sdk.Coin) { switch len(coins) { From d9f5e4168c04491b0cc57d36aacb9b657870c0a7 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Wed, 22 Feb 2023 09:58:58 +0100 Subject: [PATCH 6/8] update params --- x/globalfee/module.go | 6 +++- x/globalfee/types/params.go | 60 ++++++++++++-------------------- x/globalfee/types/params_test.go | 12 +++++++ 3 files changed, 39 insertions(+), 39 deletions(-) diff --git a/x/globalfee/module.go b/x/globalfee/module.go index 484bb7778ec..2549e8403ce 100644 --- a/x/globalfee/module.go +++ b/x/globalfee/module.go @@ -58,7 +58,11 @@ func (a AppModuleBasic) RegisterRESTRoutes(context client.Context, router *mux.R } func (a AppModuleBasic) RegisterGRPCGatewayRoutes(clientCtx client.Context, mux *runtime.ServeMux) { - _ = types.RegisterQueryHandlerClient(context.Background(), mux, types.NewQueryClient(clientCtx)) + err := types.RegisterQueryHandlerClient(context.Background(), mux, types.NewQueryClient(clientCtx)) + if err != nil { + // same behavior as in cosmos-sdk + panic(err) + } } func (a AppModuleBasic) GetTxCmd() *cobra.Command { diff --git a/x/globalfee/types/params.go b/x/globalfee/types/params.go index 59aa38cc568..305849ea836 100644 --- a/x/globalfee/types/params.go +++ b/x/globalfee/types/params.go @@ -45,53 +45,37 @@ func validateMinimumGasPrices(i interface{}) error { return dec.Validate() } -// Validate checks that the DecCoins are sorted, have nonnegtive amount, with a valid and unique -// denomination (i.e no duplicates). Otherwise, it returns an error. type DecCoins sdk.DecCoins +// Validate checks that the DecCoins are sorted, have nonnegtive amount, with a valid and unique +// denomination (i.e no duplicates). Otherwise, it returns an error. func (coins DecCoins) Validate() error { - switch len(coins) { - case 0: + if len(coins) == 0 { return nil + } - case 1: - // match the denom reg expr - if err := sdk.ValidateDenom(coins[0].Denom); err != nil { - return err - } - if coins[0].IsNegative() { - return fmt.Errorf("coin %s amount is negtive", coins[0]) + lowDenom := "" + seenDenoms := make(map[string]bool) + + for _, coin := range coins { + if seenDenoms[coin.Denom] { + return fmt.Errorf("duplicate denomination %s", coin.Denom) } - return nil - default: - // check single coin case - if err := (DecCoins{coins[0]}).Validate(); err != nil { + if err := sdk.ValidateDenom(coin.Denom); err != nil { return err } - - lowDenom := coins[0].Denom - seenDenoms := make(map[string]bool) - seenDenoms[lowDenom] = true - - for _, coin := range coins[1:] { - if seenDenoms[coin.Denom] { - return fmt.Errorf("duplicate denomination %s", coin.Denom) - } - if err := sdk.ValidateDenom(coin.Denom); err != nil { - return err - } - if coin.Denom <= lowDenom { - return fmt.Errorf("denomination %s is not sorted", coin.Denom) - } - if coin.IsNegative() { - return fmt.Errorf("coin %s amount is negtive", coin.Denom) - } - - // we compare each coin against the last denom - lowDenom = coin.Denom - seenDenoms[coin.Denom] = true + if coin.Denom <= lowDenom { + return fmt.Errorf("denomination %s is not sorted", coin.Denom) + } + if coin.IsNegative() { + return fmt.Errorf("coin %s amount is negative", coin.Amount) } - return nil + // we compare each coin against the last denom + lowDenom = coin.Denom + seenDenoms[coin.Denom] = true } + + return nil + } diff --git a/x/globalfee/types/params_test.go b/x/globalfee/types/params_test.go index b4e67379f33..ba53a7f1f7f 100644 --- a/x/globalfee/types/params_test.go +++ b/x/globalfee/types/params_test.go @@ -46,6 +46,18 @@ func Test_validateParams(t *testing.T) { }, true, }, + "negative amount, fail": { + sdk.DecCoins{ + sdk.DecCoin{Denom: "photon", Amount: sdk.OneDec().Neg()}, + }, + true, + }, + "invalid denom, fail": { + sdk.DecCoins{ + sdk.DecCoin{Denom: "photon!", Amount: sdk.OneDec().Neg()}, + }, + true, + }, } for name, test := range tests { From 7be1e72ba26d562023e1e139f026c426da6a27f6 Mon Sep 17 00:00:00 2001 From: Yaru Wang Date: Thu, 23 Feb 2023 11:26:42 +0100 Subject: [PATCH 7/8] refactor: containsOnlyBypassMinFeeMsgs --- x/globalfee/ante/fee.go | 3 +-- x/globalfee/ante/fee_utils.go | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/x/globalfee/ante/fee.go b/x/globalfee/ante/fee.go index 832743b21c4..e818e7d398b 100644 --- a/x/globalfee/ante/fee.go +++ b/x/globalfee/ante/fee.go @@ -65,9 +65,8 @@ func (mfd FeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, ne // - 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 <= mfd.MaxTotalBypassMinFeeMsgGasUsage - allowedToBypassMinFee := containsOnlyBypassMinFeeMsgs && doesNotExceedMaxGasUsage + 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 index c5b353a888b..9fa48b01e99 100644 --- 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 From 8477af7d1ea4c3557157c4234fd38d5597c83fec Mon Sep 17 00:00:00 2001 From: Yaru Wang Date: Thu, 23 Feb 2023 12:45:27 +0100 Subject: [PATCH 8/8] docs: update docs --- docs/modules/globalfee.md | 3 ++- x/globalfee/ante/fee.go | 3 --- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/docs/modules/globalfee.md b/docs/modules/globalfee.md index 8d68e06e169..e3027769688 100644 --- a/docs/modules/globalfee.md +++ b/docs/modules/globalfee.md @@ -203,7 +203,8 @@ Note that the required amount of `uatom` in globalfee is overwritten by the amou **Setting:** globalfee=[0.1uatom], minimum-gas-prices=[0.2uatom, 1stake], gas=200000, bypass-min-fee-msg-types = ["/cosmos.distribution.v1beta1.MsgWithdrawDelegatorReward"] -Note that the required amounts of `uatom` and `stake` in minimum-gas-prices is are ignored. +Note that the required amount of `uatom` in globalfee is overwritten by the amount in minimum-gas-prices. +Also, the `1stake` in minimum-gas-prices is ignored. - msg withdraw-all-rewards with paidfee="", `pass` - msg withdraw-all-rewards with paidfee="200000 * 0.05uatom", `pass` diff --git a/x/globalfee/ante/fee.go b/x/globalfee/ante/fee.go index 3a835d8ff44..fd74228ec6b 100644 --- a/x/globalfee/ante/fee.go +++ b/x/globalfee/ante/fee.go @@ -78,9 +78,6 @@ func (mfd FeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, ne // // - 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.