perf(consensus): align calls to `ValidateBlock`, `ProcessProposal` and `isTimely` by sergio-mena · Pull Request #2960 · cometbft/cometbft

sergio-mena · 2024-05-01T10:31:59Z

~~This change is an implementation of the alignment of ValidateBlock to ProcessProposal explained here.~~

We need to:

Check if that helps, perf-wise
Carefully make sure we're not compromising algorithm's correctness

EDIT:

In the end, we are implementing this as agreed here

EDIT2:

This PR will just deal with alignment of ValidateBlock, ProcessProposal and isTimely (so it's no longer closing #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

internal/consensus/state.go

adizere · 2024-05-01T14:38:43Z

Removed from project board & Milestone b/c it's tracked already under #2854

cason

I am not 100% positive about this approach, as it would allow nodes to commit a block they deem invalid if 2/3+ validators have voted for the block, where in some scenarios we could have more than 1/3 validators that are faulty (e.g., with a validation bug).

internal/consensus/state.go

sergio-mena · 2024-05-13T10:48:21Z

See this comment for an explanation of the latest commits

cason

This PR now has 3 contributions:

On prevote step, only validate blocks when they are new (POLRound == -1) and we are not locked
The same for the timely validation performed as part of PBTS
Minimal chaching mechanism for block validation

I think the PR should implement 1. and 2., with description updated to reflect this change. In this case it is not really a performance improvement, as there is no change or benefit in the good case (single round).

I would leave 3. for another PR.

state/execution.go

sergio-mena · 2024-05-13T12:56:44Z

This PR now has 3 contributions:
1. On prevote step, only validate blocks when they are new (`POLRound == -1`) and we are not locked

2. The same for the timely validation performed as part of PBTS

3. Minimal chaching mechanism for block validation
I think the PR should implement 1. and 2., with description updated to reflect this change. In this case it is not really a performance improvement, as there is no change or benefit in the good case (single round).

I would leave 3. for another PR.

@cason OK, I'll split this into two PRs. An extra PR is cheap

This reverts commit 5cf4b29.

cason

Good refactoring :)

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

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>

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)

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

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>

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

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

Align calls to ValidateBlock to ProcessProposal

f95c754

sergio-mena requested a review from a team as a code owner May 1, 2024 10:32

sergio-mena requested a review from a team May 1, 2024 10:32

sergio-mena self-assigned this May 1, 2024

sergio-mena added this to the 2024-Q2 milestone May 1, 2024

sergio-mena changed the title ~~Align calls to ValidateBlock to ProcessProposal~~ perf(consensus): align calls to ValidateBlock to ProcessProposal May 1, 2024

sergio-mena mentioned this pull request May 1, 2024

enterPrecommit is almost always double verifying the block #2854

Closed

sergio-mena commented May 1, 2024

View reviewed changes

internal/consensus/state.go Show resolved Hide resolved

adizere removed this from the 2024-Q2 milestone May 1, 2024

cason reviewed May 8, 2024

View reviewed changes

sergio-mena added 4 commits May 13, 2024 12:10

Revert changes on enterPrecommit and finalizeCommit

b8d4a9c

Moved proposalIsTimely check as well

de460d7

Simplify "isTimely" checks

4607717

Add 1-element cache of validated blocks

5cf4b29

cason reviewed May 13, 2024

View reviewed changes

state/execution.go Show resolved Hide resolved

state/execution.go Outdated Show resolved Hide resolved

sergio-mena changed the title ~~perf(consensus): align calls to ValidateBlock to ProcessProposal~~ perf(consensus): align calls to ValidateBlock, ProcessProposal and isTimely + add simplistic block validation cache May 13, 2024

sergio-mena changed the title ~~perf(consensus): align calls to ValidateBlock, ProcessProposal and isTimely + add simplistic block validation cache~~ perf(consensus): align calls to ValidateBlock, ProcessProposal and isTimely May 13, 2024

Revert "Add 1-element cache of validated blocks"

c569d72

This reverts commit 5cf4b29.

cason approved these changes May 13, 2024

View reviewed changes

sergio-mena mentioned this pull request May 13, 2024

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

Merged

4 tasks

sergio-mena added this pull request to the merge queue May 13, 2024

sergio-mena added the consensus label May 13, 2024

Merged via the queue into main with commit f8c369d May 13, 2024

sergio-mena deleted the sergio/2854-align-validateblock-to-procprop branch May 13, 2024 13:23

ValarDragon mentioned this pull request May 25, 2024

perf(consensus): add simplistic block validation cache (#3070) osmosis-labs/cometbft#73

Merged

7 tasks

mergify bot mentioned this pull request May 28, 2024

perf(consensus): add simplistic block validation cache (#3070) (backport #73) osmosis-labs/cometbft#78

Merged

7 tasks

PaddyMc mentioned this pull request Aug 19, 2024

perf(consensus): add simplistic block validation cache (#3070) osmosis-labs/cometbft#133

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(consensus): align calls to `ValidateBlock`, `ProcessProposal` and `isTimely`#2960

perf(consensus): align calls to `ValidateBlock`, `ProcessProposal` and `isTimely`#2960
sergio-mena merged 6 commits intomainfrom
sergio/2854-align-validateblock-to-procprop

sergio-mena commented May 1, 2024 •

edited

Loading

Uh oh!

Uh oh!

adizere commented May 1, 2024

Uh oh!

cason left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sergio-mena commented May 13, 2024

Uh oh!

cason left a comment

Uh oh!

Uh oh!

Uh oh!

sergio-mena commented May 13, 2024

Uh oh!

cason left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

sergio-mena commented May 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

Uh oh!

adizere commented May 1, 2024

Uh oh!

cason left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sergio-mena commented May 13, 2024

Uh oh!

cason left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sergio-mena commented May 13, 2024

Uh oh!

cason left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sergio-mena commented May 1, 2024 •

edited

Loading