chore: address builder circuit breaker review follow-ups - #9780
Conversation
nflaig
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #9780 +/- ##
=========================================
Coverage 52.60% 52.60%
=========================================
Files 848 848
Lines 59969 59969
Branches 4414 4414
=========================================
Hits 31545 31545
Misses 28365 28365
Partials 59 59 🚀 New features to boost your workflow:
|
Performance Report✔️ no performance regression detected Full benchmark results
|
| // Scale the fault budget by blocks present so sparse windows still trigger on high non-reveal rates | ||
| this.active = faults * this.faultInspectionWindow > this.allowedFaults * blocksPresent; | ||
| // Keep the previous state if the window has no blocks, there is no data to assess builder health | ||
| if (blocksPresent > 0) { |
There was a problem hiding this comment.
Looks like this is the follow up of #9598 (comment) . Are you settled on 0, or just for the time being?
There was a problem hiding this comment.
need to think about this more, we can use a threshold of 4 or whatever for now but if there are so few blocks in the epoch it might be good to enable the circuit breaker also, there was an attack by Toni on devnet-7 which caused missed slots due to slow to process payloads he was broadcasting, so even in that case, we might not wanna accept bids and rather self-build so the network can recover
There was a problem hiding this comment.
I think 4 is good on mainnet, but we probably want 2 for minimal. But if we don't want to complicate things, we can just hardcode MIN_BLOCKS_TO_DEACTIVATE to 2 regardless of what config.
I am actually also okay with how you originally did
if (blocksPresent > 0) {
// Scale the fault budget by blocks present so sparse windows still trigger on high non-reveal rates
this.active = faults * this.faultInspectionWindow > this.allowedFaults * blocksPresent;
}
if you want to keep things simple
There was a problem hiding this comment.
I think 4 is good on mainnet, but we probably want 2 for minimal
even minimal, I would just keep 4, because 2 doesn't give you a meaningful sample for ~25% fault rate
with 2 it's basically
- 0/2 faults = deactivate
- 1/2 faults = keep active
- 2/2 faults = keep active
with 4 you have
- 0/4 faults = deactive
- 1/4 faults = deactive
- 2/4 faults = keep active
- 3/4 faults = keep active
- 4/4 faults = keep active
this isn't really meaningful, I would maybe even increase 4 to 8 to make sample more meaningful, also since this only applied for deactivating the circuit breaker, it's more conservative which I like here
the issue doesn't document anything meaningful, eg. I left a comment here myself #9598 (comment) which blocks we should count, so right now we don't check if block was strong, but that's a separate topic, not part of the comments you left there is also open design space like #9598 (comment) and #9598 (comment) I am not sure we gonna figure this out now until we see how we wanna deal with per-builder blacklisting/whitelisting if at all so yes, the issue states
which this PR should fully resolve, bigger design decisions need more broad discussion and iteration on the circuit breaker itself so I left some breadcrumbs on the PR for that, I'd rather open a proper issue and document open design decisions there |
| Math.max(clockSlot - this.faultInspectionWindow, 0), | ||
| clockSlot - 1 | ||
| ); | ||
| const faults = blocksPresent - payloadsRevealed; |
There was a problem hiding this comment.
if there are proposer equivocations, there are 2 blocks per slot, but there is only 1 payload, and it's the canonical
the current implementation counts that as a fault, which is not correct. But I'm not sure if we need to be that precise, the case is so rare.
There was a problem hiding this comment.
You're right. getPayloadRevealCounts counts each PENDING beacon block by its own blockRoot with no per-slot dedup (protoArray.ts:711), so two equivocating blocks at the same slot both increment blocksPresent, while only the canonical one gets a FULL variant via onExecutionPayload — so hasPayload → payloadsRevealed maxes at 1. Net +1 fault for that slot even though the canonical payload was revealed. Confirmed.
Agree it's negligible in practice: equivocation is slashable and rare, and the +1 sits inside the scaled tolerance (faults * window > allowedFaults * blocksPresent, ~25% over a 32-64 block window), so a single spurious fault won't move the breaker unless it's already right at the threshold.
Worth noting it's the same class as the "only count canonical/strong blocks" gap from the anchor thread — the non-canonical equivocation block is exactly one the builder was right not to reveal for. A canonical-only filter would fix both this and the non-canonical side-branch case in one go; a cheaper per-slot dedup would cover just the equivocation. I'd fold it into that follow-up rather than special-case it here, but it's an easy either/or — your + @nflaig's call on whether the precision is worth it now.
There was a problem hiding this comment.
if there are proposer equivocations, there are 2 blocks per slot, but there is only 1 payload, and it's the canonical
yes this is another problem of going through all nodes in the proto array, I feel like this makes the implementation quite complex, I am thinking about doing getAllAncestorNodes(headRoot, headPayloadStatus) instead as also suggested by @ensi321, this seems much simpler and more easy to get right, and we call builderCircuitBreaker.update(clockSlot) inside prepareNextSlot after recomputeForkChoiceHead so it's seems fine to just consider our canonical branch?
There was a problem hiding this comment.
Agree — I think this is the right move: simpler and more correct, and it collapses Thread D + the "canonical-only" follow-up into one change.
Verified on HEAD af6864fb:
It does what you want. getAllAncestorNodes(blockRoot, payloadStatus) (protoArray.ts:1806, wrapped in forkChoice.ts:1307) walks parent pointers via getParentNodeIndex → getParentPayloadStatus, so it returns exactly one node per ancestor block, each as its canonical EMPTY/FULL variant (never PENDING). So blocksPresent = ancestors in window, payloadsRevealed = the FULL ones — no hasPayload/PENDING-dedup dance needed.
Fixes Thread D for free, as you said: one ancestor chain has a single block per slot by construction, so equivocating siblings (and any non-canonical side branch) are off-branch. That's also the entirety of the "only count canonical/strong blocks" follow-up — so this subsumes it rather than being a separate change.
Call order checks out. update(clockSlot) runs at prepareNextSlot.ts:156, after recomputeForkChoiceHead at :84, inside the if (feeRecipient) (we're proposing) block — so the fork-choice head is fresh canonical there; walking forkChoice.getHead() is sound.
A few things to keep/get right:
- Still window-bound it.
getAllAncestorNodesreturns the full chain back to the anchor, so keep the[clockSlot - faultInspectionWindow, clockSlot - 1]filter — bonus: ancestors are in strictly decreasing slot order, so you can early-break onceslot < fromSlot(the current full scan explicitly can't). Under normal finality the anchor is ≥2 epochs back, so the window never runs past it. - Keep the genesis skip (bbce592). Genesis is an ancestor of head and, on a fresh net, in-window; it shows up as EMPTY, so it'd still count as a fault without
slot === GENESIS_SLOT. - Head variant. Passing the head's PENDING status drops the head node itself (getAllAncestorNodes skips a PENDING start node, protoArray.ts:1823) and starts from its ancestors — which lines up with your existing "exclude clockSlot, reveal may be in flight." Just worth being deliberate about which
headPayloadStatusyou pass.
One minor semantic upgrade to be aware of (not a problem): the ancestor walk counts a block as revealed only if the canonical child actually built on its FULL (via getParentPayloadStatus), vs the current hasPayload = "a FULL variant exists anywhere." So a payload revealed too late for the chain to build on reads as EMPTY here — arguably the more accurate builder-health signal anyway.
There was a problem hiding this comment.
@lodekeeper please review #9815, does that look good?
There was a problem hiding this comment.
Looks good — reviewed #9815 end to end, the ancestor-walk approach is correctly implemented and it covers everything from this thread.
Verified (head 5bed75eb):
getCanonicalPayloadCounts(fromSlot, toSlot, headRoot, headPayloadStatus)walksgetAllAncestorNodesand counts canonical FULL vs EMPTY withfaults = empty. The three things I flagged earlier are all handled: window bound kept via thefromSlot/toSlotfilter, the genesis skip is retained, and a PENDING head is excluded (start-node drop + the explicitpayloadStatus === PENDINGguard), with a new test for it.- The
"keeps EMPTY after a late FULL arrives"test is the important one — it nails the devnet-7 case: a late payload for0x03no longer masks the canonical EMPTY that0x05built on, and competing siblings (0x04/0x06) drop out for free, so Thread D's equivocation double-count is gone too. !isGloasBlockcorrectly excludes pre-gloas ancestors at the fork boundary.this.head.payloadStatuswiring matches the existingiterateAncestorNodescall sites, so that's idiomatic.- Rename is clean (no dangling
getPayloadRevealCounts/payloadsRevealed), andpayloads_revealed → payloads_fullis safe — nothing references the metric (no dashboards, unreleased). Dropping the startup-log assertion test is fine, it was a brittle low-value check.
Two minor, non-blocking notes:
- The genesis skip (
node.slot === GENESIS_SLOT) is still load-bearing — on a gloas-genesis net genesis is an in-window ancestor returned as EMPTY, so without it genesis counts as a fault — but its dedicated test ("does not count the genesis block") was dropped in the rewrite. Might be worth re-adding a small one so that guard doesn't silently regress. - Optional:
getAllAncestorNodesmaterializes the whole chain back to the anchor each update; since ancestors are slot-descending and you already have theiterateAncestorNodesgenerator, you could iterate + break onceslot < fromSlotto keep it O(window). Negligible under normal finality, matters more under prolonged non-finality.
Neither blocks it. 👍
There was a problem hiding this comment.
@lodekeeper please leave your review on the PR and leave in-line comments there for any feedback
There was a problem hiding this comment.
Done — submitted a formal review with inline notes: #9780 (review). No blocking concerns; it summarizes the A–D threads (genesis skip, hysteresis, JSDoc sync, getFaultInspectionParams simplification) and notes the equivocation edge @twoeths raised is handled by the stacked #9815.
There was a problem hiding this comment.
I also meant you should review #9815 and leave comments there please
There was a problem hiding this comment.
Done — the review is now on #9815 directly: #9815 (review) (looks good, no blocking concerns + 2 non-blocking inline notes: re-add the dropped genesis-guard test, and an optional early-break via the iterateAncestorNodes generator). Also +1'd your resolution of the codex synthetic-anchor P2.
lodekeeper
left a comment
lodekeeper
left a comment
There was a problem hiding this comment.
Did a full pass — no blocking concerns from me. The substantive points all got worked through in the inline threads above; summarizing for the record:
- Genesis skip (Thread A) — narrowing from
parent === undefinedtoslot === GENESIS_SLOTis correct: genesis is the only anchor reliably in-window on a fresh gloas net, it isn't builder-produced, and it's always EMPTY, so counting it would be a spurious fault. Now has a dedicated test. - Activate/deactivate hysteresis (Thread B) — asymmetric budget (trip on any breach, recover only with ≥
MIN_BLOCKS_TO_DEACTIVATEin-budget observations) reads correctly and is covered by the new "requires a minimum sample to deactivate" test. The exact4is @ensi321's call from his thread. - JSDoc / option-description sync (Thread C) —
options.ts, the CLI--builder.allowedFaultshelp, andgetFaultInspectionParamsnow line up, and the "~25% budget = window // 4" framing is accurate. getFaultInspectionParamssimplification —?? Infinitycapped atwindow // 4is equivalent to the old double-floor and reads cleaner.- Equivocation double-count (Thread D, @twoeths) — real but negligible in this all-branches scan (one spurious fault, absorbed by the scaled budget); the canonical-only refinement is the stacked #9815, which I've reviewed separately ("looks good").
LGTM. A few inline notes anchoring the above.
4 participants
Motivation
Address the remaining builder circuit breaker review feedback from #9598 that does not require major design changes, these can be discussed separately here.
Description
MIN_BLOCKS_TO_DEACTIVATE = 4observed blocks are within budgetEMPTYbuilder.allowedFaultsCLI semanticsclockSlotin circuit breaker logsCloses #9678