From 5ee9be837f31c8abb3b41c0e7bc3060262e3a1c8 Mon Sep 17 00:00:00 2001 From: matkt Date: Wed, 22 Jun 2022 16:48:54 +0200 Subject: [PATCH] Snapsync clear correctly heal pending task before restart (#3920) Signed-off-by: Karim TAAM --- CHANGELOG.md | 1 + .../snapsync/DynamicPivotBlockManager.java | 2 +- .../eth/sync/snapsync/PersistDataStep.java | 5 ++-- .../sync/snapsync/SnapWorldDownloadState.java | 13 ++++----- .../snapsync/request/TrieNodeDataRequest.java | 9 ------ .../DynamicPivotBlockManagerTest.java | 28 +++++++++++++++++++ .../snapsync/SnapWorldDownloadStateTest.java | 12 +++++--- 7 files changed, 46 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29214fe652..5df2c250ce 100644 --- 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/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/DynamicPivotBlockManager.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/DynamicPivotBlockManager.java index 3de19d9532..0c11573bd9 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/DynamicPivotBlockManager.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/DynamicPivotBlockManager.java @@ -93,9 +93,9 @@ public class DynamicPivotBlockManager { 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/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/PersistDataStep.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/PersistDataStep.java index 9bd15739cb..28594cbd97 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/PersistDataStep.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/PersistDataStep.java @@ -49,9 +49,8 @@ public class PersistDataStep { } 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/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/SnapWorldDownloadState.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/SnapWorldDownloadState.java index 76a1bb1daa..5d73e655f6 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/SnapWorldDownloadState.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/SnapWorldDownloadState.java @@ -164,17 +164,16 @@ public class SnapWorldDownloadState extends WorldDownloadState 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/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/request/TrieNodeDataRequest.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/request/TrieNodeDataRequest.java index 649be9b32f..f82789fd2f 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/request/TrieNodeDataRequest.java +++ b/ethereum/eth/src/main/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 abstract class TrieNodeDataRequest extends SnapDataRequest implements Tas 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/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/DynamicPivotBlockManagerTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/DynamicPivotBlockManagerTest.java index 3d3591cf2c..46306555ec 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/DynamicPivotBlockManagerTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/DynamicPivotBlockManagerTest.java @@ -119,4 +119,32 @@ public class DynamicPivotBlockManagerTest { verify(snapSyncState).setCurrentHeader(pivotBlockHeader); verify(fastSyncActions).waitForSuitablePeers(any()); } + + @Test + public void shouldSwitchToNewPivotOnlyOnce() { + + final CompletableFuture 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/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/SnapWorldDownloadStateTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/SnapWorldDownloadStateTest.java index efc2cb1b84..ec819fd2fc 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/SnapWorldDownloadStateTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/SnapWorldDownloadStateTest.java @@ -18,7 +18,6 @@ import static org.assertj.core.api.Assertions.assertThat; 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.BlockHeader; 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 class SnapWorldDownloadStateTest { 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(); } }