removes manual trait impl for contact-info (#17332)

The current implementations use only the id and disregard other fields,
in particular wallclock. This can lead to bugs where an outdated
contact-info shadows or overrides a current one because they compare
equal.
This commit is contained in:
behzad nouri
2021-05-19 20:56:10 +00:00
committed by GitHub
parent 477898f682
commit 13b032b2d4
3 changed files with 21 additions and 49 deletions

View File

@ -6,11 +6,10 @@ use solana_sdk::sanitize::{Sanitize, SanitizeError};
#[cfg(test)] #[cfg(test)]
use solana_sdk::signature::{Keypair, Signer}; use solana_sdk::signature::{Keypair, Signer};
use solana_sdk::timing::timestamp; use solana_sdk::timing::timestamp;
use std::cmp::{Ord, Ordering, PartialEq, PartialOrd};
use std::net::{IpAddr, SocketAddr}; use std::net::{IpAddr, SocketAddr};
/// Structure representing a node on the network /// Structure representing a node on the network
#[derive(Serialize, Deserialize, Clone, Debug, AbiExample)] #[derive(Clone, Debug, Eq, Ord, PartialEq, PartialOrd, AbiExample, Deserialize, Serialize)]
pub struct ContactInfo { pub struct ContactInfo {
pub id: Pubkey, pub id: Pubkey,
/// gossip address /// gossip address
@ -48,34 +47,13 @@ impl Sanitize for ContactInfo {
} }
} }
impl Ord for ContactInfo {
fn cmp(&self, other: &Self) -> Ordering {
self.id.cmp(&other.id)
}
}
impl PartialOrd for ContactInfo {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(self.cmp(other))
}
}
impl PartialEq for ContactInfo {
fn eq(&self, other: &Self) -> bool {
self.id == other.id
}
}
impl Eq for ContactInfo {}
#[macro_export] #[macro_export]
macro_rules! socketaddr { macro_rules! socketaddr {
($ip:expr, $port:expr) => { ($ip:expr, $port:expr) => {
std::net::SocketAddr::from((std::net::Ipv4Addr::from($ip), $port)) std::net::SocketAddr::from((std::net::Ipv4Addr::from($ip), $port))
}; };
($str:expr) => {{ ($str:expr) => {{
let a: std::net::SocketAddr = $str.parse().unwrap(); $str.parse::<std::net::SocketAddr>().unwrap()
a
}}; }};
} }
#[macro_export] #[macro_export]

View File

@ -19,7 +19,7 @@
//! CrdsValue enums. //! CrdsValue enums.
//! //!
//! Merge strategy is implemented in: //! Merge strategy is implemented in:
//! impl PartialOrd for VersionedCrdsValue //! fn overrides(value: &CrdsValue, other: &VersionedCrdsValue) -> bool
//! //!
//! A value is updated to a new version if the labels match, and the value //! A value is updated to a new version if the labels match, and the value
//! wallclock is later, or the value hash is greater. //! wallclock is later, or the value hash is greater.
@ -66,8 +66,6 @@ pub enum CrdsError {
} }
/// This structure stores some local metadata associated with the CrdsValue /// This structure stores some local metadata associated with the CrdsValue
/// The implementation of PartialOrd ensures that the "highest" version is always picked to be
/// stored in the Crds
#[derive(PartialEq, Debug, Clone)] #[derive(PartialEq, Debug, Clone)]
pub struct VersionedCrdsValue { pub struct VersionedCrdsValue {
/// Ordinal index indicating insert order. /// Ordinal index indicating insert order.

View File

@ -753,36 +753,32 @@ mod test {
#[test] #[test]
fn test_personalized_push_messages() { fn test_personalized_push_messages() {
let now = timestamp(); let now = timestamp();
let mut rng = rand::thread_rng();
let mut crds = Crds::default(); let mut crds = Crds::default();
let mut push = CrdsGossipPush::default(); let mut push = CrdsGossipPush::default();
let peer_1 = CrdsValue::new_unsigned(CrdsData::ContactInfo(ContactInfo::new_localhost( let peers: Vec<_> = vec![0, 0, now]
&solana_sdk::pubkey::new_rand(), .into_iter()
0, .map(|wallclock| {
))); let mut peer = ContactInfo::new_rand(&mut rng, /*pubkey=*/ None);
assert_eq!(crds.insert(peer_1.clone(), now), Ok(None)); peer.wallclock = wallclock;
let peer_2 = CrdsValue::new_unsigned(CrdsData::ContactInfo(ContactInfo::new_localhost( CrdsValue::new_unsigned(CrdsData::ContactInfo(peer))
&solana_sdk::pubkey::new_rand(), })
0, .collect();
))); assert_eq!(crds.insert(peers[0].clone(), now), Ok(None));
assert_eq!(crds.insert(peer_2.clone(), now), Ok(None)); assert_eq!(crds.insert(peers[1].clone(), now), Ok(None));
let peer_3 = CrdsValue::new_unsigned(CrdsData::ContactInfo(ContactInfo::new_localhost(
&solana_sdk::pubkey::new_rand(),
now,
)));
assert_eq!( assert_eq!(
push.process_push_message(&mut crds, &Pubkey::default(), peer_3.clone(), now), push.process_push_message(&mut crds, &Pubkey::default(), peers[2].clone(), now),
Ok(None) Ok(None)
); );
push.refresh_push_active_set(&crds, &HashMap::new(), None, &Pubkey::default(), 0, 1, 1); push.refresh_push_active_set(&crds, &HashMap::new(), None, &Pubkey::default(), 0, 1, 1);
// push 3's contact info to 1 and 2 and 3 // push 3's contact info to 1 and 2 and 3
let new_msg = CrdsValue::new_unsigned(CrdsData::ContactInfo(ContactInfo::new_localhost( let expected: HashMap<_, _> = vec![
&peer_3.pubkey(), (peers[0].pubkey(), vec![peers[2].clone()]),
0, (peers[1].pubkey(), vec![peers[2].clone()]),
))); ]
let mut expected = HashMap::new(); .into_iter()
expected.insert(peer_1.pubkey(), vec![new_msg.clone()]); .collect();
expected.insert(peer_2.pubkey(), vec![new_msg]);
assert_eq!(push.active_set.len(), 3); assert_eq!(push.active_set.len(), 3);
assert_eq!(push.new_push_messages(&crds, now), expected); assert_eq!(push.new_push_messages(&crds, now), expected);
} }