simln-lib/bugfix: return all nodes from ln_node_from_graph

39257da introduced bug where we
would not return all `SimNode`s from ln_node_from_graph because
we were using the `SimGraph` which does not have the excluded
nodes, if any. Here we use the pathfinding graph for the nodes
list which would include all the nodes.

Author: elnosh

sim-cli/src/parsing.rs

@@ -301,7 +301,7 @@ pub async fn create_simulation_with_network(
     ));
 
     // Copy all simulated channels into a read-only routing graph, allowing to pathfind for
-    // individual payments without locking th simulation graph (this is a duplication of the channels,
+    // individual payments without locking the simulation graph (this is a duplication of the channels,
     // but the performance tradeoff is worthwhile for concurrent pathfinding).
     let routing_graph = Arc::new(
         populate_network_graph(channels, clock.clone())
@@ -312,7 +312,7 @@ pub async fn create_simulation_with_network(
     // custom actions on the simulated network. For the nodes we'll pass our simulation, cast them
     // to a dyn trait and exclude any nodes that shouldn't be included in random activity
     // generation.
-    let nodes = ln_node_from_graph(simulation_graph.clone(), routing_graph, clock.clone()).await?;
+    let nodes = ln_node_from_graph(simulation_graph, routing_graph, clock.clone()).await?;
     let mut nodes_dyn: HashMap<_, Arc<Mutex<dyn LightningNode>>> = nodes
         .iter()
         .map(|(pk, node)| (*pk, Arc::clone(node) as Arc<Mutex<dyn LightningNode>>))

simln-lib/src/sim_node.rs

@@ -798,7 +798,16 @@ impl<T: SimNetwork, C: Clock> LightningNode for SimNode<T, C> {
     }
 
     async fn channel_capacities(&self) -> Result<u64, LightningError> {
-        let channels = self.network.lock().await.lookup_node(&self.info.pubkey)?;
+        let channels = match self.network.lock().await.lookup_node(&self.info.pubkey) {
+            Ok(channels) => channels,
+            Err(e) => match e {
+                // This is a bit weird in that a user could have a SimNode but if it was excluded
+                // from sending payments, it won't be returned from the lookup_node call. In that
+                // case, we return a capacity of 0 instead of returning a node not found error.
+                LightningError::GetNodeInfoError(_) => return Ok(0),
+                _ => return Err(e),
+            },
+        };
         Ok(channels.1.iter().sum())
     }
 
@@ -1102,13 +1111,46 @@ pub async fn ln_node_from_graph<C: Clock>(
     routing_graph: Arc<LdkNetworkGraph>,
     clock: Arc<C>,
 ) -> Result<HashMap<PublicKey, Arc<Mutex<SimNode<SimGraph, C>>>>, LightningError> {
-    let mut nodes: HashMap<PublicKey, Arc<Mutex<SimNode<SimGraph, C>>>> = HashMap::new();
+    let network_graph = routing_graph.read_only();
+    let mut nodes: HashMap<PublicKey, Arc<Mutex<SimNode<SimGraph, C>>>> =
+        HashMap::with_capacity(network_graph.nodes().len());
+
+    // We want to return all the nodes even if they were excluded in the SimGraph. So we iterate
+    // over the routing_graph that should include all nodes.
+    for node in network_graph.nodes().unordered_iter() {
+        let node_pubkey = node
+            .0
+            .as_pubkey()
+            .map_err(|e| LightningError::GetInfoError(e.to_string()))?;
+
+        let current_node_info = network_graph.node(node.0);
+        debug_assert!(current_node_info.is_some());
+        debug_assert!(current_node_info.unwrap().channels.len() > 0);
+        let node_channel = current_node_info.unwrap().channels[0];
+
+        let channel_info = graph
+            .lock()
+            .await
+            .channels
+            .lock()
+            .await
+            .get(&node_channel.into())
+            .cloned();
 
-    for node in graph.lock().await.nodes.iter() {
+        debug_assert!(channel_info.is_some());
+        let channel_info = channel_info.unwrap();
+
+        let node_alias = if channel_info.node_1.policy.pubkey == node_pubkey {
+            channel_info.node_1.policy.alias.clone()
+        } else {
+            channel_info.node_2.policy.alias.clone()
+        };
+
+        let info = node_info(node_pubkey, node_alias);
         nodes.insert(
-            *node.0,
+            node_pubkey,
             Arc::new(Mutex::new(SimNode::new(
-                node.1 .0.clone(),
+                info,
                 graph.clone(),
                 routing_graph.clone(),
                 clock.clone(),
@@ -1966,19 +2008,25 @@ mod tests {
             .await
             .unwrap();
 
+        assert!(nodes.len() == 3);
+
         let node_1 = nodes.get(&pk1).unwrap().lock().await;
         let node_1_capacity = node_1.channel_capacities().await.unwrap();
 
-        // Node 1 has 2 channels but one was excluded so here we should only have the one that was
-        // not excluded.
+        // Node 1 has 2 channels but one was excluded so here we should only have the capacity of
+        // the channel that was not excluded.
         assert!(node_1_capacity == capacity_1);
 
         let node_2 = nodes.get(&pk2).unwrap().lock().await;
         let node_2_capacity = node_2.channel_capacities().await.unwrap();
         assert!(node_2_capacity == capacity_1);
 
-        // Node 3's only channel was excluded so it won't be present here.
-        assert!(!nodes.contains_key(&pk3));
+        // Node 3 should be returned from ln_node_from_graph but it won't have any channel capacity
+        // present because its only channel was excluded.
+        let node_3 = nodes.get(&pk3);
+        assert!(node_3.is_some());
+        let node_3 = node_3.unwrap().lock().await;
+        assert!(node_3.channel_capacities().await.unwrap() == 0);
     }
 
     /// Tests basic functionality of a `SimulatedChannel` but does no endeavor to test the underlying