From 2825d65ca98424789f30cfface0299c3ba53216b Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 18 Jun 2021 04:31:50 +0000 Subject: [PATCH] Fix panic in router given to bogus last-hop hints See new comments and test cases for more info --- lightning/src/routing/router.rs | 90 +++++++++++++++++++++------------ 1 file changed, 59 insertions(+), 31 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 9f4b8ac35..1d8bf268d 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -513,8 +513,11 @@ pub fn get_route(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; // 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) @@ -726,6 +729,7 @@ pub fn get_route(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"))] { @@ -756,7 +760,8 @@ pub fn get_route(our_node_id: &PublicKey, network: &NetworkGraph, paye } } } - }; + did_add_update_path_to_src_node + } } } let empty_node_features = NodeFeatures::empty(); @@ -859,22 +864,10 @@ pub fn get_route(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. @@ -888,7 +881,18 @@ pub fn get_route(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::, &empty_channel_features, 0, path_value_msat, 0); + if add_entry!(hop.short_channel_id, hop.src_node_id, payee, directional_info, None::, &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); + } + } } } @@ -1159,7 +1163,7 @@ pub fn get_route(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}; @@ -2307,11 +2311,7 @@ mod tests { assert_eq!(route.paths[0][4].channel_features.le_flags(), &Vec::::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, last_hop_fee_prop: u32, outbound_capacity_msat: u64, route_val: u64) -> Result { 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()); @@ -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], @@ -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![&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![&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(); + + 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. -- 2.39.5