From 0dd2130f72a23a0c2aff0769bcd3cdd2d23d3552 Mon Sep 17 00:00:00 2001 From: aljo242 Date: Tue, 4 Jun 2024 18:36:28 -0400 Subject: [PATCH 1/8] fix --- tests/e2e/consumer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/consumer.go b/tests/e2e/consumer.go index 30f5c48..b784c50 100644 --- a/tests/e2e/consumer.go +++ b/tests/e2e/consumer.go @@ -35,7 +35,7 @@ func CCVChainConstructor(t *testing.T, spec *interchaintest.ChainSpec) []*cosmos NumValidators: &providerNumValidators, ChainConfig: ibc.ChainConfig{ GasPrices: "1uatom", - GasAdjustment: 1.5, + GasAdjustment: 0.0, ChainID: providerChainID, TrustingPeriod: "336h", ModifyGenesis: cosmos.ModifyGenesis( From 4738acae5456943a2471ce87c23df0141b3b8ea9 Mon Sep 17 00:00:00 2001 From: aljo242 Date: Wed, 5 Jun 2024 11:49:37 -0400 Subject: [PATCH 2/8] beauty --- x/feemarket/ante/fee.go | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/x/feemarket/ante/fee.go b/x/feemarket/ante/fee.go index b7f460f..cb6e143 100644 --- a/x/feemarket/ante/fee.go +++ b/x/feemarket/ante/fee.go @@ -113,16 +113,38 @@ 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) + fee, tip, 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, fee.Add(tip), params.FeeDenom) + if err != nil { + return ctx, errorsmod.Wrapf(err, "error resolving fee priority") + } + + ctx = ctx.WithPriority(getTxPriority(priorityFee, int64(gas))) 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) { @@ -171,10 +193,7 @@ 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. +// getTxPriority returns a naive tx priority based on the amount of gas price provided in a transaction. func getTxPriority(fee sdk.Coin, gas int64) int64 { var priority int64 p := int64(math.MaxInt64) From d69b4be5abfab41bd13e5a9b2cfc50c57353bd44 Mon Sep 17 00:00:00 2001 From: aljo242 Date: Wed, 5 Jun 2024 14:14:52 -0400 Subject: [PATCH 3/8] fix --- tests/e2e/consumer.go | 2 +- x/feemarket/ante/fee.go | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/e2e/consumer.go b/tests/e2e/consumer.go index b784c50..b4de702 100644 --- a/tests/e2e/consumer.go +++ b/tests/e2e/consumer.go @@ -34,7 +34,7 @@ func CCVChainConstructor(t *testing.T, spec *interchaintest.ChainSpec) []*cosmos Version: providerVersion, NumValidators: &providerNumValidators, ChainConfig: ibc.ChainConfig{ - GasPrices: "1uatom", + GasPrices: "0uatom", GasAdjustment: 0.0, ChainID: providerChainID, TrustingPeriod: "336h", diff --git a/x/feemarket/ante/fee.go b/x/feemarket/ante/fee.go index cb6e143..71637ca 100644 --- a/x/feemarket/ante/fee.go +++ b/x/feemarket/ante/fee.go @@ -165,6 +165,7 @@ func CheckTxFee(ctx sdk.Context, minGasPrice sdk.DecCoin, feeCoin sdk.Coin, feeG 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()) @@ -180,12 +181,9 @@ func CheckTxFee(ctx sdk.Context, minGasPrice sdk.DecCoin, feeCoin sdk.Coin, feeG 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 } } From 422c26bafe10c0185b109ec02a5cecd8bde77385 Mon Sep 17 00:00:00 2001 From: aljo242 Date: Wed, 5 Jun 2024 14:27:27 -0400 Subject: [PATCH 4/8] fix --- x/feemarket/post/fee_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) 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())) From 74b25665bc50faf92af2501b6f84058263da3249 Mon Sep 17 00:00:00 2001 From: aljo242 Date: Wed, 5 Jun 2024 16:14:01 -0400 Subject: [PATCH 5/8] fix --- tests/e2e/consumer.go | 3 +- x/feemarket/ante/fee.go | 56 +++++++++++++++++++--------- x/feemarket/fuzz/tx_priority_test.go | 47 +++++++++++++++++++++++ 3 files changed, 87 insertions(+), 19 deletions(-) create mode 100644 x/feemarket/fuzz/tx_priority_test.go diff --git a/tests/e2e/consumer.go b/tests/e2e/consumer.go index b4de702..307caf8 100644 --- a/tests/e2e/consumer.go +++ b/tests/e2e/consumer.go @@ -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 71637ca..057f4f3 100644 --- a/x/feemarket/ante/fee.go +++ b/x/feemarket/ante/fee.go @@ -123,7 +123,12 @@ func (dfd feeMarketCheckDecorator) anteHandle(ctx sdk.Context, tx sdk.Tx, simula return ctx, errorsmod.Wrapf(err, "error resolving fee priority") } - ctx = ctx.WithPriority(getTxPriority(priorityFee, int64(gas))) + 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) @@ -147,11 +152,11 @@ func (dfd feeMarketCheckDecorator) resolveTxPriorityCoins(ctx sdk.Context, fee s // 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 @@ -163,18 +168,18 @@ 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) + consumedFeeAmount := gasPrice.Amount.Mul(gcDec) + limitFee := gasPrice.Amount.Mul(glDec) - consumedFee = sdk.NewCoin(minGasPrice.Denom, consumedFeeAmount.Ceil().RoundInt()) - requiredFee = sdk.NewCoin(minGasPrice.Denom, limitFee.Ceil().RoundInt()) + 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, ) } @@ -191,17 +196,32 @@ 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 gas price provided in a transaction. -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() +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 } - if p < priority { - priority = p + + 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..fef97cd --- /dev/null +++ b/x/feemarket/fuzz/tx_priority_test.go @@ -0,0 +1,47 @@ +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, 0) + require.LessOrEqual(t, priority, math.MaxInt64) + }) +} + +// CreateRandomInput returns a random inputs to the priority function. +func CreateRandomInput(t *rapid.T) input { + denom := "skip" + + price := rapid.Int64Range(1, math.MaxInt64).Draw(t, "gas price") + gasLimit := rapid.Int64Range(1, math.MaxInt64).Draw(t, "gas limit") + priceDec := sdkmath.LegacyNewDecWithPrec(price, 1000) + + 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), + } +} From 3e051c8ca6235167beb0dacc9ad2c5c38189ef26 Mon Sep 17 00:00:00 2001 From: aljo242 Date: Wed, 5 Jun 2024 16:57:37 -0400 Subject: [PATCH 6/8] test --- x/feemarket/fuzz/tx_priority_test.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/x/feemarket/fuzz/tx_priority_test.go b/x/feemarket/fuzz/tx_priority_test.go index fef97cd..717b419 100644 --- a/x/feemarket/fuzz/tx_priority_test.go +++ b/x/feemarket/fuzz/tx_priority_test.go @@ -24,8 +24,8 @@ func TestGetTxPriority(t *testing.T) { inputs := CreateRandomInput(t) priority := ante.GetTxPriority(inputs.payFee, inputs.gasLimit, inputs.currentGasPrice) - require.GreaterOrEqual(t, priority, 0) - require.LessOrEqual(t, priority, math.MaxInt64) + require.GreaterOrEqual(t, priority, int64(0)) + require.LessOrEqual(t, priority, int64(math.MaxInt64)) }) } @@ -33,9 +33,14 @@ func TestGetTxPriority(t *testing.T) { func CreateRandomInput(t *rapid.T) input { denom := "skip" - price := rapid.Int64Range(1, math.MaxInt64).Draw(t, "gas price") - gasLimit := rapid.Int64Range(1, math.MaxInt64).Draw(t, "gas limit") - priceDec := sdkmath.LegacyNewDecWithPrec(price, 1000) + 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") From f0ce8cbc3a6de0b994dd5fb85d40b674dfdb9a47 Mon Sep 17 00:00:00 2001 From: aljo242 Date: Wed, 5 Jun 2024 17:02:13 -0400 Subject: [PATCH 7/8] lint --- x/feemarket/fuzz/tx_priority_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x/feemarket/fuzz/tx_priority_test.go b/x/feemarket/fuzz/tx_priority_test.go index 717b419..c30c6ed 100644 --- a/x/feemarket/fuzz/tx_priority_test.go +++ b/x/feemarket/fuzz/tx_priority_test.go @@ -21,7 +21,7 @@ type input struct { // TestGetTxPriority ensures that tx priority is properly bounded func TestGetTxPriority(t *testing.T) { rapid.Check(t, func(t *rapid.T) { - inputs := CreateRandomInput(t) + inputs := createRandomInput(t) priority := ante.GetTxPriority(inputs.payFee, inputs.gasLimit, inputs.currentGasPrice) require.GreaterOrEqual(t, priority, int64(0)) @@ -30,7 +30,7 @@ func TestGetTxPriority(t *testing.T) { } // CreateRandomInput returns a random inputs to the priority function. -func CreateRandomInput(t *rapid.T) input { +func createRandomInput(t *rapid.T) input { denom := "skip" price := rapid.Int64Range(1, 1_000_000_000).Draw(t, "gas price") From 5cc2cad0c3bceeddf170a7ea2236e97a0041cadb Mon Sep 17 00:00:00 2001 From: aljo242 Date: Wed, 5 Jun 2024 17:07:59 -0400 Subject: [PATCH 8/8] fix --- x/feemarket/ante/fee.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/x/feemarket/ante/fee.go b/x/feemarket/ante/fee.go index 057f4f3..aff6034 100644 --- a/x/feemarket/ante/fee.go +++ b/x/feemarket/ante/fee.go @@ -113,12 +113,12 @@ func (dfd feeMarketCheckDecorator) anteHandle(ctx sdk.Context, tx sdk.Tx, simula ctx = ctx.WithMinGasPrices(sdk.NewDecCoins(minGasPrice)) if !simulate { - fee, 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") } - priorityFee, err := dfd.resolveTxPriorityCoins(ctx, fee.Add(tip), params.FeeDenom) + priorityFee, err := dfd.resolveTxPriorityCoins(ctx, feeCoin, params.FeeDenom) if err != nil { return ctx, errorsmod.Wrapf(err, "error resolving fee priority") } @@ -217,9 +217,7 @@ func GetTxPriority(fee sdk.Coin, gasLimit int64, currentGasPrice sdk.DecCoin) in // overflow panic protection if scaledGasPrice.GTE(sdkmath.LegacyNewDec(math.MaxInt64)) { return math.MaxInt64 - } - - if scaledGasPrice.LTE(sdkmath.LegacyOneDec()) { + } else if scaledGasPrice.LTE(sdkmath.LegacyOneDec()) { return 0 }