return all `SimNode`s from `ln_node_from_graph` #306

elnosh · 2025-09-23T14:15:05Z

Changes:

repurpose list_channels to channel_capacities in the LightningNode trait since this call was only used to determine how much the node will send per month.
fixes bug introduced in 39257da where it would not return all the nodes (the ones with excluded channels). Even if we are excluding channels from nodes, we still want to return SimNodes so that a user could use them.

carlaKC

Feel like we haven't quite hit the right API for this, but stuggling to think of a better approach - let's chat about it irl.

carlaKC · 2025-09-24T13:41:14Z

simln-lib/src/lib.rs

+    /// Sum of channel capacities. It will be used to determine how much the node will send per
+    /// month.


nit: only used if running with random activity generation

carlaKC · 2025-09-24T13:44:53Z

simln-lib/src/sim_node.rs

+            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())


nit: can inline this

match self.network.lock().await.lookup_node(&self.info.pubkey) { Ok(channels) => Ok(channels.1.iter().sum()), 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(_) => Ok(0), _ => Err(e), }, }

carlaKC · 2025-09-24T13:50:50Z

simln-lib/src/sim_node.rs

+    // 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() {


Can't we still use graph here? Looking at where we call ln_node_from_graph, we only filter the excluded nodes after getting the node list.

This whole function can probably go back to what it was.

carlaKC

tl;dr:

SimGraph: has all of the nodes in the network, some have channels that are marked as exclude. We filter our capacity by the exclude field.
RoutingGraph: has all of the nodes, that's it.
ln_node_from_graph: returns the full list of nodes, so we can use any one we want for misc purposes.
Simulation::new: delete any nodes that we want to exclude because we don't want them sending/receiving.

Removing from the list of nodes that we give the simulator stops them from being eligible for sending/receiving payments. Having the exclude field stop their peers from over estimating the amount they should send/receive, while still allowing us to forward payments over the excluded channels.

carlaKC · 2025-09-24T18:02:15Z

simln-lib/src/sim_node.rs

+        let channels = self.network.lock().await.lookup_node(&self.info.pubkey)?;
+        Ok(channels.1.iter().sum())


Here we should throw a node not found error if one is not in the list.

We can filter the channels by exclude to get the balance, so that we get a zero balance.

carlaKC · 2025-09-24T18:03:35Z

simln-lib/src/sim_node.rs

+                // 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),


No implicit zero, fail if we don't find the node like in the previous commit.

carlaKC · 2025-09-24T18:04:45Z

simln-lib/src/sim_node.rs

+    // 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() {


This whole function can probably go back to what it was.

Currently, list_channels is only used for returning a list of channel capacities and using that to determine how much the node should send in a month. Change it to channel_capacities that will return the sum of the capacities of the channels that should be taken into account.

elnosh · 2025-09-24T20:01:11Z

simln-lib/src/sim_node.rs


 /// SimNetwork represents a high level network coordinator that is responsible for the task of actually propagating
 /// payments through the simulated network.
+#[async_trait]


had to make this an async_trait because of tokio mutex :( un-yay

elnosh · 2025-09-24T20:02:49Z

addressed feedback - storing all nodes in SimGraph but excluding capacity based on exclude field in SimulatedChannel

carlaKC · 2025-09-25T14:04:13Z

simln-lib/src/sim_node.rs

+            Some(node) => node.clone(),
+            None => {
+                return Err(LightningError::GetNodeInfoError(
+                    "Node not found".to_string(),


nit: include pubkey of node for debugging

39257da introduced bug where we would not return all `SimNode`s from `ln_node_from_graph` because `SimGraph` did not have the excluded nodes. Here we store all the nodes but exclude them from the capacity based on the exclude field in the `SimulatedChannel`.

elnosh force-pushed the fix-excluded-nodes branch from 6e9cda4 to 4462d7b Compare September 23, 2025 14:23

elnosh requested a review from carlaKC September 23, 2025 14:46

carlaKC reviewed Sep 24, 2025

View reviewed changes

elnosh force-pushed the fix-excluded-nodes branch 3 times, most recently from a949210 to 1851075 Compare September 24, 2025 19:58

elnosh commented Sep 24, 2025

View reviewed changes

carlaKC approved these changes Sep 25, 2025

View reviewed changes

elnosh force-pushed the fix-excluded-nodes branch from 1851075 to bfb24fd Compare September 25, 2025 22:03

carlaKC merged commit 66daf69 into bitcoin-dev-project:main Sep 26, 2025
2 checks passed

		/// Sum of channel capacities. It will be used to determine how much the node will send per
		/// month.

		let channels = self.network.lock().await.lookup_node(&self.info.pubkey)?;
		Ok(channels.1.iter().sum())

return all SimNodes from ln_node_from_graph #306

return all SimNodes from ln_node_from_graph #306

Uh oh!

Conversation

elnosh commented Sep 23, 2025

Uh oh!

carlaKC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlaKC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elnosh commented Sep 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

return all `SimNode`s from `ln_node_from_graph` #306

return all `SimNode`s from `ln_node_from_graph` #306