Track in-flight HTLCs across payments when routing

[jurvis]

Addresses #1267.

Purpose

As a sender, we want to keep track of all the HTLCs that are currently in flight. We currently do that between pathfinding attempts in find_route. However, that information is not currently shared across payments, leading to some inaccuracies.

Proposed Implementation

Accounting for the HTLCs will consist of two efforts: keeping track of them, and using them.

Keep Track

In pay_internal, we want to respond to specific results of send_payment which will tell us when we'll need to store the path information for use later.

Path information is then stored in payment_cache of our payer.

Using

Before find_route is called, we retrieve all the paths we stored, and loop through them to construct a HashMap (HashMap<(u64, bool), u64>) mapping a channel's short channel id and direction with the amount being sent.

This map is wrapped with our user-supplied score in aAccountForInFlightHtlcs struct, which will serve to delegate Score functions to its underlying scorer. It will also use our new liquidity map to construct a ChannelUsage accounting for the inflight HTLCs to be passed to channel_penalty_msat.

Last Edited: 2022-08-15T05:27:11.602Z

Other Notes

Future opportunities:

• @ariard: track CLTV deltas to know when HTLCs are expiring/stuck
• @jkczyz: have router not need to depend on scorer (possibly in a future commit of this PR)

[jkczyz]

@TheBlueMatt Would good to have some early feedback on this PR. Upcoming commits will use in-flight HTLC tracking during routing. Follow-up PR will deal with persistence.

[TheBlueMatt]

As for the followup to do the tracking, I don't think we need a new interface in the router at all, I think we can do it entirely by wrapping the scorer and changing the amounts we pass through to the user-provided scorer.

[dunxen]

Going to be using this for review club session 5. Nice to dig into payment logic a bit. :)

[jurvis]

@TheBlueMatt

As for the followup to do the tracking, I don't think we need a new interface in the router at all, I think we can do it entirely by wrapping the scorer and changing the amounts we pass through to the user-provided scorer.

hm, I think we do need to pass in the inflight htlcs map into get_route because we do account for used_channel_liquidities when pathfinding here as well.

[TheBlueMatt]

hm, I think we do need to pass in the inflight htlcs map into get_route because we do account for used_channel_liquidities when pathfinding here as well.

Yea, if we want to exactly match the behavior, but I'm not sure that is neccessary - it'll result in a few suboptimal decisions, but we can do it only in the scorer and we'll still get most of the way there, without more complexity in the router itself.

[jkczyz]

Yea, if we want to exactly match the behavior, but I'm not sure that is neccessary - it'll result in a few suboptimal decisions, but we can do it only in the scorer and we'll still get most of the way there, without more complexity in the router itself.

There may be another case when computing available_value_contribution_msat:

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

Lines 1002 to 1004 in d024251

	.map_or(0, |used_liquidity_msat| {
	available_value_contribution_msat = available_value_contribution_msat
	.saturating_sub(*used_liquidity_msat);

[TheBlueMatt]

There may be another case when computing available_value_contribution_msat:

Yea, that's generally what I was referring to - we'll not make the same decisions we'd like to around how much we'd maximally want to send over a channel. I do kinda now wonder if we shouldn't have made that a method on the scorer, but either way, I'm not sure its totally critical that we get it exactly right here - at worse we'll end up refusing to re-use channels that we already have pending payments over where we may have been willing to re-use some small part of their capacity.

[jurvis]

I do kinda now wonder if we shouldn't have made that a method on the scorer

which method? channel_penalty_msat? is your suggestion to pass our map of inflight htlcs to the scorer and just have it subtracted when calculating capacity_msat?

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

Lines 807 to 808 in d024251

	let capacity_msat = usage.effective_capacity.as_msat()
	.saturating_sub(usage.inflight_htlc_msat);

[jkczyz]

I do kinda now wonder if we shouldn't have made that a method on the scorer

which method? channel_penalty_msat? is your suggestion to pass our map of inflight htlcs to the scorer and just have it subtracted when calculating capacity_msat?

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

Lines 807 to 808 in d024251

	let capacity_msat = usage.effective_capacity.as_msat()
	.saturating_sub(usage.inflight_htlc_msat);

@TheBlueMatt Did you mean should have not shouldn't have?

[TheBlueMatt]

@TheBlueMatt Did you mean should have not shouldn't have?

Err, yes lol.

[jkczyz]

Another thing to keep in mind is that we'd eventually like to move the LockableScore from InvoicePayer to DefaultRouter since a remote Router wouldn't need a scorer locally. So we may need to pass the in-flight HTLC map to Router::find_route even if it is not used by the standalone find_route function that DefaultRoute::find_route calls. But I think the change would be fairly trivial (i.e., moving the use of the AccountForInFlightHtlcs wrapper into DefaultRoute::find_route).

[jkczyz]

LGTM other than a few nits.

[jkczyz]

Ok, I'm happy with this other than one test change to make it more realistic.

[jkczyz]

LGTM. @TheBlueMatt could you take another look?

[adamritter]

Are these RefCells really needed? Wouldn't just creating TestScorers work generally?

[jurvis]

I believe this is because InvoicePayer expects the scorer to implement LockableScore.

TestScorer here is really just a mocked version of a scorer that we'd expect users to implement on their own

[jkczyz]

Yeah, a scorer needs to be locked to (a) use its mutable interface in InvoicePayer's event handling and (b) use its immutable interface when routing such that its state doesn't change between calls. Additionally, another reference to it may be needed for serialization.

[adamritter]

I see, thanks! MinCostFlowRouter pull request is at a point where it needs to be tested together with InvoicePayer and tracking in-flight HTLCs, but in a conversion with @TheBlueMatt, he said that as MinCostFlowRouter needs to access liquidity tracking, but not a custom scorer provided by LDK users, LiquidityTracker should be a separate trait implemented by the scorers. Another option in my opinion would be keeping liquidity tracking from scoring totally separate, and DefaultRouter should request the estimated liquidity information and provide it to the immutable Scorer (that can be customized by LDK user, while liquidity tracking implementation probably shouldn't be surfaced in the API).

What do you guys think?

Here's a link to the comments:

#1668 (comment)

[jkczyz]

Yeah, I think we want at least two follow-ups to this PR:

(1) Move the LockableScore type parameter to DefaultRouter since some routers (e.g., remote server routing) don't need a scorer.
(2) Move liquidity tracking to either the Payer trait or a dedicated one, having InvoicePayer query it and pass the liquidity information to Router akin to how it currently passes a scorer.

The latter comes from an observation that I made in review club. ChannelManager actually knows all the liquidity information given it is the one producing PaymentPathFailed and PaymentPathSuccessful events. It also would make liquidity tracking persist across restarts.

See also #1157 for exposing pending payments more generally, which should be considered when making this change.

[adamritter]

I see, thanks, that makes a lot of sense.