Skip to content

Fix panic in router given to bogus last-hop hints #958

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
Jul 5, 2021
Merged

Fix panic in router given to bogus last-hop hints #958

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
90 changes: 59 additions & 31 deletions lightning/src/routing/router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,8 +513,11 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
// $directional_info.
// $next_hops_fee_msat represents the fees paid for using all the channel *after* this one,
// since that value has to be transferred over this channel.
// Returns whether this channel caused an update to `targets`.
( $chan_id: expr, $src_node_id: expr, $dest_node_id: expr, $directional_info: expr, $capacity_sats: expr, $chan_features: expr, $next_hops_fee_msat: expr,
$next_hops_value_contribution: expr, $next_hops_path_htlc_minimum_msat: expr ) => {
$next_hops_value_contribution: expr, $next_hops_path_htlc_minimum_msat: expr ) => { {
// We "return" whether we updated the path at the end, via this:
let mut did_add_update_path_to_src_node = false;
Copy link
Contributor

@valentinewallace valentinewallace Jul 4, 2021

Choose a reason for hiding this comment

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

Does this mean did_add_or_update_path_to_src_node? (i.e. the "or" is implied?)

Copy link
Collaborator Author

@TheBlueMatt TheBlueMatt Jul 4, 2021

Choose a reason for hiding this comment

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

I figure its implied - the block can only either add or update, but even if it did both the meaning would still hold.

// Channels to self should not be used. This is more of belt-and-suspenders, because in
// practice these cases should be caught earlier:
// - for regular channels at channel announcement (TODO)
Expand Down Expand Up @@ -726,6 +729,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
{
old_entry.value_contribution_msat = value_contribution_msat;
}
did_add_update_path_to_src_node = true;
} else if old_entry.was_processed && new_cost < old_cost {
#[cfg(any(test, feature = "fuzztarget"))]
{
Expand Down Expand Up @@ -756,7 +760,8 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
}
}
}
};
did_add_update_path_to_src_node
} }
}

let empty_node_features = NodeFeatures::empty();
Expand Down Expand Up @@ -859,22 +864,10 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
// it matters only if the fees are exactly the same.
for hop in last_hops.iter() {
let have_hop_src_in_graph =
if let Some(&(ref first_hop, ref features, ref outbound_capacity_msat, _)) = first_hop_targets.get(&hop.src_node_id) {
// If this hop connects to a node with which we have a direct channel, ignore
// the network graph and add both the hop and our direct channel to
// the candidate set.
//
// Currently there are no channel-context features defined, so we are a
// bit lazy here. In the future, we should pull them out via our
// ChannelManager, but there's no reason to waste the space until we
// need them.
add_entry!(first_hop, *our_node_id , hop.src_node_id, dummy_directional_info, Some(outbound_capacity_msat / 1000), features, 0, path_value_msat, 0);
true
} else {
// In any other case, only add the hop if the source is in the regular network
// graph:
network.get_nodes().get(&hop.src_node_id).is_some()
};
// Only add the last hop to our candidate set if either we have a direct channel or
// they are in the regular network graph.
first_hop_targets.get(&hop.src_node_id).is_some() ||
network.get_nodes().get(&hop.src_node_id).is_some();
if have_hop_src_in_graph {
// BOLT 11 doesn't allow inclusion of features for the last hop hints, which
// really sucks, cause we're gonna need that eventually.
Expand All @@ -888,7 +881,18 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
htlc_maximum_msat: hop.htlc_maximum_msat,
fees: hop.fees,
};
add_entry!(hop.short_channel_id, hop.src_node_id, payee, directional_info, None::<u64>, &empty_channel_features, 0, path_value_msat, 0);
if add_entry!(hop.short_channel_id, hop.src_node_id, payee, directional_info, None::<u64>, &empty_channel_features, 0, path_value_msat, 0) {
// If this hop connects to a node with which we have a direct channel,
// ignore the network graph and, if the last hop was added, add our
// direct channel to the candidate set.
//
// Note that we *must* check if the last hop was added as `add_entry`
// always assumes that the third argument is a node to which we have a
// path.
if let Some(&(ref first_hop, ref features, ref outbound_capacity_msat, _)) = first_hop_targets.get(&hop.src_node_id) {
add_entry!(first_hop, *our_node_id , hop.src_node_id, dummy_directional_info, Some(outbound_capacity_msat / 1000), features, 0, path_value_msat, 0);
}
}
}
}

Expand Down Expand Up @@ -1159,7 +1163,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye

