Add Custom Pathfinding #303

chuksys · 2025-09-10T16:35:42Z

This PR builds upon the work done here #273

This PR introduces a new PathFinder trait which allows for more flexible pathfinding. Instead of being locked into the LDK pathfinding algorithm, custom pathfinding algorithms can be swapped in. A DefaultPathFinder has been provided to maintain backwards compatibility.

chuksys

Shared some thoughts.

chuksys · 2025-09-10T16:56:46Z

simln-lib/src/sim_node.rs

+        amount_msat: u64,
+        pathfinding_graph: &LdkNetworkGraph,
+    ) -> Result<Route, SimulationError> {
+        let scorer_graph = NetworkGraph::new(bitcoin::Network::Regtest, Arc::new(WrappedLog {}));


I think it makes more sense to pass in this graph rather than instantiating it every time.

chuksys · 2025-09-10T16:58:08Z

simln-lib/src/test_utils.rs

+    use super::*;
+
+    #[test]
+    fn test_create_activity() {


Not sure if this helper function is still needed in this PR

elnosh · 2025-09-10T22:49:01Z

sim-cli/src/parsing.rs

+=======
+pub async fn create_simulation_with_network<P: for<'a> PathFinder<'a> + Clone + 'static>(
+    cli: &Cli,
+>>>>>>> 59dbbe6 (Add pathfinder trait)


Some of the commits in between seem to have these from conflicts

Yeah - will fix before taking it out of draft. Thanks!

elnosh · 2025-09-10T22:52:54Z

simln-lib/src/sim_node.rs

+        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 {}),
+            )
+        });


I'm not following - could you explain why does it need a map and one scorer per sender?

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.

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

Also, here it's creating a new empty graph for each scorer. Shouldn't it use the already existing graph

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?

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 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?

Yeah this makes sense! If the DefaultPathfinder holds its own scorer, that gives us one scorer per node.

elnosh · 2025-09-10T22:58:47Z

simln-lib/src/sim_node.rs

+pub struct DefaultPathFinder {
+    scorers: ScorersMap,
+}
+


feels like this should have a NetworkGraph

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?

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.

Edit: internally the DefaultPathfinder could use the graph we already have for scoring.

Yeah! That makes sense.

elnosh · 2025-09-22T17:08:45Z

simln-lib/src/sim_node.rs

                routing_graph.clone(),
                clock.clone(),
-            )?)),
+                pathfinder.clone(),


wouldn't cloning the pathfinder here cause every SimNode to have the same scorer?

I think the issue here is that the scorer in DefaultPathfinder is wrapped in an Arc which prevents pathfinder.clone() from creating a unique deep copy of the mutable scorer state for each SimNode. I'll push a fix to remove the Arc and implement the necessary custom Clone logic to ensure isolation.

elnosh · 2025-09-22T17:10:17Z

sim-cli/src/parsing.rs

    );

+    // Create the pathfinder instance
+    let pathfinder = DefaultPathFinder::new(routing_graph.clone());


why not pass the pathfinder to create_simulation_with_network? there should be a way to create a simulation with a custom one.

Yeah that makes sense though since DefaultPathFinder depends on the network graph, we would probably have to pass the graph into create_simulation_with_network too which seems unnecessary to me.

If a user needs to create a simulation with a custom pathfinder, don't you think they can simply instantiate their custom pathfinder here? Moreso, if they have a custom pathfinder, they would probably be comfortable using simln-lib directly, no?

we would probably have to pass the graph into create_simulation_with_network too

yeah... don't see any other way.

If a user needs to create a simulation with a custom pathfinder, don't you think they can simply instantiate their custom pathfinder here? Moreso, if they have a custom pathfinder, they would probably be comfortable using simln-lib directly, no?

here how? If using simln as a lib, this create_simulation_with_network function is the one they would use to create a Simulation. They can setup their own custom pathfinder but would need a way to pass it somehow to simln-lib so that a Simulation can be setup using their custom pathfinder.

Here's my perspective - sim-cli is the application layer which should have the default settings which can be used to run a simulation out-of-the-box and simln-lib is the library which users can depend on and which they can pass custom values to, to run custom simulations that are not possible with the default sim-cli application. From what you are saying, the user would be expected to depend on both sim-cli as well as simln-lib. Shouldn't the user just depend on simln-lib (this is making me think create_simulation and create_simulation_with_network methods belong to simln-lib and not sim-cli)?

Hehe woops, my bad, I just realized that create_simulation and create_simulation_with_network are in sim-cli.

(this is making me think create_simulation and create_simulation_with_network methods belong to simln-lib and not sim-cli)?

I saw those (create_simulation and create_simulation_with_network) as lib utils since having users do everything that is in there seemed like a handful. But you made me realize they are not in simln-lib 😅 so what you said makes sense.

chuksys commented Sep 10, 2025

View reviewed changes

elnosh reviewed Sep 10, 2025

View reviewed changes

sangbida and others added 4 commits September 16, 2025 16:43

simln-lib/feat: Add Pathfinder trait

4a74f9f

simln-lib/test: Add tests for custom pathfinding

8c43804

sim_node/feat: Add a default implementation for DefaultPathFinder

e5b94fa

sim_node/feat: Implement Scoring for DefaultPathfinder

04842b1

chuksys force-pushed the custom-pathfinding branch from d4423a0 to 04842b1 Compare September 16, 2025 17:45

chuksys marked this pull request as ready for review September 16, 2025 17:59

elnosh reviewed Sep 22, 2025

View reviewed changes

Uh oh!

Add Custom Pathfinding #303

Are you sure you want to change the base?

Add Custom Pathfinding #303

Uh oh!

Conversation

chuksys commented Sep 10, 2025

Uh oh!

chuksys 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elnosh Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elnosh Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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 Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elnosh Sep 11, 2025 •

edited

Loading

elnosh Sep 11, 2025 •

edited

Loading

elnosh Oct 6, 2025 •

edited

Loading