lightningd: unify pay vs forward path when handling failures.

It's a bit tricky since we want to hand more verbose errors to the local case, but the locally-created and forwarded paths had diverged (the local one missing some things). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
7 years ago · 7882bc0536
4 changed files with 82 additions and 78 deletions
--- a/lightningd/pay.c
+++ b/lightningd/pay.c
@ -9,6 +9,7 @@
 #include <lightningd/channel/gen_channel_wire.h>
 #include <lightningd/lightningd.h>
 #include <lightningd/peer_control.h>
+#include <lightningd/peer_htlcs.h>
 #include <lightningd/sphinx.h>
 #include <lightningd/subd.h>
 #include <sodium/randombytes.h>
@ -70,12 +71,23 @@ void payment_succeeded(struct lightningd *ld, struct htlc_out *hout,
 	hout->pay_command->out = NULL;
 }

-void payment_failed(struct lightningd *ld, const struct htlc_out *hout)
+void payment_failed(struct lightningd *ld, const struct htlc_out *hout,
+		    const char *localfail)
 {
 	struct pay_command *pc = hout->pay_command;
 	enum onion_type failcode;
 	struct onionreply *reply;

+	/* This gives more details than a generic failure message,
+	 * and also the failuremsg here is unencrypted */
+	if (localfail) {
+		size_t max = tal_len(hout->failuremsg);
+		const u8 *p = hout->failuremsg;
+		failcode = fromwire_u16(&p, &max);
+		json_pay_failed(pc, NULL, failcode, localfail);
+		return;
+	}
+
 	if (hout->malformed)
 		failcode = hout->malformed;
 	else {
@ -106,35 +118,6 @@ void payment_failed(struct lightningd *ld, const struct htlc_out *hout)
 	json_pay_failed(pc, NULL, failcode, "reply from remote");
 }

-static bool rcvd_htlc_reply(struct subd *subd, const u8 *msg, const int *fds,
-			    struct pay_command *pc)
-{
-	u16 failcode;
-	u8 *failstr;
-
-	if (!fromwire_channel_offer_htlc_reply(msg, msg, NULL,
-					       &pc->out->key.id,
-					       &failcode, &failstr)) {
-		json_pay_failed(pc, &subd->ld->dstate.id, -1,
-				"daemon bad response");
-		return false;
-	}
-
-	if (failcode != 0) {
-		/* Make sure we have a nul-terminated string. */
-		char *str = (char *)tal_dup_arr(msg, u8,
-						failstr, tal_len(failstr), 1);
-		str[tal_len(failstr)] = 0;
-		json_pay_failed(pc, &subd->ld->dstate.id, failcode, str);
-		return true;
-	}
-
-	/* HTLC endpoint now owned by lightningd. */
-	tal_steal(subd->ld, pc->out);
-	connect_htlc_out(&subd->ld->htlcs_out, pc->out);
-	return true;
-}
-
 /* When JSON RPC goes away, cmd is freed: detach from any running paycommand */
 static void remove_cmd_from_pc(struct command *cmd, struct pay_command *pc)
 {
@ -179,7 +162,6 @@ static void json_sendpay(struct command *cmd,
 	struct hop_data first_hop_data;
 	u64 amount, lastamount;
 	struct onionpacket *packet;
-	u8 *msg;
 	struct secret *path_secrets;

 	if (!json_get_params(buffer, params,
@ -357,15 +339,10 @@ static void json_sendpay(struct command *cmd,
 	pc->msatoshi = lastamount;
 	pc->path_secrets = tal_steal(pc, path_secrets);

-	pc->out = new_htlc_out(pc, peer, amount, first_hop_data.outgoing_cltv,
-			       &rhash, onion, NULL, pc);
-
 	log_info(ld->log, "Sending %"PRIu64" over %zu hops to deliver %"PRIu64,
 		 amount, n_hops, lastamount);
-	msg = towire_channel_offer_htlc(pc, amount,
-					first_hop_data.outgoing_cltv,
-					&pc->rhash, onion);
-	subd_req(pc, peer->owner, take(msg), -1, 0, rcvd_htlc_reply, pc);
+	pc->out = send_htlc_out(peer, amount, first_hop_data.outgoing_cltv,
+				&rhash, onion, NULL, pc);

 	/* Wait until we get response. */
 	tal_add_destructor2(cmd, remove_cmd_from_pc, pc);
--- a/lightningd/pay.h
+++ b/lightningd/pay.h
@ -11,6 +11,7 @@ struct pubkey;
 void payment_succeeded(struct lightningd *ld, struct htlc_out *hout,
 		       const struct preimage *rval);

-void payment_failed(struct lightningd *ld, const struct htlc_out *hout);
+void payment_failed(struct lightningd *ld, const struct htlc_out *hout,
+		    const char *localfail);

 #endif /* LIGHTNING_LIGHTNINGD_PAY_H */
--- a/lightningd/peer_htlcs.c
+++ b/lightningd/peer_htlcs.c
@ -52,7 +52,7 @@ static void relay_htlc_failmsg(struct htlc_in *hin)
 	}
 }

-static u8 *make_failmsg(const tal_t *ctx, const struct htlc_in *hin,
+static u8 *make_failmsg(const tal_t *ctx, u64 msatoshi,
 			enum onion_type failcode,
 			const u8 *channel_update)
 {
@ -76,9 +76,9 @@ static u8 *make_failmsg(const tal_t *ctx, const struct htlc_in *hin,
 	case WIRE_UNKNOWN_NEXT_PEER:
 		return towire_unknown_next_peer(ctx);
 	case WIRE_AMOUNT_BELOW_MINIMUM:
-		return towire_amount_below_minimum(ctx, hin->msatoshi, channel_update);
+		return towire_amount_below_minimum(ctx, msatoshi, channel_update);
 	case WIRE_FEE_INSUFFICIENT:
-		return towire_fee_insufficient(ctx, hin->msatoshi, channel_update);
+		return towire_fee_insufficient(ctx, msatoshi, channel_update);
 	case WIRE_INCORRECT_CLTV_EXPIRY:
 		/* FIXME: ctlv! */
 		return towire_incorrect_cltv_expiry(ctx, 0, channel_update);
@ -94,12 +94,11 @@ static u8 *make_failmsg(const tal_t *ctx, const struct htlc_in *hin,
 		/* FIXME: ctlv! */
 		return towire_final_incorrect_cltv_expiry(ctx, 0);
 	case WIRE_FINAL_INCORRECT_HTLC_AMOUNT:
-		return towire_final_incorrect_htlc_amount(ctx, hin->msatoshi);
+		return towire_final_incorrect_htlc_amount(ctx, msatoshi);
 	case WIRE_INVALID_ONION_VERSION:
 	case WIRE_INVALID_ONION_HMAC:
 	case WIRE_INVALID_ONION_KEY:
-		log_broken(hin->key.peer->log, "Bad failmsg for %s",
-			   onion_type_name(failcode));
+		fatal("Bad failmsg for %s", onion_type_name(failcode));
 	}
 	abort();
 }
@ -118,7 +117,7 @@ static void fail_htlc(struct htlc_in *hin, enum onion_type failcode)
 			/* FIXME: Ask gossip daemon for channel_update. */
 		}

-		msg = make_failmsg(hin, hin, failcode, NULL);
+		msg = make_failmsg(hin, hin->msatoshi, failcode, NULL);
 		hin->failuremsg = create_onionreply(hin, &hin->shared_secret,
 						    msg);
 		tal_free(msg);
@ -127,6 +126,27 @@ static void fail_htlc(struct htlc_in *hin, enum onion_type failcode)
 	relay_htlc_failmsg(hin);
 }

+/* localfail are for handing to the local payer if it's local. */
+static void fail_out_htlc(struct htlc_out *hout, const char *localfail)
+{
+	htlc_out_check(hout, __func__);
+	assert(hout->malformed || hout->failuremsg);
+	if (hout->in) {
+		if (hout->in->malformed)
+			hout->malformed = hout->in->malformed;
+		else {
+			hout->in->failuremsg
+				= tal_dup_arr(hout->in, u8,
+					      hout->failuremsg,
+					      tal_len(hout->failuremsg),
+					      0);
+		}
+		relay_htlc_failmsg(hout->in);
+	} else {
+		payment_failed(hout->key.peer->ld, hout, localfail);
+	}
+}
+
 /* BOLT #4:
 *
 * * `amt_to_forward` - The amount in milli-satoshi to forward to the next
@ -290,13 +310,16 @@ fail:
 *
 * We could queue this and wait for it to come back, but this is simple.
 */
-static void hend_subd_died(struct htlc_out *hout)
+static void hout_subd_died(struct htlc_out *hout)
 {
-	log_debug(hout->in->key.peer->owner->log,
+	log_debug(hout->key.peer->log,
 		  "Failing HTLC %"PRIu64" due to peer death",
-		  hout->in->key.id);
+		  hout->key.id);

-	fail_htlc(hout->in, WIRE_TEMPORARY_CHANNEL_FAILURE);
+	hout->failuremsg = make_failmsg(hout, hout->msatoshi,
+					WIRE_TEMPORARY_CHANNEL_FAILURE,
+					NULL);
+	fail_out_htlc(hout, "Outgoing subdaemon died");
 }

 /* This is where channeld gives us the HTLC id, and also reports if it
@ -347,6 +370,26 @@ static bool rcvd_htlc_reply(struct subd *subd, const u8 *msg, const int *fds,
 	return true;
 }

+struct htlc_out *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 pay_command *pc)
+{
+	struct htlc_out *hout;
+	u8 *msg;
+
+	/* 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, pc);
+	tal_add_destructor(hout, hout_subd_died);
+
+	msg = towire_channel_offer_htlc(out, amount, cltv, payment_hash,
+					onion_routing_packet);
+	subd_req(out->ld, out->owner, take(msg), -1, 0, rcvd_htlc_reply, hout);
+	return hout;
+}
+
 static void forward_htlc(struct htlc_in *hin,
 			 u32 cltv_expiry,
 			 const struct sha256 *payment_hash,
@ -355,12 +398,10 @@ static void forward_htlc(struct htlc_in *hin,
 			 const struct pubkey *next_hop,
 			 const u8 next_onion[TOTAL_PACKET_SIZE])
 {
-	u8 *msg;
 	enum onion_type failcode;
 	u64 fee;
 	struct lightningd *ld = hin->key.peer->ld;
 	struct peer *next = peer_by_id(ld, next_hop);
-	struct htlc_out *out;

 	if (!next) {
 		failcode = WIRE_UNKNOWN_NEXT_PEER;
@ -426,17 +467,9 @@ static void forward_htlc(struct htlc_in *hin,
 		goto fail;
 	}

-	/* Make sure daemon owns it, in case it fails. */
-	out = new_htlc_out(next->owner, next, amt_to_forward,
-			   outgoing_cltv_value, &hin->payment_hash,
-			   next_onion, hin, NULL);
-	tal_add_destructor(out, hend_subd_died);
-
-	msg = towire_channel_offer_htlc(next, amt_to_forward,
-					outgoing_cltv_value,
-					payment_hash, next_onion);
-	subd_req(next->owner, next->owner, take(msg), -1, 0,
-		 rcvd_htlc_reply, out);
+	send_htlc_out(next, amt_to_forward,
+		      outgoing_cltv_value, &hin->payment_hash,
+		      next_onion, hin, NULL);
 	return;

 fail:
@ -723,20 +756,7 @@ static void remove_htlc_out(struct peer *peer, struct htlc_out *hout)

 	/* If it's failed, now we can forward since it's completely locked-in */
 	if (!hout->preimage) {
-		if (hout->in) {
-			if (hout->in->malformed)
-				hout->malformed = hout->in->malformed;
-			else {
-				hout->in->failuremsg
-					= tal_dup_arr(hout->in, u8,
-						      hout->failuremsg,
-						      tal_len(hout->failuremsg),
-						      0);
-			}
-			relay_htlc_failmsg(hout->in);
-		} else {
-			payment_failed(peer->ld, hout);
-		}
+		fail_out_htlc(hout, NULL);
 	} else {
 		/* We paid for this HTLC, so deduct balance. */
 		*peer->balance -= hout->msatoshi;
@ -779,7 +799,7 @@ static bool update_out_htlc(struct peer *peer, u64 id, enum htlc_state newstate)

 	/* First transition into commitment; now it outlives peer. */
 	if (newstate == SENT_ADD_COMMIT) {
-		tal_del_destructor(hout, hend_subd_died);
+		tal_del_destructor(hout, hout_subd_died);
 		tal_steal(peer->ld, hout);

 		/* From now onwards, penalty tx might need this */
--- a/lightningd/peer_htlcs.h
+++ b/lightningd/peer_htlcs.h
@ -3,6 +3,7 @@
 #define LIGHTNING_LIGHTNINGD_PEER_HTLCS_H
 #include "config.h"
 #include <ccan/short_types/short_types.h>
+#include <lightningd/derive_basepoints.h>

 /* FIXME: Define serialization primitive for this? */
 struct channel_info {
@ -30,4 +31,9 @@ int peer_sending_commitsig(struct peer *peer, const u8 *msg);
 int peer_got_commitsig(struct peer *peer, const u8 *msg);
 int peer_got_revoke(struct peer *peer, const u8 *msg);

+struct htlc_out *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 pay_command *pc);
 #endif /* LIGHTNING_LIGHTNINGD_PEER_HTLCS_H */