Routing improvements #651

naumenkogs · 2020-06-29T18:37:32Z

A bunch of tiny fixes and introduction of 2 fields: htlc_maximum_msat (on channel update) and channel capacity, which would be later used in #646

lightning/src/ln/channel.rs

naumenkogs · 2020-06-30T12:27:58Z

@TheBlueMatt just noticed the issue with fuzzing, do you have an idea of what's wrong here? I'm going through the fuzz logs and it's not particularly helpful.

lightning/src/ln/channel.rs

lightning/src/util/config.rs

lightning/src/ln/channel.rs

valentinewallace

Thanks for the contribution @naumenkogs =]

lightning/src/ln/msgs.rs

lightning/src/util/test_utils.rs

lightning/src/util/config.rs

lightning/src/routing/network_graph.rs

lightning/src/ln/channel.rs

naumenkogs · 2020-07-15T11:12:17Z

Ready for review again :)
Would use some help with figuring why fuzzing fails though.

valentinewallace · 2020-07-15T22:21:17Z

Ready for review again :)
Would use some help with figuring why fuzzing fails though.

I was able to make progress on this by following the instructions here: https://github.com/rust-bitcoin/rust-lightning/tree/master/fuzz#a-fuzz-test-failed-on-travis-what-do-i-do

Looks related to writing out the excess data on an UnsignedChannelUpdate (specifically, it panics on this line).

lightning/src/ln/msgs.rs

TheBlueMatt

Looks good. A few minor nits, but great work!

lightning/src/ln/msgs.rs

TheBlueMatt · 2020-07-20T18:46:07Z

lightning/src/routing/network_graph.rs

+						}
+					}
+
+					// TODO check that htlc_maximum_msat is less than max_htlc_value_in_flight_msat


Why not just do it now :)

The spec says MUST set this to less than or equal to max_htlc_value_in_flight_msat it received from the peer..

I'm somewhat lost here... How can we even get this field (max_htlc_value_in_flight_msat)? I see it in OpenChannel/AcceptChannel messages, but those are relevant for our channels, not remote channels (for which we will receive channel updates).

Ohhh, sorry, I'd missed that that field isnt in the routing gossip. No, so what we should be doing is including max_htlc_value_in_flight in ChannelDetails in channelmanager and then use that via the first_hops argument in get_route. No need to bother crossing layers to enforce the rule in the routing graph when we already allow next-hop channels to override the graph completely.

Let me re-iterate on your suggestion:

Add max_htlc_value_in_flight field to ChannelDetails struct

When calling for ChannelDetails by list_channels, this value should be filled from the Channel record

Then a user will be providing these channels to get_route via first_hops field

Inside get_route, make sure max_htlc_value_in_flight is not violated by the payment we're sending.

But this has absolutely nothing to do with htlc_maximum_msat, which was why I initially added this comment to implement a check per the spec?
So you are suggesting to just ignore that spec part, and implement the flow I listed above?

Yea, definitely belongs in a separate pr, and your understanding is correct. This TODO is just out of place given that context.

lightning/src/ln/msgs.rs

codecov · 2020-07-23T17:09:17Z

Codecov Report

Merging #651 into master will increase coverage by 0.04%.
The diff coverage is 99.23%.

@@            Coverage Diff             @@
##           master     #651      +/-   ##
==========================================
+ Coverage   91.25%   91.29%   +0.04%     
==========================================
  Files          35       35              
  Lines       21281    21388     +107     