#[cfg(test)]
mod tests {
use routing::router::{get_route, RouteHint, RouteHintHop, RoutingFees};
use routing::router::{get_route, Route, RouteHint, RouteHintHop, RoutingFees};
use routing::network_graph::{NetworkGraph, NetGraphMsgHandler};
use chain::transaction::OutPoint;
use ln::features::{ChannelFeatures, InitFeatures, InvoiceFeatures, NodeFeatures};
Expand Down Expand Up @@ -2307,11 +2311,7 @@ mod tests {
assert_eq!(route.paths[0][4].channel_features.le_flags(), &Vec::<u8>::new()); // We can't learn any flags from invoices, sadly
}

#[test]
fn unannounced_path_test() {
// We should be able to send a payment to a destination without any help of a routing graph
// if we have a channel with a common counterparty that appears in the first and last hop
// hints.
fn do_unannounced_path_test(last_hop_htlc_max: Option<u64>, last_hop_fee_prop: u32, outbound_capacity_msat: u64, route_val: u64) -> Result<Route, LightningError> {
let source_node_id = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&hex::decode(format!("{:02}", 41).repeat(32)).unwrap()[..]).unwrap());
let middle_node_id = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&hex::decode(format!("{:02}", 42).repeat(32)).unwrap()[..]).unwrap());
let target_node_id = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&hex::decode(format!("{:02}", 43).repeat(32)).unwrap()[..]).unwrap());
Expand All @@ -2322,11 +2322,11 @@ mod tests {
short_channel_id: 8,
fees: RoutingFees {
base_msat: 1000,
proportional_millionths: 0,
proportional_millionths: last_hop_fee_prop,
},
cltv_expiry_delta: (8 << 8) | 1,
htlc_minimum_msat: None,
htlc_maximum_msat: None,
htlc_maximum_msat: last_hop_htlc_max,
}]);
let our_chans = vec![channelmanager::ChannelDetails {
channel_id: [0; 32],
Expand All @@ -2336,31 +2336,59 @@ mod tests {
counterparty_features: InitFeatures::from_le_bytes(vec![0b11]),
channel_value_satoshis: 100000,
user_id: 0,
outbound_capacity_msat: 100000,
outbound_capacity_msat: outbound_capacity_msat,
inbound_capacity_msat: 100000,
is_outbound: true, is_funding_locked: true,
is_usable: true, is_public: true,
counterparty_forwarding_info: None,
}];
let route = get_route(&source_node_id, &NetworkGraph::new(genesis_block(Network::Testnet).header.block_hash()), &target_node_id, None, Some(&our_chans.iter().collect::<Vec<_>>()), &vec![&last_hops], 100, 42, Arc::new(test_utils::TestLogger::new())).unwrap();
get_route(&source_node_id, &NetworkGraph::new(genesis_block(Network::Testnet).header.block_hash()), &target_node_id, None, Some(&our_chans.iter().collect::<Vec<_>>()), &vec![&last_hops], route_val, 42, Arc::new(test_utils::TestLogger::new()))
}

#[test]
fn unannounced_path_test() {
// We should be able to send a payment to a destination without any help of a routing graph
// if we have a channel with a common counterparty that appears in the first and last hop
// hints.
let route = do_unannounced_path_test(None, 1, 2000000, 1000000).unwrap();
Copy link
Contributor

@jkczyz jkczyz Jun 30, 2021

Choose a reason for hiding this comment

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

Did these values need to be updated from what was used in the original test to exercise the same behavior?

Copy link
Collaborator Author

@TheBlueMatt TheBlueMatt Jul 1, 2021

Choose a reason for hiding this comment

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

I don't see why they would?


let middle_node_id = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&hex::decode(format!("{:02}", 42).repeat(32)).unwrap()[..]).unwrap());
let target_node_id = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&hex::decode(format!("{:02}", 43).repeat(32)).unwrap()[..]).unwrap());
assert_eq!(route.paths[0].len(), 2);

assert_eq!(route.paths[0][0].pubkey, middle_node_id);
assert_eq!(route.paths[0][0].short_channel_id, 42);
assert_eq!(route.paths[0][0].fee_msat, 1000);
assert_eq!(route.paths[0][0].fee_msat, 1001);
assert_eq!(route.paths[0][0].cltv_expiry_delta, (8 << 8) | 1);
assert_eq!(route.paths[0][0].node_features.le_flags(), &[0b11]);
assert_eq!(route.paths[0][0].channel_features.le_flags(), &[0; 0]); // We can't learn any flags from invoices, sadly

assert_eq!(route.paths[0][1].pubkey, target_node_id);
assert_eq!(route.paths[0][1].short_channel_id, 8);
assert_eq!(route.paths[0][1].fee_msat, 100);
assert_eq!(route.paths[0][1].fee_msat, 1000000);
assert_eq!(route.paths[0][1].cltv_expiry_delta, 42);
assert_eq!(route.paths[0][1].node_features.le_flags(), &[0; 0]); // We dont pass flags in from invoices yet
assert_eq!(route.paths[0][1].channel_features.le_flags(), &[0; 0]); // We can't learn any flags from invoices, sadly
}

#[test]
fn overflow_unannounced_path_test_liquidity_underflow() {
// Previously, when we had a last-hop hint connected directly to a first-hop channel, where
// the last-hop had a fee which overflowed a u64, we'd panic.
// This was due to us adding the first-hop from us unconditionally, causing us to think
// we'd built a path (as our node is in the "best candidate" set), when we had not.
// In this test, we previously hit a subtraction underflow due to having less available
// liquidity at the last hop than 0.
assert!(do_unannounced_path_test(Some(21_000_000_0000_0000_000), 0, 21_000_000_0000_0000_000, 21_000_000_0000_0000_000).is_err());
}

#[test]
fn overflow_unannounced_path_test_feerate_overflow() {
// This tests for the same case as above, except instead of hitting a subtraction
// underflow, we hit a case where the fee charged at a hop overflowed.
assert!(do_unannounced_path_test(Some(21_000_000_0000_0000_000), 50000, 21_000_000_0000_0000_000, 21_000_000_0000_0000_000).is_err());
}

#[test]
fn available_amount_while_routing_test() {
// Tests whether we choose the correct available channel amount while routing.
Expand Down