From 382341f60022bbe5ac6b8c5120217270f1c7a27c Mon Sep 17 00:00:00 2001 From: behzad nouri Date: Sun, 3 Nov 2024 13:54:24 -0600 Subject: [PATCH] removes CrdsValue.{signature,data} from pub interface --- gossip/src/cluster_info.rs | 32 +++++++++++++------------- gossip/src/crds.rs | 46 +++++++++++++++++++------------------- gossip/src/crds_entry.rs | 8 +++---- gossip/src/crds_gossip.rs | 4 ++-- gossip/src/crds_value.rs | 14 ++++++++++-- 5 files changed, 57 insertions(+), 47 deletions(-) diff --git a/gossip/src/cluster_info.rs b/gossip/src/cluster_info.rs index 9408b0fdcc8760..d8917c68f01629 100644 --- a/gossip/src/cluster_info.rs +++ b/gossip/src/cluster_info.rs @@ -424,7 +424,7 @@ fn retain_staked( drop_unstaked_node_instance: bool, ) { values.retain(|value| { - match value.data { + match value.data() { CrdsData::ContactInfo(_) => true, CrdsData::LegacyContactInfo(_) => true, // May Impact new validators starting up without any stake yet. @@ -1191,7 +1191,7 @@ impl ClusterInfo { .time_gossip_read_lock("get_votes", &self.stats.get_votes) .get_votes(cursor) .map(|vote| { - let CrdsData::Vote(_, vote) = &vote.value.data else { + let CrdsData::Vote(_, vote) = vote.value.data() else { panic!("this should not happen!"); }; vote.transaction().clone() @@ -1211,7 +1211,7 @@ impl ClusterInfo { .get_votes(cursor) .map(|vote| { let label = vote.value.label(); - let CrdsData::Vote(_, vote) = &vote.value.data else { + let CrdsData::Vote(_, vote) = vote.value.data() else { panic!("this should not happen!"); }; (label, vote.transaction().clone()) @@ -1257,7 +1257,7 @@ impl ClusterInfo { let origin = entry.value.pubkey(); gossip_crds.get_shred_version(&origin) == self_shred_version }) - .map(|entry| match &entry.value.data { + .map(|entry| match entry.value.data() { CrdsData::EpochSlots(_, slots) => slots.clone(), _ => panic!("this should not happen!"), }) @@ -1273,7 +1273,7 @@ impl ClusterInfo { gossip_crds .get_entries(cursor) .filter_map(|entry| { - let CrdsData::RestartLastVotedForkSlots(slots) = &entry.value.data else { + let CrdsData::RestartLastVotedForkSlots(slots) = entry.value.data() else { return None; }; (slots.shred_version == self_shred_version).then_some(slots) @@ -1288,7 +1288,7 @@ impl ClusterInfo { gossip_crds .get_entries(cursor) .filter_map(|entry| { - let CrdsData::RestartHeaviestFork(fork) = &entry.value.data else { + let CrdsData::RestartHeaviestFork(fork) = entry.value.data() else { return None; }; (fork.shred_version == self_shred_version).then_some(fork) @@ -1302,7 +1302,7 @@ impl ClusterInfo { let gossip_crds = self.gossip.crds.read().unwrap(); gossip_crds .get_duplicate_shreds(cursor) - .map(|entry| match &entry.value.data { + .map(|entry| match entry.value.data() { CrdsData::DuplicateShred(_, dup) => dup.clone(), _ => panic!("this should not happen!"), }) @@ -2003,7 +2003,7 @@ impl ClusterInfo { requests .into_par_iter() .with_min_len(1024) - .filter(|(_, _, caller)| match &caller.data { + .filter(|(_, _, caller)| match caller.data() { CrdsData::LegacyContactInfo(_) | CrdsData::ContactInfo(_) => { if caller.pubkey() == self_pubkey { warn!("PullRequest ignored, I'm talking to myself"); @@ -2154,7 +2154,7 @@ impl ClusterInfo { } else { score }; - let score = match response.data { + let score = match response.data() { CrdsData::LegacyContactInfo(_) | CrdsData::ContactInfo(_) => 2 * score, _ => score, }; @@ -2479,7 +2479,7 @@ impl ClusterInfo { if should_check_duplicate_instance && values.iter().any(|value| { instance.check_duplicate(value) - || matches!(&value.data, CrdsData::ContactInfo(other) + || matches!(value.data(), CrdsData::ContactInfo(other) if my_contact_info.check_duplicate(other)) }) { @@ -3297,7 +3297,7 @@ fn filter_on_shred_version( // * prevent two running instances of the same identity key cross // contaminate gossip between clusters. if crds.get_shred_version(from) == Some(self_shred_version) { - values.retain(|value| match &value.data { + values.retain(|value| match value.data() { // Allow contact-infos so that shred-versions are updated. CrdsData::ContactInfo(_) => true, CrdsData::LegacyContactInfo(_) => true, @@ -3306,7 +3306,7 @@ fn filter_on_shred_version( _ => crds.get_shred_version(&value.pubkey()) == Some(self_shred_version), }) } else { - values.retain(|value| match &value.data { + values.retain(|value| match value.data() { // Allow node to update its own contact info in case their // shred-version changes CrdsData::ContactInfo(node) => node.pubkey() == from, @@ -3321,7 +3321,7 @@ fn filter_on_shred_version( } }; match &mut msg { - Protocol::PullRequest(_, caller) => match &caller.data { + Protocol::PullRequest(_, caller) => match caller.data() { // Allow spy nodes with shred-verion == 0 to pull from other nodes. CrdsData::LegacyContactInfo(node) if node.shred_version() == 0 || node.shred_version() == self_shred_version => @@ -3373,7 +3373,7 @@ fn verify_gossip_addr( ping_cache: &Mutex, pings: &mut Vec<(SocketAddr, Protocol /* ::PingMessage */)>, ) -> bool { - let (pubkey, addr) = match &value.data { + let (pubkey, addr) = match value.data() { CrdsData::ContactInfo(node) => (node.pubkey(), node.gossip()), CrdsData::LegacyContactInfo(node) => (node.pubkey(), node.gossip()), _ => return true, // If not a contact-info, nothing to verify. @@ -4344,11 +4344,11 @@ mod tests { let mut i = 0; while value.size() < PUSH_MESSAGE_MAX_PAYLOAD_SIZE as u64 { - value.data = CrdsData::AccountsHashes(AccountsHashes { + value = CrdsValue::new_unsigned(CrdsData::AccountsHashes(AccountsHashes { from: Pubkey::default(), hashes: vec![(0, Hash::default()); i], wallclock: 0, - }); + })); i += 1; } let split: Vec<_> = diff --git a/gossip/src/crds.rs b/gossip/src/crds.rs index 087ce66a7455b6..e6291de9368271 100644 --- a/gossip/src/crds.rs +++ b/gossip/src/crds.rs @@ -199,13 +199,13 @@ fn overrides(value: &CrdsValue, other: &VersionedCrdsValue) -> bool { // Contact-infos and node instances are special cased so that if there are // two running instances of the same node, the more recent start is // propagated through gossip regardless of wallclocks. - if let CrdsData::NodeInstance(value) = &value.data { + if let CrdsData::NodeInstance(value) = value.data() { if let Some(out) = value.overrides(&other.value) { return out; } } - if let CrdsData::ContactInfo(value) = &value.data { - if let CrdsData::ContactInfo(other) = &other.value.data { + if let CrdsData::ContactInfo(value) = value.data() { + if let CrdsData::ContactInfo(other) = other.value.data() { if let Some(out) = value.overrides(other) { return out; } @@ -250,7 +250,7 @@ impl Crds { stats.record_insert(&value, route); let entry_index = entry.index(); self.shards.insert(entry_index, &value); - match &value.value.data { + match value.value.data() { CrdsData::ContactInfo(node) => { self.nodes.insert(entry_index); self.shred_versions.insert(pubkey, node.shred_version()); @@ -277,12 +277,12 @@ impl Crds { let entry_index = entry.index(); self.shards.remove(entry_index, entry.get()); self.shards.insert(entry_index, &value); - match &value.value.data { + match value.value.data() { CrdsData::ContactInfo(node) => { self.shred_versions.insert(pubkey, node.shred_version()); // self.nodes does not need to be updated since the // entry at this index was and stays contact-info. - debug_assert_matches!(entry.get().value.data, CrdsData::ContactInfo(_)); + debug_assert_matches!(entry.get().value.data(), CrdsData::ContactInfo(_)); } CrdsData::Vote(_, _) => { self.votes.remove(&entry.get().ordinal); @@ -355,7 +355,7 @@ impl Crds { /// Returns ContactInfo of all known nodes. pub(crate) fn get_nodes_contact_info(&self) -> impl Iterator { - self.get_nodes().map(|v| match &v.value.data { + self.get_nodes().map(|v| match v.value.data() { CrdsData::ContactInfo(info) => info, _ => panic!("this should not happen!"), }) @@ -556,7 +556,7 @@ impl Crds { }; self.purged.push_back((value.value_hash, now)); self.shards.remove(index, &value); - match value.value.data { + match value.value.data() { CrdsData::ContactInfo(_) => { self.nodes.swap_remove(&index); } @@ -592,7 +592,7 @@ impl Crds { let value = self.table.index(index); self.shards.remove(size, value); self.shards.insert(index, value); - match value.value.data { + match value.value.data() { CrdsData::ContactInfo(_) => { self.nodes.swap_remove(&size); self.nodes.insert(index); @@ -695,7 +695,7 @@ impl Default for CrdsDataStats { impl CrdsDataStats { fn record_insert(&mut self, entry: &VersionedCrdsValue, route: GossipRoute) { self.counts[Self::ordinal(entry)] += 1; - if let CrdsData::Vote(_, vote) = &entry.value.data { + if let CrdsData::Vote(_, vote) = entry.value.data() { if let Some(slot) = vote.slot() { let num_nodes = self.votes.get(&slot).copied().unwrap_or_default(); self.votes.put(slot, num_nodes + 1); @@ -706,7 +706,7 @@ impl CrdsDataStats { return; }; - if should_report_message_signature(&entry.value.signature) { + if should_report_message_signature(entry.value.signature()) { datapoint_info!( "gossip_crds_sample", ( @@ -716,7 +716,7 @@ impl CrdsDataStats { ), ( "signature", - entry.value.signature.to_string().get(..8), + entry.value.signature().to_string().get(..8), Option ), ( @@ -733,7 +733,7 @@ impl CrdsDataStats { } fn ordinal(entry: &VersionedCrdsValue) -> usize { - match &entry.value.data { + match entry.value.data() { CrdsData::LegacyContactInfo(_) => 0, CrdsData::Vote(_, _) => 1, CrdsData::LowestSlot(_, _) => 2, @@ -1159,7 +1159,7 @@ mod tests { .table .values() .filter(|v| v.ordinal >= since) - .filter(|v| matches!(v.value.data, CrdsData::EpochSlots(_, _))) + .filter(|v| matches!(v.value.data(), CrdsData::EpochSlots(_, _))) .count(); let mut cursor = Cursor(since); assert_eq!(num_epoch_slots, crds.get_epoch_slots(&mut cursor).count()); @@ -1174,13 +1174,13 @@ mod tests { ); for value in crds.get_epoch_slots(&mut Cursor(since)) { assert!(value.ordinal >= since); - assert_matches!(value.value.data, CrdsData::EpochSlots(_, _)); + assert_matches!(value.value.data(), CrdsData::EpochSlots(_, _)); } let num_votes = crds .table .values() .filter(|v| v.ordinal >= since) - .filter(|v| matches!(v.value.data, CrdsData::Vote(_, _))) + .filter(|v| matches!(v.value.data(), CrdsData::Vote(_, _))) .count(); let mut cursor = Cursor(since); assert_eq!(num_votes, crds.get_votes(&mut cursor).count()); @@ -1188,7 +1188,7 @@ mod tests { cursor.0, crds.table .values() - .filter(|v| matches!(v.value.data, CrdsData::Vote(_, _))) + .filter(|v| matches!(v.value.data(), CrdsData::Vote(_, _))) .map(|v| v.ordinal) .max() .map(|k| k + 1) @@ -1197,7 +1197,7 @@ mod tests { ); for value in crds.get_votes(&mut Cursor(since)) { assert!(value.ordinal >= since); - assert_matches!(value.value.data, CrdsData::Vote(_, _)); + assert_matches!(value.value.data(), CrdsData::Vote(_, _)); } let num_entries = crds .table @@ -1221,17 +1221,17 @@ mod tests { let num_nodes = crds .table .values() - .filter(|v| matches!(v.value.data, CrdsData::ContactInfo(_))) + .filter(|v| matches!(v.value.data(), CrdsData::ContactInfo(_))) .count(); let num_votes = crds .table .values() - .filter(|v| matches!(v.value.data, CrdsData::Vote(_, _))) + .filter(|v| matches!(v.value.data(), CrdsData::Vote(_, _))) .count(); let num_epoch_slots = crds .table .values() - .filter(|v| matches!(v.value.data, CrdsData::EpochSlots(_, _))) + .filter(|v| matches!(v.value.data(), CrdsData::EpochSlots(_, _))) .count(); assert_eq!( crds.table.len(), @@ -1244,10 +1244,10 @@ mod tests { crds.get_epoch_slots(&mut Cursor::default()).count() ); for vote in crds.get_votes(&mut Cursor::default()) { - assert_matches!(vote.value.data, CrdsData::Vote(_, _)); + assert_matches!(vote.value.data(), CrdsData::Vote(_, _)); } for epoch_slots in crds.get_epoch_slots(&mut Cursor::default()) { - assert_matches!(epoch_slots.value.data, CrdsData::EpochSlots(_, _)); + assert_matches!(epoch_slots.value.data(), CrdsData::EpochSlots(_, _)); } (num_nodes, num_votes, num_epoch_slots) } diff --git a/gossip/src/crds_entry.rs b/gossip/src/crds_entry.rs index 9c37b264cc987f..33ab68cd40e164 100644 --- a/gossip/src/crds_entry.rs +++ b/gossip/src/crds_entry.rs @@ -37,7 +37,7 @@ macro_rules! impl_crds_entry ( type Key = Pubkey; fn get_entry(table:&'a CrdsTable, key: Self::Key) -> Option { let key = CrdsValueLabel::$name(key); - match &table.get(&key)?.value.data { + match table.get(&key)?.value.data() { $pat => Some($expr), _ => None, } @@ -47,7 +47,7 @@ macro_rules! impl_crds_entry ( ); // Lookup by CrdsValueLabel. -impl_crds_entry!(CrdsData, |entry| Some(&entry?.value.data)); +impl_crds_entry!(CrdsData, |entry| Some(entry?.value.data())); impl_crds_entry!(CrdsValue, |entry| Some(&entry?.value)); impl_crds_entry!(VersionedCrdsValue, |entry| entry); @@ -98,10 +98,10 @@ mod tests { for entry in entries.values() { let key = entry.label(); assert_eq!(crds.get::<&CrdsValue>(&key), Some(entry)); - assert_eq!(crds.get::<&CrdsData>(&key), Some(&entry.data)); + assert_eq!(crds.get::<&CrdsData>(&key), Some(entry.data())); assert_eq!(crds.get::<&VersionedCrdsValue>(&key).unwrap().value, *entry); let key = entry.pubkey(); - match &entry.data { + match entry.data() { CrdsData::ContactInfo(node) => { assert_eq!(crds.get::<&ContactInfo>(key), Some(node)) } diff --git a/gossip/src/crds_gossip.rs b/gossip/src/crds_gossip.rs index 984a3547081c2a..0d100687a651cd 100644 --- a/gossip/src/crds_gossip.rs +++ b/gossip/src/crds_gossip.rs @@ -101,7 +101,7 @@ impl CrdsGossip { let mut crds = self.crds.write().unwrap(); if crds .get_records(&pubkey) - .any(|value| match &value.value.data { + .any(|value| match value.value.data() { CrdsData::DuplicateShred(_, value) => value.slot == shred_slot, _ => false, }) @@ -121,7 +121,7 @@ impl CrdsGossip { let mut num_dup_shreds = 0; let offset = crds .get_records(&pubkey) - .filter_map(|value| match &value.value.data { + .filter_map(|value| match value.value.data() { CrdsData::DuplicateShred(ix, value) => { num_dup_shreds += 1; Some((value.wallclock, *ix)) diff --git a/gossip/src/crds_value.rs b/gossip/src/crds_value.rs index 83d690555af379..9da9d30d904e44 100644 --- a/gossip/src/crds_value.rs +++ b/gossip/src/crds_value.rs @@ -44,8 +44,8 @@ pub const MAX_EPOCH_SLOTS: EpochSlotsIndex = 255; #[cfg_attr(feature = "frozen-abi", derive(AbiExample))] #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)] pub struct CrdsValue { - pub signature: Signature, - pub data: CrdsData, + signature: Signature, + data: CrdsData, } impl Sanitize for CrdsValue { @@ -605,6 +605,16 @@ impl CrdsValue { } } + #[inline] + pub(crate) fn signature(&self) -> &Signature { + &self.signature + } + + #[inline] + pub(crate) fn data(&self) -> &CrdsData { + &self.data + } + /// Totally unsecure unverifiable wallclock of the node that generated this message /// Latest wallclock is always picked. /// This is used to time out push messages.