Skip to content

Commit

Permalink
Add separate min gas price option for txes with high gas wanted (#777)
Browse files Browse the repository at this point in the history
  • Loading branch information
hleb-albau authored Jan 25, 2022
1 parent 6e7d32b commit f0842b9
Show file tree
Hide file tree
Showing 7 changed files with 142 additions and 81 deletions.
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
}

0 comments on commit f0842b9

Please sign in to comment.