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.

Sign up for GitHub

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

refactor: bypass-msgs #2218

Merged
merged 18 commits into from
Mar 17, 2023
Merged

refactor: bypass-msgs #2218

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
b26588a
refactor: maxBypassMinFeeMsgGasUsage -> maxTotalBypassMinFeeMsgGasUsage
Feb 17, 2023
9ace1a4
add more bypass-msg-types to default bypass-msg
Feb 17, 2023
34459c9
docs: update bypass-msg in globalfee.md
Feb 17, 2023
ed407c0
test: add tests for bypass-msg
Feb 17, 2023
7be1e72
refactor: containsOnlyBypassMinFeeMsgs
Feb 23, 2023
b5ccab3
merge main
Mar 15, 2023
3d0b902
docs: update changelog
Mar 16, 2023
82a7274
Apply suggestions from code review
yaruwangway Mar 16, 2023
21ff8b1
docs: update comments on bypass-min-fee-msgs
Mar 16, 2023
508f1a4
Update docs/modules/globalfee.md
yaruwangway Mar 16, 2023
f9964c3
merge main
Mar 16, 2023
7b04c97
workflow: increase upgrade test timeout
Mar 17, 2023
f708f6e
workflow: increase upgrade test timeout
Mar 17, 2023
52ac3fc
workflow: use latest cosmovisor in upgrade test
Mar 17, 2023
9bf484b
fix typo
Mar 17, 2023
480b2fb
fix: upgrade test
Mar 17, 2023
8ed9ab7
fix: cosmovisor start in upgrade test
Mar 17, 2023
4c991c3
change timeout for upgrade test in workflow
Mar 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ante/ante.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
yaruwangway marked this conversation as resolved.
Show resolved Hide resolved

anteDecorators := []sdk.AnteDecorator{
ante.NewSetUpContextDecorator(), // outermost AnteDecorator. SetUpContext must be called first
Expand All @@ -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),
yaruwangway marked this conversation as resolved.
Show resolved Hide resolved

ante.NewDeductFeeDecorator(opts.AccountKeeper, opts.BankKeeper, opts.FeegrantKeeper),
ante.NewSetPubKeyDecorator(opts.AccountKeeper), // SetPubKeyDecorator must be called before all signature verification decorators
Expand Down
2 changes: 2 additions & 0 deletions app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}),
}
}

Expand Down
11 changes: 6 additions & 5 deletions docs/modules/globalfee.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
yaruwangway marked this conversation as resolved.
Show resolved Hide resolved
- 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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version need to be updated, it will not be included in v9.0.1. This PR is consensus breaking.

yaruwangway marked this conversation as resolved.
Show resolved Hide resolved
- 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`:

Expand All @@ -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"]
```


Expand Down
50 changes: 46 additions & 4 deletions x/globalfee/ante/antetest/fee_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,8 +468,7 @@ func (s *IntegrationTestSuite) TestGlobalFeeMinimumGasFeeAnteHandler() {
txCheck: true,
yaruwangway marked this conversation as resolved.
Show resolved Hide resolved
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())),
Expand All @@ -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,
Expand Down Expand Up @@ -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)
Expand All @@ -597,3 +635,7 @@ func (s *IntegrationTestSuite) TestGlobalFeeMinimumGasFeeAnteHandler() {
func newTestGasLimit() uint64 {
return 200000
}

func newTestMaxTotalBypassMinFeeMsgGasUsage() uint64 {
return 1000000
}
24 changes: 12 additions & 12 deletions x/globalfee/ante/fee.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand All @@ -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,
}
}

Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alexanderbez , can you please help confirm here, it is ok to refactor uint64(len(msgs))*mfd.MaxBypassMinFeeMsgGasUsage to MaxTotalBypassMinFeeMsgGasUsage ?
without upgrade

MaxBypassMinFeeMsgGasUsage is 200_000,
MaxTotalBypassMinFeeMsgGasUsage is 1_000_000

Copy link
Contributor

@alexanderbez alexanderbez Feb 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you cannot change gas consumed in a soft upgrade (i.e. a point release). If a node is replaying or catching up, it will yield in a consensus fault (e.g. you said gas was X but it's Y for me)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if MaxTotalBypassMinFeeMsgGasUsage isn't checked in DeliverTx ?

gaia/x/globalfee/ante/fee.go

Lines 79 to 84 in 44d5e05

// 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 {


var allFees sdk.Coins
Expand Down