From f2054c325f4f11041cae12bc0fa088ce1f1da978 Mon Sep 17 00:00:00 2001 From: Paul Harris Date: Thu, 6 Jun 2024 14:21:59 +1000 Subject: [PATCH] reverted validatorIndexCache (#8354) Reverted the changes to validatorIndexCache, we'll have to take a look again for full electra support. Although the intent was to only index finalized validators, the result was to constantly be doing StateTransitions somehow to get indices, and the outcome was a serious slow down in the important state transitions that happen constantly, so as a starting point, this will put us back to releasable code that will be ok for Deneb. Before reverting, it seemed to process 1 state transition every 20 seconds or so, and even just rebuilding a handful of states was taking 10 minutes or longer, which is not really going to work in production. We'll also need to build out testing for this once we re-implement. Archive storage is already slow due to epoch processing, we can't afford to take this kind of increased performance hit. I'm guessing the tracker may still be part of an eventual solution, haven't removed at this point but i've cleaned it out functionally... i probably need to clean that before any merge... `BlockProcessorElectra` was changed to not use the cache to find validator index, as it's a simple operation, and it was causing problems. Signed-off-by: Paul Harris --- CHANGELOG.md | 1 + .../common/ValidatorIndexCache.java | 90 +++++++------------ .../electra/block/BlockProcessorElectra.java | 12 ++- .../common/ValidatorIndexCacheTest.java | 20 ----- 4 files changed, 46 insertions(+), 77 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3e5f63ee40..6935f806ad9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,3 +19,4 @@ the [releases page](https://github.com/Consensys/teku/releases). endpoints. In the cases where the block has been produced in the same beacon node, only equivocation validation will be done instead of the entire gossip validation. ### Bug Fixes +- Fixed performance degradation introduced in 24.4.0 regarding archive state retrieval time. diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/state/beaconstate/common/ValidatorIndexCache.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/state/beaconstate/common/ValidatorIndexCache.java index 98713f69066..9507530a814 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/state/beaconstate/common/ValidatorIndexCache.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/state/beaconstate/common/ValidatorIndexCache.java @@ -16,6 +16,8 @@ import com.google.common.annotations.VisibleForTesting; import java.util.Optional; import java.util.concurrent.atomic.AtomicInteger; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import tech.pegasys.teku.bls.BLSPublicKey; import tech.pegasys.teku.infrastructure.collections.cache.Cache; import tech.pegasys.teku.infrastructure.collections.cache.LRUCache; @@ -25,16 +27,15 @@ import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState; public class ValidatorIndexCache { + private static final Logger LOG = LogManager.getLogger(); + private final Cache validatorIndices; + private final AtomicInteger lastCachedIndex; private static final int INDEX_NONE = -1; - + private final AtomicInteger latestFinalizedIndex; static final ValidatorIndexCache NO_OP_INSTANCE = new ValidatorIndexCache(NoOpCache.getNoOpCache(), INDEX_NONE, INDEX_NONE); - private final Cache validatorIndices; - private final AtomicInteger latestFinalizedIndex; - private final AtomicInteger lastCachedIndex; - @VisibleForTesting ValidatorIndexCache( final Cache validatorIndices, @@ -46,39 +47,29 @@ public class ValidatorIndexCache { } public ValidatorIndexCache() { - validatorIndices = LRUCache.create(Integer.MAX_VALUE - 1); + this.validatorIndices = LRUCache.create(Integer.MAX_VALUE - 1); + this.lastCachedIndex = new AtomicInteger(INDEX_NONE); latestFinalizedIndex = new AtomicInteger(INDEX_NONE); - lastCachedIndex = new AtomicInteger(INDEX_NONE); } public Optional getValidatorIndex( final BeaconState state, final BLSPublicKey publicKey) { - final SszList validators = state.getValidators(); final Optional validatorIndex = validatorIndices.getCached(publicKey); if (validatorIndex.isPresent()) { - return validatorIndex.filter(index -> index < validators.size()); + return validatorIndex.filter(index -> index < state.getValidators().size()); } - // Using the same latestFinalizedIndex when scanning through - // the finalized and the non-finalized states ensures consistency - final int latestFinalizedIndexSnapshot = latestFinalizedIndex.get(); - return findIndexFromFinalizedState(validators, publicKey, latestFinalizedIndexSnapshot) - .or( - () -> - findIndexFromNonFinalizedState( - validators, publicKey, latestFinalizedIndexSnapshot)); + + return findIndexFromState(state.getValidators(), publicKey); } - private Optional findIndexFromFinalizedState( - final SszList validators, - final BLSPublicKey publicKey, - final int latestFinalizedIndex) { - for (int i = lastCachedIndex.get() + 1; - i <= Math.min(latestFinalizedIndex, validators.size() - 1); - i++) { - final BLSPublicKey pubKey = validators.get(i).getPublicKey(); - // cache finalized mapping + private Optional findIndexFromState( + final SszList validatorList, final BLSPublicKey publicKey) { + for (int i = Math.max(lastCachedIndex.get() + 1, 0); i < validatorList.size(); i++) { + BLSPublicKey pubKey = validatorList.get(i).getPublicKey(); + // from electra, only finalized validator indices are stable, + // so we look to store only finalized indices not already stored. validatorIndices.invalidateWithNewValue(pubKey, i); - updateLastCachedIndex(i); + updateLastIndex(i); if (pubKey.equals(publicKey)) { return Optional.of(i); } @@ -86,39 +77,21 @@ private Optional findIndexFromFinalizedState( return Optional.empty(); } - private void updateLastCachedIndex(final int updatedIndex) { - lastCachedIndex.updateAndGet(curr -> Math.max(curr, updatedIndex)); - } - - private Optional findIndexFromNonFinalizedState( - final SszList validators, - final BLSPublicKey publicKey, - final int latestFinalizedIndex) { - for (int i = latestFinalizedIndex + 1; i < validators.size(); i++) { - final BLSPublicKey pubKey = validators.get(i).getPublicKey(); - if (pubKey.equals(publicKey)) { - return Optional.of(i); - } - } - return Optional.empty(); - } - - public void updateLatestFinalizedIndex(final BeaconState finalizedState) { - latestFinalizedIndex.updateAndGet( - curr -> Math.max(curr, finalizedState.getValidators().size() - 1)); + private void updateLastIndex(final int i) { + lastCachedIndex.updateAndGet(curr -> Math.max(curr, i)); } public void invalidateWithNewValue(final BLSPublicKey pubKey, final int updatedIndex) { - if (updatedIndex > latestFinalizedIndex.get()) { - // do not cache if index is not finalized - return; + if (latestFinalizedIndex.get() >= updatedIndex) { + validatorIndices.invalidateWithNewValue(pubKey, updatedIndex); + } else { + LOG.trace("Ignoring invalidateWithNewValue {}:{}", pubKey, updatedIndex); } - validatorIndices.invalidateWithNewValue(pubKey, updatedIndex); } @VisibleForTesting - public int getLatestFinalizedIndex() { - return latestFinalizedIndex.get(); + int getLastCachedIndex() { + return lastCachedIndex.get(); } @VisibleForTesting @@ -126,8 +99,13 @@ Cache getValidatorIndices() { return validatorIndices; } - @VisibleForTesting - int getLastCachedIndex() { - return lastCachedIndex.get(); + public void updateLatestFinalizedIndex(final BeaconState finalizedState) { + LOG.trace("Update finalized index: {}", finalizedState.getSlot()); + latestFinalizedIndex.updateAndGet( + curr -> Math.max(curr, finalizedState.getValidators().size() - 1)); + } + + public int getLatestFinalizedIndex() { + return latestFinalizedIndex.get(); } } diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/electra/block/BlockProcessorElectra.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/electra/block/BlockProcessorElectra.java index 03dbd3b39e4..89f005b8f4d 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/electra/block/BlockProcessorElectra.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/electra/block/BlockProcessorElectra.java @@ -522,7 +522,17 @@ protected void addValidatorToRegistry( final Validator validator = getValidatorFromDeposit(pubkey, withdrawalCredentials); LOG.debug("Adding new validator with index {} to state", state.getValidators().size()); state.getValidators().append(validator); - final int validatorIndex = validatorsUtil.getValidatorIndex(state, pubkey).orElseThrow(); + int validatorIndex = -1; + for (int i = state.getValidators().size() - 1; i >= 0; i--) { + if (state.getValidators().get(i).getPublicKey().equals(pubkey)) { + validatorIndex = i; + break; + } + } + if (validatorIndex < 0) { + throw new IllegalStateException( + "Could not locate validator " + pubkey + " after adding to state."); + } final MutableBeaconStateElectra stateElectra = MutableBeaconStateElectra.required(state); stateElectra.getBalances().appendElement(UInt64.ZERO); diff --git a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/datastructures/state/beaconstate/common/ValidatorIndexCacheTest.java b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/datastructures/state/beaconstate/common/ValidatorIndexCacheTest.java index 3e09ce9b3c8..dcc5a8ca151 100644 --- a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/datastructures/state/beaconstate/common/ValidatorIndexCacheTest.java +++ b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/datastructures/state/beaconstate/common/ValidatorIndexCacheTest.java @@ -16,7 +16,6 @@ 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.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -98,25 +97,6 @@ public void shouldStartScanningFinalizedStateFromLastCachedIndex() { assertThat(validatorIndexCache.getLastCachedIndex()).isEqualTo(latestFinalizedIndex); } - @Test - public void shouldScanNonFinalizedStateButDoNotCacheIfIndexNotFoundInFinalizedState() { - final SszList validators = state.getValidators(); - final int latestFinalizedIndex = 31; - final ValidatorIndexCache validatorIndexCache = - new ValidatorIndexCache(cache, latestFinalizedIndex, latestFinalizedIndex); - - when(cache.getCached(any())).thenReturn(Optional.empty()); - - final Optional index = - validatorIndexCache.getValidatorIndex( - state, validators.get(NUMBER_OF_VALIDATORS - 1).getPublicKey()); - - verify(cache, never()).invalidateWithNewValue(any(), any()); - assertThat(index).hasValue(NUMBER_OF_VALIDATORS - 1); - - assertThat(validatorIndexCache.getLastCachedIndex()).isEqualTo(latestFinalizedIndex); - } - @Test public void shouldReturnEmptyIfPubkeyNotFoundInState() { final ValidatorIndexCache validatorIndexCache = new ValidatorIndexCache();