Fix panic in router given to bogus last-hop hints 2021-06-fix-router-panic
authorMatt Corallo <git@bluematt.me>
Fri, 18 Jun 2021 04:31:50 +0000 (04:31 +0000)
committerMatt Corallo <git@bluematt.me>
Sun, 4 Jul 2021 23:26:21 +0000 (23:26 +0000)
See new comments and test cases for more info

lightning/src/routing/router.rs

index 9f4b8ac3589a27ef7c713751eb1d98ad18596978..1d8bf268da17a4c9351132b7ffc697b675574afd 100644 (file)
@@ -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;
                        // 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<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"))]
                                                                {
@@ -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();
@@ -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.
@@ -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);
+                                       }
+                               }
                        }
                }
 
@@ -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};
@@ -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());
@@ -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<_>>()), &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();
+
+               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.