lightningd: stop including channel_update in onion failures

contrib/msggen/msggen/schema.json

@@ -29547,7 +29547,7 @@
                 "created_at": 1738000000,
                 "completed_at": 1738000000,
                 "status": "failed",
-                "erroronion": "8346129e26e458a49e1bcc6c47ad778597e5512384d8ebed58b0cf5b4c1f6479154155d4777ca31b57e56821f82a9d4fa6d1b3498989cacc1c08724a2e4f11c6957b6b7b9f637286f77c0ba22b196cc74e3ec8646b784e478aee3de2f5cf6833870461815532b88cf8787733df99829b2556b9cea45d1de18490d7cb30830eb8ed3eead32ac3cff904a478eb55cf6ec9ae66bfbfa1dd17a7e7827f010b25e22faf8de0968945be013c29153f3a7eda0cd67d377644cc648bf9a2d652e8cc45802809072203bf6554e655bbc67edea50e3578caa965d13e2dd800f4cf29ecbb78b5a33df05de6879e9aa738cff36153011c22d543b616a721529df6337930a14fc184792c4788511b537e9261e36c5fbd64ff80c09f1ea585bc668c1c5c169dca8837b402"
+                "erroronion": "1620ba013cb01068d0e48bb1f001df4a4345e05862f21a27865afb1c9b0f460315db55d4777ca31b576f69dbfd7e34bf69c88a83e7d575471faaaf188f7685acc075a02d6248f274a7620ac758bd9b7f4c105aee382a5b27e808dcb3246fa8956508b57e4029d378af51dc6cd9bbecdd344cb2976ef20f81c77b8c7418404182b30dc0cced7178c5f52ce9e455cf18c9ae67bfbfc64ad126e6827f070b25e22faf8de0968945be003c2915353a7eda0ced7f04f644a2648bf9a2d652e8cc45802809072203bf6554e655bbc67edea50e3578caa965d13e2dd800f4cf29ecbb78b5a33df05de6879e9aa738cff36153011c22d543b616a721529df6337930a14fc184792c4788511b537e9261e36c5fbd64ff80c09f1ea585bc668c1c5c169dca8837b402"
               },
               {
                 "created_index": 11,

doc/schemas/listsendpays.json

@@ -504,7 +504,7 @@
             "created_at": 1738000000,
             "completed_at": 1738000000,
             "status": "failed",
-            "erroronion": "8346129e26e458a49e1bcc6c47ad778597e5512384d8ebed58b0cf5b4c1f6479154155d4777ca31b57e56821f82a9d4fa6d1b3498989cacc1c08724a2e4f11c6957b6b7b9f637286f77c0ba22b196cc74e3ec8646b784e478aee3de2f5cf6833870461815532b88cf8787733df99829b2556b9cea45d1de18490d7cb30830eb8ed3eead32ac3cff904a478eb55cf6ec9ae66bfbfa1dd17a7e7827f010b25e22faf8de0968945be013c29153f3a7eda0cd67d377644cc648bf9a2d652e8cc45802809072203bf6554e655bbc67edea50e3578caa965d13e2dd800f4cf29ecbb78b5a33df05de6879e9aa738cff36153011c22d543b616a721529df6337930a14fc184792c4788511b537e9261e36c5fbd64ff80c09f1ea585bc668c1c5c169dca8837b402"
+            "erroronion": "1620ba013cb01068d0e48bb1f001df4a4345e05862f21a27865afb1c9b0f460315db55d4777ca31b576f69dbfd7e34bf69c88a83e7d575471faaaf188f7685acc075a02d6248f274a7620ac758bd9b7f4c105aee382a5b27e808dcb3246fa8956508b57e4029d378af51dc6cd9bbecdd344cb2976ef20f81c77b8c7418404182b30dc0cced7178c5f52ce9e455cf18c9ae67bfbfc64ad126e6827f070b25e22faf8de0968945be003c2915353a7eda0ced7f04f644a2648bf9a2d652e8cc45802809072203bf6554e655bbc67edea50e3578caa965d13e2dd800f4cf29ecbb78b5a33df05de6879e9aa738cff36153011c22d543b616a721529df6337930a14fc184792c4788511b537e9261e36c5fbd64ff80c09f1ea585bc668c1c5c169dca8837b402"
           },
           {
             "created_index": 11,

lightningd/channel.c

@@ -1324,32 +1324,3 @@ channel_scid_or_local_alias(const struct channel *chan)
 		return *chan->alias[LOCAL];
 }
 
