Disable MPP routing when the payee does not support it #827

TheBlueMatt · 2021-03-05T03:38:50Z

We really need to provide a features object to show the router if the payee supports MPP, which we do through a new InvoiceFeatures object.

codecov · 2021-03-05T03:57:01Z

Codecov Report

Merging #827 (399f50e) into main (8b4ea56) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #827      +/-   ##
==========================================
+ Coverage   90.97%   90.99%   +0.01%     
==========================================
  Files          48       48              
  Lines       26538    26583      +45     
==========================================
+ Hits        24144    24189      +45     
  Misses       2394     2394

Impacted Files	Coverage Δ
lightning/src/chain/chainmonitor.rs	`93.93% <100.00%> (-0.27%)`	⬇️
lightning/src/chain/channelmonitor.rs	`95.54% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.56% <100.00%> (ø)`
lightning/src/ln/features.rs	`98.85% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`95.02% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`96.88% <100.00%> (ø)`
lightning/src/ln/onion_route_tests.rs	`96.85% <100.00%> (ø)`
lightning/src/routing/router.rs	`96.95% <100.00%> (+0.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b4ea56...b9fef85. Read the comment docs.

valentinewallace · 2021-03-05T16:42:06Z

Ah, I think this needs to add var_onion_option feature. Great to have this, working on getting the features from lightning-invoice now...

TheBlueMatt · 2021-03-05T17:40:26Z

Oops! Indeed, pushed an update to include var_onion_option.

naumenkogs · 2021-03-06T11:17:21Z

I don't know much about how we pass features over the codebase, but integration with get_route seems correct.

ACK babda7b

lightning/src/ln/features.rs

jkczyz

Looks like this fails to build on 1.30.0 and 1.34.2. I guess because of #821? Not sure why that wasn't caught there. :(

jkczyz · 2021-03-08T17:23:13Z

lightning/src/routing/router.rs

+/// Without this, any knowledge we have of the payee's features may be used if they are available
+/// in the network graph, however MPP routing will be disabled and routing is more likely to fail.


Maybe a bit clearer as:

s/features may be used if they are available in the network graph/features may be used in the network graph if they are available

Or set off "if they are available" as "(if available)".

I rewrote it a bunch more.

jkczyz · 2021-03-08T17:28:35Z

lightning/src/routing/router.rs

+/// Without this, any knowledge we have of the payee's features may be used if they are available
+/// in the network graph, however MPP routing will be disabled and routing is more likely to fail.


grammar nit: "however" is not a coordinating conjunction

Either replace it with "but" or use a semi-colon or start a new sentence.

That part of the sentence was stale anyway :)

jkczyz · 2021-03-08T17:37:05Z

lightning/src/routing/router.rs

+
+	#[bench]
+	fn generate_mpp_routes(bench: &mut Bencher) {
+		let mut d = File::open("net_graph-2021-02-12.bin").expect("Please fetch https://bitcoin.ninja/ldk-net_graph-879e309c128-2020-02-12.bin and place it at lightning/net_graph-2021-02-12.bin");


I noticed that generate_routes has been failing on continuous integration. Does this need to be setup there?

Yea, I was braindead when MPP got merged - a last-minute change caused the benchmark to fail due to a bug. It should be fixed in #815, but I'm still working on finishing it. I'm gonna move bindings out of the main repo today so that we get PRs to be all-green normally, making it harder to miss a red X.

In the past we skipped doing this since invoice parsing occurs in a different crate. However, we need to accept InvoiceFeatures in routing now that we support MPP route collection, to detect if we can select multiple paths or not. Further, we should probably take rust-lightning-invoice as either a module or a subcrate in this repo.

TheBlueMatt · 2021-03-08T18:37:41Z

Oops, yea, rebased and fixed the build.

jkczyz · 2021-03-08T20:23:02Z

lightning/src/routing/router.rs

@@ -328,7 +328,7 @@ fn compute_fees(amount_msat: u64, channel_fees: RoutingFees) -> Option<u64> {
 /// The fees on channels from us to next-hops are ignored (as they are assumed to all be
 /// equal), however the enabled/disabled bit on such channels as well as the
 /// htlc_minimum_msat/htlc_maximum_msat *are* checked as they may change based on the receiving node.
-pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, payee: &PublicKey, first_hops: Option<&[&ChannelDetails]>,
+pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, payee: &PublicKey, payee_features: Option<InvoiceFeatures>, first_hops: Option<&[&ChannelDetails]>,


High-level question on how this should be used:

Rather than passing in None in a ton of places, would it be preferable for the caller to modify the returned route? Or maybe instead provide some sort of wrapper that given an InvoiceFeatures calls get_route and updates the returned value as necessary?

Hmm, I strongly think its not preferable to have the user modify the Route object. While its all visible, it really shouldn't fall to the user to figure out how to tweak it properly. If you're worried about the number of arguments, we could pull them out into an args struct, but the types/argument names are all pretty clear.

I was leaning more to the second option of adding something like get_route_using_features which delegates to get_route and then updates the result before returning. Then get_route doesn't need an extra parameter and all those call sites taking None, which isn't very readable (what is None for?).

In this case the None-or-not also impacts how we build the route (MPP or not), so we can't just drop it and copy it into the route at the end.

Not sure I follow. Isn't lines 969-974 doing just that?

Yes, but lines 402-411 is also checking the features object provided to decide if we can use MPP.

Ah, that's what I get for looking at just one commit. :P I still wonder if we could come up with a better API, which would probably easier to do if get_route was refactor into smaller functions. I feel like passing None in places is similar to passing a bool. Just makes reading code difficult.

No need to hold up on this, BTW. We could take a more wholistic look at the APIs later.

Yea, generally agree, it definitely makes some tests harder. That said, in "real" applications it'd be somewhat "wrong" to pass None in any of the fields - the last-hop hints and invoice features should be at least be passed if they're present.

We currently only use it to override the graph-specific features returned in the route, though we should also use it to enable or disable MPP. Note that tests which relied on MPP behavior have had all of their get_route calls upgraded to provide the MPP flag.

TheBlueMatt · 2021-03-08T22:19:52Z

Squashed. Lets follow up on further API improvements in followup PRs if it looks bad in eg the sample node or in Overtorment's work.

valentinewallace

LGTM ⭐ I've been using this in the sample for a while as well.

TheBlueMatt force-pushed the 2021-03-invoice-features branch from 8a18913 to fc7227b Compare March 5, 2021 03:47

TheBlueMatt force-pushed the 2021-03-invoice-features branch from fc7227b to babda7b Compare March 5, 2021 17:40

TheBlueMatt added this to the 0.0.13 milestone Mar 5, 2021

valentinewallace reviewed Mar 8, 2021

View reviewed changes

lightning/src/ln/features.rs Show resolved Hide resolved

TheBlueMatt force-pushed the 2021-03-invoice-features branch from babda7b to ed824a5 Compare March 8, 2021 01:34

jkczyz reviewed Mar 8, 2021

View reviewed changes

lightning/src/ln/features.rs Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the 2021-03-invoice-features branch from ed824a5 to 9811e81 Compare March 8, 2021 17:19

jkczyz reviewed Mar 8, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-03-invoice-features branch from 9811e81 to a43905c Compare March 8, 2021 18:37

jkczyz reviewed Mar 8, 2021

View reviewed changes

TheBlueMatt added 3 commits March 8, 2021 17:19

Disable MPP routing when the payee does not support it

ef0f249

Split router benchmark into an MPP and a non-MPP route benchmark

b9fef85

TheBlueMatt force-pushed the 2021-03-invoice-features branch from a43905c to b9fef85 Compare March 8, 2021 22:19

TheBlueMatt mentioned this pull request Mar 8, 2021

Bump Version to 0.0.13 #835

Merged

jkczyz approved these changes Mar 9, 2021

View reviewed changes

valentinewallace approved these changes Mar 9, 2021

View reviewed changes

TheBlueMatt merged commit c896461 into lightningdevkit:main Mar 9, 2021

		/// Without this, any knowledge we have of the payee's features may be used if they are available
		/// in the network graph, however MPP routing will be disabled and routing is more likely to fail.

Disable MPP routing when the payee does not support it #827

Disable MPP routing when the payee does not support it #827

Uh oh!

Conversation

TheBlueMatt commented Mar 5, 2021

Uh oh!

codecov bot commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

valentinewallace commented Mar 5, 2021

Uh oh!

TheBlueMatt commented Mar 5, 2021

Uh oh!

naumenkogs commented Mar 6, 2021

Uh oh!

Uh oh!

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Mar 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Mar 8, 2021

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Mar 5, 2021 •

edited

Loading