Track lowest inbound channel fees per channel direction

[naumenkogs]

This allows to avoid iterating through the nodes data to see what would be the cost to reach a given node while routing.

Closes #1686.

[TheBlueMatt]

First, I don't think this actually addresses #1686 - that issue is (mostly) about actually ensuring the value is correct when we remove channels, which it currently is not. Secondly, this needs some kind of upgrade story. Luckily both problems have the same solution - have a method to recalculate the lowest inbound values for a given peer and then fill them in in all the relevant channels, we'll just have to use it on upgrade and in some of the removal functions.

[codecov-commenter]

Codecov Report

Base: 90.84% // Head: 92.41% // Increases project coverage by +1.56% 🎉

Coverage data is based on head (ae53d1c) compared to base (15a5966).
Patch coverage: 97.64% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1722      +/-   ##
==========================================
+ Coverage   90.84%   92.41%   +1.56%     
==========================================
  Files          86       86              
  Lines       46448    58282   +11834     
  Branches    46448    58282   +11834     
==========================================
+ Hits        42198    53863   +11665     
- Misses       4250     4419     +169     

Impacted Files	Coverage Δ
lightning/src/routing/gossip.rs	93.78% <97.43%> (+2.36%)	⬆️
lightning/src/routing/router.rs	92.08% <100.00%> (+0.01%)	⬆️
lightning-block-sync/src/init.rs	93.03% <0.00%> (-0.54%)	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	97.47% <0.00%> (-0.25%)	⬇️
lightning/src/ln/shutdown_tests.rs	96.36% <0.00%> (-0.16%)	⬇️
lightning/src/ln/script.rs	92.13% <0.00%> (-0.06%)	⬇️
lightning/src/ln/payment_tests.rs	98.80% <0.00%> (-0.02%)	⬇️
lightning/src/ln/reorg_tests.rs	100.00% <0.00%> (ø)
lightning/src/ln/functional_test_utils.rs	93.57% <0.00%> (+0.12%)	⬆️
lightning/src/ln/monitor_tests.rs	99.68% <0.00%> (+0.12%)	⬆️
... and 34 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Hmm, okay, let's see. So I went and re-benchmarked using the A* vs traditional dijkstras a bit (by running the recalculate function for every node on NetworkGraph load) and....it really doesn't do a whole lot anymore. For 1000 get_route calls in our benchmarks, the number of priority-queue-pop operations went from:

• generate_routes_with_zero_penalty_scorer 5095270 -> 5090910
• generate_routes_with_probabilistic_scorer 6581750 -> 6615840
• generate_mpp_routes_with_zero_penalty_scorer 14581080 -> 14588420
• generate_mpp_routes_with_probabilistic_scorer 19179700 -> 19233330

I don't really understand why the complexity of the MPP route calls would have gone up, but a big part of the problem appears to be that these days there's lots and lots of have-at-least-one-zero-base-zero-proportional fee channel nodes, in the same tests I see, by simply measuring when we insert into dist,

• generate_routes_with_zero_penalty_scorer 7797872/10697512 were zero.
• generate_routes_with_probabilistic_scorer 8778315/11966592 were zero.
• generate_mpp_routes_with_zero_penalty_scorer 20656457/28377549 were zero.
• generate_mpp_routes_with_probabilistic_scorer 23613677/32233936 were zero.

Given that, I'm wondering if we should just rip out all the A* stuff and go full-dijkstras.

[TheBlueMatt]

Also re-did the %-zeros calculations for a newer snapshot (the benchmark one is now more than a year old) and get:

• generate_routes_with_zero_penalty_scorer 12140664/13092404 were zero.
• generate_routes_with_probabilistic_scorer 13645093/14470466 were zero.
• generate_mpp_routes_with_zero_penalty_scorer 34095178/36668782 were zero.
• generate_mpp_routes_with_probabilistic_scorer 38170435/40536573 were zero.

which seems like pretty strong indication the problem is getting much worse.

[tnull]

Given that, I'm wondering if we should just rip out all the A* stuff and go full-dijkstras.

Hum, IIUC going back to a Dijkstra could generally simplify things quite a bit. So primarily spoken from a maintainability perspective, this might be a good idea, if there's not a massive performance difference anymore. Also, I started playing around a bit with replacing the macros with inlined methods in an experimental branch a while back, which might get quite a bit easier by switching back. I think if we decide to switch back I'd like to give this another go and see what the benchmarks say.

[naumenkogs]

@TheBlueMatt I'm not sure I understand what you're saying 100% so I'll try to rephrase and please correct me if I'm wrong.

My code hasn't improved the performance of A* (even made a little worse), although we replaced an expensive map call with trivial in-memory field access.

I think one reason could be the other part we updated (keeping up-to-date data). So perhaps drop newly added recompute_and_update_lowest_inbound_channel_fees calls and compare? If this would make more sense, it would mean that my PR simply increased the routing quality while keeping performance the same. But that's probably not that important in the end, when we compare to Dijkstra (see below).

---

Are you implying because zero-fee channels are so common A* is bad at optimizing, and performs similarly to Dijkstra?

I don't see a good way to check this theory (perhaps if fee==zero skip?), but since the numbers speak for themselves, I probably agree to switch full-Dijkstra for the current network.

[TheBlueMatt]

Yes, to both, I'm saying that both I was unable to see a material performance difference between current git and this (on a very modern x86) and A* no longer materially changes the algorithm behavior since the value is so often zero.

Re: the performance difference, it may be that the hash table lookup is, in effect, able to run in parallel with the rest of the algorithm, given the result isn't used for some time and there's a lot of additional branching and comparisons before we go to look up the next node. That said, benchmarking exclusively on a very modern x86 may be too naive here - x86/Intel tends to be very good at this kind of thing, but we may see a very substantial improvement on other platforms.

Re: A* not being so useful anymore, yea, given its increasingly rare for A* to add any value, we could clean up a good chunk of code by dropping it entirely and switching back to classic Dijkstras.

Maybe we just need to start a popular twitter campaign for nodes to increase their fees? #nonzerobasefee 😂

[naumenkogs]

So practically speaking, unless someone shows up and gives us new evidence/idea, we should close this PR/issue and bring back Dijkstra instead, i guess.

[TheBlueMatt]

Yea, I think so. The shift in % of nodes with at least one 0-fee-channel between 2021 and today is pretty damning.

[naumenkogs]

I guess once #1724 is merged, we can use that code for routing payments unless some new good news about A* emerge.

[TheBlueMatt]

I don't think so - the payment routing logic is complicated almost entirely because of the various limits we have, not the A* part, that's trivial. We should just modify the existing one to remove the A* logic (unless we come up with some super clever A* heuristic).