From b2dc5715fc56f5c5ca44398aa589819d9593b9e9 Mon Sep 17 00:00:00 2001 From: Alex Johnson Date: Thu, 6 Jun 2024 11:25:37 -0400 Subject: [PATCH] fix: tx prioritization (#101) * fix * beauty * fix * fix * fix * test * lint * fix --- tests/e2e/consumer.go | 7 ++- x/feemarket/ante/fee.go | 89 +++++++++++++++++++--------- x/feemarket/fuzz/tx_priority_test.go | 52 ++++++++++++++++ x/feemarket/post/fee_test.go | 7 +-- 4 files changed, 121 insertions(+), 34 deletions(-) create mode 100644 x/feemarket/fuzz/tx_priority_test.go diff --git a/tests/e2e/consumer.go b/tests/e2e/consumer.go index 30f5c48..307caf8 100644 --- a/tests/e2e/consumer.go +++ b/tests/e2e/consumer.go @@ -34,8 +34,8 @@ func CCVChainConstructor(t *testing.T, spec *interchaintest.ChainSpec) []*cosmos Version: providerVersion, NumValidators: &providerNumValidators, ChainConfig: ibc.ChainConfig{ - GasPrices: "1uatom", - GasAdjustment: 1.5, + GasPrices: "0uatom", + GasAdjustment: 0.0, ChainID: providerChainID, TrustingPeriod: "336h", ModifyGenesis: cosmos.ModifyGenesis( @@ -43,7 +43,8 @@ func CCVChainConstructor(t *testing.T, spec *interchaintest.ChainSpec) []*cosmos cosmos.NewGenesisKV("app_state.provider.params.blocks_per_epoch", "1"), }, ), - }}, + }, + }, }, ) diff --git a/x/feemarket/ante/fee.go b/x/feemarket/ante/fee.go index b7f460f..aff6034 100644 --- a/x/feemarket/ante/fee.go +++ b/x/feemarket/ante/fee.go @@ -113,23 +113,50 @@ func (dfd feeMarketCheckDecorator) anteHandle(ctx sdk.Context, tx sdk.Tx, simula ctx = ctx.WithMinGasPrices(sdk.NewDecCoins(minGasPrice)) if !simulate { - _, tip, err := CheckTxFee(ctx, minGasPrice, feeCoin, feeGas, true) + _, _, err := CheckTxFee(ctx, minGasPrice, feeCoin, feeGas, true) if err != nil { return ctx, errorsmod.Wrapf(err, "error checking fee") } - ctx = ctx.WithPriority(getTxPriority(tip, int64(gas))) + + priorityFee, err := dfd.resolveTxPriorityCoins(ctx, feeCoin, params.FeeDenom) + if err != nil { + return ctx, errorsmod.Wrapf(err, "error resolving fee priority") + } + + baseGasPrice, err := dfd.feemarketKeeper.GetMinGasPrice(ctx, params.FeeDenom) + if err != nil { + return ctx, err + } + + ctx = ctx.WithPriority(GetTxPriority(priorityFee, int64(gas), baseGasPrice)) return next(ctx, tx, simulate) } return next(ctx, tx, simulate) } +// resolveTxPriorityCoins converts the coins to the proper denom used for tx prioritization calculation. +func (dfd feeMarketCheckDecorator) resolveTxPriorityCoins(ctx sdk.Context, fee sdk.Coin, baseDenom string) (sdk.Coin, error) { + if fee.Denom == baseDenom { + return fee, nil + } + + feeDec := sdk.NewDecCoinFromCoin(fee) + convertedDec, err := dfd.feemarketKeeper.GetDenomResolver().ConvertToDenom(ctx, feeDec, baseDenom) + if err != nil { + return sdk.Coin{}, err + } + + // truncate down + return sdk.NewCoin(baseDenom, convertedDec.Amount.TruncateInt()), nil +} + // CheckTxFee implements the logic for the fee market to check if a Tx has provided sufficient // fees given the current state of the fee market. Returns an error if insufficient fees. -func CheckTxFee(ctx sdk.Context, minGasPrice sdk.DecCoin, feeCoin sdk.Coin, feeGas int64, isAnte bool) (payCoin sdk.Coin, tip sdk.Coin, err error) { +func CheckTxFee(ctx sdk.Context, gasPrice sdk.DecCoin, feeCoin sdk.Coin, feeGas int64, isAnte bool) (payCoin sdk.Coin, tip sdk.Coin, err error) { payCoin = feeCoin // Ensure that the provided fees meet the minimum - if !minGasPrice.IsZero() { + if !gasPrice.IsZero() { var ( requiredFee sdk.Coin consumedFee sdk.Coin @@ -141,29 +168,27 @@ func CheckTxFee(ctx sdk.Context, minGasPrice sdk.DecCoin, feeCoin sdk.Coin, feeG gcDec := sdkmath.LegacyNewDec(gasConsumed) glDec := sdkmath.LegacyNewDec(feeGas) - consumedFeeAmount := minGasPrice.Amount.Mul(gcDec) - limitFee := minGasPrice.Amount.Mul(glDec) - consumedFee = sdk.NewCoin(minGasPrice.Denom, consumedFeeAmount.Ceil().RoundInt()) - requiredFee = sdk.NewCoin(minGasPrice.Denom, limitFee.Ceil().RoundInt()) + consumedFeeAmount := gasPrice.Amount.Mul(gcDec) + limitFee := gasPrice.Amount.Mul(glDec) + + consumedFee = sdk.NewCoin(gasPrice.Denom, consumedFeeAmount.Ceil().RoundInt()) + requiredFee = sdk.NewCoin(gasPrice.Denom, limitFee.Ceil().RoundInt()) if !payCoin.IsGTE(requiredFee) { return sdk.Coin{}, sdk.Coin{}, sdkerrors.ErrInsufficientFee.Wrapf( "got: %s required: %s, minGasPrice: %s, gas: %d", payCoin, requiredFee, - minGasPrice, + gasPrice, gasConsumed, ) } if isAnte { tip = payCoin.Sub(requiredFee) - // set fee coins to be required amount if checking payCoin = requiredFee } else { - // tip is the difference between payCoin and the required fee - tip = payCoin.Sub(requiredFee) - // set fee coin to be ONLY the consumed amount if we are calculated consumed fee to deduct + tip = payCoin.Sub(consumedFee) payCoin = consumedFee } } @@ -171,20 +196,30 @@ func CheckTxFee(ctx sdk.Context, minGasPrice sdk.DecCoin, feeCoin sdk.Coin, feeG return payCoin, tip, nil } -// getTxPriority returns a naive tx priority based on the amount of the smallest denomination of the gas price -// provided in a transaction. -// NOTE: This implementation should be used with a great consideration as it opens potential attack vectors -// where txs with multiple coins could not be prioritized as expected. -func getTxPriority(fee sdk.Coin, gas int64) int64 { - var priority int64 - p := int64(math.MaxInt64) - gasPrice := fee.Amount.QuoRaw(gas) - if gasPrice.IsInt64() { - p = gasPrice.Int64() - } - if p < priority { - priority = p +const ( + // gasPricePrecision is the amount of digit precision to scale the gas prices to. + gasPricePrecision = 6 +) + +// GetTxPriority returns a naive tx priority based on the amount of gas price provided in a transaction. +// +// The fee amount is divided by the gasLimit to calculate "Effective Gas Price". +// This value is then normalized and scaled into an integer, so it can be used as a priority. +// +// effectiveGasPrice = feeAmount / gas limit (denominated in fee per gas) +// normalizedGasPrice = effectiveGasPrice / currentGasPrice (floor is 1. The minimum effective gas price can ever be is current gas price) +// scaledGasPrice = normalizedGasPrice * 10 ^ gasPricePrecision (amount of decimal places in the normalized gas price to consider when converting to int64). +func GetTxPriority(fee sdk.Coin, gasLimit int64, currentGasPrice sdk.DecCoin) int64 { + effectiveGasPrice := fee.Amount.ToLegacyDec().QuoInt64(gasLimit) + normalizedGasPrice := effectiveGasPrice.Quo(currentGasPrice.Amount) + scaledGasPrice := normalizedGasPrice.MulInt64(int64(math.Pow10(gasPricePrecision))) + + // overflow panic protection + if scaledGasPrice.GTE(sdkmath.LegacyNewDec(math.MaxInt64)) { + return math.MaxInt64 + } else if scaledGasPrice.LTE(sdkmath.LegacyOneDec()) { + return 0 } - return priority + return scaledGasPrice.TruncateInt64() } diff --git a/x/feemarket/fuzz/tx_priority_test.go b/x/feemarket/fuzz/tx_priority_test.go new file mode 100644 index 0000000..c30c6ed --- /dev/null +++ b/x/feemarket/fuzz/tx_priority_test.go @@ -0,0 +1,52 @@ +package fuzz_test + +import ( + "math" + "testing" + + sdkmath "cosmossdk.io/math" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/stretchr/testify/require" + "pgregory.net/rapid" + + "github.com/skip-mev/feemarket/x/feemarket/ante" +) + +type input struct { + payFee sdk.Coin + gasLimit int64 + currentGasPrice sdk.DecCoin +} + +// TestGetTxPriority ensures that tx priority is properly bounded +func TestGetTxPriority(t *testing.T) { + rapid.Check(t, func(t *rapid.T) { + inputs := createRandomInput(t) + + priority := ante.GetTxPriority(inputs.payFee, inputs.gasLimit, inputs.currentGasPrice) + require.GreaterOrEqual(t, priority, int64(0)) + require.LessOrEqual(t, priority, int64(math.MaxInt64)) + }) +} + +// CreateRandomInput returns a random inputs to the priority function. +func createRandomInput(t *rapid.T) input { + denom := "skip" + + price := rapid.Int64Range(1, 1_000_000_000).Draw(t, "gas price") + priceDec := sdkmath.LegacyNewDecWithPrec(price, 6) + + gasLimit := rapid.Int64Range(1_000_000, 1_000_000_000_000).Draw(t, "gas limit") + + if priceDec.MulInt64(gasLimit).GTE(sdkmath.LegacyNewDec(math.MaxInt64)) { + t.Fatalf("not supposed to happen") + } + + payFeeAmt := rapid.Int64Range(priceDec.MulInt64(gasLimit).TruncateInt64(), math.MaxInt64).Draw(t, "fee amount") + + return input{ + payFee: sdk.NewCoin(denom, sdkmath.NewInt(payFeeAmt)), + gasLimit: gasLimit, + currentGasPrice: sdk.NewDecCoinFromDec(denom, priceDec), + } +} diff --git a/x/feemarket/post/fee_test.go b/x/feemarket/post/fee_test.go index 4e89573..48dc741 100644 --- a/x/feemarket/post/fee_test.go +++ b/x/feemarket/post/fee_test.go @@ -5,14 +5,12 @@ import ( "testing" "cosmossdk.io/math" - "github.com/cosmos/cosmos-sdk/testutil/testdata" + sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" "github.com/stretchr/testify/mock" - sdk "github.com/cosmos/cosmos-sdk/types" - antesuite "github.com/skip-mev/feemarket/x/feemarket/ante/suite" "github.com/skip-mev/feemarket/x/feemarket/post" "github.com/skip-mev/feemarket/x/feemarket/types" @@ -154,7 +152,8 @@ func TestPostHandle(t *testing.T) { resolvableDenom = "atom" ) - gasLimit := antesuite.NewTestGasLimit() + // exact cost of transaction + gasLimit := uint64(27284) validFeeAmount := types.DefaultMinBaseGasPrice.MulInt64(int64(gasLimit)) validFeeAmountWithTip := validFeeAmount.Add(math.LegacyNewDec(100)) validFee := sdk.NewCoins(sdk.NewCoin(baseDenom, validFeeAmount.TruncateInt()))