Hit htlc_min in add entry + fix

Author: naumenkogs

lightning/src/routing/router.rs

@@ -509,21 +509,17 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 
 					let value_contribution_msat = cmp::min(available_value_contribution_msat, $next_hops_value_contribution);
 					// Includes paying fees for the use of the following channels.
-					let amount_to_transfer_over_msat: u64 = match value_contribution_msat.checked_add($next_hops_fee_msat) {
+					let mut amount_to_transfer_over_msat: u64 = match value_contribution_msat.checked_add($next_hops_fee_msat) {
 						Some(result) => result,
 						// Can't overflow due to how the values were computed right above.
 						None => unreachable!(),
 					};
 
-					// If HTLC minimum is larger than the amount we're going to transfer, we shouldn't
-					// bother considering this channel.
-					// Since we're choosing amount_to_transfer_over_msat as maximum possible, it can
-					// be only reduced later (not increased), so this channel should just be skipped
-					// as not sufficient.
-					// TODO: Explore simply adding fee to hit htlc_minimum_msat
+					// If HTLC minimum is larger than the amount we're going to transfer, we shouldn
+					// transfer that value instead, thus overpaying fees.
 					let effective_htlc_minimum_msat = cmp::max($directional_info.htlc_minimum_msat, $incl_fee_next_hops_htlc_minimum_msat);
-					let over_path_minimum_msat = amount_to_transfer_over_msat >= effective_htlc_minimum_msat;
-					if contributes_sufficient_value && over_path_minimum_msat {
+					amount_to_transfer_over_msat = cmp::max(amount_to_transfer_over_msat, effective_htlc_minimum_msat);
+					if contributes_sufficient_value {
 						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
@@ -602,34 +598,12 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 							},
 						};
 
-						// Update the way of reaching $src_node_id with the given $chan_id (from $dest_node_id),
-						// if this way is cheaper than the already known
+						// Update the way of reaching $src_node_id with the given $chan_id (from
+						// $dest_node_id), if this way is cheaper than the already known
 						// (considering the cost to "reach" this channel from the route destination,
-						// the cost of using this channel,
+						// the cost of using this channel (with possible overpayment for htlc min)
 						// and the cost of routing to the source node of this channel).
-						// Also, consider that htlc_minimum_msat_difference, because we might end up
-						// paying it. Consider the following exploit:
-						// we use 2 paths to transfer 1.5 BTC. One of them is 0-fee normal 1 BTC path,
-						// and for the other one we picked a 1sat-fee path with htlc_minimum_msat of
-						// 1 BTC. Now, since the latter is more expensive, we gonna try to cut it
-						// by 0.5 BTC, but then match htlc_minimum_msat by paying a fee of 0.5 BTC
-						// to this channel.
-						// TODO: this scoring could be smarter (e.g. 0.5*htlc_minimum_msat here).
-						let mut old_cost = old_entry.total_fee_msat;
-						if let Some(increased_old_cost) = old_cost.checked_add(old_entry.htlc_minimum_msat) {
-							old_cost = increased_old_cost;
-						} else {
-							old_cost = u64::max_value();
-						}
-
-						let mut new_cost = total_fee_msat;
-						if let Some(increased_new_cost) = new_cost.checked_add($directional_info.htlc_minimum_msat) {
-							new_cost = increased_new_cost;
-						} else {
-							new_cost = u64::max_value();
-						}
-
-						if new_cost < old_cost {
+						if total_fee_msat < old_entry.total_fee_msat {
 							targets.push(new_graph_node);
 							old_entry.next_hops_fee_msat = $next_hops_fee_msat;
 							old_entry.hop_use_fee_msat = hop_use_fee_msat;
@@ -820,35 +794,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 					}
 
 					new_entry = match dist.remove(&ordered_hops.last().unwrap().route_hop.pubkey) {
-						Some(payment_hop) => {
-							if new_entry.hop_use_fee_msat + value_contribution_msat <
-								ordered_hops.last_mut().unwrap().effective_htlc_minimum_msat {
-								// This could happen if a mismatch happened in add_entry! when
-								// there is a path fork: e.g. source-A-B-payee and source-A-C-payee.
-								// The mismatch of htlc_min could happen if B is the cheapest path,
-								// but while adding A-for-B, A can't hit htlc_minimum_msat.
-								// Then, we add A-for-C because it does hit htlc_minimum msat.
-								//
-								// This block would trigger if after this we choose A-for-C
-								// as a way to reach B, and we break from the loop because this
-								// path would be invalid otherwise.
-								//
-								// The actual problem would happen when we propagate fees one hop
-								// backward (see below), the A-for-C is not sufficient to hit
-								// htlc_minimum_msat anymore.
-								//
-								// This check here attempts to check the compatibility:
-								// (fine if A-for-C just matches following C or overpays for D).
-								//
-								// TODO. this is a safe approach. It indeed avoids invalid paths,
-								// but it also could drop valid ones. Ideally, we should prevent
-								// the latter too. Although dropping is rare, because in the example
-								// above, C would be dropped from candidates, and then we just have
-								// to implement hitting htlc_minimum_msat for B in add_entry.
-								break 'paths_collection;
-							}
-							payment_hop
-						}
+						Some(payment_hop) => payment_hop,
 						None => {
 							// If we can't reach a given node, something went wrong
 							// during path traverse.
@@ -1578,7 +1524,6 @@ mod tests {
 		});
 
 		// Check against amount_to_transfer_over_msat.
-		// We will be transferring 300_000_000 msat.
 		// Set minimal HTLC of 200_000_000 msat.
 		update_channel(&net_graph_msg_handler, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
 			chain_hash: genesis_block(Network::Testnet).header.block_hash(),
@@ -1608,10 +1553,9 @@ mod tests {
 			excess_data: Vec::new()
 		});
 
-		// Not possible to send 299_999_999: we first allocate 200_000_000 for the channel#12,
-		// and then the amount_to_transfer_over_msat over channel#2 is less than our limit.
-		if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(&our_id, &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2], None, &Vec::new(), 199_999_999, 42, Arc::clone(&logger)) {
-			assert_eq!(err, "Failed to find a path to the given destination");
+		// Not possible to send 299_999_999.
+		if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(&our_id, &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2], None, &Vec::new(), 299_999_999, 42, Arc::clone(&logger)) {
+			assert_eq!(err, "Failed to find a sufficient route to the given destination");
 		} else { panic!(); }
 
 		// Lift the restriction on the first hop.