return all SimNodes from ln_node_from_graph#306
return all SimNodes from ln_node_from_graph#306carlaKC merged 2 commits intobitcoin-dev-project:mainfrom
SimNodes from ln_node_from_graph#306Conversation
elnosh
force-pushed
the
fix-excluded-nodes
branch
from
6e9cda4 to
4462d7b
Compare
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
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.
simln-lib/src/lib.rs
Outdated
| /// Sum of channel capacities. It will be used to determine how much the node will send per | ||
| /// month. |
There was a problem hiding this comment.
nit: only used if running with random activity generation
simln-lib/src/sim_node.rs
Outdated
| 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()) |
There was a problem hiding this comment.
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),
},
}
simln-lib/src/sim_node.rs
Outdated
| // 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() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This whole function can probably go back to what it was.
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
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.
simln-lib/src/sim_node.rs
Outdated
| let channels = self.network.lock().await.lookup_node(&self.info.pubkey)?; | ||
| Ok(channels.1.iter().sum()) |
There was a problem hiding this comment.
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.
simln-lib/src/sim_node.rs
Outdated
| // 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), |
There was a problem hiding this comment.
No implicit zero, fail if we don't find the node like in the previous commit.
simln-lib/src/sim_node.rs
Outdated
| // 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() { |
There was a problem hiding this comment.
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
force-pushed
the
fix-excluded-nodes
branch
3 times, most recently
from
a949210 to
1851075
Compare
|
|
||
| /// SimNetwork represents a high level network coordinator that is responsible for the task of actually propagating | ||
| /// payments through the simulated network. | ||
| #[async_trait] |
There was a problem hiding this comment.
had to make this an async_trait because of tokio mutex :( un-yay
|
addressed feedback - storing all nodes in |
simln-lib/src/sim_node.rs
Outdated
| Some(node) => node.clone(), | ||
| None => { | ||
| return Err(LightningError::GetNodeInfoError( | ||
| "Node not found".to_string(), |
There was a problem hiding this comment.
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
1851075 to
bfb24fd
Compare
2 participants
Changes:
list_channelstochannel_capacitiesin theLightningNodetrait since this call was only used to determine how much the node will send per month.SimNodes so that a user could use them.