Mempool Rechecking all txs blocks consensus #2925

ValarDragon · 2024-04-29T06:23:59Z

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

The text was updated successfully, but these errors were encountered:

andynog · 2024-04-29T20:37:24Z

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 · 2024-05-01T11:51:51Z

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 · 2024-05-01T11:53:20Z

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

ValarDragon · 2024-05-02T18:36:56Z

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 · 2024-05-10T13:57:45Z

Should we add this to #2803?

cason · 2024-05-10T13:58:31Z

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 · 2024-05-10T14:02:09Z

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 · 2024-05-10T14:03:11Z

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

First step to fixing #2925 PR'ing this to see if we have any test failures. Note that this is safe in the happy path, as Reap and CheckTx both share this same lock. The functionality behavior is that: - Full nodes and non-proposers `timeout_prevote` beginning should not block on updating the mempool - Block proposers get _very slight_ increased concurrency before reaping their next block. (Should be significantly fixed in subsequent PR's in #2925) - Reap takes a lock on the mempool mutex, so there is no concurrency safety issues right now. - Mempool errors will not halt consensus, instead they just log an error and call mempool flush. I actually think this may be better behavior? If we want to preserve the old behavior, we can thread a generic "consensus halt error" channel perhaps? I'm not sure how/where to best document this. Please also let me know if tests need creating. Seems like the create empty block tests sometimes hit failures, I'll investigate tmrw Also please feel free to take over this PR, just thought I"d make it to help us with performance improvements. Happy to get this into an experimental release to test on mainnets. --- #### PR checklist - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- C 8000 o-authored-by: Sergio Mena <sergio@informal.systems>

First step to fixing cometbft#2925 PR'ing this to see if we have any test failures. Note that this is safe in the happy path, as Reap and CheckTx both share this same lock. The functionality behavior is that: - Full nodes and non-proposers `timeout_prevote` beginning should not block on updating the mempool - Block proposers get _very slight_ increased concurrency before reaping their next block. (Should be significantly fixed in subsequent PR's in - Reap takes a lock on the mempool mutex, so there is no concurrency safety issues right now. - Mempool errors will not halt consensus, instead they just log an error and call mempool flush. I actually think this may be better behavior? If we want to preserve the old behavior, we can thread a generic "consensus halt error" channel perhaps? I'm not sure how/where to best document this. Please also let me know if tests need creating. Seems like the create empty block tests sometimes hit failures, I'll investigate tmrw Also please feel free to take over this PR, just thought I"d make it to help us with performance improvements. Happy to get this into an experimental release to test on mainnets. --- - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Sergio Mena <sergio@informal.systems>

First step to fixing cometbft#2925 PR'ing this to see if we have any test failures. Note that this is safe in the happy path, as Reap and CheckTx both share this same lock. The functionality behavior is that: - Full nodes and non-proposers `timeout_prevote` beginning should not block on updating the mempool - Block proposers get _very slight_ increased concurrency before reaping their next block. (Should be significantly fixed in subsequent PR's in - Reap takes a lock on the mempool mutex, so there is no concurrency safety issues right now. - Mempool errors will not halt consensus, instead they just log an error and call mempool flush. I actually think this may be better behavior? If we want to preserve the old behavior, we can thread a generic "consensus halt error" channel perhaps? I'm not sure how/where to best document this. Please also let me know if tests need creating. Seems like the create empty block tests sometimes hit failures, I'll investigate tmrw Also please feel free to take over this PR, just thought I"d make it to help us with performance improvements. Happy to get this into an experimental release to test on mainnets. --- - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Sergio Mena <sergio@informal.systems>

