enterPrecommit is almost always double verifying the block #2854

ValarDragon · 2024-04-20T06:41:34Z

Feature Request

Summary

enterPrecommit almost always runs ValidateBlock, however in most cases this is duplicate logic as we have already prevoted the block. We only enter a lock after this line! This is the line where we run validate block: https://github.com/cometbft/cometbft/blob/main/internal/consensus/state.go#L1667

I suggest that we change this line here, to be one of two things:

Only validate the block after checking some cache for whether we have already validated this block.
Check "did we prevote this block", and only ran ValidateBlock if we did not.

Problem Definition

We see we have duplicate VerifyBlock logic on synced full nodes:

This operation is expensive, especially in consensus critical contexts!

Proposal

I think that making a cache for checking if we've already validated a block is better, as it seems easier to also avoid extra redundant validations for finalizeCommit. I haven't read that logic, but guessing by how all 3 of these validate block calls are pretty similar in the amount of time spent, I'm guessing its a redundant ValidateBlock as well.

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

cason · 2024-04-22T07:10:10Z

We had a discussion at some point regarding the multiple validations of the same block. I found this comment:

cometbft/knowledge-base#14 (comment)

cason · 2024-04-22T07:12:06Z

A similar discussion also here: #1391

cason · 2024-04-22T07:13:08Z

We need to separate the validation of a block by the state-machine/blockchain, and the validation of a block by the application. The first validation, whether a block can be appended to the blockchain, could indeed be cached.

ValarDragon · 2024-04-22T09:56:01Z

Thanks for the links to past discussions! This would be great to do for the comet side checks! Its easy enough to say its the app's job to handle it for app-side checks

ValarDragon · 2024-04-29T05:38:57Z

Just checked, we are indeed also doing an extra validate Block call in FinalizeCommit, that would be mitigated with a simple cache :)

ValarDragon · 2024-04-29T07:22:26Z

Ah we are actually double validating the block in FinalizeCommit -- though this one is super easy to fix using ApplyVerifiedBlock:

#2928)  Simplest component of #2854 We already run ValidateBlock in finalizeCommit, so this PR removes one extra redundant call by using ApplyVerifiedBlock. (The other call is also redundant, but that likely requires a more complex caching strategy as noted in #2854 to remedy) From my understanding of these benchmarks, at Osmosis 150 validators, this should be saving ~13ms of execution time per block. --- #### PR checklist - [X] Tests written/updated - Theres no test to update, as its impossible to reach the difference in codepaths! - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

#2928)  Simplest component of #2854 We already run ValidateBlock in finalizeCommit, so this PR removes one extra redundant call by using ApplyVerifiedBlock. (The other call is also redundant, but that likely requires a more complex caching strategy as noted in #2854 to remedy) From my understanding of these benchmarks, at Osmosis 150 validators, this should be saving ~13ms of execution time per block. --- #### PR checklist - [X] Tests written/updated - Theres no test to update, as its impossible to reach the difference in codepaths! - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit 94fa3c9)

#2928)  Simplest component of #2854 We already run ValidateBlock in finalizeCommit, so this PR removes one extra redundant call by using ApplyVerifiedBlock. (The other call is also redundant, but that likely requires a more complex caching strategy as noted in #2854 to remedy) From my understanding of these benchmarks, at Osmosis 150 validators, this should be saving ~13ms of execution time per block. --- #### PR checklist - [X] Tests written/updated - Theres no test to update, as its impossible to reach the difference in codepaths! - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit 94fa3c9) # Conflicts: # consensus/state.go

