From 7c378381045cbd5e6e3d51315e2113d1a1f90a11 Mon Sep 17 00:00:00 2001 From: Tuyen Nguyen Date: Wed, 17 May 2023 15:22:09 +0700 Subject: [PATCH 1/4] fix: use proper state to verify attestations --- .../src/chain/errors/attestationError.ts | 8 +-- .../src/chain/validation/aggregateAndProof.ts | 22 ++++---- .../src/chain/validation/attestation.ts | 52 ++++++++++++++++--- .../src/metrics/metrics/lodestar.ts | 14 +++++ 4 files changed, 75 insertions(+), 21 deletions(-) diff --git a/packages/beacon-node/src/chain/errors/attestationError.ts b/packages/beacon-node/src/chain/errors/attestationError.ts index 7a98a62109c6..b2b573d99d1b 100644 --- a/packages/beacon-node/src/chain/errors/attestationError.ts +++ b/packages/beacon-node/src/chain/errors/attestationError.ts @@ -115,9 +115,9 @@ export enum AttestationErrorCode { */ COMMITTEE_INDEX_OUT_OF_RANGE = "ATTESTATION_ERROR_COMMITTEE_INDEX_OUT_OF_RANGE", /** - * Missing attestation head state + * Missing state to verify attestation */ - MISSING_ATTESTATION_HEAD_STATE = "ATTESTATION_ERROR_MISSING_ATTESTATION_HEAD_STATE", + MISSING_STATE_TO_VERIFY_ATTESTATION = "ATTESTATION_ERROR_MISSING_STATE_TO_VERIFY_ATTESTATION", /** * Invalid aggregator. */ @@ -162,7 +162,7 @@ export type AttestationErrorType = | {code: AttestationErrorCode.INVALID_TARGET_ROOT; targetRoot: RootHex; expected: string | null} | {code: AttestationErrorCode.TARGET_BLOCK_NOT_AN_ANCESTOR_OF_LMD_BLOCK} | {code: AttestationErrorCode.COMMITTEE_INDEX_OUT_OF_RANGE; index: number} - | {code: AttestationErrorCode.MISSING_ATTESTATION_HEAD_STATE; error: Error} + | {code: AttestationErrorCode.MISSING_STATE_TO_VERIFY_ATTESTATION; error: Error} | {code: AttestationErrorCode.INVALID_AGGREGATOR} | {code: AttestationErrorCode.INVALID_INDEXED_ATTESTATION} | {code: AttestationErrorCode.INVALID_SERIALIZED_BYTES} @@ -174,7 +174,7 @@ export class AttestationError extends GossipActionError { switch (type.code) { case AttestationErrorCode.UNKNOWN_TARGET_ROOT: return {code: type.code, root: toHexString(type.root)}; - case AttestationErrorCode.MISSING_ATTESTATION_HEAD_STATE: + case AttestationErrorCode.MISSING_STATE_TO_VERIFY_ATTESTATION: // TODO: The stack trace gets lost here return {code: type.code, error: type.error.message}; diff --git a/packages/beacon-node/src/chain/validation/aggregateAndProof.ts b/packages/beacon-node/src/chain/validation/aggregateAndProof.ts index a6b6d897ef94..93a76fb05a7a 100644 --- a/packages/beacon-node/src/chain/validation/aggregateAndProof.ts +++ b/packages/beacon-node/src/chain/validation/aggregateAndProof.ts @@ -12,7 +12,12 @@ import {AttestationError, AttestationErrorCode, GossipAction} from "../errors/in import {RegenCaller} from "../regen/index.js"; import {getAttDataBase64FromSignedAggregateAndProofSerialized} from "../../util/sszBytes.js"; import {getSelectionProofSignatureSet, getAggregateAndProofSignatureSet} from "./signatureSets/index.js"; -import {getCommitteeIndices, verifyHeadBlockAndTargetRoot, verifyPropagationSlotRange} from "./attestation.js"; +import { + getCommitteeIndices, + getStateForAttestationVerification, + verifyHeadBlockAndTargetRoot, + verifyPropagationSlotRange, +} from "./attestation.js"; export type AggregateAndProofValidationResult = { indexedAttestation: phase0.IndexedAttestation; @@ -106,14 +111,13 @@ export async function validateGossipAggregateAndProof( // Using the target checkpoint state here caused unstable memory issue // See https://github.com/ChainSafe/lodestar/issues/4896 // TODO: https://github.com/ChainSafe/lodestar/issues/4900 - const attHeadState = await chain.regen - .getState(attHeadBlock.stateRoot, RegenCaller.validateGossipAggregateAndProof) - .catch((e: Error) => { - throw new AttestationError(GossipAction.IGNORE, { - code: AttestationErrorCode.MISSING_ATTESTATION_HEAD_STATE, - error: e as Error, - }); - }); + const attHeadState = await getStateForAttestationVerification( + chain, + attSlot, + attEpoch, + attHeadBlock, + RegenCaller.validateGossipAggregateAndProof + ); const committeeIndices: number[] = cachedAttData ? cachedAttData.committeeIndices diff --git a/packages/beacon-node/src/chain/validation/attestation.ts b/packages/beacon-node/src/chain/validation/attestation.ts index 776e14ef4ac0..690e005d646e 100644 --- a/packages/beacon-node/src/chain/validation/attestation.ts +++ b/packages/beacon-node/src/chain/validation/attestation.ts @@ -163,14 +163,13 @@ export async function validateGossipAttestation( // Using the target checkpoint state here caused unstable memory issue // See https://github.com/ChainSafe/lodestar/issues/4896 // TODO: https://github.com/ChainSafe/lodestar/issues/4900 - const attHeadState = await chain.regen - .getState(attHeadBlock.stateRoot, RegenCaller.validateGossipAttestation) - .catch((e: Error) => { - throw new AttestationError(GossipAction.IGNORE, { - code: AttestationErrorCode.MISSING_ATTESTATION_HEAD_STATE, - error: e as Error, - }); - }); + const attHeadState = await getStateForAttestationVerification( + chain, + attSlot, + attEpoch, + attHeadBlock, + RegenCaller.validateGossipAttestation + ); // [REJECT] The committee index is within the expected range // -- i.e. data.index < get_committee_count_per_slot(state, data.target.epoch) @@ -356,6 +355,43 @@ export function verifyHeadBlockAndTargetRoot( return headBlock; } +/** + * Get a state for attestation verification. + * Use head state if: + * - attestation slot is in the same fork as head block + * - head state includes committees of target epoch + * + * Otherwise, regenerate state from head state dialing to target epoch + */ +export async function getStateForAttestationVerification( + chain: IBeaconChain, + attSlot: Slot, + attEpoch: Epoch, + attHeadBlock: ProtoBlock, + regenCaller: RegenCaller +): Promise { + const isSameFork = chain.config.getForkSeq(attSlot) === chain.config.getForkSeq(attHeadBlock.slot); + // thanks for 1 epoch look ahead of shuffling, a state at epoch n can get committee for epoch n+1 + const headStateHasTargetEpochCommmittee = attEpoch - computeEpochAtSlot(attHeadBlock.slot) <= 1; + try { + if (isSameFork && headStateHasTargetEpochCommmittee) { + // most of the time it should just use head state + chain.metrics?.gossipAttestation.useHeadBlockState.inc({caller: regenCaller}); + return await chain.regen.getState(attHeadBlock.stateRoot, regenCaller); + } + + // at fork boundary we should dial head state to target epoch + // see https://github.com/ChainSafe/lodestar/pull/4849 + chain.metrics?.gossipAttestation.useHeadBlockStateDialedToTargetEpoch.inc({caller: regenCaller}); + return await chain.regen.getBlockSlotState(attHeadBlock.blockRoot, attSlot, {dontTransferCache: true}, regenCaller); + } catch (e) { + throw new AttestationError(GossipAction.IGNORE, { + code: AttestationErrorCode.MISSING_STATE_TO_VERIFY_ATTESTATION, + error: e as Error, + }); + } +} + /** * Checks if the `attestation.data.beaconBlockRoot` is known to this chain. * diff --git a/packages/beacon-node/src/metrics/metrics/lodestar.ts b/packages/beacon-node/src/metrics/metrics/lodestar.ts index 879b6aaa6f3b..caf065e90e81 100644 --- a/packages/beacon-node/src/metrics/metrics/lodestar.ts +++ b/packages/beacon-node/src/metrics/metrics/lodestar.ts @@ -531,6 +531,20 @@ export function createLodestarMetrics( }), }, + // Gossip attestation + gossipAttestation: { + useHeadBlockState: register.gauge<"caller">({ + name: "lodestar_gossip_attestation_use_head_block_state_count", + help: "Count of gossip attestation verification using head block state", + labelNames: ["caller"], + }), + useHeadBlockStateDialedToTargetEpoch: register.gauge<"caller">({ + name: "lodestar_gossip_attestation_use_head_block_state_dialed_to_target_epoch_count", + help: "Count of gossip attestation verification using head block state and dialed to target epoch", + labelNames: ["caller"], + }), + }, + // Gossip block gossipBlock: { elapsedTimeTillReceived: register.histogram({ From dfa0e7e9d3418fc4e5e59445a4ae0dd39ad977b2 Mon Sep 17 00:00:00 2001 From: Tuyen Nguyen Date: Wed, 17 May 2023 15:50:13 +0700 Subject: [PATCH 2/4] feat: add more metrics for gossip attestation verification --- .../src/chain/validation/aggregateAndProof.ts | 6 ++++++ .../beacon-node/src/chain/validation/attestation.ts | 12 +++++++++++- packages/beacon-node/src/metrics/metrics/lodestar.ts | 12 ++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/packages/beacon-node/src/chain/validation/aggregateAndProof.ts b/packages/beacon-node/src/chain/validation/aggregateAndProof.ts index 93a76fb05a7a..a3df4335b0f4 100644 --- a/packages/beacon-node/src/chain/validation/aggregateAndProof.ts +++ b/packages/beacon-node/src/chain/validation/aggregateAndProof.ts @@ -52,6 +52,11 @@ export async function validateGossipAggregateAndProof( const attTarget = attData.target; const targetEpoch = attTarget.epoch; + chain.metrics?.gossipAttestation.attestationSlotToClockSlot.observe( + {caller: RegenCaller.validateGossipAggregateAndProof}, + chain.clock.currentSlot - attSlot + ); + if (!cachedAttData) { // [REJECT] The attestation's epoch matches its target -- i.e. attestation.data.target.epoch == compute_epoch_at_slot(attestation.data.slot) if (targetEpoch !== attEpoch) { @@ -101,6 +106,7 @@ export async function validateGossipAggregateAndProof( attTarget.root, attSlot, attEpoch, + RegenCaller.validateGossipAggregateAndProof, chain.opts.maxSkipSlots ); diff --git a/packages/beacon-node/src/chain/validation/attestation.ts b/packages/beacon-node/src/chain/validation/attestation.ts index 690e005d646e..b48ff1119f4a 100644 --- a/packages/beacon-node/src/chain/validation/attestation.ts +++ b/packages/beacon-node/src/chain/validation/attestation.ts @@ -91,6 +91,11 @@ export async function validateGossipAttestation( const attTarget = attData.target; const targetEpoch = attTarget.epoch; + chain.metrics?.gossipAttestation.attestationSlotToClockSlot.observe( + {caller: RegenCaller.validateGossipAttestation}, + chain.clock.currentSlot - attSlot + ); + if (!attestationOrCache.cache) { // [REJECT] The attestation's epoch matches its target -- i.e. attestation.data.target.epoch == compute_epoch_at_slot(attestation.data.slot) if (targetEpoch !== attEpoch) { @@ -146,6 +151,7 @@ export async function validateGossipAttestation( attestationOrCache.attestation.data.target.root, attSlot, attEpoch, + RegenCaller.validateGossipAttestation, chain.opts.maxSkipSlots ); @@ -337,6 +343,7 @@ export function verifyHeadBlockAndTargetRoot( targetRoot: Root, attestationSlot: Slot, attestationEpoch: Epoch, + caller: string, maxSkipSlots?: number ): ProtoBlock { const headBlock = verifyHeadBlockIsKnown(chain, beaconBlockRoot); @@ -344,7 +351,10 @@ export function verifyHeadBlockAndTargetRoot( // it's more about a DOS protection to us // With verifyPropagationSlotRange() and maxSkipSlots = 32, it's unlikely we have to regenerate states in queue // to validate beacon_attestation and aggregate_and_proof - if (maxSkipSlots !== undefined && attestationSlot - headBlock.slot > maxSkipSlots) { + const slotDistance = attestationSlot - headBlock.slot; + chain.metrics?.gossipAttestation.headSlotToAttestationSlot.observe({caller}, slotDistance); + + if (maxSkipSlots !== undefined && slotDistance > maxSkipSlots) { throw new AttestationError(GossipAction.IGNORE, { code: AttestationErrorCode.TOO_MANY_SKIPPED_SLOTS, attestationSlot, diff --git a/packages/beacon-node/src/metrics/metrics/lodestar.ts b/packages/beacon-node/src/metrics/metrics/lodestar.ts index caf065e90e81..e28c49db9dde 100644 --- a/packages/beacon-node/src/metrics/metrics/lodestar.ts +++ b/packages/beacon-node/src/metrics/metrics/lodestar.ts @@ -543,6 +543,18 @@ export function createLodestarMetrics( help: "Count of gossip attestation verification using head block state and dialed to target epoch", labelNames: ["caller"], }), + headSlotToAttestationSlot: register.histogram<"caller">({ + name: "lodestar_gossip_attestation_head_slot_to_attestation_slot", + help: "Slot distance between attestation slot and head slot", + labelNames: ["caller"], + buckets: [0, 1, 2, 4, 8, 16, 32, 64], + }), + attestationSlotToClockSlot: register.histogram<"caller">({ + name: "lodestar_gossip_attestation_attestation_slot_to_clock_slot", + help: "Slot distance between clock slot and attestation slot", + labelNames: ["caller"], + buckets: [0, 1, 2, 4, 8, 16, 32, 64], + }), }, // Gossip block From 508274f0280e19ab8cbd38848c28123fbc46380d Mon Sep 17 00:00:00 2001 From: Tuyen Nguyen Date: Tue, 23 May 2023 10:13:52 +0700 Subject: [PATCH 3/4] chore: add unit tests --- .../src/chain/validation/attestation.ts | 9 +- .../unit/chain/validation/attestation.test.ts | 87 ++++++++++++++++++- 2 files changed, 91 insertions(+), 5 deletions(-) diff --git a/packages/beacon-node/src/chain/validation/attestation.ts b/packages/beacon-node/src/chain/validation/attestation.ts index b48ff1119f4a..bee0b126fc2f 100644 --- a/packages/beacon-node/src/chain/validation/attestation.ts +++ b/packages/beacon-node/src/chain/validation/attestation.ts @@ -40,6 +40,12 @@ export type AttestationOrBytes = attSlot: Slot; }; +/** + * The beacon chain shufflings are designed to provide 1 epoch lookahead + * At each state, we have previous shuffling, current shuffling and next shuffling + */ +const SHUFFLING_LOOK_AHEAD_EPOCHS = 1; + /** * Only deserialize the attestation if needed, use the cached AttestationData instead * This is to avoid deserializing similar attestation multiple times which could help the gc @@ -382,7 +388,8 @@ export async function getStateForAttestationVerification( ): Promise { const isSameFork = chain.config.getForkSeq(attSlot) === chain.config.getForkSeq(attHeadBlock.slot); // thanks for 1 epoch look ahead of shuffling, a state at epoch n can get committee for epoch n+1 - const headStateHasTargetEpochCommmittee = attEpoch - computeEpochAtSlot(attHeadBlock.slot) <= 1; + const headStateHasTargetEpochCommmittee = + attEpoch - computeEpochAtSlot(attHeadBlock.slot) <= SHUFFLING_LOOK_AHEAD_EPOCHS; try { if (isSameFork && headStateHasTargetEpochCommmittee) { // most of the time it should just use head state diff --git a/packages/beacon-node/test/unit/chain/validation/attestation.test.ts b/packages/beacon-node/test/unit/chain/validation/attestation.test.ts index 46319787cf93..24cdd90042df 100644 --- a/packages/beacon-node/test/unit/chain/validation/attestation.test.ts +++ b/packages/beacon-node/test/unit/chain/validation/attestation.test.ts @@ -1,15 +1,25 @@ +import sinon, {SinonStubbedInstance} from "sinon"; +import {expect} from "chai"; import {SLOTS_PER_EPOCH} from "@lodestar/params"; import {BitArray} from "@chainsafe/ssz"; -import {processSlots} from "@lodestar/state-transition"; -import {ssz} from "@lodestar/types"; +import {computeEpochAtSlot, computeStartSlotAtEpoch, processSlots} from "@lodestar/state-transition"; +import {defaultChainConfig, createChainForkConfig, BeaconConfig} from "@lodestar/config"; +import {Slot, ssz} from "@lodestar/types"; +import {ProtoBlock} from "@lodestar/fork-choice"; import {IBeaconChain} from "../../../../src/chain/index.js"; import {AttestationErrorCode, GossipErrorCode} from "../../../../src/chain/errors/index.js"; -import {AttestationOrBytes, validateGossipAttestation} from "../../../../src/chain/validation/index.js"; +import { + AttestationOrBytes, + getStateForAttestationVerification, + validateGossipAttestation, +} from "../../../../src/chain/validation/index.js"; import {expectRejectedWithLodestarError} from "../../../utils/errors.js"; import {generateTestCachedBeaconStateOnlyValidators} from "../../../../../state-transition/test/perf/util.js"; import {memoOnce} from "../../../utils/cache.js"; import {getAttestationValidData, AttestationValidDataOpts} from "../../../utils/validationData/attestation.js"; -import {IStateRegenerator} from "../../../../src/chain/regen/interface.js"; +import {IStateRegenerator, RegenCaller} from "../../../../src/chain/regen/interface.js"; +import {StateRegenerator} from "../../../../src/chain/regen/regen.js"; +import {ZERO_HASH_HEX} from "../../../../src/constants/constants.js"; describe("chain / validation / attestation", () => { const vc = 64; @@ -281,3 +291,72 @@ describe("chain / validation / attestation", () => { await expectRejectedWithLodestarError(validateGossipAttestation(chain, attestationOrBytes, subnet), errorCode); } }); + +describe("getStateForAttestationVerification", () => { + // eslint-disable-next-line @typescript-eslint/naming-convention + const config = createChainForkConfig({...defaultChainConfig, CAPELLA_FORK_EPOCH: 2}); + const sandbox = sinon.createSandbox(); + let regenStub: SinonStubbedInstance & StateRegenerator; + let chain: IBeaconChain; + + beforeEach(() => { + regenStub = sandbox.createStubInstance(StateRegenerator) as SinonStubbedInstance & + StateRegenerator; + chain = { + config: config as BeaconConfig, + regen: regenStub, + } as Partial as IBeaconChain; + }); + + afterEach(() => { + sandbox.restore(); + }); + + const forkSlot = computeStartSlotAtEpoch(config.CAPELLA_FORK_EPOCH); + const getBlockSlotStateTestCases: {id: string; attSlot: Slot; headSlot: Slot; regenCall: keyof StateRegenerator}[] = [ + { + id: "should call regen.getBlockSlotState at fork boundary", + attSlot: forkSlot + 1, + headSlot: forkSlot - 1, + regenCall: "getBlockSlotState", + }, + { + id: "should call regen.getBlockSlotState if > 1 epoch difference", + attSlot: forkSlot + 2 * SLOTS_PER_EPOCH, + headSlot: forkSlot + 1, + regenCall: "getBlockSlotState", + }, + { + id: "should call getState if 1 epoch difference", + attSlot: forkSlot + 2 * SLOTS_PER_EPOCH, + headSlot: forkSlot + SLOTS_PER_EPOCH, + regenCall: "getState", + }, + { + id: "should call getState if 0 epoch difference", + attSlot: forkSlot + 2 * SLOTS_PER_EPOCH, + headSlot: forkSlot + 2 * SLOTS_PER_EPOCH, + regenCall: "getState", + }, + ]; + + for (const {id, attSlot, headSlot, regenCall} of getBlockSlotStateTestCases) { + it(id, async () => { + const attEpoch = computeEpochAtSlot(attSlot); + const attHeadBlock = { + slot: headSlot, + stateRoot: ZERO_HASH_HEX, + blockRoot: ZERO_HASH_HEX, + } as Partial as ProtoBlock; + expect(regenStub[regenCall].callCount).to.equal(0); + await getStateForAttestationVerification( + chain, + attSlot, + attEpoch, + attHeadBlock, + RegenCaller.validateGossipAttestation + ); + expect(regenStub[regenCall].callCount).to.equal(1); + }); + } +}); From 8304fe8e8b6eda7f32bbb8956f0119d9ca4a0a83 Mon Sep 17 00:00:00 2001 From: Cayman Date: Wed, 24 May 2023 12:05:27 -0400 Subject: [PATCH 4/4] Apply suggestions from code review --- packages/beacon-node/src/chain/validation/aggregateAndProof.ts | 3 --- packages/beacon-node/src/chain/validation/attestation.ts | 3 --- 2 files changed, 6 deletions(-) diff --git a/packages/beacon-node/src/chain/validation/aggregateAndProof.ts b/packages/beacon-node/src/chain/validation/aggregateAndProof.ts index a3df4335b0f4..982818a48a7a 100644 --- a/packages/beacon-node/src/chain/validation/aggregateAndProof.ts +++ b/packages/beacon-node/src/chain/validation/aggregateAndProof.ts @@ -114,9 +114,6 @@ export async function validateGossipAggregateAndProof( // -- i.e. get_ancestor(store, aggregate.data.beacon_block_root, compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)) == store.finalized_checkpoint.root // > Altready check in `chain.forkChoice.hasBlock(attestation.data.beaconBlockRoot)` - // Using the target checkpoint state here caused unstable memory issue - // See https://github.com/ChainSafe/lodestar/issues/4896 - // TODO: https://github.com/ChainSafe/lodestar/issues/4900 const attHeadState = await getStateForAttestationVerification( chain, attSlot, diff --git a/packages/beacon-node/src/chain/validation/attestation.ts b/packages/beacon-node/src/chain/validation/attestation.ts index bee0b126fc2f..02e4f45d2ef8 100644 --- a/packages/beacon-node/src/chain/validation/attestation.ts +++ b/packages/beacon-node/src/chain/validation/attestation.ts @@ -172,9 +172,6 @@ export async function validateGossipAttestation( // --i.e. get_ancestor(store, attestation.data.beacon_block_root, compute_start_slot_at_epoch(attestation.data.target.epoch)) == attestation.data.target.root // > Altready check in `verifyHeadBlockAndTargetRoot()` - // Using the target checkpoint state here caused unstable memory issue - // See https://github.com/ChainSafe/lodestar/issues/4896 - // TODO: https://github.com/ChainSafe/lodestar/issues/4900 const attHeadState = await getStateForAttestationVerification( chain, attSlot,