From e308f08272c5e261c61d3f3a0c610556531e9daa Mon Sep 17 00:00:00 2001 From: Alex Johnson Date: Thu, 6 Jun 2024 11:25:37 -0400 Subject: [PATCH 1/2] fix: tx prioritization (#101) * fix * beauty * fix * fix * fix * test * lint * fix (cherry picked from commit b2dc5715fc56f5c5ca44398aa589819d9593b9e9) # Conflicts: # tests/e2e/consumer.go --- tests/e2e/consumer.go | 122 +++++++++++++++++++++++++++ x/feemarket/ante/fee.go | 89 +++++++++++++------ x/feemarket/fuzz/tx_priority_test.go | 52 ++++++++++++ x/feemarket/post/fee_test.go | 7 +- 4 files changed, 239 insertions(+), 31 deletions(-) create mode 100644 tests/e2e/consumer.go create mode 100644 x/feemarket/fuzz/tx_priority_test.go diff --git a/tests/e2e/consumer.go b/tests/e2e/consumer.go new file mode 100644 index 0000000..307caf8 --- /dev/null +++ b/tests/e2e/consumer.go @@ -0,0 +1,122 @@ +package e2e + +import ( + "context" + "fmt" + "testing" + "time" + + interchaintest "github.com/strangelove-ventures/interchaintest/v8" + "github.com/strangelove-ventures/interchaintest/v8/chain/cosmos" + "github.com/strangelove-ventures/interchaintest/v8/ibc" + "github.com/strangelove-ventures/interchaintest/v8/testreporter" + "github.com/stretchr/testify/require" + "go.uber.org/zap/zaptest" +) + +var ( + providerChainID = "provider-1" + providerNumValidators = 4 + providerVersion = "v5.0.0" +) + +// CCVChainConstructor is a constructor for the CCV chain +func CCVChainConstructor(t *testing.T, spec *interchaintest.ChainSpec) []*cosmos.CosmosChain { + // require that we only have 4 validators + require.Equal(t, 4, *spec.NumValidators) + + cf := interchaintest.NewBuiltinChainFactory( + zaptest.NewLogger(t), + []*interchaintest.ChainSpec{ + spec, + { + Name: "ics-provider", + Version: providerVersion, + NumValidators: &providerNumValidators, + ChainConfig: ibc.ChainConfig{ + GasPrices: "0uatom", + GasAdjustment: 0.0, + ChainID: providerChainID, + TrustingPeriod: "336h", + ModifyGenesis: cosmos.ModifyGenesis( + []cosmos.GenesisKV{ + cosmos.NewGenesisKV("app_state.provider.params.blocks_per_epoch", "1"), + }, + ), + }, + }, + }, + ) + + chains, err := cf.Chains(t.Name()) + require.NoError(t, err) + + require.Len(t, chains, 2) + + return []*cosmos.CosmosChain{chains[0].(*cosmos.CosmosChain), chains[1].(*cosmos.CosmosChain)} +} + +type CCVInterchain struct { + relayer ibc.Relayer + reporter *testreporter.RelayerExecReporter + ibcPath string +} + +func (c *CCVInterchain) Relayer() ibc.Relayer { + return c.relayer +} + +func (c *CCVInterchain) Reporter() *testreporter.RelayerExecReporter { + return c.reporter +} + +func (c *CCVInterchain) IBCPath() string { + return c.ibcPath +} + +// CCVInterchainConstructor is a constructor for the CCV interchain +func CCVInterchainConstructor(ctx context.Context, t *testing.T, chains []*cosmos.CosmosChain) Interchain { + // expect 2 chains + require.Len(t, chains, 2) + + // create a relayer + client, network := interchaintest.DockerSetup(t) + r := interchaintest.NewBuiltinRelayerFactory( + ibc.CosmosRly, + zaptest.NewLogger(t), + ).Build(t, client, network) + + path := "feemarket-ibc-path" + // create the interchain + ic := interchaintest.NewInterchain(). + AddChain(chains[0]). + AddChain(chains[1]). + AddRelayer(r, "relayer"). + AddProviderConsumerLink(interchaintest.ProviderConsumerLink{ + Provider: chains[1], + Consumer: chains[0], + Relayer: r, + Path: path, + }) + // Log location + f, err := interchaintest.CreateLogFile(fmt.Sprintf("%d.json", time.Now().Unix())) + require.NoError(t, err) + // Reporter/logs + rep := testreporter.NewReporter(f) + eRep := rep.RelayerExecReporter(t) + + require.NoError(t, ic.Build(ctx, eRep, interchaintest.InterchainBuildOptions{ + TestName: t.Name(), + Client: client, + NetworkID: network, + SkipPathCreation: false, + })) + + require.NoError(t, chains[1].FinishICSProviderSetup(ctx, r, eRep, path)) + + return &CCVInterchain{ + relayer: r, + reporter: eRep, + ibcPath: path, + } +} diff --git a/x/feemarket/ante/fee.go b/x/feemarket/ante/fee.go index 752b2bf..17adc1e 100644 --- a/x/feemarket/ante/fee.go +++ b/x/feemarket/ante/fee.go @@ -80,23 +80,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 @@ -108,29 +135,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 } } @@ -138,20 +163,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 be3064c..446e987 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 a7547ca515fc18c1b6fefa8a7ab2f3e5b3185fc7 Mon Sep 17 00:00:00 2001 From: aljo242 Date: Thu, 6 Jun 2024 13:53:08 -0400 Subject: [PATCH 2/2] fix --- tests/e2e/consumer.go | 122 ----------------------------------- x/feemarket/post/fee_test.go | 2 +- 2 files changed, 1 insertion(+), 123 deletions(-) delete mode 100644 tests/e2e/consumer.go diff --git a/tests/e2e/consumer.go b/tests/e2e/consumer.go deleted file mode 100644 index 307caf8..0000000 --- a/tests/e2e/consumer.go +++ /dev/null @@ -1,122 +0,0 @@ -package e2e - -import ( - "context" - "fmt" - "testing" - "time" - - interchaintest "github.com/strangelove-ventures/interchaintest/v8" - "github.com/strangelove-ventures/interchaintest/v8/chain/cosmos" - "github.com/strangelove-ventures/interchaintest/v8/ibc" - "github.com/strangelove-ventures/interchaintest/v8/testreporter" - "github.com/stretchr/testify/require" - "go.uber.org/zap/zaptest" -) - -var ( - providerChainID = "provider-1" - providerNumValidators = 4 - providerVersion = "v5.0.0" -) - -// CCVChainConstructor is a constructor for the CCV chain -func CCVChainConstructor(t *testing.T, spec *interchaintest.ChainSpec) []*cosmos.CosmosChain { - // require that we only have 4 validators - require.Equal(t, 4, *spec.NumValidators) - - cf := interchaintest.NewBuiltinChainFactory( - zaptest.NewLogger(t), - []*interchaintest.ChainSpec{ - spec, - { - Name: "ics-provider", - Version: providerVersion, - NumValidators: &providerNumValidators, - ChainConfig: ibc.ChainConfig{ - GasPrices: "0uatom", - GasAdjustment: 0.0, - ChainID: providerChainID, - TrustingPeriod: "336h", - ModifyGenesis: cosmos.ModifyGenesis( - []cosmos.GenesisKV{ - cosmos.NewGenesisKV("app_state.provider.params.blocks_per_epoch", "1"), - }, - ), - }, - }, - }, - ) - - chains, err := cf.Chains(t.Name()) - require.NoError(t, err) - - require.Len(t, chains, 2) - - return []*cosmos.CosmosChain{chains[0].(*cosmos.CosmosChain), chains[1].(*cosmos.CosmosChain)} -} - -type CCVInterchain struct { - relayer ibc.Relayer - reporter *testreporter.RelayerExecReporter - ibcPath string -} - -func (c *CCVInterchain) Relayer() ibc.Relayer { - return c.relayer -} - -func (c *CCVInterchain) Reporter() *testreporter.RelayerExecReporter { - return c.reporter -} - -func (c *CCVInterchain) IBCPath() string { - return c.ibcPath -} - -// CCVInterchainConstructor is a constructor for the CCV interchain -func CCVInterchainConstructor(ctx context.Context, t *testing.T, chains []*cosmos.CosmosChain) Interchain { - // expect 2 chains - require.Len(t, chains, 2) - - // create a relayer - client, network := interchaintest.DockerSetup(t) - r := interchaintest.NewBuiltinRelayerFactory( - ibc.CosmosRly, - zaptest.NewLogger(t), - ).Build(t, client, network) - - path := "feemarket-ibc-path" - // create the interchain - ic := interchaintest.NewInterchain(). - AddChain(chains[0]). - AddChain(chains[1]). - AddRelayer(r, "relayer"). - AddProviderConsumerLink(interchaintest.ProviderConsumerLink{ - Provider: chains[1], - Consumer: chains[0], - Relayer: r, - Path: path, - }) - // Log location - f, err := interchaintest.CreateLogFile(fmt.Sprintf("%d.json", time.Now().Unix())) - require.NoError(t, err) - // Reporter/logs - rep := testreporter.NewReporter(f) - eRep := rep.RelayerExecReporter(t) - - require.NoError(t, ic.Build(ctx, eRep, interchaintest.InterchainBuildOptions{ - TestName: t.Name(), - Client: client, - NetworkID: network, - SkipPathCreation: false, - })) - - require.NoError(t, chains[1].FinishICSProviderSetup(ctx, r, eRep, path)) - - return &CCVInterchain{ - relayer: r, - reporter: eRep, - ibcPath: path, - } -} diff --git a/x/feemarket/post/fee_test.go b/x/feemarket/post/fee_test.go index 446e987..f14ac49 100644 --- a/x/feemarket/post/fee_test.go +++ b/x/feemarket/post/fee_test.go @@ -153,7 +153,7 @@ func TestPostHandle(t *testing.T) { ) // exact cost of transaction - gasLimit := uint64(27284) + gasLimit := uint64(25635) validFeeAmount := types.DefaultMinBaseGasPrice.MulInt64(int64(gasLimit)) validFeeAmountWithTip := validFeeAmount.Add(math.LegacyNewDec(100)) validFee := sdk.NewCoins(sdk.NewCoin(baseDenom, validFeeAmount.TruncateInt()))