Skip to content

Commit 03e5a34

Browse files
ajsuttonajsutton
authored
Revert "Make it possible to disable BLS verification for everything except Deposits (#6116)" (#6148)
This reverts commit 35eb475.
1 parent f25cf69 commit 03e5a34

File tree

21 files changed

+101
-87
lines changed

21 files changed

+101
-87
lines changed

‎CHANGELOG.md

+2-1
Original file line numberDiff line numberDiff line change
@@ -19,4 +19,5 @@ For information on changes in released versions of Teku, see the [releases page]
1919

2020
### Additions and Improvements
2121

22-
### Bug Fixes
22+
### Bug Fixes
23+
- Resolves an issue with public key validation.

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

+2-4
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@
3030
import org.junit.jupiter.api.BeforeAll;
3131
import org.junit.jupiter.api.Test;
3232
import tech.pegasys.teku.bls.BLSSignature;
33-
import tech.pegasys.teku.bls.BLSSignatureVerifier;
3433
import tech.pegasys.teku.infrastructure.async.SafeFuture;
3534
import tech.pegasys.teku.infrastructure.ssz.SszList;
3635
import tech.pegasys.teku.infrastructure.unsigned.UInt64;
@@ -87,13 +86,12 @@ class BlockFactoryTest {
8786

8887
@BeforeAll
8988
public static void initSession() {
90-
AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
89+
AbstractBlockProcessor.blsVerifyDeposit = false;
9190
}
9291

9392
@AfterAll
9493
public static void resetSession() {
95-
AbstractBlockProcessor.depositSignatureVerifier =
96-
AbstractBlockProcessor.DEFAULT_DEPOSIT_SIGNATURE_VERIFIER;
94+
AbstractBlockProcessor.blsVerifyDeposit = true;
9795
}
9896

9997
@Test

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

+1-2
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@
3434
import tech.pegasys.teku.benchmarks.gen.BlsKeyPairIO;
3535
import tech.pegasys.teku.benchmarks.util.CustomRunner;
3636
import tech.pegasys.teku.bls.BLSKeyPair;
37-
import tech.pegasys.teku.bls.BLSSignatureVerifier;
3837
import tech.pegasys.teku.infrastructure.async.eventthread.InlineEventThread;
3938
import tech.pegasys.teku.infrastructure.ssz.collections.SszMutableUInt64List;
4039
import tech.pegasys.teku.infrastructure.unsigned.UInt64;
@@ -88,7 +87,7 @@ public class EpochTransitionBenchmark {
8887

8988
@Setup(Level.Trial)
9089
public void init() throws Exception {
91-
AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
90+
AbstractBlockProcessor.blsVerifyDeposit = false;
9291

9392
spec = TestSpecFactory.createMainnetAltair();
9493
String blocksFile =

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

+2-3
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@
2828
import tech.pegasys.teku.benchmarks.gen.BlsKeyPairIO;
2929
import tech.pegasys.teku.bls.BLSKeyPair;
3030
import tech.pegasys.teku.bls.BLSPublicKey;
31-
import tech.pegasys.teku.bls.BLSSignatureVerifier;
3231
import tech.pegasys.teku.bls.BLSTestUtil;
3332
import tech.pegasys.teku.infrastructure.async.eventthread.InlineEventThread;
3433
import tech.pegasys.teku.infrastructure.unsigned.UInt64;
@@ -63,7 +62,7 @@ public class ProfilingRun {
6362
@Test
6463
public void importBlocks() throws Exception {
6564

66-
AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
65+
AbstractBlockProcessor.blsVerifyDeposit = false;
6766

6867
int validatorsCount = 32 * 1024;
6968
int iterationBlockLimit = 1024;
@@ -152,7 +151,7 @@ public static void main(String[] args) throws Exception {
152151
@Test
153152
public void importBlocksMemProfiling() throws Exception {
154153

155-
AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
154+
AbstractBlockProcessor.blsVerifyDeposit = false;
156155

157156
int validatorsCount = 32 * 1024;
158157

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

+1-2
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@
3232
import tech.pegasys.teku.benchmarks.gen.BlockIO;
3333
import tech.pegasys.teku.benchmarks.gen.BlsKeyPairIO;
3434
import tech.pegasys.teku.bls.BLSKeyPair;
35-
import tech.pegasys.teku.bls.BLSSignatureVerifier;
3635
import tech.pegasys.teku.infrastructure.async.eventthread.InlineEventThread;
3736
import tech.pegasys.teku.infrastructure.unsigned.UInt64;
3837
import tech.pegasys.teku.spec.Spec;
@@ -72,7 +71,7 @@ public abstract class TransitionBenchmark {
7271
@Setup(Level.Trial)
7372
public void init() throws Exception {
7473
spec = TestSpecFactory.createMainnetAltair();
75-
AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
74+
AbstractBlockProcessor.blsVerifyDeposit = false;
7675

7776
String blocksFile =
7877
"/blocks/blocks_epoch_"

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

+1-2
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
import org.junit.jupiter.api.Test;
2626
import tech.pegasys.teku.benchmarks.gen.BlockIO.Writer;
2727
import tech.pegasys.teku.bls.BLSKeyPair;
28-
import tech.pegasys.teku.bls.BLSSignatureVerifier;
2928
import tech.pegasys.teku.bls.BLSTestUtil;
3029
import tech.pegasys.teku.infrastructure.unsigned.UInt64;
3130
import tech.pegasys.teku.spec.Spec;
@@ -50,7 +49,7 @@ public class Generator {
5049
public void generateBlocks() throws Exception {
5150
final Spec spec = TestSpecFactory.createMainnetAltair();
5251

53-
AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
52+
AbstractBlockProcessor.blsVerifyDeposit = false;
5453

5554
System.out.println("Generating keypairs...");
5655
int validatorsCount = 400000;

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

+2-4
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
import org.junit.jupiter.api.BeforeAll;
2020
import org.junit.jupiter.api.BeforeEach;
2121
import org.junit.jupiter.api.Test;
22-
import tech.pegasys.teku.bls.BLSSignatureVerifier;
2322
import tech.pegasys.teku.infrastructure.unsigned.UInt64;
2423
import tech.pegasys.teku.spec.Spec;
2524
import tech.pegasys.teku.spec.TestSpecFactory;
@@ -42,13 +41,12 @@ public class BuilderCircuitBreakerImplTest {
4241

4342
@BeforeAll
4443
public static void disableDepositBlsVerification() {
45-
AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
44+
AbstractBlockProcessor.blsVerifyDeposit = false;
4645
}
4746

4847
@AfterAll
4948
public static void enableDepositBlsVerification() {
50-
AbstractBlockProcessor.depositSignatureVerifier =
51-
AbstractBlockProcessor.DEFAULT_DEPOSIT_SIGNATURE_VERIFIER;
49+
AbstractBlockProcessor.blsVerifyDeposit = true;
5250
}
5351

5452
@BeforeEach

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

+26-20
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
import static java.lang.Math.toIntExact;
1818
import static tech.pegasys.teku.spec.config.SpecConfig.FAR_FUTURE_EPOCH;
1919

20-
import com.google.common.annotations.VisibleForTesting;
2120
import it.unimi.dsi.fastutil.ints.IntList;
2221
import it.unimi.dsi.fastutil.objects.Object2IntMap;
2322
import java.util.ArrayList;
@@ -29,9 +28,11 @@
2928
import org.apache.logging.log4j.Logger;
3029
import org.apache.tuweni.bytes.Bytes;
3130
import org.apache.tuweni.bytes.Bytes32;
31+
import tech.pegasys.teku.bls.BLS;
3232
import tech.pegasys.teku.bls.BLSPublicKey;
3333
import tech.pegasys.teku.bls.BLSSignature;
3434
import tech.pegasys.teku.bls.BLSSignatureVerifier;
35+
import tech.pegasys.teku.bls.impl.BlsException;
3536
import tech.pegasys.teku.infrastructure.bytes.Bytes4;
3637
import tech.pegasys.teku.infrastructure.crypto.Hash;
3738
import tech.pegasys.teku.infrastructure.ssz.SszList;
@@ -77,16 +78,11 @@
7778
import tech.pegasys.teku.spec.logic.versions.bellatrix.block.OptimisticExecutionPayloadExecutor;
7879

7980
public abstract class AbstractBlockProcessor implements BlockProcessor {
80-
81-
@VisibleForTesting
82-
public static final BLSSignatureVerifier DEFAULT_DEPOSIT_SIGNATURE_VERIFIER =
83-
BLSSignatureVerifier.SIMPLE;
8481
/**
8582
* For debug/test purposes only enables/disables {@link DepositData} BLS signature verification
8683
* Setting to <code>false</code> significantly speeds up state initialization
8784
*/
88-
@VisibleForTesting
89-
public static BLSSignatureVerifier depositSignatureVerifier = DEFAULT_DEPOSIT_SIGNATURE_VERIFIER;
85+
public static boolean blsVerifyDeposit = true;
9086

9187
private static final Logger LOG = LogManager.getLogger();
9288

@@ -610,25 +606,30 @@ public void processDeposits(MutableBeaconState state, SszList<? extends Deposit>
610606
throws BlockProcessingException {
611607
safelyProcess(
612608
() -> {
613-
final boolean depositSignaturesAreAllGood = batchVerifyDepositSignatures(deposits);
609+
final boolean depositSignaturesAreAllGood =
610+
!blsVerifyDeposit || batchVerifyDepositSignatures(deposits);
614611
for (Deposit deposit : deposits) {
615612
processDeposit(state, deposit, depositSignaturesAreAllGood);
616613
}
617614
});
618615
}
619616

620617
private boolean batchVerifyDepositSignatures(SszList<? extends Deposit> deposits) {
621-
final List<List<BLSPublicKey>> publicKeys = new ArrayList<>();
622-
final List<Bytes> messages = new ArrayList<>();
623-
final List<BLSSignature> signatures = new ArrayList<>();
624-
for (Deposit deposit : deposits) {
625-
final BLSPublicKey pubkey = deposit.getData().getPubkey();
626-
publicKeys.add(List.of(pubkey));
627-
messages.add(computeDepositSigningRoot(deposit, pubkey));
628-
signatures.add(deposit.getData().getSignature());
618+
try {
619+
final List<List<BLSPublicKey>> publicKeys = new ArrayList<>();
620+
final List<Bytes> messages = new ArrayList<>();
621+
final List<BLSSignature> signatures = new ArrayList<>();
622+
for (Deposit deposit : deposits) {
623+
final BLSPublicKey pubkey = deposit.getData().getPubkey();
624+
publicKeys.add(List.of(pubkey));
625+
messages.add(computeDepositSigningRoot(deposit, pubkey));
626+
signatures.add(deposit.getData().getSignature());
627+
}
628+
// Overwhelmingly often we expect all the deposit signatures to be good
629+
return BLS.batchVerify(publicKeys, messages, signatures);
630+
} catch (final BlsException e) {
631+
return false;
629632
}
630-
// Overwhelmingly often we expect all the deposit signatures to be good
631-
return depositSignatureVerifier.verify(publicKeys, messages, signatures);
632633
}
633634

634635
public void processDeposit(
@@ -704,8 +705,13 @@ private void handleInvalidDeposit(
704705
}
705706

706707
private boolean depositSignatureIsValid(final Deposit deposit, BLSPublicKey pubkey) {
707-
return depositSignatureVerifier.verify(
708-
pubkey, computeDepositSigningRoot(deposit, pubkey), deposit.getData().getSignature());
708+
try {
709+
return !blsVerifyDeposit
710+
|| BLS.verify(
711+
pubkey, computeDepositSigningRoot(deposit, pubkey), deposit.getData().getSignature());
712+
} catch (final BlsException e) {
713+
return false;
714+
}
709715
}
710716

711717
private Bytes computeDepositSigningRoot(final Deposit deposit, BLSPublicKey pubkey) {

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

+40-3
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,8 @@ public abstract class BlockProcessorTest {
5757
protected abstract Spec createSpec();
5858

5959
@Test
60-
void ensureDepositSignatureVerifierHasDefaultValue() {
61-
assertThat(AbstractBlockProcessor.depositSignatureVerifier)
62-
.isSameAs(AbstractBlockProcessor.DEFAULT_DEPOSIT_SIGNATURE_VERIFIER);
60+
void ensureVerifyDepositDefaultsToTrue() {
61+
assertThat(AbstractBlockProcessor.blsVerifyDeposit).isTrue();
6362
}
6463

6564
@Test
@@ -161,6 +160,44 @@ void processDepositIgnoresDepositWithInvalidPublicKey() throws BlockProcessingEx
161160
"The balances list has changed.");
162161
}
163162

163+
@Test
164+
void processDepositIgnoresDepositWithZeroPublicKey() throws BlockProcessingException {
165+
// The following deposit uses a "rogue" public key that is not in the G1 group
166+
BLSPublicKey pubkey =
167+
BLSPublicKey.fromBytesCompressed(
168+
Bytes48.fromHexString(
169+
"0x000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"));
170+
Bytes32 withdrawalCredentials =
171+
Bytes32.fromHexString("0x79e43d39ee55749c55994a7ab2a3cb91460cec544fdbf27eb5717c43f970c1b6");
172+
UInt64 amount = UInt64.valueOf(1000000000L);
173+
BLSSignature signature =
174+
BLSSignature.fromBytesCompressed(
175+
Bytes.fromHexString(
176+
"0xddc1ca509e29c6452441069f26da6e073589b3bd1cace50e3427426af5bfdd566d077d4bdf618e249061b9770471e3d515779aa758b8ccb4b06226a8d5ebc99e19d4c3278e5006b837985bec4e0ce39df92c1f88d1afd0f98dbae360024a390d"));
177+
DepositData depositInput =
178+
new DepositData(new DepositMessage(pubkey, withdrawalCredentials, amount), signature);
179+
180+
BeaconState preState = createBeaconState();
181+
182+
int originalValidatorRegistrySize = preState.getValidators().size();
183+
int originalValidatorBalancesSize = preState.getBalances().size();
184+
185+
BeaconState postState = processDepositHelper(preState, depositInput);
186+
187+
assertEquals(
188+
postState.getValidators().size(),
189+
originalValidatorRegistrySize,
190+
"The validator was added to the validator registry.");
191+
assertEquals(
192+
postState.getBalances().size(),
193+
originalValidatorBalancesSize,
194+
"The balance was added to the validator balances.");
195+
assertEquals(
196+
preState.getBalances().hashTreeRoot(),
197+
postState.getBalances().hashTreeRoot(),
198+
"The balances list has changed.");
199+
}
200+
164201
protected BeaconState createBeaconState() {
165202
return createBeaconState(false, null, null);
166203
}

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

+2-4
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@
3333
import org.junit.jupiter.api.Test;
3434
import tech.pegasys.teku.bls.BLSKeyGenerator;
3535
import tech.pegasys.teku.bls.BLSKeyPair;
36-
import tech.pegasys.teku.bls.BLSSignatureVerifier;
3736
import tech.pegasys.teku.bls.BLSTestUtil;
3837
import tech.pegasys.teku.infrastructure.async.SafeFuture;
3938
import tech.pegasys.teku.infrastructure.async.eventthread.InlineEventThread;
@@ -108,13 +107,12 @@ public class BlockImporterTest {
108107

109108
@BeforeAll
110109
public static void init() {
111-
AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
110+
AbstractBlockProcessor.blsVerifyDeposit = false;
112111
}
113112

114113
@AfterAll
115114
public static void dispose() {
116-
AbstractBlockProcessor.depositSignatureVerifier =
117-
AbstractBlockProcessor.DEFAULT_DEPOSIT_SIGNATURE_VERIFIER;
115+
AbstractBlockProcessor.blsVerifyDeposit = true;
118116
}
119117

120118
@BeforeEach

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

+2-4
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@
4242
import org.junit.jupiter.api.BeforeAll;
4343
import org.junit.jupiter.api.BeforeEach;
4444
import org.junit.jupiter.api.Test;
45-
import tech.pegasys.teku.bls.BLSSignatureVerifier;
4645
import tech.pegasys.teku.infrastructure.async.SafeFuture;
4746
import tech.pegasys.teku.infrastructure.async.eventthread.InlineEventThread;
4847
import tech.pegasys.teku.infrastructure.logging.EventLogger;
@@ -135,13 +134,12 @@ public class BlockManagerTest {
135134

136135
@BeforeAll
137136
public static void initSession() {
138-
AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
137+
AbstractBlockProcessor.blsVerifyDeposit = false;
139138
}
140139

141140
@AfterAll
142141
public static void resetSession() {
143-
AbstractBlockProcessor.depositSignatureVerifier =
144-
AbstractBlockProcessor.DEFAULT_DEPOSIT_SIGNATURE_VERIFIER;
142+
AbstractBlockProcessor.blsVerifyDeposit = true;
145143
}
146144

147145
@BeforeEach

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

+2-4
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737
import org.junit.jupiter.api.BeforeEach;
3838
import org.junit.jupiter.api.Test;
3939
import org.mockito.invocation.InvocationOnMock;
40-
import tech.pegasys.teku.bls.BLSSignatureVerifier;
4140
import tech.pegasys.teku.infrastructure.async.SafeFuture;
4241
import tech.pegasys.teku.infrastructure.async.SafeFutureAssert;
4342
import tech.pegasys.teku.infrastructure.async.eventthread.InlineEventThread;
@@ -92,13 +91,12 @@ class ForkChoiceNotifierTest {
9291

9392
@BeforeAll
9493
public static void initSession() {
95-
AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
94+
AbstractBlockProcessor.blsVerifyDeposit = false;
9695
}
9796

9897
@AfterAll
9998
public static void resetSession() {
100-
AbstractBlockProcessor.depositSignatureVerifier =
101-
AbstractBlockProcessor.DEFAULT_DEPOSIT_SIGNATURE_VERIFIER;
99+
AbstractBlockProcessor.blsVerifyDeposit = true;
102100
}
103101

104102
@BeforeEach

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

+2-4
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
import org.junit.jupiter.api.BeforeAll;
2727
import org.junit.jupiter.api.BeforeEach;
2828
import org.junit.jupiter.api.Test;
29-
import tech.pegasys.teku.bls.BLSSignatureVerifier;
3029
import tech.pegasys.teku.infrastructure.async.SafeFuture;
3130
import tech.pegasys.teku.spec.Spec;
3231
import tech.pegasys.teku.spec.TestSpecFactory;
@@ -60,13 +59,12 @@ class ForkChoicePayloadExecutorTest {
6059

6160
@BeforeAll
6261
public static void initSession() {
63-
AbstractBlockProcessor.depositSignatureVerifier = BLSSignatureVerifier.NO_OP;
62+
AbstractBlockProcessor.blsVerifyDeposit = false;
6463
}
6564

6665
@AfterAll
6766
public static void resetSession() {
68-
AbstractBlockProcessor.depositSignatureVerifier =
69-
AbstractBlockProcessor.DEFAULT_DEPOSIT_SIGNATURE_VERIFIER;
67+
AbstractBlockProcessor.blsVerifyDeposit = true;
7068
}
7169

7270
@BeforeEach

0 commit comments

Comments
 (0)