==========================================
+ Hits        19420    19527     +107     
  Misses       1861     1861

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`96.58% <95.65%> (+0.12%)`	⬆️
lightning/src/ln/channel.rs	`86.64% <100.00%> (ø)`
lightning/src/ln/channelmanager.rs	`85.25% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.14% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/msgs.rs	`90.31% <100.00%> (+0.27%)`	⬆️
lightning/src/routing/network_graph.rs	`91.43% <100.00%> (+0.36%)`	⬆️
lightning/src/util/test_utils.rs	`86.13% <100.00%> (+0.05%)`	⬆️
lightning/src/util/enforcing_trait_impls.rs	`100.00% <0.00%> (ø)`
... and 1 more

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 50df4cf...dd0e4f4. Read the comment docs.

TheBlueMatt · 2020-07-23T17:12:37Z

lightning/src/ln/channelmanager.rs

@@ -1186,7 +1186,9 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 							res.extend_from_slice(&byte_utils::be32_to_array(msg.cltv_expiry));
 						}
 						else if code == 0x1000 | 20 {
-							res.extend_from_slice(&byte_utils::be16_to_array(chan_update.contents.flags));
+							let mut flags = chan_update.contents.flags as u16;
+							if let OptionalField::Present(_) = chan_update.contents.htlc_maximum_msat {flags |= 1 << 8};


flags is a u8, I think this diff hunk can be droped.

chan_update.contents.flags is u8, so I get this compile error: expected u16, found u8``
If I switch to res.push(upd.contents.flags);, 4 tests fail.

But also, I don't understand why you're suggesting to omit the message_flags part of flags?

Oh, interesting, I think this is a misreading of the spec (in the original code). See lightning/bolts#791 but my read is that we should likely be pushing two zero bytes, not the flags bytes from the update.

TheBlueMatt · 2020-07-23T17:12:43Z

lightning/src/ln/channelmanager.rs

@@ -2494,7 +2497,9 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 							let reason = if let Ok(upd) = self.get_channel_update(chan) {
 								onion_utils::build_first_hop_failure_packet(incoming_shared_secret, error_code, &{
 									let mut res = Vec::with_capacity(8 + 128);
-									res.extend_from_slice(&byte_utils::be16_to_array(upd.contents.flags));
+									let mut flags = upd.contents.flags as u16;
+									if let OptionalField::Present(_) = upd.contents.htlc_maximum_msat {flags |= 1 << 8};


TheBlueMatt reviewed Jun 29, 2020

View reviewed changes

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

naumenkogs force-pushed the 2020-06-routing-data-improvements branch from 6818e76 to 922be75 Compare June 30, 2020 11:45

TheBlueMatt reviewed Jul 2, 2020

View reviewed changes

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

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

lightning/src/util/config.rs Outdated Show resolved Hide resolved

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

naumenkogs force-pushed the 2020-06-routing-data-improvements branch from 922be75 to 5f551a8 Compare July 3, 2020 07:41

naumenkogs mentioned this pull request Jul 8, 2020

Store + use channel value pulled from the chain to inform routing #483

Closed

valentinewallace self-requested a review July 9, 2020 21:35

valentinewallace reviewed Jul 10, 2020

View reviewed changes

naumenkogs force-pushed the 2020-06-routing-data-improvements branch 2 times, most recently from a96ba6c to a761fb4 Compare July 15, 2020 10:54

naumenkogs commented Jul 16, 2020

View reviewed changes

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

naumenkogs force-pushed the 2020-06-routing-data-improvements branch 3 times, most recently from 26fb2e0 to 80308d8 Compare July 18, 2020 13:52

TheBlueMatt reviewed Jul 20, 2020

View reviewed changes

naumenkogs force-pushed the 2020-06-routing-data-improvements branch 3 times, most recently from 8b62d1a to d2bd959 Compare July 22, 2020 12:15

naumenkogs added 4 commits July 22, 2020 16:20

Update get_route comment to reflect new parameters

7838727

Use constant for max msats

5309197

Fix formatting

430d57c

Improve routing announcement test

a66b801

naumenkogs force-pushed the 2020-06-routing-data-improvements branch 6 times, most recently from b0141e2 to f3b4d8d Compare July 23, 2020 11:20

naumenkogs force-pushed the 2020-06-routing-data-improvements branch from f3b4d8d to 45ee4e8 Compare July 23, 2020 11:31

TheBlueMatt reviewed Jul 23, 2020

View reviewed changes

Update comment for test_msg_hole

b8fc761

naumenkogs force-pushed the 2020-06-routing-data-improvements branch 2 times, most recently from 905ba70 to a086924 Compare July 27, 2020 10:53

naumenkogs added 3 commits July 27, 2020 14:06

Add htlc_maximum_msat field

8b4f6e8

Store channel capacity if available

3a57cfc

Check htlc_maximum_msat on channel update

dd0e4f4

naumenkogs force-pushed the 2020-06-routing-data-improvements branch from a086924 to dd0e4f4 Compare July 27, 2020 11:06

TheBlueMatt merged commit 779ff67 into lightningdevkit:master Jul 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Routing improvements #651

Routing improvements #651

naumenkogs commented Jun 29, 2020

naumenkogs commented Jun 30, 2020

valentinewallace left a comment

naumenkogs commented Jul 15, 2020

valentinewallace commented Jul 15, 2020 •

edited

Loading

TheBlueMatt left a comment

TheBlueMatt Jul 20, 2020

naumenkogs Jul 21, 2020

TheBlueMatt Jul 22, 2020

naumenkogs Jul 23, 2020

TheBlueMatt Jul 23, 2020

codecov bot commented Jul 23, 2020 •

edited

Loading

TheBlueMatt Jul 23, 2020

naumenkogs Jul 24, 2020

TheBlueMatt Jul 24, 2020

naumenkogs Jul 27, 2020

TheBlueMatt Jul 23, 2020

Routing improvements #651

Routing improvements #651

Conversation

naumenkogs commented Jun 29, 2020

naumenkogs commented Jun 30, 2020

valentinewallace left a comment

Choose a reason for hiding this comment

naumenkogs commented Jul 15, 2020

valentinewallace commented Jul 15, 2020 • edited Loading

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 23, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Jul 15, 2020 •

edited

Loading

codecov bot commented Jul 23, 2020 •

edited

Loading