8000 perf(consensus): add simplistic block validation cache by sergio-mena · Pull Request #3070 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

perf(consensus): add simplistic block validation cache #3070

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 14, 2024
Merged

perf(consensus): add simplistic block validation cache #3070

merged 3 commits into from
May 14, 2024

Conversation

sergio-mena
Copy link
Contributor

Closes #2854

Follow up from #2960

This PR adds a simplistic 1-element block validation cache in BlockExecutor. This addresses a performance bottleneck raised as part of #2854

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

@sergio-mena sergio-mena requested review from a team as code owners May 13, 2024 13:06
@sergio-mena
Copy link
Contributor Author
sergio-mena commented May 13, 2024 •
edited
Loading

@ValarDragon could you please validate this approach is doing the job performance-wise? Thanks!

cason
cason approved these changes May 13, 2024
View reviewed changes
Copy link
Contributor
@cason cason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple yet powerful. Thanks :)

@sergio-mena sergio-mena self-assigned this May 13, 2024
ValarDragon
ValarDragon approved these changes May 13, 2024
View reviewed changes
Copy link
Contributor
@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, looks great to me!

@sergio-mena sergio-mena added this pull request to the merge queue May 14, 2024
Merged via the queue into main with commit c263abb May 14, 2024
35 checks passed
@sergio-mena sergio-mena deleted the sergio/2854-add-validation-cache branch May 14, 2024 09:21
@melekes
Copy link
Contributor
melekes commented May 14, 2024

Do we want to backport this?

@sergio-mena
Copy link
Contributor Author
sergio-mena commented May 14, 2024 •
edited
Loading

@melekes AFAIU, @ValarDragon and team need this, but they are cherry-picking to their fork (they just want to know we're up-streaming these changes, so they'll eventually disappear from their fork; @ValarDragon please chime in if that's not the case). So I see no point in backporting to v0.37. or v0.38.x. As for v1.x we are about to cut RC1 (we are long past "code freeze", which we reached when we carried out QA), so backports to v1.x should be done under the same conditions as the older branches at this point.

Changes to consensus are delicate, I'd prefer to backport only if needed (bug, security issue, etc) or if totally harmless (doc, comments, logs, etc).

@ValarDragon
Copy link
Contributor

I'd like to get this backported to v0.38.x if folks are fine with this. Our standing plan was to do an SDK 50 upgrade and be pretty on par with v0.38.x . (I am not caught up on whats the latency / breaking behavior of v1.x vs v0.38.x)

ValarDragon pushed a commit to osmosis-labs/cometbft that referenced this pull request May 25, 2024
@sergio-mena @andynog @ValarDragon
perf(consensus): add simplistic block validation cache (cometbft#3070)
2dd4e03 
Closes cometbft#2854

Follow up from cometbft#2960

This PR adds a simplistic 1-element block validation cache in
`BlockExecutor`. This addresses a performance bottleneck raised as part
of cometbft#2854

---

- [ ] 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

---------

Co-authored-by: Andy Nogueira <me@andynogueira.dev>
ValarDragon added a commit to osmosis-labs/cometbft that referenced this pull request May 25, 2024
@ValarDragon
Merge pull request #73 from osmosis-labs/bp/sergio/2854-add-validatio…
bdbed26 
…n-cache

perf(consensus): add simplistic block validation cache (cometbft#3070)
@sergio-mena
Copy link
Contributor Author
sergio-mena commented May 27, 2024 •
edited
Loading

@ValarDragon For backporting changes -- that are not bugs, or security related -- to release branches, our policy is to backport to the corresponding *-experimental branch. In the case of v0.38.x, the branch is v0.38.x-experimental. The difference is that we do not guarantee that the code changes landing on the *-experimental branch underwent any quality assurance (IOW, the *-experimental branches are not considered code-frozen). The advantage is that it's us who carry out the backport (solving conflicts, adapting where necessary) rather than the users.

Is backporting this to v0.38.x-experimental good enough for you folks?

@ValarDragon
Copy link
Contributor

Yes!

mergify bot pushed a commit to osmosis-labs/cometbft that referenced this pull request May 28, 2024
@sergio-mena @andynog @mergify
perf(consensus): add simplistic block validation cache (cometbft#3070)
f9bb4e4 
Closes cometbft#2854

Follow up from cometbft#2960

This PR adds a simplistic 1-element block validation cache in
`BlockExecutor`. This addresses a performance bottleneck raised as part
of cometbft#2854

---

- [ ] 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

---------

Co-authored-by: Andy Nogueira <me@andynogueira.dev>
(cherry picked from commit 2dd4e03)
PaddyMc pushed a commit to osmosis-labs/cometbft that referenced this pull request May 28, 2024
@mergify @sergio-mena
perf(consensus): add simplistic block validation cache (cometbft#3070) (
a17c8bc 
#78)

Closes cometbft#2854

Follow up from cometbft#2960

This PR adds a simplistic 1-element block validation cache in
`BlockExecutor`. This addresses a performance bottleneck raised as part
of cometbft#2854

---

- [ ] 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

---------

Co-authored-by: Andy Nogueira <me@andynogueira.dev>
(cherry picked from commit 2dd4e03)

Co-authored-by: Sergio Mena <sergio@informal.systems>
@ValarDragon
Copy link
Contributor

In a patch release and working great!

PaddyMc pushed a commit to osmosis-labs/cometbft that referenced this pull request Aug 19, 2024
@sergio-mena @andynog @PaddyMc
perf(consensus): add simplistic block validation cache (cometbft#3070)
8ff84c6 
Closes cometbft#2854

Follow up from cometbft#2960

This PR adds a simplistic 1-element block validation cache in
`BlockExecutor`. This addresses a performance bottleneck raised as part
of cometbft#2854

---

- [ ] 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

---------

Co-authored-by: Andy Nogueira <me@andynogueira.dev>
ValarDragon pushed a commit to osmosis-labs/cometbft that referenced this pull request Aug 19, 2024
@PaddyMc @sergio-mena @andynog
perf(consensus): add simplistic block validation cache (cometbft#3070) (
8b6562b 
#133)

Closes cometbft#2854

Follow up from cometbft#2960

This PR adds a simplistic 1-element block validation cache in
`BlockExecutor`. This addresses a performance bottleneck raised as part
of cometbft#2854

---

- [ ] 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

---------

Co-authored-by: Sergio Mena <sergio@informal.systems>
Co-authored-by: Andy Nogueira <me@andynogueira.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cason cason cason approved these changes

@ValarDragon ValarDragon ValarDragon approved these changes

Assignees

@sergio-mena sergio-mena

Labels
consensus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

enterPrecommit is almost always double verifying the block
5 participants
@sergio-mena @melekes @ValarDragon @cason @andynog
0