-
-/* BOLT #4:
- * An _intermediate hop_ MUST NOT, but the _final node_:
- *...
- *   - if it returns a `channel_update`:
- *     - MUST set `short_channel_id` to the `short_channel_id` used
- *       by the incoming onion.
- */
-/* So, if the scid doesn't match (redirect or we chose an equivalent
- * channel), we simply don't return an update. */
-const u8 *channel_update_for_error(const tal_t *ctx,
-				   const struct htlc_in *hin,
-				   struct channel *channel)
-{
-	if (!hin || !hin->payload || !hin->payload->forward_channel)
-		return NULL;
-
-	/* We shouldn't have forwarded to the channel if it wasn't
-	 * allowed to use that scid, so we can keep it simple here. */
-	if ((!channel->alias[LOCAL]
-	     || !short_channel_id_eq(*hin->payload->forward_channel, *channel->alias[LOCAL]))
-	    && (!channel->scid
-		|| !short_channel_id_eq(*hin->payload->forward_channel, *channel->scid))) {
-		return NULL;
-	}
-
-	return channel_gossip_update_for_error(ctx, channel);
-}
-

lightningd/channel.h

@@ -973,11 +973,6 @@ void channel_set_billboard(struct channel *channel, bool perm,
 struct htlc_in *channel_has_htlc_in(const struct channel *channel);
 struct htlc_out *channel_has_htlc_out(const struct channel *channel);
 
-/* hin can be NULL */
-const u8 *channel_update_for_error(const tal_t *ctx,
-				   const struct htlc_in *hin,
-				   struct channel *channel);
-
 struct amount_msat htlc_max_possible_send(const struct channel *channel);
 
 /* Given features, what channel_type do we want? */

lightningd/channel_gossip.c

@@ -555,6 +555,28 @@ static bool update_channel_update(const struct channel *channel,
 	return true;
 }
 
+/* We MAY disable a channel on loss of connectivity and re-enable it later
+ * (BOLT #7's channel_update `disable` bit). */
+static void set_channel_disabled(struct channel *channel, bool disabled)
+{
+	struct channel_gossip *cg = channel->channel_gossip;
+	struct lightningd *ld = channel->peer->ld;
+
+	/* Only announced channels are gossiped. */
+	if (!cg || cg->state != CGOSSIP_ANNOUNCED)
+		return;
+
+	/* Don't create/broadcast updates while shutting down. */
+	if (!ld->gossip || ld->state == LD_STATE_GRACE
+	    || ld->state == LD_STATE_SHUTDOWN)
+		return;
+
+	if (update_channel_update(channel, !disabled))
+		broadcast_new_gossip(ld, cg->cupdate, NULL,
+				     disabled ? "channel disabled"
+					      : "channel enabled");
+}
+
 /* Using default logic, should this channel be enabled? */
 static bool channel_should_enable(const struct channel *channel,
 				  bool ok_if_disconnected)
@@ -625,7 +647,7 @@ static void cupdate_timer_refresh(struct channel *channel)
 
 	/* Free old cupdate to force a new one to be generated */
 	cg->cupdate = tal_free(cg->cupdate);
-	update_channel_update(channel, channel_should_enable(channel, true));
+	update_channel_update(channel, channel_should_enable(channel, false));
 
 	broadcast_new_gossip(ld, cg->cupdate, NULL, "channel update");
 	arm_refresh_timer(channel);
@@ -856,7 +878,7 @@ static void set_gossip_state(struct channel *channel,
 		send_channel_announcement(channel);
 
 		/* Any private cupdate will be different from this, so will force a refresh. */
-		update_channel_update(channel, channel_should_enable(channel, true));
+		update_channel_update(channel, channel_should_enable(channel, false));
 		broadcast_new_gossip(channel->peer->ld, cg->cupdate, NULL, "channel update");
 
 		/* We need to refresh channel update every 13 days */
@@ -919,7 +941,7 @@ static void update_gossip_state(struct channel *channel)
 		return;
 	case CGOSSIP_ANNOUNCED:
 		/* If a channel parameter has changed, send new update */
-		if (update_channel_update(channel, channel_should_enable(channel, true)))
+		if (update_channel_update(channel, channel_should_enable(channel, false)))
 			broadcast_new_gossip(channel->peer->ld, cg->cupdate, NULL,
 					     "channel update");
 		return;
@@ -1193,7 +1215,7 @@ void channel_gossip_init_done(struct lightningd *ld)
 				    "gossipd lost track of announced channel: re-announcing!");
 			check_channel_gossip(channel);
 			send_channel_announcement(channel);
-			update_channel_update(channel, channel_should_enable(channel, true));
+			update_channel_update(channel, channel_should_enable(channel, false));
 			broadcast_new_gossip(ld, channel->channel_gossip->cupdate, NULL, "channel update");
 
 			/* We need to refresh channel update every 13 days */
@@ -1232,6 +1254,9 @@ void channel_gossip_channel_reestablished(struct channel *channel,
 	/* We can re-xmit sigs once per reconnect */
 	channel->channel_gossip->sent_sigs = false;
 
+	/* We may have disabled this channel when the peer went away. */
+	set_channel_disabled(channel, false);
+
 	if (announcement_sigs_requested)
 		send_channel_announce_sigs(channel);
 
@@ -1271,40 +1296,10 @@ void channel_gossip_channel_disconnect(struct channel *channel)
 {
 	channel->stable_conn_timer = tal_free(channel->stable_conn_timer);
 	channel->reestablished = false;
-}
-
-/* We *could* send channel_updates for private channels, or
- * unannounced.  We do not */
-const u8 *channel_gossip_update_for_error(const tal_t *ctx,
-					  struct channel *channel)
-{
-	/* We cannot ask this about unsaved channels. */
-	struct channel_gossip *cg = channel->channel_gossip;
 
-	switch (cg->state) {
-	case CGOSSIP_CHANNEL_DEAD:
-	case CGOSSIP_CHANNEL_UNANNOUNCED_DYING:
-	case CGOSSIP_PRIVATE_WAITING_FOR_USABLE:
-	case CGOSSIP_PRIVATE:
-	case CGOSSIP_WAITING_FOR_USABLE:
-	case CGOSSIP_WAITING_FOR_SCID:
-	case CGOSSIP_WAITING_FOR_MATCHING_PEER_SIGS:
-	case CGOSSIP_WAITING_FOR_ANNOUNCE_DEPTH:
-		return NULL;
-	case CGOSSIP_CHANNEL_ANNOUNCED_DYING:
-	case CGOSSIP_CHANNEL_ANNOUNCED_DEAD:
-		return cg->cupdate;
-	case CGOSSIP_ANNOUNCED:
-		/* At this point we actually disable disconnected peers. */
-		if (update_channel_update(channel, channel_should_enable(channel, false))) {
-			broadcast_new_gossip(channel->peer->ld,
-					     cg->cupdate, NULL,
-					     "channel update");
-		}
-		check_channel_gossip(channel);
-		return cg->cupdate;
-	}
-	fatal("Bad channel_gossip_state %u", cg->state);
+	/* We MAY disable on loss of connectivity; re-enabled in
+	 * channel_gossip_channel_reestablished() (BOLT #7). */
+	set_channel_disabled(channel, true);
 }
 
 /* BOLT #7:

lightningd/channel_gossip.h

@@ -54,10 +54,6 @@ void channel_gossip_set_remote_update(struct lightningd *ld,
 				      const struct peer_update *update TAKES,
 				      const struct node_id *source);
 
-/* Get channel_update to send in an error onion reply (can give NULL!) */
-const u8 *channel_gossip_update_for_error(const tal_t *ctx,
-					  struct channel *channel);
-
 /* Get the peer's last-sent channel_update info, if any. */
 const struct peer_update *channel_gossip_get_remote_update(const struct channel *channel);
 

lightningd/peer_htlcs.c

@@ -568,10 +568,7 @@ static void destroy_hout_subd_died(struct htlc_out *hout)
 		  "Failing HTLC %"PRIu64" due to peer death",
 		  hout->key.id);
 
-	hout->failmsg = towire_temporary_channel_failure(hout,
-							 channel_update_for_error(tmpctx,
-										  hout->in,
-										  hout->key.channel));
+	hout->failmsg = towire_temporary_channel_failure(hout, NULL);
 
 	/* Assign a temporary state (we're about to free it!) so checks
 	 * are happy that it has a failure message */
@@ -618,11 +615,8 @@ static void rcvd_htlc_reply(struct subd *subd, const u8 *msg, const int *fds UNU
 		 * are expected to set the `channel_update` `len` field to
 		 * zero.
 		 */
-		/* We still append the channel_update (if we have one!)  FIXME: provide an option? */
-		if (fromwire_peektype(failmsg) & UPDATE) {
-			const u8 *update = channel_update_for_error(tmpctx, hout->in, hout->key.channel);
-			towire(&failmsg, update, tal_bytelen(update));
-		}
+		/* We do not include a channel_update: channeld builds these
+		 * with len=0, and we leave it that way (BOLT #1173). */
 		hout->failmsg = tal_steal(hout, failmsg);
 		if (hout->am_origin) {
 			char *localfail = tal_fmt(msg, "%s: %s",
@@ -725,8 +719,7 @@ const u8 *send_htlc_out(const tal_t *ctx,
 	if (!out->owner) {
 		log_info(out->log, "Attempt to send HTLC but unowned (%s)",
 			 channel_state_name(out));
-		return towire_temporary_channel_failure(ctx,
-							channel_update_for_error(tmpctx, in, out));
+		return towire_temporary_channel_failure(ctx, NULL);
 	}
 
 	/* Note: we allow outgoing HTLCs before sync, for fast startup. */
@@ -865,8 +858,7 @@ static void forward_htlc(struct htlc_in *hin,
 					 next->old_feerate_base,
 					 next->old_feerate_ppm)) {
 			failmsg = towire_fee_insufficient(tmpctx, hin->msat,
-							  channel_update_for_error(tmpctx,
-										   hin, next));
+							  NULL);
 			goto fail;
 		}
 		log_info(hin->key.channel->log,
@@ -880,7 +872,7 @@ static void forward_htlc(struct htlc_in *hin,
 		    || amount_msat_less(amt_to_forward, next->old_htlc_minimum_msat)
 		    || amount_msat_greater(amt_to_forward, next->old_htlc_maximum_msat)) {
 			failmsg = towire_temporary_channel_failure(tmpctx,
-								   channel_update_for_error(tmpctx, hin, next));
+								   NULL);
 			goto fail;
 		}
 		log_info(hin->key.channel->log,
@@ -890,7 +882,7 @@ static void forward_htlc(struct htlc_in *hin,
 	if (!check_cltv(hin, cltv_expiry, outgoing_cltv_value,
 			ld->config.cltv_expiry_delta)) {
 		failmsg = towire_incorrect_cltv_expiry(tmpctx, cltv_expiry,
-						       channel_update_for_error(tmpctx, hin, next));
+						       NULL);
 		goto fail;
 	}
 
@@ -908,8 +900,7 @@ static void forward_htlc(struct htlc_in *hin,
 			  "Expiry cltv %u too close to current %u",
 			  outgoing_cltv_value,
 			  get_block_height(ld->topology));
-		failmsg = towire_expiry_too_soon(tmpctx,
-						 channel_update_for_error(tmpctx, hin, next));
+		failmsg = towire_expiry_too_soon(tmpctx, NULL);
 		goto fail;
 	}
 

tests/test_gossip.py

@@ -99,6 +99,11 @@ def count_active(node):
         connected = len([p for p in node.rpc.listpeerchannels()['channels'] if p['peer_connected'] is True])
         return connected * len(active)
 
+    def our_dir_active(node):
+        # `active` of the update this node itself originated.
+        return [c['active'] for c in node.rpc.listchannels(scid)['channels']
+                if c['source'] == node.info['id']]
+
     l1.wait_channel_active(scid)
     l2.wait_channel_active(scid)
 
@@ -110,11 +115,16 @@ def count_active(node):
     wait_for(lambda: count_active(l1) == 0)
     assert(count_active(l2) == 0)
 
+    # We MAY disable our side while the peer is unreachable (see BOLT #7).
+    wait_for(lambda: our_dir_active(l1) == [False])
+
     # Now reconnect, they should re-enable the channels
     l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
 
     wait_for(lambda: count_active(l1) == 2)
     wait_for(lambda: count_active(l2) == 2)
+    wait_for(lambda: our_dir_active(l1) == [True])
+    wait_for(lambda: our_dir_active(l2) == [True])
 
 
 def test_announce_address(node_factory, bitcoind):

tests/test_pay.py

@@ -8,7 +8,8 @@
 from utils import (
     wait_for, only_one, sync_blockheight, TIMEOUT,
     mine_funding_to_announce, first_scid, serialize_payload_tlv, serialize_payload_final_tlv,
-    tu64_encode
+    tu64_encode, get_channel_update_hex, channel_direction,
+    fail_htlc_with_channel_update
 )
 import copy
 import os
@@ -283,8 +284,20 @@ def test_pay_disconnect(node_factory, bitcoind):
 
 
 def test_pay_error_update_fees(node_factory):
-    """We should process an update inside a temporary_channel_failure"""
-    l1, l2, l3 = node_factory.line_graph(3, fundchannel=True, wait_for_announce=True)
+    """We should process a channel_update inside an onion failure.
+
+    CLN no longer includes channel_update in failures (BOLT #1173), so
+    this simulates a peer that still does: l2 fails the first forward
+    with fee_insufficient carrying the updated channel_update.
+    """
+    upd = {'hex': None}
+
+    l1 = node_factory.get_node()
+    l2 = node_factory.get_node(
+        inline_plugin=fail_htlc_with_channel_update(lambda: upd['hex']))
+    l3 = node_factory.get_node()
+    node_factory.join_nodes([l1, l2, l3], fundchannel=True,
+                            wait_for_announce=True)
 
     # Don't include any routehints in first invoice.
     inv1 = l3.dev_invoice(amount_msat=123000,
@@ -295,9 +308,21 @@ def test_pay_error_update_fees(node_factory):
     inv2 = l3.rpc.invoice(123000, 'test_pay_error_update_fees2', 'desc')  # noqa: F841
 
     # Make sure l2 doesn't tell l1 directly that channel fee is changed.
+    scid = l2.get_channel_scid(l3)
     l2.rpc.dev_suppress_gossip()
     l2.rpc.setchannel(l3.info['id'], 1337, 137, enforcedelay=0)
 
+    # l2 still stores the new update in its gossip_store (addgossip happens
+    # even when peer broadcast is suppressed).  Fetch it for the plugin.
+    def fetch():
+        h = get_channel_update_hex(l2, scid, channel_direction(l2, l3),
+                                   fee_base=1337)
+        if h:
+            upd['hex'] = h
+        return h
+
+    wait_for(fetch)
+
     # Should bounce off and retry...
     ret = l1.rpc.pay(inv1['bolt11'])
     assert ret['parts'] == 2