From 255f0f7ce2b53587985ea57013bbd8fe0688bdd3 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Thu, 18 Aug 2022 09:58:21 +0800 Subject: [PATCH 1/5] Change the default priority mechanism to be based on gas price Closes: #12932 gas price based priority is more intuitive and works better with ibc relayers. --- CHANGELOG.md | 1 + x/auth/ante/fee_test.go | 10 +++++----- x/auth/ante/validator_tx_fee.go | 9 +++++---- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 43d16411c856..3d3b966acdc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#12187](https://github.com/cosmos/cosmos-sdk/pull/12187) Add batch operation for x/nft module. * [#12693](https://github.com/cosmos/cosmos-sdk/pull/12693) Make sure the order of each node is consistent when emitting proto events. * [#12455](https://github.com/cosmos/cosmos-sdk/pull/12455) Show attempts count in error for signing. +* [#]() Change the default priority mechanism to be based on gas price. ### State Machine Breaking diff --git a/x/auth/ante/fee_test.go b/x/auth/ante/fee_test.go index 0bddace64b5e..9653cb1ae25f 100644 --- a/x/auth/ante/fee_test.go +++ b/x/auth/ante/fee_test.go @@ -59,7 +59,7 @@ func TestEnsureMempoolFees(t *testing.T) { // msg and signatures msg := testdata.NewTestMsg(accs[0].acc.GetAddress()) feeAmount := testdata.NewTestFeeAmount() - gasLimit := testdata.NewTestGasLimit() + gasLimit := uint64(15) require.NoError(t, s.txBuilder.SetMsgs(msg)) s.txBuilder.SetFeeAmount(feeAmount) s.txBuilder.SetGasLimit(gasLimit) @@ -71,7 +71,7 @@ func TestEnsureMempoolFees(t *testing.T) { require.NoError(t, err) // Set high gas price so standard test fee fails - atomPrice := sdk.NewDecCoinFromDec("atom", math.LegacyNewDec(200).Quo(math.LegacyNewDec(100000))) + atomPrice := sdk.NewDecCoinFromDec("atom", math.LegacyNewDec(20)) highGasPrice := []sdk.DecCoin{atomPrice} s.ctx = s.ctx.WithMinGasPrices(highGasPrice) @@ -103,9 +103,9 @@ func TestEnsureMempoolFees(t *testing.T) { newCtx, err := antehandler(s.ctx, tx, false) require.Nil(t, err, "Decorator should not have errored on fee higher than local gasPrice") - // Priority is the smallest amount in any denom. Since we have only 1 fee - // of 150atom, the priority here is 150. - require.Equal(t, feeAmount.AmountOf("atom").Int64(), newCtx.Priority()) + // Priority is the smallest gas price amount in any denom. Since we have only 1 gas price + // of 10atom, the priority here is 10. + require.Equal(t, int64(10), newCtx.Priority()) } func TestDeductFees(t *testing.T) { diff --git a/x/auth/ante/validator_tx_fee.go b/x/auth/ante/validator_tx_fee.go index d62467e108cf..27ca55215871 100644 --- a/x/auth/ante/validator_tx_fee.go +++ b/x/auth/ante/validator_tx_fee.go @@ -41,18 +41,19 @@ func checkTxFeeWithValidatorMinGasPrices(ctx sdk.Context, tx sdk.Tx) (sdk.Coins, } } - priority := getTxPriority(feeCoins) + priority := getTxPriority(feeCoins, int64(gas)) return feeCoins, priority, nil } // getTxPriority returns a naive tx priority based on the amount of the smallest denomination of the fee // provided in a transaction. -func getTxPriority(fee sdk.Coins) int64 { +func getTxPriority(fee sdk.Coins, gas int64) int64 { var priority int64 for _, c := range fee { p := int64(math.MaxInt64) - if c.Amount.IsInt64() { - p = c.Amount.Int64() + price := c.Amount.QuoRaw(gas) + if price.IsInt64() { + p = price.Int64() } if priority == 0 || p < priority { priority = p From fd5e23b1cc70f689ac67affef98050f2d1096876 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Thu, 18 Aug 2022 12:45:50 +0200 Subject: [PATCH 2/5] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d3b966acdc8..16f70145d424 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,7 +61,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#12187](https://github.com/cosmos/cosmos-sdk/pull/12187) Add batch operation for x/nft module. * [#12693](https://github.com/cosmos/cosmos-sdk/pull/12693) Make sure the order of each node is consistent when emitting proto events. * [#12455](https://github.com/cosmos/cosmos-sdk/pull/12455) Show attempts count in error for signing. -* [#]() Change the default priority mechanism to be based on gas price. +* [#12953](https://github.com/cosmos/cosmos-sdk/pull/12953) Change the default priority mechanism to be based on gas price. ### State Machine Breaking From 03eae331be5dfe0ee540bda421f626e639537754 Mon Sep 17 00:00:00 2001 From: yihuang Date: Thu, 18 Aug 2022 22:07:37 +0800 Subject: [PATCH 3/5] Update x/auth/ante/validator_tx_fee.go Co-authored-by: Aleksandr Bezobchuk --- x/auth/ante/validator_tx_fee.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/x/auth/ante/validator_tx_fee.go b/x/auth/ante/validator_tx_fee.go index 27ca55215871..8a5f4382bd41 100644 --- a/x/auth/ante/validator_tx_fee.go +++ b/x/auth/ante/validator_tx_fee.go @@ -51,9 +51,9 @@ func getTxPriority(fee sdk.Coins, gas int64) int64 { var priority int64 for _, c := range fee { p := int64(math.MaxInt64) - price := c.Amount.QuoRaw(gas) - if price.IsInt64() { - p = price.Int64() + gasPrice := c.Amount.QuoRaw(gas) + if gasPrice.IsInt64() { + p = gasPrice.Int64() } if priority == 0 || p < priority { priority = p From 7e1920a1a834f7ffeabf65b497f1e1b21ae01008 Mon Sep 17 00:00:00 2001 From: yihuang Date: Mon, 22 Aug 2022 10:05:07 +0800 Subject: [PATCH 4/5] Update x/auth/ante/validator_tx_fee.go --- x/auth/ante/validator_tx_fee.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/auth/ante/validator_tx_fee.go b/x/auth/ante/validator_tx_fee.go index 8a5f4382bd41..225d540c36c7 100644 --- a/x/auth/ante/validator_tx_fee.go +++ b/x/auth/ante/validator_tx_fee.go @@ -45,7 +45,7 @@ func checkTxFeeWithValidatorMinGasPrices(ctx sdk.Context, tx sdk.Tx) (sdk.Coins, return feeCoins, priority, nil } -// getTxPriority returns a naive tx priority based on the amount of the smallest denomination of the fee +// getTxPriority returns a naive tx priority based on the amount of the smallest denomination of the gas price // provided in a transaction. func getTxPriority(fee sdk.Coins, gas int64) int64 { var priority int64 From 30a7e135d6d9401927292bede15050a70572fdee Mon Sep 17 00:00:00 2001 From: HuangYi Date: Mon, 22 Aug 2022 23:23:26 +0800 Subject: [PATCH 5/5] add comment --- x/auth/ante/validator_tx_fee.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x/auth/ante/validator_tx_fee.go b/x/auth/ante/validator_tx_fee.go index 225d540c36c7..e8c865b757ed 100644 --- a/x/auth/ante/validator_tx_fee.go +++ b/x/auth/ante/validator_tx_fee.go @@ -47,6 +47,8 @@ func checkTxFeeWithValidatorMinGasPrices(ctx sdk.Context, tx sdk.Tx) (sdk.Coins, // 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 prioritize as expected. func getTxPriority(fee sdk.Coins, gas int64) int64 { var priority int64 for _, c := range fee {