Include all channel route hints if no connected channels exist

Mobile clients often take a second or two before they reconnect to
their peers as its not always clear immediately that connections
have been killed (especially on iOS). This can cause us to
spuriously fail to include route hints in our invoices if we're on
mobile.

The fix is simple, if we're selecting channels to include in route
hints and we're not not connected to the peer for any of our
channels, we should simply include the hints for all channels, even
though we're disconencted.

Fixes #1768.

Author: TheBlueMatt

lightning-invoice/src/utils.rs

@@ -326,7 +326,7 @@ where
 	F::Target: FeeEstimator,
 	L::Target: Logger,
 {
-	let route_hints = filter_channels(channelmanager.list_usable_channels(), amt_msat);
+	let route_hints = filter_channels(channelmanager.list_channels(), amt_msat);
 
 	// `create_inbound_payment` only returns an error if the amount is greater than the total bitcoin
 	// supply.
@@ -381,12 +381,13 @@ where
 /// channel with a higher or equal inbound capacity than `min_inbound_capacity_msat` exists
 /// * If any public channel exists, the returned `RouteHint`s will be empty, and the sender will
 /// need to find the path by looking at the public channels instead
-fn filter_channels(channels: Vec<ChannelDetails>, min_inbound_capacity_msat: Option<u64>) -> Vec<RouteHint>{
-	let mut filtered_channels: HashMap<PublicKey, &ChannelDetails> = HashMap::new();
+fn filter_channels(mut channels: Vec<ChannelDetails>, min_inbound_capacity_msat: Option<u64>) -> Vec<RouteHint>{
+	let mut filtered_channels: HashMap<PublicKey, ChannelDetails> = HashMap::new();
 	let min_inbound_capacity = min_inbound_capacity_msat.unwrap_or(0);
 	let mut min_capacity_channel_exists = false;
+	let mut online_channel_exists = false;
 
-	for channel in channels.iter() {
+	for channel in channels.drain(..).filter(|chan| chan.is_channel_ready) {
 		if channel.get_inbound_payment_scid().is_none() || channel.counterparty.forwarding_info.is_none() {
 			continue;
 		}
@@ -400,6 +401,9 @@ fn filter_channels(channels: Vec<ChannelDetails>, min_inbound_capacity_msat: Opt
 		if channel.inbound_capacity_msat >= min_inbound_capacity {
 			min_capacity_channel_exists = true;
 		};
+		if channel.is_usable {
+			online_channel_exists = true;
+		}
 		match filtered_channels.entry(channel.counterparty.node_id) {
 			hash_map::Entry::Occupied(mut entry) => {
 				let current_max_capacity = entry.get().inbound_capacity_msat;
@@ -414,7 +418,7 @@ fn filter_channels(channels: Vec<ChannelDetails>, min_inbound_capacity_msat: Opt
 		}
 	}
 
-	let route_hint_from_channel = |channel: &ChannelDetails| {
+	let route_hint_from_channel = |channel: ChannelDetails| {
 		let forwarding_info = channel.counterparty.forwarding_info.as_ref().unwrap();
 		RouteHint(vec![RouteHintHop {
 			src_node_id: channel.counterparty.node_id,
@@ -430,12 +434,19 @@ fn filter_channels(channels: Vec<ChannelDetails>, min_inbound_capacity_msat: Opt
 	// If all channels are private, return the route hint for the highest inbound capacity channel
 	// per counterparty node. If channels with an higher inbound capacity than the
 	// min_inbound_capacity exists, filter out the channels with a lower capacity than that.
+	// Further, if we are connected to our peer for any channels, only return those.
 	filtered_channels.into_iter()
 		.filter(|(_counterparty_id, channel)| {
-			min_capacity_channel_exists && channel.inbound_capacity_msat >= min_inbound_capacity ||
-			!min_capacity_channel_exists
+			if min_capacity_channel_exists {
+				channel.inbound_capacity_msat >= min_inbound_capacity
+			} else { true }
+		})
+		.filter(|(_counterparty_id, channel)| {
+			if online_channel_exists {
+				channel.is_usable
+			} else { true }
 		})
-		.map(|(_counterparty_id, channel)| route_hint_from_channel(&channel))
+		.map(|(_counterparty_id, channel)| route_hint_from_channel(channel))
 		.collect::<Vec<RouteHint>>()
 }
 
@@ -723,6 +734,33 @@ mod test {
 		match_invoice_routes(Some(5000), &nodes[0], scid_aliases);
 	}
 
+	#[test]
+	fn test_hints_has_only_online_channels() {
+		let chanmon_cfgs = create_chanmon_cfgs(3);
+		let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+		let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+		let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+		let chan_a = create_unannounced_chan_between_nodes_with_value(&nodes, 1, 0, 10_000_000, 0, channelmanager::provided_init_features(), channelmanager::provided_init_features());
+		let chan_b = create_unannounced_chan_between_nodes_with_value(&nodes, 2, 0, 10_000_000, 0, channelmanager::provided_init_features(), channelmanager::provided_init_features());
+
+		// With both peers connected we should get all hints
+		let mut scid_aliases = HashSet::new();
+		scid_aliases.insert(chan_a.0.short_channel_id_alias.unwrap());
+		scid_aliases.insert(chan_b.0.short_channel_id_alias.unwrap());
+
+		match_invoice_routes(Some(5000), &nodes[0], scid_aliases.clone());
+
+		// With only one peer connected other hints should go away
+		scid_aliases.remove(&chan_b.0.short_channel_id_alias.unwrap());
+		nodes[0].node.peer_disconnected(&nodes[2].node.get_our_node_id(), false);
+		match_invoice_routes(Some(5000), &nodes[0], scid_aliases.clone());
+
+		// With both peers disconnected we should just get all the hints
+		scid_aliases.insert(chan_b.0.short_channel_id_alias.unwrap());
+		nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+		match_invoice_routes(Some(5000), &nodes[0], scid_aliases);
+	}
+
 	#[test]
 	fn test_forwarding_info_not_assigned_channel_excluded_from_hints() {
 		let chanmon_cfgs = create_chanmon_cfgs(3);