channeld: handle HTLCs failed by failcode uniformly.

'struct htlc' in channeld has a 'malformed' field, which is really only
used in the "retransmit updates on reconnect" case.  That's quite confusing,
and I'm not entirely convinced that it can only be set to a failcode
with the BADONION bit set.

So generalize it, using the same logic we use in the master daemon:

failcode: a locally generated error, for channeld to turn into the appropriate
          error message.
fail: a remotely generated onion error, for forwarding.

Either of these being non-zero/non-NULL means we've failed, and only one
should be set at any time.

We unify the "send htlc fail/fulfill update due to retransmit" and the
normal send update paths, by always calling send_fail_or_fulfill.

This unification revealed that we accidentally skipped the
onion-wrapping stage when we retransmit failed htlcs!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

channeld/channel.c

@@ -1214,7 +1214,7 @@ static u8 *got_commitsig_msg(const tal_t *ctx,
 				f = tal_arr_append(&failed);
 				*f = tal(failed, struct failed_htlc);
 				(*f)->id = htlc->id;
-				(*f)->malformed = htlc->malformed;
+				(*f)->malformed = htlc->failcode;
 				(*f)->failreason = cast_const(u8 *, htlc->fail);
 			}
 		} else {
@@ -1464,6 +1464,7 @@ static void handle_peer_fulfill_htlc(struct peer *peer, const u8 *msg)
 	u64 id;
 	struct preimage preimage;
 	enum channel_remove_err e;
+	struct htlc *h;
 
 	if (!fromwire_update_fulfill_htlc(msg, &channel_id,
 					  &id, &preimage)) {
@@ -1472,9 +1473,10 @@ static void handle_peer_fulfill_htlc(struct peer *peer, const u8 *msg)
 			    "Bad update_fulfill_htlc %s", tal_hex(msg, msg));
 	}
 
