fix(mempool)!: stop accepting TXs in the mempool if we can't keep up with reCheckTX by sergio-mena · Pull Request #3314 · cometbft/cometbft

sergio-mena · 2024-06-21T13:17:12Z

This PR is a combination of ideas from @ValarDragon, @hvanz and @sergio-mena to alleviate nodes that, while not having their mempool full "officially", they have too many TXs lingering in the mempool which causes them to fall behind.
The mechanism works as follows:

We mark when we start and end reChecking
If, by the time a new block is decided we are still running the previous reCheckTx, we declare the mempool as rechecktx-full
Otherwise, we declare the mempool as not rechecktx-full

We have tested this and it fixes the failing nightlies that are blocking us from cutting v1.0.0-rc1.

Some UTs need to be fixed, hence posting as draft for the moment.

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments
Title follows the Conventional Commits spec

mempool/errors.go

mempool/clist_mempool.go

Co-authored-by: Sergio Mena <sergio@informal.systems>

… into sergio/bound-rechecktx

mempool/clist_mempool.go

greg-szabo

I'd rather have Debug level for the newly introduced log message, because it's a bit cryptic for validators. For n end-user, I could consider a "mempool is soft full" message useful, although there's not much they can do about it.

melekes

Shouldn't we respect the Recheck config setting?

mempool/clist_mempool.go

@ValarDragon

