From 1923c1bf3da931d13a85fc4baf387b56883f4953 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 7 Apr 2025 17:54:42 +0000 Subject: [PATCH 1/2] Update `probes_for_diversity` to test datapoint time updating It turns out `probes_for_diversity` wasn't actually testing that the datapoint update time logic worked as it was indicating a payment failed while trying to fully saturate a channel (which teaches us nothing). Instead, we need to "send" less over the channel and update twice to get the channel last-update-time logic tested. --- lightning/src/routing/scoring.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs index d88bce4f580..3b45349379d 100644 --- a/lightning/src/routing/scoring.rs +++ b/lightning/src/routing/scoring.rs @@ -4139,21 +4139,24 @@ mod tests { short_channel_id: 42, }); - // Apply some update to set the last-update time to now - scorer.payment_path_failed(&payment_path_for_amount(1000), 42, Duration::ZERO); + // Initialize the state for channel 42 + scorer.payment_path_failed(&payment_path_for_amount(500), 42, Duration::ZERO); + + // Apply an update to set the last-update time to 1 second + scorer.payment_path_failed(&payment_path_for_amount(490), 42, Duration::from_secs(1)); // If no time has passed, we get the full probing_diversity_penalty_msat assert_eq!(scorer.channel_penalty_msat(&candidate, usage, ¶ms), 1_000_000); // As time passes the penalty decreases. - scorer.time_passed(Duration::from_secs(1)); + scorer.time_passed(Duration::from_secs(2)); assert_eq!(scorer.channel_penalty_msat(&candidate, usage, ¶ms), 999_976); - scorer.time_passed(Duration::from_secs(2)); + scorer.time_passed(Duration::from_secs(3)); assert_eq!(scorer.channel_penalty_msat(&candidate, usage, ¶ms), 999_953); // Once we've gotten halfway through the day our penalty is 1/4 the configured value. - scorer.time_passed(Duration::from_secs(86400/2)); + scorer.time_passed(Duration::from_secs(86400/2 + 1)); assert_eq!(scorer.channel_penalty_msat(&candidate, usage, ¶ms), 250_000); } } From c22a0322d39769a7933b7ff445c5d8a831fd67b9 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 8 Apr 2025 14:47:43 +0000 Subject: [PATCH 2/2] Update `last_datapoint_time` even if we don't update bounds When we see a failure or success and it doesn't result in a bounds update, we previously were'nt updating the `last_datapoint_time` field as we were updating it in the bounds-update logic. This is wrong for the purpose of the `probing_diversity_penalty` because the whole point is to avoid repeatedly probing the same channel, but because of this we'll probe the same channel again and again as long as we don't learn any new information from the probes which causes a bounds update. --- lightning/src/routing/scoring.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs index 3b45349379d..0e7cfc1bdc5 100644 --- a/lightning/src/routing/scoring.rs +++ b/lightning/src/routing/scoring.rs @@ -1556,6 +1556,7 @@ DirectedChannelLiquidity { chan_descr, existing_max_msat, amount_msat); } self.update_history_buckets(0, duration_since_epoch); + *self.last_datapoint_time = duration_since_epoch; } /// Adjusts the channel liquidity balance bounds when failing to route `amount_msat` downstream. @@ -1571,6 +1572,7 @@ DirectedChannelLiquidity { chan_descr, existing_min_msat, amount_msat); } self.update_history_buckets(0, duration_since_epoch); + *self.last_datapoint_time = duration_since_epoch; } /// Adjusts the channel liquidity balance bounds when successfully routing `amount_msat`. @@ -1580,6 +1582,7 @@ DirectedChannelLiquidity { let max_liquidity_msat = self.max_liquidity_msat().checked_sub(amount_msat).unwrap_or(0); log_debug!(logger, "Subtracting {} from max liquidity of {} (setting it to {})", amount_msat, chan_descr, max_liquidity_msat); self.set_max_liquidity_msat(max_liquidity_msat, duration_since_epoch); + *self.last_datapoint_time = duration_since_epoch; self.update_history_buckets(amount_msat, duration_since_epoch); } @@ -1603,7 +1606,6 @@ DirectedChannelLiquidity { *self.max_liquidity_offset_msat = 0; } *self.last_updated = duration_since_epoch; - *self.last_datapoint_time = duration_since_epoch; } /// Adjusts the upper bound of the channel liquidity balance in this direction. @@ -1613,7 +1615,6 @@ DirectedChannelLiquidity { *self.min_liquidity_offset_msat = 0; } *self.last_updated = duration_since_epoch; - *self.last_datapoint_time = duration_since_epoch; } } @@ -4143,7 +4144,7 @@ mod tests { scorer.payment_path_failed(&payment_path_for_amount(500), 42, Duration::ZERO); // Apply an update to set the last-update time to 1 second - scorer.payment_path_failed(&payment_path_for_amount(490), 42, Duration::from_secs(1)); + scorer.payment_path_failed(&payment_path_for_amount(500), 42, Duration::from_secs(1)); // If no time has passed, we get the full probing_diversity_penalty_msat assert_eq!(scorer.channel_penalty_msat(&candidate, usage, ¶ms), 1_000_000);