From f37ffe4b8d88ad688fb6aff5ca2b03c13a7ee523 Mon Sep 17 00:00:00 2001 From: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 13 May 2024 18:13:32 +0300 Subject: [PATCH] Do not request current child lookup peers (#5724) * Do not request current child lookup peers * Update tests --- .../network/src/sync/block_lookups/mod.rs | 5 +- .../sync/block_lookups/single_block_lookup.rs | 13 +- .../network/src/sync/block_lookups/tests.rs | 467 +++++++----------- 3 files changed, 197 insertions(+), 288 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index dd823a307b3..d2a0066c840 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -134,7 +134,10 @@ impl BlockLookups { block_root, Some(block_component), Some(parent_root), - &[peer_id], + // On a `UnknownParentBlock` or `UnknownParentBlob` event the peer is not required + // to have the rest of the block components (refer to decoupled blob gossip). Create + // the lookup with zero peers to house the block components. + &[], cx, ); } diff --git a/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs b/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs index 6ee519b0dd1..ec3256ce584 100644 --- a/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs +++ b/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs @@ -174,10 +174,15 @@ impl SingleBlockLookup { return Err(LookupRequestError::TooManyAttempts { cannot_process }); } - let peer_id = request - .get_state_mut() - .use_rand_available_peer() - .ok_or(LookupRequestError::NoPeers)?; + let Some(peer_id) = request.get_state_mut().use_rand_available_peer() else { + if awaiting_parent { + // Allow lookups awaiting for a parent to have zero peers. If when the parent + // resolve they still have zero peers the lookup will fail gracefully. + return Ok(()); + } else { + return Err(LookupRequestError::NoPeers); + } + }; match request.make_request(id, peer_id, downloaded_block_expected_blobs, cx)? { LookupRequestResult::RequestSent => request.get_state_mut().on_download_start()?, diff --git a/beacon_node/network/src/sync/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index 761e54144d6..619db0469ba 100644 --- a/beacon_node/network/src/sync/block_lookups/tests.rs +++ b/beacon_node/network/src/sync/block_lookups/tests.rs @@ -67,6 +67,7 @@ type T = Witness, E, MemoryStore, Memo struct TestRig { /// Receiver for `BeaconProcessor` events (e.g. block processing results). beacon_processor_rx: mpsc::Receiver>, + beacon_processor_rx_queue: Vec>, /// Receiver for `NetworkMessage` (e.g. outgoing RPC requests from sync) network_rx: mpsc::UnboundedReceiver>, /// Stores all `NetworkMessage`s received from `network_recv`. (e.g. outgoing RPC requests) @@ -127,6 +128,7 @@ impl TestRig { let rng = XorShiftRng::from_seed([42; 16]); TestRig { beacon_processor_rx, + beacon_processor_rx_queue: vec![], network_rx, network_rx_queue: vec![], rng, @@ -285,15 +287,6 @@ impl TestRig { self.expect_no_active_single_lookups(); } - fn expect_lookups(&self, expected_block_roots: &[Hash256]) { - let block_roots = self - .active_single_lookups() - .iter() - .map(|(_, b, _)| *b) - .collect::>(); - assert_eq!(&block_roots, expected_block_roots); - } - fn new_connected_peer(&mut self) -> PeerId { let peer_id = PeerId::random(); self.network_globals @@ -544,6 +537,12 @@ impl TestRig { } } + fn drain_processor_rx(&mut self) { + while let Ok(event) = self.beacon_processor_rx.try_recv() { + self.beacon_processor_rx_queue.push(event); + } + } + fn pop_received_network_event) -> Option>( &mut self, predicate_transform: F, @@ -564,8 +563,34 @@ impl TestRig { } } - #[track_caller] - fn expect_block_lookup_request(&mut self, for_block: Hash256) -> SingleLookupReqId { + fn pop_received_processor_event) -> Option>( + &mut self, + predicate_transform: F, + ) -> Result { + self.drain_processor_rx(); + + if let Some(index) = self + .beacon_processor_rx_queue + .iter() + .position(|x| predicate_transform(x).is_some()) + { + // Transform the item, knowing that it won't be None because we checked it in the position predicate. + let transformed = predicate_transform(&self.beacon_processor_rx_queue[index]).unwrap(); + self.beacon_processor_rx_queue.remove(index); + Ok(transformed) + } else { + Err(format!( + "current processor messages {:?}", + self.beacon_processor_rx_queue + ) + .to_string()) + } + } + + fn find_block_lookup_request( + &mut self, + for_block: Hash256, + ) -> Result { self.pop_received_network_event(|ev| match ev { NetworkMessage::SendRequest { peer_id: _, @@ -574,11 +599,18 @@ impl TestRig { } if request.block_roots().to_vec().contains(&for_block) => Some(*id), _ => None, }) - .unwrap_or_else(|e| panic!("Expected block request for {for_block:?}: {e}")) } #[track_caller] - fn expect_blob_lookup_request(&mut self, for_block: Hash256) -> SingleLookupReqId { + fn expect_block_lookup_request(&mut self, for_block: Hash256) -> SingleLookupReqId { + self.find_block_lookup_request(for_block) + .unwrap_or_else(|e| panic!("Expected block request for {for_block:?}: {e}")) + } + + fn find_blob_lookup_request( + &mut self, + for_block: Hash256, + ) -> Result { self.pop_received_network_event(|ev| match ev { NetworkMessage::SendRequest { peer_id: _, @@ -594,7 +626,12 @@ impl TestRig { } _ => None, }) - .unwrap_or_else(|e| panic!("Expected blob request for {for_block:?}: {e}")) + } + + #[track_caller] + fn expect_blob_lookup_request(&mut self, for_block: Hash256) -> SingleLookupReqId { + self.find_blob_lookup_request(for_block) + .unwrap_or_else(|e| panic!("Expected blob request for {for_block:?}: {e}")) } #[track_caller] @@ -610,6 +647,15 @@ impl TestRig { .unwrap_or_else(|e| panic!("Expected block parent request for {for_block:?}: {e}")) } + fn expect_no_requests_for(&mut self, block_root: Hash256) { + if let Ok(request) = self.find_block_lookup_request(block_root) { + panic!("Expected no block request for {block_root:?} found {request:?}"); + } + if let Ok(request) = self.find_blob_lookup_request(block_root) { + panic!("Expected no blob request for {block_root:?} found {request:?}"); + } + } + #[track_caller] fn expect_blob_parent_request(&mut self, for_block: Hash256) -> SingleLookupReqId { self.pop_received_network_event(|ev| match ev { @@ -653,18 +699,16 @@ impl TestRig { #[track_caller] fn expect_block_process(&mut self, response_type: ResponseType) { match response_type { - ResponseType::Block => match self.beacon_processor_rx.try_recv() { - Ok(work) => { - assert_eq!(work.work_type(), beacon_processor::RPC_BLOCK); - } - other => panic!("Expected block process, found {:?}", other), - }, - ResponseType::Blob => match self.beacon_processor_rx.try_recv() { - Ok(work) => { - assert_eq!(work.work_type(), beacon_processor::RPC_BLOBS); - } - other => panic!("Expected blob process, found {:?}", other), - }, + ResponseType::Block => self + .pop_received_processor_event(|ev| { + (ev.work_type() == beacon_processor::RPC_BLOCK).then_some(()) + }) + .unwrap_or_else(|e| panic!("Expected block work event: {e}")), + ResponseType::Blob => self + .pop_received_processor_event(|ev| { + (ev.work_type() == beacon_processor::RPC_BLOBS).then_some(()) + }) + .unwrap_or_else(|e| panic!("Expected blobs work event: {e}")), } } @@ -907,22 +951,29 @@ fn test_single_block_lookup_happy_path() { rig.expect_no_active_lookups(); } +// Tests that if a peer does not respond with a block, we downscore and retry the block only #[test] fn test_single_block_lookup_empty_response() { - let mut rig = TestRig::test_setup(); + let mut r = TestRig::test_setup(); - let block_hash = Hash256::random(); - let peer_id = rig.new_connected_peer(); + let block = r.rand_block(); + let block_root = block.canonical_root(); + let peer_id = r.new_connected_peer(); // Trigger the request - rig.trigger_unknown_block_from_attestation(block_hash, peer_id); - let id = rig.expect_lookup_request_block_and_blobs(block_hash); + r.trigger_unknown_block_from_attestation(block_root, peer_id); + let id = r.expect_lookup_request_block_and_blobs(block_root); // The peer does not have the block. It should be penalized. - rig.single_lookup_block_response(id, peer_id, None); - rig.expect_penalty(peer_id, "NoResponseReturned"); - - rig.expect_block_lookup_request(block_hash); // it should be retried + r.single_lookup_block_response(id, peer_id, None); + r.expect_penalty(peer_id, "NoResponseReturned"); + // it should be retried + let id = r.expect_block_lookup_request(block_root); + // Send the right block this time. + r.single_lookup_block_response(id, peer_id, Some(block.into())); + r.expect_block_process(ResponseType::Block); + r.single_block_component_processed_imported(block_root); + r.expect_no_active_lookups(); } #[test] @@ -1014,6 +1065,8 @@ fn test_parent_lookup_happy_path() { rig.expect_block_process(ResponseType::Block); rig.expect_empty_network(); + // Add peer to child lookup to prevent it being dropped + rig.trigger_unknown_block_from_attestation(block_root, peer_id); // Processing succeeds, now the rest of the chain should be sent for processing. rig.parent_block_processed( block_root, @@ -1049,6 +1102,8 @@ fn test_parent_lookup_wrong_response() { rig.parent_lookup_block_response(id2, peer_id, Some(parent.into())); rig.expect_block_process(ResponseType::Block); + // Add peer to child lookup to prevent it being dropped + rig.trigger_unknown_block_from_attestation(block_root, peer_id); // Processing succeeds, now the rest of the chain should be sent for processing. rig.parent_block_processed_imported(block_root); rig.expect_parent_chain_process(); @@ -1056,33 +1111,6 @@ fn test_parent_lookup_wrong_response() { rig.expect_no_active_lookups(); } -#[test] -fn test_parent_lookup_empty_response() { - let mut rig = TestRig::test_setup(); - - let (parent, block, parent_root, block_root) = rig.rand_block_and_parent(); - let peer_id = rig.new_connected_peer(); - - // Trigger the request - rig.trigger_unknown_parent_block(peer_id, block.into()); - let id1 = rig.expect_parent_request_block_and_blobs(parent_root); - - // Peer sends an empty response, peer should be penalized and the block re-requested. - rig.parent_lookup_block_response(id1, peer_id, None); - rig.expect_penalty(peer_id, "NoResponseReturned"); - let id2 = rig.expect_block_parent_request(parent_root); - - // Send the right block this time. - rig.parent_lookup_block_response(id2, peer_id, Some(parent.into())); - rig.expect_block_process(ResponseType::Block); - - // Processing succeeds, now the rest of the chain should be sent for processing. - rig.parent_block_processed_imported(block_root); - - rig.single_block_component_processed_imported(block_root); - rig.expect_no_active_lookups(); -} - #[test] fn test_parent_lookup_rpc_failure() { let mut rig = TestRig::test_setup(); @@ -1102,6 +1130,8 @@ fn test_parent_lookup_rpc_failure() { rig.parent_lookup_block_response(id2, peer_id, Some(parent.into())); rig.expect_block_process(ResponseType::Block); + // Add peer to child lookup to prevent it being dropped + rig.trigger_unknown_block_from_attestation(block_root, peer_id); // Processing succeeds, now the rest of the chain should be sent for processing. rig.parent_block_processed_imported(block_root); rig.expect_parent_chain_process(); @@ -1287,19 +1317,6 @@ fn test_skip_creating_failed_parent_lookup() { rig.expect_no_active_lookups(); } -#[test] -fn test_skip_creating_failed_current_lookup() { - let mut rig = TestRig::test_setup(); - let (_, block, parent_root, block_root) = rig.rand_block_and_parent(); - let peer_id = rig.new_connected_peer(); - rig.insert_failed_chain(block_root); - rig.trigger_unknown_parent_block(peer_id, block.into()); - // Expect single penalty for peer - rig.expect_single_penalty(peer_id, "failed_chain"); - // Only the current lookup should be rejected - rig.expect_lookups(&[parent_root]); -} - #[test] fn test_single_block_lookup_ignored_response() { let mut rig = TestRig::test_setup(); @@ -1396,7 +1413,10 @@ fn test_same_chain_race_condition() { rig.trigger_unknown_parent_block(peer_id, trigger_block.clone()); rig.expect_empty_network(); - // Processing succeeds, now the rest of the chain should be sent for processing. + // Add a peer to the tip child lookup which has zero peers + rig.trigger_unknown_block_from_attestation(trigger_block.canonical_root(), peer_id); + + rig.log("Processing succeeds, now the rest of the chain should be sent for processing."); for block in blocks.iter().skip(1).chain(&[trigger_block]) { rig.expect_parent_chain_process(); rig.single_block_component_processed_imported(block.canonical_root()); @@ -1497,6 +1517,7 @@ mod deneb_only { rig: TestRig, block: Arc>, blobs: Vec>>, + parent_block_roots: Vec, parent_block: VecDeque>>, parent_blobs: VecDeque>>>, unknown_parent_block: Option>>, @@ -1512,16 +1533,16 @@ mod deneb_only { enum RequestTrigger { AttestationUnknownBlock, - GossipUnknownParentBlock { num_parents: usize }, - GossipUnknownParentBlob { num_parents: usize }, + GossipUnknownParentBlock(usize), + GossipUnknownParentBlob(usize), } impl RequestTrigger { fn num_parents(&self) -> usize { match self { RequestTrigger::AttestationUnknownBlock => 0, - RequestTrigger::GossipUnknownParentBlock { num_parents } => *num_parents, - RequestTrigger::GossipUnknownParentBlob { num_parents } => *num_parents, + RequestTrigger::GossipUnknownParentBlock(num_parents) => *num_parents, + RequestTrigger::GossipUnknownParentBlob(num_parents) => *num_parents, } } } @@ -1539,6 +1560,7 @@ mod deneb_only { let num_parents = request_trigger.num_parents(); let mut parent_block_chain = VecDeque::with_capacity(num_parents); let mut parent_blobs_chain = VecDeque::with_capacity(num_parents); + let mut parent_block_roots = vec![]; for _ in 0..num_parents { // Set the current block as the parent. let parent_root = block.canonical_root(); @@ -1546,6 +1568,7 @@ mod deneb_only { let parent_blobs = blobs.clone(); parent_block_chain.push_front(parent_block); parent_blobs_chain.push_front(parent_blobs); + parent_block_roots.push(parent_root); // Create the next block. let (child_block, child_blobs) = @@ -1580,13 +1603,12 @@ mod deneb_only { )); let parent_root = block.parent_root(); - let blob_req_id = rig.expect_blob_lookup_request(block_root); let parent_block_req_id = rig.expect_block_parent_request(parent_root); let parent_blob_req_id = rig.expect_blob_parent_request(parent_root); rig.expect_empty_network(); // expect no more requests ( None, - Some(blob_req_id), + None, Some(parent_block_req_id), Some(parent_blob_req_id), ) @@ -1596,14 +1618,12 @@ mod deneb_only { let parent_root = single_blob.block_parent_root(); rig.send_sync_message(SyncMessage::UnknownParentBlob(peer_id, single_blob)); - let block_req_id = rig.expect_block_lookup_request(block_root); - let blobs_req_id = rig.expect_blob_lookup_request(block_root); let parent_block_req_id = rig.expect_block_parent_request(parent_root); let parent_blob_req_id = rig.expect_blob_parent_request(parent_root); rig.expect_empty_network(); // expect no more requests ( - Some(block_req_id), - Some(blobs_req_id), + None, + None, Some(parent_block_req_id), Some(parent_blob_req_id), ) @@ -1616,6 +1636,7 @@ mod deneb_only { blobs, parent_block: parent_block_chain, parent_blobs: parent_blobs_chain, + parent_block_roots, unknown_parent_block: None, unknown_parent_blobs: None, peer_id, @@ -1633,6 +1654,13 @@ mod deneb_only { self } + fn trigger_unknown_block_from_attestation(mut self) -> Self { + let block_root = self.block.canonical_root(); + self.rig + .trigger_unknown_block_from_attestation(block_root, self.peer_id); + self + } + fn parent_block_response(mut self) -> Self { self.rig.expect_empty_network(); let block = self.parent_block.pop_front().unwrap().clone(); @@ -1743,15 +1771,6 @@ mod deneb_only { self } - fn empty_parent_block_response(mut self) -> Self { - self.rig.parent_lookup_block_response( - self.parent_block_req_id.expect("block request id"), - self.peer_id, - None, - ); - self - } - fn empty_parent_blobs_response(mut self) -> Self { self.rig.parent_lookup_blob_response( self.parent_blob_req_id.expect("blob request id"), @@ -1800,23 +1819,28 @@ mod deneb_only { } fn parent_block_imported(mut self) -> Self { - self.rig.log("parent_block_imported"); + let parent_root = *self.parent_block_roots.first().unwrap(); + self.rig + .log(&format!("parent_block_imported {parent_root:?}")); self.rig.parent_block_processed( self.block_root, - BlockProcessingResult::Ok(AvailabilityProcessingStatus::Imported(self.block_root)), + BlockProcessingResult::Ok(AvailabilityProcessingStatus::Imported(parent_root)), ); - self.rig.expect_empty_network(); + self.rig.expect_no_requests_for(parent_root); self.rig.assert_parent_lookups_count(0); self } fn parent_blob_imported(mut self) -> Self { - self.rig.log("parent_blob_imported"); + let parent_root = *self.parent_block_roots.first().unwrap(); + self.rig + .log(&format!("parent_blob_imported {parent_root:?}")); self.rig.parent_blob_processed( self.block_root, - BlockProcessingResult::Ok(AvailabilityProcessingStatus::Imported(self.block_root)), + BlockProcessingResult::Ok(AvailabilityProcessingStatus::Imported(parent_root)), ); - self.rig.expect_empty_network(); + + self.rig.expect_no_requests_for(parent_root); self.rig.assert_parent_lookups_count(0); self } @@ -1914,6 +1938,34 @@ mod deneb_only { self } + fn complete_current_block_and_blobs_lookup(self) -> Self { + self.expect_block_request() + .expect_blobs_request() + .block_response() + .blobs_response() + // TODO: Should send blobs for processing + .expect_block_process() + .block_imported() + } + + fn empty_parent_blobs_then_parent_block(self) -> Self { + self.log( + " Return empty blobs for parent, block errors with missing components, downscore", + ) + .empty_parent_blobs_response() + .expect_no_penalty_and_no_requests() + .parent_block_response() + .parent_block_missing_components() + .expect_penalty("sent_incomplete_blobs") + .log("Re-request parent blobs, succeed and import parent") + .expect_parent_blobs_request() + .parent_blob_response() + .expect_block_process() + // Insert new peer into child request before completing parent + .trigger_unknown_block_from_attestation() + .parent_blob_imported() + } + fn expect_penalty(mut self, expect_penalty_msg: &'static str) -> Self { self.rig.expect_penalty(self.peer_id, expect_penalty_msg); self @@ -1971,10 +2023,6 @@ mod deneb_only { self.blobs.push(first_blob); self } - fn expect_parent_chain_process(mut self) -> Self { - self.rig.expect_parent_chain_process(); - self - } fn expect_block_process(mut self) -> Self { self.rig.expect_block_process(ResponseType::Block); self @@ -1995,7 +2043,6 @@ mod deneb_only { let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { return; }; - tester .block_response_triggering_process() .blobs_response() @@ -2009,7 +2056,6 @@ mod deneb_only { let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { return; }; - tester .blobs_response() // hold blobs for processing .block_response_triggering_process() @@ -2023,7 +2069,6 @@ mod deneb_only { let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { return; }; - tester .empty_block_response() .expect_penalty("NoResponseReturned") @@ -2075,7 +2120,6 @@ mod deneb_only { let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { return; }; - tester .block_response_triggering_process() .invalid_block_processed() @@ -2092,7 +2136,6 @@ mod deneb_only { let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { return; }; - tester .block_response_triggering_process() .missing_components_from_block_request() @@ -2108,7 +2151,6 @@ mod deneb_only { let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { return; }; - tester .block_response_triggering_process() .missing_components_from_block_request() @@ -2125,7 +2167,6 @@ mod deneb_only { let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { return; }; - tester .block_response_triggering_process() .invalidate_blobs_too_many() @@ -2140,7 +2181,6 @@ mod deneb_only { let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { return; }; - tester .invalidate_blobs_too_few() .blobs_response() // blobs are not sent until the block is processed @@ -2153,7 +2193,6 @@ mod deneb_only { let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { return; }; - tester .invalidate_blobs_too_many() .blobs_response() @@ -2163,16 +2202,13 @@ mod deneb_only { .block_response_triggering_process(); } + // Test peer returning block that has unknown parent, and a new lookup is created #[test] fn parent_block_unknown_parent() { - let Some(tester) = - DenebTester::new(RequestTrigger::GossipUnknownParentBlock { num_parents: 1 }) - else { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownParentBlock(1)) else { return; }; - tester - .blobs_response() .expect_empty_beacon_processor() .parent_block_response() .parent_blob_response() @@ -2183,17 +2219,13 @@ mod deneb_only { .expect_empty_beacon_processor(); } + // Test peer returning invalid (processing) block, expect retry #[test] fn parent_block_invalid_parent() { - let Some(tester) = - DenebTester::new(RequestTrigger::GossipUnknownParentBlock { num_parents: 1 }) - else { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownParentBlock(1)) else { return; }; - tester - .blobs_response() - .expect_empty_beacon_processor() .parent_block_response() .parent_blob_response() .expect_block_process() @@ -2203,99 +2235,44 @@ mod deneb_only { .expect_empty_beacon_processor(); } + // Tests that if a peer does not respond with a block, we downscore and retry the block only #[test] - fn parent_block_and_blob_lookup_parent_returned_first() { - let Some(tester) = - DenebTester::new(RequestTrigger::GossipUnknownParentBlock { num_parents: 1 }) - else { - return; - }; - - tester - .parent_block_response() - .parent_blob_response() - .expect_block_process() - .parent_block_imported() - .blobs_response() - .expect_parent_chain_process(); - } - - #[test] - fn parent_block_and_blob_lookup_child_returned_first() { - let Some(tester) = - DenebTester::new(RequestTrigger::GossipUnknownParentBlock { num_parents: 1 }) - else { - return; - }; - - tester - .blobs_response() - .expect_no_penalty_and_no_requests() - .parent_block_response() - .parent_blob_response() - .expect_block_process() - .parent_block_imported() - .expect_parent_chain_process(); - } - - #[test] - fn empty_parent_block_then_parent_blob() { - let Some(tester) = - DenebTester::new(RequestTrigger::GossipUnknownParentBlock { num_parents: 1 }) - else { + fn empty_block_is_retried() { + let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { return; }; - tester - .empty_parent_block_response() + .empty_block_response() .expect_penalty("NoResponseReturned") - .expect_parent_block_request() + .expect_block_request() .expect_no_blobs_request() - .parent_blob_response() - .expect_empty_beacon_processor() - .parent_block_response() - .expect_block_process() - .parent_block_imported() + .block_response() .blobs_response() - .expect_parent_chain_process(); + .block_imported() + .expect_no_active_lookups(); } #[test] fn empty_parent_blobs_then_parent_block() { - let Some(tester) = - DenebTester::new(RequestTrigger::GossipUnknownParentBlock { num_parents: 1 }) - else { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownParentBlock(1)) else { return; }; - tester - .blobs_response() - .log(" Return empty blobs for parent, block errors with missing components, downscore") - .empty_parent_blobs_response() - .expect_no_penalty_and_no_requests() - .parent_block_response() - .parent_block_missing_components() - .expect_penalty("sent_incomplete_blobs") - .log("Re-request parent blobs, succeed and import parent") - .expect_parent_blobs_request() - .parent_blob_response() - .expect_block_process() - .parent_blob_imported() + .empty_parent_blobs_then_parent_block() .log("resolve original block trigger blobs request and import") + // Should not have block request, it is cached + .expect_blobs_request() + // TODO: Should send blobs for processing .block_imported() .expect_no_active_lookups(); } #[test] fn parent_blob_unknown_parent() { - let Some(tester) = - DenebTester::new(RequestTrigger::GossipUnknownParentBlob { num_parents: 1 }) - else { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownParentBlob(1)) else { return; }; - tester - .block_response() .expect_empty_beacon_processor() .parent_block_response() .parent_blob_response() @@ -2308,14 +2285,10 @@ mod deneb_only { #[test] fn parent_blob_invalid_parent() { - let Some(tester) = - DenebTester::new(RequestTrigger::GossipUnknownParentBlob { num_parents: 1 }) - else { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownParentBlob(1)) else { return; }; - tester - .block_response() .expect_empty_beacon_processor() .parent_block_response() .parent_blob_response() @@ -2329,106 +2302,37 @@ mod deneb_only { #[test] fn parent_block_and_blob_lookup_parent_returned_first_blob_trigger() { - let Some(tester) = - DenebTester::new(RequestTrigger::GossipUnknownParentBlob { num_parents: 1 }) - else { - return; - }; - - tester - .parent_block_response() - .parent_blob_response() - .expect_block_process() - .parent_block_imported() - .block_response() - .blobs_response() - .expect_parent_chain_process() - .block_imported() - .expect_no_active_lookups(); - } - - #[test] - fn parent_block_and_blob_lookup_child_returned_first_blob_trigger() { - let Some(tester) = - DenebTester::new(RequestTrigger::GossipUnknownParentBlob { num_parents: 1 }) - else { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownParentBlob(1)) else { return; }; - tester - .block_response() - .expect_no_penalty_and_no_requests() .parent_block_response() .parent_blob_response() .expect_block_process() + .trigger_unknown_block_from_attestation() .parent_block_imported() - .blobs_response() - .expect_parent_chain_process() - .block_imported() - .expect_no_active_lookups(); - } - - #[test] - fn empty_parent_block_then_parent_blob_blob_trigger() { - let Some(tester) = - DenebTester::new(RequestTrigger::GossipUnknownParentBlob { num_parents: 1 }) - else { - return; - }; - - tester - .empty_parent_block_response() - .expect_penalty("NoResponseReturned") - .expect_parent_block_request() - .expect_no_blobs_request() - .parent_blob_response() - .expect_empty_beacon_processor() - .parent_block_response() - .expect_block_process() - .parent_block_imported() - .blobs_response() - .block_response() - .block_imported() + .complete_current_block_and_blobs_lookup() .expect_no_active_lookups(); } #[test] fn empty_parent_blobs_then_parent_block_blob_trigger() { - let Some(tester) = - DenebTester::new(RequestTrigger::GossipUnknownParentBlob { num_parents: 1 }) - else { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownParentBlob(1)) else { return; }; - tester - .block_response() - .log(" Return empty blobs for parent, block errors with missing components, downscore") - .empty_parent_blobs_response() - .expect_no_penalty_and_no_requests() - .parent_block_response() - .parent_block_missing_components() - .expect_penalty("sent_incomplete_blobs") - .log("Re-request parent blobs, succeed and import parent") - .expect_parent_blobs_request() - .parent_blob_response() - .expect_block_process() - .parent_blob_imported() + .empty_parent_blobs_then_parent_block() .log("resolve original block trigger blobs request and import") - .blobs_response() - .block_imported() + .complete_current_block_and_blobs_lookup() .expect_no_active_lookups(); } #[test] fn parent_blob_unknown_parent_chain() { - let Some(tester) = - DenebTester::new(RequestTrigger::GossipUnknownParentBlob { num_parents: 2 }) - else { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownParentBlob(2)) else { return; }; - tester - .block_response() .expect_empty_beacon_processor() .parent_block_response() .parent_blob_response() @@ -2446,12 +2350,9 @@ mod deneb_only { #[test] fn unknown_parent_block_dup() { - let Some(tester) = - DenebTester::new(RequestTrigger::GossipUnknownParentBlock { num_parents: 1 }) - else { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownParentBlock(1)) else { return; }; - tester .search_parent_dup() .expect_no_blobs_request() @@ -2460,18 +2361,18 @@ mod deneb_only { #[test] fn unknown_parent_blob_dup() { - let Some(tester) = - DenebTester::new(RequestTrigger::GossipUnknownParentBlob { num_parents: 1 }) - else { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownParentBlob(1)) else { return; }; - tester .search_parent_dup() .expect_no_blobs_request() .expect_no_block_request(); } + // This test no longer applies, we don't issue requests for child lookups + // Keep for after updating rules on fetching blocks only first + #[ignore] #[test] fn no_peer_penalty_when_rpc_response_already_known_from_gossip() { let Some(mut r) = TestRig::test_setup_after_deneb() else {