Retry blocks when parent becomes known while importing (#2668)

sync/src/main/java/tech/pegasys/teku/sync/BlockManager.java

@@ -103,15 +103,15 @@ 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<SignedBeaconBlock> children = pendingBlocks.getItemsDependingOn(blockRoot, false);
     children.forEach(pendingBlocks::remove);
     children.forEach(this::importBlock);
   }
 
   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<SignedBeaconBlock> blocksToDrop = new HashSet<>();
     blocksToDrop.add(block);
     blocksToDrop.addAll(pendingBlocks.getItemsDependingOn(blockRoot, true));

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