Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Change the default priority mechanism to be based on gas price #12953

Merged
merged 9 commits into from Aug 22, 2022
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -64,6 +64,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#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.
* [#12886](https://github.com/cosmos/cosmos-sdk/pull/12886) Amortize cost of processing cache KV store
* [#12953](https://github.com/cosmos/cosmos-sdk/pull/12953) Change the default priority mechanism to be based on gas price.

### State Machine Breaking

Expand Down
10 changes: 5 additions & 5 deletions x/auth/ante/fee_test.go
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, it makes the test easier to understand / read. In my version of the fee prioritization I'm scaling the gas based on fee amount (using QuoRaw)

require.NoError(t, s.txBuilder.SetMsgs(msg))
s.txBuilder.SetFeeAmount(feeAmount)
s.txBuilder.SetGasLimit(gasLimit)
Expand All @@ -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)

Expand Down Expand Up @@ -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) {
Expand Down
13 changes: 8 additions & 5 deletions x/auth/ante/validator_tx_fee.go
Expand Up @@ -41,18 +41,21 @@ 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
// 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) int64 {
// 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 {
p := int64(math.MaxInt64)
if c.Amount.IsInt64() {
p = c.Amount.Int64()
gasPrice := c.Amount.QuoRaw(gas)
if gasPrice.IsInt64() {
p = gasPrice.Int64()
}
if priority == 0 || p < priority {
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
priority = p
Expand Down