Make use of abstract unit tests for BLST implementation (#4242)

* Make use of abstract unit tests for BLST implementation * Return back aggregateVerifyDuplicateMessages() test and prohibit duplicate messages in aggregateVerify() for BLST impl * Remove duplicate message checks and corresponding test as we are under ProofOfPossession BLS scheme Co-authored-by: Ben Edgington <[email protected]>
Consensys · Sep 1, 2021 · d7ab463 · d7ab463
1 parent e0f42a7
commit d7ab463
Show file tree

Hide file tree

Showing 10 changed files with 38 additions and 64 deletions.
diff --git a/bls/src/main/java/tech/pegasys/teku/bls/BLS.java b/bls/src/main/java/tech/pegasys/teku/bls/BLS.java
@@ -18,7 +18,6 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Streams;
-import java.util.HashSet;
 import java.util.List;
 import java.util.stream.Collectors;
 import java.util.stream.IntStream;
@@ -146,8 +145,7 @@ public static boolean aggregateVerify(
         publicKeys.size() == messages.size(),
         "Number of public keys and number of messages differs.");
     if (publicKeys.isEmpty()) return false;
-    // Check that there are no duplicate messages
-    if (new HashSet<>(messages).size() != messages.size()) return false;
+
     List<PublicKeyMessagePair> publicKeyMessagePairs =
         Streams.zip(
                 publicKeys.stream(),

diff --git a/bls/src/main/java/tech/pegasys/teku/bls/impl/blst/BlstBLS12381.java b/bls/src/main/java/tech/pegasys/teku/bls/impl/blst/BlstBLS12381.java
@@ -156,6 +156,10 @@ public BatchSemiAggregate prepareBatchVerify2(
   @Override
   public boolean completeBatchVerify(List<? extends BatchSemiAggregate> preparedList) {
     try {
+      if (preparedList.isEmpty()) {
+        return true;
+      }
+
       List<BlstSemiAggregate> blstList =
           preparedList.stream().map(b -> (BlstSemiAggregate) b).collect(Collectors.toList());
 

diff --git a/bls/src/test/java/tech/pegasys/teku/bls/BLSTest.java b/bls/src/test/java/tech/pegasys/teku/bls/BLSTest.java
@@ -126,27 +126,6 @@ void fastAggregateVerify_verify4Signers() {
     assertTrue(BLS.fastAggregateVerify(publicKeys, message, aggregatedSignature));
   }
 
-  @Test
-  void succeedsWhenAggregateVerifyWithRepeatedMessagesReturnsFalse() {
-    Bytes message1 = Bytes.wrap("Hello, world 1!".getBytes(UTF_8));
-    Bytes message2 = Bytes.wrap("Hello, world 2!".getBytes(UTF_8));
-    BLSKeyPair keyPair1 = BLSTestUtil.randomKeyPair(1);
-    BLSKeyPair keyPair2 = BLSTestUtil.randomKeyPair(2);
-    BLSKeyPair keyPair3 = BLSTestUtil.randomKeyPair(3);
-
-    List<BLSPublicKey> publicKeys =
-        Arrays.asList(keyPair1.getPublicKey(), keyPair2.getPublicKey(), keyPair3.getPublicKey());
-    List<Bytes> messages = Arrays.asList(message1, message2, message2);
-    List<BLSSignature> signatures =
-        Arrays.asList(
-            BLS.sign(keyPair1.getSecretKey(), message1),
-            BLS.sign(keyPair2.getSecretKey(), message2),
-            BLS.sign(keyPair3.getSecretKey(), message2));
-    BLSSignature aggregatedSignature = BLS.aggregate(signatures);
-
-    assertFalse(BLS.aggregateVerify(publicKeys, messages, aggregatedSignature));
-  }
-
   @Test
   void succeedsWhenAggregateVerifyWithDistinctMessagesReturnsTrue() {
     Bytes message1 = Bytes.wrap("Hello, world 1!".getBytes(UTF_8));

diff --git a/...h/pegasys/teku/bls/impl/BLS12381Test.java → ...s/teku/bls/impl/AbstractBLS12381Test.java b/...h/pegasys/teku/bls/impl/BLS12381Test.java → ...s/teku/bls/impl/AbstractBLS12381Test.java
@@ -29,7 +29,7 @@
 import org.junit.jupiter.api.Test;
 import tech.pegasys.teku.bls.BatchSemiAggregate;
 
-public abstract class BLS12381Test {
+public abstract class AbstractBLS12381Test {
 
   private final List<KeyPair> keys =
       IntStream.range(0, 8).mapToObj(getBls()::generateKeyPair).collect(Collectors.toList());
@@ -57,7 +57,7 @@ public abstract class BLS12381Test {
   private final int aggrIndex;
   private final int invalidIndex;
 
-  protected BLS12381Test() {
+  protected AbstractBLS12381Test() {
     aggrIndex = pubKeys.size();
     pubKeys.add(aggrSigPubkeys);
     messages.add(aggrSigMsg);
@@ -116,27 +116,6 @@ void fastAggregateVerify() {
     assertTrue(aggregatedSignature.verify(publicKeys, message));
   }
 
-  @Test
-  void aggregateVerifyDuplicateMessages() {
-    Bytes message1 = Bytes.wrap("Hello, world 1!".getBytes(UTF_8));
-    Bytes message2 = Bytes.wrap("Hello, world 2!".getBytes(UTF_8));
-    KeyPair keyPair1 = getBls().generateKeyPair(1);
-    KeyPair keyPair2 = getBls().generateKeyPair(2);
-    KeyPair keyPair3 = getBls().generateKeyPair(3);
-
-    List<PublicKey> publicKeys =
-        Arrays.asList(keyPair1.getPublicKey(), keyPair2.getPublicKey(), keyPair3.getPublicKey());
-    List<Bytes> messages = Arrays.asList(message1, message2, message2);
-    List<Signature> signatures =
-        Arrays.asList(
-            keyPair1.getSecretKey().sign(message1),
-            keyPair2.getSecretKey().sign(message2),
-            keyPair3.getSecretKey().sign(message2));
-    Signature aggregatedSignature = getBls().aggregateSignatures(signatures);
-
-    assertFalse(aggregatedSignature.verify(PublicKeyMessagePair.fromLists(publicKeys, messages)));
-  }
-
   @Test
   void batchVerifyPositiveSimpleTest() {
     // positive simple case

diff --git a/.../pegasys/teku/bls/impl/PublicKeyTest.java → .../teku/bls/impl/AbstractPublicKeyTest.java b/.../pegasys/teku/bls/impl/PublicKeyTest.java → .../teku/bls/impl/AbstractPublicKeyTest.java
@@ -13,22 +13,15 @@
 
 package tech.pegasys.teku.bls.impl;
 
-import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
-import java.util.Collections;
 import org.apache.tuweni.bytes.Bytes48;
 import org.junit.jupiter.api.Test;
 
-public abstract class PublicKeyTest {
+public abstract class AbstractPublicKeyTest {
 
   protected abstract BLS12381 getBls();
 
-  @Test
-  void succeedsWhenPassingEmptyListToAggregatePublicKeysDoesNotThrowException() {
-    assertDoesNotThrow(() -> getBls().aggregatePublicKeys(Collections.emptyList()));
-  }
-
   @Test
   public void shouldHaveConsistentHashCodeAndEquals() {
     final PublicKey key =

diff --git a/.../pegasys/teku/bls/impl/SignatureTest.java → .../teku/bls/impl/AbstractSignatureTest.java b/.../pegasys/teku/bls/impl/SignatureTest.java → .../teku/bls/impl/AbstractSignatureTest.java
@@ -20,7 +20,7 @@
 import java.util.Collections;
 import org.junit.jupiter.api.Test;
 
-public abstract class SignatureTest {
+public abstract class AbstractSignatureTest {
 
   protected abstract BLS12381 getBls();
 

diff --git a/bls/src/test/java/tech/pegasys/teku/bls/impl/blst/BlstPublicKeyTest.java b/bls/src/test/java/tech/pegasys/teku/bls/impl/blst/BlstPublicKeyTest.java
@@ -13,18 +13,26 @@
 
 package tech.pegasys.teku.bls.impl.blst;
 
-import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import org.apache.tuweni.bytes.Bytes48;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
+import tech.pegasys.teku.bls.impl.AbstractPublicKeyTest;
+import tech.pegasys.teku.bls.impl.BLS12381;
 
-public class BlstPublicKeyTest {
+public class BlstPublicKeyTest extends AbstractPublicKeyTest {
+
+  private static BLS12381 BLS;
 
   @BeforeAll
   static void setup() {
-    assertThat(BlstLoader.INSTANCE).isNotEmpty();
+    BLS = BlstLoader.INSTANCE.orElseThrow();
+  }
+
+  @Override
+  protected BLS12381 getBls() {
+    return BLS;
   }
 
   // The infinite public key is now considered to be invalid

diff --git a/bls/src/test/java/tech/pegasys/teku/bls/impl/blst/BlstSignatureTest.java b/bls/src/test/java/tech/pegasys/teku/bls/impl/blst/BlstSignatureTest.java
@@ -19,12 +19,20 @@
 import org.apache.tuweni.bytes.Bytes;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
+import tech.pegasys.teku.bls.impl.AbstractSignatureTest;
+import tech.pegasys.teku.bls.impl.BLS12381;
 
-public class BlstSignatureTest {
+public class BlstSignatureTest extends AbstractSignatureTest {
+  private static BLS12381 BLS;
 
   @BeforeAll
   static void setup() {
-    assertThat(BlstLoader.INSTANCE).isNotEmpty();
+    BLS = BlstLoader.INSTANCE.orElseThrow();
+  }
+
+  @Override
+  protected BLS12381 getBls() {
+    return BLS;
   }
 
   private static final Bytes INFINITY_BYTES =

diff --git a/bls/src/test/java/tech/pegasys/teku/bls/impl/blst/BlstTest.java b/bls/src/test/java/tech/pegasys/teku/bls/impl/blst/BlstTest.java
@@ -22,9 +22,10 @@
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 import tech.pegasys.teku.bls.BatchSemiAggregate;
+import tech.pegasys.teku.bls.impl.AbstractBLS12381Test;
 import tech.pegasys.teku.bls.impl.BLS12381;
 
-public class BlstTest {
+public class BlstTest extends AbstractBLS12381Test {
   private static final Random random = new Random(1);
 
   private static BLS12381 BLS;
@@ -34,6 +35,11 @@ static void setup() {
     BLS = BlstLoader.INSTANCE.orElseThrow();
   }
 
+  @Override
+  protected BLS12381 getBls() {
+    return BLS;
+  }
+
   @Test
   void testBatchVerifySingleSig() {
     Bytes msg = Bytes32.ZERO;

diff --git a/...sys/teku/spec/logic/common/statetransition/blockvalidator/BatchSignatureVerifierTest.java b/...sys/teku/spec/logic/common/statetransition/blockvalidator/BatchSignatureVerifierTest.java
@@ -42,8 +42,7 @@ public void shouldRaiseExceptionIfNoValidPublicKeys() {
         Bytes.wrap("Hello, world!".getBytes(UTF_8)),
         BLSTestUtil.randomSignature(43));
 
-    // a case that should probably be handled differently, but equally doesnt seem to occur
-    assertThatThrownBy(verifier::batchVerify).isInstanceOf(IndexOutOfBoundsException.class);
+    assertThat(verifier.batchVerify()).isFalse();
   }
 
   @Test