feat: add circuit breaker for gloas block production#9598
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a post-Gloas bid circuit breaker for builder bids to mitigate issues with unrevealed payloads. It adds the BidCircuitBreaker class, integrates it into the block production and slot preparation workflows, exposes configuration options via the CLI, and implements tracking in the fork choice and proto-array modules. The review feedback correctly identifies a potential issue at genesis where clockSlot - 1 can evaluate to -1, which is semantically incorrect for slot numbers, and provides a clear code suggestion to guard against this case.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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: eb1068a49a
ℹ️ 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".
Performance Report🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
nflaig
changed the title
|
merged into |
| * Beacon clients select randomized values from the following ranges when initializing | ||
| * the circuit breaker (so at boot time and once for each unique boot). |
There was a problem hiding this comment.
I know this is just moved (existing code). But why are we randomizing these values?
There was a problem hiding this comment.
so that you can't game the circuit breaker, if you know the exact algorithm the builders could abuse that. this isn't that relevant since each client has it's own implementation but assuming we had 100% network share this would be needed so each proposer behaves slightly different and builders are not able to predict when local blocks will be forced by the circuit breaker
| getPayloadRevealCounts(fromSlot: Slot, toSlot: Slot): {blocksPresent: number; payloadsRevealed: number} { | ||
| let blocksPresent = 0; | ||
| let payloadsRevealed = 0; | ||
| for (const node of this.nodes) { |
There was a problem hiding this comment.
per slot, there could be different Blocks on different branches (this happened on devnets)
so we should count on the head branch to make it more precise
we can also iterate backward from head and break when slot is out of range to save some iterations
There was a problem hiding this comment.
reviewed again, we should not only count on a specific branch
but looping backward is more performant, especially in non-finality time
There was a problem hiding this comment.
need to think about this more, it's not clear we should count unrevealed on multiple branches, I guess you could have a circuit breaker per branch
There was a problem hiding this comment.
I think counting as we currently do is most robust, added a test with a comment here
lodestar/packages/fork-choice/test/unit/protoArray/gloas.test.ts
Lines 142 to 144 in 34e3cd4
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it("builds with the builder bid when a bid is available", async () => { |
There was a problem hiding this comment.
this is unrelated but obviously a bug, we should not pick builder bid always, if local payload has higher value we should pick that one instead, will be addressed by #9595
There was a problem hiding this comment.
- this is fixed by feat: builder selection, broadcast validation, stateless flow for gloas block production #9595, the test has been updated there
| continue; | ||
| } | ||
| blocksPresent++; | ||
| if (this.hasPayload(node.blockRoot)) { |
There was a problem hiding this comment.
What do you think calling isPayloadTimely here? It would make the circuit breaker more sensitive due to late payloads
There was a problem hiding this comment.
it's a consideration, need to think more about it, generally, a late payload can still become canonical if attesters vote for it and next proposer doesn't reorg, so in that case I think it would be a mistake to count these as well. the ptc becomes less reliable in bad network conditions as well since you can't reach the >50% threshold easily. that would be covered by is_payload_timely so maybe there is no harm is using that instead
| let payloadsRevealed = 0; | ||
| // Iterate backward as recent blocks are at the end of the nodes array. Only break on block | ||
| // nodes below the window since FULL variants are appended late when the envelope is imported | ||
| for (let i = this.nodes.length - 1; i >= 0; i--) { |
There was a problem hiding this comment.
This would also counts blocks from the side chains. Then the rboth blocksPresent and payloadsRevealted will be inflated. If there are multiple side chains.
There was a problem hiding this comment.
I think a safer heuristic is to start from head block, and traverse through the ancestors to get the reveal count
There was a problem hiding this comment.
counting from side chains is intended as discussed with @twoeths above, see #9598 (comment). I do think the safer heuristic is to actually consider side chains but we can discuss this more
| let payloadsRevealed = 0; | ||
| // Iterate backward as recent blocks are at the end of the nodes array. Only break on block | ||
| // nodes below the window since FULL variants are appended late when the envelope is imported | ||
| for (let i = this.nodes.length - 1; i >= 0; i--) { |
There was a problem hiding this comment.
Another thing is we should also filter out self-built blocks here. Since we are measuring the external builder reliability.
Counting self-built blocks will lead to it contribute favourably towards builder.
There was a problem hiding this comment.
Another thing is we should also filter out self-built blocks here
no I don't think I would support this, it's intended to also consider self-builds, in fact, any safety mechanism should consider them equal, the problem is there is nothing stopping out of protocol mechanism to allow builders to work together with proposers to get their payloads onchain via self-builds
|
|
||
| const wasActive = this.active; | ||
| // 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; |
There was a problem hiding this comment.
I think it would be great to also have minObservedBlocks or minObservedBlocksPercentage.
If we have only observed say 4 blocks in the past 32 slots, we would probably want to activate this because chain is unhealthy.
Only if it's >= 4 blocks, that we start the calculation of blocksPresent and payloadRevealed.
Numbers here are just arbitrary, just to illustrate an example
There was a problem hiding this comment.
If we have only observed say 4 blocks in the past 32 slots, we would probably want to activate this because chain is unhealthy.
to be clear, the circuit breaker has nothing to do with blocks at all in gloas
| nativeStateView?: boolean; | ||
| /** Builder circuit breaker fault inspection window in slots */ | ||
| faultInspectionWindow?: number; | ||
| /** Allowed unrevealed payloads within the fault inspection window */ |
There was a problem hiding this comment.
This description is wrong though. I originally thought this means the absolute count of unrevealed payloads that triggers the circuit breaker but apparently not.
What it means is if fault rate is greater than allowedFaults / window, circuit breaker will trigger.
If window = 32, allowedFaults = 8, observedBlocks = 16, then circuit breaker activates at 5 unrevealed payloads
There was a problem hiding this comment.
I think it's better to define allowedFaultsPercentage = 0.25 to allow up to 25% fault
There was a problem hiding this comment.
yeah it's not 100% accurate, I also think for gloas we can even re-design the inputs a bit, I don't really like faultInspectionWindow and allowedFaults as input parameters, just kept to not overcomplicate this PR, I think we can do better here if we have more time to think how we wanna design this. Especially now that we have to consider blocks as well, it makes sense to use different semantics
| } | ||
|
|
||
| const testCases: [string, {blocksPresent: number; payloadsRevealed: number}, boolean][] = [ | ||
| ["empty window", {blocksPresent: 0, payloadsRevealed: 0}, false], |
There was a problem hiding this comment.
This is an interesting interaction. If we observe no block within the window, we deactivate the circuit breaker. Shouldn't we fall back to local build if this is the case?
There was a problem hiding this comment.
as mentioned above, blocks are irrelevant for the circuit breaker activation/deactivation, we just need to consider them to know how many payloads could have been revealed.
but to this specific case, I think if there is no data observed in the window it might be better to just keep the previous circuit breaker state, ie. if we it was active before, keep it active, and similar, if it's was deactivated, keep it deactivated
| ); | ||
| // allowedFaults should be < faultInspectionWindow, limiting them to faultInspectionWindow/4 | ||
| const allowedFaults = Math.min( | ||
| opts.allowedFaults ?? Math.floor(faultInspectionWindow / 4), |
There was a problem hiding this comment.
I don't get what you mean here so you are saying if opts.allowedFaults is not configured the is should be infinity?
There was a problem hiding this comment.
ah I think I got what you mean, sure can do that
| * Beacon clients select randomized values from the following ranges when initializing | ||
| * the circuit breaker (so at boot time and once for each unique boot). | ||
| * | ||
| * ALLOWED_FAULTS: between 1 and SLOTS_PER_EPOCH // 4 |
There was a problem hiding this comment.
Allowed faults is not between 1 and 8 (mainnet), but L93 calculation says between 8 and 15
There was a problem hiding this comment.
ah good catch, this comment is wrong, should have read more carefully, would be concerning if allowed faults could be 1, that would be too aggressive behavior
6 participants
Motivation
The pre-gloas builder circuit breaker (
updateBuilderStatus) counts missed slots via fork choice and disables the external builder flow when the chain is unhealthy. This mechanism does not apply to gloas:produceBlockV4always prefers a builder bid from the bid pool without any chain health checkgetSlotsPresentalso breaks post-gloas since each gloas block inserts multiple protoArray nodes (PENDING/EMPTY/FULL variants)Description
BuilderCircuitBreakerwhich counts blocks with unrevealed payloads (no FULL variant in fork choice) within the fault inspection window and ignores builder bids inproduceBlockV4while the breaker is activeprepareNextSlotand lazily at block production time, the pre-gloas builder status update is now gated to pre-gloas forks--builder.faultInspectionWindowand--builder.allowedFaultsflags configure the breaker on every fork, defaults are randomized per boot as beforebeacon_builder_circuit_breaker_active: whether the breaker is currently active (1) causing builder bids to be ignoredbeacon_builder_circuit_breaker_faults: count of blocks with unrevealed payloads in the fault inspection windowbeacon_builder_circuit_breaker_blocks_present: count of blocks present in the fault inspection windowbeacon_builder_circuit_breaker_payloads_revealed: count of blocks with revealed payloads in the fault inspection window