From b281d01221344dc66c05d61b3ab71331dd06a455 Mon Sep 17 00:00:00 2001 From: Tim Gretler Date: Thu, 3 Aug 2023 09:26:06 +0000 Subject: [PATCH] fix: Advertise all available states --- rs/interfaces/src/state_sync_client.rs | 4 ++-- rs/p2p/state_sync_manager/src/lib.rs | 21 +++++++++++++++++---- rs/p2p/state_sync_manager/src/metrics.rs | 13 +++++++++---- rs/p2p/state_sync_manager/src/ongoing.rs | 2 +- rs/p2p/state_sync_manager/tests/common.rs | 8 ++++---- rs/state_manager/src/state_sync.rs | 6 +++--- 6 files changed, 36 insertions(+), 18 deletions(-) diff --git a/rs/interfaces/src/state_sync_client.rs b/rs/interfaces/src/state_sync_client.rs index 1bc81dc5aa2..86a92a7e318 100644 --- a/rs/interfaces/src/state_sync_client.rs +++ b/rs/interfaces/src/state_sync_client.rs @@ -5,8 +5,8 @@ use ic_types::{ }; pub trait StateSyncClient: Send + Sync { - /// Returns the Id of the latest available state or None if no state is available. - fn latest_state(&self) -> Option; + /// Returns a list of all states available. + fn available_states(&self) -> Vec; /// Initiates new state sync for the specified Id. Returns None if the state should not be synced. /// If `Some(..)` is returned a new state sync is initiated. /// Callers of this interface need to uphold the following: `start_state_sync` is not called again diff --git a/rs/p2p/state_sync_manager/src/lib.rs b/rs/p2p/state_sync_manager/src/lib.rs index dcd1282aa9f..0680c73e8f8 100644 --- a/rs/p2p/state_sync_manager/src/lib.rs +++ b/rs/p2p/state_sync_manager/src/lib.rs @@ -170,10 +170,23 @@ impl StateSyncManager { } fn handle_advert_tick(&mut self) { - if let Some(state_id) = self.state_sync.latest_state() { - self.metrics - .latest_state_height_broadcasted - .set(state_id.height.get() as i64); + let available_states = self.state_sync.available_states(); + self.metrics.lowest_state_broadcasted.set( + available_states + .iter() + .map(|h| h.height.get()) + .min() + .unwrap_or_default() as i64, + ); + self.metrics.highest_state_broadcasted.set( + available_states + .iter() + .map(|h| h.height.get()) + .max() + .unwrap_or_default() as i64, + ); + + for state_id in available_states { // Unreliable broadcast of adverts to all current peers. for peer_id in self.transport.peers() { let request = build_advert_handler_request(state_id.clone()); diff --git a/rs/p2p/state_sync_manager/src/metrics.rs b/rs/p2p/state_sync_manager/src/metrics.rs index b9e4e4e2864..0a02e522590 100644 --- a/rs/p2p/state_sync_manager/src/metrics.rs +++ b/rs/p2p/state_sync_manager/src/metrics.rs @@ -20,7 +20,8 @@ const CHUNK_DOWNLOAD_STATUS_SUCCESS: &str = "success"; pub(crate) struct StateSyncManagerMetrics { pub state_syncs_total: IntCounter, pub adverts_received_total: IntCounter, - pub latest_state_height_broadcasted: IntGauge, + pub highest_state_broadcasted: IntGauge, + pub lowest_state_broadcasted: IntGauge, pub ongoing_state_sync_metrics: OngoingStateSyncMetrics, } @@ -35,9 +36,13 @@ impl StateSyncManagerMetrics { "state_sync_manager_adverts_received_total", "Total number of adverts received.", ), - latest_state_height_broadcasted: metrics_registry.int_gauge( - "state_sync_manager_latest_state_height_broadcasted", - "State height that was last broadcasted.", + highest_state_broadcasted: metrics_registry.int_gauge( + "state_sync_manager_highest_state_broadcasted", + "Highest state height broadcasted.", + ), + lowest_state_broadcasted: metrics_registry.int_gauge( + "state_sync_manager_lowest_state_broadcasted", + "Lowest state height broadcasted.", ), ongoing_state_sync_metrics: OngoingStateSyncMetrics::new(metrics_registry), } diff --git a/rs/p2p/state_sync_manager/src/ongoing.rs b/rs/p2p/state_sync_manager/src/ongoing.rs index 634675f8275..c4825ecc757 100644 --- a/rs/p2p/state_sync_manager/src/ongoing.rs +++ b/rs/p2p/state_sync_manager/src/ongoing.rs @@ -368,7 +368,7 @@ mod tests { pub StateSync {} impl StateSyncClient for StateSync { - fn latest_state(&self) -> Option; + fn available_states(&self) -> Vec; fn start_state_sync( &self, diff --git a/rs/p2p/state_sync_manager/tests/common.rs b/rs/p2p/state_sync_manager/tests/common.rs index dfc07ae7e24..af34169fc06 100644 --- a/rs/p2p/state_sync_manager/tests/common.rs +++ b/rs/p2p/state_sync_manager/tests/common.rs @@ -182,14 +182,14 @@ impl FakeStateSync { } impl StateSyncClient for FakeStateSync { - fn latest_state(&self) -> Option { + fn available_states(&self) -> Vec { if self.disconnected.load(Ordering::SeqCst) { - return None; + return vec![]; } if self.uses_global() { - Some(self.global_state.artifact_id()) + vec![self.global_state.artifact_id()] } else { - Some(self.local_state.artifact_id()) + vec![self.local_state.artifact_id()] } } diff --git a/rs/state_manager/src/state_sync.rs b/rs/state_manager/src/state_sync.rs index 08c70013b38..2c003e66676 100644 --- a/rs/state_manager/src/state_sync.rs +++ b/rs/state_manager/src/state_sync.rs @@ -357,16 +357,16 @@ impl ArtifactProcessor for StateSync { impl StateSyncClient for StateSync { /// Non-blocking. - fn latest_state(&self) -> Option { + fn available_states(&self) -> Vec { // Using height 0 here is sane because for state sync `get_all_validated_by_filter` // return at most the number of states present on the node. Currently this is usually 1-2. let filter = StateSyncFilter { height: Height::from(0), }; self.get_all_validated_by_filter(&filter) - .last() - .cloned() + .into_iter() .map(|a| a.id) + .collect() } /// Non-blocking.