Ensure operations that are in the pool and not valid are not included (…

…#6090) - exits that are for an epoch prior to the last milestone should not be included - slashings that are for an epoch prior to the last milestone should not be included - ensure we error if a request is made to voluntary exit command to generate an exit that would be too old partially addresses #6087 Signed-off-by: Paul Harris <[email protected]>
Consensys · Aug 18, 2022 · c514633 · c514633
1 parent 5cd7492
commit c514633
Show file tree

Hide file tree

Showing 8 changed files with 117 additions and 6 deletions.
diff --git a/.../src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java b/.../src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java
@@ -90,6 +90,10 @@ public Consumer<BeaconBlockBodyBuilder> createSelector(
     return bodyBuilder -> {
       final Eth1Data eth1Data = eth1DataCache.getEth1Vote(blockSlotState);
 
+      final UInt64 firstEpochOfPreviousFork =
+          spec.computePreviousForkEpochStart(blockSlotState.getSlot());
+      final UInt64 firstSlotOfPreviousFork = spec.computeStartSlotAtEpoch(firstEpochOfPreviousFork);
+
       final SszList<Attestation> attestations =
           attestationPool.getAttestationsForBlock(
               blockSlotState,
@@ -101,22 +105,37 @@ public Consumer<BeaconBlockBodyBuilder> createSelector(
       final SszList<AttesterSlashing> attesterSlashings =
           attesterSlashingPool.getItemsForBlock(
               blockSlotState,
-              slashing -> !exitedValidators.containsAll(slashing.getIntersectingValidatorIndices()),
+              slashing ->
+                  !exitedValidators.containsAll(slashing.getIntersectingValidatorIndices())
+                      && slashing
+                          .getAttestation1()
+                          .getData()
+                          .getSlot()
+                          .isGreaterThanOrEqualTo(firstSlotOfPreviousFork),
               slashing -> exitedValidators.addAll(slashing.getIntersectingValidatorIndices()));
 
       final SszList<ProposerSlashing> proposerSlashings =
           proposerSlashingPool.getItemsForBlock(
               blockSlotState,
               slashing ->
-                  !exitedValidators.contains(slashing.getHeader1().getMessage().getProposerIndex()),
+                  !exitedValidators.contains(slashing.getHeader1().getMessage().getProposerIndex())
+                      && slashing
+                          .getHeader1()
+                          .getMessage()
+                          .getSlot()
+                          .isGreaterThanOrEqualTo(firstSlotOfPreviousFork),
               slashing ->
                   exitedValidators.add(slashing.getHeader1().getMessage().getProposerIndex()));
 
       // Collect exits to include
       final SszList<SignedVoluntaryExit> voluntaryExits =
           voluntaryExitPool.getItemsForBlock(
               blockSlotState,
-              exit -> !exitedValidators.contains(exit.getMessage().getValidatorIndex()),
+              exit ->
+                  !exitedValidators.contains(exit.getMessage().getValidatorIndex())
+                      && exit.getMessage()
+                          .getEpoch()
+                          .isGreaterThanOrEqualTo(firstEpochOfPreviousFork),
               exit -> exitedValidators.add(exit.getMessage().getValidatorIndex()));
 
       bodyBuilder

diff --git a/.../test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java b/.../test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java
@@ -25,16 +25,21 @@
 import java.util.Optional;
 import java.util.function.Function;
 import java.util.function.Supplier;
+import java.util.stream.Stream;
 import org.apache.tuweni.bytes.Bytes32;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 import tech.pegasys.teku.bls.BLSSignature;
 import tech.pegasys.teku.infrastructure.async.SafeFuture;
 import tech.pegasys.teku.infrastructure.metrics.StubMetricsSystem;
 import tech.pegasys.teku.infrastructure.ssz.SszData;
 import tech.pegasys.teku.infrastructure.ssz.SszList;
 import tech.pegasys.teku.infrastructure.unsigned.UInt64;
 import tech.pegasys.teku.spec.Spec;
+import tech.pegasys.teku.spec.SpecMilestone;
 import tech.pegasys.teku.spec.TestSpecFactory;
 import tech.pegasys.teku.spec.datastructures.blocks.Eth1Data;
 import tech.pegasys.teku.spec.datastructures.blocks.SignedBeaconBlock;
@@ -51,6 +56,7 @@
 import tech.pegasys.teku.spec.datastructures.operations.Deposit;
 import tech.pegasys.teku.spec.datastructures.operations.ProposerSlashing;
 import tech.pegasys.teku.spec.datastructures.operations.SignedVoluntaryExit;
+import tech.pegasys.teku.spec.datastructures.operations.VoluntaryExit;
 import tech.pegasys.teku.spec.datastructures.operations.versions.altair.SignedContributionAndProof;
 import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState;
 import tech.pegasys.teku.spec.executionlayer.ExecutionLayerChannel;
@@ -187,6 +193,43 @@ void shouldNotSelectOperationsWhenNoneAreAvailable() {
         .isTrue();
   }
 
+  @ParameterizedTest(name = "{0}_{1}")
+  @MethodSource("getIncludeFromSourcesUseCases")
+  public void shouldNotIncludeExitsFromIncorrectFork(
+      final SpecMilestone specMilestone, final int exitSlot, final boolean isIncluded) {
+    final Spec localSpec =
+        TestSpecFactory.createMinimalWithAltairAndBellatrixForkEpoch(
+            UInt64.valueOf(50), UInt64.valueOf(100));
+    final DataStructureUtil data = new DataStructureUtil(localSpec);
+    final UInt64 slot = UInt64.valueOf(101 * 8);
+    final BeaconState blockSlotState = data.randomBeaconState(slot);
+    final SignedVoluntaryExit voluntaryExit =
+        new SignedVoluntaryExit(
+            new VoluntaryExit(UInt64.valueOf(exitSlot), UInt64.ONE), data.randomSignature());
+    final BlockOperationSelectorFactory myFactory =
+        new BlockOperationSelectorFactory(
+            localSpec,
+            attestationPool,
+            attesterSlashingPool,
+            proposerSlashingPool,
+            voluntaryExitPool,
+            contributionPool,
+            depositProvider,
+            eth1DataCache,
+            defaultGraffiti,
+            forkChoiceNotifier,
+            executionLayer);
+    addToPool(voluntaryExitPool, voluntaryExit);
+    myFactory
+        .createSelector(parentRoot, blockSlotState, randaoReveal, Optional.empty())
+        .accept(bodyBuilder);
+    if (isIncluded) {
+      assertThat(bodyBuilder.voluntaryExits).containsExactly(voluntaryExit);
+    } else {
+      assertThat(bodyBuilder.voluntaryExits).isEmpty();
+    }
+  }
+
   @Test
   void shouldIncludeValidOperations() {
     final UInt64 slot = UInt64.valueOf(2);
@@ -405,6 +448,17 @@ void shouldUnblindSignedBlindedBeaconBlock() {
     assertThat(blockUnblinder.executionPayload).isCompletedWithValue(randomExecutionPayload);
   }
 
+  private static Stream<Arguments> getIncludeFromSourcesUseCases() {
+    // 0 == phase0, 50 == altair, 100 = bellatrix
+    // at slot 101 to compute inclusion
+    return Stream.of(
+        Arguments.of(SpecMilestone.PHASE0, 0, false),
+        Arguments.of(SpecMilestone.PHASE0, 49, false),
+        Arguments.of(SpecMilestone.ALTAIR, 50, true),
+        Arguments.of(SpecMilestone.BELLATRIX, 100, true),
+        Arguments.of(SpecMilestone.BELLATRIX, 101, true));
+  }
+
   private static class CapturingBeaconBlockBodyBuilder implements BeaconBlockBodyBuilder {
     private final boolean blinded;
 

diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/ForkSchedule.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/ForkSchedule.java
@@ -88,6 +88,10 @@ public Fork getFork(final UInt64 epoch) {
     return milestoneToFork.get(getSpecMilestoneAtEpoch(epoch));
   }
 
+  public Optional<Fork> getFork(final SpecMilestone milestone) {
+    return Optional.ofNullable(milestoneToFork.get(milestone));
+  }
+
   public Optional<Fork> getNextFork(final UInt64 epoch) {
     return Optional.ofNullable(epochToMilestone.ceilingEntry(epoch.plus(1)))
         .map(Map.Entry::getValue)

diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/Spec.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/Spec.java
@@ -752,6 +752,17 @@ public boolean isMergeTransitionComplete(final BeaconState state) {
     return atState(state).miscHelpers().isMergeTransitionComplete(state);
   }
 
+  public UInt64 computePreviousForkEpochStart(final UInt64 slot) {
+    final SpecMilestone milestone = atSlot(slot).getMilestone();
+    final Optional<SpecMilestone> previousMilestone = SpecMilestone.getPreviousMilestone(milestone);
+    if (previousMilestone.isEmpty()) {
+      return UInt64.ZERO;
+    }
+    final Optional<Fork> fork = forkSchedule.getFork(previousMilestone.get());
+
+    return fork.map(Fork::getEpoch).orElse(UInt64.ZERO);
+  }
+
   // Private helpers
   private SpecVersion atState(final BeaconState state) {
     return atSlot(state.getSlot());

diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/SpecMilestone.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/SpecMilestone.java
@@ -37,6 +37,12 @@ public boolean isGreaterThanOrEqualTo(final SpecMilestone other) {
     return compareTo(other) >= 0;
   }
 
+  static Optional<SpecMilestone> getPreviousMilestone(final SpecMilestone milestone) {
+    if (milestone.ordinal() == 0) {
+      return Optional.empty();
+    }
+    return Optional.of(SpecMilestone.values()[milestone.ordinal() - 1]);
+  }
   /**
    * @param milestone The milestone being inspected
    * @return An ordered list of all milestones preceding the supplied milestone

diff --git a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/SpecMilestoneTest.java b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/SpecMilestoneTest.java
@@ -44,8 +44,12 @@ public void isGreaterThanOrEqualTo() {
   }
 
   @Test
-  public void getAllPriorMilestones_phase0() {
-    assertThat(SpecMilestone.getAllPriorMilestones(SpecMilestone.PHASE0)).isEmpty();
+  void getPreviousMilestone() {
+    assertThat(SpecMilestone.getPreviousMilestone(SpecMilestone.PHASE0)).isEmpty();
+    assertThat(SpecMilestone.getPreviousMilestone(SpecMilestone.ALTAIR))
+        .contains(SpecMilestone.PHASE0);
+    assertThat(SpecMilestone.getPreviousMilestone(SpecMilestone.BELLATRIX))
+        .contains(SpecMilestone.ALTAIR);
   }
 
   @Test

diff --git a/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/TestSpecFactory.java b/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/TestSpecFactory.java
@@ -13,6 +13,7 @@
 
 package tech.pegasys.teku.spec;
 
+import com.google.common.base.Preconditions;
 import java.util.function.Consumer;
 import tech.pegasys.teku.infrastructure.unsigned.UInt64;
 import tech.pegasys.teku.spec.config.SpecConfig;
@@ -99,6 +100,18 @@ public static Spec createMinimalWithBellatrixForkEpoch(final UInt64 bellatrixFor
     return create(config, SpecMilestone.BELLATRIX);
   }
 
+  public static Spec createMinimalWithAltairAndBellatrixForkEpoch(
+      final UInt64 altairEpoch, final UInt64 bellatrixForkEpoch) {
+    Preconditions.checkArgument(
+        altairEpoch.isLessThan(bellatrixForkEpoch),
+        String.format(
+            "Altair epoch %s must be less than bellatrix epoch %s",
+            altairEpoch, bellatrixForkEpoch));
+    final SpecConfigBellatrix config =
+        getBellatrixSpecConfig(Eth2Network.MINIMAL, altairEpoch, bellatrixForkEpoch);
+    return create(config, SpecMilestone.BELLATRIX);
+  }
+
   public static Spec createMinimalPhase0() {
     final SpecConfig specConfig = SpecConfigLoader.loadConfig(Eth2Network.MINIMAL.configName());
     return create(specConfig, SpecMilestone.PHASE0);

diff --git a/teku/src/main/java/tech/pegasys/teku/cli/subcommand/VoluntaryExitCommand.java b/teku/src/main/java/tech/pegasys/teku/cli/subcommand/VoluntaryExitCommand.java
@@ -232,7 +232,7 @@ private void initialise() {
     spec = getSpec(apiClient);
 
     validateOrDefaultEpoch();
-    fork = spec.getForkSchedule().getFork(epoch);
+    fork = spec.fork(epoch);
 
     // get genesis time
     final Optional<Bytes32> maybeRoot = getGenesisRoot();