refactor: optimize head event emission logic - #9720
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
馃挕 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e03d4e1ca
鈩癸笍 About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 馃憤.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
lodekeeper
left a comment
lodekeeper
left a comment
There was a problem hiding this comment.
Reviewed commit: 7e03d4e1ca
Behavior looks good to me. I rechecked the Codex PENDING concern: Gloas import creates PENDING -> EMPTY, payload import adds FULL as the sibling, and findHead() returns the best descendant, so this path should expose EMPTY/FULL, not PENDING. Over that domain this preserves the old event behavior: head only on root change, head_v2 on root or payload-status change.
Verification: pnpm exec biome check packages/beacon-node/src/chain/chain.ts; pnpm vitest run --project unit packages/fork-choice/test/unit/protoArray/getViableHeads.test.ts; pnpm --filter @lodestar/beacon-node... build.
Holding approval for Nico's naming nit: headChanged should be something like headRootChanged since same-root EMPTY/FULL flips are still head changes for head_v2.
|
@lodekeeper can you give this another review, this should be a pure refactor with now behavioral changes |
nflaig
changed the title
lodekeeper
left a comment
lodekeeper
left a comment
There was a problem hiding this comment.
Reviewed commit: 970ccd70ca7e251c07c7c497c619e4ca122a4f89
Re-reviewed after the headRootChanged rename. This is now a pure refactor / no behavior change: head still emits only when the head root changes, and head_v2 still emits when the root changes or the exposed payload status changes. The earlier PENDING concern does not apply because this path gets the canonical getHead() / findHead() result, where Gloas PENDING is internal and exposed heads are EMPTY/FULL.
Local verification:
pnpm exec biome check packages/beacon-node/src/chain/chain.tspnpm --filter @lodestar/beacon-node... buildpnpm vitest run --project unit packages/fork-choice/test/unit/protoArray/getViableHeads.test.ts
CI: build/lint/type/unit checks are green; broader E2E/spec jobs are still pending.
nflaig
left a comment
nflaig
left a comment
There was a problem hiding this comment.
LGTM, thanks for the follow-up @markolazic01 馃檹
|
馃帀 This PR is included in v1.46.0 馃帀 |
4 participants
Description
Simplifies conditioning of
headevent emissions.Inspired by #9486 (comment).