Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-647: Review 1 #214

Merged
merged 6 commits into from
Dec 30, 2022
Merged

GH-647: Review 1 #214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
52ad191
GH-647: rename peers_addrs to connection_progress_peers
utkarshg6 Dec 23, 2022
8441883
GH-647: provide the accurate name inside the constructor of System in…
utkarshg6 Dec 23, 2022
6df4afe
GH-647: reword the trace logs for the unnecessary connection progress
utkarshg6 Dec 23, 2022
fee88f5
GH-647: use a unique number to generate a unique socket address for p…
utkarshg6 Dec 23, 2022
e95368c
GH-647: add review changes for overall_connection_status.rs
utkarshg6 Dec 23, 2022
c712a6d
GH-647: add connection_progress_peers as a param in the mock (#216)
utkarshg6 Dec 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 12 additions & 11 deletions node/src/neighborhood/gossip_acceptor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ trait GossipHandler: NamedType + Send /* Send because lazily-written tests requi
database: &mut NeighborhoodDatabase,
agrs: Vec<AccessibleGossipRecord>,
gossip_source: SocketAddr,
peer_addrs: &[IpAddr],
connection_progress_peers: &[IpAddr],
cpm_recipient: &Recipient<ConnectionProgressMessage>,
) -> GossipAcceptanceResult;
}
Expand Down Expand Up @@ -133,7 +133,7 @@ impl GossipHandler for DebutHandler {
database: &mut NeighborhoodDatabase,
mut agrs: Vec<AccessibleGossipRecord>,
gossip_source: SocketAddr,
_peer_addrs: &[IpAddr],
_connection_progress_peers: &[IpAddr],
_cpm_recipient: &Recipient<ConnectionProgressMessage>,
) -> GossipAcceptanceResult {
let source_agr = {
Expand Down Expand Up @@ -510,7 +510,7 @@ impl GossipHandler for PassHandler {
database: &mut NeighborhoodDatabase,
agrs: Vec<AccessibleGossipRecord>,
_gossip_source: SocketAddr,
peer_addrs: &[IpAddr],
connection_progress_peers: &[IpAddr],
cpm_recipient: &Recipient<ConnectionProgressMessage>,
) -> GossipAcceptanceResult {
let pass_agr = &agrs[0]; // empty Gossip shouldn't get here
Expand Down Expand Up @@ -541,7 +541,7 @@ impl GossipHandler for PassHandler {

let mut hash_map = self.previous_pass_targets.borrow_mut();
let gossip_acceptance_result = match hash_map.get_mut(&pass_target_ip_addr) {
None => match peer_addrs.contains(&pass_target_ip_addr) {
None => match connection_progress_peers.contains(&pass_target_ip_addr) {
true => {
send_cpm(ConnectionProgressEvent::PassLoopFound);
GossipAcceptanceResult::Ignored
Expand Down Expand Up @@ -628,7 +628,7 @@ impl GossipHandler for IntroductionHandler {
database: &mut NeighborhoodDatabase,
agrs: Vec<AccessibleGossipRecord>,
gossip_source: SocketAddr,
_peer_addrs: &[IpAddr],
_connection_progress_peers: &[IpAddr],
cpm_recipient: &Recipient<ConnectionProgressMessage>,
) -> GossipAcceptanceResult {
if database.root().full_neighbor_keys(database).len() >= MAX_DEGREE {
Expand Down Expand Up @@ -933,7 +933,7 @@ impl GossipHandler for StandardGossipHandler {
database: &mut NeighborhoodDatabase,
agrs: Vec<AccessibleGossipRecord>,
gossip_source: SocketAddr,
_peer_addrs: &[IpAddr],
_connection_progress_peers: &[IpAddr],
cpm_recipient: &Recipient<ConnectionProgressMessage>,
) -> GossipAcceptanceResult {
let initial_neighborship_status =
Expand Down Expand Up @@ -1191,7 +1191,7 @@ impl GossipHandler for RejectHandler {
_database: &mut NeighborhoodDatabase,
_agrs: Vec<AccessibleGossipRecord>,
_gossip_source: SocketAddr,
_peer_addrs: &[IpAddr],
_connection_progress_peers: &[IpAddr],
_cpm_recipient: &Recipient<ConnectionProgressMessage>,
) -> GossipAcceptanceResult {
panic!("Should never be called")
Expand All @@ -1210,7 +1210,7 @@ pub trait GossipAcceptor: Send /* Send because lazily-written tests require it *
database: &mut NeighborhoodDatabase,
agrs: Vec<AccessibleGossipRecord>,
gossip_source: SocketAddr,
peer_addrs: &[IpAddr],
connection_progress_peers: &[IpAddr],
) -> GossipAcceptanceResult;
}

Expand All @@ -1227,7 +1227,7 @@ impl<'a> GossipAcceptor for GossipAcceptorReal<'a> {
database: &mut NeighborhoodDatabase,
agrs: Vec<AccessibleGossipRecord>,
gossip_source: SocketAddr,
peer_addrs: &[IpAddr],
connection_progress_peers: &[IpAddr],
) -> GossipAcceptanceResult {
let (qualification, handler_ref) = self
.gossip_handlers
Expand All @@ -1247,7 +1247,7 @@ impl<'a> GossipAcceptor for GossipAcceptorReal<'a> {
database,
agrs,
gossip_source,
peer_addrs,
connection_progress_peers,
&self.cpm_recipient,
)
}
Expand Down Expand Up @@ -3495,7 +3495,8 @@ mod tests {
let subject = PassHandler::new();
let (gossip, pass_target, gossip_source) = make_pass(2345);
let pass_target_ip_addr = pass_target.node_addr_opt().unwrap().ip_addr();
let system = System::new("handles_pass_target_that_is_not_yet_expired");
let system =
System::new("handles_pass_target_that_is_a_part_of_a_different_connection_progress");
let (cpm_recipient, recording_arc) = make_cpm_recipient();

let result = subject.handle(
Expand Down
40 changes: 26 additions & 14 deletions node/src/neighborhood/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,11 @@ impl Handler<ConnectionProgressMessage> for Neighborhood {
}
}
Err(e) => {
trace!(self.logger, "{}", e);
trace!(
self.logger,
"Found unnecessary connection progress message - {}",
e
);
}
}
}
Expand Down Expand Up @@ -722,7 +726,7 @@ impl Neighborhood {
fn handle_agrs(&mut self, agrs: Vec<AccessibleGossipRecord>, gossip_source: SocketAddr) {
let ignored_node_name = self.gossip_source_name(&agrs, gossip_source);
let gossip_record_count = agrs.len();
let peer_addrs = self.overall_connection_status.get_peer_addrs();
let connection_progress_peers = self.overall_connection_status.get_peer_addrs();
let acceptance_result = self
.gossip_acceptor_opt
.as_ref()
Expand All @@ -731,7 +735,7 @@ impl Neighborhood {
&mut self.neighborhood_database,
agrs,
gossip_source,
&peer_addrs,
&connection_progress_peers,
);
match acceptance_result {
GossipAcceptanceResult::Accepted => self.gossip_to_neighbors(),
Expand Down Expand Up @@ -1840,7 +1844,7 @@ mod tests {
System::current().stop();
assert_eq!(system.run(), 0);
TestLogHandler::new().exists_log_containing(&format!(
"TRACE: Neighborhood: An unnecessary ConnectionProgressMessage received containing IP Address: {:?}",
"TRACE: Neighborhood: Found unnecessary connection progress message - No peer found with the IP Address: {:?}",
unknown_peer
));
}
Expand Down Expand Up @@ -1887,7 +1891,8 @@ mod tests {
System::current().stop();
assert_eq!(system.run(), 0);
TestLogHandler::new().exists_log_containing(&format!(
"TRACE: Neighborhood: We've been passed to a peer with IP Address: {:?} that's already a part of different connection progress.",
"TRACE: Neighborhood: Found unnecessary connection progress message - Pass target with \
IP Address: {:?} is already a part of different connection progress.",
peer_1
));
}
Expand Down Expand Up @@ -3508,14 +3513,17 @@ mod tests {
System::current().stop();
system.run();
let mut handle_params = handle_params_arc.lock().unwrap();
let (call_database, call_agrs, call_gossip_source) = handle_params.remove(0);
let (call_database, call_agrs, call_gossip_source, connection_progress_peers) =
handle_params.remove(0);
assert!(handle_params.is_empty());
assert_eq!(&subject_node, call_database.root());
assert_eq!(1, call_database.keys().len());
let agrs: Vec<AccessibleGossipRecord> = gossip.try_into().unwrap();
assert_eq!(agrs, call_agrs);
let actual_gossip_source: SocketAddr = subject_node.node_addr_opt().unwrap().into();
assert_eq!(actual_gossip_source, call_gossip_source);
let neighbor_ip = neighbor.node_addr_opt().unwrap().ip_addr();
assert_eq!(connection_progress_peers, vec![neighbor_ip]);
}

#[test]
Expand Down Expand Up @@ -3620,7 +3628,7 @@ mod tests {
database: &mut NeighborhoodDatabase,
_agrs: Vec<AccessibleGossipRecord>,
_gossip_source: SocketAddr,
_peer_addrs: &[IpAddr],
_connection_progress_peers: &[IpAddr],
) -> GossipAcceptanceResult {
let non_root_database_keys = database
.keys()
Expand Down Expand Up @@ -3741,7 +3749,7 @@ mod tests {
.handle_result(GossipAcceptanceResult::Ignored);
let (node_to_ui_recipient, _) = make_node_to_ui_recipient();
let peer_1 = make_node_record(1234, true);
let peer_2 = make_node_record(2345, true);
let peer_2 = make_node_record(6721, true);
let desc_1 = peer_1.node_descriptor(Chain::Dev, main_cryptde());
let desc_2 = peer_2.node_descriptor(Chain::Dev, main_cryptde());
let this_node = make_node_record(7777, true);
Expand Down Expand Up @@ -3852,7 +3860,7 @@ mod tests {
database: &mut NeighborhoodDatabase,
_agrs: Vec<AccessibleGossipRecord>,
_gossip_source: SocketAddr,
_peer_addrs: &[IpAddr],
_connection_progress_peers: &[IpAddr],
) -> GossipAcceptanceResult {
let half_neighbor_keys = database
.root()
Expand Down Expand Up @@ -5501,6 +5509,7 @@ mod tests {
NeighborhoodDatabase,
Vec<AccessibleGossipRecord>,
SocketAddr,
Vec<IpAddr>,
)>,
>,
>,
Expand All @@ -5513,12 +5522,14 @@ mod tests {
database: &mut NeighborhoodDatabase,
agrs: Vec<AccessibleGossipRecord>,
gossip_source: SocketAddr,
_peer_addrs: &[IpAddr],
connection_progress_peers: &[IpAddr],
) -> GossipAcceptanceResult {
self.handle_params
.lock()
.unwrap()
.push((database.clone(), agrs, gossip_source));
self.handle_params.lock().unwrap().push((
database.clone(),
agrs,
gossip_source,
connection_progress_peers.to_vec(),
));
self.handle_results.borrow_mut().remove(0)
}
}
Expand All @@ -5539,6 +5550,7 @@ mod tests {
NeighborhoodDatabase,
Vec<AccessibleGossipRecord>,
SocketAddr,
Vec<IpAddr>,
)>,
>,
>,
Expand Down
17 changes: 7 additions & 10 deletions node/src/neighborhood/overall_connection_status.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,16 +239,12 @@ impl OverallConnectionStatus {
) -> Result<&mut ConnectionProgress, String> {
if let ConnectionProgressEvent::PassGossipReceived(pass_target) = msg.event {
// Check if Pass Target can potentially create a duplicate ConnectionProgress
let is_duplicate = self
.progress
.iter()
.map(|connection_progress| connection_progress.current_peer_addr)
.any(|peer| peer == pass_target);
let is_duplicate = self.get_peer_addrs().contains(&pass_target);

if is_duplicate {
return Err(format!(
"We've been passed to a peer with IP Address: {:?} that's \
already a part of different connection progress.",
"Pass target with IP Address: {:?} is already a \
part of different connection progress.",
pass_target
));
}
Expand All @@ -258,7 +254,7 @@ impl OverallConnectionStatus {
Ok(connection_progress)
} else {
Err(format!(
"An unnecessary ConnectionProgressMessage received containing IP Address: {:?}",
"No peer found with the IP Address: {:?}",
msg.peer_addr
))
}
Expand Down Expand Up @@ -512,14 +508,15 @@ mod tests {
let peer_1 = make_ip(1);
let peer_2 = make_ip(2);
let peer_3 = make_ip(3);

let subject = OverallConnectionStatus::new(vec![
make_node_descriptor(peer_1),
make_node_descriptor(peer_2),
make_node_descriptor(peer_3),
]);

assert_eq!(subject.get_peer_addrs(), vec![peer_1, peer_2, peer_3]);
let result = subject.get_peer_addrs();

assert_eq!(result, vec![peer_1, peer_2, peer_3]);
}

#[test]
Expand Down