From b0bdf299aeca76c51dbaa0b69d22c677b51b9bdf Mon Sep 17 00:00:00 2001 From: Enrico Del Fante Date: Tue, 17 Dec 2024 18:12:17 +0100 Subject: [PATCH] Fix single attestation publication (#8929) --- .../teku/spec/util/DataStructureUtil.java | 6 +- .../AggregatingAttestationPoolTest.java | 4 +- .../eth2/gossip/AttestationGossipManager.java | 2 +- .../gossip/AttestationGossipManagerTest.java | 126 +++++++++++++----- 4 files changed, 99 insertions(+), 39 deletions(-) diff --git a/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java b/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java index 6cea4f93c38..7e312c58cf7 100644 --- a/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java +++ b/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java @@ -821,6 +821,10 @@ public Attestation randomAttestation() { } public SingleAttestation randomSingleAttestation() { + return randomSingleAttestation(randomUInt64()); + } + + public SingleAttestation randomSingleAttestation(final UInt64 slot) { return spec.getGenesisSchemaDefinitions() .toVersionElectra() .orElseThrow() @@ -828,7 +832,7 @@ public SingleAttestation randomSingleAttestation() { .create( randomUInt64(Integer.MAX_VALUE), randomUInt64(Integer.MAX_VALUE), - randomAttestationData(), + randomAttestationData(slot), randomSignature()); } diff --git a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/attestation/AggregatingAttestationPoolTest.java b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/attestation/AggregatingAttestationPoolTest.java index d5f7acae9ad..22d2277ff45 100644 --- a/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/attestation/AggregatingAttestationPoolTest.java +++ b/ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/attestation/AggregatingAttestationPoolTest.java @@ -46,7 +46,7 @@ import tech.pegasys.teku.spec.SpecMilestone; import tech.pegasys.teku.spec.TestSpecContext; import tech.pegasys.teku.spec.TestSpecFactory; -import tech.pegasys.teku.spec.TestSpecInvocationContextProvider; +import tech.pegasys.teku.spec.TestSpecInvocationContextProvider.SpecContext; import tech.pegasys.teku.spec.datastructures.attestation.ValidatableAttestation; import tech.pegasys.teku.spec.datastructures.operations.Attestation; import tech.pegasys.teku.spec.datastructures.operations.AttestationData; @@ -84,7 +84,7 @@ class AggregatingAttestationPoolTest { private Int2IntMap committeeSizes; @BeforeEach - public void setUp(final TestSpecInvocationContextProvider.SpecContext specContext) { + public void setUp(final SpecContext specContext) { spec = specContext.getSpec(); specMilestone = specContext.getSpecMilestone(); dataStructureUtil = specContext.getDataStructureUtil(); diff --git a/networking/eth2/src/main/java/tech/pegasys/teku/networking/eth2/gossip/AttestationGossipManager.java b/networking/eth2/src/main/java/tech/pegasys/teku/networking/eth2/gossip/AttestationGossipManager.java index 3e9dfa6ba60..e2a02b11450 100644 --- a/networking/eth2/src/main/java/tech/pegasys/teku/networking/eth2/gossip/AttestationGossipManager.java +++ b/networking/eth2/src/main/java/tech/pegasys/teku/networking/eth2/gossip/AttestationGossipManager.java @@ -53,7 +53,7 @@ public void onNewAttestation(final ValidatableAttestation validatableAttestation if (validatableAttestation.isAggregate() || !validatableAttestation.markGossiped()) { return; } - final Attestation attestation = validatableAttestation.getAttestation(); + final Attestation attestation = validatableAttestation.getUnconvertedAttestation(); subnetSubscriptions .gossip(attestation) .finish( diff --git a/networking/eth2/src/test/java/tech/pegasys/teku/networking/eth2/gossip/AttestationGossipManagerTest.java b/networking/eth2/src/test/java/tech/pegasys/teku/networking/eth2/gossip/AttestationGossipManagerTest.java index 94e562b650c..0a1f7bff35d 100644 --- a/networking/eth2/src/test/java/tech/pegasys/teku/networking/eth2/gossip/AttestationGossipManagerTest.java +++ b/networking/eth2/src/test/java/tech/pegasys/teku/networking/eth2/gossip/AttestationGossipManagerTest.java @@ -19,10 +19,14 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static tech.pegasys.teku.infrastructure.async.SafeFutureAssert.safeJoin; +import static tech.pegasys.teku.spec.SpecMilestone.ELECTRA; +import static tech.pegasys.teku.spec.SpecMilestone.PHASE0; +import it.unimi.dsi.fastutil.ints.Int2IntMap; +import it.unimi.dsi.fastutil.ints.Int2IntOpenHashMap; import org.apache.tuweni.bytes.Bytes; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestTemplate; import tech.pegasys.teku.infrastructure.async.SafeFuture; import tech.pegasys.teku.infrastructure.async.StubAsyncRunner; import tech.pegasys.teku.infrastructure.metrics.StubCounter; @@ -35,7 +39,9 @@ import tech.pegasys.teku.networking.eth2.gossip.topics.OperationProcessor; import tech.pegasys.teku.networking.p2p.gossip.GossipNetwork; import tech.pegasys.teku.spec.Spec; -import tech.pegasys.teku.spec.TestSpecFactory; +import tech.pegasys.teku.spec.SpecMilestone; +import tech.pegasys.teku.spec.TestSpecContext; +import tech.pegasys.teku.spec.TestSpecInvocationContextProvider.SpecContext; import tech.pegasys.teku.spec.datastructures.attestation.ValidatableAttestation; import tech.pegasys.teku.spec.datastructures.operations.Attestation; import tech.pegasys.teku.spec.datastructures.state.ForkInfo; @@ -45,43 +51,56 @@ import tech.pegasys.teku.storage.client.MemoryOnlyRecentChainData; import tech.pegasys.teku.storage.client.RecentChainData; +@TestSpecContext(milestone = {PHASE0, ELECTRA}) public class AttestationGossipManagerTest { - private final Spec spec = TestSpecFactory.createMinimalPhase0(); - private final DataStructureUtil dataStructureUtil = new DataStructureUtil(spec); + private Spec spec; + private SpecMilestone specMilestone; + private DataStructureUtil dataStructureUtil; + private RecentChainData recentChainData; + private ForkInfo forkInfo; + private AttestationGossipManager attestationGossipManager; @SuppressWarnings("unchecked") private final OperationProcessor gossipedAttestationProcessor = mock(OperationProcessor.class); - private final RecentChainData recentChainData = MemoryOnlyRecentChainData.create(spec); private final GossipNetwork gossipNetwork = mock(GossipNetwork.class); private final GossipEncoding gossipEncoding = GossipEncoding.SSZ_SNAPPY; - private AttestationGossipManager attestationGossipManager; + private final StubMetricsSystem metricsSystem = new StubMetricsSystem(); private final StubAsyncRunner asyncRunner = new StubAsyncRunner(); - private final ForkInfo forkInfo = - new ForkInfo(spec.fork(UInt64.ZERO), dataStructureUtil.randomBytes32()); - private final AttestationSubnetSubscriptions attestationSubnetSubscriptions = - new AttestationSubnetSubscriptions( - spec, - asyncRunner, - gossipNetwork, - gossipEncoding, - recentChainData, - gossipedAttestationProcessor, - forkInfo, - DebugDataDumper.NOOP); + + private final Int2IntMap committeeSizes = new Int2IntOpenHashMap(); @BeforeEach - public void setup() { + public void setup(final SpecContext specContext) { + spec = specContext.getSpec(); + specMilestone = specContext.getSpecMilestone(); + dataStructureUtil = specContext.getDataStructureUtil(); + recentChainData = MemoryOnlyRecentChainData.create(spec); + forkInfo = new ForkInfo(spec.fork(UInt64.ZERO), dataStructureUtil.randomBytes32()); + + final AttestationSubnetSubscriptions attestationSubnetSubscriptions = + new AttestationSubnetSubscriptions( + spec, + asyncRunner, + gossipNetwork, + gossipEncoding, + recentChainData, + gossipedAttestationProcessor, + forkInfo, + DebugDataDumper.NOOP); + BeaconChainUtil.create(spec, 0, recentChainData).initializeStorage(); + attestationGossipManager = new AttestationGossipManager(metricsSystem, attestationSubnetSubscriptions); } - @Test - public void onNewAttestation_afterMatchingAssignment() { + @TestTemplate + public void onNewAttestation_afterMatchingAssignment(final SpecContext specContext) { + specContext.assumeIsOneOf(PHASE0); // Create a new DataStructureUtil so that generated attestations are not subject to change // when access to the global DataStructureUtil changes final DataStructureUtil dataStructureUtil = new DataStructureUtil(spec); @@ -113,23 +132,30 @@ public void onNewAttestation_afterMatchingAssignment() { verify(gossipNetwork).gossip(getSubnetTopic(subnetId), serialized2); } - @Test + @TestTemplate public void onNewAttestation_noMatchingAssignment() { - final Attestation attestation = dataStructureUtil.randomAttestation(2); + final ValidatableAttestation validatableAttestation = createAttestation(2); + final Attestation attestation = + getExpectedAttestationFromValidatableAttestation(validatableAttestation); + final int subnetId = computeSubnetId(attestation); // Subscribed to different subnet attestationGossipManager.subscribeToSubnetId(subnetId + 1); // Post new attestation - attestationGossipManager.onNewAttestation(ValidatableAttestation.from(spec, attestation)); + attestationGossipManager.onNewAttestation(validatableAttestation); verify(gossipNetwork).gossip(getSubnetTopic(subnetId), gossipEncoding.encode(attestation)); } - @Test + @TestTemplate public void onNewAttestation_afterDismissal() { - final Attestation attestation = dataStructureUtil.randomAttestation(1); - final Attestation attestation2 = dataStructureUtil.randomAttestation(1); + final ValidatableAttestation validatableAttestation = createAttestation(1); + final ValidatableAttestation validatableAttestation2 = createAttestation(1); + final Attestation attestation = + getExpectedAttestationFromValidatableAttestation(validatableAttestation); + final Attestation attestation2 = + getExpectedAttestationFromValidatableAttestation(validatableAttestation2); // Setup committee assignment final int subnetId = computeSubnetId(attestation2); final int dismissedSubnetId = computeSubnetId(attestation); @@ -140,42 +166,72 @@ public void onNewAttestation_afterDismissal() { // Attestation for dismissed assignment should be ignored final Bytes serialized = gossipEncoding.encode(attestation); - attestationGossipManager.onNewAttestation(ValidatableAttestation.from(spec, attestation)); + attestationGossipManager.onNewAttestation(validatableAttestation); verify(gossipNetwork).gossip(getSubnetTopic(dismissedSubnetId), serialized); // Attestation for remaining assignment should be processed final Bytes serialized2 = gossipEncoding.encode(attestation2); - attestationGossipManager.onNewAttestation(ValidatableAttestation.from(spec, attestation2)); + attestationGossipManager.onNewAttestation(validatableAttestation2); verify(gossipNetwork).gossip(getSubnetTopic(subnetId), serialized2); } - @Test + @TestTemplate void onNewAttestation_incrementSuccessCount() { - final Attestation attestation = dataStructureUtil.randomAttestation(3); + final ValidatableAttestation validatableAttestation = createAttestation(3); + when(gossipNetwork.gossip(any(), any())).thenReturn(SafeFuture.completedFuture(null)); // Attestation for dismissed assignment should be ignored - attestationGossipManager.onNewAttestation(ValidatableAttestation.from(spec, attestation)); + attestationGossipManager.onNewAttestation(validatableAttestation); assertThat(getPublishSuccessCounterValue()).isEqualTo(1); assertThat(getPublishFailureCounterValue()).isZero(); } - @Test + @TestTemplate void onNewAttestation_incrementFailureCount() { - final Attestation attestation = dataStructureUtil.randomAttestation(); + final ValidatableAttestation validatableAttestation = createAttestation(3); when(gossipNetwork.gossip(any(), any())) .thenReturn(SafeFuture.failedFuture(new RuntimeException("Ooops"))); // Attestation for dismissed assignment should be ignored - attestationGossipManager.onNewAttestation(ValidatableAttestation.from(spec, attestation)); + attestationGossipManager.onNewAttestation(validatableAttestation); assertThat(getPublishSuccessCounterValue()).isZero(); assertThat(getPublishFailureCounterValue()).isEqualTo(1); } + private ValidatableAttestation createAttestation(final int slot) { + final Attestation attestationFromValidators; + + if (specMilestone.isGreaterThanOrEqualTo(ELECTRA)) { + attestationFromValidators = dataStructureUtil.randomSingleAttestation(UInt64.valueOf(slot)); + } else { + attestationFromValidators = dataStructureUtil.randomAttestation(slot); + } + + final ValidatableAttestation validatableAttestation = + ValidatableAttestation.from(spec, attestationFromValidators, committeeSizes); + + if (attestationFromValidators.isSingleAttestation()) { + validatableAttestation.convertToAggregatedFormatFromSingleAttestation( + dataStructureUtil.randomAttestation(slot)); + } + + return validatableAttestation; + } + + private Attestation getExpectedAttestationFromValidatableAttestation( + final ValidatableAttestation validatableAttestation) { + if (specMilestone.isGreaterThanOrEqualTo(ELECTRA)) { + return validatableAttestation.getUnconvertedAttestation(); + } else { + return validatableAttestation.getAttestation(); + } + } + private long getPublishSuccessCounterValue() { return getPublishCounter().getValue("success"); }