Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mempool needs more acceptance checks #329

Closed
jchappelow opened this issue Oct 2, 2023 · 4 comments · Fixed by #338
Closed

mempool needs more acceptance checks #329

jchappelow opened this issue Oct 2, 2023 · 4 comments · Fixed by #338

Comments

@jchappelow
Copy link
Member

jchappelow commented Oct 2, 2023

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 #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 jchappelow self-assigned this Oct 2, 2023
@jchappelow
Copy link
Member Author

@charithabandi I made some comments on CheckTx in the branch where I'm working on #330

5e44cea

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

@jchappelow
Copy link
Member Author

jchappelow commented Oct 2, 2023

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.

@jchappelow jchappelow removed their assignment Oct 2, 2023
@charithabandi charithabandi linked a pull request Oct 5, 2023 that will close this issue
@charithabandi
Copy link
Contributor

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
Copy link
Member Author

jchappelow commented Oct 5, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants