feedback and test improvement

ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/util/AttestationUtil.java

@@ -252,24 +252,39 @@ public SafeFuture<AttestationProcessingResult> isValidIndexedAttestationAsync(
           AttestationProcessingResult.invalid("Attesting indices include non-existent validator"));
     }
 
-    final BLSSignature signature = indexedAttestation.getSignature();
+    return validateAttestationDataSignature(
+        fork,
+        state,
+        pubkeys,
+        indexedAttestation.getSignature(),
+        indexedAttestation.getData(),
+        signatureVerifier);
+  }
+
+  protected SafeFuture<AttestationProcessingResult> validateAttestationDataSignature(
+      final Fork fork,
+      final BeaconState state,
+      final List<BLSPublicKey> publicKeys,
+      final BLSSignature signature,
+      final AttestationData attestationData,
+      final AsyncBLSSignatureVerifier signatureVerifier) {
+
     final Bytes32 domain =
         beaconStateAccessors.getDomain(
             Domain.BEACON_ATTESTER,
-            indexedAttestation.getData().getTarget().getEpoch(),
+            attestationData.getTarget().getEpoch(),
             fork,
             state.getGenesisValidatorsRoot());
-    final Bytes signingRoot = miscHelpers.computeSigningRoot(indexedAttestation.getData(), domain);
+    final Bytes signingRoot = miscHelpers.computeSigningRoot(attestationData, domain);
 
     return signatureVerifier
-        .verify(pubkeys, signingRoot, signature)
+        .verify(publicKeys, signingRoot, signature)
         .thenApply(
             isValidSignature -> {
               if (isValidSignature) {
                 return AttestationProcessingResult.SUCCESSFUL;
               } else {
-                LOG.debug(
-                    "AttestationUtil.is_valid_indexed_attestation: Verify aggregate signature");
+                LOG.debug("AttestationUtil.validateAttestationSignature: Verify signature");
                 return AttestationProcessingResult.invalid("Signature is invalid");
               }
             });

ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/electra/util/AttestationUtilElectra.java

@@ -23,15 +23,11 @@
 import java.util.stream.IntStream;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
-import org.apache.tuweni.bytes.Bytes;
-import org.apache.tuweni.bytes.Bytes32;
 import tech.pegasys.teku.bls.BLSPublicKey;
-import tech.pegasys.teku.bls.BLSSignature;
 import tech.pegasys.teku.infrastructure.async.SafeFuture;
 import tech.pegasys.teku.infrastructure.ssz.collections.SszBitlist;
 import tech.pegasys.teku.infrastructure.unsigned.UInt64;
 import tech.pegasys.teku.spec.config.SpecConfig;
-import tech.pegasys.teku.spec.constants.Domain;
 import tech.pegasys.teku.spec.datastructures.attestation.ValidatableAttestation;
 import tech.pegasys.teku.spec.datastructures.blocks.BeaconBlockSummary;
 import tech.pegasys.teku.spec.datastructures.operations.Attestation;
@@ -228,25 +224,13 @@ private SafeFuture<AttestationProcessingResult> validateSingleAttestationSignatu
           AttestationProcessingResult.invalid("Attesting index include non-existent validator"));
     }
 
-    final BLSSignature signature = singleAttestation.getSignature();
-    final Bytes32 domain =
-        beaconStateAccessors.getDomain(
-            Domain.BEACON_ATTESTER,
-            singleAttestation.getData().getTarget().getEpoch(),
-            fork,
-            state.getGenesisValidatorsRoot());
-    final Bytes signingRoot = miscHelpers.computeSigningRoot(singleAttestation.getData(), domain);
-
-    return signatureVerifier
-        .verify(pubkey.get(), signingRoot, signature)
-        .thenApply(
-            isValidSignature -> {
-              if (isValidSignature) {
-                return AttestationProcessingResult.SUCCESSFUL;
-              } else {
-                return AttestationProcessingResult.invalid("Signature is invalid");
-              }
-            });
+    return validateAttestationDataSignature(
+        fork,
+        state,
+        List.of(pubkey.get()),
+        singleAttestation.getSignature(),
+        singleAttestation.getData(),
+        signatureVerifier);
   }
 
   private SszBitlist getSingleAttestationAggregationBits(
@@ -257,9 +241,8 @@ private SszBitlist getSingleAttestationAggregationBits(
             singleAttestation.getData().getSlot(),
             singleAttestation.getFirstCommitteeIndex());
 
-    int validatorIndex = singleAttestation.getValidatorIndexRequired().intValue();
-
-    int validatorCommitteeBit = committee.indexOf(validatorIndex);
+    final int validatorIndex = singleAttestation.getValidatorIndexRequired().intValue();
+    final int validatorCommitteeBit = committee.indexOf(validatorIndex);
 
     checkArgument(
         validatorCommitteeBit >= 0,

ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/util/AttestationUtilTest.java

@@ -37,7 +37,6 @@
 import org.apache.tuweni.bytes.Bytes32;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.TestTemplate;
-import tech.pegasys.teku.bls.BLSPublicKey;
 import tech.pegasys.teku.bls.BLSSignature;
 import tech.pegasys.teku.infrastructure.async.SafeFuture;
 import tech.pegasys.teku.infrastructure.ssz.Merkleizable;
@@ -179,10 +178,12 @@ void createsValidatesIndexedAttestationAndConcertsFromSingleAttestation(
     final ValidatableAttestation validatableAttestation =
         ValidatableAttestation.fromNetwork(spec, attestation, 1);
 
-    // single pubkey signature verification
-    when(asyncBLSSignatureVerifier.verify(
-            any(BLSPublicKey.class), any(Bytes.class), any(BLSSignature.class)))
-        .thenReturn(SafeFuture.completedFuture(true));
+    // we want to make sure that we do signature verification before we do the committee lookup,
+    // that may trigger shuffling calculation
+    // To do that, let's control signature verification result
+    final SafeFuture<Boolean> signatureVerificationResult = new SafeFuture<>();
+    when(asyncBLSSignatureVerifier.verify(anyList(), any(Bytes.class), any(BLSSignature.class)))
+        .thenReturn(signatureVerificationResult);
 
     // let validator be the second in the committee
     doAnswer(invocation -> IntList.of(validatorIndex.intValue() + 1, validatorIndex.intValue()))
@@ -192,6 +193,19 @@ void createsValidatesIndexedAttestationAndConcertsFromSingleAttestation(
     final SafeFuture<AttestationProcessingResult> result =
         executeValidation(validatableAttestation);
 
+    // no beacon committee lookup before signature verification
+    verify(beaconStateAccessors, never()).getBeaconCommittee(any(), any(), any());
+
+    // validation still in progress
+    assertThat(result).isNotDone();
+
+    // signature verification completed
+    signatureVerificationResult.complete(true);
+
+    // now we should have the beacon committee lookup
+    verify(beaconStateAccessors).getBeaconCommittee(any(), any(), any());
+
+    // now we have successful validation
     assertThat(result).isCompletedWithValue(AttestationProcessingResult.SUCCESSFUL);
 
     assertThat(validatableAttestation.getUnconvertedAttestation().isSingleAttestation())
@@ -215,9 +229,6 @@ void createsValidatesIndexedAttestationAndConcertsFromSingleAttestation(
     } else {
       assertThat(validatableAttestation.getCommitteesSize()).isEmpty();
     }
-
-    verify(asyncBLSSignatureVerifier)
-        .verify(any(BLSPublicKey.class), any(Bytes.class), any(BLSSignature.class));
   }
 
   @TestTemplate

networking/eth2/src/test/java/tech/pegasys/teku/networking/eth2/gossip/subnets/AttestationSubnetSubscriptionsTest.java

@@ -14,6 +14,7 @@
 package tech.pegasys.teku.networking.eth2.gossip.subnets;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.argThat;
 import static org.mockito.ArgumentMatchers.contains;
@@ -163,7 +164,8 @@ void shouldCreateHandlerForSingleAttestationWhenMilestoneIsElectra() {
             processor,
             storageSystem.recentChainData().getCurrentForkInfo().orElseThrow(),
             DebugDataDumper.NOOP);
-    subnetSubscriptions.getAttestationSchema().toSingleAttestationSchemaRequired();
+    assertDoesNotThrow(
+        () -> subnetSubscriptions.getAttestationSchema().toSingleAttestationSchemaRequired());
   }
 
   private int computeSubnetId(final Attestation attestation) {