Remove LDK specific scoring from PathFinder trait

Author: sangbida

sim-cli/src/parsing.rs

@@ -308,6 +308,7 @@ pub async fn create_simulation_with_network<P: PathFinder + Clone + 'static>(
             .map_err(|e| SimulationError::SimulatedNetworkError(format!("{:?}", e)))?,
     );
 
+<<<<<<< HEAD
 <<<<<<< HEAD
 <<<<<<< HEAD
     // We want the full set of nodes in our graph to return to the caller so that they can take
@@ -327,6 +328,8 @@ pub async fn create_simulation_with_network<P: PathFinder + Clone + 'static>(
 =======
 >>>>>>> 079041b (Fix formatting)
     // Pass the pathfinder to ln_node_from_graph
+=======
+>>>>>>> 4a0f276 (Remove LDK specific scoring from PathFinder trait)
     let nodes = ln_node_from_graph(simulation_graph.clone(), routing_graph, pathfinder).await;
     let validated_activities =
         get_validated_activities(&nodes_dyn, nodes_info, sim_params.activity.clone()).await?;
@@ -671,7 +674,7 @@ mod tests {
         (secret_key, public_key)
     }
 
-    /// Helper function to create simulated channels for testing
+    /// Helper function to create simulated channels for testing.
     fn create_simulated_channels(num_channels: usize, capacity_msat: u64) -> Vec<SimulatedChannel> {
         let mut channels = Vec::new();
         for i in 0..num_channels {
@@ -707,66 +710,67 @@ mod tests {
         channels
     }
 
-    /// A pathfinder that always fails to find a path
+    /// A pathfinder that always fails to find a path.
     #[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(),
                         ))
                     }
                 },
@@ -785,15 +789,9 @@ 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
+        // Should always fail.
         assert!(result.is_err());
     }
 
@@ -806,31 +804,24 @@ 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)
+        // 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();
             assert_eq!(route.paths[0].hops.len(), 1); // Only one hop
         }
 
-        // Test indirect connection (should fail)
+        // 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
+        // May fail because no direct route exists.
         // (depends on your test network topology)
     }
 
-    /// Test that different pathfinders produce different behavior in payments
+    /// Test that different pathfinders produce different behavior in payments.
     #[tokio::test]
     async fn test_pathfinder_affects_payment_behavior() {
         let channels = create_simulated_channels(3, 1_000_000_000);
@@ -848,7 +839,7 @@ mod tests {
         let routing_graph =
             Arc::new(populate_network_graph(channels.clone(), Arc::new(SystemClock {})).unwrap());
 
-        // Create nodes with different pathfinders
+        // Create nodes with different pathfinders.
         let nodes_default = ln_node_from_graph(
             sim_graph.clone(),
             routing_graph.clone(),
@@ -863,7 +854,7 @@ mod tests {
         )
         .await;
 
-        // Both should create the same structure
+        // Both should create the same structure.
         assert_eq!(nodes_default.len(), nodes_fail.len());
     }
 }

simln-lib/src/sim_node.rs

@@ -502,7 +502,18 @@ pub trait SimNetwork: Send + Sync {
 //type LdkNetworkGraph = NetworkGraph<Arc<WrappedLog>>;
 type LdkNetworkGraph = NetworkGraph<&'static WrappedLog>;
 /// 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,
@@ -512,19 +523,32 @@ pub trait PathFinder<'a>: Send + Sync {
     ) -> 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 {
@@ -535,10 +559,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],
         )
@@ -573,7 +597,7 @@ pub struct SimNode<T: SimNetwork, C: Clock, P: PathFinder = DefaultPathFinder> {
     pathfinder: P,
 }
 
-impl<'a, T: SimNetwork, C: Clock, P: PathFinder<'a>> SimNode<'a, T, C, P> {
+impl<T: SimNetwork, C: Clock, P: PathFinder> SimNode<T, C, 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(
@@ -583,24 +607,14 @@ impl<'a, T: SimNetwork, C: Clock, P: PathFinder<'a>> SimNode<'a, T, C, P> {
         clock: Arc<C>,
         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(),
-            Arc::new(WrappedLog {}),
-        );
-
-        Ok(SimNode {
+        SimNode {
             info,
             network: payment_network,
             in_flight: Mutex::new(HashMap::new()),
             pathfinding_graph,
-            scorer,
             clock,
             pathfinder,
-        })
+        }
     }
 
     /// Dispatches a payment to a specified route. If `custom_records` is `Some`, they will be attached to the outgoing
@@ -735,7 +749,6 @@ impl<T: SimNetwork, C: Clock, P: PathFinder> LightningNode for SimNode<T, C, P>
             dest,
             amount_msat,
             &self.pathfinding_graph,
-            &self.scorer,
         ) {
             Ok(path) => path,
             // In the case that we can't find a route for the payment, we still report a successful payment *api call*
@@ -1146,7 +1159,7 @@ pub async fn ln_node_from_graph<C: Clock, P>(
     pathfinder: P,
 ) -> Result<HashMap<PublicKey, Arc<Mutex<dyn LightningNode>>>, LightningError> 
 where
-    P: for<'a> PathFinder<'a> + Clone + 'static,
+    P: PathFinder + 'static,
 {
     let mut nodes: HashMap<PublicKey, Arc<Mutex<dyn LightningNode>>> = HashMap::new();
 
@@ -1651,7 +1664,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.
@@ -2204,7 +2216,6 @@ mod tests {
         graph: SimGraph,
         nodes: Vec<PublicKey>,
         routing_graph: Arc<LdkNetworkGraph>,
-        scorer: Mutex<ProbabilisticScorer<Arc<LdkNetworkGraph>, Arc<WrappedLog>>>,
         shutdown: (Trigger, Listener),
         pathfinder: DefaultPathFinder,
     }
@@ -2227,12 +2238,19 @@ mod tests {
                 populate_network_graph(channels.clone(), Arc::new(SystemClock {})).unwrap(),
             );
 
+<<<<<<< HEAD
             let scorer = Mutex::new(ProbabilisticScorer::new(
+=======
+<<<<<<< HEAD
+            let scorer = ProbabilisticScorer::new(
+>>>>>>> 4a0f276 (Remove LDK specific scoring from PathFinder trait)
                 ProbabilisticScoringDecayParameters::default(),
                 routing_graph.clone(),
                 Arc::new(WrappedLog {}),
             ));
 
+=======
+>>>>>>> 3e72658 (Remove LDK specific scoring from PathFinder trait)
             // 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
@@ -2253,9 +2271,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.
@@ -2300,18 +2317,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(), None, 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.