Don't over-penalize channels with inflight HTLCs

[jkczyz]

Commit df52da7 modified ProbabilisticScorer to apply some penalty amount multipliers (e.g., liquidity_penalty_amount_multiplier_msat) to the total amount flowing over the channel (i.e., including inflight HTLCs), not just the payment in question. This led to over-penalizing in-use channels. Instead, only apply the total amount when calculating success probability.

Fixes #3271

[jkczyz]

Unclear why this change causes routing::router::tests::avoids_saturating_channels to fail.

[TheBlueMatt]

Code changes lgtm except the one comment.

[TheBlueMatt]

avoids_saturating_channels may actually be buggy in structure, not, sure. Probably needs someone to sit down and carefully look at the graph structure we build.

[jkczyz]

avoids_saturating_channels may actually be buggy in structure, not, sure. Probably needs someone to sit down and carefully look at the graph structure we build.

AFAICT, this has to due with using unstable sort with selected_route, which contains paths both with and without channel_saturation_pow_half applied. Prior to my change, the paths using channel_saturation_pow_half came after those not using it after being sorted. But this is the opposite with my change.

rust-lightning/lightning/src/routing/router.rs

Lines 3240 to 3243 in d4e8b49

	selected_route.sort_unstable_by(|a, b|
	(((b.get_cost_msat() as u128) << 64) / (b.get_value_msat() as u128))
	.cmp(&(((a.get_cost_msat() as u128) << 64) / (a.get_value_msat() as u128)))
	);

Next, the retain only keeps the path with channel_saturation_pow_half applied. But with my change when the sort order is different, only the path without using channel_saturation_pow_half is kept.

rust-lightning/lightning/src/routing/router.rs

Lines 3246 to 3258 in d4e8b49

	let mut paths_left = selected_route.len();
	selected_route.retain(|path| {
	if paths_left == 1 {
	return true
	}
	let path_value_msat = path.get_value_msat();
	if path_value_msat <= overpaid_value_msat {
	overpaid_value_msat -= path_value_msat;
	paths_left -= 1;
	return false;
	}
	true
	});

Thus, the test fails because it wants two paths but it only gets one (i.e., the one not applying channel_saturation_pow_half). Haven't looked further yet to see what the fix would be.

[jkczyz]

Ok, I took a closer look. Unstable sort isn't actually the problem. Since the the penalty has decreased, the cost per value on those paths where channel_saturation_pow_half was applied is no longer less than the cost for the paths where it wasn't applied.

[jkczyz]

Changing the amount used in the test from 100_000_000 to 75_000_000 msats fixed the test. PTAL.

[G8XSU]

Does what it says in commit description.

• We now send inflight_htlc_amount and htlc_amount separately to calculate penalty.
• Uses htlc_amount to calculate penalty multiplers(negative).
• Uses total_amount (inflight_htlc+htlc_amount) to calculate success probability(positive) and bounds.
• All instances of htlc_amount and total_amount (inflight_htlc+htlc_amount) checked.

[G8XSU]

Doubt: Can you explain why liquidity_penalty only needs to be applied over htlc_amount_msat and not total_inflight?
Previously, this penalty would increase with each new htlc being routed through the same channel, but now, every htlc(whether the 1st or the 10th) will be treated equal for liquidity penalty iiuc.

I understand it might not be working as designed/intended at first but how was it wrong?
Is it because we don't want to penalize twice(once for liquidity_penalty and next for success_probability), since success_probability is already non-linear for increasing amounts?

[jkczyz]

IIUC, we want to use the total amount when computing the success probability since in-flight HTLCs will affect this. However, when computing the actual penalties we use the payment amount (i.e., amount_msat) and the success probability. So each successive HTLC in-flight would cause the penalty to increase since the success probability would decrease (assuming the total amount is above the minimum liquidity estimate).

That said, as written, our docs may need to be clearer about what amount is used to compute the success probability (total amount) and what amount is used for the penalty (payment amount w/ success probability).

[jkczyz]

That said, as written, our docs may need to be clearer about what amount is used to compute the success probability (total amount) and what amount is used for the penalty (payment amount w/ success probability).

Updated the docs in the latest push, BTW.

[TheBlueMatt]

LGTM, feel free to squash fixups.

[G8XSU]

Lgtm!

[G8XSU]

nit: there is a typo in last commit's description,
mount -> amount.

[TheBlueMatt]

Not gonna hold this up on a commit message typo.

[G8XSU]

Not gonna hold this up on a commit message typo.

Yes, was just waiting for your review.