Deposit tree snapshots should only be queried/loaded once (Consensys#…

…8113)

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)

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<LoadDepositSnapshotResult> loadDepositSnapshot() {
     if (!depositSnapshotStorageEnabled) {
       return SafeFuture.completedFuture(LoadDepositSnapshotResult.EMPTY);
     }
+
+    STATUS_LOG.loadingDepositSnapshotFromDb();
     return historicalChainData
         .getFinalizedDepositSnapshot()
         .thenApply(LoadDepositSnapshotResult::create);

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<UInt64> 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<UInt64> 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<LoadDepositSnapshotResult> 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() {

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,31 +385,63 @@ 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));
     when(depositSnapshotFileLoader.loadDepositSnapshot())
         .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(
                 LoadDepositSnapshotResult.create(Optional.of(depositTreeSnapshotFromDb))));
 
     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();
   }

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);

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())