From 66dfac10baad88b3ca0cdaa42e288f1cfc556380 Mon Sep 17 00:00:00 2001 From: Enrico Del Fante Date: Thu, 5 Dec 2024 16:46:56 +0100 Subject: [PATCH] feedback and test improvement --- .../logic/common/util/AttestationUtil.java | 27 ++++++++++---- .../electra/util/AttestationUtilElectra.java | 35 +++++-------------- .../common/util/AttestationUtilTest.java | 27 +++++++++----- .../AttestationSubnetSubscriptionsTest.java | 4 ++- 4 files changed, 52 insertions(+), 41 deletions(-) diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/util/AttestationUtil.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/util/AttestationUtil.java index 5da5b45f023..a46f3006e90 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/util/AttestationUtil.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/util/AttestationUtil.java @@ -252,24 +252,39 @@ public SafeFuture 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 validateAttestationDataSignature( + final Fork fork, + final BeaconState state, + final List 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"); } }); diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/electra/util/AttestationUtilElectra.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/electra/util/AttestationUtilElectra.java index 878d1948fb5..dfa67c18f36 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/electra/util/AttestationUtilElectra.java +++ b/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 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, diff --git a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/util/AttestationUtilTest.java b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/util/AttestationUtilTest.java index c7bd424612c..ef831e9fdf0 100644 --- a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/util/AttestationUtilTest.java +++ b/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 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 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 diff --git a/networking/eth2/src/test/java/tech/pegasys/teku/networking/eth2/gossip/subnets/AttestationSubnetSubscriptionsTest.java b/networking/eth2/src/test/java/tech/pegasys/teku/networking/eth2/gossip/subnets/AttestationSubnetSubscriptionsTest.java index aad37fc0c4d..fae952c80e1 100644 --- a/networking/eth2/src/test/java/tech/pegasys/teku/networking/eth2/gossip/subnets/AttestationSubnetSubscriptionsTest.java +++ b/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) {