lightningd: fail incoming HTLCs if peer would close channel.

This cause of cascading failure was pointed out by @t-bast: if fees spike and
you don't timeout an outgoing onchain HTLC, you should nonetheless fail the incoming htlc
because otherwise the incoming peer will close on you.

Of course, there's a risk of losing funds, but this only happens if you weren't going to get the HTLC spend in time anyway.  And it would also catch any other reason that the downstream onchain goes wrong, containing the damage.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reported-by: @t-bast
Changelog-Fixed: Protocol: We will close incoming HTLCs early if the outgoing HTLC is stuck onchain long enough, to avoid cascating failure.

lightningd/peer_htlcs.c

@@ -1754,8 +1754,14 @@ void onchain_failed_our_htlc(const struct channel *channel,
 	} else if (hout->in) {
 		log_debug(channel->log, "HTLC id %"PRIu64" has incoming",
 			  htlc->id);
-		local_fail_in_htlc(hout->in,
-				   take(towire_permanent_channel_failure(NULL)));
+ 		/* Careful!  We might have already timed out incoming
+		 * HTLC in consider_failing_incoming */
+		if (hout->in->badonion == 0
+		    && !hout->in->failonion
+		    && !hout->in->preimage) {
+			local_fail_in_htlc(hout->in,
+					   take(towire_permanent_channel_failure(NULL)));
+		}
 		wallet_forwarded_payment_add(hout->key.channel->peer->ld->wallet,
 					 hout->in, FORWARD_STYLE_TLV,
 					 channel_scid_or_local_alias(channel), hout,
@@ -2661,6 +2667,43 @@ static u32 htlc_in_deadline(const struct lightningd *ld,
 	return hin->cltv_expiry - (ld->config.cltv_expiry_delta + 1)/2;
 }
 
+/* onchaind might fail to time out an HTLC: maybe fees spiked, or maybe
+ * it decided it wasn't worthwhile.  This risks cascading failure if
+ * it was routed: the incoming peer will get upset with us, too.
+ *
+ * So, if we're within 3 blocks of this happening, we fail upstream.
+ * It's weird to do this by looking at hout, rather than hin, but
+ * there's a pointer from hout->hin and not vice versa (we don't
+ * normally need it). */
+static void consider_failing_incoming(struct lightningd *ld,
+				      u32 height,
+				      struct htlc_out *hout)
+{
+	/* Already failed or succeeded? */
+	if (hout->failmsg || hout->failonion || hout->preimage)
+		return;
+
+	/* Has no corresponding input we should be stressed about? */
+	if (!hout->in)
+		return;
+
+	/* Already done it once? */
+	if (hout->in->failonion)
+		return;
+
+	/* OK, if we're within 3 blocks of upstream getting upset, force it
+	 * to fail without waiting for onchaind. */
+	if (height + 3 < hout->in->cltv_expiry)
+		return;
+
+	log_unusual(hout->key.channel->log,
+		    "Abandoning unresolved onchain HTLC at block %u"
+		    " (expired at %u) to avoid peer closing incoming HTLC at block %u",
+		    height, hout->cltv_expiry, hout->in->cltv_expiry);
+
+	local_fail_in_htlc(hout->in, take(towire_permanent_channel_failure(NULL)));
+}
+
 void htlcs_notify_new_block(struct lightningd *ld, u32 height)
 {
 	bool removed;
@@ -2687,8 +2730,10 @@ void htlcs_notify_new_block(struct lightningd *ld, u32 height)
 				continue;
 
 			/* Peer on chain already? */
-			if (channel_on_chain(hout->key.channel))
+			if (channel_on_chain(hout->key.channel)) {
+				consider_failing_incoming(ld, height, hout);
 				continue;
+			}
 
 			/* Peer already failed, or we hit it? */
 			if (hout->key.channel->error)

tests/test_closing.py

@@ -3799,7 +3799,6 @@ def test_closing_anchorspend_htlc_tx_rbf(node_factory, bitcoind):
     bitcoind.generate_block(1, needfeerate=4990)
 
 
-@pytest.mark.xfail(strict=True)
 @pytest.mark.developer("needs dev_disconnect")
 @pytest.mark.parametrize("anchors", [False, True])
 def test_htlc_no_force_close(node_factory, bitcoind, anchors):