Logic to handle new validators during epoch processing

ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/AbstractEpochProcessor.java

@@ -15,6 +15,8 @@
 
 import static tech.pegasys.teku.infrastructure.unsigned.UInt64.ZERO;
 
+import com.google.common.annotations.VisibleForTesting;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.function.Consumer;
 import java.util.function.Supplier;
@@ -104,18 +106,10 @@ public BeaconState processEpoch(final BeaconState preState) throws EpochProcessi
 
   protected void processEpoch(final BeaconState preState, final MutableBeaconState state)
       throws EpochProcessingException {
-    /*
-     WARNING: After Electra, it is possible that the validator set is updated within epoch processing
-     (process_pending_deposits). This means that the validator set in the state can get out of sync with
-     our validatorStatuses cache. This is not a problem for the current epoch processing, but it can cause
-     undesired side effects in the future.
-
-     Up until Electra, the only function that uses validatorStatuses after process_pending_deposits is
-     process_effective_balance_updates, and in this particular case it is ok that we don't have the new validators
-     in validatorStatuses.
-    */
-    final ValidatorStatuses validatorStatuses =
-        validatorStatusFactory.createValidatorStatuses(preState);
+    // After Electra, it is possible that the validator set is updated within epoch processing
+    // (process_pending_deposits). This is handled by recreateValidatorStatusIfNewValidatorsAreFound
+    // (post-Electra)
+    ValidatorStatuses validatorStatuses = validatorStatusFactory.createValidatorStatuses(preState);
 
     final UInt64 currentEpoch = beaconStateAccessors.getCurrentEpoch(state);
     final TotalBalances totalBalances = validatorStatuses.getTotalBalances();
@@ -133,6 +127,12 @@ protected void processEpoch(final BeaconState preState, final MutableBeaconState
     processSlashings(state, validatorStatuses);
     processEth1DataReset(state);
     processPendingDeposits(state);
+
+    if (shouldCheckNewValidatorsDuringEpochProcessing()) {
+      validatorStatuses =
+          recreateValidatorStatusIfNewValidatorsAreFound(state, validatorStatuses, currentEpoch);
+    }
+
     processPendingConsolidations(state);
     processEffectiveBalanceUpdates(state, validatorStatuses.getStatuses());
     processSlashingsReset(state);
@@ -148,6 +148,40 @@ protected void processEpoch(final BeaconState preState, final MutableBeaconState
     }
   }
 
+  @VisibleForTesting
+  public ValidatorStatuses recreateValidatorStatusIfNewValidatorsAreFound(
+      final BeaconState state,
+      final ValidatorStatuses validatorStatuses,
+      final UInt64 currentEpoch) {
+    final int preValidatorCount = validatorStatuses.getValidatorCount();
+    final int postValidatorCount = state.getValidators().size();
+    if (postValidatorCount > preValidatorCount) {
+      // New validators added, create new  validator statuses
+      final List<ValidatorStatus> newValidatorStatuses =
+          new ArrayList<>(postValidatorCount - preValidatorCount);
+      for (int i = preValidatorCount; i < postValidatorCount; i++) {
+        final ValidatorStatus status =
+            validatorStatusFactory.createValidatorStatus(
+                state.getValidators().get(i), currentEpoch.minus(1), currentEpoch);
+        newValidatorStatuses.add(status);
+      }
+      return validatorStatusFactory.recreateValidatorStatuses(
+          validatorStatuses, newValidatorStatuses);
+    } else {
+      return validatorStatuses;
+    }
+  }
+
+  /**
+   * This method is used to decide if we want to check the possibility of the validator set changing
+   * mid-processing an epoch. This is only required post-Electra.
+   *
+   * @return false by default, true post-Electra (EpochProcessorElectra overrides this method)
+   */
+  protected boolean shouldCheckNewValidatorsDuringEpochProcessing() {
+    return false;
+  }
+
   private void updateTransitionCaches(
       final MutableBeaconState state,
       final UInt64 currentEpoch,
@@ -463,7 +497,7 @@ public void processEffectiveBalanceUpdates(
     final UInt64 maxEffectiveBalance = specConfig.getMaxEffectiveBalance();
     final UInt64 hysteresisQuotient = specConfig.getHysteresisQuotient();
     final UInt64 effectiveBalanceIncrement = specConfig.getEffectiveBalanceIncrement();
-    for (int index = 0; index < validators.size(); index++) {
+    for (int index = 0; index < statuses.size(); index++) {
       final ValidatorStatus status = statuses.get(index);
       final UInt64 balance = balances.getElement(index);
 

ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/status/AbstractValidatorStatusFactory.java

@@ -16,6 +16,7 @@
 import static tech.pegasys.teku.infrastructure.unsigned.UInt64.MAX_VALUE;
 
 import java.util.List;
+import java.util.stream.Stream;
 import org.apache.tuweni.bytes.Bytes32;
 import tech.pegasys.teku.infrastructure.ssz.SszList;
 import tech.pegasys.teku.infrastructure.unsigned.UInt64;
@@ -70,6 +71,16 @@ public ValidatorStatuses createValidatorStatuses(final BeaconState state) {
     return new ValidatorStatuses(statuses, createTotalBalances(state, statuses));
   }
 
+  @Override
+  public ValidatorStatuses recreateValidatorStatuses(
+      final ValidatorStatuses validatorStatuses,
+      final List<ValidatorStatus> validatorStatusesToAppend) {
+    final List<ValidatorStatus> validatorStatusesList =
+        Stream.concat(validatorStatuses.getStatuses().stream(), validatorStatusesToAppend.stream())
+            .toList();
+    return new ValidatorStatuses(validatorStatusesList, validatorStatuses.getTotalBalances());
+  }
+
   private TotalBalances createTotalBalances(
       final BeaconState state, final List<ValidatorStatus> statuses) {
     final TransitionCaches transitionCaches = BeaconStateCache.getTransitionCaches(state);

ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/status/ValidatorStatusFactory.java

@@ -13,13 +13,28 @@
 
 package tech.pegasys.teku.spec.logic.common.statetransition.epoch.status;
 
+import java.util.List;
 import tech.pegasys.teku.infrastructure.unsigned.UInt64;
 import tech.pegasys.teku.spec.datastructures.state.Validator;
 import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState;
 
 public interface ValidatorStatusFactory {
+
   ValidatorStatuses createValidatorStatuses(BeaconState state);
 
   ValidatorStatus createValidatorStatus(
       final Validator validator, final UInt64 previousEpoch, final UInt64 currentEpoch);
+
+  /**
+   * Creates a new ValidatorStatuses object with the existing list of statuses and the new statuses
+   * from the given list. This is cheaper than creating a new one using createValidatorStatus method
+   * because it will not recompute any data for validators already mapped in this object.
+   *
+   * @param validatorStatuses existing ValidatorStatuses object
+   * @param validatorStatusesToAppend new statuses to append to the exiting ValidatorStatuses object
+   * @return a new instance of ValidatorStatuses with both pre-existing and new validator statuses
+   */
+  ValidatorStatuses recreateValidatorStatuses(
+      final ValidatorStatuses validatorStatuses,
+      final List<ValidatorStatus> validatorStatusesToAppend);
 }

ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/electra/statetransition/epoch/EpochProcessorElectra.java

@@ -379,4 +379,9 @@ public void processSlashings(
       }
     }
   }
+
+  @Override
+  protected boolean shouldCheckNewValidatorsDuringEpochProcessing() {
+    return true;
+  }
 }

ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/AbstractEpochProcessorTest.java

@@ -13,6 +13,7 @@
 
 package tech.pegasys.teku.spec.logic.common.statetransition.epoch;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.mock;
@@ -23,6 +24,8 @@
 
 import java.lang.reflect.Field;
 import java.time.Duration;
+import java.util.List;
+import org.apache.commons.lang3.reflect.FieldUtils;
 import org.apache.logging.log4j.Logger;
 import org.junit.jupiter.api.Test;
 import org.junit.platform.commons.util.ReflectionUtils;
@@ -36,18 +39,16 @@
 import tech.pegasys.teku.spec.TestSpecFactory;
 import tech.pegasys.teku.spec.datastructures.state.Validator;
 import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState;
+import tech.pegasys.teku.spec.logic.common.statetransition.epoch.status.ValidatorStatusFactory;
+import tech.pegasys.teku.spec.logic.common.statetransition.epoch.status.ValidatorStatuses;
 import tech.pegasys.teku.spec.logic.versions.capella.statetransition.epoch.EpochProcessorCapella;
 import tech.pegasys.teku.spec.util.DataStructureUtil;
 
 class AbstractEpochProcessorTest {
 
   private final Spec spec = TestSpecFactory.createMinimalCapella();
   private final DataStructureUtil dataStructureUtil = new DataStructureUtil(spec);
-  private final StubTimeProvider timeProvider = StubTimeProvider.withTimeInSeconds(100_000L);
-  private final EpochProcessorCapella epochProcessor =
-      new EpochProcessorCapella(
-          (EpochProcessorCapella) spec.getGenesisSpec().getEpochProcessor(), timeProvider);
-
+  private final EpochProcessor epochProcessor = spec.getGenesisSpec().getEpochProcessor();
   private final int throttlingPeriod = 1; // expect maximum of one call per second
   private static final Logger LOGGER = mock(Logger.class);
   private final Throttler<Logger> loggerThrottler = spyLogThrottler(LOGGER, throttlingPeriod);
@@ -56,6 +57,10 @@ class AbstractEpochProcessorTest {
 
   @Test
   public void shouldThrottleInactivityLeakLogs() throws Exception {
+    final StubTimeProvider timeProvider = StubTimeProvider.withTimeInSeconds(100_000L);
+    final EpochProcessor epochProcessor = spec.getGenesisSpec().getEpochProcessor();
+    FieldUtils.writeField(epochProcessor, "timeProvider", timeProvider, true);
+
     // First two processEpoch calls within the same second
     epochProcessor.processEpoch(state);
     epochProcessor.processEpoch(advanceNSlots(state, 1));
@@ -111,4 +116,62 @@ private Throttler<Logger> spyLogThrottler(final Logger logger, final int throttl
 
     return loggerThrottler;
   }
+
+  @Test
+  public void shouldCheckNewValidatorsDuringEpochProcessingReturnsFalse() {
+    assertThat(
+            ((AbstractEpochProcessor) epochProcessor)
+                .shouldCheckNewValidatorsDuringEpochProcessing())
+        .isFalse();
+  }
+
+  @Test
+  public void recreateValidatorStatusWithNoNewValidators() {
+    final BeaconState preState =
+        dataStructureUtil.stateBuilder(spec.getGenesisSpec().getMilestone(), 10, 3).build();
+    final UInt64 currentEpoch = spec.computeEpochAtSlot(preState.getSlot());
+    final ValidatorStatusFactory validatorStatusFactory =
+        spec.atSlot(preState.getSlot()).getValidatorStatusFactory();
+
+    final ValidatorStatuses validatorStatuses =
+        validatorStatusFactory.createValidatorStatuses(preState);
+
+    final ValidatorStatuses newValidatorStatuses =
+        ((AbstractEpochProcessor) epochProcessor)
+            .recreateValidatorStatusIfNewValidatorsAreFound(
+                preState, validatorStatuses, currentEpoch);
+
+    assertThat(preState.getValidators().size())
+        .isEqualTo(newValidatorStatuses.getStatuses().size());
+  }
+
+  @Test
+  public void recreateValidatorStatusWithNewValidators() {
+    final BeaconState preState =
+        dataStructureUtil.stateBuilder(spec.getGenesisSpec().getMilestone(), 10, 3).build();
+    final UInt64 currentEpoch = spec.computeEpochAtSlot(preState.getSlot());
+    final ValidatorStatusFactory validatorStatusFactory =
+        spec.atSlot(preState.getSlot()).getValidatorStatusFactory();
+
+    final ValidatorStatuses validatorStatuses =
+        validatorStatusFactory.createValidatorStatuses(preState);
+
+    final List<Validator> newValidators =
+        List.of(
+            dataStructureUtil.randomValidator(),
+            dataStructureUtil.randomValidator(),
+            dataStructureUtil.randomValidator());
+    final BeaconState postState =
+        preState.updated(state -> newValidators.forEach(state.getValidators()::append));
+
+    final ValidatorStatuses newValidatorStatuses =
+        ((AbstractEpochProcessor) epochProcessor)
+            .recreateValidatorStatusIfNewValidatorsAreFound(
+                postState, validatorStatuses, currentEpoch);
+
+    assertThat(preState.getValidators().size() + newValidators.size())
+        .isEqualTo(newValidatorStatuses.getStatuses().size());
+    assertThat(postState.getValidators().size())
+        .isEqualTo(newValidatorStatuses.getStatuses().size());
+  }
 }

ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/status/AbstractValidatorStatusFactoryTest.java

@@ -15,10 +15,16 @@
 
 import static java.util.Collections.emptyList;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 import static tech.pegasys.teku.spec.config.SpecConfig.FAR_FUTURE_EPOCH;
 
 import java.util.List;
 import java.util.stream.Stream;
+import org.apache.commons.lang3.reflect.FieldUtils;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
@@ -28,6 +34,8 @@
 import tech.pegasys.teku.spec.Spec;
 import tech.pegasys.teku.spec.config.SpecConfig;
 import tech.pegasys.teku.spec.datastructures.state.Validator;
+import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState;
+import tech.pegasys.teku.spec.logic.common.util.AttestationUtil;
 import tech.pegasys.teku.spec.util.DataStructureUtil;
 
 public abstract class AbstractValidatorStatusFactoryTest {
@@ -249,6 +257,64 @@ void createTotalBalances_shouldReturnMinimumOfOneEffectiveBalanceIncrement() {
     assertThat(balances.getPreviousEpochHeadAttesters()).isEqualTo(effectiveBalanceInc);
   }
 
+  @Test
+  public void recreateValidatorStatusesShouldAppendNewValidatorsAndKeepOrder() {
+    final BeaconState state =
+        dataStructureUtil
+            .stateBuilder(spec.getGenesisSpec().getMilestone(), 3, 1)
+            .setSlotToStartOfEpoch(UInt64.ONE)
+            .build();
+    final ValidatorStatuses validatorStatuses =
+        validatorStatusFactory.createValidatorStatuses(state);
+
+    final UInt64 currentEpoch = spec.computeEpochAtSlot(state.getSlot());
+    final Validator newValidator = dataStructureUtil.validatorBuilder().slashed(true).build();
+    final ValidatorStatus newValidatorStatus =
+        validatorStatusFactory.createValidatorStatus(
+            newValidator, currentEpoch.minusMinZero(1), currentEpoch);
+
+    final ValidatorStatuses updatedValidatorStatuses =
+        validatorStatusFactory.recreateValidatorStatuses(
+            validatorStatuses, List.of(newValidatorStatus));
+
+    final ValidatorStatus[] expectedStatuses =
+        Stream.concat(validatorStatuses.getStatuses().stream(), Stream.of(newValidatorStatus))
+            .toArray(ValidatorStatus[]::new);
+
+    assertThat(updatedValidatorStatuses.getStatuses()).containsExactly(expectedStatuses);
+  }
+
+  @Test
+  public void shouldNotRecalculateValidatorStatusForPreviousExistingValidators()
+      throws IllegalAccessException {
+    final ValidatorStatusFactory factory = spy(createFactory());
+    // Magic to get around requiring a full state with valid previous and current epoch attestations
+    // (only on Phase0)
+    FieldUtils.writeField(factory, "attestationUtil", mock(AttestationUtil.class), true);
+
+    final int validatorCount = 10;
+    final BeaconState state =
+        dataStructureUtil
+            .stateBuilder(spec.getGenesisSpec().getMilestone(), validatorCount, 1)
+            .build();
+    final UInt64 currentEpoch = spec.computeEpochAtSlot(state.getSlot());
+    final ValidatorStatuses validatorStatuses = factory.createValidatorStatuses(state);
+
+    // Created ValidatorStatus for all validators in state
+    verify(factory, times(validatorCount)).createValidatorStatus(any(), any(), any());
+
+    final Validator newValidator = dataStructureUtil.validatorBuilder().slashed(true).build();
+    final ValidatorStatus newValidatorStatus =
+        factory.createValidatorStatus(newValidator, currentEpoch.minusMinZero(1), currentEpoch);
+    // Created ValidatorStatus for the new validator
+    verify(factory, times(validatorCount + 1)).createValidatorStatus(any(), any(), any());
+
+    factory.recreateValidatorStatuses(validatorStatuses, List.of(newValidatorStatus));
+
+    // Verifying that recreateValidatorStatuses does not trigger any ValidatorStatus creation
+    verify(factory, times(validatorCount + 1)).createValidatorStatus(any(), any(), any());
+  }
+
   private ValidatorStatus createValidator(final int effectiveBalance) {
     return new ValidatorStatus(
         false, false, balance(effectiveBalance), withdrawableEpoch, true, true, true);

ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/versions/electra/statetransition/epoch/EpochProcessorElectraTest.java

@@ -0,0 +1,32 @@
+/*
+ * Copyright Consensys Software Inc., 2024
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+
+package tech.pegasys.teku.spec.logic.versions.electra.statetransition.epoch;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.jupiter.api.Test;
+import tech.pegasys.teku.spec.Spec;
+import tech.pegasys.teku.spec.TestSpecFactory;
+
+class EpochProcessorElectraTest {
+
+  private final Spec spec = TestSpecFactory.createMinimalElectra();
+  private final EpochProcessorElectra epochProcessor =
+      (EpochProcessorElectra) spec.getGenesisSpec().getEpochProcessor();
+
+  @Test
+  public void shouldCheckNewValidatorsDuringEpochProcessingReturnsTrue() {
+    assertThat(epochProcessor.shouldCheckNewValidatorsDuringEpochProcessing()).isTrue();
+  }
+}