chore: address builder circuit breaker review follow-ups

packages/beacon-node/src/chain/builderCircuitBreaker.ts

@@ -15,11 +15,15 @@ export type BuilderCircuitBreakerModules = {
   metrics: Metrics | null;
 };
 
+/** Four observations is the minimum useful recovery sample for the default ~25% fault budget */
+const MIN_BLOCKS_TO_DEACTIVATE = 4;
+
 /**
  * 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.
+ * count blocks whose payload was never revealed. Activate when the non-reveal rate exceeds the
+ * fault budget, and resume selecting builder bids only when the observed blocks are within budget
+ * and meet the minimum recovery sample size.
  */
 export class BuilderCircuitBreaker {
   readonly faultInspectionWindow: number;
@@ -58,14 +62,21 @@ export class BuilderCircuitBreaker {
 
     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;
+    const exceedsFaultBudget = faults * this.faultInspectionWindow > this.allowedFaults * blocksPresent;
+    if (exceedsFaultBudget) {
+      this.active = true;
+    } else if (blocksPresent >= MIN_BLOCKS_TO_DEACTIVATE) {
+      // Require a minimum sample within the fault budget before accepting builder bids again
+      this.active = false;
+    }
 
     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 = {
+      clockSlot,
       blocksPresent,
       faults,
       faultInspectionWindow: this.faultInspectionWindow,

packages/beacon-node/src/chain/options.ts

@@ -53,7 +53,7 @@ export type IChainOptions = BlockProcessOpts &
     nativeStateView?: boolean;
     /** Builder circuit breaker fault inspection window in slots */
     faultInspectionWindow?: number;
-    /** Allowed unrevealed payloads within the fault inspection window */
+    /** Unrevealed payloads allowed per `faultInspectionWindow` observed blocks */
     allowedFaults?: number;
   };
 

packages/beacon-node/src/execution/builder/http.ts

@@ -71,15 +71,18 @@ 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).
+ * Default circuit breaker parameters:
  *
- * ALLOWED_FAULTS: between 1 and SLOTS_PER_EPOCH // 4
- * FAULT_INSPECTION_WINDOW: between SLOTS_PER_EPOCH and 2 * SLOTS_PER_EPOCH
+ * SLOTS_PER_EPOCH <= FAULT_INSPECTION_WINDOW < 2 * SLOTS_PER_EPOCH (randomized at initialization)
+ * ALLOWED_FAULTS: FAULT_INSPECTION_WINDOW // 4
  *
- * 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.
+ * e.g. on mainnet SLOTS_PER_EPOCH is 32, so FAULT_INSPECTION_WINDOW is between 32 and 63,
+ * and a window of 40 results in ALLOWED_FAULTS = 10.
+ *
+ * The randomized default window keeps the exact slots under inspection unpredictable per node,
+ * so a builder cannot tell when past faults age out of a given proposer's window and time
+ * withholding around trip or recovery points. Explicitly configured windows are clamped to at
+ * least SLOTS_PER_EPOCH, and configured allowedFaults is capped at the default ~25% budget.
  */
 export function getFaultInspectionParams(opts: {faultInspectionWindow?: number; allowedFaults?: number}): {
   faultInspectionWindow: number;
@@ -90,10 +93,7 @@ export function getFaultInspectionParams(opts: {faultInspectionWindow?: number;
     SLOTS_PER_EPOCH
   );
   // allowedFaults should be < faultInspectionWindow, limiting them to faultInspectionWindow/4
-  const allowedFaults = Math.min(
-    opts.allowedFaults ?? Math.floor(faultInspectionWindow / 4),
-    Math.floor(faultInspectionWindow / 4)
-  );
+  const allowedFaults = Math.min(opts.allowedFaults ?? Infinity, Math.floor(faultInspectionWindow / 4));
   return {faultInspectionWindow, allowedFaults};
 }
 

packages/beacon-node/test/unit/chain/builderCircuitBreaker.test.ts

@@ -21,12 +21,13 @@ describe("BuilderCircuitBreaker", () => {
   }
 
   const testCases: [string, {blocksPresent: number; payloadsRevealed: number}, boolean][] = [
-    ["empty window", {blocksPresent: 0, payloadsRevealed: 0}, false],
+    ["empty window keeps initial state", {blocksPresent: 0, payloadsRevealed: 0}, false],
     ["full window, no faults", {blocksPresent: 32, payloadsRevealed: 32}, false],
     ["full window, faults at budget", {blocksPresent: 32, payloadsRevealed: 24}, false],
     ["full window, faults above budget", {blocksPresent: 32, payloadsRevealed: 23}, true],
     ["sparse window, faults within scaled budget", {blocksPresent: 8, payloadsRevealed: 6}, false],
     ["sparse window, faults above scaled budget", {blocksPresent: 8, payloadsRevealed: 5}, true],
+    ["single unrevealed payload", {blocksPresent: 1, payloadsRevealed: 0}, true],
     ["sparse window, all payloads unrevealed", {blocksPresent: 4, payloadsRevealed: 0}, true],
   ];
 
@@ -43,6 +44,20 @@ describe("BuilderCircuitBreaker", () => {
     expect(getPayloadRevealCounts).toHaveBeenCalledWith(100 - faultInspectionWindow, 99);
   });
 
+  it("requires a minimum sample to deactivate", () => {
+    const {breaker, getPayloadRevealCounts} = setup({blocksPresent: 1, payloadsRevealed: 0});
+    expect(breaker.isActive(100)).toBe(true);
+
+    getPayloadRevealCounts.mockReturnValue({blocksPresent: 0, payloadsRevealed: 0});
+    expect(breaker.isActive(101)).toBe(true);
+
+    getPayloadRevealCounts.mockReturnValue({blocksPresent: 3, payloadsRevealed: 3});
+    expect(breaker.isActive(102)).toBe(true);
+
+    getPayloadRevealCounts.mockReturnValue({blocksPresent: 4, payloadsRevealed: 3});
+    expect(breaker.isActive(103)).toBe(false);
+  });
+
   it("only updates once per slot", () => {
     const {breaker, getPayloadRevealCounts} = setup({blocksPresent: 32, payloadsRevealed: 32});
     expect(breaker.isActive(100)).toBe(false);

packages/cli/src/options/beaconNodeOptions/builder.ts

@@ -62,7 +62,7 @@ export const options: CliCommandOptions<ExecutionBuilderArgs> = {
   "builder.allowedFaults": {
     type: "number",
     description:
-      "Number of missed slots (pre-gloas) or unrevealed payloads (post-gloas) allowed in the `faultInspectionWindow` for builder circuit",
+      "Number of missed slots allowed within `faultInspectionWindow` before ignoring the external builder (pre-gloas). Post-gloas, sets the tolerated rate of unrevealed payloads, defined as `allowedFaults` out of `faultInspectionWindow` and applied to blocks observed in the window",
     group: "builder",
   },
 };

packages/fork-choice/src/forkChoice/interface.ts

@@ -250,8 +250,8 @@ export interface IForkChoice {
   hasPayloadHexUnsafe(blockRoot: RootHex): boolean;
   getSlotsPresent(windowStart: number): number;
   /**
-   * Count gloas blocks with fromSlot <= slot <= toSlot and how many of them have a revealed
-   * payload (FULL variant exists). Used by the builder circuit breaker.
+   * Count Gloas blocks with fromSlot <= slot <= toSlot, and how many of them have a revealed payload
+   * (FULL variant exists). Used by the builder circuit breaker.
    */
   getPayloadRevealCounts(fromSlot: Slot, toSlot: Slot): {blocksPresent: number; payloadsRevealed: number};
   getPTCVotes(blockRootHex: RootHex): (boolean | null)[] | null;

packages/fork-choice/src/protoArray/protoArray.ts

@@ -1,5 +1,5 @@
 import {BitArray} from "@chainsafe/ssz";
-import {GENESIS_EPOCH, PTC_SIZE} from "@lodestar/params";
+import {GENESIS_EPOCH, GENESIS_SLOT, PTC_SIZE} from "@lodestar/params";
 import {DataAvailabilityStatus, computeEpochAtSlot, computeStartSlotAtEpoch} from "@lodestar/state-transition";
 import {Epoch, RootHex, Slot} from "@lodestar/types";
 import {bitCount, toRootHex} from "@lodestar/utils";
@@ -691,8 +691,8 @@ export class ProtoArray {
   }
 
   /**
-   * Count gloas blocks with fromSlot <= slot <= toSlot and how many of them have a revealed
-   * payload (FULL variant exists). Used by the builder circuit breaker.
+   * Count Gloas blocks with fromSlot <= slot <= toSlot, and how many of them have a revealed payload
+   * (FULL variant exists). Used by the builder circuit breaker.
    */
   getPayloadRevealCounts(fromSlot: Slot, toSlot: Slot): {blocksPresent: number; payloadsRevealed: number} {
     let blocksPresent = 0;
@@ -704,6 +704,10 @@ export class ProtoArray {
       if (node.slot < fromSlot || node.slot > toSlot || node.payloadStatus !== PayloadStatus.PENDING) {
         continue;
       }
+      // Genesis block is always EMPTY
+      if (node.slot === GENESIS_SLOT) {
+        continue;
+      }
       blocksPresent++;
       if (this.hasPayload(node.blockRoot)) {
         payloadsRevealed++;

packages/fork-choice/test/unit/protoArray/gloas.test.ts

@@ -1,6 +1,6 @@
 import {beforeEach, describe, expect, it} from "vitest";
 import {BitArray} from "@chainsafe/ssz";
-import {PTC_SIZE} from "@lodestar/params";
+import {GENESIS_SLOT, PTC_SIZE} from "@lodestar/params";
 import {DataAvailabilityStatus, computeStartSlotAtEpoch} from "@lodestar/state-transition";
 import {RootHex} from "@lodestar/types";
 import {ExecutionStatus, PayloadStatus, ProtoArray, ProtoBlock, ProtoNode} from "../../../src/index.js";
@@ -226,6 +226,40 @@ describe("Gloas Fork Choice", () => {
         payloadsRevealed: 0,
       });
     });
+
+    it("does not count the genesis block", () => {
+      const currentSlot = GENESIS_SLOT + 1;
+      const protoArray = ProtoArray.initialize(createTestBlock(GENESIS_SLOT, genesisRoot, "0x00", "0x00"), currentSlot);
+
+      protoArray.onBlock(createTestBlock(currentSlot, "0x02", genesisRoot, genesisRoot), currentSlot, null);
+
+      expect(protoArray.getPayloadRevealCounts(GENESIS_SLOT, currentSlot)).toEqual({
+        blocksPresent: 1,
+        payloadsRevealed: 0,
+      });
+    });
+
+    it("counts a non-genesis anchor block seeded at initialization", () => {
+      const anchorRoot = "0x02";
+      const currentSlot = gloasForkSlot + 1;
+      const protoArray = ProtoArray.initialize(createTestBlock(gloasForkSlot, anchorRoot, "0x00", "0x00"), currentSlot);
+
+      protoArray.onExecutionPayload(
+        anchorRoot,
+        currentSlot,
+        "0x02ff",
+        1,
+        30000000,
+        null,
+        ExecutionStatus.Valid,
+        DataAvailabilityStatus.Available
+      );
+
+      expect(protoArray.getPayloadRevealCounts(gloasForkSlot, currentSlot)).toEqual({
+        blocksPresent: 1,
+        payloadsRevealed: 1,
+      });
+    });
   });
 
   describe("Pre-Gloas (Fulu) behavior", () => {