Bound lookup parent chain length with tip extension (#5705)

* Bound lookup parent chain length with tip extension

* Add test

beacon_node/network/src/sync/block_lookups/mod.rs

@@ -194,7 +194,15 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
         let parent_chains = self.active_parent_lookups();
 
         for (chain_idx, parent_chain) in parent_chains.iter().enumerate() {
-            if parent_chain.ancestor() == child_block_root_trigger
+            // `block_root_to_search` will trigger a new lookup, and it will extend a parent_chain
+            // beyond its max length
+            let block_would_extend_chain = parent_chain.ancestor() == child_block_root_trigger;
+            // `block_root_to_search` already has a lookup, and with the block trigger it extends
+            // the parent_chain beyond its length. This can happen because when creating a lookup
+            // for a new root we don't do any parent chain length checks
+            let trigger_is_chain_tip = parent_chain.tip == child_block_root_trigger;
+
+            if (block_would_extend_chain || trigger_is_chain_tip)
                 && parent_chain.len() >= PARENT_DEPTH_TOLERANCE
             {
                 debug!(self.log, "Parent lookup chain too long"; "block_root" => ?block_root_to_search);
@@ -375,6 +383,14 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
                     "response_type" => ?response_type,
                 );
 
+                // Here we could check if response extends a parent chain beyond its max length.
+                // However we defer that check to the handling of a processing error ParentUnknown.
+                //
+                // Here we could check if there's already a lookup for parent_root of `response`. In
+                // that case we know that sending the response for processing will likely result in
+                // a `ParentUnknown` error. However, for simplicity we choose to not implement this
+                // optimization.
+
                 // Register the download peer here. Once we have received some data over the wire we
                 // attribute it to this peer for scoring latter regardless of how the request was
                 // done.

beacon_node/network/src/sync/block_lookups/tests.rs

@@ -278,8 +278,11 @@ impl TestRig {
         }
     }
 
-    fn failed_chains_contains(&mut self, chain_hash: &Hash256) -> bool {
-        self.sync_manager.get_failed_chains().contains(chain_hash)
+    fn assert_failed_chain(&mut self, chain_hash: Hash256) {
+        let failed_chains = self.sync_manager.get_failed_chains();
+        if !failed_chains.contains(&chain_hash) {
+            panic!("expected failed chains to contain {chain_hash:?}: {failed_chains:?}");
+        }
     }
 
     fn find_single_lookup_for(&self, block_root: Hash256) -> Id {
@@ -1201,7 +1204,7 @@ fn test_parent_lookup_too_many_download_attempts_no_blacklist() {
     // Trigger the request
     rig.trigger_unknown_parent_block(peer_id, block.into());
     for i in 1..=PARENT_FAIL_TOLERANCE {
-        assert!(!rig.failed_chains_contains(&block_root));
+        rig.assert_not_failed_chain(block_root);
         let id = rig.expect_block_parent_request(parent_root);
         if i % 2 != 0 {
             // The request fails. It should be tried again.
@@ -1214,8 +1217,8 @@ fn test_parent_lookup_too_many_download_attempts_no_blacklist() {
         }
     }
 
-    assert!(!rig.failed_chains_contains(&block_root));
-    assert!(!rig.failed_chains_contains(&parent.canonical_root()));
+    rig.assert_not_failed_chain(block_root);
+    rig.assert_not_failed_chain(parent.canonical_root());
     rig.expect_no_active_lookups_empty_network();
 }
 
@@ -1253,7 +1256,7 @@ fn test_parent_lookup_too_many_processing_attempts_must_blacklist() {
 }
 
 #[test]
-fn test_parent_lookup_too_deep() {
+fn test_parent_lookup_too_deep_grow_ancestor() {
     let mut rig = TestRig::test_setup();
     let mut blocks = rig.rand_blockchain(PARENT_DEPTH_TOLERANCE);
 
@@ -1278,7 +1281,31 @@ fn test_parent_lookup_too_deep() {
     }
 
     rig.expect_penalty(peer_id, "chain_too_long");
-    assert!(rig.failed_chains_contains(&chain_hash));
+    rig.assert_failed_chain(chain_hash);
+}
+
+#[test]
+fn test_parent_lookup_too_deep_grow_tip() {
+    let mut rig = TestRig::test_setup();
+    let blocks = rig.rand_blockchain(PARENT_DEPTH_TOLERANCE - 1);
+    let peer_id = rig.new_connected_peer();
+    let tip = blocks.last().unwrap().clone();
+
+    for block in blocks.into_iter() {
+        let block_root = block.canonical_root();
+        rig.trigger_unknown_block_from_attestation(block_root, peer_id);
+        let id = rig.expect_block_parent_request(block_root);
+        rig.single_lookup_block_response(id, peer_id, Some(block.clone()));
+        rig.single_lookup_block_response(id, peer_id, None);
+        rig.expect_block_process(ResponseType::Block);
+        rig.single_block_component_processed(
+            id.lookup_id,
+            BlockError::ParentUnknown(RpcBlock::new_without_blobs(None, block)).into(),
+        );
+    }
+
+    rig.expect_penalty(peer_id, "chain_too_long");
+    rig.assert_failed_chain(tip.canonical_root());
 }
 
 #[test]