fix(mempool): align cosmos tx priority with priority setup of anteHandler #534

cloudgray · 2025-08-27T12:39:52Z

Description

This PR should be merged after PR #512.

Summary

AnteHandler now allows evmDenom as the sole feeDenom. (staking bond denom is excluded)
Update default cosmosPoolConfig to use ctx.priority as the priority of cosmos tx
Update effectiveTip calculation of cosmos tx to be aligned with anteHandler effectiveTip calcualtion

Code review guide

The review scope of this PR is as below.
Other changes belongs to PR #512.

ante/cosmos/min_gas_price.go
mempool/iterator.go
mempool/mempool.go

Closes: #510

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...

tackled an existing issue or discussed with a team member
left instructions on how to review the changes
targeted the main branch

aljo242 · 2025-08-27T14:36:23Z

ante/cosmos/min_gas_price.go

-	// TODO: is the handling of stake necessary here? Why not adjust the tests to contain the correct denom?
-	validFees := len(feeCoins) == 0 || (len(feeCoins) == 1 && slices.Contains([]string{evmDenom, sdk.DefaultBondDenom}, feeCoins.GetDenomByIndex(0)))
+	// only allow user to pass in evm coin as transaction fee
+	validFees := len(feeCoins) == 0 || (len(feeCoins) == 1 && slices.Contains([]string{evmDenom}, feeCoins.GetDenomByIndex(0)))


This should be a helper function that we can test

applied (8c25160)

aljo242 · 2025-08-27T14:37:07Z

tests/integration/mempool/test_helper.go

+	tx, err := s.factory.BuildCosmosTx(key.Priv, txArgs)
+	s.Require().NoError(err)
+
+	fmt.Printf("DEBUG: Created cosmos transaction successfully\n")


let's not use fmt in any of our testing. we can use t.Log()

I'll apply this comment in PR #512 .

applied in PR #512 (fd4e829)

aljo242 · 2025-08-27T14:37:36Z

tests/integration/mempool/test_helper.go

@@ -0,0 +1,153 @@
+package mempool


nit: rename to test_helpers.go

I'll apply this comment in PR #512 .

applied in PR #512 (b39c89d)

canu0205

Left few comments regarding using context priority also in the iterator. Besides that, thank you for your work!

canu0205 · 2025-08-28T10:12:51Z

mempool/iterator.go

@@ -287,6 +318,10 @@ func (i *EVMMempoolIterator) extractCosmosEffectiveTip(tx sdk.Tx) *uint256.Int {
 	}

 	effectiveTip := new(uint256.Int).Sub(gasPrice, baseFee)
+	if effectiveTip.Cmp(gasTipCap) > 0 {
+		effectiveTip = gasTipCap


I suggest to use context priority here too. Since if cosmos tx doesn't set TxFeeChecker, it falls thru checkTxFeeWIthValidatorMinGasPrices which calculates priority based on gas price.

Therefore, in this case, there would be discrepancy in the priority of the context and the priority calculated in this function.

My suggestion is getting a context with blockchain and then retain priority from that context. In this case, this function extractCosmosEffectiveTip might not needed.

I agree with this. We should lean into fully supporting context priority. Alongside this, we should set the default context priority for Cosmos EVM to be the effective gas tip, allowing people to modify it if they wish.

canu0205 · 2025-08-28T10:18:39Z

tests/integration/mempool/test_helper.go

+
+// calculateCosmosEffectiveTip calculates the effective tip for a Cosmos transaction
+// This aligns with EVM transaction prioritization: effective_tip = gas_price - base_fee
+func (s *IntegrationTestSuite) calculateCosmosEffectiveTip(feeAmount int64, gasLimit uint64, baseFee *big.Int) *big.Int {


priority tip is also needed when calculating cosmos effective tip

applied (db33377)

canu0205 · 2025-08-28T10:21:38Z

tests/integration/mempool/test_helper.go

+	TxGas = 100_000
+)
+
+// createCosmosSendTransactionWithKey creates a simple bank send transaction with the specified key


nit: comment unmatched with the function signature

applied (db33377)

canu0205 · 2025-08-28T10:21:58Z

tests/integration/mempool/test_helper.go

+	return tx
+}
+
+// createEVMTransaction creates an EVM transaction using the provided key


nit: comment unmatched with the function signature

applied (db33377)

canu0205 · 2025-08-28T10:22:36Z

tests/integration/mempool/test_helper.go

+	return nil
+}
+
+// checkTxs call abci CheckTx for a transaction


typo checkTxs -> checkTx

applied (db33377)

vladjdk

Adding onto what @canu0205 is saying, we should fully lean into ctx.Priority() everywhere Cosmos comparisons are used, with the assumption that the ctx.Priority() behaves sensibly.

Because we're generalizing this to be customizable, we should also set the default priority to be effectiveTip-based for all Cosmos transactions. This should be available in EVMD as a reference and should be easily extensible and modifiable.

vladjdk · 2025-09-12T17:49:33Z

mempool/iterator.go

@@ -287,6 +318,10 @@ func (i *EVMMempoolIterator) extractCosmosEffectiveTip(tx sdk.Tx) *uint256.Int {
 	}

 	effectiveTip := new(uint256.Int).Sub(gasPrice, baseFee)
+	if effectiveTip.Cmp(gasTipCap) > 0 {
+		effectiveTip = gasTipCap


I agree with this. We should lean into fully supporting context priority. Alongside this, we should set the default context priority for Cosmos EVM to be the effective gas tip, allowing people to modify it if they wish.

cloudgray added 18 commits August 21, 2025 21:00

WIP: test(mempool): add integration test

fca99f2

WIP: test(mempool): add integration test

3591815

test(mempool): refactor integration test

ad31857

test(mempool): fix integration test

a656805

test(mempool): refactor integration test

5a384a7

chore: modify comments

d74f5ad

Merge branch 'main' into test/appside-mempool

9a4d096

test(mempool): add PrepareProposal check to integration test

abbe071

chore: fix lint

603c890

chore: update changelog

bc522eb

Merge branch 'main' into test/appside-mempool

375fee6

test(mempool): modify testcode

dbe047b

chore: fix lint

29fd29f

chore: fix lint

142fbca

fix(ante): remove bond denom from allowed fee denom

5d29424

fix(mempool): calculation of priority of cosmos tx

66811c5

test(mempool): fix mempool integration test

9449d73

Merge branch 'main' into fix/cosmos-tx-priority

60a2cc7

cloudgray marked this pull request as ready for review August 27, 2025 13:01

cloudgray requested review from a team as code owners August 27, 2025 13:01

aljo242 reviewed Aug 27, 2025

View reviewed changes

canu0205 suggested changes Aug 28, 2025

View reviewed changes

cloudgray added 3 commits August 29, 2025 19:01

add unit test for cosmos tx feeCoins validation

8c25160

chore: fix lint

155b2b9

chore: modify test helper function and comments

db33377

cloudgray self-assigned this Sep 1, 2025

cloudgray added 2 commits September 9, 2025 19:44

Merge branch 'main' into fix/cosmos-tx-priority

272d598

chore: resolve merge conflict

ca3cdb2

chore: update CHANGELOG.md

ac9cd72

vladjdk requested changes Sep 12, 2025

View reviewed changes

fix(mempool): align cosmos tx priority with priority setup of anteHandler #534

Are you sure you want to change the base?

fix(mempool): align cosmos tx priority with priority setup of anteHandler #534

Conversation

cloudgray commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Summary

Code review guide

Author Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

canu0205 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladjdk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cloudgray commented Aug 27, 2025 •

edited

Loading