From: Valentine Wallace Date: Wed, 27 Sep 2023 03:44:27 +0000 (-0400) Subject: get_route: fix path value contribution to include min htlc overpay X-Git-Tag: v0.0.117-rc1~19^2 X-Git-Url: http://git.bitcoin.ninja/?a=commitdiff_plain;h=f3857d0f533d7a540e9ee321cbc11a393608827d;p=rust-lightning get_route: fix path value contribution to include min htlc overpay Previously, the fuzzer hit a debug panic because we wouldn't include the amount overpaid to meet a last hop's min_htlc in the total collected paths value. We now include this value and also penalize hops along the overpaying path to ensure that it gets deprioritized in path selection. --- diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 47b6bcd2b..09d83bb78 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1265,7 +1265,11 @@ impl<'a> PaymentPath<'a> { // Note that this function is not aware of the available_liquidity limit, and thus does not // support increasing the value being transferred beyond what was selected during the initial // routing passes. - fn update_value_and_recompute_fees(&mut self, value_msat: u64) { + // + // Returns the amount that this path contributes to the total payment value, which may be greater + // than `value_msat` if we had to overpay to meet the final node's `htlc_minimum_msat`. + fn update_value_and_recompute_fees(&mut self, value_msat: u64) -> u64 { + let mut extra_contribution_msat = 0; let mut total_fee_paid_msat = 0 as u64; for i in (0..self.hops.len()).rev() { let last_hop = i == self.hops.len() - 1; @@ -1280,6 +1284,7 @@ impl<'a> PaymentPath<'a> { let cur_hop = &mut self.hops.get_mut(i).unwrap().0; cur_hop.next_hops_fee_msat = total_fee_paid_msat; + cur_hop.path_penalty_msat += extra_contribution_msat; // Overpay in fees if we can't save these funds due to htlc_minimum_msat. // We try to account for htlc_minimum_msat in scoring (add_entry!), so that nodes don't // set it too high just to maliciously take more fees by exploiting this @@ -1295,8 +1300,15 @@ impl<'a> PaymentPath<'a> { // Also, this can't be exploited more heavily than *announce a free path and fail // all payments*. cur_hop_transferred_amount_msat += extra_fees_msat; - total_fee_paid_msat += extra_fees_msat; - cur_hop_fees_msat += extra_fees_msat; + + // We remember and return the extra fees on the final hop to allow accounting for + // them in the path's value contribution. + if last_hop { + extra_contribution_msat = extra_fees_msat; + } else { + total_fee_paid_msat += extra_fees_msat; + cur_hop_fees_msat += extra_fees_msat; + } } if last_hop { @@ -1324,6 +1336,7 @@ impl<'a> PaymentPath<'a> { } } } + value_msat + extra_contribution_msat } } @@ -2330,8 +2343,8 @@ where L::Target: Logger { // recompute the fees again, so that if that's the case, we match the currently // underpaid htlc_minimum_msat with fees. debug_assert_eq!(payment_path.get_value_msat(), value_contribution_msat); - value_contribution_msat = cmp::min(value_contribution_msat, final_value_msat); - payment_path.update_value_and_recompute_fees(value_contribution_msat); + let desired_value_contribution = cmp::min(value_contribution_msat, final_value_msat); + value_contribution_msat = payment_path.update_value_and_recompute_fees(desired_value_contribution); // Since a path allows to transfer as much value as // the smallest channel it has ("bottleneck"), we should recompute @@ -7521,6 +7534,67 @@ mod tests { assert_eq!(err, "Failed to find a path to the given destination"); } else { panic!() } } + + #[test] + fn path_contribution_includes_min_htlc_overpay() { + // Previously, the fuzzer hit a debug panic because we wouldn't include the amount overpaid to + // meet a last hop's min_htlc in the total collected paths value. We now include this value and + // also penalize hops along the overpaying path to ensure that it gets deprioritized in path + // selection, both tested here. + let secp_ctx = Secp256k1::new(); + let logger = Arc::new(ln_test_utils::TestLogger::new()); + let network_graph = Arc::new(NetworkGraph::new(Network::Testnet, Arc::clone(&logger))); + let scorer = ProbabilisticScorer::new(ProbabilisticScoringDecayParameters::default(), network_graph.clone(), logger.clone()); + let keys_manager = ln_test_utils::TestKeysInterface::new(&[0u8; 32], Network::Testnet); + let random_seed_bytes = keys_manager.get_secure_random_bytes(); + let config = UserConfig::default(); + + // Values are taken from the fuzz input that uncovered this panic. + let amt_msat = 562_0000; + let (_, our_id, _, nodes) = get_nodes(&secp_ctx); + let first_hops = vec![ + get_channel_details( + Some(83), nodes[0], channelmanager::provided_init_features(&config), 2199_0000, + ), + ]; + + let htlc_mins = [49_0000, 1125_0000]; + let payment_params = { + let blinded_path = BlindedPath { + introduction_node_id: nodes[0], + blinding_point: ln_test_utils::pubkey(42), + blinded_hops: vec![ + BlindedHop { blinded_node_id: ln_test_utils::pubkey(42 as u8), encrypted_payload: Vec::new() }, + BlindedHop { blinded_node_id: ln_test_utils::pubkey(42 as u8), encrypted_payload: Vec::new() } + ], + }; + let mut blinded_hints = Vec::new(); + for htlc_min in htlc_mins.iter() { + blinded_hints.push((BlindedPayInfo { + fee_base_msat: 0, + fee_proportional_millionths: 0, + htlc_minimum_msat: *htlc_min, + htlc_maximum_msat: *htlc_min * 100, + cltv_expiry_delta: 10, + features: BlindedHopFeatures::empty(), + }, blinded_path.clone())); + } + let bolt12_features: Bolt12InvoiceFeatures = channelmanager::provided_invoice_features(&config).to_context(); + PaymentParameters::blinded(blinded_hints.clone()) + .with_bolt12_features(bolt12_features.clone()).unwrap() + }; + + let netgraph = network_graph.read_only(); + let route_params = RouteParameters::from_payment_params_and_value( + payment_params, amt_msat); + let route = get_route( + &our_id, &route_params, &netgraph, Some(&first_hops.iter().collect::>()), + Arc::clone(&logger), &scorer, &ProbabilisticScoringFeeParameters::default(), + &random_seed_bytes + ).unwrap(); + assert_eq!(route.paths.len(), 1); + assert_eq!(route.get_total_amount(), amt_msat); + } } #[cfg(all(any(test, ldk_bench), not(feature = "no-std")))]