Add a read-only view of NetworkGraph
authorJeffrey Czyz <jkczyz@gmail.com>
Mon, 16 Aug 2021 23:40:19 +0000 (18:40 -0500)
committerJeffrey Czyz <jkczyz@gmail.com>
Fri, 10 Sep 2021 04:11:12 +0000 (23:11 -0500)
Hide the internal locking of NetworkGraph by providing a read-only
view. This way the locking order is handled internally.

lightning/src/routing/network_graph.rs
lightning/src/routing/router.rs

index 2d5051166be38f127080b2364c9182fe2359a7c6..61e94e0494c4e48cbe67c489b3f9b93315042026 100644 (file)
@@ -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,7 +177,7 @@ 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 channels = self.network_graph.channels.read().unwrap();
                let mut iter = channels.range(starting_point..);
                while result.len() < batch_amount as usize {
                        if let Some((_, ref chan)) = iter.next() {
@@ -199,7 +205,7 @@ 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 nodes = self.network_graph.nodes.read().unwrap();
                let mut iter = if let Some(pubkey) = starting_point {
                                let mut iter = nodes.range((*pubkey)..);
                                iter.next();
@@ -340,7 +346,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 channels = self.network_graph.channels.read().unwrap();
+               for (_, ref chan) in 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() {
@@ -351,6 +358,7 @@ impl<C: Deref , L: Deref > RoutingMessageHandler for NetGraphMsgHandler<C, L> wh
                                batch.push(chan_announcement.contents.short_channel_id);
                        }
                }
+               drop(channels);
 
                let mut pending_events = self.pending_events.lock().unwrap();
                let batch_count = batches.len();
@@ -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);
        }
index 6d6d603c56a377383a64f268b44634c190f4fdf9..770b62d9cd2e1e9598cd312dd8137def71eab1bb 100644 (file)
@@ -443,8 +443,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,
@@ -4213,7 +4214,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;
@@ -4242,7 +4243,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;
@@ -4301,7 +4302,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();
@@ -4333,7 +4334,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();