Only push latest snapshot hashes in SnapshotGossipManager #31154
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,7 @@ use { | |
| solana_gossip::cluster_info::ClusterInfo, | ||
| solana_runtime::{ | ||
| snapshot_hash::{ | ||
| FullSnapshotHash, FullSnapshotHashes, SnapshotHash, StartingSnapshotHashes, | ||
| }, | ||
| snapshot_package::{retain_max_n_elements, SnapshotType}, | ||
| }, | ||
|
|
@@ -14,10 +13,9 @@ use { | |
| /// Manage pushing snapshot hash information to gossip | ||
| pub struct SnapshotGossipManager { | ||
| cluster_info: Arc<ClusterInfo>, | ||
| latest_snapshot_hashes: Option<LatestSnapshotHashes>, | ||
| max_full_snapshot_hashes: usize, | ||
| legacy_full_snapshot_hashes: FullSnapshotHashes, | ||
| } | ||
|
|
||
| impl SnapshotGossipManager { | ||
|
|
@@ -26,120 +24,153 @@ impl SnapshotGossipManager { | |
| pub fn new( | ||
| cluster_info: Arc<ClusterInfo>, | ||
| max_full_snapshot_hashes: usize, | ||
| _max_incremental_snapshot_hashes: usize, | ||
|
There was a problem hiding this comment. I've left this parameter here to make the diff smaller. I have a subsequent PR that removes it. |
||
| ) -> Self { | ||
| SnapshotGossipManager { | ||
| cluster_info, | ||
| latest_snapshot_hashes: None, | ||
| max_full_snapshot_hashes, | ||
| legacy_full_snapshot_hashes: FullSnapshotHashes { | ||
| hashes: Vec::default(), | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| /// Push any starting snapshot hashes to the cluster via CRDS | ||
| pub fn push_starting_snapshot_hashes( | ||
| &mut self, | ||
| starting_snapshot_hashes: Option<StartingSnapshotHashes>, | ||
| ) { | ||
| let Some(starting_snapshot_hashes) = starting_snapshot_hashes else { | ||
| return; | ||
| }; | ||
|
Comment on lines
+44
to
+46
There was a problem hiding this comment. This function was originally coded before the There was a problem hiding this comment. Anything that reduces nesting gets a thumbs up from me |
||
|
|
||
| self.update_latest_full_snapshot_hash(starting_snapshot_hashes.full.hash); | ||
| if let Some(starting_incremental_snapshot_hash) = starting_snapshot_hashes.incremental { | ||
| self.update_latest_incremental_snapshot_hash( | ||
| starting_incremental_snapshot_hash.hash, | ||
| starting_incremental_snapshot_hash.base.0, | ||
| ); | ||
| } | ||
| self.push_latest_snapshot_hashes_to_cluster(); | ||
|
|
||
| // Handle legacy snapshot hashes here too | ||
| // Once LegacySnapshotHashes are removed from CRDS, also remove them here | ||
| self.push_legacy_full_snapshot_hash(starting_snapshot_hashes.full); | ||
| } | ||
|
|
||
| /// Push new snapshot hash to the cluster via CRDS | ||
| pub fn push_snapshot_hash( | ||
bw-solana marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| &mut self, | ||
| snapshot_type: SnapshotType, | ||
| snapshot_hash: (Slot, SnapshotHash), | ||
| ) { | ||
| match snapshot_type { | ||
| SnapshotType::FullSnapshot => { | ||
| self.push_full_snapshot_hash(snapshot_hash); | ||
| } | ||
| SnapshotType::IncrementalSnapshot(base_slot) => { | ||
|
Comment on lines
-76
to
-80
There was a problem hiding this comment. This assert is moved, not deleted. (Now is in |
||
| self.push_incremental_snapshot_hash(snapshot_hash, base_slot); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Push new full snapshot hash to the cluster via CRDS | ||
| fn push_full_snapshot_hash(&mut self, full_snapshot_hash: (Slot, SnapshotHash)) { | ||
| self.update_latest_full_snapshot_hash(full_snapshot_hash); | ||
| self.push_latest_snapshot_hashes_to_cluster(); | ||
|
|
||
| // Handle legacy snapshot hashes here too | ||
| // Once LegacySnapshotHashes are removed from CRDS, also remove them here | ||
| self.push_legacy_full_snapshot_hash(FullSnapshotHash { | ||
| hash: full_snapshot_hash, | ||
| }); | ||
| } | ||
|
|
||
| /// Push new incremental snapshot hash to the cluster via CRDS | ||
| fn push_incremental_snapshot_hash( | ||
| &mut self, | ||
| incremental_snapshot_hash: (Slot, SnapshotHash), | ||
| base_slot: Slot, | ||
| ) { | ||
| self.update_latest_incremental_snapshot_hash(incremental_snapshot_hash, base_slot); | ||
| self.push_latest_snapshot_hashes_to_cluster(); | ||
| } | ||
|
|
||
| /// Update the latest snapshot hashes with a new full snapshot | ||
| fn update_latest_full_snapshot_hash(&mut self, full_snapshot_hash: (Slot, SnapshotHash)) { | ||
| self.latest_snapshot_hashes = Some(LatestSnapshotHashes { | ||
| full: full_snapshot_hash, | ||
| incremental: None, | ||
|
There was a problem hiding this comment. If we've gotten a new full snapshot, we know the cannot be any incremental snapshots yet (based on this full snapshot). There was a problem hiding this comment. This might be nice as a comment in the code There was a problem hiding this comment. Opened #31171 to handle this request. |
||
| }); | ||
| } | ||
|
|
||
| /// Update the latest snapshot hashes with a new incremental snapshot | ||
| fn update_latest_incremental_snapshot_hash( | ||
| &mut self, | ||
| incremental_snapshot_hash: (Slot, SnapshotHash), | ||
| base_slot: Slot, | ||
| ) { | ||
| let latest_snapshot_hashes = self | ||
| .latest_snapshot_hashes | ||
| .as_mut() | ||
| .expect("there must already be a full snapshot hash"); | ||
|
Comment on lines
+114
to
+117
There was a problem hiding this comment. And the other side of the same coin is here. In order to get a new incremental snapshot, there must already be a full snapshot to base the incremental on. |
||
| assert_eq!( | ||
| base_slot, latest_snapshot_hashes.full.0, | ||
| "the incremental snapshot's base slot ({}) must match the latest full snapshot's slot ({})", | ||
| base_slot, latest_snapshot_hashes.full.0, | ||
| ); | ||
| latest_snapshot_hashes.incremental = Some(incremental_snapshot_hash); | ||
|
There was a problem hiding this comment. Since we only care about the latest snapshots, we don't need to push to a vector and then trim the size. Just directly set the new value. |
||
| } | ||
|
|
||
| /// Push the latest snapshot hashes to the cluster via CRDS | ||
| fn push_latest_snapshot_hashes_to_cluster(&self) { | ||
|
There was a problem hiding this comment. The impl of this function looks similar to the bottom half of the old |
||
| let Some(latest_snapshot_hashes) = self.latest_snapshot_hashes.as_ref() else { | ||
| return; | ||
| }; | ||
|
|
||
| // Pushing snapshot hashes to the cluster should never fail. The only error case is when | ||
| // the length of the incremental hashes is too big, (and we send a maximum of one here). | ||
| // If this call ever does error, it's a programmer bug! Check to see what changed in | ||
| // `push_snapshot_hashes()` and handle the new error condition here. | ||
| self.cluster_info | ||
| .push_snapshot_hashes( | ||
| clone_hash_for_crds(&latest_snapshot_hashes.full), | ||
| latest_snapshot_hashes | ||
| .incremental | ||
bw-solana marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| .iter() | ||
| .map(clone_hash_for_crds) | ||
| .collect(), | ||
| ) | ||
| .expect( | ||
| "Bug! The programmer contract has changed for push_snapshot_hashes() \ | ||
| and a new error case has been added that has not been handled here.", | ||
| ); | ||
| } | ||
|
|
||
| /// Add `full_snapshot_hash` to the vector of full snapshot hashes, then push that vector to | ||
| /// the cluster via CRDS. | ||
| fn push_legacy_full_snapshot_hash(&mut self, full_snapshot_hash: FullSnapshotHash) { | ||
|
There was a problem hiding this comment. This fn is unchanged from the old |
||
| self.legacy_full_snapshot_hashes | ||
| .hashes | ||
| .push(full_snapshot_hash.hash); | ||
|
|
||
| retain_max_n_elements( | ||
| &mut self.legacy_full_snapshot_hashes.hashes, | ||
| self.max_full_snapshot_hashes, | ||
| ); | ||
|
|
||
| self.cluster_info | ||
| .push_legacy_snapshot_hashes(clone_hashes_for_crds( | ||
| &self.legacy_full_snapshot_hashes.hashes, | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, Copy, Clone, Eq, PartialEq)] | ||
| struct LatestSnapshotHashes { | ||
| full: (Slot, SnapshotHash), | ||
| incremental: Option<(Slot, SnapshotHash)>, | ||
| } | ||
|
|
||
| /// Clones and maps snapshot hashes into what CRDS expects | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually both of these fields will go away. The exist currently to support
LegacySnapshotHashes.