-
-
Notifications
You must be signed in to change notification settings - Fork 479
feat: add circuit breaker for gloas block production #9598
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
eb1068a
1b1430e
b66bc86
15b1f96
e9946fd
91d6805
335bacd
ca36433
96e2fc1
9d02e97
ae10b68
43c7a9b
a2aa07f
d82fc4c
6004e8e
069269a
a01bca7
8a3df06
34e3cd4
91f691e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -169,6 +169,7 @@ flamegraphs | |
| floodsub | ||
| fsSL | ||
| getNetworkIdentity | ||
| gloas | ||
| gnosis | ||
| gpg | ||
| heapdump | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| import {IForkChoice} from "@lodestar/fork-choice"; | ||
| import {Slot} from "@lodestar/types"; | ||
| import {Logger} from "@lodestar/utils"; | ||
| import {getFaultInspectionParams} from "../execution/builder/http.js"; | ||
| import {Metrics} from "../metrics/index.js"; | ||
|
|
||
| export type BuilderCircuitBreakerOpts = { | ||
| faultInspectionWindow?: number; | ||
| allowedFaults?: number; | ||
| }; | ||
|
|
||
| export type BuilderCircuitBreakerModules = { | ||
| forkChoice: IForkChoice; | ||
| logger: Logger; | ||
| metrics: Metrics | null; | ||
| }; | ||
|
|
||
| /** | ||
| * Post-gloas circuit breaker for builder bids. The beacon block is produced by the proposer | ||
| * regardless of bid source, so missed blocks are not a useful builder health signal. Instead | ||
| * count blocks whose payload was never revealed and stop selecting builder bids while the | ||
| * non-reveal rate in the fault inspection window is too high. | ||
| */ | ||
| export class BuilderCircuitBreaker { | ||
| readonly faultInspectionWindow: number; | ||
| readonly allowedFaults: number; | ||
|
|
||
| private active = false; | ||
| private lastUpdatedSlot = -1; | ||
|
|
||
| constructor( | ||
| opts: BuilderCircuitBreakerOpts, | ||
| private readonly modules: BuilderCircuitBreakerModules | ||
| ) { | ||
| const {faultInspectionWindow, allowedFaults} = getFaultInspectionParams(opts); | ||
| this.faultInspectionWindow = faultInspectionWindow; | ||
| this.allowedFaults = allowedFaults; | ||
| } | ||
|
|
||
| /** Whether builder bids must be ignored for a block produced at clockSlot */ | ||
| isActive(clockSlot: Slot): boolean { | ||
| this.update(clockSlot); | ||
| return this.active; | ||
| } | ||
|
|
||
| update(clockSlot: Slot): void { | ||
| if (clockSlot <= this.lastUpdatedSlot) { | ||
| return; | ||
| } | ||
| this.lastUpdatedSlot = clockSlot; | ||
|
|
||
| // Exclude clockSlot itself, its payload reveal may still be in flight | ||
| const {blocksPresent, payloadsRevealed} = this.modules.forkChoice.getPayloadRevealCounts( | ||
| Math.max(clockSlot - this.faultInspectionWindow, 0), | ||
| clockSlot - 1 | ||
| ); | ||
| const faults = blocksPresent - payloadsRevealed; | ||
|
|
||
| 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; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be great to also have If we have only observed say 4 blocks in the past 32 slots, we would probably want to activate this because chain is unhealthy. Numbers here are just arbitrary, just to illustrate an example
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
to be clear, the circuit breaker has nothing to do with blocks at all in gloas, I wouldn't activate the circuit breaker if there are <4 blocks in the epoch, but I think it could set a min threshold of blocks that are needed for the circuit breaker changes it's state, eg. in that case above, if it's <4, then the circuit breaker just keeps it's previous state, either activated or deactived |
||
|
|
||
| this.modules.metrics?.builderCircuitBreaker.active.set(this.active ? 1 : 0); | ||
| this.modules.metrics?.builderCircuitBreaker.faults.set(faults); | ||
| this.modules.metrics?.builderCircuitBreaker.blocksPresent.set(blocksPresent); | ||
| this.modules.metrics?.builderCircuitBreaker.payloadsRevealed.set(payloadsRevealed); | ||
|
|
||
| const logCtx = { | ||
| blocksPresent, | ||
| faults, | ||
| faultInspectionWindow: this.faultInspectionWindow, | ||
| allowedFaults: this.allowedFaults, | ||
| }; | ||
| if (this.active !== wasActive) { | ||
| if (this.active) { | ||
| this.modules.logger.warn("Builder circuit breaker activated, ignoring builder bids", logCtx); | ||
| } else { | ||
| this.modules.logger.info("Builder circuit breaker deactivated", logCtx); | ||
| } | ||
| } else { | ||
| this.modules.logger.verbose("Builder circuit breaker status", {active: this.active, ...logCtx}); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,10 @@ export type IChainOptions = BlockProcessOpts & | |
| archiveDateEpochs?: number; | ||
| nHistoricalStatesFileDataStore?: boolean; | ||
| nativeStateView?: boolean; | ||
| /** Builder circuit breaker fault inspection window in slots */ | ||
| faultInspectionWindow?: number; | ||
| /** Allowed unrevealed payloads within the fault inspection window */ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 If window = 32, allowedFaults = 8, observedBlocks = 16, then circuit breaker activates at 5 unrevealed payloads
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's better to define
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| allowedFaults?: number; | ||
| }; | ||
|
|
||
| export type BlockProcessOpts = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,6 +70,33 @@ export class NoBidReceived extends Error { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Beacon clients select randomized values from the following ranges when initializing | ||
| * the circuit breaker (so at boot time and once for each unique boot). | ||
|
Comment on lines
+74
to
+75
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this is just moved (existing code). But why are we randomizing these values?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added a comment here 6004e8e |
||
| * | ||
| * ALLOWED_FAULTS: between 1 and SLOTS_PER_EPOCH // 4 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Allowed faults is not between 1 and 8 (mainnet), but L93 calculation says between 8 and 15
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed by #9780 |
||
| * FAULT_INSPECTION_WINDOW: between SLOTS_PER_EPOCH and 2 * SLOTS_PER_EPOCH | ||
| * | ||
| * The values are randomized per node so builders cannot predict when a given proposer will | ||
| * fall back to local blocks. With fixed thresholds a builder could withhold payloads right up | ||
| * to the limit without ever tripping the breaker. | ||
| */ | ||
| export function getFaultInspectionParams(opts: {faultInspectionWindow?: number; allowedFaults?: number}): { | ||
| faultInspectionWindow: number; | ||
| allowedFaults: number; | ||
| } { | ||
| const faultInspectionWindow = Math.max( | ||
| opts.faultInspectionWindow ?? SLOTS_PER_EPOCH + Math.floor(Math.random() * SLOTS_PER_EPOCH), | ||
| SLOTS_PER_EPOCH | ||
| ); | ||
| // allowedFaults should be < faultInspectionWindow, limiting them to faultInspectionWindow/4 | ||
| const allowedFaults = Math.min( | ||
| opts.allowedFaults ?? Math.floor(faultInspectionWindow / 4), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get what you mean here so you are saying if
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah I think I got what you mean, sure can do that
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed by #9780 |
||
| Math.floor(faultInspectionWindow / 4) | ||
| ); | ||
| return {faultInspectionWindow, allowedFaults}; | ||
| } | ||
|
|
||
| /** | ||
| * Additional duration to account for potential event loop lag which causes | ||
| * builder blocks to be rejected even though the response was sent in time. | ||
|
|
@@ -122,23 +149,9 @@ export class ExecutionBuilderHttp implements IExecutionBuilder { | |
| this.registrations = new ValidatorRegistrationCache(); | ||
| this.issueLocalFcUWithFeeRecipient = opts.issueLocalFcUWithFeeRecipient; | ||
|
|
||
| /** | ||
| * 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 | ||
| * FAULT_INSPECTION_WINDOW: between SLOTS_PER_EPOCH and 2 * SLOTS_PER_EPOCH | ||
| * | ||
| */ | ||
| this.faultInspectionWindow = Math.max( | ||
| opts.faultInspectionWindow ?? SLOTS_PER_EPOCH + Math.floor(Math.random() * SLOTS_PER_EPOCH), | ||
| SLOTS_PER_EPOCH | ||
| ); | ||
| // allowedFaults should be < faultInspectionWindow, limiting them to faultInspectionWindow/4 | ||
| this.allowedFaults = Math.min( | ||
| opts.allowedFaults ?? Math.floor(this.faultInspectionWindow / 4), | ||
| Math.floor(this.faultInspectionWindow / 4) | ||
| ); | ||
| const {faultInspectionWindow, allowedFaults} = getFaultInspectionParams(opts); | ||
| this.faultInspectionWindow = faultInspectionWindow; | ||
| this.allowedFaults = allowedFaults; | ||
| } | ||
|
|
||
| updateStatus(status: BuilderStatus): void { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.