Min cost flow router impelementation for Pickhardt Payments

[adamritter]

After creating an alternative min cost flow implementation for Pickhardt Payments in C++ and translating to Rust (https://github.com/adamritter/pickhardtpayments), it got to a level where I feel it's competitive with other implementations (min cost flow runs in about 150-200ms on the 0.3 BTC sending example, and probably faster for smaller payments).

I created an alternative find_route implementation for LDK (which is still missing a lot of critical components), and I got to a point where at least ,,it compiles''. I believe that after a lot more work on it, it could be a great addition to LDK.

Although it's fun working on it, I would also appreciate help if other people are interested in getting it to a working state (and maybe creating a branch in the LDK GitHub repo would be the way to go in that case).

[adamritter]

Note: the payment implementation probably needs tracking of in-flight HTLCs (#1668).

[adamritter]

After trying different possibilities I added estimated_channel_liquidity_range function inside the Score trait, this is the easiest way to port the unit tests as well to LargePaymentRouter.

[adamritter]

I've got a reply email from Truong Long [email protected] about using the negative cycle finding algorithm. I wrote back to ask if he could put it in public, but here's the email that I got anyways:

Hi Adam,

Feel free to relicense the code to the Apache-2.0 and MIT licenses.

Do note however that the code was made for educational purposes, and has not been checked for production readiness.

Cheers,

Akira

[TheBlueMatt]

I've got a reply email from Truong Long [email protected] about using the negative cycle finding algorithm. I wrote back to ask if he could put it in public, but here's the email that I got anyways:

That's awesome! Can we get him to post that somewhere? Not to be a stickler, but "I swear he sent me an email" probably isn't a great bar for license claims :p

Do note however that the code was made for educational purposes, and has not been checked for production readiness.

Fair enough, we'll do plenty of review on it once you give the go-ahead :p

[TheBlueMatt]

After trying different possibilities I added estimated_channel_liquidity_range function inside the Score trait, this is the easiest way to port the unit tests as well to LargePaymentRouter.

Hmm, that's fine for testing, but I think ideally we'd really have two separate scorer traits - if a user wants to provide their own scorer trait that works in terms of "amount I'm willing to pay to avoid this channel", they should be able to, without having to implement the liquidity estimations as well. It should be easy-enough to just add a second trait, implement both traits on the current scorer, and then have the different methods require only one, or both, for testing, traits.

[tnull]

Although it's fun working on it, I would also appreciate help if other people are interested in getting it to a working state (and maybe creating a branch in the LDK GitHub repo would be the way to go in that case).

Great to see progress towards an MCF implementation! As mentioned in the Discord, I'm really curious to see how it stacks up against our current splitting implementation. I'll pick up review after Matt's initial round of feedback, but let me know if I can be of any help in the meantime.

[adamritter]

Although it's fun working on it, I would also appreciate help if other people are interested in getting it to a working state (and maybe creating a branch in the LDK GitHub repo would be the way to go in that case).

Great to see progress towards an MCF implementation! As mentioned in the Discord, I'm really curious to see how it stacks up against our current splitting implementation. I'll pick up review after Matt's initial round of feedback, but let me know if I can be of any help in the meantime.

Hey, thanks very much, any help in any way is welcome, as finishing a change is much harder than starting it generally :)

There are lots of small edge cases to handle (setting fees, lock-time deltas, other flags correctly for majority of the cases, lots of error handling is missing).

We should also probably have a port of the payment simulator that Rene wrote (load a lightning network from disk, set hidden liquidity parameters randomly secretly from the altorithm, and simulate HLTC errors / successes by simulating messages from the network). If you want to write that simulation (or know somebody who would write it), that would be awesome for example.

[adamritter]

scorer tr

After trying different possibilities I added estimated_channel_liquidity_range function inside the Score trait, this is the easiest way to port the unit tests as well to LargePaymentRouter.

Hmm, that's fine for testing, but I think ideally we'd really have two separate scorer traits - if a user wants to provide their own scorer trait that works in terms of "amount I'm willing to pay to avoid this channel", they should be able to, without having to implement the liquidity estimations as well. It should be easy-enough to just add a second trait, implement both traits on the current scorer, and then have the different methods require only one, or both, for testing, traits.

After thinking more about this and looking at the ,,Track in-flight HTLCs across payments'' change (https://github.com/lightningdevkit/rust-lightning/pull/1643/files which I think is a great change), I would like to push back on separating the Scorer traits.

Tracking in-flight HTLCs makes the scorer stateful between payment attempts and changes the find_route function to be dependent on the Score trait. It also tracks liquidity more precisely than the scorers before, so it anyways does the job (liquidity estimation) that can be surfaced in the Score trait.

If a LDK user wants to improve on this, the user at this point has to do the liquidity tracking anyways. One possibility would be changing the Track in-flight HTLCs to do only the liquidity estimation by providing only the liquidity estimator interface and the score trait could be used by the DefaultRouter, but I think that could be a next refactoring if a user really needs that.

One more reason for my push back for having 2 different traits for tracking in-flight HTLCs is that the payment could try one routing algorithm and retry the other one if one routing algorithm fails. Min cost routing can only route through channels that have base_fee=0, so I don't see DefaultRouter being replaced short term, but I don't see any reason why we couldn't change routing algorithm after one failing and leaving partial payments behind (if those partial payments weren't too expensive to begin with).

[TheBlueMatt]

Hmm, yea, good points, I suppose for Payer stuff we'll need both in either case. I still think we should consider two traits, but for Payer we'll want to define a supertrait that encompasses both (ie pub trait BothScorers : Scorer + LiquidityEstimator) and I admit most users will just always want to use the supertrait.

[adamritter]

Status update: most of the unit tests copied from DefaultRouter to MinCostFlowRouter are working. The most important tests that don't work are the ones with min_htlc_msat > 1000, base_fee_msat > 0 and some tests that have unrealistically big fees on the routes. Algorithm improvements should improve on this cases long term, but hopefully the routing algorithm will be significant improvement already.

The next step is testing the interaction of payment attempts (and changing liquidity between payment attempts) with the router, and later seeing how it handles real world cases.

[tnull]

Sorry, for the delay here. Seems this needs a rebase, will then give it an initial round of review.

[tnull]

Please don't check in local files. In order to avoid this, you may want to add .vscode to your local gitignore file.

[adamritter]

Sorry, for the delay here. Seems this needs a rebase, will then give it an initial round of review.

Thanks Elias,

there's no delay, I just got stuck at the simulation/integration test code that I believe is important to be able to compare the routers and find cases where the MinCostRouter doesn't perform, or where it doesn't work well together with the current in-flight-HTLC tracking.

I could clean up and commit the router as it should work in theory, but I wouldn't have the tools to be able to find the bugs that are interesting for testing and improving the performance.

[TheBlueMatt]

Closing as abandoned 😢