test(mempool): add integration test

[cloudgray]

Description

Integration test added for appside mempool

• Fix test helper functions to generate valid transaction that has valid signature, gasPrice, gasLimit to pass CheckTx
• Call CheckTx for generated transactions and verify expected transactions are included in mempool in expected order.
• Call PrepareProposal and verify expected transactions are included in proposal in expected order.
• Modify existing unit test for mempool interface to use valid transaction instead of failing dummy transactions

Found Issues

CheckTx reject valid tx for some case

When two transactions (Tx1 and Tx2) with the same account and the same nonce but different GasPrice sequentially go through the CheckTx logic, replacement does not occur, and the later transaction is treated as an error.

• Expected behavior: Tx2, which has a higher GasPrice, should replace Tx1, which has a lower GasPrice, already in the mempool.
• Actual behavior: Only Tx1 remains in the mempool.
• Error return point: anteHandler

Consideration for code review

• some test cases are bypassed for the found issue above "CheckTx reject valid tx for some case" issue. After fixing the issue, that test cases should be tested.

• mempool.Remove() can remove EVM transactoin only in case that tx is invalid After fixing tx gernerating helper functions to generate valida transaction, false positive test case that tries to remove tx from mempool was deleted.

Closes: #516

---

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

[zsystm]

@cloudgray

When two transactions (Tx1 and Tx2) with the same account and the same nonce but different GasPrice sequentially go through the CheckTx logic, replacement does not occur, and the later transaction is treated as an error.
Expected behavior: Tx2, which has a higher GasPrice, should replace Tx1, which has a lower GasPrice, already in the mempool.
Actual behavior: Only Tx1 remains in the mempool.
Error return point: anteHandler

Just to confirm — in Geth’s mempool, when two txs with the same account/nonce are submitted, the one with the higher gas price replaces the earlier one, right?

[cloudgray]

@zsystm

In Geth’s mempool, when two txs with the same account/nonce are submitted, the one with the higher gas price replaces the earlier one, right?

Yes, more specifically, the following conditions apply, and these conditions are also applied in the same way to both geth(list.go:307-325) and cosmos/evm (list.go:307-325).

1. Price Bump Requirement: The new transaction must have a gas price that's at least X% higher than the existing one (where X is the priceBump parameter, typically 10% by default)
2. Both Fee Cap and Tip Must Be Higher: For EIP-1559 transactions, both the GasFeeCap and GasTipCap must be higher than the existing transaction AND meet the percentage threshold

[vladjdk]

When two transactions (Tx1 and Tx2) with the same account and the same nonce but different GasPrice sequentially go through the CheckTx logic, replacement does not occur, and the later transaction is treated as an error.
Expected behavior: Tx2, which has a higher GasPrice, should replace Tx1, which has a lower GasPrice, already in the mempool.
Actual behavior: Only Tx1 remains in the mempool.
Error return point: anteHandler

This only actually happens in a test environment due to the fact that the antehandler's sequence incrementing is being persisted. In production, the increment would only be saved during finalization and inclusion in the block, and not during the inclusion in the mempool.

I wrote a simple system test to confirm this, which you can add to the suite.

func TestPriorityPendingReplacement(t *testing.T) {
	sut := systemtests.Sut
	sut.ResetChain(t)
	StartChain(t, sut)

	sut.AwaitNBlocks(t, 10)

	// get the directory of the counter project to run commands from
	_, filename, _, _ := runtime.Caller(0)
	testDir := filepath.Dir(filename)
	counterDir := filepath.Join(testDir, "Counter")

	// deploy the contract
	cmd := exec.Command(
		"forge",
		"create", "src/Counter.sol:Counter",
		"--rpc-url", "http://127.0.0.1:8545",
		"--broadcast",
		"--private-key", pk,
	)
	cmd.Dir = counterDir
	res, err := cmd.CombinedOutput()
	require.NoError(t, err)
	require.NotEmpty(t, string(res))

	// get contract address
	contractAddr := parseContractAddress(string(res))
	require.NotEmpty(t, contractAddr)

	wg := sync.WaitGroup{}

	wg.Add(1)
	var lowPrioRes []byte
	go func() {
		defer wg.Done()
		var prioErr error
		lowPrioRes, prioErr = exec.Command(
			"cast", "send",
			contractAddr,
			"increment()",
			"--rpc-url", "http://127.0.0.1:8545",
			"--private-key", pk,
			"--gas-price", "100000000000",
			"--nonce", "1",
		).CombinedOutput()
		require.Error(t, prioErr)
	}()

	var highPrioRes []byte
	wg.Add(1)
	go func() {
		defer wg.Done()
		var prioErr error
		highPrioRes, prioErr = exec.Command(
			"cast", "send",
			contractAddr,
			"increment()",
			"--rpc-url", "http://127.0.0.1:8545",
			"--private-key", pk,
			"--gas-price", "100000000000000",
			"--priority-gas-price", "100",
			"--nonce", "1",
		).CombinedOutput()
		require.NoError(t, prioErr)
	}()

	wg.Wait()

	lowPrioReceipt, err := parseReceipt(string(lowPrioRes))
	require.NoError(t, err)

	highPrioReceipt, err := parseReceipt(string(highPrioRes))
	require.NoError(t, err)

	// 1 = success, 0 = failure.
	require.Equal(t, highPrioReceipt.Status, uint64(1))
	require.Equal(t, lowPrioReceipt.Status, uint64(0))
}

