diff --git a/.changelog/unreleased/improvements/2016-blocksync-avoid-double-calling-block-from-proto.md b/.changelog/unreleased/improvements/2016-blocksync-avoid-double-calling-block-from-proto.md new file mode 100644 index 00000000000..7251221be18 --- /dev/null +++ b/.changelog/unreleased/improvements/2016-blocksync-avoid-double-calling-block-from-proto.md @@ -0,0 +1,2 @@ +- `[blocksync]` Avoid double-calling `types.BlockFromProto` for performance + reasons ([\#2016](https://github.com/cometbft/cometbft/pull/2016)) diff --git a/.github/mergify.yml b/.github/mergify.yml index 270c06fbb2c..5d98052284c 100644 --- a/.github/mergify.yml +++ b/.github/mergify.yml @@ -1,13 +1,19 @@ +defaults: + actions: + backport: + assignees: + - "{{ author }}" + queue_rules: - name: default conditions: - - base=main - - label=S:automerge + - "#approved-reviews-by>=1" pull_request_rules: - name: Automerge to main conditions: - - base=main + - "#approved-reviews-by>=1" + - base=osmo/v0.37.4 - label=S:automerge actions: queue: @@ -15,13 +21,12 @@ pull_request_rules: name: default commit_message_template: | {{ title }} (#{{ number }}) - {{ body }} - - name: backport patches to v0.34.x branch + - name: backport patches to v23 branch conditions: - - base=main - - label=S:backport-to-v0.34.x + - base=osmo/v0.37.4 + - label=S:backport/v23 actions: backport: branches: - - v0.34.x + - osmo-v23/v0.37.4 diff --git a/blocksync/msgs.go b/blocksync/msgs.go index 447748ecb92..74f9b783bbf 100644 --- a/blocksync/msgs.go +++ b/blocksync/msgs.go @@ -31,10 +31,9 @@ func ValidateMsg(pb proto.Message) error { return errors.New("negative Height") } case *bcproto.BlockResponse: - _, err := types.BlockFromProto(msg.Block) - if err != nil { - return err - } + // Avoid double-calling `types.BlockFromProto` for performance reasons. + // See https://github.com/cometbft/cometbft/issues/1964 + return nil case *bcproto.NoBlockResponse: if msg.Height < 0 { return errors.New("negative Height") diff --git a/blocksync/reactor.go b/blocksync/reactor.go index c05d7e2f3f8..629fbbedc40 100644 --- a/blocksync/reactor.go +++ b/blocksync/reactor.go @@ -230,7 +230,8 @@ func (bcR *Reactor) ReceiveEnvelope(e p2p.Envelope) { case *bcproto.BlockResponse: bi, err := types.BlockFromProto(msg.Block) if err != nil { - bcR.Logger.Error("Block content is invalid", "err", err) + bcR.Logger.Error("Peer sent us invalid block", "peer", e.Src, "msg", e.Message, "err", err) + bcR.Switch.StopPeerForError(e.Src, err) return } bcR.pool.AddBlock(e.Src.ID(), bi, msg.Block.Size()) @@ -419,7 +420,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.ApplyBlock(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)) diff --git a/mempool/nop_mempool.go b/mempool/nop_mempool.go index 28ce974703a..0e5b451e8dc 100644 --- a/mempool/nop_mempool.go +++ b/mempool/nop_mempool.go @@ -109,7 +109,4 @@ func (*NopMempoolReactor) ReceiveEnvelope(p2p.Envelope) {} // SetSwitch does nothing. func (*NopMempoolReactor) SetSwitch(*p2p.Switch) {} -// AppHashErrorsCh always returns nil. -func (r *NopMempoolReactor) AppHashErrorsCh() chan p2p.AppHashError { - return nil -} +func (*NopMempoolReactor) AppHashErrorsCh() chan p2p.AppHashError { return nil } diff --git a/state/execution.go b/state/execution.go index 4d312a99ea7..6a460d4bfe9 100644 --- a/state/execution.go +++ b/state/execution.go @@ -179,6 +179,14 @@ func (blockExec *BlockExecutor) ValidateBlock(state State, block *types.Block) e return blockExec.evpool.CheckEvidence(block.Evidence.Evidence) } +// 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 +} + // ApplyBlock validates the block against the state, executes it against the app, // fires the relevant events, commits the app, and saves the new state and responses. // It returns the new state and the block height to retain (pruning older blocks). @@ -193,6 +201,10 @@ func (blockExec *BlockExecutor) ApplyBlock( return state, 0, ErrInvalidBlock(err) } + return blockExec.applyBlock(state, blockID, block) +} + +func (blockExec *BlockExecutor) applyBlock(state State, blockID types.BlockID, block *types.Block) (State, int64, error) { startTime := time.Now().UnixNano() abciResponses, err := execBlockOnProxyApp( blockExec.logger, blockExec.proxyApp, block, blockExec.store, state.InitialHeight,