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.

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

Add separate min gas price option for txes with high gas wanted #777

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
64 changes: 2 additions & 62 deletions app/ante.go
Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -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,
Expand All @@ -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)
}
8 changes: 8 additions & 0 deletions cmd/osmosisd/cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions x/txfees/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
39 changes: 21 additions & 18 deletions x/txfees/keeper/feedecorator.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,33 +8,24 @@ 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.
// Note this only applies when ctx.CheckTx = true
// 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
Expand All @@ -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 {
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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
}
29 changes: 28 additions & 1 deletion x/txfees/keeper/feedecorator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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"
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
80 changes: 80 additions & 0 deletions x/txfees/types/options.go
Original file line number Diff line number Diff line change
@@ -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
}