-
Notifications
You must be signed in to change notification settings - Fork 421
Add probing_diversity_penalty_msat to the scorer parameters
#3422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -476,6 +476,9 @@ where L::Target: Logger { | |
| network_graph: G, | ||
| logger: L, | ||
| channel_liquidities: ChannelLiquidities, | ||
| /// The last time we were given via a [`ScoreUpdate`] method. This does not imply that we've | ||
| /// decayed every liquidity bound up to that time. | ||
| last_update_time: Duration, | ||
| } | ||
| /// Container for live and historical liquidity bounds for each channel. | ||
| #[derive(Clone)] | ||
|
|
@@ -745,6 +748,29 @@ pub struct ProbabilisticScoringFeeParameters { | |
| /// | ||
| /// Default value: false | ||
| pub linear_success_probability: bool, | ||
|
|
||
| /// In order to ensure we have knowledge for as many paths as possible, when probing it makes | ||
| /// sense to bias away from channels for which we have very recent data. | ||
| /// | ||
| /// This value is a penalty that is applied based on the last time that we updated the bounds | ||
| /// on the available liquidity in a channel. The specified value is the maximum penalty that | ||
| /// will be applied. | ||
| /// | ||
| /// It obviously does not make sense to assign a non-0 value here unless you are using the | ||
joostjager marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| /// pathfinding result for background probing. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This way of probing where you set this exploration/exploitation trade-off is just one way to do it, isn't it? From https://lightningdevkit.org/probing:
This is a different approach where no scores are used for probe route selection, which is different again from what is in the PR description (using a single scorer). What's your current thinking on what's best?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, certainly depends on your goal, but I think one model of probing I've kinda come around to is the idea of finding "backup routes" to destination you will likely pay in the future. ie its great when we repeatedly pay a destination and we have a route we always use that works basically all the time, but what happens when that route suddenly fails? Now we have no data on any other path because we're always paying over that same route. This PR is certainly not a thorough way to get good "backup routes", of course, but with regular probing my gut says it should do at least an okay job.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can follow your thoughts there, but I am quite uncomfortable with the level of gut feeling involved in these decisions. This is probably something that keeps coming back. In #2709 we were also guessing a bit at the best approach, and there are probably more areas where it's not very clear what the best approach or parameters are. It does make me think more about the idea of setting up a test framework that involves a virtual network of nodes.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, there's definitely a bit of a gut approach here, and a sim-framework might help, but the amount of gut-feelings that would go into building a sim-framework, I assume, would end up with more gut-result than even this :/. IMO we'd be better off A/B testing two nodes with identical channels to the same peers probing on the same interval and making test payments on the same interval to evaluate such things, but that requires a lot of time to get results.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe that is true. With a sim network though, I think the discussion will move to a higher level. We'd discuss whether the network is realistic enough, and parameters / probe methods / pathfinding algorithms will follow from that. To me, A/B testing sounds even more prone to getting non-representative results and is probably too slow to scan parameter spaces.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I fear that in a sim network the impact of guesses/gut-informed decisions is multiplicative (each guess compounds the effects of previous ones), but the only way to find out would be to build it and see what we get, plus to compare with reality (via some dataset...not sure what datset...). Maybe comparing with my probing dataset would work, when we shipped the new scorer in 0.1 a data scientist who works on c= helped run some analysis and at least found that our results were robust across a number of dimensions (channel size, etc), so there's at least some reason to think that those results are moderately representative of the broader network. |
||
| /// | ||
| /// Specifically, the following penalty is applied | ||
| /// `probing_diversity_penalty_msat * max(0, (86400 - current time + last update))^2 / 86400^2` is | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just talked offline with Joost a bit about this and during that I wondered what a good magic value would be for the penalty, i.e, from what threshold does it actually have an impact on diversity of the picked paths? Could we maybe have the applied penalty be proportional to the amount of datapoints tracked (mayb in the last day)? This way, even if we'd start with the penalty having no impact on diversity and we'd probe the same path over and over again, it would accrue over time and eventually the applied penalty would be so big that it would have an impact, having us moving on to the next likely path at least?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I set mine to 100_000. 100 sats is about max of where fees lie anyway, so a penalty between 25 sats (for 12 hours) and 100 sats (absolute max) should have a material impact. In general, I'm not sure I understand your question - its the same type of penalty as any of our other penalties, and we have to pick penalties which appropriately prefer other paths which have lower fees, ie they need to be somehow proportional to fees on the network.
We currently don't track any information that would allow us to do that, and tracking it would probably be nontrivial in size (eg a list of the last N times, as u64s, we got a result for each channel). I'm not entirely convinced that makes a difference, though - the theory here is that we probably don't need more results for a channel than once or twice a day if we're probing constantly over the course of months. Achieving that by calculating the number of times we probed a channel in the last day vs just looking at the last time we probed a channel should come out in the wash. |
||
| /// | ||
| /// As this is a maximum value, when setting this you should consider it in relation to the | ||
| /// other values set to ensure that, at maximum, we strongly avoid paths which we recently | ||
| /// tried (similar to if they have a low success probability). For example, you might set this | ||
| /// to be the sum of [`Self::base_penalty_msat`] and | ||
| /// [`Self::historical_liquidity_penalty_multiplier_msat`] (plus some multiple of their | ||
| /// corresponding `amount_multiplier`s). | ||
| /// | ||
| /// Default value: 0 | ||
joostjager marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| pub probing_diversity_penalty_msat: u64, | ||
| } | ||
|
|
||
| impl Default for ProbabilisticScoringFeeParameters { | ||
|
|
@@ -760,6 +786,7 @@ impl Default for ProbabilisticScoringFeeParameters { | |
| historical_liquidity_penalty_multiplier_msat: 10_000, | ||
| historical_liquidity_penalty_amount_multiplier_msat: 1_250, | ||
| linear_success_probability: false, | ||
| probing_diversity_penalty_msat: 0, | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -814,6 +841,7 @@ impl ProbabilisticScoringFeeParameters { | |
| anti_probing_penalty_msat: 0, | ||
| considered_impossible_penalty_msat: 0, | ||
| linear_success_probability: true, | ||
| probing_diversity_penalty_msat: 0, | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -927,6 +955,7 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref> ProbabilisticScorer<G, L> whe | |
| network_graph, | ||
| logger, | ||
| channel_liquidities: ChannelLiquidities::new(), | ||
| last_update_time: Duration::from_secs(0), | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1383,7 +1412,7 @@ DirectedChannelLiquidity< L, HT, T> { | |
| /// Returns a liquidity penalty for routing the given HTLC `amount_msat` through the channel in | ||
| /// this direction. | ||
| fn penalty_msat( | ||
| &self, amount_msat: u64, inflight_htlc_msat: u64, | ||
| &self, amount_msat: u64, inflight_htlc_msat: u64, last_update_time: Duration, | ||
| score_params: &ProbabilisticScoringFeeParameters, | ||
| ) -> u64 { | ||
| let total_inflight_amount_msat = amount_msat.saturating_add(inflight_htlc_msat); | ||
|
|
@@ -1465,6 +1494,18 @@ DirectedChannelLiquidity< L, HT, T> { | |
| } | ||
| } | ||
|
|
||
| if score_params.probing_diversity_penalty_msat != 0 { | ||
| // We use `last_update_time` as a stand-in for the current time as we don't want to | ||
| // fetch the current time in every score call (slowing things down substantially on | ||
| // some platforms where a syscall is required), don't want to add an unnecessary `std` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Re-reading this now, I am not so sure anymore that it is important to avoid std for probing specifically. It seems likely that probing is done only on servers that have std? For attr failures we do a similar thing where we only report hold time data if the platform has std. This would avoid the extra disk writes, and make for a simpler changeset in general. Not just lines of code, but also reasoning about the time approximation. This comment basically.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm incredibly skeptical of making exceptions for more and more use-cases. While its true that most active probers will be built with We can cut some corners here and there but diverging too much in terms of functionality between
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The alternative approach is to implement the minimum that is needed today for probing and don't support non-std probing diversity yet. Then if any of the potential requirements that you list above materializes, add it then. In the case of this PR, it wouldn't increase the total amount of work much. In this PR you'd just fetch time, and then in the potential future non-std extension, delete that one line and add the time derivation.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd much rather do it right the first time than have a user complain that something isn't working in their build and have to rush out a release to get it working. If its a ton of complexity or not practical its one thing, but its fairly straightforward here. |
||
| // requirement. Assuming we're probing somewhat regularly, it should reliably be close | ||
| // to the current time, (and using the last the last time we probed is also fine here). | ||
| let time_since_update = last_update_time.saturating_sub(*self.last_datapoint_time); | ||
| let mul = Duration::from_secs(60 * 60 * 24).saturating_sub(time_since_update).as_secs(); | ||
| let penalty = score_params.probing_diversity_penalty_msat.saturating_mul(mul * mul); | ||
| res = res.saturating_add(penalty / ((60 * 60 * 24) * (60 * 60 * 24))); | ||
| } | ||
|
|
||
| res | ||
| } | ||
|
|
||
|
|
@@ -1616,11 +1657,12 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref> ScoreLookUp for Probabilistic | |
| } | ||
|
|
||
| let capacity_msat = usage.effective_capacity.as_msat(); | ||
| let time = self.last_update_time; | ||
| self.channel_liquidities | ||
| .get(scid) | ||
| .unwrap_or(&ChannelLiquidity::new(Duration::ZERO)) | ||
| .as_directed(&source, &target, capacity_msat) | ||
| .penalty_msat(usage.amount_msat, usage.inflight_htlc_msat, score_params) | ||
| .penalty_msat(usage.amount_msat, usage.inflight_htlc_msat, time, score_params) | ||
| .saturating_add(anti_probing_penalty_msat) | ||
| .saturating_add(base_penalty_msat) | ||
| } | ||
|
|
@@ -1666,6 +1708,7 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref> ScoreUpdate for Probabilistic | |
| } | ||
| if at_failed_channel { break; } | ||
| } | ||
| self.last_update_time = duration_since_epoch; | ||
| } | ||
|
|
||
| fn payment_path_successful(&mut self, path: &Path, duration_since_epoch: Duration) { | ||
|
|
@@ -1693,6 +1736,7 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref> ScoreUpdate for Probabilistic | |
| hop.short_channel_id); | ||
| } | ||
| } | ||
| self.last_update_time = duration_since_epoch; | ||
| } | ||
|
|
||
| fn probe_failed(&mut self, path: &Path, short_channel_id: u64, duration_since_epoch: Duration) { | ||
|
|
@@ -1705,6 +1749,7 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref> ScoreUpdate for Probabilistic | |
|
|
||
| fn time_passed(&mut self, duration_since_epoch: Duration) { | ||
| self.channel_liquidities.time_passed(duration_since_epoch, self.decay_params); | ||
| self.last_update_time = duration_since_epoch; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -2361,11 +2406,16 @@ ReadableArgs<(ProbabilisticScoringDecayParameters, G, L)> for ProbabilisticScore | |
| ) -> Result<Self, DecodeError> { | ||
| let (decay_params, network_graph, logger) = args; | ||
| let channel_liquidities = ChannelLiquidities::read(r)?; | ||
| let mut last_update_time = Duration::from_secs(0); | ||
| for (_, liq) in channel_liquidities.0.iter() { | ||
| last_update_time = cmp::max(last_update_time, liq.last_updated); | ||
| } | ||
| Ok(Self { | ||
| decay_params, | ||
| network_graph, | ||
| logger, | ||
| channel_liquidities, | ||
| last_update_time, | ||
| }) | ||
| } | ||
| } | ||
|
|
@@ -4060,6 +4110,52 @@ mod tests { | |
| combined_scorer.scorer.estimated_channel_liquidity_range(42, &target_node_id()); | ||
| assert_eq!(liquidity_range.unwrap(), (0, 0)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn probes_for_diversity() { | ||
| // Tests the probing_diversity_penalty_msat is applied | ||
| let logger = TestLogger::new(); | ||
| let network_graph = network_graph(&logger); | ||
| let params = ProbabilisticScoringFeeParameters { | ||
| probing_diversity_penalty_msat: 1_000_000, | ||
| ..ProbabilisticScoringFeeParameters::zero_penalty() | ||
| }; | ||
| let decay_params = ProbabilisticScoringDecayParameters { | ||
| liquidity_offset_half_life: Duration::from_secs(10), | ||
| ..ProbabilisticScoringDecayParameters::zero_penalty() | ||
| }; | ||
| let mut scorer = ProbabilisticScorer::new(decay_params, &network_graph, &logger); | ||
| let source = source_node_id(); | ||
|
|
||
| let usage = ChannelUsage { | ||
| amount_msat: 512, | ||
| inflight_htlc_msat: 0, | ||
| effective_capacity: EffectiveCapacity::Total { capacity_msat: 1_024, htlc_maximum_msat: 1_024 }, | ||
| }; | ||
| let channel = network_graph.read_only().channel(42).unwrap().to_owned(); | ||
| let (info, _) = channel.as_directed_from(&source).unwrap(); | ||
| let candidate = CandidateRouteHop::PublicHop(PublicHopCandidate { | ||
| info, | ||
| 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); | ||
|
|
||
| // 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)); | ||
| assert_eq!(scorer.channel_penalty_msat(&candidate, usage, ¶ms), 999_976); | ||
|
|
||
| scorer.time_passed(Duration::from_secs(2)); | ||
| 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)); | ||
| assert_eq!(scorer.channel_penalty_msat(&candidate, usage, ¶ms), 250_000); | ||
| } | ||
| } | ||
|
|
||
| #[cfg(ldk_bench)] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, why not
last_update_duration_since_epoch?