pay: remove cmd pointer from htlc_out.

Maintaining it was always fraught, since the command could go away if the JSON RPC died. Most recently, it was broken again on shutdown (see below). In future we may allow pay commands to block on previous payments, so it won't even be a 1:1 mapping. Generalize it: keep commands in a simple list and do a lookup when a payment fails/succeeds. Valgrind error file: valgrind-errors.5732 ==5732== Invalid read of size 8 ==5732== at 0x4149FD: remove_cmd_from_hout (pay.c:292) ==5732== by 0x468BAB: notify (tal.c:237) ==5732== by 0x469077: del_tree (tal.c:400) ==5732== by 0x4690C7: del_tree (tal.c:410) ==5732== by 0x46948A: tal_free (tal.c:509) ==5732== by 0x40F1EA: main (lightningd.c:362) ==5732== Address 0x69df148 is 1,512 bytes inside a block of size 1,544 free'd ==5732== at 0x4C2EDEB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==5732== by 0x469150: del_tree (tal.c:421) ==5732== by 0x46948A: tal_free (tal.c:509) ==5732== by 0x4198F2: free_htlcs (peer_control.c:1281) ==5732== by 0x40EBA9: shutdown_subdaemons (lightningd.c:209) ==5732== by 0x40F1DE: main (lightningd.c:360) ==5732== Block was alloc'd at ==5732== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==5732== by 0x468C30: allocate (tal.c:250) ==5732== by 0x4691F7: tal_alloc_ (tal.c:448) ==5732== by 0x40A279: new_htlc_out (htlc_end.c:143) ==5732== by 0x41FD64: send_htlc_out (peer_htlcs.c:397) ==5732== by 0x41511C: send_payment (pay.c:388) ==5732== by 0x41589E: json_sendpay (pay.c:513) ==5732== by 0x40D9B1: parse_request (jsonrpc.c:600) ==5732== by 0x40DCAC: read_json (jsonrpc.c:667) ==5732== by 0x45C706: next_plan (io.c:59) ==5732== by 0x45D1DD: do_plan (io.c:387) ==5732== by 0x45D21B: io_ready (io.c:397) Signed-off-by: Rusty Russell <[email protected]>
ElementsProject · Feb 2, 2018 · 89fb750 · 89fb750
1 parent 53a9e4f
commit 89fb750
Show file tree

Hide file tree

Showing 8 changed files with 70 additions and 44 deletions.
diff --git a/lightningd/htlc_end.c b/lightningd/htlc_end.c
@@ -125,8 +125,6 @@ struct htlc_out *htlc_out_check(const struct htlc_out *hout,
 			       htlc_state_name(hout->hstate));
 	else if (hout->failuremsg && hout->preimage)
 		return corrupt(hout, abortstr, "Both failed and succeeded");
-	else if (hout->cmd && hout->in)
-		return corrupt(hout, abortstr, "Both local and forwarded");
 
 	return cast_const(struct htlc_out *, hout);
 }
