From 1160bb302dcb3d870c5a12324a86786561425d89 Mon Sep 17 00:00:00 2001 From: Courtney <45641759+courtneyeh@users.noreply.github.com> Date: Wed, 27 Mar 2024 08:34:17 +1100 Subject: [PATCH] Deposit tree snapshots should only be queried/loaded once (#8113) --- CHANGELOG.md | 1 + .../pow/DepositSnapshotStorageLoader.java | 4 + .../teku/beacon/pow/Eth1DepositManager.java | 51 ++++++------ .../beacon/pow/Eth1DepositManagerTest.java | 78 ++++++++++++------- .../services/powchain/PowchainService.java | 4 +- .../powchain/PowchainServiceTest.java | 32 ++++++++ 6 files changed, 115 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6cfc17e9e95..f158759ab98 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ the [releases page](https://github.com/Consensys/teku/releases). - Increased the attestation cache capacity to allow Teku a bigger pool of attestations when block building. - Defaulted `--builder-bid-compare-factor` to 90. This makes it necessary for external block builders to give at least 10% additional profit compared to a local build before being taken into consideration. If you would like to go back to the previous default, set `--builder-bid-compare-factor` to 100. - Added `--p2p-direct-peers` command line option to configure explicit peers as per [Explicit Peering Agreements](https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/gossipsub-v1.1.md#explicit-peering-agreements) libp2p spec. +- Deposit tree snapshots will be loaded from database as a default unless custom snapshot has been provided. ### Bug Fixes - Fix incompatibility between Teku validator client and Lighthouse beacon nodes [#8117](https://github.com/Consensys/teku/pull/8117) diff --git a/beacon/pow/src/main/java/tech/pegasys/teku/beacon/pow/DepositSnapshotStorageLoader.java b/beacon/pow/src/main/java/tech/pegasys/teku/beacon/pow/DepositSnapshotStorageLoader.java index 5feb8995081..e56c8a2d138 100644 --- a/beacon/pow/src/main/java/tech/pegasys/teku/beacon/pow/DepositSnapshotStorageLoader.java +++ b/beacon/pow/src/main/java/tech/pegasys/teku/beacon/pow/DepositSnapshotStorageLoader.java @@ -13,6 +13,8 @@ package tech.pegasys.teku.beacon.pow; +import static tech.pegasys.teku.infrastructure.logging.StatusLogger.STATUS_LOG; + import tech.pegasys.teku.ethereum.pow.api.schema.LoadDepositSnapshotResult; import tech.pegasys.teku.infrastructure.async.SafeFuture; import tech.pegasys.teku.storage.api.StorageQueryChannel; @@ -31,6 +33,8 @@ public SafeFuture loadDepositSnapshot() { if (!depositSnapshotStorageEnabled) { return SafeFuture.completedFuture(LoadDepositSnapshotResult.EMPTY); } + + STATUS_LOG.loadingDepositSnapshotFromDb(); return historicalChainData .getFinalizedDepositSnapshot() .thenApply(LoadDepositSnapshotResult::create); diff --git a/beacon/pow/src/main/java/tech/pegasys/teku/beacon/pow/Eth1DepositManager.java b/beacon/pow/src/main/java/tech/pegasys/teku/beacon/pow/Eth1DepositManager.java index 4737276b0c0..58baac92aab 100644 --- a/beacon/pow/src/main/java/tech/pegasys/teku/beacon/pow/Eth1DepositManager.java +++ b/beacon/pow/src/main/java/tech/pegasys/teku/beacon/pow/Eth1DepositManager.java @@ -21,7 +21,6 @@ import com.google.common.base.Preconditions; import java.math.BigInteger; import java.util.Optional; -import org.apache.commons.lang3.ObjectUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.web3j.protocol.core.methods.response.EthBlock; @@ -47,7 +46,7 @@ public class Eth1DepositManager { private final Eth1DepositStorageChannel eth1DepositStorageChannel; private final DepositSnapshotFileLoader depositSnapshotFileLoader; private final DepositSnapshotStorageLoader depositSnapshotStorageLoader; - private final boolean takeBestDepositSnapshot; + private final boolean customDepositSnapshotPathPresent; private final DepositProcessingController depositProcessingController; private final MinimumGenesisTimeBlockFinder minimumGenesisTimeBlockFinder; private final Optional depositContractDeployBlock; @@ -61,7 +60,7 @@ public Eth1DepositManager( final Eth1DepositStorageChannel eth1DepositStorageChannel, final DepositSnapshotFileLoader depositSnapshotFileLoader, final DepositSnapshotStorageLoader depositSnapshotStorageLoader, - final boolean takeBestDepositSnapshot, + final boolean customDepositSnapshotPathPresent, final DepositProcessingController depositProcessingController, final MinimumGenesisTimeBlockFinder minimumGenesisTimeBlockFinder, final Optional depositContractDeployBlock, @@ -73,7 +72,7 @@ public Eth1DepositManager( this.eth1DepositStorageChannel = eth1DepositStorageChannel; this.depositSnapshotFileLoader = depositSnapshotFileLoader; this.depositSnapshotStorageLoader = depositSnapshotStorageLoader; - this.takeBestDepositSnapshot = takeBestDepositSnapshot; + this.customDepositSnapshotPathPresent = customDepositSnapshotPathPresent; this.depositProcessingController = depositProcessingController; this.minimumGenesisTimeBlockFinder = minimumGenesisTimeBlockFinder; this.depositContractDeployBlock = depositContractDeployBlock; @@ -118,31 +117,29 @@ public void start() { } private SafeFuture loadDepositSnapshot() { - final LoadDepositSnapshotResult fileDepositSnapshotResult = - depositSnapshotFileLoader.loadDepositSnapshot(); - if (fileDepositSnapshotResult.getDepositTreeSnapshot().isPresent() - && !takeBestDepositSnapshot) { - LOG.debug("Deposit snapshot from file is provided, using it"); - return SafeFuture.completedFuture(fileDepositSnapshotResult); + if (customDepositSnapshotPathPresent) { + LOG.debug("Custom deposit snapshot is provided, using it"); + return SafeFuture.completedFuture(depositSnapshotFileLoader.loadDepositSnapshot()); } - if (takeBestDepositSnapshot) { - STATUS_LOG.loadingDepositSnapshotFromDb(); - return depositSnapshotStorageLoader - .loadDepositSnapshot() - .thenApply( - storageDepositSnapshotResult -> { - if (storageDepositSnapshotResult.getDepositTreeSnapshot().isPresent()) { - final DepositTreeSnapshot storageSnapshot = - storageDepositSnapshotResult.getDepositTreeSnapshot().get(); - STATUS_LOG.onDepositSnapshot( - storageSnapshot.getDepositCount(), storageSnapshot.getExecutionBlockHash()); - } - return ObjectUtils.max(storageDepositSnapshotResult, fileDepositSnapshotResult); - }); - } else { - return depositSnapshotStorageLoader.loadDepositSnapshot(); - } + return depositSnapshotStorageLoader + .loadDepositSnapshot() + .thenApply( + storageDepositSnapshotResult -> { + if (storageDepositSnapshotResult.getDepositTreeSnapshot().isPresent()) { + final DepositTreeSnapshot storageSnapshot = + storageDepositSnapshotResult.getDepositTreeSnapshot().get(); + STATUS_LOG.onDepositSnapshot( + storageSnapshot.getDepositCount(), storageSnapshot.getExecutionBlockHash()); + return storageDepositSnapshotResult; + } + + final LoadDepositSnapshotResult fileDepositSnapshotResult = + depositSnapshotFileLoader.loadDepositSnapshot(); + LOG.debug( + "Database deposit snapshot is not present, falling back to the file provided"); + return fileDepositSnapshotResult; + }); } public void stop() { diff --git a/beacon/pow/src/test/java/tech/pegasys/teku/beacon/pow/Eth1DepositManagerTest.java b/beacon/pow/src/test/java/tech/pegasys/teku/beacon/pow/Eth1DepositManagerTest.java index 942c282e099..51665e1ea3b 100644 --- a/beacon/pow/src/test/java/tech/pegasys/teku/beacon/pow/Eth1DepositManagerTest.java +++ b/beacon/pow/src/test/java/tech/pegasys/teku/beacon/pow/Eth1DepositManagerTest.java @@ -362,6 +362,8 @@ void shouldStartWithStoredDepositsChainReorgedToBeShorter() { final BigInteger headBlockNumber = BigInteger.valueOf(60); final BigInteger lastReplayedBlock = BigInteger.valueOf(70); final BigInteger lastReplayedDepositIndex = BigInteger.valueOf(71); + + // No deposit tree snapshot loaded, this will be used when(eth1DepositStorageChannel.replayDepositEvents()) .thenReturn( SafeFuture.completedFuture( @@ -383,11 +385,11 @@ void shouldStartWithStoredDepositsChainReorgedToBeShorter() { } @Test - void shouldStartFromSnapshotFileWhenProvided() { + void shouldIgnoreSnapshotFileWhenDBSavedVersionProvided() { final UInt64 deposits = UInt64.valueOf(100); final BigInteger lastBlockNumber = BigInteger.valueOf(1000); - // DepositTreeSnapshot from file will be used to start from + // This one will be ignored as DB already has a saved version final DepositTreeSnapshot depositTreeSnapshotFromFile = dataStructureUtil.randomDepositTreeSnapshot( deposits.longValue(), UInt64.valueOf(lastBlockNumber)); @@ -395,10 +397,11 @@ void shouldStartFromSnapshotFileWhenProvided() { .thenReturn(LoadDepositSnapshotResult.create(Optional.of(depositTreeSnapshotFromFile))); when(depositProcessingController.fetchDepositsInRange(any(), any())).thenReturn(COMPLETE); - // This one will be ignored + // DepositTreeSnapshot from DB will be used to start from + final UInt64 dbDepositCount = deposits.plus(50); + final UInt64 dbBlockHeight = UInt64.valueOf(lastBlockNumber).plus(500); final DepositTreeSnapshot depositTreeSnapshotFromDb = - dataStructureUtil.randomDepositTreeSnapshot( - deposits.plus(50).longValue(), UInt64.valueOf(lastBlockNumber).plus(500)); + dataStructureUtil.randomDepositTreeSnapshot(dbDepositCount.longValue(), dbBlockHeight); when(depositSnapshotStorageLoader.loadDepositSnapshot()) .thenReturn( SafeFuture.completedFuture( @@ -406,8 +409,39 @@ void shouldStartFromSnapshotFileWhenProvided() { manager.start(); notifyHeadBlock(lastBlockNumber, MIN_GENESIS_BLOCK_TIMESTAMP + 1000); + verify(depositSnapshotFileLoader, never()).loadDepositSnapshot(); verify(eth1DepositStorageChannel, never()).replayDepositEvents(); - verify(eth1EventsChannel).setLatestPublishedDeposit(deposits.decrement()); + verify(eth1EventsChannel).setLatestPublishedDeposit(dbDepositCount.decrement()); + inOrder + .verify(depositProcessingController) + .startSubscription(dbBlockHeight.bigIntegerValue().add(BigInteger.ONE)); + inOrder.verifyNoMoreInteractions(); + assertNoUncaughtExceptions(); + } + + @Test + void shouldTakeSnapshotFileWhenDBSavedVersionNotProvided() { + final UInt64 depositsCount = UInt64.valueOf(100); + final BigInteger lastBlockNumber = BigInteger.valueOf(1000); + + // This one will be used as DB empty + final DepositTreeSnapshot depositTreeSnapshotFromFile = + dataStructureUtil.randomDepositTreeSnapshot( + depositsCount.longValue(), UInt64.valueOf(lastBlockNumber)); + when(depositSnapshotFileLoader.loadDepositSnapshot()) + .thenReturn(LoadDepositSnapshotResult.create(Optional.of(depositTreeSnapshotFromFile))); + when(depositProcessingController.fetchDepositsInRange(any(), any())).thenReturn(COMPLETE); + + // DepositTreeSnapshot from DB empty + when(depositSnapshotStorageLoader.loadDepositSnapshot()) + .thenReturn(SafeFuture.completedFuture(LoadDepositSnapshotResult.EMPTY)); + + manager.start(); + notifyHeadBlock(lastBlockNumber, MIN_GENESIS_BLOCK_TIMESTAMP + 1000); + verify(depositSnapshotStorageLoader).loadDepositSnapshot(); + verify(depositSnapshotFileLoader).loadDepositSnapshot(); + verify(eth1DepositStorageChannel, never()).replayDepositEvents(); + verify(eth1EventsChannel).setLatestPublishedDeposit(depositsCount.decrement()); inOrder .verify(depositProcessingController) .startSubscription(lastBlockNumber.add(BigInteger.ONE)); @@ -444,8 +478,8 @@ void shouldStartFromStorageSnapshotWhenProvided() { } @Test - void shouldStartFromBestSnapshotWhenOptionEnabled() { - final Eth1DepositManager manager2 = + void shouldUseCustomDepositSnapshotPathWhenProvided() { + final Eth1DepositManager manager = new Eth1DepositManager( config, eth1Provider, @@ -460,34 +494,26 @@ void shouldStartFromBestSnapshotWhenOptionEnabled() { Optional.empty(), eth1HeadTracker); - final UInt64 deposits = UInt64.valueOf(100); - final BigInteger lastBlockNumberFile = BigInteger.valueOf(1000); - final BigInteger lastBlockNumberStorage = BigInteger.valueOf(2000); + final UInt64 depositsCount = UInt64.valueOf(100); + final BigInteger lastBlockNumber = BigInteger.valueOf(1000); - // DepositTreeSnapshot from file will be loaded + // Custom deposit snapshot path provided, so will be used final DepositTreeSnapshot depositTreeSnapshotFromFile = dataStructureUtil.randomDepositTreeSnapshot( - deposits.longValue(), UInt64.valueOf(lastBlockNumberFile)); + depositsCount.longValue(), UInt64.valueOf(lastBlockNumber)); when(depositSnapshotFileLoader.loadDepositSnapshot()) .thenReturn(LoadDepositSnapshotResult.create(Optional.of(depositTreeSnapshotFromFile))); when(depositProcessingController.fetchDepositsInRange(any(), any())).thenReturn(COMPLETE); - // Storage snapshot will be loaded too and compared to the one from file - final DepositTreeSnapshot depositTreeSnapshotFromDb = - dataStructureUtil.randomDepositTreeSnapshot( - deposits.longValue(), UInt64.valueOf(lastBlockNumberStorage)); - when(depositSnapshotStorageLoader.loadDepositSnapshot()) - .thenReturn( - SafeFuture.completedFuture( - LoadDepositSnapshotResult.create(Optional.of(depositTreeSnapshotFromDb)))); - - manager2.start(); - notifyHeadBlock(BigInteger.valueOf(3000), MIN_GENESIS_BLOCK_TIMESTAMP + 1000); + manager.start(); + notifyHeadBlock(lastBlockNumber, MIN_GENESIS_BLOCK_TIMESTAMP + 1000); + verify(depositSnapshotStorageLoader, never()).loadDepositSnapshot(); + verify(depositSnapshotFileLoader).loadDepositSnapshot(); verify(eth1DepositStorageChannel, never()).replayDepositEvents(); - verify(eth1EventsChannel).setLatestPublishedDeposit(deposits.decrement()); + verify(eth1EventsChannel).setLatestPublishedDeposit(depositsCount.decrement()); inOrder .verify(depositProcessingController) - .startSubscription(lastBlockNumberStorage.add(BigInteger.ONE)); + .startSubscription(lastBlockNumber.add(BigInteger.ONE)); inOrder.verifyNoMoreInteractions(); assertNoUncaughtExceptions(); } diff --git a/services/powchain/src/main/java/tech/pegasys/teku/services/powchain/PowchainService.java b/services/powchain/src/main/java/tech/pegasys/teku/services/powchain/PowchainService.java index 3db8fdebc5e..6728a514b8a 100644 --- a/services/powchain/src/main/java/tech/pegasys/teku/services/powchain/PowchainService.java +++ b/services/powchain/src/main/java/tech/pegasys/teku/services/powchain/PowchainService.java @@ -188,7 +188,7 @@ public PowchainService( eth1DepositStorageChannel, depositSnapshotFileLoader, depositSnapshotStorageLoader, - depositTreeSnapshotConfiguration.isBundledDepositSnapshotEnabled(), + depositTreeSnapshotConfiguration.getCustomDepositSnapshotPath().isPresent(), depositProcessingController, new MinimumGenesisTimeBlockFinder(config, eth1Provider, eth1DepositContractDeployBlock), eth1DepositContractDeployBlock, @@ -203,7 +203,7 @@ private DepositSnapshotFileLoader createDepositSnapshotFileLoader( if (depositTreeSnapshotConfiguration.getCustomDepositSnapshotPath().isPresent()) { depositSnapshotFileLoaderBuilder.addRequiredResource( depositTreeSnapshotConfiguration.getCustomDepositSnapshotPath().get()); - } else { + } else if (depositTreeSnapshotConfiguration.isBundledDepositSnapshotEnabled()) { depositTreeSnapshotConfiguration .getCheckpointSyncDepositSnapshotUrl() .ifPresent(depositSnapshotFileLoaderBuilder::addOptionalResource); diff --git a/services/powchain/src/test/java/tech/pegasys/teku/services/powchain/PowchainServiceTest.java b/services/powchain/src/test/java/tech/pegasys/teku/services/powchain/PowchainServiceTest.java index 4137e42692a..591db64da95 100644 --- a/services/powchain/src/test/java/tech/pegasys/teku/services/powchain/PowchainServiceTest.java +++ b/services/powchain/src/test/java/tech/pegasys/teku/services/powchain/PowchainServiceTest.java @@ -125,6 +125,7 @@ public void shouldUseBundledDepositSnapshotPathWhenAvailableAndCustomPathNotPres .thenReturn(Optional.empty()); when(depositTreeSnapshotConfiguration.getBundledDepositSnapshotPath()) .thenReturn(Optional.of("/foo/bundled")); + when(depositTreeSnapshotConfiguration.isBundledDepositSnapshotEnabled()).thenReturn(true); final PowchainService powchainService = new PowchainService(serviceConfig, powConfig, Optional.of(engineWeb3jClientProvider)); @@ -133,6 +134,20 @@ public void shouldUseBundledDepositSnapshotPathWhenAvailableAndCustomPathNotPres powchainService, List.of(new DepositSnapshotResource("/foo/bundled", true))); } + @Test + public void shouldNotUseBundledDepositSnapshotPathWhenDepositSnapshotDisabled() { + when(depositTreeSnapshotConfiguration.getCustomDepositSnapshotPath()) + .thenReturn(Optional.empty()); + when(depositTreeSnapshotConfiguration.getBundledDepositSnapshotPath()) + .thenReturn(Optional.of("/foo/bundled")); + when(depositTreeSnapshotConfiguration.isBundledDepositSnapshotEnabled()).thenReturn(false); + + final PowchainService powchainService = + new PowchainService(serviceConfig, powConfig, Optional.of(engineWeb3jClientProvider)); + + verifyExpectedOrderOfDepositSnapshotPathResources(powchainService, List.of()); + } + @Test public void shouldUseCheckpointSyncAndBundledDepositSnapshotPath() { when(depositTreeSnapshotConfiguration.getCustomDepositSnapshotPath()) @@ -141,6 +156,7 @@ public void shouldUseCheckpointSyncAndBundledDepositSnapshotPath() { .thenReturn(Optional.of("/foo/checkpoint")); when(depositTreeSnapshotConfiguration.getBundledDepositSnapshotPath()) .thenReturn(Optional.of("/foo/bundled")); + when(depositTreeSnapshotConfiguration.isBundledDepositSnapshotEnabled()).thenReturn(true); final PowchainService powchainService = new PowchainService(serviceConfig, powConfig, Optional.of(engineWeb3jClientProvider)); @@ -152,6 +168,22 @@ public void shouldUseCheckpointSyncAndBundledDepositSnapshotPath() { new DepositSnapshotResource("/foo/bundled", true))); } + @Test + public void shouldNotUseCheckpointSyncAndBundledDepositSnapshotPathWhenSnapshotDisabled() { + when(depositTreeSnapshotConfiguration.getCustomDepositSnapshotPath()) + .thenReturn(Optional.empty()); + when(depositTreeSnapshotConfiguration.getCheckpointSyncDepositSnapshotUrl()) + .thenReturn(Optional.of("/foo/checkpoint")); + when(depositTreeSnapshotConfiguration.getBundledDepositSnapshotPath()) + .thenReturn(Optional.of("/foo/bundled")); + when(depositTreeSnapshotConfiguration.isBundledDepositSnapshotEnabled()).thenReturn(false); + + final PowchainService powchainService = + new PowchainService(serviceConfig, powConfig, Optional.of(engineWeb3jClientProvider)); + + verifyExpectedOrderOfDepositSnapshotPathResources(powchainService, List.of()); + } + @Test public void shouldUseCustomDepositSnapshotPathEvenWhenCheckpointSyncIsPresent() { when(depositTreeSnapshotConfiguration.getCustomDepositSnapshotPath())