-	e = channel_fulfill_htlc(peer->channel, LOCAL, id, &preimage, NULL);
+	e = channel_fulfill_htlc(peer->channel, LOCAL, id, &preimage, &h);
 	switch (e) {
 	case CHANNEL_ERR_REMOVE_OK:
+		h->r = tal_dup(h, struct preimage, &preimage);
 		/* FIXME: We could send preimages to master immediately. */
 		start_commit_timer(peer);
 		return;
@@ -1740,16 +1742,31 @@ static void resend_revoke(struct peer *peer)
 static void send_fail_or_fulfill(struct peer *peer, const struct htlc *h)
 {
 	u8 *msg;
-	if (h->malformed) {
+
+	if (h->failcode & BADONION) {
+		/* Malformed: use special reply since we can't onion. */
 		struct sha256 sha256_of_onion;
 		sha256(&sha256_of_onion, h->routing, tal_len(h->routing));
 
 		msg = towire_update_fail_malformed_htlc(NULL, &peer->channel_id,
 							h->id, &sha256_of_onion,
-							h->malformed);
+							h->failcode);
-	} else if (h->fail) {
+	} else if (h->failcode || h->fail) {
-		msg = towire_update_fail_htlc(NULL, &peer->channel_id, h->id,
+		const u8 *onion;
-					      h->fail);
+		if (h->failcode) {
+			/* Local failure, make a message. */
+			u8 *failmsg = make_failmsg(tmpctx, peer, h, h->failcode,
+						   &peer->short_channel_ids[LOCAL]);
+			onion = create_onionreply(tmpctx, h->shared_secret,
+						  failmsg);
+		} else /* Remote failure, just forward. */
+			onion = h->fail;
+
+		/* Now we wrap, just before sending out. */
+		msg = towire_update_fail_htlc(peer, &peer->channel_id, h->id,
+					      wrap_onionreply(tmpctx,
+							      h->shared_secret,
+							      onion));
 	} else if (h->r) {
 		msg = towire_update_fulfill_htlc(NULL, &peer->channel_id, h->id,
 						 h->r);
@@ -2194,18 +2211,17 @@ static void handle_feerates(struct peer *peer, const u8 *inmsg)
 
 static void handle_preimage(struct peer *peer, const u8 *inmsg)
 {
-	u8 *msg;
 	u64 id;
 	struct preimage preimage;
+	struct htlc *h;
 
 	if (!fromwire_channel_fulfill_htlc(inmsg, &id, &preimage))
 		master_badmsg(WIRE_CHANNEL_FULFILL_HTLC, inmsg);
 
-	switch (channel_fulfill_htlc(peer->channel, REMOTE, id, &preimage, NULL)) {
+	switch (channel_fulfill_htlc(peer->channel, REMOTE, id, &preimage, &h)) {
 	case CHANNEL_ERR_REMOVE_OK:
-		msg = towire_update_fulfill_htlc(NULL, &peer->channel_id,
+		h->r = tal_dup(h, struct preimage, &preimage);
-						 id, &preimage);
+		send_fail_or_fulfill(peer, h);
-		enqueue_peer_msg(peer, take(msg));
 		start_commit_timer(peer);
 		return;
 	/* These shouldn't happen, because any offered HTLC (which would give
@@ -2224,7 +2240,6 @@ static void handle_preimage(struct peer *peer, const u8 *inmsg)
 
 static void handle_fail(struct peer *peer, const u8 *inmsg)
 {
-	u8 *msg;
 	u64 id;
 	u8 *errpkt;
 	u16 failcode;
@@ -2244,33 +2259,9 @@ static void handle_fail(struct peer *peer, const u8 *inmsg)
 	e = channel_fail_htlc(peer->channel, REMOTE, id, &h);
 	switch (e) {
 	case CHANNEL_ERR_REMOVE_OK:
-		if (failcode & BADONION) {
+		h->failcode = failcode;
-			struct sha256 sha256_of_onion;
+		h->fail = tal_steal(h, errpkt);
-			status_trace("Failing %"PRIu64" with code %u",
+		send_fail_or_fulfill(peer, h);
-				     id, failcode);
-			sha256(&sha256_of_onion, h->routing,
-			       tal_len(h->routing));
-			msg = towire_update_fail_malformed_htlc(peer,
-							&peer->channel_id,
-							id, &sha256_of_onion,
-							failcode);
-		} else {
-			u8 *reply;
-
-			if (failcode) {
-				u8 *failmsg = make_failmsg(inmsg, peer, h,
-							   failcode, &scid);
-				errpkt = create_onionreply(inmsg,
-							   h->shared_secret,
-							   failmsg);
-			}
-
-			reply = wrap_onionreply(inmsg, h->shared_secret,
-						errpkt);
-			msg = towire_update_fail_htlc(peer, &peer->channel_id,
-						      id, reply);
-		}
-		enqueue_peer_msg(peer, take(msg));
 		start_commit_timer(peer);
 		return;
 	case CHANNEL_ERR_NO_SUCH_ID:

channeld/channeld_htlc.h

@@ -27,8 +27,12 @@ struct htlc {
 	/* FIXME: We could union these together: */
 	/* Routing information sent with this HTLC. */
 	const u8 *routing;
+
+	/* Failure message we received or generated. */
 	const u8 *fail;
-	enum onion_type malformed;
+	/* For a local failure, we might have to generate fail ourselves
+	 * (or, if BADONION we send a update_fail_malformed_htlc). */
+	enum onion_type failcode;
 };
 
 static inline bool htlc_has(const struct htlc *h, int flag)

channeld/full_channel.c

@@ -107,7 +107,8 @@ static void dump_htlc(const struct htlc *htlc, const char *prefix)
 		     htlc_state_name(htlc->state),
 		     htlc_state_name(remote_state),
 		     htlc->r ? "FULFILLED" : htlc->fail ? "FAILED" :
-		     htlc->malformed ? "MALFORMED" : "");
+		     htlc->failcode
+		     ? tal_fmt(tmpctx, "FAILCODE:%u", htlc->failcode) : "");
 }
 
 void dump_htlcs(const struct channel *channel, const char *prefix)
@@ -324,7 +325,7 @@ static enum channel_add_err add_htlc(struct channel *channel,
 
 	htlc->rhash = *payment_hash;
 	htlc->fail = NULL;
-	htlc->malformed = 0;
+	htlc->failcode = 0;
 	htlc->r = NULL;
 	htlc->routing = tal_dup_arr(htlc, u8, routing, TOTAL_PACKET_SIZE, 0);
 
@@ -903,7 +904,7 @@ static bool adjust_balance(struct channel *channel, struct htlc *htlc)
 		if (htlc_has(htlc, HTLC_FLAG(side, HTLC_F_COMMITTED)))
 			continue;
 
-		if (!htlc->fail && !htlc->malformed && !htlc->r) {
+		if (!htlc->fail && !htlc->failcode && !htlc->r) {
 			status_trace("%s HTLC %"PRIu64
 				     " %s neither fail nor fulfill?",
 				     htlc_state_owner(htlc->state) == LOCAL
@@ -1028,10 +1029,10 @@ bool channel_force_htlcs(struct channel *channel,
 				     failed[i]->id);
 			return false;
 		}
-		if (htlc->malformed) {
+		if (htlc->failcode) {
-			status_trace("Fail %s HTLC %"PRIu64" already malformed",
+			status_trace("Fail %s HTLC %"PRIu64" already fail %u",
 				     failed_sides[i] == LOCAL ? "out" : "in",
-				     failed[i]->id);
+				     failed[i]->id, htlc->failcode);
 			return false;
 		}
 		if (!htlc_has(htlc, HTLC_REMOVING)) {
@@ -1042,7 +1043,7 @@ bool channel_force_htlcs(struct channel *channel,
 			return false;
 		}
 		if (failed[i]->malformed)
-			htlc->malformed = failed[i]->malformed;
+			htlc->failcode = failed[i]->malformed;
 		else
 			htlc->fail = tal_dup_arr(htlc, u8,
 						 failed[i]->failreason,