Revert "Make it possible to disable BLS verification for everything e…

…xcept Deposits (#6116)" (#6148)

This reverts commit 35eb475.

CHANGELOG.md

@@ -19,4 +19,5 @@ For information on changes in released versions of Teku, see the [releases page]
 
 ### Additions and Improvements
 
-### Bug Fixes
+### Bug Fixes
+- Resolves an issue with public key validation.

beacon/validator/src/test/java/tech/pegasys/teku/validator/coordinator/BlockFactoryTest.java

@@ -30,7 +30,6 @@
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 import tech.pegasys.teku.bls.BLSSignature;
-import tech.pegasys.teku.bls.BLSSignatureVerifier;
 import tech.pegasys.teku.infrastructure.async.SafeFuture;
 import tech.pegasys.teku.infrastructure.ssz.SszList;
 import tech.pegasys.teku.infrastructure.unsigned.UInt64;
@@ -87,13 +86,12 @@ class BlockFactoryTest {
 
   @BeforeAll
   public static void initSession() {
-    AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
+    AbstractBlockProcessor.blsVerifyDeposit = false;
   }
 
   @AfterAll
   public static void resetSession() {
-    AbstractBlockProcessor.depositSignatureVerifier =
-        AbstractBlockProcessor.DEFAULT_DEPOSIT_SIGNATURE_VERIFIER;
+    AbstractBlockProcessor.blsVerifyDeposit = true;
   }
 
   @Test

eth-benchmark-tests/src/jmh/java/tech/pegasys/teku/benchmarks/EpochTransitionBenchmark.java

@@ -34,7 +34,6 @@
 import tech.pegasys.teku.benchmarks.gen.BlsKeyPairIO;
 import tech.pegasys.teku.benchmarks.util.CustomRunner;
 import tech.pegasys.teku.bls.BLSKeyPair;
-import tech.pegasys.teku.bls.BLSSignatureVerifier;
 import tech.pegasys.teku.infrastructure.async.eventthread.InlineEventThread;
 import tech.pegasys.teku.infrastructure.ssz.collections.SszMutableUInt64List;
 import tech.pegasys.teku.infrastructure.unsigned.UInt64;
@@ -88,7 +87,7 @@ public class EpochTransitionBenchmark {
 
   @Setup(Level.Trial)
   public void init() throws Exception {
-    AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
+    AbstractBlockProcessor.blsVerifyDeposit = false;
 
     spec = TestSpecFactory.createMainnetAltair();
     String blocksFile =

eth-benchmark-tests/src/jmh/java/tech/pegasys/teku/benchmarks/ProfilingRun.java

@@ -28,7 +28,6 @@
 import tech.pegasys.teku.benchmarks.gen.BlsKeyPairIO;
 import tech.pegasys.teku.bls.BLSKeyPair;
 import tech.pegasys.teku.bls.BLSPublicKey;
-import tech.pegasys.teku.bls.BLSSignatureVerifier;
 import tech.pegasys.teku.bls.BLSTestUtil;
 import tech.pegasys.teku.infrastructure.async.eventthread.InlineEventThread;
 import tech.pegasys.teku.infrastructure.unsigned.UInt64;
@@ -63,7 +62,7 @@ public class ProfilingRun {
   @Test
   public void importBlocks() throws Exception {
 
-    AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
+    AbstractBlockProcessor.blsVerifyDeposit = false;
 
     int validatorsCount = 32 * 1024;
     int iterationBlockLimit = 1024;
@@ -152,7 +151,7 @@ public static void main(String[] args) throws Exception {
   @Test
   public void importBlocksMemProfiling() throws Exception {
 
-    AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
+    AbstractBlockProcessor.blsVerifyDeposit = false;
 
     int validatorsCount = 32 * 1024;
 

eth-benchmark-tests/src/jmh/java/tech/pegasys/teku/benchmarks/TransitionBenchmark.java

@@ -32,7 +32,6 @@
 import tech.pegasys.teku.benchmarks.gen.BlockIO;
 import tech.pegasys.teku.benchmarks.gen.BlsKeyPairIO;
 import tech.pegasys.teku.bls.BLSKeyPair;
-import tech.pegasys.teku.bls.BLSSignatureVerifier;
 import tech.pegasys.teku.infrastructure.async.eventthread.InlineEventThread;
 import tech.pegasys.teku.infrastructure.unsigned.UInt64;
 import tech.pegasys.teku.spec.Spec;
@@ -72,7 +71,7 @@ public abstract class TransitionBenchmark {
   @Setup(Level.Trial)
   public void init() throws Exception {
     spec = TestSpecFactory.createMainnetAltair();
-    AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
+    AbstractBlockProcessor.blsVerifyDeposit = false;
 
     String blocksFile =
         "/blocks/blocks_epoch_"

eth-benchmark-tests/src/jmh/java/tech/pegasys/teku/benchmarks/gen/Generator.java

@@ -25,7 +25,6 @@
 import org.junit.jupiter.api.Test;
 import tech.pegasys.teku.benchmarks.gen.BlockIO.Writer;
 import tech.pegasys.teku.bls.BLSKeyPair;
-import tech.pegasys.teku.bls.BLSSignatureVerifier;
 import tech.pegasys.teku.bls.BLSTestUtil;
 import tech.pegasys.teku.infrastructure.unsigned.UInt64;
 import tech.pegasys.teku.spec.Spec;
@@ -50,7 +49,7 @@ public class Generator {
   public void generateBlocks() throws Exception {
     final Spec spec = TestSpecFactory.createMainnetAltair();
 
-    AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
+    AbstractBlockProcessor.blsVerifyDeposit = false;
 
     System.out.println("Generating keypairs...");
     int validatorsCount = 400000;

ethereum/executionlayer/src/test/java/tech/pegasys/teku/ethereum/executionlayer/BuilderCircuitBreakerImplTest.java

@@ -19,7 +19,6 @@
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
-import tech.pegasys.teku.bls.BLSSignatureVerifier;
 import tech.pegasys.teku.infrastructure.unsigned.UInt64;
 import tech.pegasys.teku.spec.Spec;
 import tech.pegasys.teku.spec.TestSpecFactory;
@@ -42,13 +41,12 @@ public class BuilderCircuitBreakerImplTest {
 
   @BeforeAll
   public static void disableDepositBlsVerification() {
-    AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
+    AbstractBlockProcessor.blsVerifyDeposit = false;
   }
 
   @AfterAll
   public static void enableDepositBlsVerification() {
-    AbstractBlockProcessor.depositSignatureVerifier =
-        AbstractBlockProcessor.DEFAULT_DEPOSIT_SIGNATURE_VERIFIER;
+    AbstractBlockProcessor.blsVerifyDeposit = true;
   }
 
   @BeforeEach

ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/block/AbstractBlockProcessor.java

@@ -17,7 +17,6 @@
 import static java.lang.Math.toIntExact;
 import static tech.pegasys.teku.spec.config.SpecConfig.FAR_FUTURE_EPOCH;
 
-import com.google.common.annotations.VisibleForTesting;
 import it.unimi.dsi.fastutil.ints.IntList;
 import it.unimi.dsi.fastutil.objects.Object2IntMap;
 import java.util.ArrayList;
@@ -29,9 +28,11 @@
 import org.apache.logging.log4j.Logger;
 import org.apache.tuweni.bytes.Bytes;
 import org.apache.tuweni.bytes.Bytes32;
+import tech.pegasys.teku.bls.BLS;
 import tech.pegasys.teku.bls.BLSPublicKey;
 import tech.pegasys.teku.bls.BLSSignature;
 import tech.pegasys.teku.bls.BLSSignatureVerifier;
+import tech.pegasys.teku.bls.impl.BlsException;
 import tech.pegasys.teku.infrastructure.bytes.Bytes4;
 import tech.pegasys.teku.infrastructure.crypto.Hash;
 import tech.pegasys.teku.infrastructure.ssz.SszList;
@@ -77,16 +78,11 @@
 import tech.pegasys.teku.spec.logic.versions.bellatrix.block.OptimisticExecutionPayloadExecutor;
 
 public abstract class AbstractBlockProcessor implements BlockProcessor {
-
-  @VisibleForTesting
-  public static final BLSSignatureVerifier DEFAULT_DEPOSIT_SIGNATURE_VERIFIER =
-      BLSSignatureVerifier.SIMPLE;
   /**
    * For debug/test purposes only enables/disables {@link DepositData} BLS signature verification
    * Setting to <code>false</code> significantly speeds up state initialization
    */
-  @VisibleForTesting
-  public static BLSSignatureVerifier depositSignatureVerifier = DEFAULT_DEPOSIT_SIGNATURE_VERIFIER;
+  public static boolean blsVerifyDeposit = true;
 
   private static final Logger LOG = LogManager.getLogger();
 
@@ -610,25 +606,30 @@ public void processDeposits(MutableBeaconState state, SszList<? extends Deposit>
       throws BlockProcessingException {
     safelyProcess(
         () -> {
-          final boolean depositSignaturesAreAllGood = batchVerifyDepositSignatures(deposits);
+          final boolean depositSignaturesAreAllGood =
+              !blsVerifyDeposit || batchVerifyDepositSignatures(deposits);
           for (Deposit deposit : deposits) {
             processDeposit(state, deposit, depositSignaturesAreAllGood);
           }
         });
   }
 
   private boolean batchVerifyDepositSignatures(SszList<? extends Deposit> deposits) {
-    final List<List<BLSPublicKey>> publicKeys = new ArrayList<>();
-    final List<Bytes> messages = new ArrayList<>();
-    final List<BLSSignature> signatures = new ArrayList<>();
-    for (Deposit deposit : deposits) {
-      final BLSPublicKey pubkey = deposit.getData().getPubkey();
-      publicKeys.add(List.of(pubkey));
-      messages.add(computeDepositSigningRoot(deposit, pubkey));
-      signatures.add(deposit.getData().getSignature());
+    try {
+      final List<List<BLSPublicKey>> publicKeys = new ArrayList<>();
+      final List<Bytes> messages = new ArrayList<>();
+      final List<BLSSignature> signatures = new ArrayList<>();
+      for (Deposit deposit : deposits) {
+        final BLSPublicKey pubkey = deposit.getData().getPubkey();
+        publicKeys.add(List.of(pubkey));
+        messages.add(computeDepositSigningRoot(deposit, pubkey));
+        signatures.add(deposit.getData().getSignature());
+      }
+      // Overwhelmingly often we expect all the deposit signatures to be good
+      return BLS.batchVerify(publicKeys, messages, signatures);
+    } catch (final BlsException e) {
+      return false;
     }
-    // Overwhelmingly often we expect all the deposit signatures to be good
-    return depositSignatureVerifier.verify(publicKeys, messages, signatures);
   }
 
   public void processDeposit(
@@ -704,8 +705,13 @@ private void handleInvalidDeposit(
   }
 
   private boolean depositSignatureIsValid(final Deposit deposit, BLSPublicKey pubkey) {
-    return depositSignatureVerifier.verify(
-        pubkey, computeDepositSigningRoot(deposit, pubkey), deposit.getData().getSignature());
+    try {
+      return !blsVerifyDeposit
+          || BLS.verify(
+              pubkey, computeDepositSigningRoot(deposit, pubkey), deposit.getData().getSignature());
+    } catch (final BlsException e) {
+      return false;
+    }
   }
 
   private Bytes computeDepositSigningRoot(final Deposit deposit, BLSPublicKey pubkey) {

ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/block/BlockProcessorTest.java

@@ -57,9 +57,8 @@ public abstract class BlockProcessorTest {
   protected abstract Spec createSpec();
 
   @Test
-  void ensureDepositSignatureVerifierHasDefaultValue() {
-    assertThat(AbstractBlockProcessor.depositSignatureVerifier)
-        .isSameAs(AbstractBlockProcessor.DEFAULT_DEPOSIT_SIGNATURE_VERIFIER);
+  void ensureVerifyDepositDefaultsToTrue() {
+    assertThat(AbstractBlockProcessor.blsVerifyDeposit).isTrue();
   }
 
   @Test
@@ -161,6 +160,44 @@ void processDepositIgnoresDepositWithInvalidPublicKey() throws BlockProcessingEx
         "The balances list has changed.");
   }
 
+  @Test
+  void processDepositIgnoresDepositWithZeroPublicKey() throws BlockProcessingException {
+    // The following deposit uses a "rogue" public key that is not in the G1 group
+    BLSPublicKey pubkey =
+        BLSPublicKey.fromBytesCompressed(
+            Bytes48.fromHexString(
+                "0x000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"));
+    Bytes32 withdrawalCredentials =
+        Bytes32.fromHexString("0x79e43d39ee55749c55994a7ab2a3cb91460cec544fdbf27eb5717c43f970c1b6");
+    UInt64 amount = UInt64.valueOf(1000000000L);
+    BLSSignature signature =
+        BLSSignature.fromBytesCompressed(
+            Bytes.fromHexString(
+                "0xddc1ca509e29c6452441069f26da6e073589b3bd1cace50e3427426af5bfdd566d077d4bdf618e249061b9770471e3d515779aa758b8ccb4b06226a8d5ebc99e19d4c3278e5006b837985bec4e0ce39df92c1f88d1afd0f98dbae360024a390d"));
+    DepositData depositInput =
+        new DepositData(new DepositMessage(pubkey, withdrawalCredentials, amount), signature);
+
+    BeaconState preState = createBeaconState();
+
+    int originalValidatorRegistrySize = preState.getValidators().size();
+    int originalValidatorBalancesSize = preState.getBalances().size();
+
+    BeaconState postState = processDepositHelper(preState, depositInput);
+
+    assertEquals(
+        postState.getValidators().size(),
+        originalValidatorRegistrySize,
+        "The validator was added to the validator registry.");
+    assertEquals(
+        postState.getBalances().size(),
+        originalValidatorBalancesSize,
+        "The balance was added to the validator balances.");
+    assertEquals(
+        preState.getBalances().hashTreeRoot(),
+        postState.getBalances().hashTreeRoot(),
+        "The balances list has changed.");
+  }
+
   protected BeaconState createBeaconState() {
     return createBeaconState(false, null, null);
   }

ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/block/BlockImporterTest.java

@@ -33,7 +33,6 @@
 import org.junit.jupiter.api.Test;
 import tech.pegasys.teku.bls.BLSKeyGenerator;
 import tech.pegasys.teku.bls.BLSKeyPair;
-import tech.pegasys.teku.bls.BLSSignatureVerifier;
 import tech.pegasys.teku.bls.BLSTestUtil;
 import tech.pegasys.teku.infrastructure.async.SafeFuture;
 import tech.pegasys.teku.infrastructure.async.eventthread.InlineEventThread;
@@ -108,13 +107,12 @@ public class BlockImporterTest {
 
   @BeforeAll
   public static void init() {
-    AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
+    AbstractBlockProcessor.blsVerifyDeposit = false;
   }
 
   @AfterAll
   public static void dispose() {
-    AbstractBlockProcessor.depositSignatureVerifier =
-        AbstractBlockProcessor.DEFAULT_DEPOSIT_SIGNATURE_VERIFIER;
+    AbstractBlockProcessor.blsVerifyDeposit = true;
   }
 
   @BeforeEach

ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/block/BlockManagerTest.java

@@ -42,7 +42,6 @@
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
-import tech.pegasys.teku.bls.BLSSignatureVerifier;
 import tech.pegasys.teku.infrastructure.async.SafeFuture;
 import tech.pegasys.teku.infrastructure.async.eventthread.InlineEventThread;
 import tech.pegasys.teku.infrastructure.logging.EventLogger;
@@ -135,13 +134,12 @@ public class BlockManagerTest {
 
   @BeforeAll
   public static void initSession() {
-    AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
+    AbstractBlockProcessor.blsVerifyDeposit = false;
   }
 
   @AfterAll
   public static void resetSession() {
-    AbstractBlockProcessor.depositSignatureVerifier =
-        AbstractBlockProcessor.DEFAULT_DEPOSIT_SIGNATURE_VERIFIER;
+    AbstractBlockProcessor.blsVerifyDeposit = true;
   }
 
   @BeforeEach

ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoiceNotifierTest.java

@@ -37,7 +37,6 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.mockito.invocation.InvocationOnMock;
-import tech.pegasys.teku.bls.BLSSignatureVerifier;
 import tech.pegasys.teku.infrastructure.async.SafeFuture;
 import tech.pegasys.teku.infrastructure.async.SafeFutureAssert;
 import tech.pegasys.teku.infrastructure.async.eventthread.InlineEventThread;
@@ -92,13 +91,12 @@ class ForkChoiceNotifierTest {
 
   @BeforeAll
   public static void initSession() {
-    AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
+    AbstractBlockProcessor.blsVerifyDeposit = false;
   }
 
   @AfterAll
   public static void resetSession() {
-    AbstractBlockProcessor.depositSignatureVerifier =
-        AbstractBlockProcessor.DEFAULT_DEPOSIT_SIGNATURE_VERIFIER;
+    AbstractBlockProcessor.blsVerifyDeposit = true;
   }
 
   @BeforeEach

ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoicePayloadExecutorTest.java

@@ -26,7 +26,6 @@
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
-import tech.pegasys.teku.bls.BLSSignatureVerifier;
 import tech.pegasys.teku.infrastructure.async.SafeFuture;
 import tech.pegasys.teku.spec.Spec;
 import tech.pegasys.teku.spec.TestSpecFactory;
@@ -60,13 +59,12 @@ class ForkChoicePayloadExecutorTest {
 
   @BeforeAll
   public static void initSession() {
-    AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
+    AbstractBlockProcessor.blsVerifyDeposit = false;
   }
 
   @AfterAll
   public static void resetSession() {
-    AbstractBlockProcessor.depositSignatureVerifier =
-        AbstractBlockProcessor.DEFAULT_DEPOSIT_SIGNATURE_VERIFIER;
+    AbstractBlockProcessor.blsVerifyDeposit = true;
   }
 
   @BeforeEach