mempool needs more acceptance checks

[jchappelow]

CheckTx is the "Guardian of the mempool" (comet docs). This is the most likely attack vector on the p2p interface. Our current implementation of CheckTx is very minimal, and it will need more sophistication to prevent malicious users or peer node from flooding a node's mempool, causing resource exhaustion.

Mempool policy is unlike consensus in that node operators are free to accept or relay transactions however they see fit. We need to craft a more thorough mempool policy with DoS mitigation in mind.

CheckTx is also used to re-check all transactions that remain in mempool after a block is mined. It is critical to evict transactions that may become invalid. For instance, if a user submits a transaction with certain nonce, and then replaces it with another that gets mined, the first one should be evicted from mempool.

Related to https://github.com/kwilteam/kwil-db/issues/328.

A "mempool module" will likely be required to facilitate some or all of the checks. For instance, basic checks on an account's standing will be needed, particularly if gas may be disabled, since we do not simply want blocks to be filled by failed transactions at no cost to the attacker.

[jchappelow]

@charithabandi I made some comments on CheckTx in the branch where I'm working on https://github.com/kwilteam/kwil-db/issues/330

https://github.com/kwilteam/kwil-db/commit/5e44cea7011bd8eef3ed55b7c8c525a1e549cbc7

The PrepareProposal changes are completely WIP there, but wanted to link to thoughts on CheckTx.

[jchappelow]

Mempool module possible requirements

• Different operating modes depending on gas enabled/disabled i.e. different strategies for defending the node if there is no cost to txn spamming. With gas disabled, preventing failed transactions from being mined is necessarily the job of the mempool policies. With gas enabled, it is obviously more tolerable to have them in blocks.
• Knowledge of the current size of the mempool so that it can evict/purge transactions (when re-checking), or potentially reject incoming transactions. This is typically done if there are fee rate spikes on public networks, forcing low fee transactions to be dropped from mempool rather than sit there for days or weeks. Without gas, it's less clear what to do, but perhaps if a single sender is responsible for a very large portion of the mempool (if nearing capacity) new ones would fail to be accepted.
• The account store will be a dependency, for nonces and balances. Incoming transactions with a nonce that is <= the best nonce in a block should be rejected. With gas enabled, we would consider rejecting 0 fee txns. When could a new user (not in the account store) be a factor?
• The mempool module may track and cache the nonces of transactions in mempool. A user may submit multiple transactions that should be accepted as long as there are no gaps. It is normally permissible to accept a transaction with the same nonce as a tx already in mempool (but not in a block), however without gas we would not want to allow that since there is no criteria for selecting the one to mine (normally higher fee).
• Delivering transactions (as they are included in a block) will probably involve informing the mempool module so that if it was tracking unconfirmed nonces for user they can be updated or cleared.
• No state. Restarting a node flushes the mempool.

[charithabandi]

I think cometbft already handles the mempool size and purges or rejects transactions based on that. We probably don't need to handle it on the abci side. what do you think?

One other thing I can think of is, checking the block size constraints in the processProposal, especially if a proposer is adding its own transactions.

[jchappelow]

I think cometbft already handles the mempool size and purges or rejects transactions based on that. We probably don't need to handle it on the abci side. what do you think? Pretty sure we can punt on it for now, but how would it know what to purge? It doesn't know about gas or nonces, so it must be... oldest or newest? It might do something undesirable.

Yeah, I checked the code, it just ensures that each tx is within the configured limit, but not the block size. So it's on app to decide