Snapsync clear correctly heal pending task before restart (hyperledge…

…r#3920) Signed-off-by: Karim TAAM <[email protected]>
lacchain · Nov 3, 2023 · baac329 · baac329
1 parent 07466fc
commit baac329
Show file tree

Hide file tree

Showing 7 changed files with 46 additions and 24 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 ### Additions and Improvements
 ### Bug Fixes
+- Fixed a snapsync issue that can sometimes block the healing step [#3920](https://github.com/hyperledger/besu/pull/3920)
 
 ## 22.4.3
 

diff --git a/...c/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/DynamicPivotBlockManager.java b/...c/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/DynamicPivotBlockManager.java
@@ -93,9 +93,9 @@ public void switchToNewPivotBlock(final BiConsumer<BlockHeader, Boolean> onSwitc
           LOG.info(
               "Select new pivot block {} {}", blockHeader.getNumber(), blockHeader.getStateRoot());
           syncState.setCurrentHeader(blockHeader);
+          lastPivotBlockFound = Optional.empty();
           onSwitchDone.accept(blockHeader, true);
         },
         () -> onSwitchDone.accept(syncState.getPivotBlockHeader().orElseThrow(), false));
-    lastPivotBlockFound = Optional.empty();
   }
 }
diff --git a/...um/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/PersistDataStep.java b/...um/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/PersistDataStep.java
@@ -49,9 +49,8 @@ public List<Task<SnapDataRequest>> persist(final List<Task<SnapDataRequest>> tas
         } else {
           if (!task.getData().isExpired(snapSyncState)) {
             enqueueChildren(childRequests);
-          } else if (childRequests.findAny().isPresent()) {
-            // not saved because it's expired and missing child requests
-            return tasks;
+          } else {
+            continue;
           }
         }
 

diff --git a/...src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/SnapWorldDownloadState.java b/...src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/SnapWorldDownloadState.java
@@ -164,17 +164,16 @@ public synchronized void startHeal() {
     snapSyncState.setHealStatus(true);
     // try to find new pivot block before healing
     dynamicPivotBlockManager.switchToNewPivotBlock(
-        (blockHeader, newPivotBlockFound) -> {
-          enqueueRequest(
-              createAccountTrieNodeDataRequest(
-                  blockHeader.getStateRoot(), Bytes.EMPTY, inconsistentAccounts));
-        });
+        (blockHeader, newPivotBlockFound) ->
+            enqueueRequest(
+                createAccountTrieNodeDataRequest(
+                    blockHeader.getStateRoot(), Bytes.EMPTY, inconsistentAccounts)));
   }
 
   public synchronized void reloadHeal() {
     worldStateStorage.clearFlatDatabase();
-    pendingTrieNodeRequests.clearInternalQueues();
-    pendingCodeRequests.clearInternalQueue();
+    pendingTrieNodeRequests.clear();
+    pendingCodeRequests.clear();
     snapSyncState.setHealStatus(false);
     checkCompletion(snapSyncState.getPivotBlockHeader().orElseThrow());
   }

diff --git a/...ain/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/request/TrieNodeDataRequest.java b/...ain/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/request/TrieNodeDataRequest.java
@@ -40,7 +40,6 @@ public abstract class TrieNodeDataRequest extends SnapDataRequest implements Tas
   private final Bytes location;
   protected Bytes data;
 
