From 24f99764821570f2197612ab6007fc689b101fab Mon Sep 17 00:00:00 2001 From: Hleb Albau Date: Sat, 22 Jan 2022 14:33:12 +0100 Subject: [PATCH] Add separate min gas price option for txes with high gas wanted --- CHANGELOG.md | 1 + app/ante.go | 64 +--------------------- cmd/osmosisd/cmd/root.go | 8 +++ x/txfees/README.md | 2 + x/txfees/keeper/feedecorator.go | 39 +++++++------- x/txfees/keeper/feedecorator_test.go | 29 +++++++++- x/txfees/types/options.go | 80 ++++++++++++++++++++++++++++ 7 files changed, 142 insertions(+), 81 deletions(-) create mode 100644 x/txfees/types/options.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 8008b32b837..f22306ba14e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Features +- [#724](https://github.com/osmosis-labs/osmosis/pull/724) Make an ante-handler filter for recognizing High gas txs, and having a min gas price for them. - [#741](https://github.com/osmosis-labs/osmosis/pull/741) Allow node operators to set a second min gas price for arbitrage txs. - [#623](https://github.com/osmosis-labs/osmosis/pull/623) Use gosec for staticly linting for common non-determinism issues in SDK applications. diff --git a/app/ante.go b/app/ante.go index 553e1d9b44b..3e1fdc6f588 100644 --- a/app/ante.go +++ b/app/ante.go @@ -1,11 +1,8 @@ package app import ( - "fmt" - servertypes "github.com/cosmos/cosmos-sdk/server/types" sdk "github.com/cosmos/cosmos-sdk/types" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/x/auth/signing" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" @@ -28,14 +25,11 @@ func NewAnteHandler( signModeHandler signing.SignModeHandler, channelKeeper channelkeeper.Keeper, ) sdk.AnteHandler { - mempoolFeeDecorator := txfeeskeeper.NewMempoolFeeDecorator(*txFeesKeeper) - // Optional if anyone else is using this repo. Primarily of impact for Osmosis. - // TODO: Abstract this better - mempoolFeeDecorator.SetArbMinGasFee(parseArbGasFromConfig(appOpts)) + mempoolFeeOptions := txfeestypes.NewMempoolFeeOptions(appOpts) + mempoolFeeDecorator := txfeeskeeper.NewMempoolFeeDecorator(*txFeesKeeper, mempoolFeeOptions) return sdk.ChainAnteDecorators( ante.NewSetUpContextDecorator(), // outermost AnteDecorator. SetUpContext must be called first ante.NewRejectExtensionOptionsDecorator(), - NewMempoolMaxGasPerTxDecorator(), // Use Mempool Fee Decorator from our txfees module instead of default one from auth // https://github.com/cosmos/cosmos-sdk/blob/master/x/auth/middleware/fee.go#L34 mempoolFeeDecorator, @@ -52,57 +46,3 @@ func NewAnteHandler( ibcante.NewAnteDecorator(channelKeeper), ) } - -// TODO: Abstract this function better. We likely need a parse `osmosis-mempool` config section. -func parseArbGasFromConfig(appOpts servertypes.AppOptions) sdk.Dec { - arbMinFeeInterface := appOpts.Get("osmosis-mempool.arbitrage-min-gas-fee") - arbMinFee := txfeeskeeper.DefaultArbMinGasFee - if arbMinFeeInterface != nil { - arbMinFeeStr, ok := arbMinFeeInterface.(string) - if !ok { - panic("invalidly configured osmosis-mempool.arbitrage-min-gas-fee") - } - var err error - // pre-pend 0 to allow the config to start with a decimal, e.g. ".01" - arbMinFee, err = sdk.NewDecFromStr("0" + arbMinFeeStr) - if err != nil { - panic(fmt.Errorf("invalidly configured osmosis-mempool.arbitrage-min-gas-fee, err= %v", err)) - } - } - return arbMinFee -} - -// NewMempoolMaxGasPerTxDecorator will check if the transaction's gas -// is greater than the local validator's max_gas_wanted_per_tx. -// TODO: max_gas_per_tx is hardcoded here, should move to being defined in app.toml. -// If gas_wanted is too high, decorator returns error and tx is rejected from mempool. -// Note this only applies when ctx.CheckTx = true -// If gas is sufficiently low or not CheckTx, then call next AnteHandler -// CONTRACT: Tx must implement FeeTx to use MempoolMaxGasPerTxDecorator -type MempoolMaxGasPerTxDecorator struct{} - -func NewMempoolMaxGasPerTxDecorator() MempoolMaxGasPerTxDecorator { - return MempoolMaxGasPerTxDecorator{} -} - -func (mgd MempoolMaxGasPerTxDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) { - feeTx, ok := tx.(sdk.FeeTx) - if !ok { - return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx") - } - - gas := feeTx.GetGas() - // maximum gas wanted per tx set to 25M - max_gas_wanted_per_tx := uint64(25000000) - - // Ensure that the provided gas is less than the maximum gas per tx. - // if this is a CheckTx. This is only for local mempool purposes, and thus - // is only ran on check tx. - if ctx.IsCheckTx() && !simulate { - if gas > max_gas_wanted_per_tx { - return ctx, sdkerrors.Wrapf(sdkerrors.ErrOutOfGas, "Too much gas wanted: %d, maximum is 25,000,000", gas) - } - } - - return next(ctx, tx, simulate) -} diff --git a/cmd/osmosisd/cmd/root.go b/cmd/osmosisd/cmd/root.go index ab303a4ad39..f9784cff15d 100644 --- a/cmd/osmosisd/cmd/root.go +++ b/cmd/osmosisd/cmd/root.go @@ -110,9 +110,17 @@ func initAppConfig() (string, interface{}) { ############################################################################### [osmosis-mempool] +# This is the max allowed gas any tx. +# This is only for local mempool purposes, and thus is only ran on check tx. +max-gas-wanted-per-tx = "25000000" + # This is the minimum gas fee any arbitrage tx should have, denominated in uosmo per gas # Default value of ".005" then means that a tx with 1 million gas costs (.005 uosmo/gas) * 1_000_000 gas = .005 osmo arbitrage-min-gas-fee = ".005" + +# This is the minimum gas fee any tx with high gas demand should have, denominated in uosmo per gas +# Default value of ".0025" then means that a tx with 1 million gas costs (.0025 uosmo/gas) * 1_000_000 gas = .0025 osmo +min-gas-price-for-high-gas-tx = ".0025" ` return OsmosisAppTemplate, OsmosisAppCfg diff --git a/x/txfees/README.md b/x/txfees/README.md index b5fb27783d5..349855a95ee 100644 --- a/x/txfees/README.md +++ b/x/txfees/README.md @@ -28,6 +28,8 @@ Currently the only supported metadata & spot price calculator is using a GAMM po * Contains both JoinPool and ExitPool messages in one tx. * Has some false positives. * These false positives seem like they primarily will get hit during batching of many distinct operations, not really in one atomic action. +* A max wanted gas per any tx can be set to filter out attack txes. +* If tx wanted gas > than predefined threshold of 1M, then separate 'min-gas-price-for-high-gas-tx' option used to calculate min gas price. ## New SDK messages diff --git a/x/txfees/keeper/feedecorator.go b/x/txfees/keeper/feedecorator.go index 50ed362b08a..1cfe2d8231b 100644 --- a/x/txfees/keeper/feedecorator.go +++ b/x/txfees/keeper/feedecorator.go @@ -8,11 +8,6 @@ import ( "github.com/osmosis-labs/osmosis/x/txfees/types" ) -// DefaultArbMinGasFee if its not set in a config somewhere. -// currently 0 uosmo/gas to preserve functionality with old node software -// TODO: Bump after next minor version. (in 6.2+) -var DefaultArbMinGasFee sdk.Dec = sdk.ZeroDec() - // MempoolFeeDecorator will check if the transaction's fee is at least as large // as the local validator's minimum gasFee (defined in validator config). // If fee is too low, decorator returns error and tx is rejected from mempool. @@ -20,21 +15,17 @@ var DefaultArbMinGasFee sdk.Dec = sdk.ZeroDec() // If fee is high enough or not CheckTx, then call next AnteHandler // CONTRACT: Tx must implement FeeTx to use MempoolFeeDecorator type MempoolFeeDecorator struct { - TxFeesKeeper Keeper - ConfigArbMinGasFee sdk.Dec + TxFeesKeeper Keeper + Opts types.MempoolFeeOptions } -func NewMempoolFeeDecorator(txFeesKeeper Keeper) MempoolFeeDecorator { +func NewMempoolFeeDecorator(txFeesKeeper Keeper, opts types.MempoolFeeOptions) MempoolFeeDecorator { return MempoolFeeDecorator{ - TxFeesKeeper: txFeesKeeper, - ConfigArbMinGasFee: DefaultArbMinGasFee.Clone(), + TxFeesKeeper: txFeesKeeper, + Opts: opts, } } -func (mfd *MempoolFeeDecorator) SetArbMinGasFee(dec sdk.Dec) { - mfd.ConfigArbMinGasFee = dec -} - func (mfd MempoolFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) { // The SDK currently requires all txs to be FeeTx's in CheckTx, within its mempool fee decorator. // See: https://github.com/cosmos/cosmos-sdk/blob/f726a2398a26bdaf71d78dbf56a82621e84fd098/x/auth/middleware/fee.go#L34-L37 @@ -44,6 +35,16 @@ func (mfd MempoolFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate b return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx") } + // Ensure that the provided gas is less than the maximum gas per tx, + // if this is a CheckTx. This is only for local mempool purposes, and thus + // is only ran on check tx. + if ctx.IsCheckTx() && !simulate { + if feeTx.GetGas() > mfd.Opts.MaxGasWantedPerTx { + msg := "Too much gas wanted: %d, maximum is %d" + return ctx, sdkerrors.Wrapf(sdkerrors.ErrOutOfGas, msg, feeTx.GetGas(), mfd.Opts.MaxGasWantedPerTx) + } + } + feeCoins := feeTx.GetFee() if len(feeCoins) > 1 { @@ -71,7 +72,7 @@ func (mfd MempoolFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate b // So we ensure that the provided fees meet a minimum threshold for the validator, // converting every non-osmo specified asset into an osmo-equivalent amount, to determine sufficiency. if (ctx.IsCheckTx() || ctx.IsReCheckTx()) && !simulate { - minBaseGasPrice := mfd.GetMinBaseGasPriceForTx(ctx, baseDenom, tx) + minBaseGasPrice := mfd.GetMinBaseGasPriceForTx(ctx, baseDenom, feeTx) if !(minBaseGasPrice.IsZero()) { if len(feeCoins) != 1 { return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "no fee attached") @@ -108,11 +109,13 @@ func (k Keeper) IsSufficientFee(ctx sdk.Context, minBaseGasPrice sdk.Dec, gasReq return nil } -func (mfd MempoolFeeDecorator) GetMinBaseGasPriceForTx(ctx sdk.Context, baseDenom string, tx sdk.Tx) sdk.Dec { +func (mfd MempoolFeeDecorator) GetMinBaseGasPriceForTx(ctx sdk.Context, baseDenom string, tx sdk.FeeTx) sdk.Dec { cfgMinGasPrice := ctx.MinGasPrices().AmountOf(baseDenom) - + if tx.GetGas() >= mfd.Opts.HighGasTxThreshold { + cfgMinGasPrice = sdk.MaxDec(cfgMinGasPrice, mfd.Opts.MinGasPriceForHighGasTx) + } if txfee_filters.IsArbTxLoose(tx) { - return sdk.MaxDec(cfgMinGasPrice, mfd.ConfigArbMinGasFee) + cfgMinGasPrice = sdk.MaxDec(cfgMinGasPrice, mfd.Opts.MinGasPriceForArbitrageTx) } return cfgMinGasPrice } diff --git a/x/txfees/keeper/feedecorator_test.go b/x/txfees/keeper/feedecorator_test.go index 895f1635094..84669934fb2 100644 --- a/x/txfees/keeper/feedecorator_test.go +++ b/x/txfees/keeper/feedecorator_test.go @@ -2,6 +2,7 @@ package keeper_test import ( sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/osmosis-labs/osmosis/x/txfees/types" "github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx" @@ -11,6 +12,8 @@ import ( func (suite *KeeperTestSuite) TestFeeDecorator() { suite.SetupTest(false) + mempoolFeeOpts := types.NewDefaultMempoolFeeOptions() + mempoolFeeOpts.MinGasPriceForHighGasTx = sdk.MustNewDecFromStr("0.0025") baseDenom, _ := suite.app.TxFeesKeeper.GetBaseDenom(suite.ctx) uion := "uion" @@ -131,6 +134,30 @@ func (suite *KeeperTestSuite) TestFeeDecorator() { isCheckTx: true, expectPass: true, }, + { + name: "tx with gas wanted more than allowed should not pass", + txFee: sdk.NewCoins(sdk.NewInt64Coin(uion, 100000000)), + minGasPrices: sdk.NewDecCoins(sdk.NewInt64DecCoin(uion, 1)), + gasRequested: mempoolFeeOpts.MaxGasWantedPerTx + 1, + isCheckTx: true, + expectPass: false, + }, + { + name: "tx with high gas and not enough fee should no pass", + txFee: sdk.NewCoins(sdk.NewInt64Coin(uion, 1)), + minGasPrices: sdk.NewDecCoins(sdk.NewInt64DecCoin(uion, 1)), + gasRequested: mempoolFeeOpts.HighGasTxThreshold, + isCheckTx: true, + expectPass: false, + }, + { + name: "tx with high gas and enough fee should pass", + txFee: sdk.NewCoins(sdk.NewInt64Coin(uion, 10*1000)), + minGasPrices: sdk.NewDecCoins(sdk.NewInt64DecCoin(uion, 1)), + gasRequested: mempoolFeeOpts.HighGasTxThreshold, + isCheckTx: true, + expectPass: true, + }, } for _, tc := range tests { @@ -143,7 +170,7 @@ func (suite *KeeperTestSuite) TestFeeDecorator() { tc.txFee, ), []legacytx.StdSignature{}, "") - mfd := keeper.NewMempoolFeeDecorator(*suite.app.TxFeesKeeper) + mfd := keeper.NewMempoolFeeDecorator(*suite.app.TxFeesKeeper, mempoolFeeOpts) antehandler := sdk.ChainAnteDecorators(mfd) _, err := antehandler(suite.ctx, tx, false) if tc.expectPass { diff --git a/x/txfees/types/options.go b/x/txfees/types/options.go new file mode 100644 index 00000000000..a7922ef3407 --- /dev/null +++ b/x/txfees/types/options.go @@ -0,0 +1,80 @@ +package types + +import ( + "fmt" + servertypes "github.com/cosmos/cosmos-sdk/server/types" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/spf13/cast" +) + +// If Options are not set in a config somewhere, +// use defaults to preserve functionality with old node software + +// TODO: Bump after next minor version. (in 6.2+) +var DefaultMinGasPriceForArbitrageTx = sdk.ZeroDec() +var DefaultMinGasPriceForHighGasTx = sdk.ZeroDec() +var DefaultMaxGasWantedPerTx = uint64(25 * 1000 * 1000) +var DefaultHighGasTxThreshold = uint64(1 * 1000 * 1000) + +type MempoolFeeOptions struct { + MaxGasWantedPerTx uint64 + MinGasPriceForArbitrageTx sdk.Dec + HighGasTxThreshold uint64 + MinGasPriceForHighGasTx sdk.Dec +} + +func NewDefaultMempoolFeeOptions() MempoolFeeOptions { + return MempoolFeeOptions{ + MaxGasWantedPerTx: DefaultMaxGasWantedPerTx, + MinGasPriceForArbitrageTx: DefaultMinGasPriceForArbitrageTx.Clone(), + HighGasTxThreshold: DefaultHighGasTxThreshold, + MinGasPriceForHighGasTx: DefaultMinGasPriceForHighGasTx.Clone(), + } +} + +func NewMempoolFeeOptions(opts servertypes.AppOptions) MempoolFeeOptions { + return MempoolFeeOptions{ + MaxGasWantedPerTx: parseMaxGasWantedPerTx(opts), + MinGasPriceForArbitrageTx: parseMinGasPriceForArbitrageTx(opts), + HighGasTxThreshold: DefaultHighGasTxThreshold, + MinGasPriceForHighGasTx: parseMinGasPriceForHighGasTx(opts), + } +} + +func parseMaxGasWantedPerTx(opts servertypes.AppOptions) uint64 { + valueInterface := opts.Get("osmosis-mempool.max-gas-wanted-per-tx") + if valueInterface == nil { + return DefaultMaxGasWantedPerTx + } + value, err := cast.ToUint64E(valueInterface) + if err != nil { + panic("invalidly configured osmosis-mempool.max-gas-wanted-per-tx") + } + return value +} + +func parseMinGasPriceForArbitrageTx(opts servertypes.AppOptions) sdk.Dec { + return parseDecFromConfig(opts, "arbitrage-min-gas-fee", DefaultMinGasPriceForArbitrageTx.Clone()) +} + +func parseMinGasPriceForHighGasTx(opts servertypes.AppOptions) sdk.Dec { + return parseDecFromConfig(opts, "min-gas-price-for-high-gas-tx", DefaultMinGasPriceForHighGasTx.Clone()) +} + +func parseDecFromConfig(opts servertypes.AppOptions, optName string, defaultValue sdk.Dec) sdk.Dec { + valueInterface := opts.Get("osmosis-mempool." + optName) + value := defaultValue + if valueInterface != nil { + valueStr, ok := valueInterface.(string) + if !ok { + panic("invalidly configured osmosis-mempool." + optName) + } + var err error + // pre-pend 0 to allow the config to start with a decimal, e.g. ".01" + value, err = sdk.NewDecFromStr("0" + valueStr) + if err != nil { + panic(fmt.Errorf("invalidly configured osmosis-mempool.%v, err= %v", optName, err)) + } + } + return value +}