First step to fixing cometbft#2925 PR'ing this to see if we have any test failures. Note that this is safe in the happy path, as Reap and CheckTx both share this same lock. The functionality behavior is that: - Full nodes and non-proposers `timeout_prevote` beginning should not block on updating the mempool - Block proposers get _very slight_ increased concurrency before reaping their next block. (Should be significantly fixed in subsequent PR's in - Reap takes a lock on the mempool mutex, so there is no concurrency safety issues right now. - Mempool errors will not halt consensus, instead they just log an error and call mempool flush. I actually think this may be better behavior? If we want to preserve the old behavior, we can thread a generic "consensus halt error" channel perhaps? I'm not sure how/where to best document this. Please also let me know if tests need creating. Seems like the create empty block tests sometimes hit failures, I'll investigate tmrw Also please feel free to take over this PR, just thought I"d make it to help us with performance improvements. Happy to get this into an experimental release to test on mainnets. --- - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Sergio Mena <sergio@informal.systems> (cherry picked from commit 2cea495)

… (#71) First step to fixing cometbft#2925 PR'ing this to see if we have any test failures. Note that this is safe in the happy path, as Reap and CheckTx both share this same lock. The functionality behavior is that: - Full nodes and non-proposers `timeout_prevote` beginning should not block on updating the mempool - Block proposers get _very slight_ increased concurrency before reaping their next block. (Should be significantly fixed in subsequent PR's in - Reap takes a lock on the mempool mutex, so there is no concurrency safety issues right now. - Mempool errors will not halt consensus, instead they just log an error and call mempool flush. I actually think this may be better behavior? If we want to preserve the old behavior, we can thread a generic "consensus halt error" channel perhaps? I'm not sure how/where to best document this. Please also let me know if tests need creating. Seems like the create empty block tests sometimes hit failures, I'll investigate tmrw Also please feel free to take over this PR, just thought I"d make it to help us with performance improvements. Happy to get this into an experimental release to test on mainnets. --- - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Sergio Mena <sergio@informal.systems> (cherry picked from commit 2cea495) Co-authored-by: PaddyMc <paddymchale@hotmail.com>

First step to fixing cometbft#2925 PR'ing this to see if we have any test failures. Note that this is safe in the happy path, as Reap and CheckTx both share this same lock. The functionality behavior is that: - Full nodes and non-proposers `timeout_prevote` beginning should not block on updating the mempool - Block proposers get _very slight_ increased concurrency before reaping their next block. (Should be significantly fixed in subsequent PR's in - Reap takes a lock on the mempool mutex, so there is no concurrency safety issues right now. - Mempool errors will not halt consensus, instead they just log an error and call mempool flush. I actually think this may be better behavior? If we want to preserve the old behavior, we can thread a generic "consensus halt error" channel perhaps? I'm not sure how/where to best document this. Please also let me know if tests need creating. Seems like the create empty block tests sometimes hit failures, I'll investigate tmrw Also please feel free to take over this PR, just thought I"d make it to help us with performance improvements. Happy to get this into an experimental release to test on mainnets. --- - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Sergio Mena <sergio@informal.systems>

cason · 2024-06-27T09:15:45Z

There is the additional problem which is:

The mempool is locked for re-checking (should it really be?)
New transactions received by the mempool reactor are not processed, waiting for a lock
Other messages coming from the same peer as the transactions in 2. are not processed, because the mempool is blocking the channel

So, the mempool channel should not block (as in #2685) because we are waiting for something that may take a lot to finish. This is a problem in general.

Then, the second problem: if re-check can be slow, it cannot block consensus/block execution and the mempool.

First step to fixing #2925 PR'ing this to see if we have any test failures. Note that this is safe in the happy path, as Reap and CheckTx both share this same lock. The functionality behavior is that: - Full nodes and non-proposers `timeout_prevote` beginning should not block on updating the mempool - Block proposers get _very slight_ increased concurrency before reaping their next block. (Should be significantly fixed in subsequent PR's in #2925) - Reap takes a lock on the mempool mutex, so there is no concurrency safety issues right now. - Mempool errors will not halt consensus, instead they just log an error and call mempool flush. I actually think this may be better behavior? If we want to preserve the old behavior, we can thread a generic "consensus halt error" channel perhaps? I'm not sure how/where to best document this. Please also let me know if tests need creating. Seems like the create empty block tests sometimes hit failures, I'll investigate tmrw Also please feel free to take over this PR, just thought I"d make it to help us with performance improvements. Happy to get this into an experimental release to test on mainnets. --- #### PR checklist - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Sergio Mena <sergio@informal.systems> (cherry picked from commit 1c277c0) # Conflicts: # state/execution.go

First step to fixing #2925 PR'ing this to see if we have any test failures. Note that this is safe in the happy path, as Reap and CheckTx both share this same lock. The functionality behavior is that: - Full nodes and non-proposers `timeout_prevote` beginning should not block on updating the mempool - Block proposers get _very slight_ increased concurrency before reaping their next block. (Should be significantly fixed in subsequent PR's in - Reap takes a lock on the mempool mutex, so there is no concurrency safety issues right now. - Mempool errors will not halt consensus, instead they just log an error and call mempool flush. I actually think this may be better behavior? If we want to preserve the old behavior, we can thread a generic "consensus halt error" channel perhaps? I'm not sure how/where to best document this. Please also let me know if tests need creating. Seems like the create empty block tests sometimes hit failures, I'll investigate tmrw Also please feel free to take over this PR, just thought I"d make it to help us with performance improvements. Happy to get this into an experimental release to test on mainnets. --- - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Sergio Mena <sergio@informal.systems>

…3362) First step to fixing #2925 PR'ing this to see if we have any test failures. Note that this is safe in the happy path, as Reap and CheckTx both share this same lock. The functionality behavior is that: - Full nodes and non-proposers `timeout_prevote` beginning should not block on updating the mempool - Block proposers get _very slight_ increased concurrency before reaping their next block. (Should be significantly fixed in subsequent PR's in #2925) - Reap takes a lock on the mempool mutex, so there is no concurrency safety issues right now. - Mempool errors will not halt consensus, instead they just log an error and call mempool flush. I actually think this may be better behavior? If we want to preserve the old behavior, we can thread a generic "consensus halt error" channel perhaps? I'm not sure how/where to best document this. Please also let me know if tests need creating. Seems like the create empty block tests sometimes hit failures, I'll investigate tmrw Also please feel free to take over this PR, just thought I"d make it to help us with performance improvements. Happy to get this into an experimental release to test on mainnets. --- #### PR checklist - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3008 done by [Mergify](https://mergify.com). --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Sergio Mena <sergio@informal.systems>

First step to fixing cometbft#2925 PR'ing this to see if we have any test failures. Note that this is safe in the happy path, as Reap and CheckTx both share this same lock. The functionality behavior is that: - Full nodes and non-proposers `timeout_prevote` beginning should not block on updating the mempool - Block proposers get _very slight_ increased concurrency before reaping their next block. (Should be significantly fixed in subsequent PR's in - Reap takes a lock on the mempool mutex, so there is no concurrency safety issues right now. - Mempool errors will not halt consensus, instead they just log an error and call mempool flush. I actually think this may be better behavior? If we want to preserve the old behavior, we can thread a generic "consensus halt error" channel perhaps? I'm not sure how/where to best document this. Please also let me know if tests need creating. Seems like the create empty block tests sometimes hit failures, I'll investigate tmrw Also please feel free to take over this PR, just thought I"d make it to help us with performance improvements. Happy to get this into an experimental release to test on mainnets. --- - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Sergio Mena <sergio@informal.systems>

First step to fixing cometbft#2925 PR'ing this to see if we have any test failures. Note that this is safe in the happy path, as Reap and CheckTx both share this same lock. The functionality behavior is that: - Full nodes and non-proposers `timeout_prevote` beginning should not block on updating the mempool - Block proposers get _very slight_ increased concurrency before reaping their next block. (Should be significantly fixed in subsequent PR's in - Reap takes a lock on the mempool mutex, so there is no concurrency safety issues right now. - Mempool errors will not halt consensus, instead they just log an error and call mempool flush. I actually think this may be better behavior? If we want to preserve the old behavior, we can thread a generic "consensus halt error" channel perhaps? I'm not sure how/where to best document this. Please also let me know if tests need creating. Seems like the create empty block tests sometimes hit failures, I'll investigate tmrw Also please feel free to take over this PR, just thought I"d make it to help us with performance improvements. Happy to get this into an experimental release to test on mainnets. --- - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Sergio Mena <sergio@informal.systems>

…3008) (cometbft#3362) First step to fixing cometbft#2925 PR'ing this to see if we have any test failures. Note that this is safe in the happy path, as Reap and CheckTx both share this same lock. The functionality behavior is that: - Full nodes and non-proposers `timeout_prevote` beginning should not block on updating the mempool - Block proposers get _very slight_ increased concurrency before reaping their next block. (Should be significantly fixed in subsequent PR's in - Reap takes a lock on the mempool mutex, so there is no concurrency safety issues right now. - Mempool errors will not halt consensus, instead they just log an error and call mempool flush. I actually think this may be better behavior? If we want to preserve the old behavior, we can thread a generic "consensus halt error" channel perhaps? I'm not sure how/where to best document this. Please also let me know if tests need creating. Seems like the create empty block tests sometimes hit failures, I'll investigate tmrw Also please feel free to take over this PR, just thought I"d make it to help us with performance improvements. Happy to get this into an experimental release to test on mainnets. --- - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request cometbft#3008 done by [Mergify](https://mergify.com). --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Sergio Mena <sergio@informal.systems>

ValarDragon added enhancement New feature or request needs-triage This issue/PR has not yet been triaged by the team. labels Apr 29, 2024

ValarDragon mentioned this issue Apr 29, 2024

perf: Make recheck not re-run validate basic cosmos/cosmos-sdk#20208

Merged

12 tasks

andynog added mempool and removed needs-triage This issue/PR has not yet been triaged by the team. labels Apr 29, 2024

andynog added this to CometBFT Apr 29, 2024

github-project-automation bot moved this to Todo in CometBFT Apr 29, 2024

ValarDragon mentioned this issue May 6, 2024

perf: Make mempool update async from block.Commit #3008

Merged

4 tasks

ValarDragon mentioned this issue May 23, 2024

perf: Make mempool recheck state only use an atomic for num rechecked txs #3112

Closed

4 tasks

PaddyMc mentioned this issue May 23, 2024

perf: Make mempool update async from block.Commit (#3008) osmosis-labs/cometbft#69

Merged

7 tasks

mergify bot mentioned this issue May 23, 2024

perf: Make mempool update async from block.Commit (#3008) (backport #69) osmosis-labs/cometbft#71

Merged

7 tasks

ValarDragon mentioned this issue May 24, 2024

perf: Make mempool reap be able to occur once Update has succesfully updated enough txs #3115

Closed

4 tasks

hvanz mentioned this issue Jun 26, 2024

perf(mempool): disable rechecking when node is catching up #3343

Closed

4 tasks

mergify bot mentioned this issue Jun 28, 2024

perf: Make mempool update async from block.Commit (backport #3008) #3362

Merged

4 tasks

cason mentioned this issue Jul 2, 2024

Node starting from BlockSync may never catch up to latest height #3398

Open

PaddyMc mentioned this issue Aug 19, 2024

perf: Make mempool update async from block.Commit (#3008) osmosis-labs/cometbft#131

Merged

7 tasks

yihuang mentioned this issue Oct 23, 2024

perf: Make mempool update async from block.Commit (backport #3008) crypto-org-chain/cometbft#3

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Mempool Rechecking all txs blocks consensus #2925

Mempool Rechecking all txs blocks consensus #2925

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Mempool Rechecking all txs blocks consensus #2925

Mempool Rechecking all txs blocks consensus #2925

Comments

Uh oh!

Feature Request

Summary

Problem Definition

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!