Skip to content

perf(fork-choice): early-break window walk in getCanonicalPayloadCounts - #9824

Merged
nflaig merged 2 commits into
ChainSafe:nflaig/circuit-breaker-canonical-countsfrom
lodekeeper:lodekeeper/cb-window-walk
Aug 14, 2026
Merged

perf(fork-choice): early-break window walk in getCanonicalPayloadCounts#9824
nflaig merged 2 commits into
ChainSafe:nflaig/circuit-breaker-canonical-countsfrom
lodekeeper:lodekeeper/cb-window-walk

Conversation

@lodekeeper

@lodekeeper lodekeeper commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Demo requested in #9815 (discussion_r3779437582) — 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 continued over those.

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

Demo for ChainSafe#9815 (discussion_r3779437582): keep the circuit-breaker payload
scan O(window) instead of O(chain-to-anchor).

Instead of materializing the whole canonical chain via getAllAncestorNodes(),
count the resolved head inline, then iterate ancestors and break once a node
falls below `fromSlot` (ancestors are slot-descending, so everything past that
is already out of the window). This handles the head-inclusion gotcha:
iterateAncestorNodes() alone drops the start node, so the head is counted
explicitly before walking `.parent` via iterateAncestorNodesFromNode().

Behavior is identical to the previous full-scan (same canonical node sequence,
same filters); only the traversal is lazy with an early exit. Matters mainly
under prolonged non-finality.

🤖 Generated with AI assistance

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@ensi321 ensi321 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good. Need this for #9815

@lodekeeper lodekeeper changed the title demo: early-break variant of getCanonicalPayloadCounts perf(fork-choice): early-break window walk in getCanonicalPayloadCounts Aug 14, 2026
@lodekeeper

Copy link
Copy Markdown
Contributor Author

Thanks @ensi321 🙏

Since #9824 targets nflaig/circuit-breaker-canonical-counts (the #9815 branch), merging it drops the early-break straight into #9815 — it's mergeable as-is (no conflict with the metric-name refactor that landed on the branch since).

@nflaig — your call on the mechanism: merge #9824 onto the branch directly, or if you'd rather fold it into your own commit for authorship/history I'll close this out. Either works for me.

(Also retitled it — demo: isn't a valid conventional-commit type so the PR-title check was failing; it's perf(fork-choice): now.)

}

/** Count blocks selected as FULL or EMPTY by the supplied head chain in the inclusive slot range. */
getCanonicalPayloadCounts(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think while this is correct, I find it hard to follow with countNode

Something like

  const headIndex = this.getNodeIndexByRootAndStatus(headRoot, headPayloadStatus);
  let node = headIndex !== undefined ? this.nodes[headIndex] : undefined;

  while (node !== undefined && node.slot >= fromSlot) {
    if (
      node.slot !== GENESIS_SLOT &&
      node.slot <= toSlot &&
      isGloasBlock(node) &&
      node.payloadStatus !== PayloadStatus.PENDING
    ) 
    ...
    node = this.nodes[this.getParentNodeIndex(node)]
  }

would be more straightforward

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call — done in 95082e3 (pushed to the branch). Flattened to the single while loop walking getParentNodeIndex, dropping the countNode closure + iterateAncestorNodesFromNode generator. Reads much better.

Confirmed equivalent: the full protoArray unit suite is still green (131/131), including the tricky getCanonicalPayloadCounts cases — keeps-EMPTY-after-late-FULL, competing branches, genesis skip, PENDING head, inclusive bounds. The only mechanical difference is that this walks purely via getParentNodeIndex (no node.parent !== undefined pre-check), which coincides for the canonical in-window chain since the anchor's parent isn't in the array → getParentNodeIndex returns undefined and the loop ends.

Per @ensi321's review on ChainSafe#9824 — replace the `countNode` closure +
`iterateAncestorNodesFromNode` generator with a single flat `while` loop
walking `getParentNodeIndex` directly. Functionally identical (131/131
protoArray unit tests pass); just easier to follow.

🤖 Generated with AI assistance

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@nflaig

nflaig commented Aug 14, 2026

Copy link
Copy Markdown
Member

@lodekeeper is this ready to merge into my branch?

@lodekeeper

Copy link
Copy Markdown
Contributor Author

Yeah, ready from my side 👍

  • MERGEABLE, no conflict with the metric-name refactor on the branch
  • protoArray unit suite green (131/131) — both after the initial commit and after ensi321's flatten (95082e3)
  • The Validate PR title check passes; the UNSTABLE merge state is just because the full CI matrix doesn't run against a non-default base branch, not a failing check

Still your call on the mechanism — merge #9824 straight onto nflaig/circuit-breaker-canonical-counts, or if you'd rather fold it into your own commit for authorship/history I'll close this out. Either works for me.

@nflaig nflaig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this will be helpful during long non-finality

@nflaig
nflaig merged commit fd84843 into ChainSafe:nflaig/circuit-breaker-canonical-counts Aug 14, 2026
1 check passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Lodestar Team Coordination Aug 14, 2026
@lodekeeper

Copy link
Copy Markdown
Contributor Author

🙏 thanks — and thanks @ensi321 for the review + the cleaner flat-loop suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants