head and reorg event not always emitted

[dapplion]

this.chain.recomputeForkChoiceHead() is called in multiple places beyond importBlock. The importBlock assumes that recomputeForkChoiceHead is only called there, so the head and reorg events are only emitted for head recomputed there

Also the strong reference to the head state is only set on the importBlock flow, so there's a risk the node gets stuck if the head changes outside of importBlock

[matthewkeil]

Sections of code affected by bug

beacon-node/src/api/impl/validator/index.ts

• Call to recomputeForkChoiceHead in produceBlockV2 before chain.produceBlock.
• produceBlock calls produceBlockV2
• Call to forkchoice.updateHead instead of recomputeForkChoiceHead in produceBlindedBlock

beacon-node/src/chain/prepareNextSlot.ts
Call to recomputeForkChoiceHead at the top of prepareForNextSlot

Potential Solution

Will refactoring code in importBlock preceded by notes #5 and #6 to be a function, that can be called in all locations above, resolve the issue?
#5 is Compute head and if new stateCache.setHeadState
#6 Queue notifyForkchoiceUpdate to engine api

The new function could be called maybeUpdateHead and would incorporate the newHead.blockRoot !== oldHead.blockRoot check, with side effects, and return the results from recomputing.

Other notes

Should the call to forkchoice.updateHead be swapped for recomputeForkChoiceHead so the metrics are updated correctly?

[philknows]

@matthewkeil will DM @dapplion to discuss :)

[dapplion]

• also called in Run forkChoice to get correct head for proposal #4058

https://github.com/ChainSafe/lodestar/blame/fa30bcf10086292883ef086202474ac4df67ffe1/packages/beacon-node/src/api/impl/validator/index.ts#L371-L372

idea

consider creating a type that selected the safe no mutate methods from the forkchoice as ForkChoiceSafe. And expect consumers to cast to ForkChoiceMutate or similar to actually recompute head. At least it's more obvious to devs you are doing something not ok

[wemeetagain]

unassigning @matthewkeil for now, feel free to reassign if its being worked on

[wemeetagain]

This is rated as a "high" priority because it affects the events api

[wemeetagain]

• Also seems we're not consistently emitting finalized checkpoint events, see metric beacon_finalized_epoch was updated slowly than other clients #5904