diff --git a/sync/src/main/java/tech/pegasys/teku/sync/BlockManager.java b/sync/src/main/java/tech/pegasys/teku/sync/BlockManager.java index bd012eafc6d..557c4a95abb 100644 --- a/sync/src/main/java/tech/pegasys/teku/sync/BlockManager.java +++ b/sync/src/main/java/tech/pegasys/teku/sync/BlockManager.java @@ -103,7 +103,7 @@ public void onSlot(final UInt64 slot) { void onBlockImported(ImportedBlockEvent blockImportedEvent) { // Check if any pending blocks can now be imported final SignedBeaconBlock block = blockImportedEvent.getBlock(); - final Bytes32 blockRoot = block.getMessage().hash_tree_root(); + final Bytes32 blockRoot = block.getRoot(); pendingBlocks.remove(block); final List children = pendingBlocks.getItemsDependingOn(blockRoot, false); children.forEach(pendingBlocks::remove); @@ -111,7 +111,7 @@ void onBlockImported(ImportedBlockEvent blockImportedEvent) { } private void importBlock(final SignedBeaconBlock block) { - recentBlockFetcher.cancelRecentBlockRequest(block.getMessage().hash_tree_root()); + recentBlockFetcher.cancelRecentBlockRequest(block.getRoot()); if (!shouldImportBlock(block)) { return; } @@ -123,7 +123,16 @@ private void importBlock(final SignedBeaconBlock block) { if (result.isSuccessful()) { LOG.trace("Imported block: {}", block); } else if (result.getFailureReason() == FailureReason.UNKNOWN_PARENT) { + // Add to the pending pool so it is triggered once the parent is imported pendingBlocks.add(block); + // Check if the parent was imported while we were trying to import this block + // and if so, remove from the pendingPool again and process now + // We must add the block to the pending pool before this check happens + // to avoid race conditions between performing the check and the parent importing. + if (recentChainData.containsBlock(block.getParent_root())) { + pendingBlocks.remove(block); + importBlock(block); + } } else if (result.getFailureReason() == FailureReason.BLOCK_IS_FROM_FUTURE) { futureBlocks.add(block); } else { @@ -149,16 +158,16 @@ private boolean shouldImportBlock(final SignedBeaconBlock block) { private boolean blockIsKnown(final SignedBeaconBlock block) { return pendingBlocks.contains(block) || futureBlocks.contains(block) - || recentChainData.containsBlock(block.getMessage().hash_tree_root()); + || recentChainData.containsBlock(block.getRoot()); } private boolean blockIsInvalid(final SignedBeaconBlock block) { - return invalidBlockRoots.contains(block.getMessage().hash_tree_root()) + return invalidBlockRoots.contains(block.getRoot()) || invalidBlockRoots.contains(block.getParent_root()); } private void dropInvalidBlock(final SignedBeaconBlock block) { - final Bytes32 blockRoot = block.getMessage().hash_tree_root(); + final Bytes32 blockRoot = block.getRoot(); final Set blocksToDrop = new HashSet<>(); blocksToDrop.add(block); blocksToDrop.addAll(pendingBlocks.getItemsDependingOn(blockRoot, true)); diff --git a/sync/src/test/java/tech/pegasys/teku/sync/BlockManagerTest.java b/sync/src/test/java/tech/pegasys/teku/sync/BlockManagerTest.java index 66ee976e457..c3c1b23ce76 100644 --- a/sync/src/test/java/tech/pegasys/teku/sync/BlockManagerTest.java +++ b/sync/src/test/java/tech/pegasys/teku/sync/BlockManagerTest.java @@ -15,7 +15,10 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static tech.pegasys.teku.infrastructure.async.FutureUtil.ignoreFuture; import com.google.common.eventbus.EventBus; import java.util.ArrayList; @@ -27,6 +30,7 @@ import tech.pegasys.teku.bls.BLSKeyGenerator; import tech.pegasys.teku.bls.BLSKeyPair; import tech.pegasys.teku.core.StateTransition; +import tech.pegasys.teku.core.results.BlockImportResult; import tech.pegasys.teku.datastructures.blocks.SignedBeaconBlock; import tech.pegasys.teku.datastructures.util.DataStructureUtil; import tech.pegasys.teku.infrastructure.async.SafeFuture; @@ -126,6 +130,46 @@ public void onGossipedBlock_unattachedBlock() throws Exception { assertThat(pendingBlocks.contains(nextNextBlock)).isTrue(); } + @Test + public void onGossipedBlock_retryIfParentWasUnknownButIsNowAvailable() throws Exception { + final BlockImporter blockImporter = mock(BlockImporter.class); + final RecentChainData localRecentChainData = mock(RecentChainData.class); + final BlockManager blockManager = + new BlockManager( + localEventBus, + localRecentChainData, + blockImporter, + pendingBlocks, + futureBlocks, + recentBlockFetcher); + assertThat(blockManager.start()).isCompleted(); + + final UInt64 nextSlot = genesisSlot.plus(UInt64.ONE); + final UInt64 nextNextSlot = nextSlot.plus(UInt64.ONE); + // Create 2 blocks + remoteChain.createAndImportBlockAtSlot(nextSlot); + final SignedBeaconBlock nextNextBlock = remoteChain.createAndImportBlockAtSlot(nextNextSlot); + + final SafeFuture blockImportResult = new SafeFuture<>(); + when(blockImporter.importBlock(nextNextBlock)) + .thenReturn(blockImportResult) + .thenReturn(new SafeFuture<>()); + + incrementSlot(); + incrementSlot(); + blockManager.onGossipedBlock(new GossipedBlockEvent(nextNextBlock)); + ignoreFuture(verify(blockImporter).importBlock(nextNextBlock)); + + // Before nextNextBlock imports, it's parent becomes available + when(localRecentChainData.containsBlock(nextNextBlock.getParent_root())).thenReturn(true); + + // So when the block import completes, it should be retried + blockImportResult.complete(BlockImportResult.FAILED_UNKNOWN_PARENT); + ignoreFuture(verify(blockImporter, times(2)).importBlock(nextNextBlock)); + + assertThat(pendingBlocks.contains(nextNextBlock)).isFalse(); + } + @Test public void onGossipedBlock_futureBlock() throws Exception { final UInt64 nextSlot = genesisSlot.plus(UInt64.ONE);