…with reCheckTX (#3314) This PR is a combination of ideas from @ValarDragon, @hvanz and @sergio-mena to alleviate nodes that, while not having their mempool full "officially", they have too many TXs lingering in the mempool which causes them to fall behind. The mechanism works as follows: * We mark when we start and end reChecking * If, by the time a new block is decided we are still running the previous `reCheckTx`, we declare the mempool as rechecktx-full * Otherwise, we declare the mempool as not rechecktx-full We have tested this and it fixes the failing nightlies that are blocking us from cutting `v1.0.0-rc1`. Some UTs need to be fixed, hence posting as draft for the moment. --- #### 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: Andy Nogueira <me@andynogueira.dev> Co-authored-by: Dev Ojha <dojha@berkeley.edu> Co-authored-by: hvanz <hernan.vanzetto@gmail.com> Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com> (cherry picked from commit 0cd2907)

@ValarDragon

…with reCheckTX (#3314) This PR is a combination of ideas from @ValarDragon, @hvanz and @sergio-mena to alleviate nodes that, while not having their mempool full "officially", they have too many TXs lingering in the mempool which causes them to fall behind. The mechanism works as follows: * We mark when we start and end reChecking * If, by the time a new block is decided we are still running the previous `reCheckTx`, we declare the mempool as rechecktx-full * Otherwise, we declare the mempool as not rechecktx-full We have tested this and it fixes the failing nightlies that are blocking us from cutting `v1.0.0-rc1`. Some UTs need to be fixed, hence posting as draft for the moment. --- #### 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: Andy Nogueira <me@andynogueira.dev> Co-authored-by: Dev Ojha <dojha@berkeley.edu> Co-authored-by: hvanz <hernan.vanzetto@gmail.com> Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com> (cherry picked from commit 0cd2907) # Conflicts: # mempool/clist_mempool.go # mempool/clist_mempool_test.go

@ValarDragon

…with reCheckTX (#3314) This PR is a combination of ideas from @ValarDragon, @hvanz and @sergio-mena to alleviate nodes that, while not having their mempool full "officially", they have too many TXs lingering in the mempool which causes them to fall behind. The mechanism works as follows: * We mark when we start and end reChecking * If, by the time a new block is decided we are still running the previous `reCheckTx`, we declare the mempool as rechecktx-full * Otherwise, we declare the mempool as not rechecktx-full We have tested this and it fixes the failing nightlies that are blocking us from cutting `v1.0.0-rc1`. Some UTs need to be fixed, hence posting as draft for the moment. --- #### 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: Andy Nogueira <me@andynogueira.dev> Co-authored-by: Dev Ojha <dojha@berkeley.edu> Co-authored-by: hvanz <hernan.vanzetto@gmail.com> Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com> (cherry picked from commit 0cd2907) # Conflicts: # internal/consensus/replay_stubs.go # mempool/clist_mempool.go # mempool/clist_mempool_test.go # mempool/errors.go # mempool/reactor_test.go # state/execution.go # state/execution_test.go

@ValarDragon

…with reCheckTX (backport #3314) (#3336) This PR is a combination of ideas from @ValarDragon, @hvanz and @sergio-mena to alleviate nodes that, while not having their mempool full "officially", they have too many TXs lingering in the mempool which causes them to fall behind. The mechanism works as follows: * We mark when we start and end reChecking * If, by the time a new block is decided we are still running the previous `reCheckTx`, we declare the mempool as rechecktx-full * Otherwise, we declare the mempool as not rechecktx-full We have tested this and it fixes the failing nightlies that are blocking us from cutting `v1.0.0-rc1`. Some UTs need to be fixed, hence posting as draft for the moment. --- #### 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 #3314 done by [Mergify](https://mergify.com). Co-authored-by: Sergio Mena <sergio@informal.systems>

@ValarDragon

…with reCheckTX (#3314) This PR is a combination of ideas from @ValarDragon, @hvanz and @sergio-mena to alleviate nodes that, while not having their mempool full "officially", they have too many TXs lingering in the mempool which causes them to fall behind. The mechanism works as follows: * We mark when we start and end reChecking * If, by the time a new block is decided we are still running the previous `reCheckTx`, we declare the mempool as rechecktx-full * Otherwise, we declare the mempool as not rechecktx-full We have tested this and it fixes the failing nightlies that are blocking us from cutting `v1.0.0-rc1`. Some UTs need to be fixed, hence posting as draft for the moment. --- - [ ] 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: Andy Nogueira <me@andynogueira.dev> Co-authored-by: Dev Ojha <dojha@berkeley.edu> Co-authored-by: hvanz <hernan.vanzetto@gmail.com> Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com>

@ValarDragon

…with reCheckTX (backport #3314) (#3337) This PR is a combination of ideas from @ValarDragon, @hvanz and @sergio-mena to alleviate nodes that, while not having their mempool full "officially", they have too many TXs lingering in the mempool which causes them to fall behind. The mechanism works as follows: * We mark when we start and end reChecking * If, by the time a new block is decided we are still running the previous `reCheckTx`, we declare the mempool as rechecktx-full * Otherwise, we declare the mempool as not rechecktx-full We have tested this and it fixes the failing nightlies that are blocking us from cutting `v1.0.0-rc1`. Some UTs need to be fixed, hence posting as draft for the moment. --- #### 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 #3314 done by [Mergify](https://mergify.com). --------- Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: hvanz <hernan.vanzetto@gmail.com>

@ValarDragon

…with reCheckTX (backport #3314) (#3338) This PR is a combination of ideas from @ValarDragon, @hvanz and @sergio-mena to alleviate nodes that, while not having their mempool full "officially", they have too many TXs lingering in the mempool which causes them to fall behind. The mechanism works as follows: * We mark when we start and end reChecking * If, by the time a new block is decided we are still running the previous `reCheckTx`, we declare the mempool as rechecktx-full * Otherwise, we declare the mempool as not rechecktx-full We have tested this and it fixes the failing nightlies that are blocking us from cutting `v1.0.0-rc1`. Some UTs need to be fixed, hence posting as draft for the moment. --- #### 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 #3314 done by [Mergify](https://mergify.com). Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Andy Nogueira <me@andynogueira.dev> Co-authored-by: Dev Ojha <dojha@berkeley.edu> Co-authored-by: hvanz <hernan.vanzetto@gmail.com> Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com>

@ValarDragon

…with reCheckTX (backport cometbft#3314) (cometbft#3338) This PR is a combination of ideas from @ValarDragon, @hvanz and @sergio-mena to alleviate nodes that, while not having their mempool full "officially", they have too many TXs lingering in the mempool which causes them to fall behind. The mechanism works as follows: * We mark when we start and end reChecking * If, by the time a new block is decided we are still running the previous `reCheckTx`, we declare the mempool as rechecktx-full * Otherwise, we declare the mempool as not rechecktx-full We have tested this and it fixes the failing nightlies that are blocking us from cutting `v1.0.0-rc1`. Some UTs need to be fixed, hence posting as draft for the moment. --- #### 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 cometbft#3314 done by [Mergify](https://mergify.com). Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Andy Nogueira <me@andynogueira.dev> Co-authored-by: Dev Ojha <dojha@berkeley.edu> Co-authored-by: hvanz <hernan.vanzetto@gmail.com> Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com>

@ValarDragon

… with reCheckTX (backport cometbft#3314) cometbft#3338 (#117) * Revert "Add ResetUpdate, so non-block proposers no longer fail to sync (#113)" This reverts commit 158bcbe. * fix(mempool)!: stop accepting TXs in the mempool if we can't keep up with reCheckTX (backport cometbft#3314) (cometbft#3338) This PR is a combination of ideas from @ValarDragon, @hvanz and @sergio-mena to alleviate nodes that, while not having their mempool full "officially", they have too many TXs lingering in the mempool which causes them to fall behind. The mechanism works as follows: * We mark when we start and end reChecking * If, by the time a new block is decided we are still running the previous `reCheckTx`, we declare the mempool as rechecktx-full * Otherwise, we declare the mempool as not rechecktx-full We have tested this and it fixes the failing nightlies that are blocking us from cutting `v1.0.0-rc1`. Some UTs need to be fixed, hence posting as draft for the moment. --- #### 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 cometbft#3314 done by [Mergify](https://mergify.com). Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Andy Nogueira <me@andynogueira.dev> Co-authored-by: Dev Ojha <dojha@berkeley.edu> Co-authored-by: hvanz <hernan.vanzetto@gmail.com> Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com> --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Andy Nogueira <me@andynogueira.dev> Co-authored-by: hvanz <hernan.vanzetto@gmail.com> Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com>

…its logic to Lock (#3361) This PR partially reverts the backport of #3314 into the recently released `v0.38.8` (and `v0.37.7`). With this change the `Mempool` interface is the same as in previous versions. The reason is that we do not want to break the public API. We still keep in the code the feature that #3314 introduced by moving it inside the existing `Lock` method. We also keep the `RecheckFull bool` field that we added to `ErrMempoolIsFull`.  --- #### PR checklist - [ ] Tests written/updated - [ ] 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 - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

…its logic to Lock (#3363) Complements #3361 This PR partially reverts the backport of #3314 into the recently released v0.37.7. With this change the Mempool interface is the same as in previous versions. The reason is that we do not want to break the public API. We still keep in the code the feature that #3314 introduced by moving it inside the existing Lock method. We also keep the RecheckFull bool field that we added to ErrMempoolIsFull.  --- #### PR checklist - [ ] Tests written/updated - [ ] 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 - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

Complements #3314 This PR adds a new error with a more clear error message when checking if the mempool is full while rechecking is in progress after a new block was committed. --- #### PR checklist - [ ] Tests written/updated - [ ] 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 - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

Complements #3314 This PR adds a new error with a more clear error message when checking if the mempool is full while rechecking is in progress after a new block was committed. --- #### PR checklist - [ ] Tests written/updated - [ ] 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 - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit 987fa83)

Complements #3314 This PR adds a new error with a more clear error message when checking if the mempool is full while rechecking is in progress after a new block was committed. --- #### PR checklist - [ ] Tests written/updated - [ ] 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 - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit 987fa83) # Conflicts: # mempool/clist_mempool.go

Complements #3314 This PR adds a new error with a more clear error message when checking if the mempool is full while rechecking is in progress after a new block was committed. --- #### PR checklist - [ ] Tests written/updated - [ ] 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 - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3654 done by [Mergify](https://mergify.com). Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com>

Complements #3314 This PR adds a new error with a more clear error message when checking if the mempool is full while rechecking is in progress after a new block was committed. --- #### PR checklist - [ ] Tests written/updated - [ ] 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 - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3654 done by [Mergify](https://mergify.com). --------- Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com> Co-authored-by: hvanz <hernan.vanzetto@gmail.com>

andynog and others added 4 commits June 21, 2024 11:42

adding recheck saturation logic

2fc5174

Don't use RecheckTimeout

1d352b6

Add ResetUpdate (needs test)

8ee95b9

Combine both ideas: mempool full + check just before next update

5da779a

sergio-mena added bug Something isn't working mempool labels Jun 21, 2024

sergio-mena self-assigned this Jun 21, 2024

ValarDragon mentioned this pull request Jun 21, 2024

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

Closed

4 tasks

sergio-mena assigned andynog and unassigned sergio-mena Jun 21, 2024

sergio-mena added backport-to-v0.37.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x labels Jun 21, 2024

sergio-mena commented Jun 21, 2024

View reviewed changes

mempool/errors.go Outdated Show resolved Hide resolved

sergio-mena commented Jun 21, 2024

View reviewed changes