[cloudgray]

This only actually happens in a test environment due to the fact that the antehandler's sequence incrementing is being persisted. In production, the increment would only be saved during finalization and inclusion in the block, and not during the inclusion in the mempool.

I wrote a simple system test to confirm this, which you can add to the suite.

@vladjdk
I don’t know the exact reason, but (it might be that unexpected behavior occurs in foundry cast). In the test code you attached, it seems that lowPrioTx is always sent later. In other words, unlike what we intended, no race condition occurs.

In fact, lowPrioTxRes is always printed as follows:

Error: server returned an error response: error code -32000: failed to broadcast transaction: invalid nonce; got 1, expected 2: invalid sequence: invalid sequence [cosmossdk.io/[email protected]/errors.go:77]

If you add a slight sleep to the goroutine that sends highPrioTx, as shown below, the result becomes the opposite. That is, the same error occurs in highPrioTxRes.

go func() {
		defer wg.Done()
		var prioErr error
		time.Sleep(100 * time.Millisecond)
		highPrioRes, prioErr = exec.Command(
			"cast", "send",
			contractAddr,
			"increment()",
			"--rpc-url", "http://127.0.0.1:8545",
			"--private-key", pk,
			"--gas-price", "100000000000000",
			"--priority-gas-price", "100",
			"--nonce", "1",
		).CombinedOutput()
		require.NoError(t, prioErr)
	}()

This matches the results we live-tested in yesterday’s remote meeting.

[cloudgray]

@vladjdk

You can reproduce with this test code.

func TestPriorityPendingReplacement(t *testing.T) {
	sut := systemtests.Sut
	sut.ResetChain(t)
	StartChain(t, sut)

	sut.AwaitNBlocks(t, 10)

	// get the directory of the counter project to run commands from
	_, filename, _, _ := runtime.Caller(0)
	testDir := filepath.Dir(filename)
	counterDir := filepath.Join(testDir, "Counter")

	// deploy the contract
	cmd := exec.Command(
		"forge",
		"create", "src/Counter.sol:Counter",
		"--rpc-url", "http://127.0.0.1:8545",
		"--broadcast",
		"--private-key", pk,
	)
	cmd.Dir = counterDir
	res, err := cmd.CombinedOutput()
	require.NoError(t, err)
	require.NotEmpty(t, string(res))

	// get contract address
	contractAddr := parseContractAddress(string(res))
	require.NotEmpty(t, contractAddr)

	wg := sync.WaitGroup{}

	wg.Add(1)
	var lowPrioRes []byte
	go func() {
		defer wg.Done()
		var prioErr error
		lowPrioRes, prioErr = exec.Command(
			"cast", "send",
			contractAddr,
			"increment()",
			"--rpc-url", "http://127.0.0.1:8545",
			"--private-key", pk,
			"--gas-price", "100000000000",
			"--nonce", "1",
		).CombinedOutput()
		require.Error(t, prioErr)
	}()

	var highPrioRes []byte
	wg.Add(1)
	go func() {
		defer wg.Done()
		var prioErr error
		time.Sleep(100 * time.Millisecond)
		highPrioRes, prioErr = exec.Command(
			"cast", "send",
			contractAddr,
			"increment()",
			"--rpc-url", "http://127.0.0.1:8545",
			"--private-key", pk,
			"--gas-price", "100000000000000",
			"--priority-gas-price", "100",
			"--nonce", "1",
		).CombinedOutput()
		require.NoError(t, prioErr)
	}()

	wg.Wait()

	lowPrioReceipt, err := parseReceipt(string(lowPrioRes))
	fmt.Println("DEBUG - lowPrioRes: ", string(lowPrioRes))
	fmt.Println("DEBUG - lowPrioReceipt: ", lowPrioReceipt)
	require.NoError(t, err)

	highPrioReceipt, err := parseReceipt(string(highPrioRes))
	fmt.Println("DEBUG - highPrioRes: ", string(highPrioRes))
	fmt.Println("DEBUG - highPrioReceipt: ", highPrioReceipt)
	require.NoError(t, err)

	// 1 = success, 0 = failure.
	require.Equal(t, highPrioReceipt.Status, uint64(1))
	require.Equal(t, lowPrioReceipt.Status, uint64(0))
}