diff --git a/CHANGELOG.md b/CHANGELOG.md index 81fea1ac8..c3ed2eff9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,8 @@ - [\#495](https://github.com/cosmos/evm/pull/495) Allow immediate SIGINT interrupt when mempool is not empty - [\#416](https://github.com/cosmos/evm/pull/416) Fix regression in CometBlockResultByNumber when height is 0 to use the latest block. This fixes eth_getFilterLogs RPC. - [\#545](https://github.com/cosmos/evm/pull/545) Check if mempool is not nil before accepting nonce gap error tx. -- [\#585](https://github.com/cosmos/evm/pull/585) Use zero constructor to avoid nil pointer panic when BaseFee is 0d +- [\#585](https://github.com/cosmos/evm/pull/585) Use zero constructor to avoid nil pointer panic when BaseFee is 0d +- [\#591](https://github.com/cosmos/evm/pull/591) CheckTxHandler should handle "invalid nonce" tx ### IMPROVEMENTS diff --git a/ante/evm/09_increment_sequence.go b/ante/evm/09_increment_sequence.go index 3440d7c61..fe28cf896 100644 --- a/ante/evm/09_increment_sequence.go +++ b/ante/evm/09_increment_sequence.go @@ -22,15 +22,15 @@ func IncrementNonce( accountNonce := account.GetSequence() // we merged the accountNonce verification to accountNonce increment, so when tx includes multiple messages // with same sender, they'll be accepted. - if txNonce != accountNonce { - if txNonce > accountNonce { - return errorsmod.Wrapf( - mempool.ErrNonceGap, - "tx nonce: %d, account accountNonce: %d", txNonce, accountNonce, - ) - } + if txNonce > accountNonce { return errorsmod.Wrapf( - errortypes.ErrInvalidSequence, + mempool.ErrNonceGap, + "tx nonce: %d, account accountNonce: %d", txNonce, accountNonce, + ) + } + if txNonce < accountNonce { + return errorsmod.Wrapf( + mempool.ErrNonceLow, "invalid nonce; got %d, expected %d", txNonce, accountNonce, ) } diff --git a/mempool/check_tx.go b/mempool/check_tx.go index 438ad9783..f42a024fe 100644 --- a/mempool/check_tx.go +++ b/mempool/check_tx.go @@ -18,7 +18,7 @@ func NewCheckTxHandler(mempool *ExperimentalEVMMempool) types.CheckTxHandler { gInfo, result, anteEvents, err := runTx(request.Tx, nil) if err != nil { // detect if there is a nonce gap error (only returned for EVM transactions) - if errors.Is(err, ErrNonceGap) { + if errors.Is(err, ErrNonceGap) || errors.Is(err, ErrNonceLow) { // send it to the mempool for further triage err := mempool.InsertInvalidNonce(request.Tx) if err != nil { diff --git a/mempool/errors.go b/mempool/errors.go index 522d2f7e5..f42ddc530 100644 --- a/mempool/errors.go +++ b/mempool/errors.go @@ -8,4 +8,5 @@ var ( ErrExpectedOneError = errors.New("expected 1 error") ErrNotEVMTransaction = errors.New("transaction is not an EVM transaction") ErrNonceGap = errors.New("tx nonce is higher than account nonce") + ErrNonceLow = errors.New("tx nonce is lower than account nonce") ) diff --git a/tests/integration/ante/test_evm_unit_09_increment_sequence.go b/tests/integration/ante/test_evm_unit_09_increment_sequence.go index d2b3e0b64..49d8fbd2e 100644 --- a/tests/integration/ante/test_evm_unit_09_increment_sequence.go +++ b/tests/integration/ante/test_evm_unit_09_increment_sequence.go @@ -9,7 +9,6 @@ import ( testkeyring "github.com/cosmos/evm/testutil/keyring" sdktypes "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/types/errors" ) func (s *EvmUnitAnteTestSuite) TestIncrementSequence() { @@ -38,8 +37,8 @@ func (s *EvmUnitAnteTestSuite) TestIncrementSequence() { }, }, { - name: "fail: invalid sequence", - expectedError: errors.ErrInvalidSequence, + name: "fail: nonce is low", + expectedError: mempool.ErrNonceLow, malleate: func(acct sdktypes.AccountI) uint64 { err := acct.SetSequence(acct.GetSequence() + 1) s.Require().NoError(err) diff --git a/tests/integration/mempool/test_helpers.go b/tests/integration/mempool/test_helpers.go index 69246265a..1c87c0586 100644 --- a/tests/integration/mempool/test_helpers.go +++ b/tests/integration/mempool/test_helpers.go @@ -4,10 +4,12 @@ import ( "encoding/hex" "fmt" "math/big" + "time" abci "github.com/cometbft/cometbft/abci/types" "github.com/cometbft/cometbft/crypto/tmhash" + evmmempool "github.com/cosmos/evm/mempool" "github.com/cosmos/evm/testutil/integration/base/factory" "github.com/cosmos/evm/testutil/keyring" evmtypes "github.com/cosmos/evm/x/vm/types" @@ -67,6 +69,29 @@ func (s *IntegrationTestSuite) createEVMValueTransferTx(key keyring.Key, nonce i return tx } +// createEVMTransaction creates an EVM transaction using the provided key +func (s *IntegrationTestSuite) createEVMValueTransferDynamicFeeTx(key keyring.Key, nonce int, gasFeeCap, gasTipCap *big.Int) sdk.Tx { + to := s.keyring.GetKey(1).Addr + + if nonce < 0 { + s.Require().NoError(fmt.Errorf("nonce must be non-negative")) + } + + ethTxArgs := evmtypes.EvmTxArgs{ + Nonce: uint64(nonce), + To: &to, + Amount: big.NewInt(1000), + GasLimit: TxGas, + GasFeeCap: gasFeeCap, + GasTipCap: gasTipCap, + Input: nil, + } + tx, err := s.factory.GenerateSignedEthTx(key.Priv, ethTxArgs) + s.Require().NoError(err) + + return tx +} + // createEVMContractDeployTx creates an EVM transaction for contract deployment func (s *IntegrationTestSuite) createEVMContractDeployTx(key keyring.Key, gasPrice *big.Int, data []byte) sdk.Tx { ethTxArgs := evmtypes.EvmTxArgs{ @@ -86,7 +111,13 @@ func (s *IntegrationTestSuite) createEVMContractDeployTx(key keyring.Key, gasPri // checkTxs call abci CheckTx for multipile transactions func (s *IntegrationTestSuite) checkTxs(txs []sdk.Tx) error { for _, tx := range txs { - if err := s.checkTx(tx); err != nil { + if res, err := s.checkTx(tx); err != nil { + if err != nil { + return fmt.Errorf("failed to execute CheckTx for tx: %s", s.getTxHash(tx)) + } + if res.Code != abci.CodeTypeOK { + return fmt.Errorf("tx (%s) failed to pass CheckTx with log: %s", s.getTxHash(tx), res.Log) + } return err } } @@ -94,21 +125,34 @@ func (s *IntegrationTestSuite) checkTxs(txs []sdk.Tx) error { } // checkTxs call abci CheckTx for a transaction -func (s *IntegrationTestSuite) checkTx(tx sdk.Tx) error { +func (s *IntegrationTestSuite) checkTx(tx sdk.Tx) (*abci.ResponseCheckTx, error) { txBytes, err := s.network.App.GetTxConfig().TxEncoder()(tx) if err != nil { - return fmt.Errorf("failed to encode cosmos tx: %w", err) + return nil, fmt.Errorf("failed to encode cosmos tx: %w", err) } - _, err = s.network.App.CheckTx(&abci.RequestCheckTx{ + res, err := s.network.App.CheckTx(&abci.RequestCheckTx{ Tx: txBytes, Type: abci.CheckTxType_New, }) if err != nil { - return fmt.Errorf("failed to encode cosmos tx: %w", err) + return nil, fmt.Errorf("failed to execute CheckTx: %w", err) } - return nil + return res, nil +} + +func (s *IntegrationTestSuite) getTxBytes(txs []sdk.Tx) ([][]byte, error) { + txEncoder := s.network.App.GetTxConfig().TxEncoder() + txBytes := make([][]byte, 0) + for _, tx := range txs { + bz, err := txEncoder(tx) + if err != nil { + return nil, fmt.Errorf("failed to encode tx: %w", err) + } + txBytes = append(txBytes, bz) + } + return txBytes, nil } // getTxHashes returns transaction hashes for multiple transactions @@ -150,3 +194,28 @@ func (s *IntegrationTestSuite) calculateCosmosEffectiveTip(feeAmount int64, gasL return new(big.Int).Sub(gasPrice, baseFee) } + +// notifyNewBlockToMempool triggers the natural block notification mechanism used in production. +// This sends a ChainHeadEvent that causes the mempool to update its state and remove committed transactions. +// The event subscription mechanism naturally calls Reset() which triggers the transaction cleanup process. +func (s *IntegrationTestSuite) notifyNewBlockToMempool() { + // Get the EVM mempool from the app + evmMempool := s.network.App.GetMempool() + + // Access the underlying blockchain interface from the EVM mempool + if evmMempoolCast, ok := evmMempool.(*evmmempool.ExperimentalEVMMempool); ok { + blockchain := evmMempoolCast.GetBlockchain() + + // Trigger a new block notification + // This sends a ChainHeadEvent that the mempool subscribes to. + // The TxPool's event loop receives this and calls Reset() for each subpool, + // which naturally removes committed transactions via demoteUnexecutables(). + blockchain.NotifyNewBlock() + + // The ChainHeadEvent is processed asynchronously, so we need to wait a bit + // for the event to be processed and the reset to complete. + // In integration tests, this might need a small delay to ensure the event + // is processed before we check the mempool state. + time.Sleep(100 * time.Millisecond) + } +} diff --git a/tests/integration/mempool/test_mempool_integration_abci.go b/tests/integration/mempool/test_mempool_integration_abci.go index b4aa655f0..cd0648468 100644 --- a/tests/integration/mempool/test_mempool_integration_abci.go +++ b/tests/integration/mempool/test_mempool_integration_abci.go @@ -4,6 +4,8 @@ import ( "encoding/hex" "math/big" + "github.com/ethereum/go-ethereum/core" + abci "github.com/cometbft/cometbft/abci/types" "github.com/cometbft/cometbft/crypto/tmhash" @@ -16,10 +18,6 @@ func (s *IntegrationTestSuite) TestTransactionOrderingWithABCIMethodCalls() { testCases := []struct { name string setupTxs func() ([]sdk.Tx, []string) - // TODO: remove bypass option after anteHandler is fixed. - // Current anteHandler rejects valid high-gas transaction to replace low-gas transaction - // So, all replacement test cases fail. - bypass bool }{ { name: "mixed EVM and cosmos transaction ordering", @@ -49,7 +47,7 @@ func (s *IntegrationTestSuite) TestTransactionOrderingWithABCIMethodCalls() { lowFeeEVMTx := s.createEVMValueTransferTx(s.keyring.GetKey(0), 0, big.NewInt(2000000000)) // 2 gaatom // Create second EVM transaction with high fee - highFeeEVMTx := s.createEVMValueTransferTx(s.keyring.GetKey(0), 0, big.NewInt(5000000000)) // 5 gaatom + highFeeEVMTx := s.createEVMValueTransferDynamicFeeTx(s.keyring.GetKey(0), 0, big.NewInt(5000000000), big.NewInt(5000000000)) // 5 gaatom // Input txs in order inputTxs := []sdk.Tx{lowFeeEVMTx, highFeeEVMTx} @@ -60,7 +58,6 @@ func (s *IntegrationTestSuite) TestTransactionOrderingWithABCIMethodCalls() { return inputTxs, expTxHashes }, - bypass: true, }, { name: "EVM-only transaction ordering", @@ -82,64 +79,44 @@ func (s *IntegrationTestSuite) TestTransactionOrderingWithABCIMethodCalls() { return inputTxs, expTxHashes }, }, - { - name: "cosmos-only transaction replacement", - setupTxs: func() ([]sdk.Tx, []string) { - highFeeTx := s.createCosmosSendTx(s.keyring.GetKey(0), big.NewInt(5000000000)) // 5 gaatom - lowFeeTx := s.createCosmosSendTx(s.keyring.GetKey(0), big.NewInt(2000000000)) // 2 gaatom - mediumFeeTx := s.createCosmosSendTx(s.keyring.GetKey(0), big.NewInt(3000000000)) // 3 gaatom - - // Input txs in order - inputTxs := []sdk.Tx{mediumFeeTx, lowFeeTx, highFeeTx} - - // Expected txs in order - expectedTxs := []sdk.Tx{highFeeTx} - expTxHashes := s.getTxHashes(expectedTxs) - - return inputTxs, expTxHashes - }, - bypass: true, - }, { name: "mixed EVM and Cosmos transactions with equal effective tips", setupTxs: func() ([]sdk.Tx, []string) { - // Create transactions with equal effective tips (assuming base fee = 0) - // EVM: 1000 aatom/gas effective tip - evmTx := s.createEVMValueTransferTx(s.keyring.GetKey(0), 0, big.NewInt(1000000000)) // 1 gaatom/gas - // Cosmos with same effective tip: 1000 * 200000 = 200000000 aatom total fee cosmosTx := s.createCosmosSendTx(s.keyring.GetKey(0), big.NewInt(1000000000)) // 1 gaatom/gas effective tip + // Create transactions with equal effective tips (assuming base fee = 0) + // EVM: 1000 aatom/gas effective tip + evmTx := s.createEVMValueTransferDynamicFeeTx(s.keyring.GetKey(0), 0, big.NewInt(1000000000), big.NewInt(1000000000)) // 1 gaatom/gas + // Input txs in order inputTxs := []sdk.Tx{cosmosTx, evmTx} // Expected txs in order - expectedTxs := []sdk.Tx{evmTx, cosmosTx} + expectedTxs := []sdk.Tx{evmTx} expTxHashes := s.getTxHashes(expectedTxs) return inputTxs, expTxHashes }, - bypass: true, }, { name: "mixed transactions with EVM having higher effective tip", setupTxs: func() ([]sdk.Tx, []string) { - // Create EVM transaction with higher gas price - evmTx := s.createEVMValueTransferTx(s.keyring.GetKey(0), 0, big.NewInt(5000000000)) // 5 gaatom/gas - // Create Cosmos transaction with lower gas price cosmosTx := s.createCosmosSendTx(s.keyring.GetKey(0), big.NewInt(2000000000)) // 2 gaatom/gas + // Create EVM transaction with higher gas price + evmTx := s.createEVMValueTransferDynamicFeeTx(s.keyring.GetKey(0), 0, big.NewInt(5000000000), big.NewInt(5000000000)) // 5 gaatom/gas + // Input txs in order inputTxs := []sdk.Tx{cosmosTx, evmTx} // Expected txs in order - expectedTxs := []sdk.Tx{evmTx, cosmosTx} + expectedTxs := []sdk.Tx{evmTx} expTxHashes := s.getTxHashes(expectedTxs) return inputTxs, expTxHashes }, - bypass: true, }, { name: "mixed transactions with Cosmos having higher effective tip", @@ -154,12 +131,11 @@ func (s *IntegrationTestSuite) TestTransactionOrderingWithABCIMethodCalls() { inputTxs := []sdk.Tx{evmTx, cosmosTx} // Expected txs in order - expectedTxs := []sdk.Tx{cosmosTx, evmTx} + expectedTxs := []sdk.Tx{evmTx} expTxHashes := s.getTxHashes(expectedTxs) return inputTxs, expTxHashes }, - bypass: true, }, { name: "mixed transaction ordering with multiple effective tips", @@ -210,10 +186,6 @@ func (s *IntegrationTestSuite) TestTransactionOrderingWithABCIMethodCalls() { }) s.Require().NoError(err) - if tc.bypass { - return - } - // Check whether expected transactions are included and returned as pending state in mempool mpool := s.network.App.GetMempool() iterator := mpool.Select(s.network.GetContext(), nil) @@ -242,7 +214,6 @@ func (s *IntegrationTestSuite) TestNonceGappedEVMTransactionsWithABCIMethodCalls name string setupTxs func() ([]sdk.Tx, []string) // Returns transactions and their expected nonces verifyFunc func(mpool mempool.Mempool) - bypass bool }{ { name: "insert transactions with nonce gaps", @@ -396,7 +367,6 @@ func (s *IntegrationTestSuite) TestNonceGappedEVMTransactionsWithABCIMethodCalls count := mpool.CountTx() s.Require().Equal(2, count, "After replacement, both transactions should be pending") }, - bypass: true, }, } @@ -424,10 +394,6 @@ func (s *IntegrationTestSuite) TestNonceGappedEVMTransactionsWithABCIMethodCalls mpool := s.network.App.GetMempool() iterator := mpool.Select(s.network.GetContext(), nil) - if tc.bypass { - return - } - // Check whether expected transactions are included and returned as pending state in mempool for _, txHash := range expTxHashes { actualTxHash := s.getTxHash(iterator.Tx()) @@ -447,3 +413,94 @@ func (s *IntegrationTestSuite) TestNonceGappedEVMTransactionsWithABCIMethodCalls }) } } + +// TestCheckTxHandlerForCommittedAndLowerNonceTxs tests that: +// 1. Committed transactions are not in the mempool after block finalization +// 2. New transactions with nonces lower than current nonce fail at mempool level +func (s *IntegrationTestSuite) TestCheckTxHandlerForCommittedAndLowerNonceTxs() { + testCases := []struct { + name string + setupTxs func() []sdk.Tx + verifyFunc func() + }{ + { + name: "EVM transactions: committed txs removed from mempool and lower nonce txs fail", + setupTxs: func() []sdk.Tx { + key := s.keyring.GetKey(0) + + // Create transactions with sequential nonces (0, 1, 2) + tx0 := s.createEVMValueTransferTx(key, 0, big.NewInt(2000000000)) + tx1 := s.createEVMValueTransferTx(key, 1, big.NewInt(2000000000)) + tx2 := s.createEVMValueTransferTx(key, 2, big.NewInt(2000000000)) + + return []sdk.Tx{tx0, tx1, tx2} + }, + verifyFunc: func() { + // 1. Verify the correct nonce transaction is in mempool + mpool := s.network.App.GetMempool() + s.Require().Equal(0, mpool.CountTx(), "Only the correct nonce transaction should be in mempool") + + // 2. Check current sequence + acc := s.network.App.GetAccountKeeper().GetAccount(s.network.GetContext(), s.keyring.GetAccAddr(0)) + sequence := acc.GetSequence() + s.Require().Equal(uint64(3), sequence) + + // 3. Check new transactions with nonces lower than current nonce fails + // Current nonce should be 3 after committing nonces 1, 2 + // + // NOTE: The reason we don't try tx with nonce 0 is + // because txFactory replace nonce 0 with curreent nonce. + // So we just test for nonce 1 and 2. + key := s.keyring.GetKey(0) + + // Try to add transaction with nonce 1 (lower than current nonce 3) - should fail + dupTx1 := s.createEVMValueTransferTx(key, 1, big.NewInt(2000000000)) + res, err := s.checkTx(dupTx1) + s.Require().NoError(err, "Transaction with nonce 1 should fail when current nonce is 3") + s.Require().Contains(res.GetLog(), core.ErrNonceTooLow.Error()) + s.Require().Equal(0, mpool.CountTx(), "Only the correct nonce transaction should be in mempool") + + // Try to add transaction with nonce 2 (lower than current nonce 3) - should fail + dupTx2 := s.createEVMValueTransferTx(key, 2, big.NewInt(2000000000)) + res, err = s.checkTx(dupTx2) + s.Require().NoError(err, "Transaction with nonce 2 should fail when current nonce is 3") + s.Require().Contains(res.GetLog(), core.ErrNonceTooLow.Error()) + s.Require().Equal(0, mpool.CountTx(), "Only the correct nonce transaction should be in mempool") + + // Verify transaction with correct nonce (3) still works + tx3 := s.createEVMValueTransferTx(key, 3, big.NewInt(2000000000)) + res, err = s.checkTx(tx3) + s.Require().NoError(err, "Transaction with correct nonce 3 should succeed") + s.Require().Equal(abci.CodeTypeOK, res.Code) + s.Require().Equal(1, mpool.CountTx(), "Only the correct nonce transaction should be in mempool") + }, + }, + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + // Reset test setup to ensure clean state + s.SetupTest() + + txs := tc.setupTxs() + + // Call CheckTx for transactions + err := s.checkTxs(txs) + s.Require().NoError(err) + + // Finalize block with txs and Commit state + txBytes, err := s.getTxBytes(txs) + s.Require().NoError(err) + + _, err = s.network.NextBlockWithTxs(txBytes...) + s.Require().NoError(err) + + // Manually trigger chain head event to notify mempool about the new block + // This simulates the natural block notification that occurs in production + s.notifyNewBlockToMempool() + + // Run verification function + tc.verifyFunc() + }) + } +}