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

doc/developers-guide/plugin-development/event-notifications.md

@@ -181,6 +181,19 @@ A notification for topic `invoice_creation` is sent every time an invoice is cre
 }
 ```
 
+If the invoice is associated with a BOLT 12 offer, an `offer_id` field will be present:
+
+```json
+{
+  "invoice_creation": {
+    "label": "unique-label-for-invoice",
+    "preimage": "0000000000000000000000000000000000000000000000000000000000000000",
+    "msat": 10000,
+    "offer_id": "b1d1062abc09790a68be83e4c257614a57978e4053a954bfee5909ed71e23e03"
+  }
+}
+```
+
 Before version `23.11` the `msat` field was a string with msat-suffix, e.g: `"10000msat"`.
 
 ### `warning`

lightningd/invoice.c

@@ -872,7 +872,7 @@ invoice_complete(struct invoice_info *info,
 	json_add_u64(response, "created_index", details->created_index);
 
 	notify_invoice_creation(info->cmd->ld, info->b11->msat,
-				&info->payment_preimage, info->label);
+				&info->payment_preimage, info->label, NULL);
 
 	if (warning_no_listincoming)
 		json_add_string(response, "warning_listincoming",
@@ -1686,7 +1686,7 @@ static struct command_result *json_createinvoice(struct command *cmd,
 				     NULL))
 			return fail_exists(cmd, label);
 
-		notify_invoice_creation(cmd->ld, b11->msat, preimage, label);
+		notify_invoice_creation(cmd->ld, b11->msat, preimage, label, NULL);
 	} else {
 		struct tlv_invoice *inv;
 		struct sha256 offer_id, *local_offer_id;
@@ -1783,7 +1783,7 @@ static struct command_result *json_createinvoice(struct command *cmd,
 				     local_offer_id))
 			return fail_exists(cmd, label);
 
-		notify_invoice_creation(cmd->ld, &msat,	preimage, label);
+		notify_invoice_creation(cmd->ld, &msat, preimage, label, local_offer_id);
 	}
 
 	response = json_stream_success(cmd);

lightningd/notification.c

@@ -228,26 +228,30 @@ void notify_invoice_payment(struct lightningd *ld,
 static void invoice_creation_notification_serialize(struct json_stream *stream,
 						    const struct amount_msat *amount,
 						    const struct preimage *preimage,
-						    const struct json_escape *label)
+						    const struct json_escape *label,
+						    const struct sha256 *offer_id)
 {
 	if (amount != NULL)
 		json_add_amount_msat(stream, "msat", *amount);
 
 	json_add_preimage(stream, "preimage", preimage);
 	json_add_escaped_string(stream, "label", label);
+	if (offer_id)
+		json_add_sha256(stream, "offer_id", offer_id);
 }
 
 REGISTER_NOTIFICATION(invoice_creation)
 
 void notify_invoice_creation(struct lightningd *ld,
 			     const struct amount_msat *amount,
 			     const struct preimage *preimage,
-			     const struct json_escape *label)
+			     const struct json_escape *label,
+			     const struct sha256 *offer_id)
 {
 	struct jsonrpc_notification *n = notify_start(ld, "invoice_creation");
 	if (!n)
 		return;
-	invoice_creation_notification_serialize(n->stream, amount, preimage, label);
+	invoice_creation_notification_serialize(n->stream, amount, preimage, label, offer_id);
 	notify_send(ld, n);
 }
 

lightningd/notification.h

@@ -46,7 +46,8 @@ void notify_invoice_payment(struct lightningd *ld,
 void notify_invoice_creation(struct lightningd *ld,
 			     const struct amount_msat *amount,
 			     const struct preimage *preimage,
-			     const struct json_escape *label);
+			     const struct json_escape *label,
+			     const struct sha256 *offer_id);
 
 void notify_channel_opened(struct lightningd *ld,
 			   const struct node_id *node_id,

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

lightningd/test/run-invoice-select-inchan.c

@@ -530,7 +530,8 @@ void notify_disconnect(struct lightningd *ld UNNEEDED, const struct node_id *nod
 void notify_invoice_creation(struct lightningd *ld UNNEEDED,
 			     const struct amount_msat *amount UNNEEDED,
 			     const struct preimage *preimage UNNEEDED,
-			     const struct json_escape *label UNNEEDED)
+			     const struct json_escape *label UNNEEDED,
+			     const struct sha256 *offer_id UNNEEDED)
 { fprintf(stderr, "notify_invoice_creation called!\n"); abort(); }
 /* Generated stub for notify_invoice_payment */
 void notify_invoice_payment(struct lightningd *ld UNNEEDED,

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,177 @@ 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[^,]')
+
+
+@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd anchors unsupported')
+def test_fulfilled_htlc_deadline_reconnect(node_factory, bitcoind, executor):
+    """After the incoming-HTLC deadline fires and l2 skips the force-close
+    (SENT_REMOVE_HTLC — removal in progress), a manual reconnect should cause
+    channeld to retransmit update_fulfill_htlc, completing the payment
+    cooperatively with the channel staying open.
+
+    This tests complements test_fulfilled_htlc_deadline_no_force_close:
+    the window between l2's deadline (cltv_expiry - 3) and l1's outgoing
+    deadline (cltv_expiry + 1) is enough for reconnect + retransmit to
+    settle the HTLC without going on-chain.
+    """
+    # dev-no-reconnect prevents automatic reconnect; we trigger it manually
+    # after the deadline fires.  may_reconnect suppresses unexpected-disconnect
+    # log-check failures in both l1 and l2.
+    opts = [{'dev-no-reconnect': None,
+             'may_reconnect': True,
+             'feerates': (7500, 7500, 7500, 7500)},
+            {'disconnect': ['-WIRE_UPDATE_FULFILL_HTLC'],
+             'dev-no-reconnect': None,
+             'may_reconnect': True,
+             '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_deadline_reconnect', 'desc')
+
+    # same route setup used for test_fulfilled_htlc_deadline_no_force_close
+    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; preimage reaches l2.  l2 transitions the incoming HTLC to
+    # SENT_REMOVE_HTLC and tells channeld to send update_fulfill_htlc.  The
+    # dev_disconnect (-WIRE_UPDATE_FULFILL_HTLC) fires before the message is
+    # sent, killing the connection.  Under Valgrind, channeld can also advance
+    # the state to SENT_REMOVE_COMMIT by calling channel_sending_commit()
+    # (optimistically reporting the state to lightningd) before the write fd
+    # is fully sabotaged.  Both states are safe: the preimage is persisted.
+    l2.daemon.wait_for_log(r'chan#1: Peer transient failure in CHANNELD_NORMAL')
+
+    htlc = only_one(only_one(l2.rpc.listpeerchannels(l1.info['id'])['channels'])['htlcs'])
+    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']
+
+    # Mine to l2's incoming-HTLC deadline. l2 should NOT force-close.
+    # The preimage is safe and channeld will retransmit on reconnect.
+    deadline = cltv_expiry - (6 + 1) // 2
+    current_height = bitcoind.rpc.getblockcount()
+    blocks_to_deadline = deadline - current_height
+    assert blocks_to_deadline >= 1, \
+        f"No room before deadline: deadline={deadline}, height={current_height}"
+    bitcoind.generate_block(blocks_to_deadline)
+    sync_blockheight(bitcoind, [l1, 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[^,]')
+
+    # Reconnect l2 -> l1 now: channeld will do channel_reestablish and then
+    # call send_fail_or_fulfill() for the SENT_REMOVE_HTLC, retransmitting
+    # update_fulfill_htlc upstream. Payment should complete.
+    l2.rpc.connect(l1.info['id'], 'localhost', l1.port)
+
+    fut = executor.submit(l1.rpc.waitsendpay, inv['payment_hash'])
+    fut.result(TIMEOUT)
+
+    # Both nodes should stay in CHANNELD_NORMAL (no force-close occurred).
+    wait_for(lambda: only_one(
+        l1.rpc.listpeerchannels(l2.info['id'])['channels'])['state'] == 'CHANNELD_NORMAL')
+    wait_for(lambda: only_one(
+        l2.rpc.listpeerchannels(l1.info['id'])['channels'])['state'] == 'CHANNELD_NORMAL')
+
+    # The HTLC must be gone from l2's view (fully settled).
+    wait_for(lambda: only_one(
+        l2.rpc.listpeerchannels(l1.info['id'])['channels'])['htlcs'] == [])
+
+
 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