-  protected boolean isInCache = false;
   protected boolean requiresPersisting = true;
 
   protected TrieNodeDataRequest(final Hash nodeHash, final Hash rootHash, final Bytes location) {
@@ -121,18 +120,10 @@ public boolean isExpired(final SnapSyncState snapSyncState) {
     return snapSyncState.isExpired(this);
   }
 
-  public boolean isInCache() {
-    return isInCache;
-  }
-
   public boolean isRequiresPersisting() {
     return requiresPersisting;
   }
 
-  public void setInCache(final boolean inCache) {
-    isInCache = inCache;
-  }
-
   public Bytes32 getNodeHash() {
     return nodeHash;
   }

diff --git a/...st/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/DynamicPivotBlockManagerTest.java b/...st/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/DynamicPivotBlockManagerTest.java
@@ -119,4 +119,32 @@ public void shouldSwitchToNewPivotBlockWhenNeeded() {
     verify(snapSyncState).setCurrentHeader(pivotBlockHeader);
     verify(fastSyncActions).waitForSuitablePeers(any());
   }
+
+  @Test
+  public void shouldSwitchToNewPivotOnlyOnce() {
+
+    final CompletableFuture<FastSyncState> COMPLETE =
+        completedFuture(FastSyncState.EMPTY_SYNC_STATE);
+    final FastSyncState selectPivotBlockState = new FastSyncState(1060);
+    final BlockHeader pivotBlockHeader = new BlockHeaderTestFixture().number(1060).buildHeader();
+    final FastSyncState downloadPivotBlockHeaderState = new FastSyncState(pivotBlockHeader);
+    when(fastSyncActions.waitForSuitablePeers(FastSyncState.EMPTY_SYNC_STATE)).thenReturn(COMPLETE);
+    when(fastSyncActions.selectPivotBlock(FastSyncState.EMPTY_SYNC_STATE))
+        .thenReturn(completedFuture(selectPivotBlockState));
+    when(fastSyncActions.downloadPivotBlockHeader(selectPivotBlockState))
+        .thenReturn(completedFuture(downloadPivotBlockHeaderState));
+
+    when(syncState.bestChainHeight()).thenReturn(1066L);
+
+    dynamicPivotBlockManager.check(
+        (blockHeader, newBlockFound) -> {
+          assertThat(blockHeader.getNumber()).isEqualTo(pivotBlockHeader.getNumber());
+          assertThat(newBlockFound).isTrue();
+          dynamicPivotBlockManager.check(
+              (blockHeader1, aBoolean) -> {
+                assertThat(blockHeader1.getNumber()).isEqualTo(939);
+                assertThat(aBoolean).isFalse();
+              });
+        });
+  }
 }
diff --git a/...test/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/SnapWorldDownloadStateTest.java b/...test/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/SnapWorldDownloadStateTest.java
@@ -18,7 +18,6 @@
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
@@ -28,6 +27,7 @@
 import org.hyperledger.besu.ethereum.core.BlockHeaderTestFixture;
 import org.hyperledger.besu.ethereum.core.InMemoryKeyValueStorageProvider;
 import org.hyperledger.besu.ethereum.eth.manager.task.EthTask;
+import org.hyperledger.besu.ethereum.eth.sync.snapsync.request.BytecodeRequest;
 import org.hyperledger.besu.ethereum.eth.sync.snapsync.request.SnapDataRequest;
 import org.hyperledger.besu.ethereum.eth.sync.worldstate.WorldStateDownloadProcess;
 import org.hyperledger.besu.ethereum.storage.keyvalue.WorldStateKeyValueStorage;
@@ -250,11 +250,15 @@ public void shouldRestartHealWhenNewPivotBlock() {
     downloadState.checkCompletion(header);
     verify(snapSyncState).setHealStatus(true);
     assertThat(downloadState.pendingTrieNodeRequests.isEmpty()).isFalse();
+    // add useless requests
+    downloadState.pendingTrieNodeRequests.add(
+        BytecodeRequest.createAccountTrieNodeDataRequest(Hash.EMPTY, Bytes.EMPTY, new HashSet<>()));
+    downloadState.pendingCodeRequests.add(
+        BytecodeRequest.createBytecodeRequest(Bytes32.ZERO, Hash.EMPTY, Bytes32.ZERO));
     // reload the heal
     downloadState.reloadHeal();
     verify(snapSyncState).setHealStatus(false);
-    spy(downloadState.pendingTrieNodeRequests).clearInternalQueues();
-    spy(downloadState).checkCompletion(header);
-    assertThat(downloadState.pendingTrieNodeRequests.isEmpty()).isFalse();
+    assertThat(downloadState.pendingTrieNodeRequests.size()).isEqualTo(1);
+    assertThat(downloadState.pendingCodeRequests.isEmpty()).isTrue();
   }
 }