renepay: rethink the computed routes storage

Simply move the "computed routes" array from the payment to the
routetracker. It makes sense to put all temporary stages of routing into
a single data structure: the routetracker.

Signed-off-by: Lagrang3 <lagrang3@protonmail.com>

plugins/renepay/mods.c

@@ -631,8 +631,11 @@ REGISTER_PAYMENT_MODIFIER(routehints, routehints_cb);
 static struct command_result *compute_routes_cb(struct payment *payment)
 {
 	assert(payment->status == PAYMENT_PENDING);
+	struct routetracker *routetracker = payment->routetracker;
+	assert(routetracker);
 
-	if (payment->routes_computed)
+	if (routetracker->computed_routes &&
+	    tal_count(routetracker->computed_routes))
 		plugin_err(pay_plugin->plugin,
 			   "%s: no previously computed routes expected.",
 			   __PRETTY_FUNCTION__);
@@ -670,24 +673,37 @@ static struct command_result *compute_routes_cb(struct payment *payment)
 	const char *err_msg = NULL;
 
 	gossmap_apply_localmods(pay_plugin->gossmap, payment->local_gossmods);
+
+	/* get_routes returns the answer, we assign it to the computed_routes,
+	 * that's why we need to tal_free the older array. Maybe it would be
+	 * better to pass computed_routes as a reference? */
+	routetracker->computed_routes = tal_free(routetracker->computed_routes);
+
 	// TODO: add an algorithm selector here
 	/* We let this return an unlikely path, as it's better to try  once than
 	 * simply refuse.  Plus, models are not truth! */
-	// TODO routes_computed should be in the routetracker instead
-	payment->routes_computed = get_routes(
-	    payment, &payment->payment_info, &pay_plugin->my_id,
-	    &payment->payment_info.destination, pay_plugin->gossmap,
-	    pay_plugin->uncertainty, payment->disabledmap, remaining, feebudget,
-
-	    &payment->next_partid, payment->groupid,
-
-	    &errcode, &err_msg);
+	routetracker->computed_routes = get_routes(
+					    routetracker,
+					    &payment->payment_info,
+					    &pay_plugin->my_id,
+					    &payment->payment_info.destination,
+					    pay_plugin->gossmap,
+					    pay_plugin->uncertainty,
+					    payment->disabledmap,
+					    remaining,
+					    feebudget,
+					    &payment->next_partid,
+					    payment->groupid,
+					    &errcode,
+					    &err_msg);
+	/* Otherwise the error message remains a child of the routetracker. */
 	err_msg = tal_steal(tmpctx, err_msg);
 
 	gossmap_remove_localmods(pay_plugin->gossmap, payment->local_gossmods);
 
 	/* Couldn't feasible route, we stop. */
-	if (!payment->routes_computed) {
+	if (!routetracker->computed_routes ||
+	    tal_count(routetracker->computed_routes) == 0) {
 		if (err_msg == NULL)
 			err_msg = tal_fmt(
 			    tmpctx, "get_routes returned NULL error message");
@@ -709,7 +725,10 @@ REGISTER_PAYMENT_MODIFIER(compute_routes, compute_routes_cb);
 static struct command_result *send_routes_cb(struct payment *payment)
 {
 	assert(payment);
-	if (!payment->routes_computed) {
+	struct routetracker *routetracker = payment->routetracker;
+	assert(routetracker);
+	if (!routetracker->computed_routes ||
+	    tal_count(routetracker->computed_routes) == 0) {
 		plugin_log(pay_plugin->plugin, LOG_UNUSUAL,
 			   "%s: there are no routes to send, skipping.",
 			   __PRETTY_FUNCTION__);
@@ -717,10 +736,10 @@ static struct command_result *send_routes_cb(struct payment *payment)
 	}
 	struct command *cmd = payment_command(payment);
 	assert(cmd);
-	for (size_t i = 0; i < tal_count(payment->routes_computed); i++) {
-		struct route *route = payment->routes_computed[i];
+	for (size_t i = 0; i < tal_count(routetracker->computed_routes); i++) {
+		struct route *route = routetracker->computed_routes[i];
 
-		route_sendpay_request(cmd, route, payment);
+		route_sendpay_request(cmd, take(route), payment);
 
 		payment_note(payment, LOG_INFORM,
 			     "Sent route request: partid=%" PRIu64
@@ -731,8 +750,7 @@ static struct command_result *send_routes_cb(struct payment *payment)
 			     fmt_amount_msat(tmpctx, route_fees(route)),
 			     route_delay(route), fmt_route_path(tmpctx, route));
 	}
-	payment->routes_computed = tal_free(payment->routes_computed);
-
+	tal_resize(&routetracker->computed_routes, 0);
 	return payment_continue(payment);
 }
 
@@ -826,9 +844,6 @@ static struct command_result *collect_results_cb(struct payment *payment)
 		payment->retry = true;
 	}
 
-	// FIXME: do we need to check for timeout? We might endup in an
-	// infinite loop of collect results.
-
 	return payment_continue(payment);
 }
 

plugins/renepay/payment.c

@@ -105,7 +105,6 @@ struct payment *payment_new(
 	p->retry = false;
 	p->waitresult_timer = NULL;
 
-	p->routes_computed = NULL;
 	p->routetracker = new_routetracker(p, p);
 	return p;
 }
@@ -124,7 +123,6 @@ static void payment_cleanup(struct payment *p)
 	disabledmap_reset(p->disabledmap);
 	p->waitresult_timer = tal_free(p->waitresult_timer);
 
-	p->routes_computed = tal_free(p->routes_computed);
 	routetracker_cleanup(p->routetracker);
 }
 
@@ -198,12 +196,6 @@ bool payment_update(
 	p->retry = false;
 	p->waitresult_timer = tal_free(p->waitresult_timer);
 
-	/* It is weird to have routes here stuck. */
-	if (p->routes_computed)
-		plugin_log(pay_plugin->plugin, LOG_UNUSUAL,
-			   "We have %zu unsent routes in this payment.",
-			   tal_count(p->routes_computed));
-	p->routes_computed = tal_free(p->routes_computed);
 	return true;
 }
 

plugins/renepay/payment.h

@@ -71,7 +71,6 @@ struct payment {
 	/* Timer we use to wait for results. */
 	struct plugin_timer *waitresult_timer;
 
-	struct route **routes_computed;
 	struct routetracker *routetracker;
 };
 

plugins/renepay/routetracker.c

@@ -22,10 +22,15 @@ struct routetracker *new_routetracker(const tal_t *ctx, struct payment *payment)
 {
 	struct routetracker *rt = tal(ctx, struct routetracker);
 
+	rt->computed_routes = tal_arr(rt, struct route *, 0);
 	rt->sent_routes = tal(rt, struct route_map);
-	route_map_init(rt->sent_routes);
-
 	rt->finalized_routes = tal_arr(rt, struct route *, 0);
+
+	if (!rt->computed_routes || !rt->sent_routes || !rt->finalized_routes)
+		/* bad allocation */
+		return tal_free(rt);
+
+	route_map_init(rt->sent_routes);
 	return rt;
 }
 
@@ -63,10 +68,22 @@ static void remove_route(struct route *route, struct route_map *map)
 }
 
 static void route_sent_register(struct routetracker *routetracker,
-				struct route *route STEALS)
+				struct route *route TAKES)
+{
+	if(taken(route))
 	{
 		route_map_add(routetracker->sent_routes, route);
-	tal_steal(routetracker, route);
+		tal_steal(routetracker->sent_routes, route);
+	}else{
+		struct route *cp_route = tal_dup(routetracker->sent_routes, struct route, route);
+		cp_route->hops = tal_dup_talarr(cp_route, struct route_hop, route->hops);
+
+		/* A full deepcopy of the struct route would require to
+		 * duplicate the final_msg and result as well, however
+		 * sent_register is called when those are supposed to be absent. */
+		assert(cp_route->final_msg==0 && cp_route->result==NULL);
+		route_map_add(routetracker->sent_routes, cp_route);
+	}
 }
 static void route_sendpay_fail(struct routetracker *routetracker,
 			       struct route *route TAKES)
@@ -260,7 +277,7 @@ void payment_collect_results(struct payment *payment,
 }
 
 struct command_result *route_sendpay_request(struct command *cmd,
-					     struct route *route,
+					     struct route *route TAKES,
 					     struct payment *payment)
 {
 	struct out_req *req =

plugins/renepay/routetracker.h

@@ -7,6 +7,9 @@
 #include <plugins/renepay/route.h>
 
 struct routetracker{
+	/* Routes that we compute and are kept here before sending them. */
+	struct route **computed_routes;
+
 	/* Routes that we sendpay and are still waiting for rpc returning
 	 * success. */
 	struct route_map *sent_routes;
@@ -33,7 +36,7 @@ void payment_collect_results(struct payment *payment,
 
 /* Sends a sendpay request for this route. */
 struct command_result *route_sendpay_request(struct command *cmd,
-					     struct route *route,
+					     struct route *route TAKES,
 					     struct payment *payment);
 
 struct command_result *notification_sendpay_failure(struct command *cmd,