From a3a611a32b78aa06faf7d5497b2fc6d3ecad443e Mon Sep 17 00:00:00 2001 From: pmnoxx Date: Tue, 30 Nov 2021 08:57:43 -0800 Subject: [PATCH] re: refactor `near-network` and `near-network-primitives` (#5507) We should fix `near-network` and `near-network-primitives` issues. --- chain/network-primitives/src/types.rs | 4 +- chain/network/src/peer/peer_actor.rs | 4 +- .../src/peer_manager/peer_manager_actor.rs | 15 +++-- chain/network/src/routing/ibf.rs | 6 +- chain/network/src/routing/ibf_peer_set.rs | 42 +++++++------- chain/network/src/routing/ibf_set.rs | 2 +- .../src/routing/routing_table_actor.rs | 55 ++++++++----------- chain/network/src/tests/cache.rs | 10 ++-- 8 files changed, 64 insertions(+), 74 deletions(-) diff --git a/chain/network-primitives/src/types.rs b/chain/network-primitives/src/types.rs index 7daf9ad2e84..6a559e2f5d2 100644 --- a/chain/network-primitives/src/types.rs +++ b/chain/network-primitives/src/types.rs @@ -369,7 +369,7 @@ impl AccountOrPeerIdOrHash { match self { AccountOrPeerIdOrHash::AccountId(_) => None, AccountOrPeerIdOrHash::PeerId(peer_id) => Some(PeerIdOrHash::PeerId(peer_id.clone())), - AccountOrPeerIdOrHash::Hash(hash) => Some(PeerIdOrHash::Hash(hash.clone())), + AccountOrPeerIdOrHash::Hash(hash) => Some(PeerIdOrHash::Hash(*hash)), } } } @@ -443,7 +443,7 @@ impl RoutedMessage { } pub fn verify(&self) -> bool { - self.signature.verify(self.hash().as_ref(), &self.author.public_key()) + self.signature.verify(self.hash().as_ref(), self.author.public_key()) } pub fn expect_response(&self) -> bool { diff --git a/chain/network/src/peer/peer_actor.rs b/chain/network/src/peer/peer_actor.rs index 0f780f0a1c2..0132512669e 100644 --- a/chain/network/src/peer/peer_actor.rs +++ b/chain/network/src/peer/peer_actor.rs @@ -555,7 +555,7 @@ impl PeerActor { /// Check whenever we exceeded number of transactions we got since last block. /// If so, drop the transaction. fn should_we_drop_msg_without_decoding(&self, msg: &Vec) -> bool { - if codec::is_forward_transaction(&msg).unwrap_or(false) { + if codec::is_forward_transaction(msg).unwrap_or(false) { let r = self.txns_since_last_block.load(Ordering::Acquire); if r > MAX_TRANSACTIONS_PER_BLOCK_MESSAGE { return true; @@ -587,7 +587,7 @@ impl PeerActor { }, )); } else { - info!(target: "network", "Received invalid data {:?} from {}: {}", logging::pretty_vec(&msg), self.peer_info, err); + info!(target: "network", "Received invalid data {:?} from {}: {}", logging::pretty_vec(msg), self.peer_info, err); } } } diff --git a/chain/network/src/peer_manager/peer_manager_actor.rs b/chain/network/src/peer_manager/peer_manager_actor.rs index a6b00075116..6cbd267ad55 100644 --- a/chain/network/src/peer_manager/peer_manager_actor.rs +++ b/chain/network/src/peer_manager/peer_manager_actor.rs @@ -537,7 +537,7 @@ impl PeerManagerActor { peer_protocol_version, { self.initialize_routing_table_exchange(peer_id, peer_type, addr.clone(), ctx); - self.send_sync(peer_type, addr, ctx, target_peer_id.clone(), new_edge, Vec::new()); + self.send_sync(peer_type, addr, ctx, target_peer_id, new_edge, Vec::new()); return; } ); @@ -2231,20 +2231,19 @@ impl Handler for PeerManagerActor { #[cfg(feature = "test_features")] #[cfg(feature = "protocol_feature_routing_exchange_algorithm")] PeerManagerMessageRequest::StartRoutingTableSync(msg) => { - PeerManagerMessageResponse::StartRoutingTableSync( - self.handle_msg_start_routing_table_sync(msg, ctx), - ) + self.handle_msg_start_routing_table_sync(msg, ctx); + PeerManagerMessageResponse::StartRoutingTableSync(()) } #[cfg(feature = "test_features")] PeerManagerMessageRequest::SetAdvOptions(msg) => { - PeerManagerMessageResponse::SetAdvOptions(self.handle_msg_set_adv_options(msg, ctx)) + self.handle_msg_set_adv_options(msg, ctx); + PeerManagerMessageResponse::SetAdvOptions(()) } #[cfg(feature = "test_features")] #[cfg(feature = "protocol_feature_routing_exchange_algorithm")] PeerManagerMessageRequest::SetRoutingTable(msg) => { - PeerManagerMessageResponse::SetRoutingTable( - self.handle_msg_set_routing_table(msg, ctx), - ) + self.handle_msg_set_routing_table(msg, ctx); + PeerManagerMessageResponse::SetRoutingTable(()) } } } diff --git a/chain/network/src/routing/ibf.rs b/chain/network/src/routing/ibf.rs index d2f92296345..2f4bd41e342 100644 --- a/chain/network/src/routing/ibf.rs +++ b/chain/network/src/routing/ibf.rs @@ -151,7 +151,7 @@ impl Ibf { } return Err("unable to recover result"); } - return Ok(result); + Ok(result) } /// Try to recover elements inserted into IBF. @@ -190,7 +190,7 @@ impl Ibf { let mask = (1 << self.k) - 1; let pos0 = elem_hash & mask; let mut pos1 = (elem_hash >> self.k) & mask; - let mut pos2 = (elem_hash >> 2 * self.k) & mask; + let mut pos2 = (elem_hash >> (2 * self.k)) & mask; if pos1 >= pos0 { pos1 = (pos1 + 1) & mask; } @@ -243,7 +243,7 @@ mod tests { #[test] fn create_blt_test() { - let set = 1000000_3_00000u64..1000000_301000u64; + let set = 1_000_000_300_000_u64..1_000_000_301_000_u64; assert_eq!(1000, create_blt(set, 2048).recover().unwrap().len()) } diff --git a/chain/network/src/routing/ibf_peer_set.rs b/chain/network/src/routing/ibf_peer_set.rs index 33ede0b6a73..ff42f5183f0 100644 --- a/chain/network/src/routing/ibf_peer_set.rs +++ b/chain/network/src/routing/ibf_peer_set.rs @@ -18,7 +18,7 @@ pub const MAX_IBF_LEVEL: ValidIBFLevel = ValidIBFLevel(17); /// Represents IbfLevel from 10 to 17. impl ValidIBFLevel { pub fn inc(&self) -> Option { - if self.0 + 1 >= MIN_IBF_LEVEL.0 && self.0 + 1 <= MAX_IBF_LEVEL.0 { + if self.0 + 1 >= MIN_IBF_LEVEL.0 && self.0 < MAX_IBF_LEVEL.0 { Some(ValidIBFLevel(self.0 + 1)) } else { None @@ -26,7 +26,7 @@ impl ValidIBFLevel { } pub fn is_valid(&self) -> bool { - return self.0 >= MIN_IBF_LEVEL.0 && self.0 <= MAX_IBF_LEVEL.0; + self.0 >= MIN_IBF_LEVEL.0 && self.0 <= MAX_IBF_LEVEL.0 } } @@ -112,7 +112,7 @@ impl IbfPeerSet { } } let seed = ibf_set.get_seed(); - self.peers.insert(peer_id.clone(), ibf_set); + self.peers.insert(peer_id, ibf_set); seed } @@ -138,7 +138,7 @@ impl IbfPeerSet { if let Some(_id) = self.slot_map.pop(edge) { self.edges -= 1; for (_, val) in self.peers.iter_mut() { - val.remove_edge(&edge); + val.remove_edge(edge); } return true; } @@ -168,7 +168,7 @@ impl IbfPeerSet { mod test { use crate::routing::edge::{Edge, SimpleEdge}; use crate::routing::ibf_peer_set::ValidIBFLevel; - use crate::routing::ibf_peer_set::{IbfPeerSet, SlotMap, SlotMapId}; + use crate::routing::ibf_peer_set::{IbfPeerSet, SlotMap}; use crate::routing::ibf_set::IbfSet; use crate::test_utils::random_peer_id; use near_primitives::network::PeerId; @@ -180,37 +180,37 @@ mod test { let p1 = random_peer_id(); let p2 = random_peer_id(); - let e0 = SimpleEdge::new(p0.clone(), p1.clone(), 0); + let e0 = SimpleEdge::new(p0, p1.clone(), 0); let e1 = SimpleEdge::new(p1.clone(), p2.clone(), 0); - let e2 = SimpleEdge::new(p1.clone(), p2.clone(), 3); + let e2 = SimpleEdge::new(p1, p2, 3); let mut sm = SlotMap::default(); - assert_eq!(0 as SlotMapId, sm.insert(&e0).unwrap()); + assert_eq!(0_u64, sm.insert(&e0).unwrap()); assert!(sm.insert(&e0).is_none()); - assert_eq!(1 as SlotMapId, sm.insert(&e1).unwrap()); - assert_eq!(2 as SlotMapId, sm.insert(&e2).unwrap()); + assert_eq!(1_u64, sm.insert(&e1).unwrap()); + assert_eq!(2_u64, sm.insert(&e2).unwrap()); - assert_eq!(Some(2 as SlotMapId), sm.pop(&e2)); + assert_eq!(Some(2_u64), sm.pop(&e2)); assert_eq!(None, sm.pop(&e2)); - assert_eq!(Some(0 as SlotMapId), sm.pop(&e0)); + assert_eq!(Some(0_u64), sm.pop(&e0)); assert_eq!(None, sm.pop(&e0)); - assert_eq!(Some(1 as SlotMapId), sm.get(&e1)); + assert_eq!(Some(1_u64), sm.get(&e1)); - assert_eq!(Some(e1.clone()), sm.get_by_id(&(1 as SlotMapId))); - assert_eq!(None, sm.get_by_id(&(1000 as SlotMapId))); + assert_eq!(Some(e1.clone()), sm.get_by_id(&1_u64)); + assert_eq!(None, sm.get_by_id(&1000_u64)); - assert_eq!(Some(1 as SlotMapId), sm.pop(&e1)); + assert_eq!(Some(1_u64), sm.pop(&e1)); assert_eq!(None, sm.get(&e1)); assert_eq!(None, sm.pop(&e1)); - assert_eq!(3 as SlotMapId, sm.insert(&e2).unwrap()); - assert_eq!(Some(3 as SlotMapId), sm.pop(&e2)); + assert_eq!(3_u64, sm.insert(&e2).unwrap()); + assert_eq!(Some(3_u64), sm.pop(&e2)); - assert_eq!(None, sm.get_by_id(&(1 as SlotMapId))); - assert_eq!(None, sm.get_by_id(&(1000 as SlotMapId))); + assert_eq!(None, sm.get_by_id(&1_u64)); + assert_eq!(None, sm.get_by_id(&1000_u64)); } #[test] @@ -238,7 +238,7 @@ mod test { ips.add_peer(peer_id.clone(), Some(1111), &mut edges_info); // Add edge - let e = SimpleEdge::new(peer_id.clone(), peer_id2.clone(), 111); + let e = SimpleEdge::new(peer_id.clone(), peer_id2, 111); let se = ips.add_edge(&e).unwrap(); ibf_set.add_edge(&e, se); assert!(ips.add_edge(&e).is_none()); diff --git a/chain/network/src/routing/ibf_set.rs b/chain/network/src/routing/ibf_set.rs index 12aba2db2ce..ed81a921665 100644 --- a/chain/network/src/routing/ibf_set.rs +++ b/chain/network/src/routing/ibf_set.rs @@ -118,7 +118,7 @@ mod test { a.remove_edge(&(i as u64)); } for i in 0..10000 { - b.add_edge(&(i + 100 as u64), (i + 2000000) as SlotMapId); + b.add_edge(&(i + 100_u64), (i + 2000000) as SlotMapId); } for i in 10..=17 { let mut ibf1 = a.get_ibf(ValidIBFLevel(i)); diff --git a/chain/network/src/routing/routing_table_actor.rs b/chain/network/src/routing/routing_table_actor.rs index 88285d2d1f9..9290e673afb 100644 --- a/chain/network/src/routing/routing_table_actor.rs +++ b/chain/network/src/routing/routing_table_actor.rs @@ -530,7 +530,7 @@ impl RoutingTableActor { } let (edge_hashes, unknown_edges_count) = new_ibf.try_recover(); - let (known, unknown_edges) = self.split_edges_for_peer(&peer_id, &edge_hashes); + let (known, unknown_edges) = self.split_edges_for_peer(peer_id, &edge_hashes); (known, unknown_edges, unknown_edges_count) } @@ -605,8 +605,7 @@ impl Handler for RoutingTableActor { } #[cfg(feature = "protocol_feature_routing_exchange_algorithm")] RoutingTableMessages::AddPeerIfMissing(peer_id, ibf_set) => { - let seed = - self.peer_ibf_set.add_peer(peer_id.clone(), ibf_set, &mut self.edges_info); + let seed = self.peer_ibf_set.add_peer(peer_id, ibf_set, &mut self.edges_info); RoutingTableMessagesResponse::AddPeerResponse { seed } } #[cfg(feature = "protocol_feature_routing_exchange_algorithm")] @@ -631,11 +630,11 @@ impl Handler for RoutingTableActor { let edges_for_peer = edges_for_peer .iter() - .filter_map(|x| self.edges_info.get(&x.key()).cloned()) + .filter_map(|x| self.edges_info.get(x.key()).cloned()) .collect(); // Prepare message let ibf_msg = if unknown_edges_count == 0 - && unknown_edge_hashes.len() > 0 + && !unknown_edge_hashes.is_empty() { crate::types::RoutingVersion2 { known_edges: self.edges_info.len() as u64, @@ -645,38 +644,32 @@ impl Handler for RoutingTableActor { unknown_edge_hashes, ), } - } else if unknown_edges_count == 0 && unknown_edge_hashes.len() == 0 { + } else if unknown_edges_count == 0 && unknown_edge_hashes.is_empty() { crate::types::RoutingVersion2 { known_edges: self.edges_info.len() as u64, seed, edges: edges_for_peer, routing_state: crate::types::RoutingState::Done, } + } else if let Some(new_ibf_level) = partial_sync.ibf_level.inc() { + let ibf_vec = ibf_set.get_ibf_vec(new_ibf_level); + crate::types::RoutingVersion2 { + known_edges: self.edges_info.len() as u64, + seed, + edges: edges_for_peer, + routing_state: crate::types::RoutingState::PartialSync( + crate::types::PartialSync { + ibf_level: new_ibf_level, + ibf: ibf_vec, + }, + ), + } } else { - if let Some(new_ibf_level) = partial_sync.ibf_level.inc() { - let ibf_vec = ibf_set.get_ibf_vec(new_ibf_level); - crate::types::RoutingVersion2 { - known_edges: self.edges_info.len() as u64, - seed, - edges: edges_for_peer, - routing_state: crate::types::RoutingState::PartialSync( - crate::types::PartialSync { - ibf_level: new_ibf_level, - ibf: ibf_vec, - }, - ), - } - } else { - crate::types::RoutingVersion2 { - known_edges: self.edges_info.len() as u64, - seed, - edges: self - .edges_info - .iter() - .map(|x| x.1.clone()) - .collect(), - routing_state: crate::types::RoutingState::RequestAllEdges, - } + crate::types::RoutingVersion2 { + known_edges: self.edges_info.len() as u64, + seed, + edges: self.edges_info.iter().map(|x| x.1.clone()).collect(), + routing_state: crate::types::RoutingState::RequestAllEdges, } }; RoutingTableMessagesResponse::ProcessIbfMessageResponse { @@ -722,7 +715,7 @@ impl Handler for RoutingTableActor { let edges_for_peer = edges_for_peer .iter() - .filter_map(|x| self.edges_info.get(&x.key()).cloned()) + .filter_map(|x| self.edges_info.get(x.key()).cloned()) .collect(); let ibf_msg = crate::types::RoutingVersion2 { diff --git a/chain/network/src/tests/cache.rs b/chain/network/src/tests/cache.rs index f7101a7716b..8988480f6ec 100644 --- a/chain/network/src/tests/cache.rs +++ b/chain/network/src/tests/cache.rs @@ -53,7 +53,7 @@ fn dont_load_on_build() { let announce0 = AnnounceAccount { account_id: "near0".parse().unwrap(), peer_id: peer_id0.clone(), - epoch_id: epoch_id0.clone(), + epoch_id: epoch_id0, signature: Signature::default(), }; @@ -68,9 +68,7 @@ fn dont_load_on_build() { routing_table.add_account(announce0.clone()); routing_table.add_account(announce1.clone()); let accounts = routing_table.get_announce_accounts(); - assert!(vec![announce0.clone(), announce1.clone()] - .iter() - .all(|announce| { accounts.contains(announce) })); + assert!(vec![announce0, announce1].iter().all(|announce| { accounts.contains(announce) })); assert_eq!(routing_table.get_announce_accounts().len(), 2); let mut routing_table1 = RoutingTableView::new(peer_id0, store); @@ -85,12 +83,12 @@ fn load_from_disk() { let epoch_id0 = random_epoch_id(); let mut routing_table = RoutingTableView::new(peer_id0.clone(), store.clone()); - let mut routing_table1 = RoutingTableView::new(peer_id0.clone(), store.clone()); + let mut routing_table1 = RoutingTableView::new(peer_id0.clone(), store); let announce0 = AnnounceAccount { account_id: "near0".parse().unwrap(), peer_id: peer_id0.clone(), - epoch_id: epoch_id0.clone(), + epoch_id: epoch_id0, signature: Signature::default(), };