Fix MPP routefinding when we first collect 95% of payment value

See comment in new test for more details.

Author: TheBlueMatt

lightning/src/routing/router.rs

@@ -1113,10 +1113,13 @@ where L::Target: Logger {
 						fees: hop.fees,
 					};
 
-					let reqd_channel_cap = if let Some (val) = final_value_msat.checked_add(match idx {
-						0 => 999,
-						_ => aggregate_next_hops_fee_msat.checked_add(999).unwrap_or(u64::max_value())
-					}) { Some( val / 1000 ) } else { break; }; // converting from msat or breaking if max ~ infinity
+					let reqd_channel_cap = if let Some (val) = final_value_msat
+						.checked_mul(ROUTE_CAPACITY_PROVISION_FACTOR).unwrap_or(u64::max_value())
+						.checked_add(match idx {
+							0 => 999,
+							_ => aggregate_next_hops_fee_msat.checked_add(999).unwrap_or(u64::max_value())
+						})
+					{ Some( val / 1000 ) } else { break; }; // converting from msat or breaking if max ~ infinity
 
 					let src_node_id = NodeId::from_pubkey(&hop.src_node_id);
 					let dest_node_id = NodeId::from_pubkey(&prev_hop_id);
@@ -4055,7 +4058,98 @@ mod tests {
 			assert_eq!(total_amount_paid_msat, 200_000);
 			assert_eq!(route.get_total_fees(), 150_000);
 		}
+	}
 
+	#[test]
+	fn mpp_with_last_hops() {
+		// Previously, if we tried to send an MPP payment to a destination which was only reachable
+		// via a single last-hop route hint, we'd fail to route if we first collected routes
+		// totaling close, but not quite enough to fund the full payment.
+		//
+		// This was because we considered last-hop hints to have exactly the sought payment amount
+		// instead of the amount we were trying to collect, needlessly limiting our path searching
+		// at the very first hop.
+		//
+		// Specifically, this interacted with our "all paths must fund at least 5% of total target"
+		// criteria to cause us to refuse all routes at the last hop hint which would be considered
+		// to only have the remaining to-collect amount in available liquidity.
+		//
+		// This bug appeared in production in some specific channel configurations.
+		let (secp_ctx, network_graph, net_graph_msg_handler, _, logger) = build_graph();
+		let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx);
+		let scorer = test_utils::TestScorer::with_fixed_penalty(0);
+		let payee = Payee::from_node_id(PublicKey::from_slice(&[02; 33]).unwrap()).with_features(InvoiceFeatures::known())
+			.with_route_hints(vec![RouteHint(vec![RouteHintHop {
+				src_node_id: nodes[2],
+				short_channel_id: 42,
+				fees: RoutingFees { base_msat: 0, proportional_millionths: 0 },
+				cltv_expiry_delta: 42,
+				htlc_minimum_msat: None,
+				htlc_maximum_msat: None,
+			}])]);
+
+		// Keep only two paths from us to nodes[2], one with no fee and a 90sat HTLC maximum, one
+		// with a slight fee and a 20sat *minimum*. Previously, trying to route 100 sats to
+		// nodes[2] here would first use the 90sat no-fee route and then fail to find a path along
+		// the second route as we think we can only send up to 10 sats over the last-hop but have a
+		// minimum of 20 sats.
+		update_channel(&net_graph_msg_handler, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
+			chain_hash: genesis_block(Network::Testnet).header.block_hash(),
+			short_channel_id: 1,
+			timestamp: 2,
+			flags: 0,
+			cltv_expiry_delta: u16::max_value(),
+			htlc_minimum_msat: 0,
+			htlc_maximum_msat: OptionalField::Present(99_000),
+			fee_base_msat: u32::max_value(),
+			fee_proportional_millionths: u32::max_value(),
+			excess_data: Vec::new()
+		});
+		update_channel(&net_graph_msg_handler, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
+			chain_hash: genesis_block(Network::Testnet).header.block_hash(),
+			short_channel_id: 2,
+			timestamp: 2,
+			flags: 0,
+			cltv_expiry_delta: u16::max_value(),
+			htlc_minimum_msat: 0,
+			htlc_maximum_msat: OptionalField::Present(60_000),
+			fee_base_msat: u32::max_value(),
+			fee_proportional_millionths: u32::max_value(),
+			excess_data: Vec::new()
+		});
+		update_channel(&net_graph_msg_handler, &secp_ctx, &privkeys[1], UnsignedChannelUpdate {
+			chain_hash: genesis_block(Network::Testnet).header.block_hash(),
+			short_channel_id: 4,
+			timestamp: 2,
+			flags: 0,
+			cltv_expiry_delta: (4 << 8) | 1,
+			htlc_minimum_msat: 0,
+			htlc_maximum_msat: OptionalField::Absent,
+			fee_base_msat: 1,
+			fee_proportional_millionths: 0,
+			excess_data: Vec::new()
+		});
+		update_channel(&net_graph_msg_handler, &secp_ctx, &privkeys[7], UnsignedChannelUpdate {
+			chain_hash: genesis_block(Network::Testnet).header.block_hash(),
+			short_channel_id: 13,
+			timestamp: 2,
+			flags: 0|2, // Channel disabled
+			cltv_expiry_delta: (13 << 8) | 1,
+			htlc_minimum_msat: 0,
+			htlc_maximum_msat: OptionalField::Absent,
+			fee_base_msat: 0,
+			fee_proportional_millionths: 2000000,
+			excess_data: Vec::new()
+		});
+
+		// Get a route for 100 sats and check that we found the MPP route no problem and didn't
+		// overpay at all.
+		let route = get_route(&our_id, &payee, &network_graph, None, 100_000, 42, Arc::clone(&logger), &scorer).unwrap();
+		assert_eq!(route.paths.len(), 2);
+		assert_eq!(route.paths[0][0].short_channel_id, 2);
+		assert_eq!(route.paths[1][0].short_channel_id, 1);
+		assert_eq!(route.get_total_fees(), 1);
+		assert_eq!(route.get_total_amount(), 100_000);
 	}
 
 	#[test]