Skip to content

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

Merged
PaddyMc merged 1 commit intoosmo-v25/v0.37.4from
mergify/bp/osmo-v25/v0.37.4/pr-69
May 23, 2024
Merged

perf: Make mempool update async from block.Commit (#3008) (backport #69)#71
PaddyMc merged 1 commit intoosmo-v25/v0.37.4from
mergify/bp/osmo-v25/v0.37.4/pr-69

Conversation

@mergify
Copy link

@mergify mergify bot commented May 23, 2024

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
  • 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


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

This is an automatic backport of pull request #69 done by [Mergify](https://mergify.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: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Sergio Mena <sergio@informal.systems>
(cherry picked from commit 2cea495)
@PaddyMc PaddyMc merged commit 17609cf into osmo-v25/v0.37.4 May 23, 2024
@mergify mergify bot deleted the mergify/bp/osmo-v25/v0.37.4/pr-69 branch May 23, 2024 17:03
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 this pull request may close these issues.

1 participant