From 0b26e1f90bf0d203d75c780b1401784abff469a4 Mon Sep 17 00:00:00 2001 From: Lucas Saldanha Date: Tue, 3 Dec 2024 14:15:52 +1300 Subject: [PATCH] Logic to handle new validator during epoch processing --- .../epoch/AbstractEpochProcessor.java | 60 +++++++++++---- .../AbstractValidatorStatusFactory.java | 11 +++ .../epoch/status/ValidatorStatusFactory.java | 15 ++++ .../epoch/EpochProcessorElectra.java | 5 ++ .../epoch/AbstractEpochProcessorTest.java | 73 +++++++++++++++++-- .../AbstractValidatorStatusFactoryTest.java | 66 +++++++++++++++++ .../epoch/EpochProcessorElectraTest.java | 32 ++++++++ 7 files changed, 244 insertions(+), 18 deletions(-) create mode 100644 ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/versions/electra/statetransition/epoch/EpochProcessorElectraTest.java diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/AbstractEpochProcessor.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/AbstractEpochProcessor.java index 6ca58ced844..e36f39d22ab 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/AbstractEpochProcessor.java +++ b/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 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); diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/status/AbstractValidatorStatusFactory.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/status/AbstractValidatorStatusFactory.java index cbf3d673201..18cceb43e60 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/status/AbstractValidatorStatusFactory.java +++ b/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 validatorStatusesToAppend) { + final List validatorStatusesList = + Stream.concat(validatorStatuses.getStatuses().stream(), validatorStatusesToAppend.stream()) + .toList(); + return new ValidatorStatuses(validatorStatusesList, validatorStatuses.getTotalBalances()); + } + private TotalBalances createTotalBalances( final BeaconState state, final List statuses) { final TransitionCaches transitionCaches = BeaconStateCache.getTransitionCaches(state); diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/status/ValidatorStatusFactory.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/status/ValidatorStatusFactory.java index 9ebc9b02fe2..62f0cb3a031 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/status/ValidatorStatusFactory.java +++ b/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 validatorStatusesToAppend); } diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/electra/statetransition/epoch/EpochProcessorElectra.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/electra/statetransition/epoch/EpochProcessorElectra.java index ebd9a1edbc0..8eedf3e372e 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/electra/statetransition/epoch/EpochProcessorElectra.java +++ b/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; + } } diff --git a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/AbstractEpochProcessorTest.java b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/AbstractEpochProcessorTest.java index 97f9dafe3c9..065554429b3 100644 --- a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/AbstractEpochProcessorTest.java +++ b/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,6 +39,8 @@ 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; @@ -43,11 +48,7 @@ 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 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 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 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()); + } } diff --git a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/status/AbstractValidatorStatusFactoryTest.java b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/status/AbstractValidatorStatusFactoryTest.java index 3239295ab16..b21c4a15250 100644 --- a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/common/statetransition/epoch/status/AbstractValidatorStatusFactoryTest.java +++ b/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); diff --git a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/versions/electra/statetransition/epoch/EpochProcessorElectraTest.java b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/logic/versions/electra/statetransition/epoch/EpochProcessorElectraTest.java new file mode 100644 index 00000000000..0fa2d60d824 --- /dev/null +++ b/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(); + } +}