From 52ad191c8c61cb6beb16c84bf4b6b651bed628a5 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 23 Dec 2022 14:18:20 +0530 Subject: [PATCH 1/6] GH-647: rename peers_addrs to connection_progress_peers --- node/src/neighborhood/gossip_acceptor.rs | 20 ++++++++++---------- node/src/neighborhood/mod.rs | 10 +++++----- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/node/src/neighborhood/gossip_acceptor.rs b/node/src/neighborhood/gossip_acceptor.rs index 6abdd93e4..a372eabb1 100644 --- a/node/src/neighborhood/gossip_acceptor.rs +++ b/node/src/neighborhood/gossip_acceptor.rs @@ -63,7 +63,7 @@ trait GossipHandler: NamedType + Send /* Send because lazily-written tests requi database: &mut NeighborhoodDatabase, agrs: Vec, gossip_source: SocketAddr, - peer_addrs: &[IpAddr], + connection_progress_peers: &[IpAddr], cpm_recipient: &Recipient, ) -> GossipAcceptanceResult; } @@ -133,7 +133,7 @@ impl GossipHandler for DebutHandler { database: &mut NeighborhoodDatabase, mut agrs: Vec, gossip_source: SocketAddr, - _peer_addrs: &[IpAddr], + _connection_progress_peers: &[IpAddr], _cpm_recipient: &Recipient, ) -> GossipAcceptanceResult { let source_agr = { @@ -510,7 +510,7 @@ impl GossipHandler for PassHandler { database: &mut NeighborhoodDatabase, agrs: Vec, _gossip_source: SocketAddr, - peer_addrs: &[IpAddr], + connection_progress_peers: &[IpAddr], cpm_recipient: &Recipient, ) -> GossipAcceptanceResult { let pass_agr = &agrs[0]; // empty Gossip shouldn't get here @@ -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 @@ -628,7 +628,7 @@ impl GossipHandler for IntroductionHandler { database: &mut NeighborhoodDatabase, agrs: Vec, gossip_source: SocketAddr, - _peer_addrs: &[IpAddr], + _connection_progress_peers: &[IpAddr], cpm_recipient: &Recipient, ) -> GossipAcceptanceResult { if database.root().full_neighbor_keys(database).len() >= MAX_DEGREE { @@ -933,7 +933,7 @@ impl GossipHandler for StandardGossipHandler { database: &mut NeighborhoodDatabase, agrs: Vec, gossip_source: SocketAddr, - _peer_addrs: &[IpAddr], + _connection_progress_peers: &[IpAddr], cpm_recipient: &Recipient, ) -> GossipAcceptanceResult { let initial_neighborship_status = @@ -1191,7 +1191,7 @@ impl GossipHandler for RejectHandler { _database: &mut NeighborhoodDatabase, _agrs: Vec, _gossip_source: SocketAddr, - _peer_addrs: &[IpAddr], + _connection_progress_peers: &[IpAddr], _cpm_recipient: &Recipient, ) -> GossipAcceptanceResult { panic!("Should never be called") @@ -1210,7 +1210,7 @@ pub trait GossipAcceptor: Send /* Send because lazily-written tests require it * database: &mut NeighborhoodDatabase, agrs: Vec, gossip_source: SocketAddr, - peer_addrs: &[IpAddr], + connection_progress_peers: &[IpAddr], ) -> GossipAcceptanceResult; } @@ -1227,7 +1227,7 @@ impl<'a> GossipAcceptor for GossipAcceptorReal<'a> { database: &mut NeighborhoodDatabase, agrs: Vec, gossip_source: SocketAddr, - peer_addrs: &[IpAddr], + connection_progress_peers: &[IpAddr], ) -> GossipAcceptanceResult { let (qualification, handler_ref) = self .gossip_handlers @@ -1247,7 +1247,7 @@ impl<'a> GossipAcceptor for GossipAcceptorReal<'a> { database, agrs, gossip_source, - peer_addrs, + connection_progress_peers, &self.cpm_recipient, ) } diff --git a/node/src/neighborhood/mod.rs b/node/src/neighborhood/mod.rs index 7338dd8bc..e863d2834 100644 --- a/node/src/neighborhood/mod.rs +++ b/node/src/neighborhood/mod.rs @@ -722,7 +722,7 @@ impl Neighborhood { fn handle_agrs(&mut self, agrs: Vec, 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() @@ -731,7 +731,7 @@ impl Neighborhood { &mut self.neighborhood_database, agrs, gossip_source, - &peer_addrs, + &connection_progress_peers, ); match acceptance_result { GossipAcceptanceResult::Accepted => self.gossip_to_neighbors(), @@ -3620,7 +3620,7 @@ mod tests { database: &mut NeighborhoodDatabase, _agrs: Vec, _gossip_source: SocketAddr, - _peer_addrs: &[IpAddr], + _connection_progress_peers: &[IpAddr], ) -> GossipAcceptanceResult { let non_root_database_keys = database .keys() @@ -3852,7 +3852,7 @@ mod tests { database: &mut NeighborhoodDatabase, _agrs: Vec, _gossip_source: SocketAddr, - _peer_addrs: &[IpAddr], + _connection_progress_peers: &[IpAddr], ) -> GossipAcceptanceResult { let half_neighbor_keys = database .root() @@ -5513,7 +5513,7 @@ mod tests { database: &mut NeighborhoodDatabase, agrs: Vec, gossip_source: SocketAddr, - _peer_addrs: &[IpAddr], + _connection_progress_peers: &[IpAddr], ) -> GossipAcceptanceResult { self.handle_params .lock() From 8441883b6708452017220132f2470c347e737fa2 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 23 Dec 2022 14:24:36 +0530 Subject: [PATCH 2/6] GH-647: provide the accurate name inside the constructor of System in the test --- node/src/neighborhood/gossip_acceptor.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/node/src/neighborhood/gossip_acceptor.rs b/node/src/neighborhood/gossip_acceptor.rs index a372eabb1..d3f756f9f 100644 --- a/node/src/neighborhood/gossip_acceptor.rs +++ b/node/src/neighborhood/gossip_acceptor.rs @@ -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( From 6df4afe75ee77a0503af2f172f11ab96f6de4fb6 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 23 Dec 2022 15:00:09 +0530 Subject: [PATCH 3/6] GH-647: reword the trace logs for the unnecessary connection progress --- node/src/neighborhood/mod.rs | 11 ++++++++--- node/src/neighborhood/overall_connection_status.rs | 6 +++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/node/src/neighborhood/mod.rs b/node/src/neighborhood/mod.rs index e863d2834..874dce135 100644 --- a/node/src/neighborhood/mod.rs +++ b/node/src/neighborhood/mod.rs @@ -295,7 +295,11 @@ impl Handler for Neighborhood { } } Err(e) => { - trace!(self.logger, "{}", e); + trace!( + self.logger, + "Found unnecessary connection progress message - {}", + e + ); } } } @@ -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 )); } @@ -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 )); } diff --git a/node/src/neighborhood/overall_connection_status.rs b/node/src/neighborhood/overall_connection_status.rs index 00e7534b1..d786f7c1c 100644 --- a/node/src/neighborhood/overall_connection_status.rs +++ b/node/src/neighborhood/overall_connection_status.rs @@ -247,8 +247,8 @@ impl OverallConnectionStatus { 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 )); } @@ -258,7 +258,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 )) } From fee88f59f69a10704e21f07399bb838c121ecec4 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 23 Dec 2022 16:16:32 +0530 Subject: [PATCH 4/6] GH-647: use a unique number to generate a unique socket address for peer 2 --- node/src/neighborhood/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/src/neighborhood/mod.rs b/node/src/neighborhood/mod.rs index 874dce135..416e0f8d0 100644 --- a/node/src/neighborhood/mod.rs +++ b/node/src/neighborhood/mod.rs @@ -3746,7 +3746,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); From e95368cc9a0fca127b921a971c9f6021093f9de0 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Fri, 23 Dec 2022 16:22:18 +0530 Subject: [PATCH 5/6] GH-647: add review changes for overall_connection_status.rs --- node/src/neighborhood/overall_connection_status.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/node/src/neighborhood/overall_connection_status.rs b/node/src/neighborhood/overall_connection_status.rs index d786f7c1c..7bec57187 100644 --- a/node/src/neighborhood/overall_connection_status.rs +++ b/node/src/neighborhood/overall_connection_status.rs @@ -239,11 +239,7 @@ 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!( @@ -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] From c712a6dca52ccce4d522e4870548b89edb9cb27b Mon Sep 17 00:00:00 2001 From: Utkarsh Gupta <32920299+utkarshg6@users.noreply.github.com> Date: Fri, 30 Dec 2022 18:24:18 +0530 Subject: [PATCH 6/6] GH-647: add connection_progress_peers as a param in the mock (#216) --- node/src/neighborhood/mod.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/node/src/neighborhood/mod.rs b/node/src/neighborhood/mod.rs index 416e0f8d0..b8d09ebbc 100644 --- a/node/src/neighborhood/mod.rs +++ b/node/src/neighborhood/mod.rs @@ -3513,7 +3513,8 @@ 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()); @@ -3521,6 +3522,8 @@ mod tests { 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] @@ -5506,6 +5509,7 @@ mod tests { NeighborhoodDatabase, Vec, SocketAddr, + Vec, )>, >, >, @@ -5518,12 +5522,14 @@ mod tests { database: &mut NeighborhoodDatabase, agrs: Vec, gossip_source: SocketAddr, - _connection_progress_peers: &[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) } } @@ -5544,6 +5550,7 @@ mod tests { NeighborhoodDatabase, Vec, SocketAddr, + Vec, )>, >, >,