-
-
Notifications
You must be signed in to change notification settings - Fork 477
fix: emit head event whenever fork choice head changes #9697
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,16 @@ | ||
| import path from "node:path"; | ||
| import {PrivateKey} from "@libp2p/interface"; | ||
| import {Type} from "@chainsafe/ssz"; | ||
| import {routes} from "@lodestar/api"; | ||
| import {BeaconConfig} from "@lodestar/config"; | ||
| import {CheckpointWithHex, ForkChoiceStateGetter, IForkChoice, ProtoBlock, UpdateHeadOpt} from "@lodestar/fork-choice"; | ||
| import { | ||
| CheckpointWithHex, | ||
| EpochDifference, | ||
| ForkChoiceStateGetter, | ||
| IForkChoice, | ||
| ProtoBlock, | ||
| UpdateHeadOpt, | ||
| } from "@lodestar/fork-choice"; | ||
| import {LoggerNode} from "@lodestar/logger/node"; | ||
| import { | ||
| EFFECTIVE_BALANCE_INCREMENT, | ||
|
|
@@ -1179,7 +1187,27 @@ export class BeaconChain implements IBeaconChain { | |
| const timer = this.metrics?.forkChoice.findHead.startTimer({caller}); | ||
|
|
||
| try { | ||
| return this.forkChoice.updateAndGetHead({mode: UpdateHeadOpt.GetCanonicalHead}).head; | ||
| const prevHead = this.forkChoice.getHead(); | ||
| const head = this.forkChoice.updateAndGetHead({mode: UpdateHeadOpt.GetCanonicalHead}).head; | ||
|
|
||
| if (head.blockRoot !== prevHead.blockRoot) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nvm, getProposerHead() is different, could be a proposer boost reorg there
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as noted in the PR description I intentionally don't wanna consider the proposer path, I think it is fine to rely on the block import path in that case, we can add the comment as you suggested |
||
| try { | ||
| this.emitter.emit(routes.events.EventType.head, { | ||
| block: head.blockRoot, | ||
| epochTransition: computeStartSlotAtEpoch(computeEpochAtSlot(head.slot)) === head.slot, | ||
| slot: head.slot, | ||
| state: head.stateRoot, | ||
| previousDutyDependentRoot: this.forkChoice.getDependentRoot(head, EpochDifference.previous), | ||
| currentDutyDependentRoot: this.forkChoice.getDependentRoot(head, EpochDifference.current), | ||
| executionOptimistic: isOptimisticBlock(head), | ||
| }); | ||
| } catch (e) { | ||
| // getDependentRoot() may fail with error: "No block for root" as we can see in holesky non-finality issue | ||
| this.logger.debug("Error emitting head event", {slot: head.slot, root: head.blockRoot}, e as Error); | ||
| } | ||
| } | ||
|
|
||
| return head; | ||
| } catch (e) { | ||
| this.metrics?.forkChoice.errors.inc({entrypoint: UpdateHeadOpt.GetCanonicalHead}); | ||
| throw e; | ||
|
|
@@ -1209,6 +1237,7 @@ export class BeaconChain implements IBeaconChain { | |
| const secFromSlot = this.clock.secFromSlot(slot); | ||
|
|
||
| try { | ||
| // Do not emit head event here, when proposing we rely on the one emitted when importing our own block | ||
| const {head, isHeadTimely, notReorgedReason} = this.forkChoice.updateAndGetHead({ | ||
| mode: UpdateHeadOpt.GetProposerHead, | ||
| secFromSlot, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the first canonical recompute after attestation weight shifts happens through block production, this new emission path is bypassed:
BeaconChain.getProposerHead()callsforkChoice.updateAndGetHead({mode: GetProposerHead}), andForkChoice.updateAndGetHead()runsupdateHead()for that mode before returning. In that scenario the cached canonical head changes, but noheadSSE event is emitted, so validators watching head events can still miss the duty-dependent-root update until some laterrecomputeForkChoiceHead()call.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is intentional, we explicitly don't want to emit a head event on the proposer path when we are proposing a block ourselves
that head change is superseded once we import our own block, which emits the head event with the correct dependent roots, so head event consumers aren't affected in practice