@@ -137,8 +135,7 @@ struct htlc_out *new_htlc_out(const tal_t *ctx,
 			      u64 msatoshi, u32 cltv_expiry,
 			      const struct sha256 *payment_hash,
 			      const u8 *onion_routing_packet,
-			      struct htlc_in *in,
-			      struct command *cmd)
+			      struct htlc_in *in)
 {
 	struct htlc_out *hout = tal(ctx, struct htlc_out);
 
@@ -150,7 +147,6 @@ struct htlc_out *new_htlc_out(const tal_t *ctx,
 	hout->msatoshi = msatoshi;
 	hout->cltv_expiry = cltv_expiry;
 	hout->payment_hash = *payment_hash;
-	hout->cmd = cmd;
 	memcpy(hout->onion_routing_packet, onion_routing_packet,
 	       sizeof(hout->onion_routing_packet));
 

diff --git a/lightningd/htlc_end.h b/lightningd/htlc_end.h
@@ -75,9 +75,6 @@ struct htlc_out {
 
 	/* Where it's from, if not going to us. */
 	struct htlc_in *in;
-
-	/* Otherwise, this MAY be non-null if there's a pay command waiting */
-	struct command *cmd;
 };
 
 static inline const struct htlc_key *keyof_htlc_in(const struct htlc_in *in)
@@ -132,8 +129,7 @@ struct htlc_out *new_htlc_out(const tal_t *ctx,
 			      u64 msatoshi, u32 cltv_expiry,
 			      const struct sha256 *payment_hash,
 			      const u8 *onion_routing_packet,
-			      struct htlc_in *in,
-			      struct command *cmd);
+			      struct htlc_in *in);
 
 void connect_htlc_in(struct htlc_in_map *map, struct htlc_in *hin);
 void connect_htlc_out(struct htlc_out_map *map, struct htlc_out *hout);

diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c
@@ -62,6 +62,7 @@ static struct lightningd *new_lightningd(const tal_t *ctx,
 	ld->alias = NULL;
 	ld->rgb = NULL;
 	list_head_init(&ld->connects);
+	list_head_init(&ld->pay_commands);
 	ld->wireaddrs = tal_arr(ld, struct wireaddr, 0);
 	ld->portnum = DEFAULT_PORT;
 	timers_init(&ld->timers, time_mono());

diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h
@@ -128,6 +128,9 @@ struct lightningd {
 
 	struct wallet *wallet;
 
+	/* Outstanding sendpay/pay commands. */
+	struct list_head pay_commands;
+
 	/* Maintained by invoices.c */
 	struct invoices *invoices;
 

diff --git a/lightningd/pay.c b/lightningd/pay.c
@@ -18,28 +18,74 @@
 #include <lightningd/subd.h>
 #include <sodium/randombytes.h>
 
-static void json_pay_success(struct command *cmd, const struct preimage *rval)
+/* pay/sendpay command */
+struct pay_command {
+	struct list_node list;
+
+	struct sha256 payment_hash;
+	struct command *cmd;
+};
+
+static void destroy_pay_command(struct pay_command *pc)
 {
-	struct json_result *response;
+	list_del(&pc->list);
+}
 
-	/* Can be NULL if JSON RPC goes away. */
-	if (!cmd)
-		return;
+/* Owned by cmd */
+static struct pay_command *new_pay_command(struct command *cmd,
+					   const struct sha256 *payment_hash,
+					   struct lightningd *ld)
+{
+	struct pay_command *pc = tal(cmd, struct pay_command);
+
+	pc->payment_hash = *payment_hash;
+	pc->cmd = cmd;
+	list_add(&ld->pay_commands, &pc->list);
+	tal_add_destructor(pc, destroy_pay_command);
+	return pc;
+}
+
+static void json_pay_command_success(struct command *cmd,
+				     const struct preimage *payment_preimage)
+{
+	struct json_result *response;
 
 	response = new_json_result(cmd);
 	json_object_start(response, NULL);
-	json_add_hex(response, "preimage", rval, sizeof(*rval));
+	json_add_hex(response, "preimage",
+		     payment_preimage, sizeof(*payment_preimage));
 	json_object_end(response);
 	command_success(cmd, response);
 }
 
-static void json_pay_failed(struct command *cmd,
+static void json_pay_success(struct lightningd *ld,
+			     const struct sha256 *payment_hash,
+			     const struct preimage *payment_preimage)
+{
+	struct pay_command *pc, *next;
+
+	list_for_each_safe(&ld->pay_commands, pc, next, list) {
+		if (!structeq(payment_hash, &pc->payment_hash))
+			continue;
+
+		/* Deletes itself. */
+		json_pay_command_success(pc->cmd, payment_preimage);
+	}
+}
+
+static void json_pay_failed(struct lightningd *ld,
+			    const struct sha256 *payment_hash,
 			    enum onion_type failure_code,
 			    const char *details)
 {
-	/* Can be NULL if JSON RPC goes away. */
-	if (cmd) {
-		command_fail(cmd, "Failed: %s (%s)",
+	struct pay_command *pc, *next;
+
+	list_for_each_safe(&ld->pay_commands, pc, next, list) {
+		if (!structeq(payment_hash, &pc->payment_hash))
+			continue;
+
+		/* Deletes itself. */
+		command_fail(pc->cmd, "failed: %s (%s)",
 			     onion_type_name(failure_code), details);
 	}
 }
@@ -49,10 +95,7 @@ void payment_succeeded(struct lightningd *ld, struct htlc_out *hout,
 {
 	wallet_payment_set_status(ld->wallet, &hout->payment_hash,
 				  PAYMENT_COMPLETE, rval);
-	/* Can be NULL if JSON RPC goes away. */
-	if (hout->cmd)
-		json_pay_success(hout->cmd, rval);
-	hout->cmd = NULL;
+	json_pay_success(ld, &hout->payment_hash, rval);
 }
 
 /* Return NULL if the wrapped onion error message has no
@@ -282,17 +325,10 @@ void payment_failed(struct lightningd *ld, const struct htlc_out *hout,
 	 * and payment again. */
 	(void) retry_plausible;
 
-	json_pay_failed(hout->cmd, failcode, failmsg);
+	json_pay_failed(ld, &hout->payment_hash, failcode, failmsg);
 	tal_free(tmpctx);
 }
 
-/* When JSON RPC goes away, cmd is freed: detach from the hout */
-static void remove_cmd_from_hout(struct command *cmd, struct htlc_out *hout)
-{
-	assert(hout->cmd == cmd);
-	hout->cmd = NULL;
-}
-
 /* Returns true if it's still pending. */
 static bool send_payment(struct command *cmd,
 			 const struct sha256 *rhash,
@@ -362,7 +398,8 @@ static bool send_payment(struct command *cmd,
 							    &payment->destination));
 				return false;
 			}
-			json_pay_success(cmd, payment->payment_preimage);
+			json_pay_command_success(cmd,
+						 payment->payment_preimage);
 			return false;
 		}
 		wallet_payment_delete(cmd->ld->wallet, rhash);
@@ -387,8 +424,7 @@ static bool send_payment(struct command *cmd,
 
 	failcode = send_htlc_out(peer, route[0].amount,
 				 base_expiry + route[0].delay,
-				 rhash, onion, NULL, cmd,
-				 &hout);
+				 rhash, onion, NULL, &hout);
 	if (failcode) {
 		command_fail(cmd, "First peer not ready: %s",
 			     onion_type_name(failcode));
@@ -416,9 +452,7 @@ static bool send_payment(struct command *cmd,
 	/* We write this into db when HTLC is actually sent. */
 	wallet_payment_setup(cmd->ld->wallet, payment);
 
-	/* If we fail, remove cmd ptr from htlc_out. */
-	tal_add_destructor2(cmd, remove_cmd_from_hout, hout);
-
+	new_pay_command(cmd, rhash, cmd->ld);
 	tal_free(tmpctx);
 	return true;
 }

diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c
@@ -375,7 +375,6 @@ enum onion_type send_htlc_out(struct peer *out, u64 amount, u32 cltv,
 			      const struct sha256 *payment_hash,
 			      const u8 *onion_routing_packet,
 			      struct htlc_in *in,
-			      struct command *cmd,
 			      struct htlc_out **houtp)
 {
 	struct htlc_out *hout;
@@ -395,8 +394,7 @@ enum onion_type send_htlc_out(struct peer *out, u64 amount, u32 cltv,
 
 	/* Make peer's daemon own it, catch if it dies. */
 	hout = new_htlc_out(out->owner, out, amount, cltv,
-			    payment_hash, onion_routing_packet, in,
-			    cmd);
+			    payment_hash, onion_routing_packet, in);
 	tal_add_destructor(hout, hout_subd_died);
 
 	msg = towire_channel_offer_htlc(out, amount, cltv, payment_hash,
@@ -488,7 +486,7 @@ static void forward_htlc(struct htlc_in *hin,
 
 	failcode = send_htlc_out(next, amt_to_forward,
 				 outgoing_cltv_value, &hin->payment_hash,
-				 next_onion, hin, NULL, NULL);
+				 next_onion, hin, NULL);
 	if (!failcode)
 		return;
 

diff --git a/lightningd/peer_htlcs.h b/lightningd/peer_htlcs.h
@@ -39,7 +39,6 @@ enum onion_type send_htlc_out(struct peer *out, u64 amount, u32 cltv,
 			      const struct sha256 *payment_hash,
 			      const u8 *onion_routing_packet,
 			      struct htlc_in *in,
-			      struct command *cmd,
 			      struct htlc_out **houtp);
 
 struct htlc_out *find_htlc_out_by_ripemd(const struct peer *peer,

diff --git a/wallet/wallet.c b/wallet/wallet.c
@@ -1059,7 +1059,6 @@ static bool wallet_stmt2htlc_out(const struct wallet_channel *channel,
 
 	out->failuremsg = NULL;
 	out->failcode = 0;
-	out->cmd = NULL;
 
 	/* Need to defer wiring until we can look up all incoming
 	 * htlcs, will wire using origin_htlc_id */