Correct Aggregate Inclusion metric

packages/lodestar/src/api/impl/validator/index.ts

@@ -439,8 +439,9 @@ export function getValidatorApi({chain, config, logger, metrics, network, sync}:
           try {
             // TODO: Validate in batch
             const {indexedAttestation, committeeIndices} = await validateGossipAggregateAndProof(
-              chain,
-              signedAggregateAndProof
+              {chain, metrics},
+              signedAggregateAndProof,
+              seenTimestampSec
             );
 
             chain.aggregatedAttestationPool.add(

packages/lodestar/src/chain/validation/aggregateAndProof.ts

@@ -9,12 +9,14 @@ import {
 import {IBeaconChain} from "..";
 import {AttestationError, AttestationErrorCode, GossipAction} from "../errors/index.js";
 import {RegenCaller} from "../regen/index.js";
+import {IMetrics} from "../../metrics/index.js";
 import {getSelectionProofSignatureSet, getAggregateAndProofSignatureSet} from "./signatureSets/index.js";
 import {getCommitteeIndices, verifyHeadBlockAndTargetRoot, verifyPropagationSlotRange} from "./attestation.js";
 
 export async function validateGossipAggregateAndProof(
-  chain: IBeaconChain,
-  signedAggregateAndProof: phase0.SignedAggregateAndProof
+  {chain, metrics = null}: {chain: IBeaconChain; metrics?: IMetrics | null},
+  signedAggregateAndProof: phase0.SignedAggregateAndProof,
+  seenTimestampSec: number
 ): Promise<{indexedAttestation: phase0.IndexedAttestation; committeeIndices: ValidatorIndex[]}> {
   // Do checks in this order:
   // - do early checks (w/o indexed attestation)
@@ -54,16 +56,6 @@ export async function validateGossipAggregateAndProof(
     });
   }
 
-  // _[IGNORE]_ A valid aggregate attestation defined by `hash_tree_root(aggregate.data)` whose `aggregation_bits`
-  // is a non-strict superset has _not_ already been seen.
-  if (chain.seenAggregatedAttestations.isKnown(targetEpoch, attDataRoot, aggregationBits)) {
-    throw new AttestationError(GossipAction.IGNORE, {
-      code: AttestationErrorCode.ATTESTERS_ALREADY_KNOWN,
-      targetEpoch,
-      aggregateRoot: attDataRoot,
-    });
-  }
-
   // [IGNORE] The block being voted for (attestation.data.beacon_block_root) has been seen (via both gossip
   // and non-gossip sources) (a client MAY queue attestations for processing once block is retrieved).
   const attHeadBlock = verifyHeadBlockAndTargetRoot(chain, attData.beaconBlockRoot, attTarget.root, attEpoch);
@@ -109,6 +101,20 @@ export async function validateGossipAggregateAndProof(
     throw new AttestationError(GossipAction.REJECT, {code: AttestationErrorCode.AGGREGATOR_NOT_IN_COMMITTEE});
   }
 
+  // a lot of AggregateAndProof is known by chain.seenAggregatedAttestations
+  // we want to count number of aggregated attestations include our validators' attestation before that check
+  metrics?.registerGossipAggregatedAttestation(seenTimestampSec, signedAggregateAndProof, indexedAttestation);
+
+  // _[IGNORE]_ A valid aggregate attestation defined by `hash_tree_root(aggregate.data)` whose `aggregation_bits`
+  // is a non-strict superset has _not_ already been seen.
+  if (chain.seenAggregatedAttestations.isKnown(targetEpoch, attDataRoot, aggregationBits)) {
+    throw new AttestationError(GossipAction.IGNORE, {
+      code: AttestationErrorCode.ATTESTERS_ALREADY_KNOWN,
+      targetEpoch,
+      aggregateRoot: attDataRoot,
+    });
+  }
+
   // [REJECT] The aggregate_and_proof.selection_proof is a valid signature of the aggregate.data.slot
   // by the validator with index aggregate_and_proof.aggregator_index.
   // [REJECT] The aggregator signature, signed_aggregate_and_proof.signature, is valid.

packages/lodestar/src/metrics/validatorMonitor.ts

@@ -112,7 +112,7 @@ type EpochSummary = {
   /** The delay between when the attestation should have been produced and when it was observed. */
   attestationMinDelay: Seconds | null;
   /** The number of times a validators attestation was seen in an aggregate. */
-  attestationAggregateIncusions: number;
+  attestationAggregateInclusions: number;
   /** The number of times a validators attestation was seen in a block. */
   attestationBlockInclusions: number;
   /** The minimum observed inclusion distance for an attestation for this epoch.. */
@@ -137,7 +137,7 @@ function withEpochSummary(validator: MonitoredValidator, epoch: Epoch, fn: (summ
     summary = {
       attestations: 0,
       attestationMinDelay: null,
-      attestationAggregateIncusions: 0,
+      attestationAggregateInclusions: 0,
       attestationBlockInclusions: 0,
       attestationMinBlockInclusionDistance: null,
       blocks: 0,
@@ -370,7 +370,7 @@ export function createValidatorMonitor(
           metrics.validatorMonitor.attestationInAggregateTotal.inc({src, index});
           metrics.validatorMonitor.attestationInAggregateDelaySeconds.observe({src, index}, delaySec);
           withEpochSummary(validator, epoch, (summary) => {
-            summary.attestationAggregateIncusions += 1;
+            summary.attestationAggregateInclusions += 1;
           });
           logger.debug("Local validator attestation is included in AggregatedAndProof", {
             validatorIndex: validator.index,
@@ -457,7 +457,7 @@ export function createValidatorMonitor(
           metrics.validatorMonitor.prevEpochAttestationsMinDelaySeconds.observe({index}, summary.attestationMinDelay);
         metrics.validatorMonitor.prevEpochAttestationAggregateInclusions.set(
           {index},
-          summary.attestationAggregateIncusions
+          summary.attestationAggregateInclusions
         );
         metrics.validatorMonitor.prevEpochAttestationBlockInclusions.set({index}, summary.attestationBlockInclusions);
         if (summary.attestationMinBlockInclusionDistance !== null) {

packages/lodestar/src/network/gossip/handlers/index.ts

@@ -148,7 +148,11 @@ export function getGossipHandlers(modules: ValidatorFnsModules, options: GossipH
     [GossipType.beacon_aggregate_and_proof]: async (signedAggregateAndProof, _topic, _peer, seenTimestampSec) => {
       let validationResult: {indexedAttestation: phase0.IndexedAttestation; committeeIndices: number[]};
       try {
-        validationResult = await validateGossipAggregateAndProofRetryUnknownRoot(chain, signedAggregateAndProof);
+        validationResult = await validateGossipAggregateAndProofRetryUnknownRoot(
+          {chain, metrics},
+          signedAggregateAndProof,
+          seenTimestampSec
+        );
       } catch (e) {
         if (e instanceof AttestationError && e.action === GossipAction.REJECT) {
           const archivedPath = chain.persistInvalidSszObject(
@@ -163,7 +167,6 @@ export function getGossipHandlers(modules: ValidatorFnsModules, options: GossipH
 
       // Handler
       const {indexedAttestation, committeeIndices} = validationResult;
-      metrics?.registerGossipAggregatedAttestation(seenTimestampSec, signedAggregateAndProof, indexedAttestation);
       const aggregatedAttestation = signedAggregateAndProof.message.aggregate;
 
       chain.aggregatedAttestationPool.add(
@@ -321,14 +324,15 @@ export function getGossipHandlers(modules: ValidatorFnsModules, options: GossipH
  * both from gossip and the API. I also prevents having to catch and re-throw in multiple places.
  */
 async function validateGossipAggregateAndProofRetryUnknownRoot(
-  chain: IBeaconChain,
-  signedAggregateAndProof: phase0.SignedAggregateAndProof
+  {chain, metrics}: {chain: IBeaconChain; metrics: IMetrics | null},
+  signedAggregateAndProof: phase0.SignedAggregateAndProof,
+  seenTimestampSec: number
 ): Promise<ReturnType<typeof validateGossipAggregateAndProof>> {
   let unknownBlockRootRetries = 0;
   // eslint-disable-next-line no-constant-condition
   while (true) {
     try {
-      return await validateGossipAggregateAndProof(chain, signedAggregateAndProof);
+      return await validateGossipAggregateAndProof({chain, metrics}, signedAggregateAndProof, seenTimestampSec);
     } catch (e) {
       if (
         e instanceof AttestationError &&

packages/lodestar/test/perf/chain/validation/aggregateAndProof.test.ts

@@ -13,6 +13,7 @@ describe("validate gossip signedAggregateAndProof", () => {
   });
 
   const aggStruct = signedAggregateAndProof;
+  const seenTimestampSec = Date.now() / 1000;
 
   for (const [id, agg] of Object.entries({struct: aggStruct})) {
     itBench({
@@ -22,7 +23,7 @@ describe("validate gossip signedAggregateAndProof", () => {
         chain.seenAggregatedAttestations["aggregateRootsByEpoch"].clear();
       },
       fn: async () => {
-        await validateGossipAggregateAndProof(chain, agg);
+        await validateGossipAggregateAndProof({chain}, agg, seenTimestampSec);
       },
     });
   }

packages/lodestar/test/unit/chain/validation/aggregateAndProof.test.ts

@@ -40,7 +40,7 @@ describe("chain / validation / aggregateAndProof", () => {
   it("Valid", async () => {
     const {chain, signedAggregateAndProof} = getValidData({});
 
-    await validateGossipAggregateAndProof(chain, signedAggregateAndProof);
+    await validateGossipAggregateAndProof({chain}, signedAggregateAndProof, Date.now() / 1000);
   });
 
   it("BAD_TARGET_EPOCH", async () => {
@@ -170,6 +170,9 @@ describe("chain / validation / aggregateAndProof", () => {
     signedAggregateAndProof: phase0.SignedAggregateAndProof,
     errorCode: AttestationErrorCode
   ): Promise<void> {
-    await expectRejectedWithLodestarError(validateGossipAggregateAndProof(chain, signedAggregateAndProof), errorCode);
+    await expectRejectedWithLodestarError(
+      validateGossipAggregateAndProof({chain}, signedAggregateAndProof, Date.now() / 1000),
+      errorCode
+    );
   }
 });