From 7ebdf45b3803f8fc4f72d02d3440bf0f795681dd Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 28 Apr 2025 23:43:37 +0000 Subject: [PATCH] Use `floor` when computing max value of a path, not `ceil` When calculating the maximum contribution of a path to a larger route, we want to ensure we don't overshoot as that might cause us to violate a maximum value limit. In 209cb2aa2e0d67bf89a130b070f7116178e9ddb4, we started by calculating with `ceil`, which can trigger exactly that, so here we drop the extra addition, switching us to `floor`. Found both by the `router` fuzzer as well as the `generate_large_mpp_routes` test. --- lightning/src/routing/router.rs | 96 ++++++++++++++++++++++++++++++++- 1 file changed, 94 insertions(+), 2 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index bc4210b5c59..e62af007057 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -2145,11 +2145,10 @@ impl<'a> PaymentPath<'a> { let (next_hops_aggregated_base, next_hops_aggregated_prop) = crate::blinded_path::payment::compute_aggregated_base_prop_fee(next_hops_feerates_iter).unwrap(); - // ceil(((hop_max_msat - agg_base) * 1_000_000) / (1_000_000 + agg_prop)) + // floor(((hop_max_msat - agg_base) * 1_000_000) / (1_000_000 + agg_prop)) let hop_max_final_value_contribution = (hop_max_msat as u128) .checked_sub(next_hops_aggregated_base as u128) .and_then(|f| f.checked_mul(1_000_000)) - .and_then(|f| f.checked_add(1_000_000 - 1)) .and_then(|f| f.checked_add(next_hops_aggregated_prop as u128)) .map(|f| f / ((next_hops_aggregated_prop as u128).saturating_add(1_000_000))); @@ -8662,6 +8661,99 @@ mod tests { assert_eq!(route.get_total_fees(), 123); } + #[test] + fn test_max_final_contribution() { + // When `compute_max_final_value_contribution` was added, it had a bug where it would + // over-estimate the maximum value contribution of a hop by using `ceil` rather than + // `floor`. This tests that case by attempting to send 1 million sats over a channel where + // the remaining hops have a base fee of zero and a proportional fee of 1 millionth. + + let (secp_ctx, network_graph, gossip_sync, _, logger) = build_graph(); + let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); + let scorer = ln_test_utils::TestScorer::new(); + let random_seed_bytes = [42; 32]; + + // Enable channel 1, setting max HTLC to 1M sats + update_channel(&gossip_sync, &secp_ctx, &our_privkey, UnsignedChannelUpdate { + chain_hash: ChainHash::using_genesis_block(Network::Testnet), + short_channel_id: 1, + timestamp: 2, + message_flags: 1, // Only must_be_one + channel_flags: 0, + cltv_expiry_delta: (1 << 4) | 1, + htlc_minimum_msat: 0, + htlc_maximum_msat: 1_000_000, + fee_base_msat: 0, + fee_proportional_millionths: 0, + excess_data: Vec::new() + }); + + // Set the fee on channel 3 to zero + update_channel(&gossip_sync, &secp_ctx, &privkeys[0], UnsignedChannelUpdate { + chain_hash: ChainHash::using_genesis_block(Network::Testnet), + short_channel_id: 3, + timestamp: 2, + message_flags: 1, // Only must_be_one + channel_flags: 0, + cltv_expiry_delta: (3 << 4) | 1, + htlc_minimum_msat: 0, + htlc_maximum_msat: 1_000_000_000, + fee_base_msat: 0, + fee_proportional_millionths: 0, + excess_data: Vec::new() + }); + + // Set the fee on channel 6 to 1 millionth + update_channel(&gossip_sync, &secp_ctx, &privkeys[2], UnsignedChannelUpdate { + chain_hash: ChainHash::using_genesis_block(Network::Testnet), + short_channel_id: 6, + timestamp: 2, + message_flags: 1, // Only must_be_one + channel_flags: 0, + cltv_expiry_delta: (6 << 4) | 1, + htlc_minimum_msat: 0, + htlc_maximum_msat: 1_000_000_000, + fee_base_msat: 0, + fee_proportional_millionths: 1, + excess_data: Vec::new() + }); + + // Now attempt to pay over the channel 1 -> channel 3 -> channel 6 path + // This should fail as we need to send 1M + 1 sats to cover the fee but channel 1 only + // allows for 1M sats to flow over it. + let config = UserConfig::default(); + let payment_params = PaymentParameters::from_node_id(nodes[4], 42) + .with_bolt11_features(channelmanager::provided_bolt11_invoice_features(&config)) + .unwrap(); + let route_params = RouteParameters::from_payment_params_and_value(payment_params, 1_000_000); + get_route(&our_id, &route_params, &network_graph.read_only(), None, + Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap_err(); + + // Now set channel 1 max HTLC to 1M + 1 sats + update_channel(&gossip_sync, &secp_ctx, &our_privkey, UnsignedChannelUpdate { + chain_hash: ChainHash::using_genesis_block(Network::Testnet), + short_channel_id: 1, + timestamp: 3, + message_flags: 1, // Only must_be_one + channel_flags: 0, + cltv_expiry_delta: (1 << 4) | 1, + htlc_minimum_msat: 0, + htlc_maximum_msat: 1_000_001, + fee_base_msat: 0, + fee_proportional_millionths: 0, + excess_data: Vec::new() + }); + + // And attempt the same payment again, but this time it should work. + let route = get_route(&our_id, &route_params, &network_graph.read_only(), None, + Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap(); + assert_eq!(route.paths.len(), 1); + assert_eq!(route.paths[0].hops.len(), 3); + assert_eq!(route.paths[0].hops[0].short_channel_id, 1); + assert_eq!(route.paths[0].hops[1].short_channel_id, 3); + assert_eq!(route.paths[0].hops[2].short_channel_id, 6); + } + #[test] fn allow_us_being_first_hint() { // Check that we consider a route hint even if we are the src of the first hop.