Mempool Rechecking all txs blocks consensus

[ValarDragon]

Feature Request

Summary

Rechecking all txs in the mempool seems to block consensus. We see this from:

• How pprof's look
• How the code appears

This is problematic as it means larger mempools will delay consensus longer. (Also IBC has a change that creeped in that is causing overly large expenses in RecheckTx)

Here is a copy of a pprof from a live-syncing Osmosis full node during 1 hour, with relatively average tx volume:

We see that it blocks Commit right here: https://github.com/cometbft/cometbft/blob/main/state/execution.go#L419-L426

If you look into the relevant code, each recheck call is actually synchronous due to how the callback's are structured.

Problem Definition

We should make the mempool rechecking not block BlockExecutor.ApplyBlock.

Ideally it should only be blocking ProposeBlock until either everything in the mempool is rechecked or blockGas worth of txs are rechecked. It should never be blocking for timeout_prevote

[andynog]

There are some refactoring on the re-checking logic happened in this PR #2268, maybe this will not solve the problem from the perspective @ValarDragon reported but just adding here for visibility. @hvanz might have a better opinion in this case.

The mempool recheck can also be controlled with a configuration parameter
https://docs.cometbft.com/v1.0/references/config/config.toml#mempoolrecheck

[hvanz]

Thanks @ValarDragon for reporting this problem! I agree that rechecking should not block consensus, though it will still need to block the mempool for checking new incoming transactions. This is for not breaking the FIFO ordering when rechecking txs.

Currently BlockExecutor.Commit calls mempool.Lock, mempool.FlushAppConn, mempool.Update and mempool.Unlock. It is the only function that calls these mempool methods. We have it in our list to refactor these calls to make the mempool code cleaner and more modular. We'll keep into account the problem you report when addressing the refactoring.

[hvanz]

Also, #2268 is about a corner case in the rechecking logic, not related to current issue.

[ValarDragon]

I think what we should do here is:

• Maintain statistics via Atomic instructions in the mempool for "how many bytes", "how many gas" is rechecked, and "are we done rechecking"
• Always run Update asynchronously. (Mempool updating should not block any consensus timeouts)
  • Update returns once the trackers are rechecked, and the recheck goroutine is started
• Make the Reap function poll/block until oneof:
  • "We are done rechecking" = true
  • "how many bytes rechecked" < Reap Bytes limit
  • "How much gas is rechecked" < Reap gas limit

[cason]

Should we add this to #2803?

[cason]

If you look into the relevant code, each recheck call is actually synchronous due to how the callback's are structured.

This is a problem by itself, we have probably to address it in another issue.

[cason]

As commented in #3008, we cannot call Commit before flushing the mempool. This is to ensure that all transactions with pending CheckTx are processed before calling Commit. The rationale here is that Commit should update the state used by CheckTx to the new state.

While we can discuss if this is the best way to go, this is the current contract with the ABCI application.

[cason]

The block execution should trigger the execution of Re-CheckTx but I agree that the re-checking should indeed be performed in parallel.