Skip to content

Include all channel route hints if no connected channels exist #1769

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 19, 2022
Merged

Include all channel route hints if no connected channels exist #1769

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 62 additions & 14 deletions lightning-invoice/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

@ViktorTigerstrom ViktorTigerstrom Oct 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we like to do this for ChannelManager::get_phantom_route_hints as well so that create_phantom_invoice will include create hints with the same filtering if all participating nodes are disconnected or have just disconnected peers (although I guess this isn't really relevant for mobile clients), as phantom invoices are useless without any meaningful route hints?

rust-lightning/lightning/src/ln/channelmanager.rs

Line 5546 in 6772609

channels: self.list_usable_channels(),

If the case that all participating nodes have only private channels which are all currently !channel.is_usable, we'll currently push route hints containing only the participating node's id, but as there are no public channels to reach those participating nodes the route hint's will be useless, which seems kinda buggy.

That would make things a bit more complicated as we'd need filter_channels to only return usable channels if any of the phantom_route_hints in _create_phantom_invoice contains a channel with channel.is_usable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PhantomRouteHints are unlikely to be used in a mobile context. The related-utilities here are ChannelManager-less because these hints may be serialized and read for multiple nodes. We may even want to consider removing the peer connection requirement in get_phantom_route_hints.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, right, interesting question. I think for now let's leave it as-is - in a server environment the "is channel connected" concept is usually "is peer not down doing maintenance or something", which we'd ideally like to avoid including those channels for.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok as a follow-up/separate issue it might be worth it to at least add some validation and verify that the length of the channels vec in the PhantomRouteHints isn't 0? As it is now, we'll call filter_channels and add the phantom hop route hint that includes the participating node's id which can't be routed to in that case. Could also be worth ensuring that the final constructed invoice actually have some route hints depending on how the validation is implemented.


// `create_inbound_payment` only returns an error if the amount is greater than the total bitcoin
// supply.
Expand Down Expand Up @@ -377,16 +377,18 @@ where
/// The filtering is based on the following criteria:
/// * Only one channel per counterparty node
/// * Always select the channel with the highest inbound capacity per counterparty node
/// * Filter out channels with a lower inbound capacity than `min_inbound_capacity_msat`, if any
/// channel with a higher or equal inbound capacity than `min_inbound_capacity_msat` exists
/// * Prefer channels with capacity at least `min_inbound_capacity_msat` and where the channel
/// `is_usable` (i.e. the peer is connected).
/// * 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
/// 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();
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;
let mut online_min_capacity_channel_exists = false;

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

if channel.inbound_capacity_msat >= min_inbound_capacity {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we still return an empty vec if any public channel exists (5 lines up), since the public channels may be offline now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good question. I think not - we're really assuming that any channels that are disconnected now may be live by the time the sender goes to pay, so if we extend that assumption to public channels we dont need to include any hints. Further, I'd be concerned about accidentally nuking privacy if we did that.

min_capacity_channel_exists = true;
};
if channel.is_usable {
online_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;
Expand All @@ -414,7 +422,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,
Expand All @@ -427,15 +435,26 @@ fn filter_channels(channels: Vec<ChannelDetails>, min_inbound_capacity_msat: Opt
htlc_minimum_msat: channel.inbound_htlc_minimum_msat,
htlc_maximum_msat: channel.inbound_htlc_maximum_msat,}])
};
// 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.
// If all channels are private, prefer to return route hints which have a higher capacity than
// the payment value and where we're currently connected to the channel counterparty.
// Even if we cannot satisfy both goals, always ensure we include *some* hints, preferring
// those which meet at least one criteria.
filtered_channels.into_iter()
.filter(|(_counterparty_id, channel)| {
min_capacity_channel_exists && channel.inbound_capacity_msat >= min_inbound_capacity ||
!min_capacity_channel_exists
if online_min_capacity_channel_exists {
channel.inbound_capacity_msat >= min_inbound_capacity && channel.is_usable
} else if min_capacity_channel_exists && online_channel_exists {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acc. to this comment "// If there are some online channels and some min_capacity channels, but no
// online-and-min_capacity channels, just include the min capacity ones and ignore
// online-ness."

check should only be on "min_capacity_channel_exists", online_channel_exists seems redundant

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the code matches the comment, and LLVM will trivially remove the redundant checks.

// If there are some online channels and some min_capacity channels, but no
// online-and-min_capacity channels, just include the min capacity ones and ignore
// online-ness.
channel.inbound_capacity_msat >= min_inbound_capacity
} else if min_capacity_channel_exists {
channel.inbound_capacity_msat >= min_inbound_capacity
} else if online_channel_exists {
channel.is_usable
} else { true }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there no limit to number of route hints we want to have?
lnd limits to 20.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should, but it depends a lot on the use-case. If the user is generating a QR code, 20 is way too many, if they're using it programatically in an API that may make sense. That seems like a separate concern from this PR, though, and we should do something about that in a followup. Can you open an issue and tag it, like, 0.1?

})
.map(|(_counterparty_id, channel)| route_hint_from_channel(&channel))
.map(|(_counterparty_id, channel)| route_hint_from_channel(channel))
.collect::<Vec<RouteHint>>()
}

Expand Down Expand Up @@ -723,6 +742,35 @@ 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(4);
let node_cfgs = create_node_cfgs(4, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
let nodes = create_network(4, &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());
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());

// With all peers connected we should get all hints that have sufficient value
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(1_000_000_000), &nodes[0], scid_aliases.clone());

// With only one sufficient-value peer connected we should only get its hint
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(1_000_000_000), &nodes[0], scid_aliases.clone());

// If we don't have any sufficient-value peers connected we should get all hints with
// sufficient value, even though there is a connected insufficient-value peer.
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(1_000_000_000), &nodes[0], scid_aliases);
}

#[test]
fn test_forwarding_info_not_assigned_channel_excluded_from_hints() {
let chanmon_cfgs = create_chanmon_cfgs(3);
Expand Down