Skip to content

Commit

Permalink
perf(consensus/state): Change finalizeCommit to use applyVerifiedBloc… (
Browse files Browse the repository at this point in the history
#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 <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>

* fix merge conflict

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Dev Ojha <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>
  • Loading branch information
4 people authored May 2, 2024
1 parent a9991fd commit 6fe6ee1
Show file tree
Hide file tree
Showing 4 changed files with 7 additions and 6 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- `[consensus/state]` Remove a redundant `VerifyBlock` call in `FinalizeCommit`
([\#2928](https://github.com/cometbft/cometbft/pull/2928))
2 changes: 1 addition & 1 deletion blocksync/reactor.go
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ FOR_LOOP:

// TODO: same thing for app - but we would need a way to
// get the hash without persisting the state
state, err = bcR.blockExec.ApplyVerifiedBlock(state, firstID, first)
state, _, err = bcR.blockExec.ApplyVerifiedBlock(state, firstID, first)
if err != nil {
// TODO This is bad, are we zombie?
panic(fmt.Sprintf("Failed to process committed block (%d:%X): %v", first.Height, first.Hash(), err))
Expand Down
4 changes: 2 additions & 2 deletions consensus/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -1701,13 +1701,13 @@ func (cs *State) finalizeCommit(height int64) {
stateCopy := cs.state.Copy()

// Execute and commit the block, update and save the state, and update the mempool.
// We use apply verified block here because we have verified the block in this function already.
// NOTE The block.AppHash wont reflect these txs until the next block.
var (
err error
retainHeight int64
)

stateCopy, retainHeight, err = cs.blockExec.ApplyBlock(
stateCopy, retainHeight, err = cs.blockExec.ApplyVerifiedBlock(
stateCopy,
types.BlockID{
Hash: block.Hash(),
Expand Down
5 changes: 2 additions & 3 deletions state/execution.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,9 +182,8 @@ func (blockExec *BlockExecutor) ValidateBlock(state State, block *types.Block) e
// ApplyVerifiedBlock does the same as `ApplyBlock`, but skips verification.
func (blockExec *BlockExecutor) ApplyVerifiedBlock(
state State, blockID types.BlockID, block *types.Block,
) (State, error) {
newState, _, err := blockExec.applyBlock(state, blockID, block)
return newState, err
) (State, int64, error) {
return blockExec.applyBlock(state, blockID, block)
}

// ApplyBlock validates the block against the state, executes it against the app,
Expand Down

0 comments on commit 6fe6ee1

Please sign in to comment.