Add logging in route hint filtering #1770

jkczyz · 2022-10-14T16:15:25Z

Knowing why a channel was not included as an invoice route hint can be valuable. Add logging on the decisions made when filtering channels.

codecov-commenter · 2022-10-14T16:39:21Z

Codecov Report

Base: 90.91% // Head: 90.91% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (7bc5d46) compared to base (a534dcc).
Patch coverage: 97.26% of modified lines in pull request are covered.

❗ Current head 7bc5d46 differs from pull request most recent head b75e111. Consider uploading reports for the commit b75e111 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1770   +/-   ##
=======================================
  Coverage   90.91%   90.91%           
=======================================
  Files          87       87           
  Lines       48048    48078   +30     
  Branches    48048    48078   +30     
=======================================
+ Hits        43681    43711   +30     
  Misses       4367     4367

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`95.13% <96.72%> (+0.31%)`	⬆️
lightning-invoice/src/payment.rs	`89.85% <100.00%> (ø)`
lightning/src/util/logger.rs	`86.13% <100.00%> (+0.72%)`	⬆️
lightning/src/util/macro_logger.rs	`86.15% <100.00%> (-0.99%)`	⬇️
lightning/src/util/events.rs	`38.13% <0.00%> (-0.27%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.96% <0.00%> (-0.02%)`	⬇️
lightning-net-tokio/src/lib.rs	`77.03% <0.00%> (+0.30%)`	⬆️

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.

lightning-invoice/src/utils.rs

ViktorTigerstrom

Looks good and is definitely helpful :)! Just leaving some suggestions for clarifications for phantom invoice route hints filtering.

ViktorTigerstrom · 2022-10-16T23:20:42Z

