Skip to content

Commit 9e99577

Browse files
committed
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.
1 parent fdfd4f0 commit 9e99577

File tree

1 file changed

+59
-11
lines changed

1 file changed

+59
-11
lines changed

lightning-invoice/src/utils.rs

+59-11
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,7 @@ where
326326
F::Target: FeeEstimator,
327327
L::Target: Logger,
328328
{
329-
let route_hints = filter_channels(channelmanager.list_usable_channels(), amt_msat);
329+
let route_hints = filter_channels(channelmanager.list_channels(), amt_msat);
330330

331331
// `create_inbound_payment` only returns an error if the amount is greater than the total bitcoin
332332
// supply.
@@ -382,11 +382,13 @@ where
382382
/// * If any public channel exists, the returned `RouteHint`s will be empty, and the sender will
383383
/// need to find the path by looking at the public channels instead
384384
fn filter_channels(channels: Vec<ChannelDetails>, min_inbound_capacity_msat: Option<u64>) -> Vec<RouteHint>{
385-
let mut filtered_channels: HashMap<PublicKey, &ChannelDetails> = HashMap::new();
385+
let mut filtered_channels: HashMap<PublicKey, ChannelDetails> = HashMap::new();
386386
let min_inbound_capacity = min_inbound_capacity_msat.unwrap_or(0);
387387
let mut min_capacity_channel_exists = false;
388+
let mut online_channel_exists = false;
389+
let mut online_min_capacity_channel_exists = false;
388390

389-
for channel in channels.iter() {
391+
for channel in channels.into_iter().filter(|chan| chan.is_channel_ready) {
390392
if channel.get_inbound_payment_scid().is_none() || channel.counterparty.forwarding_info.is_none() {
391393
continue;
392394
}
@@ -399,7 +401,13 @@ fn filter_channels(channels: Vec<ChannelDetails>, min_inbound_capacity_msat: Opt
399401

400402
if channel.inbound_capacity_msat >= min_inbound_capacity {
401403
min_capacity_channel_exists = true;
402-
};
404+
if channel.is_usable {
405+
online_min_capacity_channel_exists = true;
406+
}
407+
}
408+
if channel.is_usable {
409+
online_channel_exists = true;
410+
}
403411
match filtered_channels.entry(channel.counterparty.node_id) {
404412
hash_map::Entry::Occupied(mut entry) => {
405413
let current_max_capacity = entry.get().inbound_capacity_msat;
@@ -414,7 +422,7 @@ fn filter_channels(channels: Vec<ChannelDetails>, min_inbound_capacity_msat: Opt
414422
}
415423
}
416424

417-
let route_hint_from_channel = |channel: &ChannelDetails| {
425+
let route_hint_from_channel = |channel: ChannelDetails| {
418426
let forwarding_info = channel.counterparty.forwarding_info.as_ref().unwrap();
419427
RouteHint(vec![RouteHintHop {
420428
src_node_id: channel.counterparty.node_id,
@@ -427,15 +435,26 @@ fn filter_channels(channels: Vec<ChannelDetails>, min_inbound_capacity_msat: Opt
427435
htlc_minimum_msat: channel.inbound_htlc_minimum_msat,
428436
htlc_maximum_msat: channel.inbound_htlc_maximum_msat,}])
429437
};
430-
// If all channels are private, return the route hint for the highest inbound capacity channel
431-
// per counterparty node. If channels with an higher inbound capacity than the
432-
// min_inbound_capacity exists, filter out the channels with a lower capacity than that.
438+
// If all channels are private, prefer to return route hints which have a higher capacity than
439+
// the payment value and where we're currently connected to the channel counterparty.
440+
// Even if we cannot satisfy both goals, always ensure we include *some* hints, preferring
441+
// those which meet at least one criteria.
433442
filtered_channels.into_iter()
434443
.filter(|(_counterparty_id, channel)| {
435-
min_capacity_channel_exists && channel.inbound_capacity_msat >= min_inbound_capacity ||
436-
!min_capacity_channel_exists
444+
if online_min_capacity_channel_exists {
445+
channel.inbound_capacity_msat >= min_inbound_capacity && channel.is_usable
446+
} else if min_capacity_channel_exists && online_channel_exists {
447+
// If there are some online channels and some min_capacity channels, but no
448+
// online-and-min_capacity channels, just include the min capacity ones and ignore
449+
// online-ness.
450+
channel.inbound_capacity_msat >= min_inbound_capacity
451+
} else if min_capacity_channel_exists {
452+
channel.inbound_capacity_msat >= min_inbound_capacity
453+
} else if online_channel_exists {
454+
channel.is_usable
455+
} else { true }
437456
})
438-
.map(|(_counterparty_id, channel)| route_hint_from_channel(&channel))
457+
.map(|(_counterparty_id, channel)| route_hint_from_channel(channel))
439458
.collect::<Vec<RouteHint>>()
440459
}
441460

@@ -723,6 +742,35 @@ mod test {
723742
match_invoice_routes(Some(5000), &nodes[0], scid_aliases);
724743
}
725744

745+
#[test]
746+
fn test_hints_has_only_online_channels() {
747+
let chanmon_cfgs = create_chanmon_cfgs(4);
748+
let node_cfgs = create_node_cfgs(4, &chanmon_cfgs);
749+
let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
750+
let nodes = create_network(4, &node_cfgs, &node_chanmgrs);
751+
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());
752+
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());
753+
let _chan_c = create_unannounced_chan_between_nodes_with_value(&nodes, 3, 0, 1_000_000, 0, channelmanager::provided_init_features(), channelmanager::provided_init_features());
754+
755+
// With all peers connected we should get all hints that have sufficient value
756+
let mut scid_aliases = HashSet::new();
757+
scid_aliases.insert(chan_a.0.short_channel_id_alias.unwrap());
758+
scid_aliases.insert(chan_b.0.short_channel_id_alias.unwrap());
759+
760+
match_invoice_routes(Some(1_000_000_000), &nodes[0], scid_aliases.clone());
761+
762+
// With only one sufficient-value peer connected we should only get its hint
763+
scid_aliases.remove(&chan_b.0.short_channel_id_alias.unwrap());
764+
nodes[0].node.peer_disconnected(&nodes[2].node.get_our_node_id(), false);
765+
match_invoice_routes(Some(1_000_000_000), &nodes[0], scid_aliases.clone());
766+
767+
// If we don't have any sufficient-value peers connected we should get all hints with
768+
// sufficient value, even though there is a connected insufficient-value peer.
769+
scid_aliases.insert(chan_b.0.short_channel_id_alias.unwrap());
770+
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
771+
match_invoice_routes(Some(1_000_000_000), &nodes[0], scid_aliases);
772+
}
773+
726774
#[test]
727775
fn test_forwarding_info_not_assigned_channel_excluded_from_hints() {
728776
let chanmon_cfgs = create_chanmon_cfgs(3);

0 commit comments

Comments
 (0)