feat: more metrics for sync committee message validation

packages/beacon-node/src/chain/errors/syncCommitteeError.ts

@@ -1,9 +1,10 @@
-import {ValidatorIndex, Slot} from "@lodestar/types";
+import {ValidatorIndex, Slot, RootHex} from "@lodestar/types";
 import {GossipActionError} from "./gossipValidation.js";
 
 export enum SyncCommitteeErrorCode {
   NOT_CURRENT_SLOT = "SYNC_COMMITTEE_ERROR_NOT_CURRENT_SLOT",
   UNKNOWN_BEACON_BLOCK_ROOT = "SYNC_COMMITTEE_ERROR_UNKNOWN_BEACON_BLOCK_ROOT",
+  SYNC_COMMITTEE_MESSAGE_KNOWN = "SYNC_COMMITTEE_ERROR_SYNC_COMMITTEE_MESSAGE_KNOWN",
   SYNC_COMMITTEE_AGGREGATOR_ALREADY_KNOWN = "SYNC_COMMITTEE_ERROR_SYNC_COMMITTEE_AGGREGATOR_ALREADY_KNOWN",
   SYNC_COMMITTEE_PARTICIPANTS_ALREADY_KNOWN = "SYNC_COMMITTEE_ERROR_SYNC_COMMITTEE_PARTICIPANTS_ALREADY_KNOWN",
   VALIDATOR_NOT_IN_SYNC_COMMITTEE = "SYNC_COMMITTEE_ERROR_VALIDATOR_NOT_IN_SYNC_COMMITTEE",
@@ -16,6 +17,13 @@ export enum SyncCommitteeErrorCode {
 export type SyncCommitteeErrorType =
   | {code: SyncCommitteeErrorCode.NOT_CURRENT_SLOT; slot: Slot; currentSlot: Slot}
   | {code: SyncCommitteeErrorCode.UNKNOWN_BEACON_BLOCK_ROOT; beaconBlockRoot: Uint8Array}
+  | {
+      code: SyncCommitteeErrorCode.SYNC_COMMITTEE_MESSAGE_KNOWN;
+      validatorIndex: ValidatorIndex;
+      slot: Slot;
+      prevRoot: RootHex;
+      newRoot: RootHex;
+    }
   | {code: SyncCommitteeErrorCode.SYNC_COMMITTEE_AGGREGATOR_ALREADY_KNOWN}
   | {code: SyncCommitteeErrorCode.SYNC_COMMITTEE_PARTICIPANTS_ALREADY_KNOWN}
   | {code: SyncCommitteeErrorCode.VALIDATOR_NOT_IN_SYNC_COMMITTEE; validatorIndex: ValidatorIndex}

packages/beacon-node/src/chain/seenCache/seenCommittee.ts

@@ -1,4 +1,4 @@
-import {SubcommitteeIndex, Slot, ValidatorIndex} from "@lodestar/types";
+import {SubcommitteeIndex, Slot, ValidatorIndex, RootHex} from "@lodestar/types";
 import {MapDef} from "@lodestar/utils";
 
 /**
@@ -7,24 +7,25 @@ import {MapDef} from "@lodestar/utils";
 const MAX_SLOTS_IN_CACHE = 3;
 
 /** ValidataorSubnetKey = `validatorIndex + subcommitteeIndex` */
-type ValidataorSubnetKey = string;
+type ValidatorSubnetKey = string;
 
 /**
  * Cache seen SyncCommitteeMessage by slot + validator index.
  */
 export class SeenSyncCommitteeMessages {
-  private readonly seenCacheBySlot = new MapDef<Slot, Set<ValidataorSubnetKey>>(() => new Set<ValidataorSubnetKey>());
+  private readonly seenCacheBySlot = new MapDef<Slot, Map<ValidatorSubnetKey, RootHex>>(() => new Map());
 
   /**
    * based on slot + validator index
    */
-  isKnown(slot: Slot, subnet: SubcommitteeIndex, validatorIndex: ValidatorIndex): boolean {
-    return this.seenCacheBySlot.get(slot)?.has(seenCacheKey(subnet, validatorIndex)) === true;
+  get(slot: Slot, subnet: SubcommitteeIndex, validatorIndex: ValidatorIndex): RootHex | null {
+    const root = this.seenCacheBySlot.getOrDefault(slot).get(seenCacheKey(subnet, validatorIndex));
+    return root ?? null;
   }
 
   /** Register item as seen in the cache */
-  add(slot: Slot, subnet: SubcommitteeIndex, validatorIndex: ValidatorIndex): void {
-    this.seenCacheBySlot.getOrDefault(slot).add(seenCacheKey(subnet, validatorIndex));
+  add(slot: Slot, subnet: SubcommitteeIndex, validatorIndex: ValidatorIndex, root: RootHex): void {
+    this.seenCacheBySlot.getOrDefault(slot).set(seenCacheKey(subnet, validatorIndex), root);
   }
 
   /** Prune per clock slot */
@@ -37,6 +38,6 @@ export class SeenSyncCommitteeMessages {
   }
 }
 
-function seenCacheKey(subnet: number, validatorIndex: ValidatorIndex): ValidataorSubnetKey {
+function seenCacheKey(subnet: number, validatorIndex: ValidatorIndex): ValidatorSubnetKey {
   return `${subnet}-${validatorIndex}`;
 }

packages/beacon-node/src/chain/validation/syncCommittee.ts

@@ -1,6 +1,7 @@
 import {CachedBeaconStateAllForks} from "@lodestar/state-transition";
 import {SYNC_COMMITTEE_SUBNET_SIZE, SYNC_COMMITTEE_SUBNET_COUNT} from "@lodestar/params";
 import {altair} from "@lodestar/types";
+import {toHexString} from "@chainsafe/ssz";
 import {GossipAction, SyncCommitteeError, SyncCommitteeErrorCode} from "../errors/index.js";
 import {IBeaconChain} from "../interface.js";
 import {getSyncCommitteeSignatureSet} from "./signatureSets/index.js";
@@ -15,7 +16,8 @@ export async function validateGossipSyncCommittee(
   syncCommittee: altair.SyncCommitteeMessage,
   subnet: number
 ): Promise<{indexInSubcommittee: IndexInSubcommittee}> {
-  const {slot, validatorIndex} = syncCommittee;
+  const {slot, validatorIndex, beaconBlockRoot} = syncCommittee;
+  const messageRoot = toHexString(beaconBlockRoot);
 
   const headState = chain.getHeadState();
   const indexInSubcommittee = validateGossipSyncCommitteeExceptSig(chain, headState, subnet, syncCommittee);
@@ -30,10 +32,30 @@ export async function validateGossipSyncCommittee(
 
   // [IGNORE] There has been no other valid sync committee signature for the declared slot for the validator referenced
   // by sync_committee_signature.validator_index.
-  if (chain.seenSyncCommitteeMessages.isKnown(slot, subnet, validatorIndex)) {
-    throw new SyncCommitteeError(GossipAction.IGNORE, {
-      code: SyncCommitteeErrorCode.SYNC_COMMITTEE_AGGREGATOR_ALREADY_KNOWN,
-    });
+  const prevRoot = chain.seenSyncCommitteeMessages.get(slot, subnet, validatorIndex);
+  if (prevRoot) {
+    let shouldIgnore = false;
+    if (prevRoot === messageRoot) {
+      shouldIgnore = true;
+    } else {
+      const headRoot = chain.forkChoice.getHeadRoot();
+      chain.metrics?.gossipSyncCommittee.equivocationCount.inc();
+      if (messageRoot === headRoot) {
+        chain.metrics?.gossipSyncCommittee.equivocationToHeadCount.inc();
+      } else {
+        shouldIgnore = true;
+      }
+    }
+
+    if (shouldIgnore) {
+      throw new SyncCommitteeError(GossipAction.IGNORE, {
+        code: SyncCommitteeErrorCode.SYNC_COMMITTEE_MESSAGE_KNOWN,
+        validatorIndex,
+        slot,
+        prevRoot,
+        newRoot: messageRoot,
+      });
+    }
   }
 
   // [REJECT] The subnet_id is valid for the given validator, i.e. subnet_id in compute_subnets_for_sync_committee(state, sync_committee_signature.validator_index).
@@ -44,7 +66,7 @@ export async function validateGossipSyncCommittee(
   await validateSyncCommitteeSigOnly(chain, headState, syncCommittee);
 
   // Register this valid item as seen
-  chain.seenSyncCommitteeMessages.add(slot, subnet, validatorIndex);
+  chain.seenSyncCommitteeMessages.add(slot, subnet, validatorIndex, messageRoot);
 
   return {indexInSubcommittee};
 }

packages/beacon-node/src/metrics/metrics/lodestar.ts

@@ -519,6 +519,18 @@ export function createLodestarMetrics(
       }),
     },
 
+    // Gossip sync committee
+    gossipSyncCommittee: {
+      equivocationCount: register.counter({
+        name: "lodestar_gossip_sync_committee_equivocation_count",
+        help: "Count of sync committee messages with same validator index for different block roots",
+      }),
+      equivocationToHeadCount: register.counter({
+        name: "lodestar_gossip_sync_committee_equivocation_to_head_count",
+        help: "Count of sync committee messages which conflict to a previous message but elect the head",
+      }),
+    },
+
     // Gossip block
     gossipBlock: {
       elapsedTimeTillReceived: register.histogram({

packages/beacon-node/test/unit/chain/seenCache/syncCommittee.test.ts

@@ -10,29 +10,30 @@ describe("chain / seenCache / SeenSyncCommittee caches", function () {
     const slot = 10;
     const subnet = 2;
     const validatorIndex = 100;
+    const rootHex = "0x1234";
 
     it("should find a sync committee based on same slot and validator index", () => {
       const cache = new SeenSyncCommitteeMessages();
 
-      expect(cache.isKnown(slot, subnet, validatorIndex)).to.equal(false, "Should not know before adding");
-      cache.add(slot, subnet, validatorIndex);
-      expect(cache.isKnown(slot, subnet, validatorIndex)).to.equal(true, "Should know before adding");
+      expect(cache.get(slot, subnet, validatorIndex), "Should not know before adding").to.be.null;
+      cache.add(slot, subnet, validatorIndex, rootHex);
+      expect(cache.get(slot, subnet, validatorIndex)).to.equal(rootHex, "Should know before adding");
 
-      expect(cache.isKnown(slot + 1, subnet, validatorIndex)).to.equal(false, "Should not know a diff slot");
-      expect(cache.isKnown(slot, subnet + 1, validatorIndex)).to.equal(false, "Should not know a diff subnet");
-      expect(cache.isKnown(slot, subnet, validatorIndex + 1)).to.equal(false, "Should not know a diff index");
+      expect(cache.get(slot + 1, subnet, validatorIndex), "Should not know a diff slot").to.be.null;
+      expect(cache.get(slot, subnet + 1, validatorIndex), "Should not know a diff subnet").to.be.null;
+      expect(cache.get(slot, subnet, validatorIndex + 1), "Should not know a diff index").to.be.null;
     });
 
     it("should prune", () => {
       const cache = new SeenSyncCommitteeMessages();
 
       for (let i = 0; i < NUM_SLOTS_IN_CACHE; i++) {
-        cache.add(slot, subnet, validatorIndex);
+        cache.add(slot + i, subnet, validatorIndex, rootHex);
       }
 
-      expect(cache.isKnown(slot, subnet, validatorIndex)).to.equal(true, "Should know before prune");
+      expect(cache.get(slot, subnet, validatorIndex)).to.equal(rootHex, "Should know before prune");
       cache.prune(99);
-      expect(cache.isKnown(slot, subnet, validatorIndex)).to.equal(false, "Should not know after prune");
+      expect(cache.get(slot, subnet, validatorIndex), "Should not know after prune").to.be.null;
     });
   });
 

packages/beacon-node/test/unit/chain/validation/syncCommittee.test.ts

@@ -1,8 +1,11 @@
 import sinon from "sinon";
 import {SinonStubbedInstance} from "sinon";
+import {expect} from "chai";
 import {altair, Epoch, Slot} from "@lodestar/types";
 import {SLOTS_PER_EPOCH} from "@lodestar/params";
 import {createChainForkConfig, defaultChainConfig} from "@lodestar/config";
+import {toHexString} from "@chainsafe/ssz";
+import {ForkChoice, IForkChoice} from "@lodestar/fork-choice";
 import {BeaconChain} from "../../../../src/chain/index.js";
 import {Clock} from "../../../../src/util/clock.js";
 import {SyncCommitteeErrorCode} from "../../../../src/chain/errors/syncCommitteeError.js";
@@ -21,6 +24,7 @@ describe("Sync Committee Signature validation", function () {
   const sandbox = sinon.createSandbox();
   let chain: StubbedChain;
   let clockStub: SinonStubbedInstance<Clock>;
+  let forkchoiceStub: SinonStubbedInstance<ForkChoice>;
   // let computeSubnetsForSyncCommitteeStub: SinonStubFn<typeof syncCommitteeUtils["computeSubnetsForSyncCommittee"]>;
   let altairForkEpochBk: Epoch;
   const altairForkEpoch = 2020;
@@ -49,6 +53,8 @@ describe("Sync Committee Signature validation", function () {
     clockStub = sandbox.createStubInstance(Clock);
     chain.clock = clockStub;
     clockStub.isCurrentSlotGivenGossipDisparity.returns(true);
+    forkchoiceStub = sandbox.createStubInstance(ForkChoice);
+    (chain as {forkChoice: IForkChoice}).forkChoice = forkchoiceStub;
   });
 
   afterEach(function () {
@@ -66,14 +72,27 @@ describe("Sync Committee Signature validation", function () {
     );
   });
 
-  it("should throw error - there has been another valid sync committee signature for the declared slot", async function () {
+  it("should throw error - messageRoot is same to prevRoot", async function () {
     const syncCommittee = getSyncCommitteeSignature(currentSlot, validatorIndexInSyncCommittee);
     const headState = generateCachedAltairState({slot: currentSlot}, altairForkEpoch);
     chain.getHeadState.returns(headState);
-    chain.seenSyncCommitteeMessages.isKnown = () => true;
+    chain.seenSyncCommitteeMessages.get = () => toHexString(syncCommittee.beaconBlockRoot);
     await expectRejectedWithLodestarError(
       validateGossipSyncCommittee(chain, syncCommittee, 0),
-      SyncCommitteeErrorCode.SYNC_COMMITTEE_AGGREGATOR_ALREADY_KNOWN
+      SyncCommitteeErrorCode.SYNC_COMMITTEE_MESSAGE_KNOWN
+    );
+  });
+
+  it("should throw error - messageRoot is different to prevRoot but not forkchoice head", async function () {
+    const syncCommittee = getSyncCommitteeSignature(currentSlot, validatorIndexInSyncCommittee);
+    const headState = generateCachedAltairState({slot: currentSlot}, altairForkEpoch);
+    chain.getHeadState.returns(headState);
+    const prevRoot = "0x1234";
+    chain.seenSyncCommitteeMessages.get = () => prevRoot;
+    forkchoiceStub.getHeadRoot.returns(prevRoot);
+    await expectRejectedWithLodestarError(
+      validateGossipSyncCommittee(chain, syncCommittee, 0),
+      SyncCommitteeErrorCode.SYNC_COMMITTEE_MESSAGE_KNOWN
     );
   });
 
@@ -113,6 +132,59 @@ describe("Sync Committee Signature validation", function () {
       SyncCommitteeErrorCode.INVALID_SIGNATURE
     );
   });
+
+  it("should pass, no prev root", async function () {
+    const syncCommittee = getSyncCommitteeSignature(currentSlot, validatorIndexInSyncCommittee);
+    const subnet = 3;
+    const {slot, validatorIndex} = syncCommittee;
+    const headState = generateCachedAltairState({slot: currentSlot}, altairForkEpoch);
+
+    chain.getHeadState.returns(headState);
+    chain.bls = new BlsVerifierMock(true);
+    expect(chain.seenSyncCommitteeMessages.get(slot, subnet, validatorIndex), "should be null").to.be.null;
+    await validateGossipSyncCommittee(chain, syncCommittee, subnet);
+    expect(chain.seenSyncCommitteeMessages.get(slot, subnet, validatorIndex)).to.be.equal(
+      toHexString(syncCommittee.beaconBlockRoot),
+      "should add message root to seenSyncCommitteeMessages"
+    );
+
+    // receive same message again
+    await expectRejectedWithLodestarError(
+      validateGossipSyncCommittee(chain, syncCommittee, subnet),
+      SyncCommitteeErrorCode.SYNC_COMMITTEE_MESSAGE_KNOWN
+    );
+  });
+
+  it("should pass, there is prev root but message root is forkchoice head", async function () {
+    const syncCommittee = getSyncCommitteeSignature(currentSlot, validatorIndexInSyncCommittee);
+    const headState = generateCachedAltairState({slot: currentSlot}, altairForkEpoch);
+
+    chain.getHeadState.returns(headState);
+    chain.bls = new BlsVerifierMock(true);
+
+    const subnet = 3;
+    const {slot, validatorIndex} = syncCommittee;
+    const prevRoot = "0x1234";
+    chain.seenSyncCommitteeMessages.add(slot, subnet, validatorIndex, prevRoot);
+    expect(chain.seenSyncCommitteeMessages.get(slot, subnet, validatorIndex)).to.be.equal(
+      prevRoot,
+      "cache should return prevRoot"
+    );
+    // but forkchoice head is message root
+    forkchoiceStub.getHeadRoot.returns(toHexString(syncCommittee.beaconBlockRoot));
+    await validateGossipSyncCommittee(chain, syncCommittee, subnet);
+    // should accept the message and overwrite prevRoot
+    expect(chain.seenSyncCommitteeMessages.get(slot, subnet, validatorIndex)).to.be.equal(
+      toHexString(syncCommittee.beaconBlockRoot),
+      "should add message root to seenSyncCommitteeMessages"
+    );
+
+    // receive same message again
+    await expectRejectedWithLodestarError(
+      validateGossipSyncCommittee(chain, syncCommittee, subnet),
+      SyncCommitteeErrorCode.SYNC_COMMITTEE_MESSAGE_KNOWN
+    );
+  });
 });
 
 function getSyncCommitteeSignature(slot: Slot, validatorIndex: number): altair.SyncCommitteeMessage {