From efee948d3a399223fb32fbb7021db728223d3dd3 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 2 Jul 2018 14:00:12 +0930 Subject: [PATCH] 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 --- channeld/channel.c | 71 ++++++++++++++++++---------------------- channeld/channeld_htlc.h | 6 +++- channeld/full_channel.c | 15 +++++---- 3 files changed, 44 insertions(+), 48 deletions(-) diff --git a/channeld/channel.c b/channeld/channel.c index 99ac871ae..1851c28b8 100644 --- a/channeld/channel.c +++ b/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); - } else if (h->fail) { - msg = towire_update_fail_htlc(NULL, &peer->channel_id, h->id, - h->fail); + h->failcode); + } else if (h->failcode || h->fail) { + const u8 *onion; + 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, - id, &preimage); - enqueue_peer_msg(peer, take(msg)); + h->r = tal_dup(h, struct preimage, &preimage); + send_fail_or_fulfill(peer, h); 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) { - struct sha256 sha256_of_onion; - status_trace("Failing %"PRIu64" with code %u", - 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)); + h->failcode = failcode; + h->fail = tal_steal(h, errpkt); + send_fail_or_fulfill(peer, h); start_commit_timer(peer); return; case CHANNEL_ERR_NO_SUCH_ID: diff --git a/channeld/channeld_htlc.h b/channeld/channeld_htlc.h index 17e811bc2..b4a3f2a73 100644 --- a/channeld/channeld_htlc.h +++ b/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) diff --git a/channeld/full_channel.c b/channeld/full_channel.c index fe87c8941..be844fb26 100644 --- a/channeld/full_channel.c +++ b/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) { - status_trace("Fail %s HTLC %"PRIu64" already malformed", + if (htlc->failcode) { + 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,