From 42c80973a54f2b3dbb0f6f17ffc779d9ef38555b Mon Sep 17 00:00:00 2001 From: tuyennhv Date: Tue, 23 May 2023 07:55:22 +0700 Subject: [PATCH] feat: more metrics for sync committee message validation (#5516) feat: track equivocation in sync committee message validation --- .../src/chain/errors/syncCommitteeError.ts | 10 ++- .../src/chain/seenCache/seenCommittee.ts | 17 ++-- .../src/chain/validation/syncCommittee.ts | 34 ++++++-- .../src/metrics/metrics/lodestar.ts | 12 +++ .../chain/seenCache/syncCommittee.test.ts | 19 ++--- .../chain/validation/syncCommittee.test.ts | 78 ++++++++++++++++++- 6 files changed, 143 insertions(+), 27 deletions(-) diff --git a/packages/beacon-node/src/chain/errors/syncCommitteeError.ts b/packages/beacon-node/src/chain/errors/syncCommitteeError.ts index 8c8f4dc9b6c4..e4a92729cecd 100644 --- a/packages/beacon-node/src/chain/errors/syncCommitteeError.ts +++ b/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} diff --git a/packages/beacon-node/src/chain/seenCache/seenCommittee.ts b/packages/beacon-node/src/chain/seenCache/seenCommittee.ts index c68ff82ffa0b..38d7f67c74a2 100644 --- a/packages/beacon-node/src/chain/seenCache/seenCommittee.ts +++ b/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>(() => new Set()); + private readonly seenCacheBySlot = new MapDef>(() => 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}`; } diff --git a/packages/beacon-node/src/chain/validation/syncCommittee.ts b/packages/beacon-node/src/chain/validation/syncCommittee.ts index 2fe39bf4c76b..79b4bc092f66 100644 --- a/packages/beacon-node/src/chain/validation/syncCommittee.ts +++ b/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}; } diff --git a/packages/beacon-node/src/metrics/metrics/lodestar.ts b/packages/beacon-node/src/metrics/metrics/lodestar.ts index 408fcd813c9f..879b6aaa6f3b 100644 --- a/packages/beacon-node/src/metrics/metrics/lodestar.ts +++ b/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({ diff --git a/packages/beacon-node/test/unit/chain/seenCache/syncCommittee.test.ts b/packages/beacon-node/test/unit/chain/seenCache/syncCommittee.test.ts index 04a5b3c511da..c420a7d50dd0 100644 --- a/packages/beacon-node/test/unit/chain/seenCache/syncCommittee.test.ts +++ b/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; }); }); diff --git a/packages/beacon-node/test/unit/chain/validation/syncCommittee.test.ts b/packages/beacon-node/test/unit/chain/validation/syncCommittee.test.ts index e530a445f3c0..71c50c7ca517 100644 --- a/packages/beacon-node/test/unit/chain/validation/syncCommittee.test.ts +++ b/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; + let forkchoiceStub: SinonStubbedInstance; // let computeSubnetsForSyncCommitteeStub: SinonStubFn; 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 {