fix: count canonical FULL and EMPTY blocks in circuit breaker - #9815
Merged
Conversation
This reverts commit affd151.
nflaig
marked this pull request as ready for review
August 13, 2026 15:12
nflaig
marked this pull request as draft
August 13, 2026 15:13
![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: 5bed75eb3e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lodekeeper
reviewed
Aug 13, 2026
lodekeeper
left a comment
lodekeeper
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the ancestor-walk refactor end to end — looks good, no blocking concerns. (I'd posted this verdict on #9780 earlier; moving it here where it belongs.)
The core is correct: getCanonicalPayloadCounts walks getAllAncestorNodes(head, headPayloadStatus) and counts canonical FULL vs EMPTY with faults = empty. It fixes the real devnet-7 case — a late/orphaned payload no longer masks a canonical EMPTY (the "keeps EMPTY after a late FULL arrives" test nails it) — and drops the equivocation double-count for free (one block/slot per branch). !isGloasBlock correctly excludes pre-gloas ancestors, the this.head.payloadStatus wiring matches the existing iterateAncestorNodes call sites, and the metric rename (payloads_revealed → payloads_full) is safe (no dashboard refs, unreleased).
+1 on your take on the codex P2 (synthetic anchor variants): same class as the genesis case — it self-resolves as the real child syncs (the walk picks the FULL variant once a real child builds on FULL), and the breaker only runs in prepareNextSlot near head, by which point those checkpoint-era synthetic nodes have aged out of the window. Not a blocker.
Two minor, non-blocking notes inline. 👍
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## nflaig/circuit-breaker-polish #9815 +/- ##
==============================================================
Coverage 52.60% 52.60%
==============================================================
Files 848 848
Lines 60047 60047
Branches 4424 4424
==============================================================
Hits 31587 31587
Misses 28401 28401
Partials 59 59 🚀 New features to boost your workflow:
|
nflaig
marked this pull request as ready for review
August 13, 2026 21:40
nflaig
added
the
status-do-not-merge
Member
Author
|
this is ready for review but should be merged separately and is currently stacked on top of #9780 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bed75eb3e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Performance Report🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
…ts (#9824) Demo requested in #9815 ([discussion_r3779437582](#9815 (comment))) — showing how the O(window) early-break variant of `getCanonicalPayloadCounts` would look. Targets `nflaig/circuit-breaker-canonical-counts`, not `unstable`. ### Approach Instead of materializing the whole canonical chain via `getAllAncestorNodes()`, walk it newest-first and stop once a node drops below `fromSlot`. Ancestors are strictly slot-descending, so everything past that point is already out of the window — the previous full scan just `continue`d over those. ```ts const headIndex = this.getNodeIndexByRootAndStatus(headRoot, headPayloadStatus); const head = headIndex !== undefined ? this.nodes[headIndex] : undefined; if (head !== undefined && head.slot >= fromSlot) { countNode(head); for (const node of this.iterateAncestorNodesFromNode(head)) { if (node.slot < fromSlot) break; countNode(node); } } ``` ### The head-inclusion gotcha (your note in r3779406069) `iterateAncestorNodes()` starts one hop up (`iterateAncestorNodesFromNode` begins at `.parent`), so a direct swap drops the resolved head and undercounts by one. Handled here by counting the head explicitly first, then walking `.parent` via `iterateAncestorNodesFromNode(head)`. A PENDING head is still skipped by `countNode` (same as `getAllAncestorNodes` not pushing a PENDING start node) — it only seeds the ancestor walk. ### Behavior Identical to the current full-scan: same canonical node sequence (head + ancestors), same filters (genesis / out-of-window / non-gloas / PENDING), same `full`/`empty` counts. Only the traversal is lazy with an early exit. The win (`O(window)` vs `O(chain-to-anchor)`) only shows under prolonged non-finality, when the chain-to-anchor grows well past the fault window. ### Tests Ran the fork-choice protoArray suite locally against this change — **131/131 pass** (`packages/fork-choice/test/unit/protoArray`, 7 files), including every `getCanonicalPayloadCounts` case: genesis skip, "keeps EMPTY after a late FULL arrives", PENDING head, inclusive bounds, uses-supplied-head-branch. CI here re-runs the full suite. Not attached to it — happy to close if you'd rather keep the simpler full-scan; just wanted to show the shape as you asked. 🤖 Generated with AI assistance
Member
Author
|
@lodekeeper I merged your changes from #9824, please give this PR another pass |
lodekeeper
reviewed
Aug 14, 2026
lodekeeper
left a comment
lodekeeper
left a comment
Contributor
There was a problem hiding this comment.
Gave it another pass — LGTM, good from my side.
The merged early-break (former #9824) integrated cleanly: getCanonicalPayloadCounts at the head is byte-identical to the version I ran the protoArray suite against (131/131 pass) and that @ensi321 endorsed — the only change since is your tweak comment reword (dd2a3ba), no logic drift.
Confirmed on this pass (head dd2a3ba):
- Full changed-file set is the expected one (circuit breaker hysteresis + canonical counts + metric names + tests) — nothing unexpected snuck in with the merge.
- Both notes from my earlier review are addressed and still in place: the genesis-guard test (
does not count the genesis block as EMPTY) and the O(window) early-break itself. - The genesis /
toSlot/!isGloasBlock/ PENDING filters and the early-break-on-fromSlotare unchanged; the flat-loop form reads well.
Nothing new to flag — it's the same code already reviewed + tested, just combined. 👍
twoeths
reviewed
Aug 14, 2026
twoeths
reviewed
Aug 14, 2026
twoeths
approved these changes
Aug 14, 2026
github-project-automation
Bot
moved this from In Progress
to Done
in Lodestar Team Coordination
nflaig
removed
the
status-do-not-merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Currently, the builder circuit breaker counts revealed payloads from all branches. On glamsterdam-devnet-7, orphaned payloads masked canonical
EMPTYblocks and prevented the circuit breaker from activating. The problem is we only check if payload exists viahasPayloadbut instead we should walk the ancestry to know whether the node isFULLorEMPTY, but this becomes more complicated if we wanna track all branches, so opting to only track the canonical branch for now.There was also a discussion here #9598 (comment)
which this PR implements for now, I do still think it would be best to track all branches but it requires a more sophisticated algorithm and tracking data, especially if we wanna do per-builder banning, eg. you will need to track the attestation votes of blocks (ie. if block was strong meaning >60% quorum was reached) and you also need to do an ancestor walk for each branch via
getAllAncestorNodesDescription
FULLandEMPTYblocks on the canonical branchEMPTYblocks as faultsFULL, andEMPTYblock counts in circuit breaker metrics