…k (backport #2928) (#2943) Simplest component of #2854 We already run ValidateBlock in finalizeCommit, so this PR removes one extra redundant call by using ApplyVerifiedBlock. (The other call is also redundant, but that likely requires a more complex caching strategy as noted in #2854 to remedy) From my understanding of these benchmarks, at Osmosis 150 validators, this should be saving ~13ms of execution time per block. --- #### PR checklist - [X] Tests written/updated - Theres no test to update, as its impossible to reach the difference in codepaths! - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] 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 #2928 done by [Mergify](https://mergify.com). Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

…k (backport #2928) (#2944) Simplest component of #2854 We already run ValidateBlock in finalizeCommit, so this PR removes one extra redundant call by using ApplyVerifiedBlock. (The other call is also redundant, but that likely requires a more complex caching strategy as noted in #2854 to remedy) From my understanding of these benchmarks, at Osmosis 150 validators, this should be saving ~13ms of execution time per block. --- #### PR checklist - [X] Tests written/updated - Theres no test to update, as its impossible to reach the difference in codepaths! - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] 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 #2928 done by [Mergify](https://mergify.com). --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

…k (backport #2928) (#2945) Simplest component of #2854 We already run ValidateBlock in finalizeCommit, so this PR removes one extra redundant call by using ApplyVerifiedBlock. (The other call is also redundant, but that likely requires a more complex caching strategy as noted in #2854 to remedy) From my understanding of these benchmarks, at Osmosis 150 validators, this should be saving ~13ms of execution time per block. --- #### PR checklist - [X] Tests written/updated - Theres no test to update, as its impossible to reach the difference in codepaths! - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] 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 #2928 done by [Mergify](https://mergify.com). --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

sergio-mena · 2024-05-01T10:26:39Z

Hi @ValarDragon @cason chiming in here

First of all, the caching mechanism that @ValarDragon is proposing shouldn't be hard to do, and would probably do the trick. So this is still and option.

However, going through @cason's old comments on the algorithm's valid(v) (which were made in the context of non-deterministic ProcessProposal, i.e., RFC 105) I think we can go further if we agree that the following is true:

valid(v) can be thought of as consisting of two parts: CometBFT-valid and App-valid. The former being ValidateBlock, the latter being ProcessProposal.
The modifications made to the Tendermint algorithm in RFC 105 are correct (in particular, the calls to valid(v) that were removed as part of RFC 105)

If we agree this is true, then we can conclude that CometBFT-valid can just be called where and only where App-valid is called; without affecting the algorithm's correctness.

If I didn't terribly miss anything in my reasoning, then the solution I would implement here is aligning ValidateBlock with ProcessProposal; IOW calling ValidateBlock only when we call ProcessProposal.

@cason what do you think?

sergio-mena · 2024-05-01T10:35:25Z

@ValarDragon I just posted #2960 for review. We need to wait for @cason to come back from vacation (he's back next Mon). I would like to have his opinion on correctness.
In the meantime, do you folks think you could measure if this PR improves the performance and how much? Thanks!

ValarDragon · 2024-05-01T17:34:30Z

Ah your right, that would argue that we can safely remove those two calls! That sounds good to me, will try to get it tested on mainnet soon! (@czarcas7ic is working on some automation for this, so that way we can also be getting benchmarks that adapt for the fixes we've already gotten in as well!)

From my understanding though, this would save those calls and be a notable time reduction!

…k (backport cometbft#2928) (cometbft#2945) Simplest component of cometbft#2854 We already run ValidateBlock in finalizeCommit, so this PR removes one extra redundant call by using ApplyVerifiedBlock. (The other call is also redundant, but that likely requires a more complex caching strategy as noted in cometbft#2854 to remedy) From my understanding of these benchmarks, at Osmosis 150 validators, this should be saving ~13ms of execution time per block. --- #### PR checklist - [X] Tests written/updated - Theres no test to update, as its impossible to reach the difference in codepaths! - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] 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#2928 done by [Mergify](https://mergify.com). --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

#39) * perf(consensus/state): Change finalizeCommit to use applyVerifiedBlock (backport cometbft#2928) (cometbft#2945) Simplest component of cometbft#2854 We already run ValidateBlock in finalizeCommit, so this PR removes one extra redundant call by using ApplyVerifiedBlock. (The other call is also redundant, but that likely requires a more complex caching strategy as noted in cometbft#2854 to remedy) From my understanding of these benchmarks, at Osmosis 150 validators, this should be saving ~13ms of execution time per block. --- #### PR checklist - [X] Tests written/updated - Theres no test to update, as its impossible to reach the difference in codepaths! - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] 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#2928 done by [Mergify](https://mergify.com). --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> * fix merge conflict --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

#39) * perf(consensus/state): Change finalizeCommit to use applyVerifiedBlock (backport cometbft#2928) (cometbft#2945) Simplest component of cometbft#2854 We already run ValidateBlock in finalizeCommit, so this PR removes one extra redundant call by using ApplyVerifiedBlock. (The other call is also redundant, but that likely requires a more complex caching strategy as noted in cometbft#2854 to remedy) From my understanding of these benchmarks, at Osmosis 150 validators, this should be saving ~13ms of execution time per block. --- #### PR checklist - [X] Tests written/updated - Theres no test to update, as its impossible to reach the difference in codepaths! - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] 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#2928 done by [Mergify](https://mergify.com). --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> * fix merge conflict --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> (cherry picked from commit 6fe6ee1)

#39) (#46) * perf(consensus/state): Change finalizeCommit to use applyVerifiedBlock (backport cometbft#2928) (cometbft#2945) Simplest component of cometbft#2854 We already run ValidateBlock in finalizeCommit, so this PR removes one extra redundant call by using ApplyVerifiedBlock. (The other call is also redundant, but that likely requires a more complex caching strategy as noted in cometbft#2854 to remedy) From my understanding of these benchmarks, at Osmosis 150 validators, this should be saving ~13ms of execution time per block. --- #### PR checklist - [X] Tests written/updated - Theres no test to update, as its impossible to reach the difference in codepaths! - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] 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#2928 done by [Mergify](https://mergify.com). --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> * fix merge conflict --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> (cherry picked from commit 6fe6ee1) Co-authored-by: Adam Tucker <adam@osmosis.team>

#39) (#47) * perf(consensus/state): Change finalizeCommit to use applyVerifiedBlock (backport cometbft#2928) (cometbft#2945) Simplest component of cometbft#2854 We already run ValidateBlock in finalizeCommit, so this PR removes one extra redundant call by using ApplyVerifiedBlock. (The other call is also redundant, but that likely requires a more complex caching strategy as noted in cometbft#2854 to remedy) From my understanding of these benchmarks, at Osmosis 150 validators, this should be saving ~13ms of execution time per block. --- #### PR checklist - [X] Tests written/updated - Theres no test to update, as its impossible to reach the difference in codepaths! - [X] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] 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#2928 done by [Mergify](https://mergify.com). --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> * fix merge conflict --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> (cherry picked from commit 6fe6ee1) Co-authored-by: Adam Tucker <adam@osmosis.team>

cason · 2024-05-08T14:58:40Z

The change in #2928 is nonetheless really relevant.

cason · 2024-05-08T14:59:18Z

@cason what do you think?

I added my comments/concerns to #2960.

sergio-mena · 2024-05-13T10:03:50Z

Following @cason's observations and comments, and an internal discussion, this is how we agreed to move forward:

We keep the part where we align the checks in defaultDoPrevote
But we revert the hunks that are removing the defensive panics
And, in order to keep the performance improvement that motivated this issue initially, we'll add a cache in memory as @ValarDragon was suggesting

…d `isTimely` (#2960) ~~This change is an implementation of the alignment of `ValidateBlock` to `ProcessProposal` explained [here](#2854 (comment) We need to: * Check if that helps, perf-wise * Carefully make we're not compromising algorithm's correctness EDIT: In the end, we are implementing this as agreed [here](#2854 (comment)) 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](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

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>

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

cason added consensus and removed needs-triage This issue/PR has not yet been triaged by the team. labels Apr 22, 2024

cason added this to CometBFT Apr 22, 2024

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

ValarDragon mentioned this issue Apr 29, 2024

perf(consensus/state): Change finalizeCommit to use applyVerifiedBlock #2928

Merged

4 tasks

adizere added this to the 2024-Q2 milestone Apr 29, 2024

sergio-mena mentioned this issue May 1, 2024

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

Merged

4 tasks

sergio-mena moved this from Todo to In Progress in CometBFT May 1, 2024

czarcas7ic mentioned this issue May 2, 2024

perf(con 8000 sensus/state): Change finalizeCommit to use applyVerifiedBloc… osmosis-labs/cometbft#39

Merged

7 tasks

This was referenced May 2, 2024

perf(consensus/state): Change finalizeCommit to use applyVerifiedBloc… (backport #39) osmosis-labs/cometbft#46

Merged

perf(consensus/state): Change finalizeCommit to use applyVerifiedBloc… (backport #39) osmosis-labs/cometbft#47

Merged

sergio-mena mentioned this issue May 13, 2024

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

Merged

4 tasks

sergio-mena closed this as completed in #3070 May 14, 2024

github-project-automation bot moved this from In Progress to Done in CometBFT May 14, 2024

ValarDragon mentioned this issue May 25, 2024

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

Merged

7 tasks

mergify bot mentioned this issue May 28, 2024

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

Merged

7 tasks

PaddyMc mentioned this issue 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

Uh oh!

enterPrecommit is almost always double verifying the block #2854

enterPrecommit is almost always double verifying the block #2854

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

enterPrecommit is almost always double verifying the block #2854

enterPrecommit is almost always double verifying the block #2854

Comments

Feature Request

Summary

Problem Definition

Proposal

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!