refactor: Use Custom Priority in Priority Nonce Mempool

[alexanderbez]

Description

The PriorityNonceMempool provides a data structure that prioritizes transactions respecting sender nonces, which is an amazing data structure and will most likely be used in 99% of use-cases.

The data structure works by using ctx.Priority() when determining priority (and weight). This value is an int64 and is set in CheckTx when inserting a tx into the mempool.

This is fine when priority is determined by fees or something similar, yet there are cases where you want priority to be something different that is either an entirely different type or something that doesn't fit into an int64.

Example:

• Your fees, which are big.Int, far exceed int64 max value (e.g. Evmos which uses 10^18)
• You want priority to be defined based on some property in a message, e.g. sdk.Coins

I've demonstrated in this PR how to achieve a generic way of allowing "generic" priority. There might be a cleaner or more straight forward way, but this seems to do the trick.

cc @kocubinski @JeancarloBarrios

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[kocubinski]

The why makes sense to me. Just another take on the how, we could also consider using a generic type to describe the comparable priority. I explored this here a bit 938280c. Far from perfect but hopefully you get the idea.

I'm not convinced its any better, but something to think about it.

[alexanderbez]

@kocubinski that could work too, but how do you set the custom priority? I.e. what I did with GetTxPriority().

[kocubinski]

It would need to be constructor injected like how you have it @alexanderbez. Another option is to just inject the priority fetcher which must return int64 and change nothing else. Would returning an int64 indicating priority from consumer code be acceptable for the use case you have in mind?

If if we don't do that I think we'd need a comparable constraint on the priority type since it's used as a map key.

[alexanderbez]

@kocubinski yeah the goal here is to use priority that isn't always an int64 or something that can even be cast to an int64.

[kocubinski]

@kocubinski yeah the goal here is to use priority that isn't always an int64 or something that can even be cast to an int64.

That's fine, just be aware that whatever priority is being returned must implement comparable otherwise priority tie resolution with weighting won't work.

[alexanderbez]

Ok, marking this ready for review since Ci is green and the approach works. For review, I guess what I'm looking for is mainly:

• A general review
• What do ppl think of this approach vs 938280c

[alexanderbez]

This is an API breaking change though, so it cannot technically be backported.

[kocubinski]

This test will fail with panic: runtime error: hash of unhashable type []int64 since GetTxPriority is returning a slice. Do you think we should do some type checking or is this OK?

func TestSenderNonce_Priorities(t *testing.T) {
	accounts := simtypes.RandomAccounts(rand.New(rand.NewSource(0)), 1)
	ctx := sdk.NewContext(nil, cmtproto.Header{}, false, log.NewNopLogger())
	sa := accounts[0].Address

	txs := []testTx{
		{priority: 20, nonce: 1, address: sa},
	}

	txPriority := mempool.TxPriority{GetTxPriority: func(ctx context.Context, tx sdk.Tx) any {
		priority := sdk.UnwrapSDKContext(ctx).Priority()
		return []int64{priority}
	}}
	mp := mempool.NewPriorityMempool(txPriority)
	for _, tx := range txs {
		c := ctx.WithPriority(tx.priority)
		require.NoError(t, mp.Insert(c, tx))
		require.Equal(t, 1, mp.CountTx())

		iter := mp.Select(ctx, nil)
		require.Equal(t, tx, iter.Tx())
	}

}

[alexanderbez]

@kocubinski re TestSenderNonce_Priorities good question. Do you have a suggestion or solution? One thing we could do is document for GetTxPriority that any value returned must be addressable. Or perhaps instead of any, we could use Stringer?

[kocubinski]

I explored using generics to assert the priority type is comparable more in b2a3921.

Another option is runtime reflection based checking.

[alexanderbez]

Pushed an approach that uses generics.

[kocubinski]

LGTM. We should probably get another reviewer though since I've looked at this too much.

[alexanderbez]

Nice idea on the config @kocubinski. Pushed changes.

[facundomedica]

utACK. btw what would be an usecase for MaxTx < 0 ? (// - if MaxTx < 0, Insert is a no-op.)

[mark-rushakoff]

Looks like a straightforward mechanical refactor, but I am not yet comfortable in this part of the codebase. I left a couple style comments.

[mark-rushakoff]

The type ( block like this is not commonly used in Go. It adds indentation and impedes grep type TYPENAME. Please keep the individual type declarations.

[alexanderbez]

When there are multiple types in a file, I use () to group them to make it easier for the reader.

[julienrbrt]

lgtm! We should add a changelog given that it had landed in v0.47, and this change is API breaking and an improvement.