Add a read-only view of NetworkGraph

Hide the internal locking of NetworkGraph by providing a read-only
view. This way the locking order is handled internally.

Author: jkczyz

lightning/src/routing/network_graph.rs

@@ -58,6 +58,12 @@ pub struct NetworkGraph {
 	nodes: RwLock<BTreeMap<PublicKey, NodeInfo>>,
 }
 
+/// A read-only view of [`NetworkGraph`].
+pub struct ReadOnlyNetworkGraph<'a> {
+	channels: RwLockReadGuard<'a, BTreeMap<u64, ChannelInfo>>,
+	nodes: RwLockReadGuard<'a, BTreeMap<PublicKey, NodeInfo>>,
+}
+
 /// Receives and validates network updates from peers,
 /// stores authentic and relevant data as a network graph.
 /// This network graph is then used for routing payments.
@@ -171,8 +177,8 @@ impl<C: Deref , L: Deref > RoutingMessageHandler for NetGraphMsgHandler<C, L> wh
 
 	fn get_next_channel_announcements(&self, starting_point: u64, batch_amount: u8) -> Vec<(ChannelAnnouncement, Option<ChannelUpdate>, Option<ChannelUpdate>)> {
 		let mut result = Vec::with_capacity(batch_amount as usize);
-		let channels = self.network_graph.get_channels();
-		let mut iter = channels.range(starting_point..);
+		let network_graph = self.network_graph.read_only();
+		let mut iter = network_graph.channels().range(starting_point..);
 		while result.len() < batch_amount as usize {
 			if let Some((_, ref chan)) = iter.next() {
 				if chan.announcement_message.is_some() {
@@ -199,7 +205,8 @@ impl<C: Deref , L: Deref > RoutingMessageHandler for NetGraphMsgHandler<C, L> wh
 
 	fn get_next_node_announcements(&self, starting_point: Option<&PublicKey>, batch_amount: u8) -> Vec<NodeAnnouncement> {
 		let mut result = Vec::with_capacity(batch_amount as usize);
-		let nodes = self.network_graph.get_nodes();
+		let network_graph = self.network_graph.read_only();
+		let nodes = network_graph.nodes();
 		let mut iter = if let Some(pubkey) = starting_point {
 				let mut iter = nodes.range((*pubkey)..);
 				iter.next();
@@ -340,7 +347,8 @@ impl<C: Deref , L: Deref > RoutingMessageHandler for NetGraphMsgHandler<C, L> wh
 		// (has at least one update). A peer may still want to know the channel
 		// exists even if its not yet routable.
 		let mut batches: Vec<Vec<u64>> = vec![Vec::with_capacity(MAX_SCIDS_PER_REPLY)];
-		for (_, ref chan) in self.network_graph.get_channels().range(inclusive_start_scid.unwrap()..exclusive_end_scid.unwrap()) {
+		let network_graph = self.network_graph.read_only();
+		for (_, ref chan) in network_graph.channels().range(inclusive_start_scid.unwrap()..exclusive_end_scid.unwrap()) {
 			if let Some(chan_announcement) = &chan.announcement_message {
 				// Construct a new batch if last one is full
 				if batches.last().unwrap().len() == batches.last().unwrap().capacity() {
@@ -662,34 +670,6 @@ impl PartialEq for NetworkGraph {
 }
 
 impl NetworkGraph {
-	/// Returns all known valid channels' short ids along with announced channel info.
-	///
-	/// (C-not exported) because we have no mapping for `BTreeMap`s
-	pub fn get_channels(&self) -> RwLockReadGuard<'_, BTreeMap<u64, ChannelInfo>> {
-		self.channels.read().unwrap()
-	}
-
-	/// Returns all known nodes' public keys along with announced node info.
-	///
-	/// (C-not exported) because we have no mapping for `BTreeMap`s
-	pub fn get_nodes(&self) -> RwLockReadGuard<'_, BTreeMap<PublicKey, NodeInfo>> {
-		self.nodes.read().unwrap()
-	}
-
-	/// Get network addresses by node id.
-	/// Returns None if the requested node is completely unknown,
-	/// or if node announcement for the node was never received.
-	///
-	/// (C-not exported) as there is no practical way to track lifetimes of returned values.
-	pub fn get_addresses(&self, pubkey: &PublicKey) -> Option<Vec<NetAddress>> {
-		if let Some(node) = self.nodes.read().unwrap().get(pubkey) {
-			if let Some(node_info) = node.announcement_info.as_ref() {
-				return Some(node_info.addresses.clone())
-			}
-		}
-		None
-	}
-
 	/// Creates a new, empty, network graph.
 	pub fn new(genesis_hash: BlockHash) -> NetworkGraph {
 		Self {
@@ -699,6 +679,16 @@ impl NetworkGraph {
 		}
 	}
 
+	/// Returns a read-only view of the network graph.
+	pub fn read_only(&'_ self) -> ReadOnlyNetworkGraph<'_> {
+		let channels = self.channels.read().unwrap();
+		let nodes = self.nodes.read().unwrap();
+		ReadOnlyNetworkGraph {
+			channels,
+			nodes,
+		}
+	}
+
 	/// For an already known node (from channel announcements), update its stored properties from a
 	/// given node announcement.
 	///
@@ -1064,6 +1054,36 @@ impl NetworkGraph {
 	}
 }
 
+impl ReadOnlyNetworkGraph<'_> {
+	/// Returns all known valid channels' short ids along with announced channel info.
+	///
+	/// (C-not exported) because we have no mapping for `BTreeMap`s
+	pub fn channels(&self) -> &BTreeMap<u64, ChannelInfo> {
+		&*self.channels
+	}
+
+	/// Returns all known nodes' public keys along with announced node info.
+	///
+	/// (C-not exported) because we have no mapping for `BTreeMap`s
+	pub fn nodes(&self) -> &BTreeMap<PublicKey, NodeInfo> {
+		&*self.nodes
+	}
+
+	/// Get network addresses by node id.
+	/// Returns None if the requested node is completely unknown,
+	/// or if node announcement for the node was never received.
+	///
+	/// (C-not exported) as there is no practical way to track lifetimes of returned values.
+	pub fn get_addresses(&self, pubkey: &PublicKey) -> Option<&Vec<NetAddress>> {
+		if let Some(node) = self.nodes.get(pubkey) {
+			if let Some(node_info) = node.announcement_info.as_ref() {
+				return Some(&node_info.addresses)
+			}
+		}
+		None
+	}
+}
+
 #[cfg(test)]
 mod tests {
 	use chain;
@@ -1268,7 +1288,7 @@ mod tests {
 
 		{
 			let network = &net_graph_msg_handler.network_graph;
-			match network.get_channels().get(&unsigned_announcement.short_channel_id) {
+			match network.read_only().channels().get(&unsigned_announcement.short_channel_id) {
 				None => panic!(),
 				Some(_) => ()
 			};
@@ -1320,7 +1340,7 @@ mod tests {
 
 		{
 			let network = &net_graph_msg_handler.network_graph;
-			match network.get_channels().get(&unsigned_announcement.short_channel_id) {
+			match network.read_only().channels().get(&unsigned_announcement.short_channel_id) {
 				None => panic!(),
 				Some(_) => ()
 			};
@@ -1351,7 +1371,7 @@ mod tests {
 		};
 		{
 			let network = &net_graph_msg_handler.network_graph;
-			match network.get_channels().get(&unsigned_announcement.short_channel_id) {
+			match network.read_only().channels().get(&unsigned_announcement.short_channel_id) {
 				Some(channel_entry) => {
 					assert_eq!(channel_entry.features, ChannelFeatures::empty());
 				},
@@ -1481,7 +1501,7 @@ mod tests {
 
 		{
 			let network = &net_graph_msg_handler.network_graph;
-			match network.get_channels().get(&short_channel_id) {
+			match network.read_only().channels().get(&short_channel_id) {
 				None => panic!(),
 				Some(channel_info) => {
 					assert_eq!(channel_info.one_to_two.as_ref().unwrap().cltv_expiry_delta, 144);
@@ -1587,7 +1607,7 @@ mod tests {
 		{
 			// There is no nodes in the table at the beginning.
 			let network = &net_graph_msg_handler.network_graph;
-			assert_eq!(network.get_nodes().len(), 0);
+			assert_eq!(network.read_only().nodes().len(), 0);
 		}
 
 		{
@@ -1643,7 +1663,7 @@ mod tests {
 		// Non-permanent closing just disables a channel
 		{
 			let network = &net_graph_msg_handler.network_graph;
-			match network.get_channels().get(&short_channel_id) {
+			match network.read_only().channels().get(&short_channel_id) {
 				None => panic!(),
 				Some(channel_info) => {
 					assert!(channel_info.one_to_two.is_some());
@@ -1661,7 +1681,7 @@ mod tests {
 		// Non-permanent closing just disables a channel
 		{
 			let network = &net_graph_msg_handler.network_graph;
-			match network.get_channels().get(&short_channel_id) {
+			match network.read_only().channels().get(&short_channel_id) {
 				None => panic!(),
 				Some(channel_info) => {
 					assert!(!channel_info.one_to_two.as_ref().unwrap().enabled);
@@ -1679,9 +1699,9 @@ mod tests {
 		// Permanent closing deletes a channel
 		{
 			let network = &net_graph_msg_handler.network_graph;
-			assert_eq!(network.get_channels().len(), 0);
+			assert_eq!(network.read_only().channels().len(), 0);
 			// Nodes are also deleted because there are no associated channels anymore
-			assert_eq!(network.get_nodes().len(), 0);
+			assert_eq!(network.read_only().nodes().len(), 0);
 		}
 		// TODO: Test HTLCFailChannelUpdate::NodeFailure, which is not implemented yet.
 	}
@@ -1998,8 +2018,8 @@ mod tests {
 
 		let network = &net_graph_msg_handler.network_graph;
 		let mut w = test_utils::TestVecWriter(Vec::new());
-		assert!(!network.get_nodes().is_empty());
-		assert!(!network.get_channels().is_empty());
+		assert!(!network.read_only().nodes().is_empty());
+		assert!(!network.read_only().channels().is_empty());
 		network.write(&mut w).unwrap();
 		assert!(<NetworkGraph>::read(&mut io::Cursor::new(&w.0)).unwrap() == *network);
 	}

lightning/src/routing/router.rs

@@ -442,8 +442,9 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 	// to use as the A* heuristic beyond just the cost to get one node further than the current
 	// one.
 
-	let network_channels = network.get_channels();
-	let network_nodes = network.get_nodes();
+	let network_graph = network.read_only();
+	let network_channels = network_graph.channels();
+	let network_nodes = network_graph.nodes();
 	let dummy_directional_info = DummyDirectionalChannelInfo { // used for first_hops routes
 		cltv_expiry_delta: 0,
 		htlc_minimum_msat: 0,
@@ -3858,7 +3859,7 @@ mod tests {
 
 		// First, get 100 (source, destination) pairs for which route-getting actually succeeds...
 		let mut seed = random_init_seed() as usize;
-		let nodes = graph.get_nodes();
+		let nodes = graph.read_only().nodes().clone();
 		'load_endpoints: for _ in 0..10 {
 			loop {
 				seed = seed.overflowing_mul(0xdeadbeef).0;
@@ -3887,7 +3888,7 @@ mod tests {
 
 		// First, get 100 (source, destination) pairs for which route-getting actually succeeds...
 		let mut seed = random_init_seed() as usize;
-		let nodes = graph.get_nodes();
+		let nodes = graph.read_only().nodes().clone();
 		'load_endpoints: for _ in 0..10 {
 			loop {
 				seed = seed.overflowing_mul(0xdeadbeef).0;
@@ -3946,7 +3947,7 @@ mod benches {
 	fn generate_routes(bench: &mut Bencher) {
 		let mut d = test_utils::get_route_file().unwrap();
 		let graph = NetworkGraph::read(&mut d).unwrap();
-		let nodes = graph.get_nodes();
+		let nodes = graph.read_only().nodes().clone();
 
 		// First, get 100 (source, destination) pairs for which route-getting actually succeeds...
 		let mut path_endpoints = Vec::new();
@@ -3978,7 +3979,7 @@ mod benches {
 	fn generate_mpp_routes(bench: &mut Bencher) {
 		let mut d = test_utils::get_route_file().unwrap();
 		let graph = NetworkGraph::read(&mut d).unwrap();
-		let nodes = graph.get_nodes();
+		let nodes = graph.read_only().nodes().clone();
 
 		// First, get 100 (source, destination) pairs for which route-getting actually succeeds...
 		let mut path_endpoints = Vec::new();