askrene: handle maxparts better.

channeld/full_channel.c

@@ -416,7 +416,7 @@ static bool get_room_above_reserve(const struct channel *channel,
 		return false;
 	}
 
-	if (!amount_msat_sub_sat(remainder, *remainder, reserve)) {
+	if (!amount_msat_deduct_sat(remainder, reserve)) {
 		status_debug("%s cannot afford htlc: would make balance %s"
 			     " below reserve %s",
 			     side_to_str(side),
@@ -504,7 +504,7 @@ static bool htlc_dust(const struct channel *channel,
 				      side, &trim_rmvd))
 		return false;
 
-	return amount_msat_sub(trim_total, *trim_total, trim_rmvd);
+	return amount_msat_deduct(trim_total, trim_rmvd);
 }
 
 /*
@@ -773,7 +773,7 @@ static enum channel_add_err add_htlc(struct channel *channel,
 		 */
 		if ((option_anchor_outputs || option_anchors_zero_fee_htlc_tx)
 		    && channel->opener == sender
-		    && !amount_msat_sub_sat(&remainder, remainder, AMOUNT_SAT(660)))
+		    && !amount_msat_deduct_sat(&remainder, AMOUNT_SAT(660)))
 			return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED;
 
 		if (channel->opener== sender) {
@@ -805,7 +805,7 @@ static enum channel_add_err add_htlc(struct channel *channel,
 
 			if ((option_anchor_outputs || option_anchors_zero_fee_htlc_tx)
 			    && channel->opener != sender
-			    && !amount_msat_sub_sat(&remainder, remainder, AMOUNT_SAT(660)))
+			    && !amount_msat_deduct_sat(&remainder, AMOUNT_SAT(660)))
 				return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED;
 
 			/* Should be able to afford both their own commit tx
@@ -1257,12 +1257,12 @@ u32 approx_max_feerate(const struct channel *channel)
 	 * `to_remote`).
 	 */
 	if ((option_anchor_outputs || option_anchors_zero_fee_htlc_tx)
-	    && !amount_msat_sub_sat(&avail, avail, AMOUNT_SAT(660))) {
+	    && !amount_msat_deduct_sat(&avail, AMOUNT_SAT(660))) {
 		avail = AMOUNT_MSAT(0);
 	} else {
 		/* We should never go below reserve. */
-		if (!amount_msat_sub_sat(&avail, avail,
-					 channel->config[!channel->opener].channel_reserve))
+		if (!amount_msat_deduct_sat(&avail,
+					    channel->config[!channel->opener].channel_reserve))
 		avail = AMOUNT_MSAT(0);
 	}
 

common/amount.c

@@ -263,6 +263,18 @@ WARN_UNUSED_RESULT bool amount_msat_accumulate(struct amount_msat *a,
 	return amount_msat_add(a, *a, b);
 }
 
+WARN_UNUSED_RESULT bool amount_msat_deduct(struct amount_msat *a,
+					       struct amount_msat b)
+{
+	return amount_msat_sub(a, *a, b);
+}
+
+WARN_UNUSED_RESULT bool amount_msat_deduct_sat(struct amount_msat *a,
+					       struct amount_sat b)
+{
+	return amount_msat_sub_sat(a, *a, b);
+}
+
 WARN_UNUSED_RESULT bool amount_msat_sub(struct amount_msat *val,
 					struct amount_msat a,
 					struct amount_msat b)

common/amount.h

@@ -105,6 +105,14 @@ WARN_UNUSED_RESULT bool amount_sat_add_sat_s64(struct amount_sat *val,
 WARN_UNUSED_RESULT bool amount_msat_accumulate(struct amount_msat *a,
 					       struct amount_msat b);
 
+/* a -= b */
+WARN_UNUSED_RESULT bool amount_msat_deduct(struct amount_msat *a,
+					   struct amount_msat b);
+
+/* a -= b */
+WARN_UNUSED_RESULT bool amount_msat_deduct_sat(struct amount_msat *a,
+					       struct amount_sat b);
+
 /* returns floor(msat/div) */
 struct amount_msat amount_msat_div(struct amount_msat msat, u64 div);
 

common/initial_commit_tx.c

@@ -40,7 +40,7 @@ bool try_subtract_fee(enum side opener, enum side side,
 	else
 		opener_amount = other;
 
-	if (amount_msat_sub_sat(opener_amount, *opener_amount, base_fee))
+	if (amount_msat_deduct_sat(opener_amount, base_fee))
 		return true;
 
 	*opener_amount = AMOUNT_MSAT(0);

devtools/mkclose.c

@@ -121,7 +121,7 @@ int main(int argc, char *argv[])
 	if (option_anchor_outputs && !amount_sat_add(&fee, fee, AMOUNT_SAT(660)))
 		errx(1, "Can't afford anchors");
 
-	if (!amount_msat_sub_sat(&local_msat, local_msat, fee))
+	if (!amount_msat_deduct_sat(&local_msat, fee))
 		errx(1, "Can't afford fee %s",
 		     fmt_amount_sat(NULL, fee));
 	if (!amount_sat_sub_msat(&remote_msat, funding_amount, local_msat))

lightningd/invoice.c

@@ -570,7 +570,7 @@ static struct route_info **select_inchan(const tal_t *ctx,
 				    candidates[i].c->our_config.channel_reserve,
 				    candidates[i].c->channel_info.their_config.channel_reserve)
 		    || !amount_sat_to_msat(&capacity, candidates[i].c->funding_sats)
-		    || !amount_msat_sub_sat(&capacity, capacity, cumulative_reserve)) {
+		    || !amount_msat_deduct_sat(&capacity, cumulative_reserve)) {
 			log_broken(ld->log, "Channel %s capacity overflow!",
 					fmt_short_channel_id(tmpctx, *candidates[i].c->scid));
 			continue;

lightningd/peer_control.c

@@ -721,7 +721,7 @@ static void subtract_offered_htlcs(const struct channel *channel,
 	     hout = htlc_out_map_next(ld->htlcs_out, &outi)) {
 		if (hout->key.channel != channel)
 			continue;
-		if (!amount_msat_sub(amount, *amount, hout->msat))
+		if (!amount_msat_deduct(amount, hout->msat))
 			*amount = AMOUNT_MSAT(0);
 	}
 }
@@ -738,7 +738,7 @@ static void subtract_received_htlcs(const struct channel *channel,
 	     hin = htlc_in_map_next(ld->htlcs_in, &ini)) {
 		if (hin->key.channel != channel)
 			continue;
-		if (!amount_msat_sub(amount, *amount, hin->msat))
+		if (!amount_msat_deduct(amount, hin->msat))
 			*amount = AMOUNT_MSAT(0);
 	}
 }
@@ -759,9 +759,9 @@ struct amount_msat channel_amount_spendable(const struct channel *channel)
 
 	/* If we're opener, subtract txfees we'll need to spend this */
 	if (channel->opener == LOCAL) {
-		if (!amount_msat_sub_sat(&spendable, spendable,
-					 commit_txfee(channel, spendable,
-						      LOCAL)))
+		if (!amount_msat_deduct_sat(&spendable,
+					    commit_txfee(channel, spendable,
+							 LOCAL)))
 			return AMOUNT_MSAT(0);
 	}
 
@@ -803,9 +803,9 @@ struct amount_msat channel_amount_receivable(const struct channel *channel)
 
 	/* If they're opener, subtract txfees they'll need to spend this */
 	if (channel->opener == REMOTE) {
-		if (!amount_msat_sub_sat(&receivable, receivable,
-					 commit_txfee(channel,
-						      receivable, REMOTE)))
+		if (!amount_msat_deduct_sat(&receivable,
+					    commit_txfee(channel,
+							 receivable, REMOTE)))
 			return AMOUNT_MSAT(0);
 	}
 

lightningd/peer_htlcs.c

@@ -2006,8 +2006,7 @@ static void remove_htlc_out(struct channel *channel, struct htlc_out *hout)
 		const struct channel_coin_mvt *mvt;
 		struct amount_msat oldamt = channel->our_msat;
 		/* We paid for this HTLC, so deduct balance. */
-		if (!amount_msat_sub(&channel->our_msat, channel->our_msat,
-				     hout->msat)) {
+		if (!amount_msat_deduct(&channel->our_msat, hout->msat)) {
 			channel_internal_error(channel,
 					       "Underflow our_msat %s - HTLC %s",
 					       fmt_amount_msat(tmpctx,

plugins/askrene/askrene.c

@@ -639,6 +639,12 @@ static struct command_result *do_getroutes(struct command *cmd,
 		       "Layer no_mpp_support is active we switch to a "
 		       "single path algorithm.");
 	}
+	if (rq->maxparts == 1 &&
+	    info->dev_algo != ALGO_SINGLE_PATH) {
+		info->dev_algo = ALGO_SINGLE_PATH;
+		rq_log(tmpctx, rq, LOG_DBG,
+		       "maxparts == 1: switching to a single path algorithm.");
+	}
 
 	/* Compute the routes. At this point we might select between multiple
 	 * algorithms. Right now there is only one algorithm available. */
@@ -830,6 +836,11 @@ static struct command_result *json_getroutes(struct command *cmd,
 				    "amount must be non-zero");
 	}
 
+	if (maxparts == 0) {
+		return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
+				    "maxparts must be non-zero");
+	}
+
 	if (*maxdelay > maxdelay_allowed) {
 		return command_fail(cmd, PAY_USER_ERROR,
 				    "maximum delay allowed is %d",
@@ -1057,7 +1068,7 @@ static struct command_result *json_askrene_inform_channel(struct command *cmd,
 		if (!reserve_accumulate(askrene->reserved, scidd, amount))
 			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
 					    "Amount overflow with reserves");
-		if (!amount_msat_sub(amount, *amount, AMOUNT_MSAT(1)))
+		if (!amount_msat_deduct(amount, AMOUNT_MSAT(1)))
 			*amount = AMOUNT_MSAT(0);
 		if (command_check_only(cmd))
 			return command_check_done(cmd);

plugins/askrene/mcf.c

@@ -345,7 +345,7 @@ static double pickhardt_richter_probability(struct amount_msat low,
 	struct amount_msat all_states, good_states;
 	if (amount_msat_greater_eq(amount, high))
 		return 0.0;
-	if (!amount_msat_sub(&amount, amount, low))
+	if (!amount_msat_deduct(&amount, low))
 		return 1.0;
 	if (!amount_msat_sub(&all_states, high, low))
 		abort(); // we expect high > low
@@ -1377,34 +1377,14 @@ linear_routes(const tal_t *ctx, struct route_query *rq,
 	struct reserve_hop *reservations = new_reservations(working_ctx, rq);
 
 	while (!amount_msat_is_zero(amount_to_deliver)) {
-		size_t num_parts, parts_slots, excess_parts;
-		u32 bottleneck_idx;
-
 		if (timemono_after(time_mono(), deadline)) {
 			error_message = rq_log(ctx, rq, LOG_BROKEN,
 					       "%s: timed out after deadline",
 					       __func__);
 			goto fail;
 		}
 
-                /* FIXME: This algorithm to limit the number of parts is dumb
-                 * for two reasons:
-                 *      1. it does not take into account that several loop
-                 *      iterations here may produce two flows along the same
-                 *      path that after "squash_flows" become a single flow.
-                 *      2. limiting the number of "slots" to 1 makes us fail to
-                 *      see some solutions that use more than one of those
-                 *      existing paths.
-                 *
-                 * A better approach could be to run MCF, remove the excess
-                 * paths and then recompute a MCF on a network where each arc is
-                 * one of the previously remaining paths, ie. redistributing the
-                 * payment amount among the selected paths in a cost-efficient
-                 * way. */
 		new_flows = tal_free(new_flows);
-		num_parts = tal_count(*flows);
-		assert(num_parts < rq->maxparts);
-		parts_slots = rq->maxparts - num_parts;
 
 		/* If the amount_to_deliver is very small we better use a single
 		 * path computation because:
@@ -1415,8 +1395,7 @@ linear_routes(const tal_t *ctx, struct route_query *rq,
                  * do not deliver the entire payment amount by just a small
                  * amount. */
 		if (amount_msat_less_eq(amount_to_deliver,
-					SINGLE_PATH_THRESHOLD) ||
-		    parts_slots == 1) {
+					SINGLE_PATH_THRESHOLD)) {
 			new_flows = single_path_flow(working_ctx, rq, srcnode,
 						     dstnode, amount_to_deliver,
 						     mu, delay_feefactor);
@@ -1433,7 +1412,7 @@ linear_routes(const tal_t *ctx, struct route_query *rq,
 		}
 
 		error_message =
-			refine_flows(ctx, rq, amount_to_deliver, &new_flows, &bottleneck_idx);
+			refine_flows(ctx, rq, amount_to_deliver, &new_flows);
 		if (error_message)
 			goto fail;
 
@@ -1457,32 +1436,6 @@ linear_routes(const tal_t *ctx, struct route_query *rq,
 			assert(!amount_msat_is_zero(new_flows[i]->delivers));
 		}
 
-		if (tal_count(new_flows) > parts_slots) {
-			/* Remove the excees of parts and leave one slot for the
-			 * next round of computations. */
-			excess_parts = 1 + tal_count(new_flows) - parts_slots;
-		} else if (tal_count(new_flows) == parts_slots &&
-			   amount_msat_less(all_deliver, amount_to_deliver)) {
-			/* Leave exactly 1 slot for the next round of
-			 * computations. */
-			excess_parts = 1;
-		} else
-			excess_parts = 0;
-		if (excess_parts > 0) {
-			/* If we removed all the flows we found, avoid selecting them again,
-			 * by disabling one. */
-			if (excess_parts == tal_count(new_flows))
-				bitmap_set_bit(rq->disabled_chans, bottleneck_idx);
-			if (!remove_flows(&new_flows, excess_parts)) {
-				error_message = rq_log(ctx, rq, LOG_BROKEN,
-						       "%s: failed to remove %zu"
-						       " flows from a set of %zu",
-						       __func__, excess_parts,
-						       tal_count(new_flows));
-				goto fail;
-			}
-		}
-
 		/* Is this set of flows too expensive?
 		 * We can check if the new flows are within the fee budget,
 		 * however in some cases we have discarded some flows at this
@@ -1587,9 +1540,8 @@ linear_routes(const tal_t *ctx, struct route_query *rq,
 			}
 		}
 
-		if (!amount_msat_sub(&feebudget, feebudget, all_fees) ||
-		    !amount_msat_sub(&amount_to_deliver, amount_to_deliver,
-				     all_deliver)) {
+		if (!amount_msat_deduct(&feebudget, all_fees) ||
+		    !amount_msat_deduct(&amount_to_deliver, all_deliver)) {
 			error_message =
 			    rq_log(ctx, rq, LOG_BROKEN,
 				   "%s: unexpected arithmetic operation "
@@ -1607,6 +1559,34 @@ linear_routes(const tal_t *ctx, struct route_query *rq,
 	/* all set! Now squash flows that use the same path */
 	squash_flows(ctx, rq, flows);
 
+	/* If we're over the number of parts, try to cram excess into the
+	 * largest-capacity parts */
+	if (tal_count(*flows) > rq->maxparts) {
+		struct amount_msat fee;
+
+		error_message = reduce_num_flows(rq, rq, flows, amount, rq->maxparts);
+		if (error_message) {
+			*flows = tal_free(*flows);
+			return error_message;
+		}
+
+		/* Check fee budget! */
+		fee = flowset_fee(rq->plugin, *flows);
+		if (amount_msat_greater(fee, maxfee)) {
+			error_message = rq_log(rq, rq, LOG_INFORM,
+					       "After reducing the flows to %zu (i.e. maxparts),"
+					       " we had a fee of %s, greater than "
+					       "max of %s.",
+					       tal_count(*flows),
+					       fmt_amount_msat(tmpctx, fee),
+					       fmt_amount_msat(tmpctx, maxfee));
+			if (error_message) {
+				*flows = tal_free(*flows);
+				return error_message;
+			}
+		}
+	}
+
 	/* flows_probability re-does a temporary reservation so we need to call
 	 * it after we have cleaned the reservations we used to build the flows
 	 * hence after we freed working_ctx. */
@@ -1619,24 +1599,21 @@ linear_routes(const tal_t *ctx, struct route_query *rq,
 		    rq_log(rq, rq, LOG_BROKEN,
 			   "%s: check_htlc_min_limits failed", __func__);
 		*flows = tal_free(*flows);
-		goto fail;
+		return error_message;
 	}
 	if (!check_htlc_max_limits(rq, *flows)) {
-		error_message =
-		    rq_log(rq, rq, LOG_BROKEN,
-			   "%s: check_htlc_max_limits failed", __func__);
 		*flows = tal_free(*flows);
-		goto fail;
+		return rq_log(rq, rq, LOG_BROKEN,
+			      "%s: check_htlc_max_limits failed", __func__);
 	}
 	if (tal_count(*flows) > rq->maxparts) {
-		error_message = rq_log(
-		    rq, rq, LOG_BROKEN,
-		    "%s: the number of flows (%zu) exceeds the limit set "
-		    "on payment parts (%" PRIu32
-		    "), please submit a bug report",
-		    __func__, tal_count(*flows), rq->maxparts);
+		size_t num_flows = tal_count(*flows);
 		*flows = tal_free(*flows);
-		goto fail;
+		return rq_log(rq, rq, LOG_BROKEN,
+			      "%s: the number of flows (%zu) exceeds the limit set "
+			      "on payment parts (%" PRIu32
+			      "), please submit a bug report",
+			      __func__, num_flows, rq->maxparts);
 	}
 
 	return NULL;