lightningd: don't force-close when fulfilled HTLC removal is in progress

lightningd/peer_htlcs.c

@@ -2975,6 +2975,23 @@ void htlcs_notify_new_block(struct lightningd *ld)
 			if (channel->error)
 				continue;
 
+			/* Fulfill is in progress: we have the preimage
+			 * and have already told channeld (or will on
+			 * reconnect).  Don't force-close; if peer goes
+			 * on-chain, onchaind will claim using the
+			 * preimage.
+			 * See https://github.com/ElementsProject/lightning/issues/8899 */
+			if (hin->hstate >= SENT_REMOVE_HTLC) {
+				log_unusual(channel->log,
+					    "Fulfilled HTLC %"PRIu64
+					    " %s cltv %u hit deadline,"
+					    " but removal already in progress",
+					    hin->key.id,
+					    htlc_state_name(hin->hstate),
+					    hin->cltv_expiry);
+				continue;
+			}
+
 			channel_fail_permanent(channel,
 					       REASON_PROTOCOL,
 					       "Fulfilled HTLC %"PRIu64

tests/test_closing.py

@@ -3887,7 +3887,16 @@ def test_closing_anchorspend_htlc_tx_rbf(node_factory, bitcoind):
 @pytest.mark.parametrize("anchors", [False, True])
 def test_htlc_no_force_close(node_factory, bitcoind, anchors):
     """l2<->l3 force closes while an HTLC is in flight from l1, but l2 can't timeout because the feerate has spiked.  It should do so anyway."""
-    opts = [{}, {}, {'disconnect': ['-WIRE_UPDATE_FULFILL_HTLC']}]
+    # l3 disconnects before sending update_fulfill_htlc to l2, and
+    # dev-no-reconnect prevents automatic reconnection.  This leaves
+    # l3's incoming HTLC in SENT_REMOVE_HTLC: l3 has the preimage but
+    # can't deliver it.  l3 no longer force-closes for this (the
+    # preimage is safe in the DB and onchaind will claim on-chain),
+    # so l2 force-closes first for the offered HTLC timeout.
+    opts = [{'dev-no-reconnect': None},
+            {'dev-no-reconnect': None},
+            {'disconnect': ['-WIRE_UPDATE_FULFILL_HTLC'],
+             'dev-no-reconnect': None}]
     if anchors is False:
         for opt in opts:
             opt['dev-force-features'] = "-23"
@@ -3911,17 +3920,20 @@ def test_htlc_no_force_close(node_factory, bitcoind, anchors):
 
     htlc_txs = []
 
-    # l3 drops to chain, holding htlc (but we stop it xmitting txs)
+    # l3 won't force-close (removal in progress), so censor its
+    # onchaind txs for when l2 goes on-chain and l3 sees the
+    # commitment.
     def censoring_sendrawtx(r):
         htlc_txs.append(r['params'][0])
         return {'id': r['id'], 'result': {}}
 
     l3.daemon.rpcproxy.mock_rpc('sendrawtransaction', censoring_sendrawtx)
 
-    # l3 gets upset, drops to chain when there are < 4 blocks remaining.
-    # But tx doesn't get mined...
+    # l3 should NOT force-close: it has the preimage and the removal is
+    # already in progress.  It just logs a warning.
     bitcoind.generate_block(8)
-    l3.daemon.wait_for_log("Peer permanent failure in CHANNELD_NORMAL: Fulfilled HTLC 0 SENT_REMOVE_.* cltv 119 hit deadline")
+    l3.daemon.wait_for_log(r'but removal already in progress')
+    assert not l3.daemon.is_in_log(r'Peer permanent failure in CHANNELD_NORMAL: Fulfilled HTLC 0 SENT_REMOVE')
 
     # l2 closes drops the commitment tx at block 115 (one block after timeout)
     bitcoind.generate_block(4)
@@ -3957,6 +3969,89 @@ def censoring_sendrawtx(r):
     # FIXME: l2 should complain!
 
 
+@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd anchors unsupported')
+def test_fulfilled_htlc_deadline_no_force_close(node_factory, bitcoind):
+    """Test that l2 does not force-close when fulfilled HTLC is in
+    SENT_REMOVE_HTLC state (preimage known, fulfill queued to channeld
+    but not yet sent to upstream peer).
+
+    Reproduces https://github.com/ElementsProject/lightning/issues/8899:
+    CLN force-closed with 'Fulfilled HTLC SENT_REMOVE_HTLC cltv hit deadline'
+    without attempting to send update_fulfill_htlc upstream first.
+    """
+    # l1 -> l2 -> l3 topology.
+    # l2 disconnects from l1 right before sending update_fulfill_htlc,
+    # so the incoming HTLC on l1-l2 stays in SENT_REMOVE_HTLC.
+    # l2 cannot reconnect (dev-no-reconnect), simulating the scenario where
+    # the upstream peer appears connected but isn't processing messages.
+    #
+    # Use identical feerates to avoid gratuitous commits to update them.
+    opts = [{'dev-no-reconnect': None,
+             'feerates': (7500, 7500, 7500, 7500)},
+            {'disconnect': ['-WIRE_UPDATE_FULFILL_HTLC'],
+             'dev-no-reconnect': None,
+             'feerates': (7500, 7500, 7500, 7500)},
+            {'feerates': (7500, 7500, 7500, 7500)}]
+
+    l1, l2, l3 = node_factory.line_graph(3, opts=opts, wait_for_announce=True)
+
+    amt = 12300000
+    inv = l3.rpc.invoice(amt, 'test_fulfilled_deadline', 'desc')
+
+    # Use explicit route with known delays to have predictable cltv_expiry.
+    # delay=16 for first hop (cltv_delta=6 + cltv_final=10),
+    # delay=10 for second hop (cltv_final=10).
+    route = [{'amount_msat': amt + 1 + amt * 10 // 1000000,
+              'id': l2.info['id'],
+              'delay': 16,
+              'channel': first_scid(l1, l2)},
+             {'amount_msat': amt,
+              'id': l3.info['id'],
+              'delay': 10,
+              'channel': first_scid(l2, l3)}]
+    l1.rpc.sendpay(route, inv['payment_hash'],
+                   payment_secret=inv['payment_secret'])
+
+    # l3 fulfills the HTLC, preimage flows back to l2.
+    # l2 transitions the incoming HTLC (from l1) to SENT_REMOVE_HTLC,
+    # then tries to send update_fulfill_htlc to l1 but disconnects.
+    # Wait for channeld on the l1-l2 channel to die after the disconnect.
+    # Note: "Peer transient failure" appears just before the dev_disconnect
+    # log in connectd, so we must wait for it first to avoid advancing the
+    # log search position past it.
+    l2.daemon.wait_for_log(r'chan#1: Peer transient failure in CHANNELD_NORMAL')
+
+    # Get the incoming HTLC's cltv_expiry from listpeerchannels.
+    htlc = only_one(only_one(l2.rpc.listpeerchannels(l1.info['id'])['channels'])['htlcs'])
+    # Under Valgrind, the state may advance to SENT_REMOVE_COMMIT before
+    # the disconnect fully takes effect.  Both states reproduce the bug.
+    assert htlc['state'] in ('SENT_REMOVE_HTLC', 'SENT_REMOVE_COMMIT'), \
+        f"Expected SENT_REMOVE_HTLC or SENT_REMOVE_COMMIT, got {htlc['state']}"
+    cltv_expiry = htlc['expiry']
+
+    # Compute the deadline dynamically from the actual HTLC cltv_expiry.
+    # htlc_in_deadline = cltv_expiry - (cltv_expiry_delta + 1)/2
+    # With regtest cltv_expiry_delta=6: deadline = cltv_expiry - 3
+    deadline = cltv_expiry - (6 + 1) // 2
+    current_height = bitcoind.rpc.getblockcount()
+
+    # Mine up to one block before the deadline — should NOT trigger force-close.
+    blocks_to_deadline = deadline - current_height
+    assert blocks_to_deadline > 1, f"Not enough room: deadline={deadline}, height={current_height}"
+    bitcoind.generate_block(blocks_to_deadline - 1)
+    sync_blockheight(bitcoind, [l2])
+    assert not l2.daemon.is_in_log('hit deadline')
+
+    # Mine one more block to hit the deadline.
+    # l2 should NOT force-close: it has the preimage and the removal is
+    # already in progress (SENT_REMOVE_HTLC or SENT_REMOVE_COMMIT).
+    # It just needs to reconnect to send update_fulfill_htlc upstream.
+    bitcoind.generate_block(1)
+    sync_blockheight(bitcoind, [l2])
+    l2.daemon.wait_for_log(r'but removal already in progress')
+    assert not l2.daemon.is_in_log(r'Fulfilled HTLC 0 SENT_REMOVE_(HTLC|COMMIT) cltv .* hit deadline[^,]')
+
+
 def test_closing_tx_valid(node_factory, bitcoind):
     l1, l2 = node_factory.line_graph(2, opts={'may_reconnect': True,
                                               'dev-no-reconnect': None})

tests/test_misc.py

@@ -521,13 +521,18 @@ def test_htlc_out_timeout(node_factory, bitcoind, executor):
 def test_htlc_in_timeout(node_factory, bitcoind, executor):
     """Test that we drop onchain if the peer doesn't accept fulfilled HTLC"""
 
-    # HTLC 1->2, 1 fails after 2 has sent committed the fulfill
+    # HTLC 1->2, 1 fails after 2 has sent committed the fulfill.
+    # l2 has the preimage but can't deliver it (l1 disconnected and
+    # won't reconnect).  l2 no longer force-closes for this (removal
+    # is in progress, preimage safe in DB).  Instead l1 force-closes
+    # when the offered HTLC hits its deadline, and l2 claims on-chain
+    # using the preimage via onchaind.
     disconnects = ['-WIRE_REVOKE_AND_ACK*2']
     # Feerates identical so we don't get gratuitous commit to update them
     l1 = node_factory.get_node(disconnect=disconnects,
                                options={'dev-no-reconnect': None},
                                feerates=(7500, 7500, 7500, 7500))
-    l2 = node_factory.get_node()
+    l2 = node_factory.get_node(options={'dev-no-reconnect': None})
     # Give it some sats for anchor spend!
     l2.fundwallet(25000, mine_block=False)
 
@@ -556,15 +561,23 @@ def test_htlc_in_timeout(node_factory, bitcoind, executor):
     assert not l2.daemon.is_in_log('hit deadline')
     bitcoind.generate_block(1)
 
-    l2.daemon.wait_for_log('Fulfilled HTLC 0 SENT_REMOVE_COMMIT cltv .* hit deadline')
-    l2.daemon.wait_for_log('sendrawtx exit 0')
-    l2.bitcoin.generate_block(1)
-    l2.daemon.wait_for_log(' to ONCHAIN')
+    # l2 has the preimage but removal is in progress — it should NOT
+    # force-close.  It logs a warning instead.
+    l2.daemon.wait_for_log(r'but removal already in progress')
+    assert not l2.daemon.is_in_log(r'Peer permanent failure in CHANNELD_NORMAL: Fulfilled HTLC')
+
+    # l1's offered HTLC hits deadline (cltv_expiry + 1), which is a
+    # few blocks after l2's fulfilled deadline.  l1 force-closes.
+    bitcoind.generate_block(4 + shadowlen)
+    l1.daemon.wait_for_log('Offered HTLC 0 SENT_ADD_ACK_REVOCATION cltv .* hit deadline')
+    l1.daemon.wait_for_log('sendrawtx exit 0')
+    l1.bitcoin.generate_block(1)
     l1.daemon.wait_for_log(' to ONCHAIN')
+    l2.daemon.wait_for_log(' to ONCHAIN')
 
-    # L2 will collect HTLC (iff no shadow route)
+    # L2 will collect HTLC using the preimage (from l1's unilateral)
     _, txid, blocks = l2.wait_for_onchaind_tx('OUR_HTLC_SUCCESS_TX',
-                                              'OUR_UNILATERAL/THEIR_HTLC')
+                                              'THEIR_UNILATERAL/THEIR_HTLC')
     assert blocks == 0
 
     # If we try to reuse the same output as we used for the anchor spend, then