From 978c1699eab4e219828e0bc184336cceda88308a Mon Sep 17 00:00:00 2001
From: Rusty Russell <rusty@rustcorp.com.au>
Date: Mon, 24 Jul 2023 12:38:50 +0930
Subject: [PATCH] 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 | 51 ++++++++++++++++++++++++++++++++++++++---
 tests/test_closing.py   |  1 -
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c
index 69f9fe2d2..91c0e52a0 100644
--- a/lightningd/peer_htlcs.c
+++ b/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)
diff --git a/tests/test_closing.py b/tests/test_closing.py
index e61d2c20e..7ab7914c9 100644
--- a/tests/test_closing.py
+++ b/tests/test_closing.py
@@ -3796,7 +3796,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):