fix: use proper state to verify attestations (#5500)

* fix: use proper state to verify attestations

* feat: add more metrics for gossip attestation verification

* chore: add unit tests

* Apply suggestions from code review

---------

Co-authored-by: Cayman <caymannava@gmail.com>

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<AttestationErrorType> {
     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};
 

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;
@@ -47,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) {
@@ -96,24 +106,21 @@ export async function validateGossipAggregateAndProof(
     attTarget.root,
     attSlot,
     attEpoch,
+    RegenCaller.validateGossipAggregateAndProof,
     chain.opts.maxSkipSlots
   );
 
   // [IGNORE] The current finalized_checkpoint is an ancestor of the block defined by aggregate.data.beacon_block_root
   // -- 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 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

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
@@ -91,6 +97,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 +157,7 @@ export async function validateGossipAttestation(
       attestationOrCache.attestation.data.target.root,
       attSlot,
       attEpoch,
+      RegenCaller.validateGossipAttestation,
       chain.opts.maxSkipSlots
     );
 
@@ -160,17 +172,13 @@ 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 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)
@@ -338,14 +346,18 @@ export function verifyHeadBlockAndTargetRoot(
   targetRoot: Root,
   attestationSlot: Slot,
   attestationEpoch: Epoch,
+  caller: string,
   maxSkipSlots?: number
 ): ProtoBlock {
   const headBlock = verifyHeadBlockIsKnown(chain, beaconBlockRoot);
   // Lighthouse rejects the attestation, however Lodestar only ignores considering it's not against the spec
   // 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,
@@ -356,6 +368,44 @@ 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<CachedBeaconStateAllForks> {
+  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) <= SHUFFLING_LOOK_AHEAD_EPOCHS;
+  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.
  *

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

@@ -531,6 +531,32 @@ 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"],
+      }),
+      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
     gossipBlock: {
       elapsedTimeTillReceived: register.histogram({

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> & StateRegenerator;
+  let chain: IBeaconChain;
+
+  beforeEach(() => {
+    regenStub = sandbox.createStubInstance(StateRegenerator) as SinonStubbedInstance<StateRegenerator> &
+      StateRegenerator;
+    chain = {
+      config: config as BeaconConfig,
+      regen: regenStub,
+    } as Partial<IBeaconChain> 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<ProtoBlock> as ProtoBlock;
+      expect(regenStub[regenCall].callCount).to.equal(0);
+      await getStateForAttestationVerification(
+        chain,
+        attSlot,
+        attEpoch,
+        attHeadBlock,
+        RegenCaller.validateGossipAttestation
+      );
+      expect(regenStub[regenCall].callCount).to.equal(1);
+    });
+  }
+});