head and reorg event not always emitted

dapplion commented 1 year ago

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 commented 1 year ago

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 commented 11 months ago

@matthewkeil will DM @dapplion to discuss :)

dapplion commented 11 months ago

also called in https://github.com/ChainSafe/lodestar/pull/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 commented 1 week ago

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

wemeetagain commented 1 week ago

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

wemeetagain commented 1 week ago

Also seems we're not consistently emitting finalized checkpoint events, see https://github.com/ChainSafe/lodestar/issues/5904

ChainSafe / lodestar