Remove LDK specific scoring from PathFinder trait

sangbida · sangbida · commit f65d20d47684 · 2025-06-09T16:48:29.000+10:00
diff --git a/sim-cli/src/parsing.rs b/sim-cli/src/parsing.rs
@@ -225,7 +225,7 @@ struct NodeMapping {
     alias_node_map: HashMap<String, NodeInfo>,
 }
 
-pub async fn create_simulation_with_network<P: for<'a> PathFinder<'a> + Clone + 'static>(
+pub async fn create_simulation_with_network<P: PathFinder + Clone + 'static>(
     cli: &Cli,
     sim_params: &SimParams,
     tasks: TaskTracker,
@@ -661,62 +661,63 @@ mod tests {
     #[derive(Clone)]
     pub struct AlwaysFailPathFinder;
 
-    impl<'a> PathFinder<'a> for AlwaysFailPathFinder {
+    impl PathFinder for AlwaysFailPathFinder {
         fn find_route(
             &self,
             _source: &PublicKey,
             _dest: PublicKey,
             _amount_msat: u64,
-            _pathfinding_graph: &NetworkGraph<&'a WrappedLog>,
-            _scorer: &ProbabilisticScorer<Arc<NetworkGraph<&'a WrappedLog>>, &'a WrappedLog>,
+            _pathfinding_graph: &NetworkGraph<&'static WrappedLog>,
         ) -> Result<Route, SimulationError> {
             Err(SimulationError::SimulatedNetworkError(
                 "No route found".to_string(),
             ))
         }
     }
 
-    /// A pathfinder that only returns single-hop paths
+    /// A pathfinder that only returns single-hop paths.
     #[derive(Clone)]
     pub struct SingleHopOnlyPathFinder;
 
-    impl<'a> PathFinder<'a> for SingleHopOnlyPathFinder {
+    impl PathFinder for SingleHopOnlyPathFinder {
         fn find_route(
             &self,
             source: &PublicKey,
             dest: PublicKey,
             amount_msat: u64,
-            pathfinding_graph: &NetworkGraph<&'a WrappedLog>,
-            scorer: &ProbabilisticScorer<Arc<NetworkGraph<&'a WrappedLog>>, &'a WrappedLog>,
+            pathfinding_graph: &NetworkGraph<&'static WrappedLog>,
         ) -> Result<Route, SimulationError> {
-            // Try to find a direct route only (single hop)
-            let route_params = RouteParameters {
-                payment_params: PaymentParameters::from_node_id(dest, 0)
-                    .with_max_total_cltv_expiry_delta(u32::MAX)
-                    .with_max_path_count(1)
-                    .with_max_channel_saturation_power_of_half(1),
-                final_value_msat: amount_msat,
-                max_total_routing_fee_msat: None,
-            };
-
-            // Try to find a route - if it fails or has more than one hop, return an error
+            let scorer = ProbabilisticScorer::new(
+                ProbabilisticScoringDecayParameters::default(),
+                pathfinding_graph,
+                &WrappedLog {},
+            );
+
+            // Try to find a route - if it fails or has more than one hop, return an error.
             match find_route(
                 source,
-                &route_params,
+                &RouteParameters {
+                    payment_params: PaymentParameters::from_node_id(dest, 0)
+                        .with_max_total_cltv_expiry_delta(u32::MAX)
+                        .with_max_path_count(1)
+                        .with_max_channel_saturation_power_of_half(1),
+                    final_value_msat: amount_msat,
+                    max_total_routing_fee_msat: None,
+                },
                 pathfinding_graph,
                 None,
                 &WrappedLog {},
-                scorer,
+                &scorer,
                 &Default::default(),
                 &[0; 32],
             ) {
                 Ok(route) => {
-                    // Check if the route has exactly one hop
+                    // Only allow single-hop routes.
                     if route.paths.len() == 1 && route.paths[0].hops.len() == 1 {
                         Ok(route)
                     } else {
                         Err(SimulationError::SimulatedNetworkError(
-                            "No direct route found".to_string(),
+                            "Only single-hop routes allowed".to_string(),
                         ))
                     }
                 },
@@ -735,13 +736,7 @@ mod tests {
         let source = channels[0].get_node_1_pubkey();
         let dest = channels[2].get_node_2_pubkey();
 
-        let scorer = ProbabilisticScorer::new(
-            ProbabilisticScoringDecayParameters::default(),
-            routing_graph.clone(),
-            &WrappedLog {},
-        );
-
-        let result = pathfinder.find_route(&source, dest, 100_000, &routing_graph, &scorer);
+        let result = pathfinder.find_route(&source, dest, 100_000, &routing_graph);
 
         // Should always fail
         assert!(result.is_err());
@@ -756,15 +751,9 @@ mod tests {
         let pathfinder = SingleHopOnlyPathFinder;
         let source = channels[0].get_node_1_pubkey();
 
-        let scorer = ProbabilisticScorer::new(
-            ProbabilisticScoringDecayParameters::default(),
-            routing_graph.clone(),
-            &WrappedLog {},
-        );
-
         // Test direct connection (should work)
         let direct_dest = channels[0].get_node_2_pubkey();
-        let result = pathfinder.find_route(&source, direct_dest, 100_000, &routing_graph, &scorer);
+        let result = pathfinder.find_route(&source, direct_dest, 100_000, &routing_graph);
 
         if result.is_ok() {
             let route = result.unwrap();
@@ -773,8 +762,7 @@ mod tests {
 
         // Test indirect connection (should fail)
         let indirect_dest = channels[2].get_node_2_pubkey();
-        let _result =
-            pathfinder.find_route(&source, indirect_dest, 100_000, &routing_graph, &scorer);
+        let _result = pathfinder.find_route(&source, indirect_dest, 100_000, &routing_graph);
 
         // May fail because no direct route exists
         // (depends on your test network topology)
diff --git a/simln-lib/src/sim_node.rs b/simln-lib/src/sim_node.rs
@@ -477,7 +477,7 @@ impl SimulatedChannel {
 
 /// SimNetwork represents a high level network coordinator that is responsible for the task of actually propagating
 /// payments through the simulated network.
-trait SimNetwork: Send + Sync {
+pub trait SimNetwork: Send + Sync {
     /// Sends payments over the route provided through the network, reporting the final payment outcome to the sender
     /// channel provided.
     fn dispatch_payment(
@@ -495,30 +495,53 @@ trait SimNetwork: Send + Sync {
 }
 
 /// A trait for custom pathfinding implementations.
-pub trait PathFinder<'a>: Send + Sync {
+/// Finds a route from the source node to the destination node for the specified amount.
+/// 
+/// # Arguments
+/// * `source` - The public key of the node initiating the payment.
+/// * `dest` - The public key of the destination node to receive the payment.
+/// * `amount_msat` - The amount to send in millisatoshis.
+/// * `pathfinding_graph` - The network graph containing channel topology and routing information.
+/// 
+/// # Returns
+/// Returns a `Route` containing the payment path, or a `SimulationError` if no route is found.
+
+pub trait PathFinder: Send + Sync + Clone {
     fn find_route(
         &self,
         source: &PublicKey,
         dest: PublicKey,
         amount_msat: u64,
-        pathfinding_graph: &NetworkGraph<&'a WrappedLog>,
-        scorer: &ProbabilisticScorer<Arc<NetworkGraph<&'a WrappedLog>>, &'a WrappedLog>,
+        pathfinding_graph: &NetworkGraph<&'static WrappedLog>,
     ) -> Result<Route, SimulationError>;
 }
 
-/// Default pathfinder that uses LDK's pathfinding algorithm.
+/// The default pathfinding implementation that uses LDK's built-in pathfinding algorithm.
 #[derive(Clone)]
 pub struct DefaultPathFinder;
 
-impl<'a> PathFinder<'a> for DefaultPathFinder {
+impl DefaultPathFinder {
+    pub fn new() -> Self {
+        Self
+    }
+}
+
+impl PathFinder for DefaultPathFinder {
     fn find_route(
         &self,
         source: &PublicKey,
         dest: PublicKey,
         amount_msat: u64,
-        pathfinding_graph: &NetworkGraph<&'a WrappedLog>,
-        scorer: &ProbabilisticScorer<Arc<NetworkGraph<&'a WrappedLog>>, &'a WrappedLog>,
+        pathfinding_graph: &NetworkGraph<&'static WrappedLog>,
     ) -> Result<Route, SimulationError> {
+        let scorer_graph = NetworkGraph::new(bitcoin::Network::Regtest, &WrappedLog {});
+        let scorer = ProbabilisticScorer::new(
+            ProbabilisticScoringDecayParameters::default(),
+            Arc::new(scorer_graph),
+            &WrappedLog {},
+        );
+
+        // Call LDK's find_route with the scorer (LDK-specific requirement)
         find_route(
             source,
             &RouteParameters {
@@ -529,10 +552,10 @@ impl<'a> PathFinder<'a> for DefaultPathFinder {
                 final_value_msat: amount_msat,
                 max_total_routing_fee_msat: None,
             },
-            pathfinding_graph,
+            pathfinding_graph, // This is the real network graph used for pathfinding
             None,
             &WrappedLog {},
-            scorer,
+            &scorer, // LDK requires a scorer, so we provide a simple one
             &Default::default(),
             &[0; 32],
         )
@@ -544,45 +567,32 @@ impl<'a> PathFinder<'a> for DefaultPathFinder {
 /// all functionality through to a coordinating simulation network. This implementation contains both the [`SimNetwork`]
 /// implementation that will allow us to dispatch payments and a read-only NetworkGraph that is used for pathfinding.
 /// While these two could be combined, we re-use the LDK-native struct to allow re-use of their pathfinding logic.
-struct SimNode<'a, T: SimNetwork, P: PathFinder<'a> = DefaultPathFinder> {
+pub struct SimNode<T: SimNetwork, P: PathFinder = DefaultPathFinder> {
     info: NodeInfo,
     /// The underlying execution network that will be responsible for dispatching payments.
     network: Arc<Mutex<T>>,
     /// Tracks the channel that will provide updates for payments by hash.
     in_flight: HashMap<PaymentHash, Receiver<Result<PaymentResult, LightningError>>>,
     /// A read-only graph used for pathfinding.
-    pathfinding_graph: Arc<NetworkGraph<&'a WrappedLog>>,
-    /// Probabilistic scorer used to rank paths through the network for routing. This is reused across
-    /// multiple payments to maintain scoring state.
-    scorer: ProbabilisticScorer<Arc<NetworkGraph<&'a WrappedLog>>, &'a WrappedLog>,
+    pathfinding_graph: Arc<NetworkGraph<&'static WrappedLog>>,
     /// The pathfinder implementation to use for finding routes
     pathfinder: P,
 }
 
-impl<'a, T: SimNetwork, P: PathFinder<'a>> SimNode<'a, T, P> {
+impl<T: SimNetwork, P: PathFinder> SimNode<T, P> {
     /// Creates a new simulation node that refers to the high level network coordinator provided to process payments
     /// on its behalf. The pathfinding graph is provided separately so that each node can handle its own pathfinding.
     pub fn new(
         pubkey: PublicKey,
         payment_network: Arc<Mutex<T>>,
-        pathfinding_graph: Arc<NetworkGraph<&'a WrappedLog>>,
+        pathfinding_graph: Arc<NetworkGraph<&'static WrappedLog>>,
         pathfinder: P,
     ) -> Self {
-        // Initialize the probabilistic scorer with default parameters for learning from payment
-        // history. These parameters control how much successful/failed payments affect routing
-        // scores and how quickly these scores decay over time.
-        let scorer = ProbabilisticScorer::new(
-            ProbabilisticScoringDecayParameters::default(),
-            pathfinding_graph.clone(),
-            &WrappedLog {},
-        );
-
         SimNode {
             info: node_info(pubkey),
             network: payment_network,
             in_flight: HashMap::new(),
             pathfinding_graph,
-            scorer,
             pathfinder,
         }
     }
@@ -602,7 +612,7 @@ fn node_info(pubkey: PublicKey) -> NodeInfo {
 }
 
 #[async_trait]
-impl<'a, T: SimNetwork, P: PathFinder<'a>> LightningNode for SimNode<'a, T, P> {
+impl<T: SimNetwork, P: PathFinder> LightningNode for SimNode<T, P> {
     fn get_info(&self) -> &NodeInfo {
         &self.info
     }
@@ -624,7 +634,6 @@ impl<'a, T: SimNetwork, P: PathFinder<'a>> LightningNode for SimNode<'a, T, P> {
             dest,
             amount_msat,
             &self.pathfinding_graph,
-            &self.scorer,
         ) {
             Ok(route) => route,
             Err(e) => {
@@ -1001,7 +1010,7 @@ pub async fn ln_node_from_graph<P>(
     pathfinder: P,
 ) -> HashMap<PublicKey, Arc<Mutex<dyn LightningNode>>>
 where
-    P: for<'a> PathFinder<'a> + Clone + 'static,
+    P: PathFinder + 'static,
 {
     let mut nodes: HashMap<PublicKey, Arc<Mutex<dyn LightningNode>>> = HashMap::new();
 
@@ -1499,7 +1508,6 @@ mod tests {
     use mockall::mock;
     use ntest::assert_true;
     use std::time::Duration;
-    use tokio::sync::oneshot;
     use tokio::time::{self, timeout};
 
     /// Creates a test channel policy with its maximum HTLC size set to half of the in flight limit of the channel.
@@ -1889,7 +1897,12 @@ mod tests {
 
         // Create a simulated node for the first channel in our network.
         let pk = channels[0].node_1.policy.pubkey;
-        let mut node = SimNode::new(pk, sim_network.clone(), Arc::new(graph), DefaultPathFinder);
+        let mut node = SimNode::new(
+            pk,
+            sim_network.clone(),
+            Arc::new(graph),
+            DefaultPathFinder::new(),
+        );
 
         // Prime mock to return node info from lookup and assert that we get the pubkey we're expecting.
         let lookup_pk = channels[3].node_1.policy.pubkey;
@@ -1974,16 +1987,15 @@ mod tests {
     }
 
     /// Contains elements required to test dispatch_payment functionality.
-    struct DispatchPaymentTestKit<'a> {
+    struct DispatchPaymentTestKit {
         graph: SimGraph,
         nodes: Vec<PublicKey>,
-        routing_graph: Arc<NetworkGraph<&'a WrappedLog>>,
-        scorer: ProbabilisticScorer<Arc<NetworkGraph<&'a WrappedLog>>, &'a WrappedLog>,
+        routing_graph: Arc<NetworkGraph<&'static WrappedLog>>,
         shutdown: (Trigger, Listener),
         pathfinder: DefaultPathFinder,
     }
 
-    impl DispatchPaymentTestKit<'_> {
+    impl DispatchPaymentTestKit {
         /// Creates a test graph with a set of nodes connected by three channels, with all the capacity of the channel
         /// on the side of the first node. For example, if called with capacity = 100 it will set up the following
         /// network:
@@ -2001,12 +2013,6 @@ mod tests {
                 populate_network_graph(channels.clone(), Arc::new(SystemClock {})).unwrap(),
             );
 
-            let scorer = ProbabilisticScorer::new(
-                ProbabilisticScoringDecayParameters::default(),
-                routing_graph.clone(),
-                &WrappedLog {},
-            );
-
             // Collect pubkeys in-order, pushing the last node on separately because they don't have an outgoing
             // channel (they are not node_1 in any channel, only node_2).
             let mut nodes = channels
@@ -2027,9 +2033,8 @@ mod tests {
                 .expect("could not create test graph"),
                 nodes,
                 routing_graph,
-                scorer,
                 shutdown: shutdown_clone,
-                pathfinder: DefaultPathFinder,
+                pathfinder: DefaultPathFinder::new(),
             };
 
             // Assert that our channel balance is all on the side of the channel opener when we start up.
@@ -2074,18 +2079,14 @@ mod tests {
         ) -> (Route, Result<PaymentResult, LightningError>) {
             let route = self
                 .pathfinder
-                .find_route(&source, dest, amt, &self.routing_graph, &self.scorer)
+                .find_route(&source, dest, amt, &self.routing_graph)
                 .unwrap();
+            let (sender, receiver) = tokio::sync::oneshot::channel();
 
-            let (sender, receiver) = oneshot::channel();
             self.graph
-                .dispatch_payment(source, route.clone(), PaymentHash([1; 32]), sender);
-
-            let payment_result = timeout(Duration::from_millis(10), receiver).await;
-            // Assert that we receive from the channel or fail.
-            assert!(payment_result.is_ok());
+                .dispatch_payment(source, route.clone(), PaymentHash([0; 32]), sender);
 
-            (route, payment_result.unwrap().unwrap())
+            (route, receiver.await.unwrap())
         }
 
         // Sets the balance on the channel to the tuple provided, used to arrange liquidity for testing.
