Add Custom Pathfinding #303
Conversation
simln-lib/src/sim_node.rs
Outdated
| amount_msat: u64, | ||
| pathfinding_graph: &LdkNetworkGraph, | ||
| ) -> Result<Route, SimulationError> { | ||
| let scorer_graph = NetworkGraph::new(bitcoin::Network::Regtest, Arc::new(WrappedLog {})); |
There was a problem hiding this comment.
I think it makes more sense to pass in this graph rather than instantiating it every time.
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_create_activity() { |
There was a problem hiding this comment.
Not sure if this helper function is still needed in this PR
sim-cli/src/parsing.rs
Outdated
| ======= | ||
| pub async fn create_simulation_with_network<P: for<'a> PathFinder<'a> + Clone + 'static>( | ||
| cli: &Cli, | ||
| >>>>>>> 59dbbe6 (Add pathfinder trait) |
There was a problem hiding this comment.
Some of the commits in between seem to have these from conflicts
There was a problem hiding this comment.
Yeah - will fix before taking it out of draft. Thanks!
simln-lib/src/sim_node.rs
Outdated
| let scorer_graph = NetworkGraph::new(bitcoin::Network::Regtest, Arc::new(WrappedLog {})); | ||
| let mut scorers = self.scorers.lock().await; | ||
| let scorer = scorers.entry(*source).or_insert_with(|| { | ||
| ProbabilisticScorer::new( | ||
| ProbabilisticScoringDecayParameters::default(), | ||
| Arc::new(scorer_graph), | ||
| Arc::new(WrappedLog {}), | ||
| ) | ||
| }); |
There was a problem hiding this comment.
I'm not following - could you explain why does it need a map and one scorer per sender?
There was a problem hiding this comment.
From my understanding, having one scorer per node gives us more realistic pathfinding as this is close to the real-world behaviour of the Lightning Network (each node is responsible for its own pathfinding), so simulations would be run under real-world conditions.
There was a problem hiding this comment.
right, I'm wondering because each SimNode has a Pathfinder and each time a node is being instantiated in ln_node_from_graph, the pathfinder is cloned. So IIUC, the entire map with all scorers will be cloned on every SimNode. Perhaps it should be in an Arc?
edit: I'm wrong - I see the map in ScorersMap is behind an Arc
There was a problem hiding this comment.
Also, here it's creating a new empty graph for each scorer. Shouldn't it use the already existing graph
There was a problem hiding this comment.
On a second look, I'm still not sure on why we need a map to keep track of all scorers. If each SimNode has a Pathfinder then I don't see why it's needed for the DefaultPathfinder of every SimNode to have that map. Each Pathfinder for each SimNode would only need its own scorer + the network graph no?
There was a problem hiding this comment.
Also, here it's creating a new empty graph for each scorer. Shouldn't it use the already existing graph
Yeah, you are right - left a comment about that above . Still thinking of the best approach as the type needed for the scorer graph is slightly different from the type of network graph we have currently.
On a second look, I'm still not sure on why we need a map to keep track of all scorers. If each
SimNodehas aPathfinderthen I don't see why it's needed for theDefaultPathfinderof everySimNodeto have that map. EachPathfinderfor eachSimNodewould only need its own scorer + the network graph no?
Yeah this makes sense! If the DefaultPathfinder holds its own scorer, that gives us one scorer per node.
simln-lib/src/sim_node.rs
Outdated
| pub struct DefaultPathFinder { | ||
| scorers: ScorersMap, | ||
| } | ||
|
|
There was a problem hiding this comment.
feels like this should have a NetworkGraph
There was a problem hiding this comment.
Yeah - thought of that but also considering allowing the user pass in the NetworkGraph (for scoring) directly into the find_route method as an Option or perhaps we could somehow still use the pathfinding_graph already being passed into find_route - What do you think?
There was a problem hiding this comment.
hmmmm API-wise not sure what's best but I'd think that find_route in the Pathfinder could do without having to pass the network graph? A pathfinder impl can have it's own view of the network and based on sender and destination pubkeys and the amount return a Route.
Edit: internally the DefaultPathfinder could use the graph we already have for scoring.
There was a problem hiding this comment.
Edit: internally the
DefaultPathfindercould use the graph we already have for scoring.
Yeah! That makes sense.
chuksys
force-pushed
the
custom-pathfinding
branch
from
d4423a0 to
04842b1
Compare
This PR builds upon the work done here #273
This PR introduces a new
PathFindertrait which allows for more flexible pathfinding. Instead of being locked into the LDK pathfinding algorithm, custom pathfinding algorithms can be swapped in. ADefaultPathFinderhas been provided to maintain backwards compatibility.