From 51c4506c537928b8a87fb3a009a1c52b1fb789a0 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Tue, 9 May 2023 19:29:03 -0400 Subject: [PATCH] smol bugfixes and moar tests --- .../network/src/sync/block_lookups/mod.rs | 64 +-- .../src/sync/block_lookups/parent_lookup.rs | 12 +- .../sync/block_lookups/single_block_lookup.rs | 58 ++- .../network/src/sync/block_lookups/tests.rs | 459 ++++++++++++++++-- beacon_node/network/src/sync/manager.rs | 6 +- 5 files changed, 508 insertions(+), 91 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index f8b9e038d3..e7fde333df 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -88,33 +88,33 @@ pub enum ResponseType { } #[derive(Debug, PartialEq, Eq, Hash, Clone, Copy, Display)] -pub enum PeerSource { - Attestation(PeerId), - Gossip(PeerId), +pub enum PeerShouldHave { + BlockAndBlobs(PeerId), + Neither(PeerId), } -impl PeerSource { +impl PeerShouldHave { fn as_peer_id(&self) -> &PeerId { match self { - PeerSource::Attestation(id) => id, - PeerSource::Gossip(id) => id, + PeerShouldHave::BlockAndBlobs(id) => id, + PeerShouldHave::Neither(id) => id, } } fn to_peer_id(self) -> PeerId { match self { - PeerSource::Attestation(id) => id, - PeerSource::Gossip(id) => id, + PeerShouldHave::BlockAndBlobs(id) => id, + PeerShouldHave::Neither(id) => id, } } fn should_have_block(&self) -> bool { match self { - PeerSource::Attestation(_) => true, - PeerSource::Gossip(_) => false, + PeerShouldHave::BlockAndBlobs(_) => true, + PeerShouldHave::Neither(_) => false, } } fn should_have_blobs(&self) -> bool { match self { - PeerSource::Attestation(_) => true, - PeerSource::Gossip(_) => false, + PeerShouldHave::BlockAndBlobs(_) => true, + PeerShouldHave::Neither(_) => false, } } } @@ -147,7 +147,7 @@ impl BlockLookups { pub fn search_block( &mut self, hash: Hash256, - peer_source: PeerSource, + peer_source: PeerShouldHave, cx: &mut SyncNetworkContext, ) { self.search_block_with(|_| {}, hash, peer_source, cx) @@ -159,7 +159,7 @@ impl BlockLookups { &mut self, cache_fn: impl Fn(&mut SingleBlockLookup), hash: Hash256, - peer_source: PeerSource, + peer_source: PeerShouldHave, cx: &mut SyncNetworkContext, ) { // Do not re-request a block that is already being requested @@ -236,7 +236,7 @@ impl BlockLookups { let _ = request.add_block_wrapper(block_root, block.clone()); }, block_root, - PeerSource::Gossip(peer_id), + PeerShouldHave::Neither(peer_id), cx, ); } @@ -253,7 +253,7 @@ impl BlockLookups { let _ = request.add_blob(blob.clone()); }, block_root, - PeerSource::Gossip(peer_id), + PeerShouldHave::Neither(peer_id), cx, ); } @@ -269,7 +269,7 @@ impl BlockLookups { cx: &mut SyncNetworkContext, ) { // Gossip blocks or blobs shouldn't be propogated if parents are unavailable. - let peer_source = PeerSource::Attestation(peer_id); + let peer_source = PeerShouldHave::BlockAndBlobs(peer_id); // If this block or it's parent is part of a known failed chain, ignore it. if self.failed_chains.contains(&parent_root) || self.failed_chains.contains(&block_root) { @@ -867,17 +867,18 @@ impl BlockLookups { pub fn single_block_processed( &mut self, - id: Id, + target_id: Id, result: BlockProcessingResult, response_type: ResponseType, cx: &mut SyncNetworkContext, ) { let lookup_components_opt = self.single_block_lookups.iter_mut().enumerate().find_map( |(index, (block_id_opt, blob_id_opt, req))| { - block_id_opt - .as_mut() - .or(blob_id_opt.as_mut()) - .and_then(|id_ref| (*id_ref == id).then_some((index, id_ref, req))) + let id_filter = |id: &&mut Id| -> bool { target_id == **id }; + + let block_id = block_id_opt.as_mut().filter(id_filter); + let blob_id = blob_id_opt.as_mut().filter(id_filter); + block_id.or(blob_id).map(|id_ref| (index, id_ref, req)) }, ); let (index, request_id_ref, request_ref) = match lookup_components_opt { @@ -1425,14 +1426,17 @@ fn handle_block_lookup_verify_error( ) -> ShouldRemoveLookup { let msg = if matches!(e, LookupVerifyError::BenignFailure) { match response_type { - ResponseType::Block => request_ref - .block_request_state - .potential_peers - .remove(&peer_id), - ResponseType::Blob => request_ref - .blob_request_state - .potential_peers - .remove(&peer_id), + // Only remove a potential peer if there are better options + ResponseType::Block => { + request_ref + .block_request_state + .remove_peer_if_useless(&peer_id); + } + ResponseType::Blob => { + request_ref + .blob_request_state + .remove_peer_if_useless(&peer_id); + } }; "peer could not response to request" } else { diff --git a/beacon_node/network/src/sync/block_lookups/parent_lookup.rs b/beacon_node/network/src/sync/block_lookups/parent_lookup.rs index 7b0b94f136..1441d26062 100644 --- a/beacon_node/network/src/sync/block_lookups/parent_lookup.rs +++ b/beacon_node/network/src/sync/block_lookups/parent_lookup.rs @@ -1,5 +1,5 @@ use super::single_block_lookup::{LookupRequestError, LookupVerifyError, SingleBlockLookup}; -use super::{DownloadedBlocks, PeerSource, ResponseType}; +use super::{DownloadedBlocks, PeerShouldHave, ResponseType}; use crate::sync::block_lookups::{single_block_lookup, RootBlobsTuple, RootBlockTuple}; use crate::sync::{ manager::{Id, SLOT_IMPORT_TOLERANCE}, @@ -90,7 +90,7 @@ impl ParentLookup { pub fn new( block_root: Hash256, parent_root: Hash256, - peer_id: PeerSource, + peer_id: PeerShouldHave, da_checker: Arc>, ) -> Self { let current_parent_request = SingleBlockLookup::new(parent_root, peer_id, da_checker); @@ -328,7 +328,11 @@ impl ParentLookup { .failed_attempts() } - pub fn add_peer_if_useful(&mut self, block_root: &Hash256, peer_source: PeerSource) -> bool { + pub fn add_peer_if_useful( + &mut self, + block_root: &Hash256, + peer_source: PeerShouldHave, + ) -> bool { self.current_parent_request .add_peer_if_useful(block_root, peer_source) } @@ -352,7 +356,7 @@ impl ParentLookup { &self, response_type: ResponseType, peer_id: PeerId, - ) -> Option { + ) -> Option { self.current_parent_request .peer_source(response_type, peer_id) } 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 13ab2fad27..b4aaaaba66 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 @@ -14,7 +14,7 @@ use strum::IntoStaticStr; use types::blob_sidecar::{BlobIdentifier, FixedBlobSidecarList}; use types::{BlobSidecar, SignedBeaconBlock}; -use super::{PeerSource, ResponseType}; +use super::{PeerShouldHave, ResponseType}; pub struct SingleBlockLookup { pub requested_block_root: Hash256, @@ -49,8 +49,8 @@ pub struct SingleLookupRequestState { #[derive(Debug, PartialEq, Eq)] pub enum State { AwaitingDownload, - Downloading { peer_id: PeerSource }, - Processing { peer_id: PeerSource }, + Downloading { peer_id: PeerShouldHave }, + Processing { peer_id: PeerShouldHave }, } #[derive(Debug, PartialEq, Eq, IntoStaticStr)] @@ -81,7 +81,7 @@ pub enum LookupRequestError { impl SingleBlockLookup { pub fn new( requested_block_root: Hash256, - peer_source: PeerSource, + peer_source: PeerShouldHave, da_checker: Arc>, ) -> Self { Self { @@ -337,7 +337,7 @@ impl SingleBlockLookup SingleBlockLookup SingleBlockLookup SingleBlockLookup SingleBlockLookup bool { + pub fn add_peer_if_useful( + &mut self, + block_root: &Hash256, + peer_source: PeerShouldHave, + ) -> bool { if *block_root != self.requested_block_root { return false; } match peer_source { - PeerSource::Attestation(peer_id) => { + PeerShouldHave::BlockAndBlobs(peer_id) => { self.block_request_state.add_peer(&peer_id); self.blob_request_state.add_peer(&peer_id); } - PeerSource::Gossip(peer_id) => { + PeerShouldHave::Neither(peer_id) => { self.block_request_state.add_potential_peer(&peer_id); self.blob_request_state.add_potential_peer(&peer_id); } @@ -435,7 +439,7 @@ impl SingleBlockLookup Result { + pub fn processing_peer(&self, response_type: ResponseType) -> Result { match response_type { ResponseType::Block => self.block_request_state.processing_peer(), ResponseType::Blob => self.blob_request_state.processing_peer(), @@ -446,22 +450,22 @@ impl SingleBlockLookup Option { + ) -> Option { match response_type { ResponseType::Block => { if self.block_request_state.available_peers.contains(&peer_id) { - Some(PeerSource::Attestation(peer_id)) + Some(PeerShouldHave::BlockAndBlobs(peer_id)) } else if self.block_request_state.potential_peers.contains(&peer_id) { - Some(PeerSource::Gossip(peer_id)) + Some(PeerShouldHave::Neither(peer_id)) } else { None } } ResponseType::Blob => { if self.blob_request_state.available_peers.contains(&peer_id) { - Some(PeerSource::Attestation(peer_id)) + Some(PeerShouldHave::BlockAndBlobs(peer_id)) } else if self.blob_request_state.potential_peers.contains(&peer_id) { - Some(PeerSource::Gossip(peer_id)) + Some(PeerShouldHave::Neither(peer_id)) } else { None } @@ -471,10 +475,12 @@ impl SingleBlockLookup SingleLookupRequestState { - pub fn new(peer_source: PeerSource) -> Self { + pub fn new(peer_source: PeerShouldHave) -> Self { let (available_peers, potential_peers) = match peer_source { - PeerSource::Attestation(peer_id) => (HashSet::from([peer_id]), HashSet::default()), - PeerSource::Gossip(peer_id) => (HashSet::default(), HashSet::from([peer_id])), + PeerShouldHave::BlockAndBlobs(peer_id) => { + (HashSet::from([peer_id]), HashSet::default()) + } + PeerShouldHave::Neither(peer_id) => (HashSet::default(), HashSet::from([peer_id])), }; Self { state: State::AwaitingDownload, @@ -529,13 +535,19 @@ impl SingleLookupRequestState { Ok(()) } - pub fn processing_peer(&self) -> Result { + pub fn processing_peer(&self) -> Result { if let State::Processing { peer_id } = &self.state { Ok(*peer_id) } else { Err(()) } } + + pub fn remove_peer_if_useless(&mut self, peer_id: &PeerId) { + if !self.available_peers.is_empty() || self.potential_peers.len() > 1 { + self.potential_peers.remove(peer_id); + } + } } impl slog::Value @@ -613,7 +625,7 @@ mod tests { #[test] fn test_happy_path() { - let peer_id = PeerSource::Attestation(PeerId::random()); + let peer_id = PeerShouldHave::BlockAndBlobs(PeerId::random()); let block = rand_block(); let spec = E::default_spec(); let slot_clock = TestingSlotClock::new( @@ -630,7 +642,7 @@ mod tests { #[test] fn test_block_lookup_failures() { const FAILURES: u8 = 3; - let peer_id = PeerSource::Attestation(PeerId::random()); + let peer_id = PeerShouldHave::BlockAndBlobs(PeerId::random()); let block = rand_block(); let spec = E::default_spec(); let slot_clock = TestingSlotClock::new( diff --git a/beacon_node/network/src/sync/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index 64b043d600..70bf2243e7 100644 --- a/beacon_node/network/src/sync/block_lookups/tests.rs +++ b/beacon_node/network/src/sync/block_lookups/tests.rs @@ -268,7 +268,7 @@ fn test_single_block_lookup_happy_path() { let peer_id = PeerId::random(); let block_root = block.canonical_root(); // Trigger the request - bl.search_block(block_root, PeerSource::Attestation(peer_id), &mut cx); + bl.search_block(block_root, PeerShouldHave::BlockAndBlobs(peer_id), &mut cx); let id = rig.expect_block_request(response_type); // If we're in deneb, a blob request should have been triggered as well, // we don't require a response because we're generateing 0-blob blocks in this test. @@ -311,7 +311,7 @@ fn test_single_block_lookup_empty_response() { let peer_id = PeerId::random(); // Trigger the request - bl.search_block(block_hash, PeerSource::Attestation(peer_id), &mut cx); + bl.search_block(block_hash, PeerShouldHave::BlockAndBlobs(peer_id), &mut cx); let id = rig.expect_block_request(response_type); // If we're in deneb, a blob request should have been triggered as well, // we don't require a response because we're generateing 0-blob blocks in this test. @@ -339,7 +339,7 @@ fn test_single_block_lookup_wrong_response() { let peer_id = PeerId::random(); // Trigger the request - bl.search_block(block_hash, PeerSource::Attestation(peer_id), &mut cx); + bl.search_block(block_hash, PeerShouldHave::BlockAndBlobs(peer_id), &mut cx); let id = rig.expect_block_request(response_type); // If we're in deneb, a blob request should have been triggered as well, // we don't require a response because we're generateing 0-blob blocks in this test. @@ -371,7 +371,7 @@ fn test_single_block_lookup_failure() { let peer_id = PeerId::random(); // Trigger the request - bl.search_block(block_hash, PeerSource::Attestation(peer_id), &mut cx); + bl.search_block(block_hash, PeerShouldHave::BlockAndBlobs(peer_id), &mut cx); let id = rig.expect_block_request(response_type); // If we're in deneb, a blob request should have been triggered as well, // we don't require a response because we're generateing 0-blob blocks in this test. @@ -400,7 +400,7 @@ fn test_single_block_lookup_becomes_parent_request() { // Trigger the request bl.search_block( block.canonical_root(), - PeerSource::Attestation(peer_id), + PeerShouldHave::BlockAndBlobs(peer_id), &mut cx, ); let id = rig.expect_block_request(response_type); @@ -916,7 +916,7 @@ fn test_single_block_lookup_ignored_response() { // Trigger the request bl.search_block( block.canonical_root(), - PeerSource::Attestation(peer_id), + PeerShouldHave::BlockAndBlobs(peer_id), &mut cx, ); let id = rig.expect_block_request(response_type); @@ -1100,6 +1100,7 @@ fn test_same_chain_race_condition() { mod deneb_only { use super::*; + use beacon_chain::blob_verification::BlobError; use std::str::FromStr; struct DenebTester { @@ -1109,14 +1110,23 @@ mod deneb_only { block: Option>>, blobs: Vec>>, peer_id: PeerId, - block_req_id: u32, + block_req_id: Option, + parent_block_req_id: Option, blob_req_id: u32, + parent_blob_req_id: Option, slot: Slot, block_root: Hash256, } + enum RequestTrigger { + AttestationUnknownBlock, + GossipUnknownParentBlock, + GossipUnknownParentBlob, + GossipUnknownBlockOrBlob, + } + impl DenebTester { - fn new() -> Option { + fn new(request_trigger: RequestTrigger) -> Option { let fork_name = get_fork_name(); if !matches!(fork_name, ForkName::Deneb) { return None; @@ -1126,6 +1136,7 @@ mod deneb_only { E::slots_per_epoch() * rig.harness.spec.deneb_fork_epoch.unwrap().as_u64(), ); let (block, blobs) = rig.rand_block_and_blobs(fork_name, NumBlobs::Random); + let block = Arc::new(block); let blobs = blobs.into_iter().map(Arc::new).collect::>(); let peer_id = PeerId::random(); @@ -1133,19 +1144,71 @@ mod deneb_only { let block_root = block.canonical_root(); // Trigger the request - bl.search_block(block_root, PeerSource::Attestation(peer_id), &mut cx); - let block_req_id = rig.expect_block_request(ResponseType::Block); - let blob_req_id = rig.expect_block_request(ResponseType::Blob); + let (block_req_id, blob_req_id, parent_block_req_id, parent_blob_req_id) = + match request_trigger { + RequestTrigger::AttestationUnknownBlock => { + bl.search_block( + block_root, + PeerShouldHave::BlockAndBlobs(peer_id), + &mut cx, + ); + let block_req_id = rig.expect_block_request(ResponseType::Block); + let blob_req_id = rig.expect_block_request(ResponseType::Blob); + (Some(block_req_id), blob_req_id, None, None) + } + RequestTrigger::GossipUnknownParentBlock => { + bl.search_current_unknown_parent( + block_root, + BlockWrapper::Block(block.clone()), + peer_id, + &mut cx, + ); + let blob_req_id = rig.expect_block_request(ResponseType::Blob); + rig.expect_empty_network(); // expect no block request + bl.search_parent(slot, block_root, block.parent_root(), peer_id, &mut cx); + let parent_block_req_id = rig.expect_parent_request(ResponseType::Block); + let parent_blob_req_id = rig.expect_parent_request(ResponseType::Blob); + ( + None, + blob_req_id, + Some(parent_block_req_id), + Some(parent_blob_req_id), + ) + } + RequestTrigger::GossipUnknownParentBlob => { + let blob = blobs.first().cloned().unwrap(); + bl.search_current_unknown_blob_parent(blob, peer_id, &mut cx); + let block_req_id = rig.expect_block_request(ResponseType::Block); + let blob_req_id = rig.expect_block_request(ResponseType::Blob); + bl.search_parent(slot, block_root, block.parent_root(), peer_id, &mut cx); + let parent_block_req_id = rig.expect_parent_request(ResponseType::Block); + let parent_blob_req_id = rig.expect_parent_request(ResponseType::Blob); + ( + Some(block_req_id), + blob_req_id, + Some(parent_block_req_id), + Some(parent_blob_req_id), + ) + } + RequestTrigger::GossipUnknownBlockOrBlob => { + bl.search_block(block_root, PeerShouldHave::Neither(peer_id), &mut cx); + let block_req_id = rig.expect_block_request(ResponseType::Block); + let blob_req_id = rig.expect_block_request(ResponseType::Blob); + (Some(block_req_id), blob_req_id, None, None) + } + }; Some(Self { bl, cx, rig, - block: Some(Arc::new(block)), + block: Some(block), blobs, peer_id, block_req_id, + parent_block_req_id, blob_req_id, + parent_blob_req_id, slot, block_root, }) @@ -1155,7 +1218,7 @@ mod deneb_only { // The peer provides the correct block, should not be penalized. Now the block should be sent // for processing. self.bl.single_block_lookup_response( - self.block_req_id, + self.block_req_id.expect("block request id"), self.peer_id, self.block.clone(), D, @@ -1178,7 +1241,6 @@ mod deneb_only { D, &mut self.cx, ); - self.rig.expect_empty_network(); assert_eq!(self.bl.single_block_lookups.len(), 1); } // Send the blob stream termination. Peer should have not been penalized. @@ -1189,6 +1251,10 @@ mod deneb_only { D, &mut self.cx, ); + self + } + + fn blobs_response_was_valid(mut self) -> Self { self.rig.expect_empty_network(); self.rig.expect_block_process(ResponseType::Blob); self @@ -1196,7 +1262,7 @@ mod deneb_only { fn empty_block_response(mut self) -> Self { self.bl.single_block_lookup_response( - self.block_req_id, + self.block_req_id.expect("block request id"), self.peer_id, None, D, @@ -1220,7 +1286,7 @@ mod deneb_only { // Missing blobs should be the request is not removed, the outstanding blobs request should // mean we do not send a new request. self.bl.single_block_processed( - self.block_req_id, + self.block_req_id.expect("block request id"), BlockProcessingResult::Ok(AvailabilityProcessingStatus::Imported(self.block_root)), ResponseType::Block, &mut self.cx, @@ -1230,9 +1296,33 @@ mod deneb_only { self } - fn missing_components(mut self) -> Self { + fn invalid_block_processed(mut self) -> Self { self.bl.single_block_processed( - self.block_req_id, + self.block_req_id.expect("block request id"), + BlockProcessingResult::Err(BlockError::ProposalSignatureInvalid), + ResponseType::Block, + &mut self.cx, + ); + assert_eq!(self.bl.single_block_lookups.len(), 1); + self + } + + fn invalid_blob_processed(mut self) -> Self { + self.bl.single_block_processed( + self.blob_req_id, + BlockProcessingResult::Err(BlockError::BlobValidation( + BlobError::ProposerSignatureInvalid, + )), + ResponseType::Blob, + &mut self.cx, + ); + assert_eq!(self.bl.single_block_lookups.len(), 1); + self + } + + fn missing_components_from_block_request(mut self) -> Self { + self.bl.single_block_processed( + self.block_req_id.expect("block request id"), BlockProcessingResult::Ok(AvailabilityProcessingStatus::MissingComponents( self.slot, self.block_root, @@ -1244,6 +1334,20 @@ mod deneb_only { self } + fn missing_components_from_blob_request(mut self) -> Self { + self.bl.single_block_processed( + self.blob_req_id, + BlockProcessingResult::Ok(AvailabilityProcessingStatus::MissingComponents( + self.slot, + self.block_root, + )), + ResponseType::Blob, + &mut self.cx, + ); + assert_eq!(self.bl.single_block_lookups.len(), 1); + self + } + fn expect_penalty(mut self) -> Self { self.rig.expect_penalty(); self @@ -1268,6 +1372,15 @@ mod deneb_only { self.rig.expect_empty_network(); self } + fn invalidate_blobs_too_few(mut self) -> Self { + self.blobs.pop().expect("blobs"); + self + } + fn invalidate_blobs_too_many(mut self) -> Self { + let first_blob = self.blobs.get(0).expect("blob").clone(); + self.blobs.push(first_blob); + self + } } fn get_fork_name() -> ForkName { @@ -1284,26 +1397,34 @@ mod deneb_only { } #[test] - fn single_block_and_blob_lookup_block_returned_first() { - let Some(tester) = DenebTester::new() else { + fn single_block_and_blob_lookup_block_returned_first_attestation() { + let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { return; }; - tester.block_response().blobs_response().block_imported(); + tester + .block_response() + .blobs_response() + .blobs_response_was_valid() + .block_imported(); } #[test] - fn single_block_and_blob_lookup_blobs_returned_first() { - let Some(tester) = DenebTester::new() else { + fn single_block_and_blob_lookup_blobs_returned_first_attestation() { + let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { return; }; - tester.blobs_response().block_response().block_imported(); + tester + .blobs_response() + .blobs_response_was_valid() + .block_response() + .block_imported(); } #[test] - fn single_block_and_blob_lookup_empty_response() { - let Some(tester) = DenebTester::new() else { + fn single_block_and_blob_lookup_empty_response_attestation() { + let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { return; }; @@ -1319,12 +1440,172 @@ mod deneb_only { } #[test] - fn single_blob_lookup_empty_response() { - let Some(tester) = DenebTester::new() else { + fn single_block_response_then_empty_blob_response_attestation() { + let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { return; }; tester + .block_response() + .missing_components_from_block_request() + .empty_blobs_response() + .expect_penalty() + .expect_blobs_request() + .expect_no_block_request(); + } + + #[test] + fn single_blob_response_then_empty_block_response_attestation() { + let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { + return; + }; + + tester + .blobs_response() + .blobs_response_was_valid() + .expect_no_penalty() + .expect_no_block_request() + .expect_no_blobs_request() + .missing_components_from_blob_request() + .empty_block_response() + .expect_penalty() + .expect_block_request() + .expect_no_blobs_request(); + } + + #[test] + fn single_invalid_block_response_then_blob_response_attestation() { + let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { + return; + }; + + tester + .block_response() + .invalid_block_processed() + .expect_penalty() + .expect_block_request() + .expect_no_blobs_request() + .blobs_response() + .missing_components_from_blob_request() + .expect_no_penalty() + .expect_no_block_request() + .expect_no_block_request(); + } + + #[test] + fn single_block_response_then_invalid_blob_response_attestation() { + let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { + return; + }; + + tester + .block_response() + .missing_components_from_block_request() + .blobs_response() + .invalid_blob_processed() + .expect_penalty() + .expect_blobs_request() + .expect_no_block_request(); + } + + #[test] + fn single_block_response_then_too_few_blobs_response_attestation() { + let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { + return; + }; + + tester + .block_response() + .invalidate_blobs_too_few() + .blobs_response() + .expect_penalty() + .expect_blobs_request() + .expect_no_block_request(); + } + + #[test] + fn single_block_response_then_too_many_blobs_response_attestation() { + let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { + return; + }; + + tester + .block_response() + .invalidate_blobs_too_many() + .blobs_response() + .expect_penalty() + .expect_blobs_request() + .expect_no_block_request(); + } + #[test] + fn too_few_blobs_response_then_block_response_attestation() { + let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { + return; + }; + + tester + .invalidate_blobs_too_few() + .blobs_response() + // No way to know if the response was valid before we've seen the block. + .blobs_response_was_valid() + .expect_no_penalty() + .expect_no_blobs_request() + .expect_no_block_request() + .block_response(); + } + + #[test] + fn too_many_blobs_response_then_block_response_attestation() { + let Some(tester) = DenebTester::new(RequestTrigger::AttestationUnknownBlock) else { + return; + }; + + tester + .invalidate_blobs_too_many() + .blobs_response() + .expect_penalty() + .expect_blobs_request() + .expect_no_block_request() + .block_response(); + } + + #[test] + fn single_block_and_blob_lookup_block_returned_first_gossip() { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownBlockOrBlob) else { + return; + }; + + tester + .block_response() + .blobs_response() + .blobs_response_was_valid() + .block_imported(); + } + + #[test] + fn single_block_and_blob_lookup_blobs_returned_first_gossip() { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownBlockOrBlob) else { + return; + }; + + tester + .blobs_response() + .blobs_response_was_valid() + .block_response() + .block_imported(); + } + + #[test] + fn single_block_and_blob_lookup_empty_response_gossip() { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownBlockOrBlob) else { + return; + }; + + tester + .empty_block_response() + .expect_block_request() + .expect_no_penalty() + .expect_no_blobs_request() .empty_blobs_response() .expect_no_penalty() .expect_no_block_request() @@ -1332,17 +1613,133 @@ mod deneb_only { } #[test] - fn single_block_response_then_empty_blob_response() { - let Some(tester) = DenebTester::new() else { + fn single_block_response_then_empty_blob_response_gossip() { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownBlockOrBlob) else { return; }; tester .block_response() - .missing_components() + .missing_components_from_block_request() .empty_blobs_response() + .expect_blobs_request() + .expect_no_penalty() + .expect_no_block_request(); + } + + #[test] + fn single_blob_response_then_empty_block_response_gossip() { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownBlockOrBlob) else { + return; + }; + + tester + .blobs_response() + .blobs_response_was_valid() + .expect_no_penalty() + .expect_no_block_request() + .expect_no_blobs_request() + .missing_components_from_blob_request() + .empty_block_response() + .expect_block_request() + // No penalty because the blob was seen over gossip + .expect_no_penalty() + .expect_no_blobs_request(); + } + + #[test] + fn single_invalid_block_response_then_blob_response_gossip() { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownBlockOrBlob) else { + return; + }; + + tester + .block_response() + .invalid_block_processed() + .expect_penalty() + .expect_block_request() + .expect_no_blobs_request() + .blobs_response() + .missing_components_from_blob_request() + .expect_no_penalty() + .expect_no_block_request() + .expect_no_block_request(); + } + + #[test] + fn single_block_response_then_invalid_blob_response_gossip() { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownBlockOrBlob) else { + return; + }; + + tester + .block_response() + .missing_components_from_block_request() + .blobs_response() + .invalid_blob_processed() .expect_penalty() .expect_blobs_request() .expect_no_block_request(); } + + #[test] + fn single_block_response_then_too_few_blobs_response_gossip() { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownBlockOrBlob) else { + return; + }; + + tester + .block_response() + .invalidate_blobs_too_few() + .blobs_response() + .expect_blobs_request() + .expect_no_penalty() + .expect_no_block_request(); + } + + #[test] + fn single_block_response_then_too_many_blobs_response_gossip() { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownBlockOrBlob) else { + return; + }; + + tester + .block_response() + .invalidate_blobs_too_many() + .blobs_response() + .expect_penalty() + .expect_blobs_request() + .expect_no_block_request(); + } + #[test] + fn too_few_blobs_response_then_block_response_gossip() { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownBlockOrBlob) else { + return; + }; + + tester + .invalidate_blobs_too_few() + .blobs_response() + // No way to know if the response was valid before we've seen the block. + .blobs_response_was_valid() + .expect_no_penalty() + .expect_no_blobs_request() + .expect_no_block_request() + .block_response(); + } + + #[test] + fn too_many_blobs_response_then_block_response_gossip() { + let Some(tester) = DenebTester::new(RequestTrigger::GossipUnknownBlockOrBlob) else { + return; + }; + + tester + .invalidate_blobs_too_many() + .blobs_response() + .expect_penalty() + .expect_blobs_request() + .expect_no_block_request() + .block_response(); + } } diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index e85a53ab2b..95b8e57627 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -34,7 +34,7 @@ //! search for the block and subsequently search for parents if needed. use super::backfill_sync::{BackFillSync, ProcessResult, SyncStart}; -use super::block_lookups::{BlockLookups, PeerSource}; +use super::block_lookups::{BlockLookups, PeerShouldHave}; use super::network_context::{BlockOrBlob, SyncNetworkContext}; use super::peer_sync_info::{remote_sync_type, PeerSyncType}; use super::range_sync::{RangeSync, RangeSyncType, EPOCHS_PER_BATCH}; @@ -700,7 +700,7 @@ impl SyncManager { if self.synced_and_connected(&peer_id) { self.block_lookups.search_block( block_hash, - PeerSource::Attestation(peer_id), + PeerShouldHave::BlockAndBlobs(peer_id), &mut self.network, ); } @@ -718,7 +718,7 @@ impl SyncManager { } else { self.block_lookups.search_block( block_hash, - PeerSource::Gossip(peer_id), + PeerShouldHave::Neither(peer_id), &mut self.network, ) }