lightning-invoice/src/utils.rs

 			continue;
 		}

 		if channel.is_public {
 			// If any public channel exists, return no hints and let the sender
 			// look at the public channels instead.
+			log_debug!(logger, "Not including invoice route hints on account of public channel {}",


This wording will be pretty confusing for phantom invoices as well. Say a phantom invoice is generated using 3 participating nodes, where only one of the nodes have public channels, but the other 2 have only private channels. That invoice will include invoice route hints for the channels of the 2 participating nodes with private channels only, yet log this message due to the 3rd participating node with public channel(s), hence incorrectly indicating that no invoice route hints were included.

Also note that phantom invoices will include invoice route hints for every participating node even if all of them have public channels. Phantom hops with the node id for every participating node will be included in the invoice route hints due to:

rust-lightning/lightning-invoice/src/utils.rs

Line 178 in 6772609

route_hints.push(RouteHint(vec![]))

Perhaps we could phrase this sentence as something like:
"Non of the considered channels are included in the invoice route hints on account of public channel {}."

Not sure what the best way to phrase this is though :)!

Tweaked the wording. Should be clearer now with the additional logging, too.

lightning-invoice/src/utils.rs

ViktorTigerstrom · 2022-10-16T23:56:25Z

lightning-invoice/src/utils.rs

+			let has_enough_inbound_capacity = channel.inbound_capacity_msat >= min_inbound_capacity;
+			let include_channel = !min_capacity_channel_exists || has_enough_inbound_capacity;
+			if !include_channel {
+				log_debug!(logger, "Ignoring channel {} without enough capacity for invoice route hints",
+					log_bytes!(channel.channel_id));
+			}
+			include_channel


This will clash a little with the cleanup in #1769 :). I have no preference for which version we go with, but could be worth considering by either version.

Based on #1769 now though that may need some changes to the filtering logic still.

TheBlueMatt · 2022-10-19T16:54:20Z

Feel free to squash, basically LGTM but the super verbose channel-inclusion-exclusion stuff may make more sense as TRACE, or at least the why-was-it-excluded ones.

jkczyz · 2022-10-20T02:51:55Z

Feel free to squash, basically LGTM but the super verbose channel-inclusion-exclusion stuff may make more sense as TRACE, or at least the why-was-it-excluded ones.

Hmm... seems like why a channel was excluded is more important than which channels were included given the latter can be gleaned from the invoice? I'm fine with making everything log_trace, if you'd like.

And anything logged outside filter_channels isn't all that important without having the logging in filter_channels.

jkczyz · 2022-10-20T03:38:34Z

Squashed and changed all logging to log_trace for now.

ViktorTigerstrom

LGTM, leaving one suggestion that's just a nice to have!

ViktorTigerstrom · 2022-10-20T12:33:27Z

lightning-invoice/src/utils.rs

 		match filtered_channels.entry(channel.counterparty.node_id) {
 			hash_map::Entry::Occupied(mut entry) => {
 				let current_max_capacity = entry.get().inbound_capacity_msat;
 				if channel.inbound_capacity_msat < current_max_capacity {
 					continue;
 				}
+				log_trace!(logger, "Preferring channel {} ({} msats) over {} ({} msats) for invoice route hints",


Could also be worth mentioning the counterparty node id for the channel here, due to the filtering only including 1 channel per counterparty node.

ViktorTigerstrom · 2022-10-20T12:38:26Z

lightning-invoice/src/utils.rs

 		}
+
 		match filtered_channels.entry(channel.counterparty.node_id) {
 			hash_map::Entry::Occupied(mut entry) => {
 				let current_max_capacity = entry.get().inbound_capacity_msat;
 				if channel.inbound_capacity_msat < current_max_capacity {


Actually sorry, we should probably log here that the channel is ignored, no?

TheBlueMatt · 2022-10-20T19:52:05Z

If it's alright with Viktor, feel free to keep this one squashed so we can land it ASAP.

ViktorTigerstrom · 2022-10-20T20:06:56Z

That's ok for me @TheBlueMatt! The new changes looks good. They could possibly be dryed up a little instead of being repeated twice, but then again being this explicit makes it easier to read, so the current version is fine for me:). LGTM once squashed.

Knowing why a channel was not included as an invoice route hint can be valuable. Add logging on the decisions made when filtering channels.

valentinewallace · 2022-10-21T16:14:19Z

lightning-invoice/src/utils.rs

@@ -353,6 +375,8 @@ where
 	if let Some(amt) = amt_msat {
 		invoice = invoice.amount_milli_satoshis(amt);
 	}
+
+	let route_hints = filter_channels(channels, amt_msat, &logger);


Some changes in this method seem unrelated

I need the filter_channels logging to come after the logging above, the latter of which needs payment_hash.

jkczyz force-pushed the 2022-10-debug-route-hints branch from e7d17e3 to 2545426 Compare October 14, 2022 16:18

jurvis reviewed Oct 14, 2022

View reviewed changes

lightning-invoice/src/utils.rs Outdated Show resolved Hide resolved

lightning-invoice/src/utils.rs Outdated Show resolved Hide resolved

ViktorTigerstrom reviewed Oct 16, 2022

View reviewed changes

jkczyz added this to the 0.0.112 milestone Oct 17, 2022

Clean up private channel route hint filtering

92a1e49

jkczyz force-pushed the 2022-10-debug-route-hints branch from 2545426 to 7bc5d46 Compare October 18, 2022 22:21

jkczyz force-pushed the 2022-10-debug-route-hints branch from 7bc5d46 to 33a5b89 Compare October 20, 2022 03:35

ViktorTigerstrom previously approved these changes Oct 20, 2022

View reviewed changes

ViktorTigerstrom reviewed Oct 20, 2022

View reviewed changes

jkczyz dismissed ViktorTigerstrom’s stale review via b75e111 October 20, 2022 14:46

Add logging in route hint filtering

90c9763

Knowing why a channel was not included as an invoice route hint can be valuable. Add logging on the decisions made when filtering channels.

jkczyz force-pushed the 2022-10-debug-route-hints branch from b75e111 to 90c9763 Compare October 20, 2022 20:18

ViktorTigerstrom approved these changes Oct 20, 2022

View reviewed changes

TheBlueMatt approved these changes Oct 20, 2022

View reviewed changes

TheBlueMatt added the Seeking Code Review label Oct 20, 2022

valentinewallace self-requested a review October 21, 2022 15:41

valentinewallace approved these changes Oct 21, 2022

View reviewed changes

valentinewallace merged commit ad82c9e into lightningdevkit:main Oct 21, 2022

Add logging in route hint filtering #1770

Add logging in route hint filtering #1770

Uh oh!

Conversation

jkczyz commented Oct 14, 2022

Uh oh!

codecov-commenter commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

Uh oh!

ViktorTigerstrom Oct 16, 2022

Choose a reason for hiding this comment

Uh oh!

jkczyz Oct 18, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ViktorTigerstrom Oct 16, 2022

Choose a reason for hiding this comment

Uh oh!

jkczyz Oct 18, 2022

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 19, 2022

Uh oh!

jkczyz commented Oct 20, 2022

Uh oh!

jkczyz commented Oct 20, 2022

Uh oh!

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

Uh oh!

ViktorTigerstrom Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

ViktorTigerstrom Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 20, 2022

Uh oh!

ViktorTigerstrom commented Oct 20, 2022

Uh oh!

valentinewallace Oct 21, 2022

Choose a reason for hiding this comment

Uh oh!

jkczyz Oct 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Oct 14, 2022 •

edited

Loading

ViktorTigerstrom Oct 20, 2022 •

edited

Loading