diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index 1b3491b9c73..3b7c43df191 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -182,7 +182,15 @@ impl BlockLookups { 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); @@ -364,6 +372,14 @@ impl BlockLookups { "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. diff --git a/beacon_node/network/src/sync/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index 5a85e57f632..fb49d43edf8 100644 --- a/beacon_node/network/src/sync/block_lookups/tests.rs +++ b/beacon_node/network/src/sync/block_lookups/tests.rs @@ -263,8 +263,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 { @@ -1186,7 +1189,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. @@ -1199,8 +1202,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(); } @@ -1238,7 +1241,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); @@ -1263,7 +1266,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]