Don't underpay htlc_min due to path contribution

We could have possibly constructed a slightly inconsistent
path: since we reduce value being transferred all the way, we
could have violated htlc_minimum_msat on some channels
we already passed (assuming dest->source direction). Here,
we recompute the fees again, so that if that's the case, we
match the currently underpaid htlc_minimum_msat with fees.

Author: naumenkogs

lightning/src/routing/router.rs

@@ -225,11 +225,7 @@ impl PaymentPath {
 	// to change fees may result in an inconsistency. Also, it's only safe to reduce the value,
 	// to not violate channel upper bounds.
 	fn update_value_and_recompute_fees(&mut self, value_msat: u64) {
-		if value_msat == self.hops.last().unwrap().route_hop.fee_msat {
-			// Nothing to change.
-			return;
-		}
-		assert!(value_msat < self.hops.last().unwrap().route_hop.fee_msat);
+		assert!(value_msat <= self.hops.last().unwrap().route_hop.fee_msat);
 
 		let mut total_fee_paid_msat = 0 as u64;
 		for i in (0..self.hops.len()).rev() {
@@ -251,6 +247,14 @@ impl PaymentPath {
 			// match htlc_minimum_msat logic.
 			let mut cur_hop_transferred_amount_msat = total_fee_paid_msat + value_msat;
 			if let Some(extra_fees_msat) = cur_hop.htlc_minimum_msat.checked_sub(cur_hop_transferred_amount_msat) {
+				// Note that there is a risk that *previous hops* (those closer to us, as we go
+				// payee->our_node here) would exceed their htlc_maximum_msat or available balance.
+				//
+				// This might make us end up with a broken route, although this should be super-rare
+				// in practice, both because of how healthy channels look like, and how we pick
+				// channels in add_entry.
+				// 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;
@@ -490,6 +494,10 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 					// as not sufficient.
 					// TODO: Explore simply adding fee to hit htlc_minimum_msat
 					if contributes_sufficient_value && amount_to_transfer_over_msat >= $directional_info.htlc_minimum_msat {
+						// Note that low contribution here (limited by available_liquidity_msat)
+						// might violate htlc_minimum_msat on the following hops. To avoid that,
+						// we recompute path fees knowing the final path contribution after
+						// constructing it.
 						let hm_entry = dist.entry(&$src_node_id);
 						let old_entry = hm_entry.or_insert_with(|| {
 							// If there was previously no known way to access
@@ -795,7 +803,15 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 				ordered_hops.last_mut().unwrap().hop_use_fee_msat = 0;
 				ordered_hops.last_mut().unwrap().route_hop.cltv_expiry_delta = final_cltv;
 
-				let payment_path = PaymentPath {hops: ordered_hops};
+				let mut payment_path = PaymentPath {hops: ordered_hops};
+
+				// We could have possibly constructed a slightly inconsistent path: since we reduce
+				// value being transferred all the way, we could have violated htlc_minimum_msat on
+				// some channels we already passed (assuming dest->source direction). Here, we
+				// recompute the fees again, so that if that's the case, we match the currently
+				// underpaid htlc_minimum_msat with fees.
+				payment_path.update_value_and_recompute_fees(value_contribution_msat);
+
 				// Since a path allows to transfer as much value as
 				// the smallest channel it has ("bottleneck"), we should recompute
 				// the fees so sender HTLC don't overpay fees when traversing