From 81f460327c9d428698e9c9b14719395cc1b748f4 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue, 8 Oct 2024 13:46:14 +0300 Subject: [PATCH 1/4] Better assert message in lookup sampling test --- .../network/src/sync/block_lookups/tests.rs | 38 +++++++++++++------ beacon_node/network/src/sync/manager.rs | 10 ++--- beacon_node/network/src/sync/peer_sampling.rs | 33 ++++++---------- 3 files changed, 43 insertions(+), 38 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index cd4609e1473..aa54f105f2b 100644 --- a/beacon_node/network/src/sync/block_lookups/tests.rs +++ b/beacon_node/network/src/sync/block_lookups/tests.rs @@ -1303,14 +1303,30 @@ impl TestRig { }); } - fn assert_sampling_request_status( - &self, - block_root: Hash256, - ongoing: &Vec, - no_peers: &Vec, - ) { - self.sync_manager - .assert_sampling_request_status(block_root, ongoing, no_peers) + fn assert_sampling_request_ongoing(&self, block_root: Hash256, indices: &Vec) { + for index in indices { + let status = self + .sync_manager + .get_sampling_request_status(block_root, index) + .unwrap_or_else(|| panic!("No request state for {index}")); + assert_eq!( + status, "Sampling", + "expected {block_root} {index} request to be ongoing" + ); + } + } + + fn assert_sampling_request_nopeers(&self, block_root: Hash256, indices: &Vec) { + for index in indices { + let status = self + .sync_manager + .get_sampling_request_status(block_root, index) + .unwrap_or_else(|| panic!("No request state for {index}")); + assert_eq!( + status, "NoPeers", + "expected {block_root} {index} request to be nopeers" + ); + } } } @@ -2061,7 +2077,7 @@ fn sampling_batch_requests() { .pop() .unwrap(); assert_eq!(column_indexes.len(), SAMPLING_REQUIRED_SUCCESSES); - r.assert_sampling_request_status(block_root, &column_indexes, &vec![]); + r.assert_sampling_request_ongoing(block_root, &column_indexes); // Resolve the request. r.complete_valid_sampling_column_requests( @@ -2089,7 +2105,7 @@ fn sampling_batch_requests_not_enough_responses_returned() { assert_eq!(column_indexes.len(), SAMPLING_REQUIRED_SUCCESSES); // The request status should be set to Sampling. - r.assert_sampling_request_status(block_root, &column_indexes, &vec![]); + r.assert_sampling_request_ongoing(block_root, &column_indexes); // Split the indexes to simulate the case where the supernode doesn't have the requested column. let (_column_indexes_supernode_does_not_have, column_indexes_to_complete) = @@ -2107,7 +2123,7 @@ fn sampling_batch_requests_not_enough_responses_returned() { ); // The request status should be set to NoPeers since the supernode, the only peer, returned not enough responses. - r.assert_sampling_request_status(block_root, &vec![], &column_indexes); + r.assert_sampling_request_nopeers(block_root, &column_indexes); // The sampling request stalls. r.expect_empty_network(); diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 708c4308b80..29143f22270 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -338,14 +338,12 @@ impl SyncManager { } #[cfg(test)] - pub(crate) fn assert_sampling_request_status( + pub(crate) fn get_sampling_request_status( &self, block_root: Hash256, - ongoing: &Vec, - no_peers: &Vec, - ) { - self.sampling - .assert_sampling_request_status(block_root, ongoing, no_peers); + index: &ColumnIndex, + ) -> Option<&'static str> { + self.sampling.get_request_status(block_root, index) } fn network_globals(&self) -> &NetworkGlobals { diff --git a/beacon_node/network/src/sync/peer_sampling.rs b/beacon_node/network/src/sync/peer_sampling.rs index 086fb0ec8d3..6438b559d03 100644 --- a/beacon_node/network/src/sync/peer_sampling.rs +++ b/beacon_node/network/src/sync/peer_sampling.rs @@ -43,15 +43,15 @@ impl Sampling { } #[cfg(test)] - pub fn assert_sampling_request_status( + pub fn get_request_status( &self, block_root: Hash256, - ongoing: &Vec, - no_peers: &Vec, - ) { + index: &ColumnIndex, + ) -> Option<&'static str> { let requester = SamplingRequester::ImportedBlock(block_root); - let active_sampling_request = self.requests.get(&requester).unwrap(); - active_sampling_request.assert_sampling_request_status(ongoing, no_peers); + self.requests + .get(&requester) + .and_then(|req| req.get_request_status(index)) } /// Create a new sampling request for a known block @@ -233,18 +233,8 @@ impl ActiveSamplingRequest { } #[cfg(test)] - pub fn assert_sampling_request_status( - &self, - ongoing: &Vec, - no_peers: &Vec, - ) { - for idx in ongoing { - assert!(self.column_requests.get(idx).unwrap().is_ongoing()); - } - - for idx in no_peers { - assert!(self.column_requests.get(idx).unwrap().is_no_peers()); - } + pub fn get_request_status(&self, index: &ColumnIndex) -> Option<&'static str> { + self.column_requests.get(index).map(|req| req.status_str()) } /// Insert a downloaded column into an active sampling request. Then make progress on the @@ -575,6 +565,7 @@ mod request { use rand::seq::SliceRandom; use rand::thread_rng; use std::collections::HashSet; + use strum::IntoStaticStr; use types::data_column_sidecar::ColumnIndex; pub(crate) struct ActiveColumnSampleRequest { @@ -584,7 +575,7 @@ mod request { peers_dont_have: HashSet, } - #[derive(Debug, Clone)] + #[derive(Debug, Clone, IntoStaticStr)] enum Status { NoPeers, NotStarted, @@ -630,8 +621,8 @@ mod request { } #[cfg(test)] - pub(crate) fn is_no_peers(&self) -> bool { - matches!(self.status, Status::NoPeers) + pub(crate) fn status_str(&self) -> &'static str { + self.status.clone().into() } pub(crate) fn choose_peer( From 08f5efbcb3f6d860285f192201ba0bc64387afb6 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Thu, 10 Oct 2024 15:30:32 +0300 Subject: [PATCH 2/4] Export status --- .../network/src/sync/block_lookups/tests.rs | 14 ++++++-------- beacon_node/network/src/sync/manager.rs | 2 +- beacon_node/network/src/sync/peer_sampling.rs | 15 +++++++++------ 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index aa54f105f2b..eeb17e4332f 100644 --- a/beacon_node/network/src/sync/block_lookups/tests.rs +++ b/beacon_node/network/src/sync/block_lookups/tests.rs @@ -1309,10 +1309,9 @@ impl TestRig { .sync_manager .get_sampling_request_status(block_root, index) .unwrap_or_else(|| panic!("No request state for {index}")); - assert_eq!( - status, "Sampling", - "expected {block_root} {index} request to be ongoing" - ); + if !matches!(status, crate::sync::peer_sampling::Status::Sampling { .. }) { + panic!("expected {block_root} {index} request to be on going: {status:?}"); + } } } @@ -1322,10 +1321,9 @@ impl TestRig { .sync_manager .get_sampling_request_status(block_root, index) .unwrap_or_else(|| panic!("No request state for {index}")); - assert_eq!( - status, "NoPeers", - "expected {block_root} {index} request to be nopeers" - ); + if !matches!(status, crate::sync::peer_sampling::Status::NoPeers { .. }) { + panic!("expected {block_root} {index} request to be no peers: {status:?}"); + } } } } diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 29143f22270..c3d8dc13b0c 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -342,7 +342,7 @@ impl SyncManager { &self, block_root: Hash256, index: &ColumnIndex, - ) -> Option<&'static str> { + ) -> Option { self.sampling.get_request_status(block_root, index) } diff --git a/beacon_node/network/src/sync/peer_sampling.rs b/beacon_node/network/src/sync/peer_sampling.rs index 6438b559d03..8d26a386739 100644 --- a/beacon_node/network/src/sync/peer_sampling.rs +++ b/beacon_node/network/src/sync/peer_sampling.rs @@ -1,4 +1,6 @@ use self::request::ActiveColumnSampleRequest; +#[cfg(test)] +pub(crate) use self::request::Status; use super::network_context::{ DataColumnsByRootSingleBlockRequest, RpcResponseError, SyncNetworkContext, }; @@ -47,7 +49,7 @@ impl Sampling { &self, block_root: Hash256, index: &ColumnIndex, - ) -> Option<&'static str> { + ) -> Option { let requester = SamplingRequester::ImportedBlock(block_root); self.requests .get(&requester) @@ -233,8 +235,8 @@ impl ActiveSamplingRequest { } #[cfg(test)] - pub fn get_request_status(&self, index: &ColumnIndex) -> Option<&'static str> { - self.column_requests.get(index).map(|req| req.status_str()) + pub fn get_request_status(&self, index: &ColumnIndex) -> Option { + self.column_requests.get(index).map(|req| req.status()) } /// Insert a downloaded column into an active sampling request. Then make progress on the @@ -575,8 +577,9 @@ mod request { peers_dont_have: HashSet, } + // Exposed only for testing assertions in lookup tests #[derive(Debug, Clone, IntoStaticStr)] - enum Status { + pub(crate) enum Status { NoPeers, NotStarted, Sampling(PeerId), @@ -621,8 +624,8 @@ mod request { } #[cfg(test)] - pub(crate) fn status_str(&self) -> &'static str { - self.status.clone().into() + pub(crate) fn status(&self) -> Status { + self.status.clone() } pub(crate) fn choose_peer( From 84e38301fec5670f31fd492e4b1141d8d8abfe56 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Fri, 11 Oct 2024 11:51:26 +0300 Subject: [PATCH 3/4] Drop unused --- beacon_node/network/src/sync/peer_sampling.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/beacon_node/network/src/sync/peer_sampling.rs b/beacon_node/network/src/sync/peer_sampling.rs index 8d26a386739..decabfd2164 100644 --- a/beacon_node/network/src/sync/peer_sampling.rs +++ b/beacon_node/network/src/sync/peer_sampling.rs @@ -567,7 +567,6 @@ mod request { use rand::seq::SliceRandom; use rand::thread_rng; use std::collections::HashSet; - use strum::IntoStaticStr; use types::data_column_sidecar::ColumnIndex; pub(crate) struct ActiveColumnSampleRequest { @@ -578,7 +577,7 @@ mod request { } // Exposed only for testing assertions in lookup tests - #[derive(Debug, Clone, IntoStaticStr)] + #[derive(Debug, Clone)] pub(crate) enum Status { NoPeers, NotStarted, From dd6370b0985c0b179aca818b2ca83f7a4ad92ef8 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Thu, 17 Oct 2024 00:24:40 +0300 Subject: [PATCH 4/4] Use slice --- .../network/src/sync/block_lookups/tests.rs | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index f123ebda0c5..ae9f96a3484 100644 --- a/beacon_node/network/src/sync/block_lookups/tests.rs +++ b/beacon_node/network/src/sync/block_lookups/tests.rs @@ -1319,7 +1319,7 @@ impl TestRig { }); } - fn assert_sampling_request_ongoing(&self, block_root: Hash256, indices: &Vec) { + fn assert_sampling_request_ongoing(&self, block_root: Hash256, indices: &[ColumnIndex]) { for index in indices { let status = self .sync_manager @@ -1331,7 +1331,7 @@ impl TestRig { } } - fn assert_sampling_request_nopeers(&self, block_root: Hash256, indices: &Vec) { + fn assert_sampling_request_nopeers(&self, block_root: Hash256, indices: &[ColumnIndex]) { for index in indices { let status = self .sync_manager @@ -1342,6 +1342,22 @@ impl TestRig { } } } + + fn log_sampling_requests(&self, block_root: Hash256, indices: &[ColumnIndex]) { + let statuses = indices + .iter() + .map(|index| { + let status = self + .sync_manager + .get_sampling_request_status(block_root, index) + .unwrap_or_else(|| panic!("No request state for {index}")); + (index, status) + }) + .collect::>(); + self.log(&format!( + "Sampling request status for {block_root}: {statuses:?}" + )); + } } #[test] @@ -2159,6 +2175,7 @@ fn sampling_batch_requests_not_enough_responses_returned() { ); // The request status should be set to NoPeers since the supernode, the only peer, returned not enough responses. + r.log_sampling_requests(block_root, &column_indexes); r.assert_sampling_request_nopeers(block_root, &column_indexes); // The sampling request stalls.