Add partial selection proof to duty object

packages/validator/src/services/attestation.ts

@@ -1,4 +1,4 @@
-import {phase0, Slot, ssz} from "@lodestar/types";
+import {BLSSignature, phase0, Slot, ssz} from "@lodestar/types";
 import {computeEpochAtSlot, isAggregatorFromCommitteeLength} from "@lodestar/state-transition";
 import {sleep} from "@lodestar/utils";
 import {Api, ApiError, routes} from "@lodestar/api";
@@ -246,7 +246,7 @@ export class AttestationService {
     const logCtx = {slot: attestation.slot, index: attestation.index};
 
     // No validator is aggregator, skip
-    if (duties.every(({isAggregator}) => !isAggregator)) {
+    if (duties.every(({selectionProof}) => selectionProof === null)) {
       return;
     }
 
@@ -262,11 +262,11 @@ export class AttestationService {
     const signedAggregateAndProofs: phase0.SignedAggregateAndProof[] = [];
 
     await Promise.all(
-      duties.map(async ({duty, selectionProof, isAggregator}) => {
+      duties.map(async ({duty, selectionProof}) => {
         const logCtxValidator = {...logCtx, validatorIndex: duty.validatorIndex};
         try {
           // Produce signed aggregates only for validators that are subscribed aggregators.
-          if (isAggregator) {
+          if (selectionProof !== null) {
             signedAggregateAndProofs.push(
               await this.validatorStore.signAggregateAndProof(duty, selectionProof, aggregate.data)
             );
@@ -297,7 +297,8 @@ export class AttestationService {
    *
    * 1. Exchange partial for combined selection proofs
    * 2. Determine validators that should aggregate attestations
-   * 3. Resubscribe aggregators on beacon committee subnets
+   * 3. Mutate duty objects to set selection proofs for aggregators
+   * 4. Resubscribe aggregators on beacon committee subnets
    *
    * See https://docs.google.com/document/d/1q9jOTPcYQa-3L8luRvQJ-M0eegtba4Nmon3dpO79TMk/mobilebasic
    */
@@ -306,11 +307,13 @@ export class AttestationService {
     slot: number,
     signal: AbortSignal
   ): Promise<void> {
-    const partialSelections: routes.validator.BeaconCommitteeSelection[] = duties.map(({duty, selectionProof}) => ({
-      validatorIndex: duty.validatorIndex,
-      slot,
-      selectionProof,
-    }));
+    const partialSelections: routes.validator.BeaconCommitteeSelection[] = duties.map(
+      ({duty, partialSelectionProof}) => ({
+        validatorIndex: duty.validatorIndex,
+        slot,
+        selectionProof: partialSelectionProof as BLSSignature,
+      })
+    );
 
     this.logger.debug("Submitting partial selection proofs", {slot, count: partialSelections.length});
 
@@ -320,7 +323,7 @@ export class AttestationService {
       // beacon node would likely not have enough time to prepare an aggregate attestation.
       // Note that the aggregations flow is not explicitly exited but rather will be skipped
       // due to the fact that calculation of `is_aggregator` in duties service is not done
-      // and defaulted to `isAggregator=false`, meaning no validator will be an aggregator.
+      // and selectionProof is set to null, meaning no validator will be considered an aggregator.
       sleep(this.clock.msToSlot(slot + 1 / 3), signal),
     ]);
 
@@ -336,21 +339,20 @@ export class AttestationService {
 
     for (const dutyAndProof of duties) {
       const {validatorIndex, committeeIndex, committeeLength, committeesAtSlot} = dutyAndProof.duty;
-      const selection = combinedSelections.find((s) => s.validatorIndex === validatorIndex);
+      const combinedSelection = combinedSelections.find((s) => s.validatorIndex === validatorIndex);
       const logCtxValidator = {slot, index: committeeIndex, validatorIndex};
 
-      if (!selection) {
+      if (!combinedSelection) {
         this.logger.warn("Did not receive combined selection proof", logCtxValidator);
         continue;
       }
 
-      const isAggregator = isAggregatorFromCommitteeLength(committeeLength, selection.selectionProof);
-
-      // Replace partial with combined selection proof by mutating object
-      dutyAndProof.selectionProof = selection.selectionProof;
-      dutyAndProof.isAggregator = isAggregator;
+      const isAggregator = isAggregatorFromCommitteeLength(committeeLength, combinedSelection.selectionProof);
 
       if (isAggregator) {
+        // Update selection proof by mutating duty object
+        dutyAndProof.selectionProof = combinedSelection.selectionProof;
+
         // Only push subnet subscriptions with `isAggregator=true` as all validators
         // with duties for slot are already subscribed to subnets with `isAggregator=false`.
         beaconCommitteeSubscriptions.push({
@@ -368,7 +370,7 @@ export class AttestationService {
     if (beaconCommitteeSubscriptions.length > 0) {
       try {
         ApiError.assert(await this.api.validator.prepareBeaconCommitteeSubnet(beaconCommitteeSubscriptions));
-        this.logger.debug("Resubscribed validators as aggregators on beacon committee subnet", {
+        this.logger.debug("Resubscribed validators as aggregators on beacon committee subnets", {
           slot,
           count: beaconCommitteeSubscriptions.length,
         });

packages/validator/src/services/attestationDuties.ts

@@ -24,10 +24,10 @@ const SUBSCRIPTIONS_PER_REQUEST = 8738;
 /** Neatly joins the server-generated `AttesterData` with the locally-generated `selectionProof`. */
 export type AttDutyAndProof = {
   duty: routes.validator.AttesterDuty;
-  /** Locally-generated selection proof, only partial if validator is part of distributed cluster */
-  selectionProof: BLSSignature;
-  /** Whether the validator is an aggregator */
-  isAggregator: boolean;
+  /** This value is only set to not null if the proof indicates that the validator is an aggregator. */
+  selectionProof: BLSSignature | null;
+  /** This value will be set if validator is part of distributed cluster and only has a key share */
+  partialSelectionProof?: BLSSignature;
 };
 
 // To assist with readability
@@ -183,14 +183,14 @@ export class AttestationDutiesService {
     for (const epoch of [currentEpoch, nextEpoch]) {
       const epochDuties = this.dutiesByIndexByEpoch.get(epoch)?.dutiesByIndex;
       if (epochDuties) {
-        for (const {duty, isAggregator} of epochDuties.values()) {
+        for (const {duty, selectionProof} of epochDuties.values()) {
           if (indexSet.has(duty.validatorIndex)) {
             beaconCommitteeSubscriptions.push({
               validatorIndex: duty.validatorIndex,
               committeesAtSlot: duty.committeesAtSlot,
               committeeIndex: duty.committeeIndex,
               slot: duty.slot,
-              isAggregator,
+              isAggregator: selectionProof !== null,
             });
           }
         }
@@ -337,11 +337,18 @@ export class AttestationDutiesService {
     if (this.opts?.distributedAggregationSelection) {
       // Validator in distributed cluster only has a key share, not the full private key.
       // Passing a partial selection proof to `is_aggregator` would produce incorrect result.
-      // Attestation service will combine selection proofs and determine aggregators at beginning of the slot.
-      return {duty, selectionProof, isAggregator: false};
+      // Attestation service will exchange partial for combined selection proofs retrieved from
+      // distributed validator middleware client and determine aggregators at beginning of every slot.
+      return {duty, selectionProof: null, partialSelectionProof: selectionProof};
     }
 
-    return {duty, selectionProof, isAggregator: isAggregatorFromCommitteeLength(duty.committeeLength, selectionProof)};
+    const isAggregator = isAggregatorFromCommitteeLength(duty.committeeLength, selectionProof);
+
+    return {
+      duty,
+      // selectionProof === null is used to check if is aggregator
+      selectionProof: isAggregator ? selectionProof : null,
+    };
   }
 
   /** Run once per epoch to prune duties map */

packages/validator/test/unit/services/attestation.test.ts

@@ -85,7 +85,6 @@ describe("AttestationService", function () {
           pubkey: pubkeys[0],
         },
         selectionProof: ZERO_HASH,
-        isAggregator: true,
       },
     ];
 

packages/validator/test/unit/services/attestationDuties.test.ts

@@ -3,7 +3,7 @@ import {expect} from "chai";
 import sinon from "sinon";
 import {chainConfig} from "@lodestar/config/default";
 import bls from "@chainsafe/bls";
-import {fromHexString, toHexString} from "@chainsafe/ssz";
+import {toHexString} from "@chainsafe/ssz";
 import {HttpStatusCode, routes} from "@lodestar/api";
 import {ssz} from "@lodestar/types";
 import {computeEpochAtSlot} from "@lodestar/state-transition";
@@ -35,11 +35,6 @@ describe("AttestationDutiesService", function () {
     status: "active",
     validator: ssz.phase0.Validator.defaultValue(),
   };
-  const signedAttSelectionProof = fromHexString(
-    "0x8d80fe4be57500d2fe4f99c7d5586d9bd65ea4f9e4def0591020dd66f7d1daad" +
-      "1cea7520beb815423e2bc8316949ac2606da80d5d00df34f352b5a946d6a4bb4" +
-      "7e402f5d1167dec97af9742e61820625c5c792ddd2b8796962243d8e8cbeadee"
-  );
 
   before(() => {
     const secretKeys = [bls.SecretKey.fromBytes(toBufferBE(BigInt(98), 32))];
@@ -101,23 +96,23 @@ describe("AttestationDutiesService", function () {
       Object.fromEntries(dutiesService["dutiesByIndexByEpoch"].get(epoch)?.dutiesByIndex || new Map())
     ).to.deep.equal(
       {
-        // Since the ZERO_HASH won't pass the isAggregator test
-        [index]: {duty, selectionProof: signedAttSelectionProof, isAggregator: false},
+        // Since the ZERO_HASH won't pass the isAggregator test, selectionProof is null
+        [index]: {duty, selectionProof: null},
       },
       "Wrong dutiesService.attesters Map at current epoch"
     );
     expect(
       Object.fromEntries(dutiesService["dutiesByIndexByEpoch"].get(epoch + 1)?.dutiesByIndex || new Map())
     ).to.deep.equal(
       {
-        // Since the ZERO_HASH won't pass the isAggregator test
-        [index]: {duty, selectionProof: signedAttSelectionProof, isAggregator: false},
+        // Since the ZERO_HASH won't pass the isAggregator test, selectionProof is null
+        [index]: {duty, selectionProof: null},
       },
       "Wrong dutiesService.attesters Map at next epoch"
     );
 
     expect(dutiesService.getDutiesAtSlot(slot)).to.deep.equal(
-      [{duty, selectionProof: signedAttSelectionProof, isAggregator: false}],
+      [{duty, selectionProof: null}],
       "Wrong getAttestersAtSlot()"
     );
 
@@ -170,13 +165,13 @@ describe("AttestationDutiesService", function () {
     // first confirm duties for this and next epoch should be persisted
     expect(Object.fromEntries(dutiesService["dutiesByIndexByEpoch"].get(0)?.dutiesByIndex || new Map())).to.deep.equal(
       {
-        4: {duty: duty, selectionProof: signedAttSelectionProof, isAggregator: false},
+        4: {duty: duty, selectionProof: null},
       },
       "Wrong dutiesService.attesters Map at current epoch"
     );
     expect(Object.fromEntries(dutiesService["dutiesByIndexByEpoch"].get(1)?.dutiesByIndex || new Map())).to.deep.equal(
       {
-        4: {duty: duty, selectionProof: signedAttSelectionProof, isAggregator: false},
+        4: {duty: duty, selectionProof: null},
       },
       "Wrong dutiesService.attesters Map at current epoch"
     );