Include both node_ids in BroadcastChannelUpdate

Sadly, the lightning gossip protocol operates by always flooding
peers with all the latest gossip they receive. For nodes with many
peers, this can result in lots of duplicative gossip as they
receive every message from every peer. As a results, some lightning
implementations disable gossip with new peers after some threshold.
This should mostly work as these peers expect to receive the latest
gossip from their many other peers.

However, in some cases an LDK node may wish to open public
channels but only has a single connection to the bulk of the  rest
of the network - with one such peer which requests that it not
receive any gossip. In that case, LDK would dutifully never send
any gossip to its only connection to the outside world. We would
still send gossip for channels with that peer as it would be sent
as unicast gossip, but if we then open another connection to
another peer which doesn't have any connection to the outside world
any information on that channel wouldn't propagate.

We've seen this setup on some LSPs, where they have a public node
and then an LSP which only connects through that public node, but
expects to open public channels to its LSP clients.

In a coming commit we'll start forwarding such gossip to all peers
by ignoring peer gossip limitations about all of our own channels.
While other gossip has the `node_id`s of both sides of the channel
explicitly in the message, `ChannelUpdate`s do not, so here we add
the `node_id`s to `BroadcastChannelUpdate` giving us the
information we need when we go to broadcast updates.

Author: TheBlueMatt

lightning/src/ln/channel_open_tests.rs

@@ -2337,7 +2337,7 @@ pub fn test_funding_and_commitment_tx_confirm_same_block() {
 		} else {
 			panic!();
 		}
-		if let MessageSendEvent::BroadcastChannelUpdate { ref msg } = msg_events.remove(0) {
+		if let MessageSendEvent::BroadcastChannelUpdate { ref msg, .. } = msg_events.remove(0) {
 			assert_eq!(msg.contents.channel_flags & 2, 2);
 		} else {
 			panic!();

lightning/src/ln/channelmanager.rs

@@ -111,6 +111,7 @@ use crate::onion_message::messenger::{
 	MessageRouter, MessageSendInstructions, Responder, ResponseInstruction,
 };
 use crate::onion_message::offers::{OffersMessage, OffersMessageHandler};
+use crate::routing::gossip::NodeId;
 use crate::routing::router::{
 	BlindedTail, FixedRouter, InFlightHtlcs, Path, Payee, PaymentParameters, Route,
 	RouteParameters, RouteParametersConfig, Router,
@@ -942,7 +943,7 @@ impl Into<LocalHTLCFailureReason> for FailureCode {
 struct MsgHandleErrInternal {
 	err: msgs::LightningError,
 	closes_channel: bool,
-	shutdown_finish: Option<(ShutdownResult, Option<msgs::ChannelUpdate>)>,
+	shutdown_finish: Option<(ShutdownResult, Option<(msgs::ChannelUpdate, NodeId, NodeId)>)>,
 	tx_abort: Option<msgs::TxAbort>,
 }
 impl MsgHandleErrInternal {
@@ -966,7 +967,7 @@ impl MsgHandleErrInternal {
 
 	fn from_finish_shutdown(
 		err: String, channel_id: ChannelId, shutdown_res: ShutdownResult,
-		channel_update: Option<msgs::ChannelUpdate>,
+		channel_update: Option<(msgs::ChannelUpdate, NodeId, NodeId)>,
 	) -> Self {
 		let err_msg = msgs::ErrorMessage { channel_id, data: err.clone() };
 		let action = if shutdown_res.monitor_update.is_some() {
@@ -3244,10 +3245,10 @@ macro_rules! handle_error {
 					log_error!(logger, "Closing channel: {}", err.err);
 
 					$self.finish_close_channel(shutdown_res);
-					if let Some(update) = update_option {
+					if let Some((update, node_id_1, node_id_2)) = update_option {
 						let mut pending_broadcast_messages = $self.pending_broadcast_messages.lock().unwrap();
 						pending_broadcast_messages.push(MessageSendEvent::BroadcastChannelUpdate {
-							msg: update
+							msg: update, node_id_1, node_id_2
 						});
 					}
 				} else {
@@ -3574,7 +3575,7 @@ macro_rules! handle_monitor_update_completion {
 			// channel_update later through the announcement_signatures process for public
 			// channels, but there's no reason not to just inform our counterparty of our fees
 			// now.
-			if let Ok(msg) = $self.get_channel_update_for_unicast($chan) {
+			if let Ok((msg, _, _)) = $self.get_channel_update_for_unicast($chan) {
 				Some(MessageSendEvent::SendChannelUpdate {
 					node_id: counterparty_node_id,
 					msg,
@@ -5125,7 +5126,9 @@ where
 		}
 	}
 
-	/// Gets the current [`channel_update`] for the given channel. This first checks if the channel is
+	/// Gets the current [`channel_update`] for the given channel (as well as our and our
+	/// counterparty's [`NodeId`], which is needed for the
+	/// [`MessageSendEvent::BroadcastChannelUpdate`]). This first checks if the channel is
 	/// public, and thus should be called whenever the result is going to be passed out in a
 	/// [`MessageSendEvent::BroadcastChannelUpdate`] event.
 	///
@@ -5137,7 +5140,7 @@ where
 	/// [`internal_closing_signed`]: Self::internal_closing_signed
 	fn get_channel_update_for_broadcast(
 		&self, chan: &FundedChannel<SP>,
-	) -> Result<msgs::ChannelUpdate, LightningError> {
+	) -> Result<(msgs::ChannelUpdate, NodeId, NodeId), LightningError> {
 		if !chan.context.should_announce() {
 			return Err(LightningError {
 				err: "Cannot broadcast a channel_update for a private channel".to_owned(),
@@ -5159,10 +5162,11 @@ where
 		self.get_channel_update_for_unicast(chan)
 	}
 
-	/// Gets the current [`channel_update`] for the given channel. This does not check if the channel
-	/// is public (only returning an `Err` if the channel does not yet have an assigned SCID),
-	/// and thus MUST NOT be called unless the recipient of the resulting message has already
-	/// provided evidence that they know about the existence of the channel.
+	/// Gets the current [`channel_update`] for the given channel (as well as our and our
+	/// counterparty's [`NodeId`]). This does not check if the channel is public (only returning an
+	/// `Err` if the channel does not yet have an assigned SCID), and thus MUST NOT be called
+	/// unless the recipient of the resulting message has already provided evidence that they know
+	/// about the existence of the channel.
 	///
 	/// Note that through [`internal_closing_signed`], this function is called without the
 	/// `peer_state`  corresponding to the channel's counterparty locked, as the channel been
@@ -5171,7 +5175,9 @@ where
 	/// [`channel_update`]: msgs::ChannelUpdate
 	/// [`internal_closing_signed`]: Self::internal_closing_signed
 	#[rustfmt::skip]
-	fn get_channel_update_for_unicast(&self, chan: &FundedChannel<SP>) -> Result<msgs::ChannelUpdate, LightningError> {
+	fn get_channel_update_for_unicast(
+		&self, chan: &FundedChannel<SP>,
+	) -> Result<(msgs::ChannelUpdate, NodeId, NodeId), LightningError> {
 		let logger = WithChannelContext::from(&self.logger, &chan.context, None);
 		log_trace!(logger, "Attempting to generate channel update for channel {}", chan.context.channel_id());
 		let short_channel_id = match chan.funding.get_short_channel_id().or(chan.context.latest_inbound_scid_alias()) {
@@ -5181,7 +5187,9 @@ where
 
 		let logger = WithChannelContext::from(&self.logger, &chan.context, None);
 		log_trace!(logger, "Generating channel update for channel {}", chan.context.channel_id());
-		let were_node_one = self.our_network_pubkey.serialize()[..] < chan.context.get_counterparty_node_id().serialize()[..];
+		let our_node_id = NodeId::from_pubkey(&self.our_network_pubkey);
+		let their_node_id = NodeId::from_pubkey(&chan.context.get_counterparty_node_id());
+		let were_node_one = our_node_id < their_node_id;
 		let enabled = chan.context.is_enabled();
 
 		let unsigned = msgs::UnsignedChannelUpdate {
@@ -5203,10 +5211,14 @@ where
 		// channel.
 		let sig = self.node_signer.sign_gossip_message(msgs::UnsignedGossipMessage::ChannelUpdate(&unsigned)).unwrap();
 
-		Ok(msgs::ChannelUpdate {
-			signature: sig,
-			contents: unsigned
-		})
+		Ok((
+			msgs::ChannelUpdate {
+				signature: sig,
+				contents: unsigned
+			},
+			if were_node_one { our_node_id } else { their_node_id },
+			if were_node_one { their_node_id } else { our_node_id },
+		))
 	}
 
 	#[cfg(any(test, feature = "_externalize_tests"))]
@@ -6649,11 +6661,11 @@ where
 					continue;
 				}
 				if let Some(channel) = channel.as_funded() {
-					if let Ok(msg) = self.get_channel_update_for_broadcast(channel) {
+					if let Ok((msg, node_id_1, node_id_2)) = self.get_channel_update_for_broadcast(channel) {
 						let mut pending_broadcast_messages = self.pending_broadcast_messages.lock().unwrap();
-						pending_broadcast_messages.push(MessageSendEvent::BroadcastChannelUpdate { msg });
+						pending_broadcast_messages.push(MessageSendEvent::BroadcastChannelUpdate { msg, node_id_1, node_id_2 });
 					} else if peer_state.is_connected {
-						if let Ok(msg) = self.get_channel_update_for_unicast(channel) {
+						if let Ok((msg, _, _)) = self.get_channel_update_for_unicast(channel) {
 							peer_state.pending_msg_events.push(MessageSendEvent::SendChannelUpdate {
 								node_id: channel.context.get_counterparty_node_id(),
 								msg,
@@ -8177,10 +8189,10 @@ where
 										n += 1;
 										if n >= DISABLE_GOSSIP_TICKS {
 											funded_chan.set_channel_update_status(ChannelUpdateStatus::Disabled);
-											if let Ok(update) = self.get_channel_update_for_broadcast(&funded_chan) {
+											if let Ok((update, node_id_1, node_id_2)) = self.get_channel_update_for_broadcast(&funded_chan) {
 												let mut pending_broadcast_messages = self.pending_broadcast_messages.lock().unwrap();
 												pending_broadcast_messages.push(MessageSendEvent::BroadcastChannelUpdate {
-													msg: update
+													msg: update, node_id_1, node_id_2
 												});
 											}
 											should_persist = NotifyOption::DoPersist;
@@ -8192,10 +8204,10 @@ where
 										n += 1;
 										if n >= ENABLE_GOSSIP_TICKS {
 											funded_chan.set_channel_update_status(ChannelUpdateStatus::Enabled);
-											if let Ok(update) = self.get_channel_update_for_broadcast(&funded_chan) {
+											if let Ok((update, node_id_1, node_id_2)) = self.get_channel_update_for_broadcast(&funded_chan) {
 												let mut pending_broadcast_messages = self.pending_broadcast_messages.lock().unwrap();
 												pending_broadcast_messages.push(MessageSendEvent::BroadcastChannelUpdate {
-													msg: update
+													msg: update, node_id_1, node_id_2
 												});
 											}
 											should_persist = NotifyOption::DoPersist;
@@ -10821,7 +10833,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 						// channel_update here if the channel is not public, i.e. we're not sending an
 						// announcement_signatures.
 						log_trace!(logger, "Sending private initial channel_update for our counterparty on channel {}", chan.context.channel_id());
-						if let Ok(msg) = self.get_channel_update_for_unicast(chan) {
+						if let Ok((msg, _, _)) = self.get_channel_update_for_unicast(chan) {
 							peer_state.pending_msg_events.push(MessageSendEvent::SendChannelUpdate {
 								node_id: counterparty_node_id.clone(),
 								msg,
@@ -11620,7 +11632,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 						msg: try_channel_entry!(self, peer_state, res, chan_entry),
 						// Note that announcement_signatures fails if the channel cannot be announced,
 						// so get_channel_update_for_broadcast will never fail by the time we get here.
-						update_msg: Some(self.get_channel_update_for_broadcast(chan).unwrap()),
+						update_msg: Some(self.get_channel_update_for_broadcast(chan).unwrap().0),
 					});
 				} else {
 					return try_channel_entry!(self, peer_state, Err(ChannelError::close(
@@ -11729,7 +11741,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 							// If the channel is in a usable state (ie the channel is not being shut
 							// down), send a unicast channel_update to our counterparty to make sure
 							// they have the latest channel parameters.
-							if let Ok(msg) = self.get_channel_update_for_unicast(chan) {
+							if let Ok((msg, _, _)) = self.get_channel_update_for_unicast(chan) {
 								channel_update = Some(MessageSendEvent::SendChannelUpdate {
 									node_id: chan.context.get_counterparty_node_id(),
 									msg,
@@ -14340,7 +14352,7 @@ where
 										send_channel_ready!(self, pending_msg_events, funded_channel, channel_ready);
 										if funded_channel.context.is_usable() && peer_state.is_connected {
 											log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", channel_id);
-											if let Ok(msg) = self.get_channel_update_for_unicast(funded_channel) {
+											if let Ok((msg, _, _)) = self.get_channel_update_for_unicast(funded_channel) {
 												pending_msg_events.push(MessageSendEvent::SendChannelUpdate {
 													node_id: funded_channel.context.get_counterparty_node_id(),
 													msg,
@@ -14433,7 +14445,7 @@ where
 												// if the channel cannot be announced, so
 												// get_channel_update_for_broadcast will never fail
 												// by the time we get here.
-												update_msg: Some(self.get_channel_update_for_broadcast(funded_channel).unwrap()),
+												update_msg: Some(self.get_channel_update_for_broadcast(funded_channel).unwrap().0),
 											});
 										}
 									}

lightning/src/ln/functional_test_utils.rs

@@ -2182,7 +2182,7 @@ macro_rules! get_closing_signed_broadcast {
 		assert!(events.len() == 1 || events.len() == 2);
 		(
 			match events[events.len() - 1] {
-				MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
+				MessageSendEvent::BroadcastChannelUpdate { ref msg, .. } => {
 					assert_eq!(msg.contents.channel_flags & 2, 2);
 					msg.clone()
 				},
@@ -2253,7 +2253,7 @@ pub fn check_closed_broadcast(
 		.into_iter()
 		.filter_map(|msg_event| {
 			match msg_event {
-				MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
+				MessageSendEvent::BroadcastChannelUpdate { ref msg, .. } => {
 					assert_eq!(msg.contents.channel_flags & 2, 2);
 					None
 				},
@@ -4875,7 +4875,7 @@ pub fn handle_announce_close_broadcast_events<'a, 'b, 'c>(
 	let events_1 = nodes[a].node.get_and_clear_pending_msg_events();
 	assert_eq!(events_1.len(), 2);
 	let as_update = match events_1[1] {
-		MessageSendEvent::BroadcastChannelUpdate { ref msg } => msg.clone(),
+		MessageSendEvent::BroadcastChannelUpdate { ref msg, .. } => msg.clone(),
 		_ => panic!("Unexpected event"),
 	};
 	match events_1[0] {
@@ -4912,7 +4912,7 @@ pub fn handle_announce_close_broadcast_events<'a, 'b, 'c>(
 	let events_2 = nodes[b].node.get_and_clear_pending_msg_events();
 	assert_eq!(events_2.len(), if needs_err_handle { 1 } else { 2 });
 	let bs_update = match events_2.last().unwrap() {
-		MessageSendEvent::BroadcastChannelUpdate { ref msg } => msg.clone(),
+		MessageSendEvent::BroadcastChannelUpdate { ref msg, .. } => msg.clone(),
 		_ => panic!("Unexpected event"),
 	};
 	if !needs_err_handle {

lightning/src/ln/functional_tests.rs

@@ -717,7 +717,7 @@ pub fn channel_monitor_network_test() {
 		let events = nodes[3].node.get_and_clear_pending_msg_events();
 		assert_eq!(events.len(), 2);
 		let close_chan_update_1 = match events[1] {
-			MessageSendEvent::BroadcastChannelUpdate { ref msg } => msg.clone(),
+			MessageSendEvent::BroadcastChannelUpdate { ref msg, .. } => msg.clone(),
 			_ => panic!("Unexpected event"),
 		};
 		match events[0] {
@@ -752,7 +752,7 @@ pub fn channel_monitor_network_test() {
 		let events = nodes[4].node.get_and_clear_pending_msg_events();
 		assert_eq!(events.len(), 2);
 		let close_chan_update_2 = match events[1] {
-			MessageSendEvent::BroadcastChannelUpdate { ref msg } => msg.clone(),
+			MessageSendEvent::BroadcastChannelUpdate { ref msg, .. } => msg.clone(),
 			_ => panic!("Unexpected event"),
 		};
 		match events[0] {
@@ -2167,7 +2167,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(
 
 	// Ensure that the last remaining message event is the BroadcastChannelUpdate msg for chan_2
 	match events[0] {
-		MessageSendEvent::BroadcastChannelUpdate { msg: msgs::ChannelUpdate { .. } } => {},
+		MessageSendEvent::BroadcastChannelUpdate { msg: msgs::ChannelUpdate { .. }, .. } => {},
 		_ => panic!("Unexpected event"),
 	}
 
@@ -6026,7 +6026,7 @@ pub fn test_announce_disable_channels() {
 	let mut chans_disabled = new_hash_map();
 	for e in msg_events {
 		match e {
-			MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
+			MessageSendEvent::BroadcastChannelUpdate { ref msg, .. } => {
 				assert_eq!(msg.contents.channel_flags & (1 << 1), 1 << 1); // The "channel disabled" bit should be set
 														   // Check that each channel gets updated exactly once
 				if chans_disabled
@@ -6077,7 +6077,7 @@ pub fn test_announce_disable_channels() {
 	assert_eq!(msg_events.len(), 3);
 	for e in msg_events {
 		match e {
-			MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
+			MessageSendEvent::BroadcastChannelUpdate { ref msg, .. } => {
 				assert_eq!(msg.contents.channel_flags & (1 << 1), 0); // The "channel disabled" bit should be off
 				match chans_disabled.remove(&msg.contents.short_channel_id) {
 					// Each update should have a higher timestamp than the previous one, replacing
@@ -7995,13 +7995,13 @@ pub fn test_error_chans_closed() {
 	let events = nodes[0].node.get_and_clear_pending_msg_events();
 	assert_eq!(events.len(), 2);
 	match events[0] {
-		MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
+		MessageSendEvent::BroadcastChannelUpdate { ref msg, .. } => {
 			assert_eq!(msg.contents.channel_flags & 2, 2);
 		},
 		_ => panic!("Unexpected event"),
 	}
 	match events[1] {
-		MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
+		MessageSendEvent::BroadcastChannelUpdate { ref msg, .. } => {
 			assert_eq!(msg.contents.channel_flags & 2, 2);
 		},
 		_ => panic!("Unexpected event"),

lightning/src/ln/msgs.rs

@@ -1917,6 +1917,16 @@ pub enum MessageSendEvent {
 	BroadcastChannelUpdate {
 		/// The channel_update which should be sent.
 		msg: ChannelUpdate,
+		/// The node_id of the first endpoint of the channel.
+		///
+		/// This is not used in the message broadcast, but rather is useful for deciding which
+		/// peer(s) to send the update to.
+		node_id_1: NodeId,
+		/// The node_id of the second endpoint of the channel.
+		///
+		/// This is not used in the message broadcast, but rather is useful for deciding which
+		/// peer(s) to send the update to.
+		node_id_2: NodeId,
 	},
 	/// Used to indicate that a node_announcement should be broadcast to all peers.
 	BroadcastNodeAnnouncement {

lightning/src/ln/onion_route_tests.rs

@@ -1662,7 +1662,7 @@ fn do_test_onion_failure_stale_channel_update(announce_for_forwarding: bool) {
 			return None;
 		}
 		let new_update = match &events[0] {
-			MessageSendEvent::BroadcastChannelUpdate { msg } => {
+			MessageSendEvent::BroadcastChannelUpdate { msg, .. } => {
 				assert!(announce_for_forwarding);
 				msg.clone()
 			},

lightning/src/ln/peer_handler.rs

@@ -3164,7 +3164,7 @@ where
 								}
 							}
 						},
-						MessageSendEvent::BroadcastChannelUpdate { msg } => {
+						MessageSendEvent::BroadcastChannelUpdate { msg, .. } => {
 							log_debug!(self.logger, "Handling BroadcastChannelUpdate event in peer_handler for contents {:?}", msg.contents);
 							match route_handler.handle_channel_update(None, &msg) {
 								Ok(_)
@@ -4409,8 +4409,6 @@ mod tests {
 
 	#[test]
 	fn test_forward_while_syncing() {
-		use crate::ln::peer_handler::tests::test_utils::get_dummy_channel_update;
-
 		// Test forwarding new channel announcements while we're doing syncing.
 		let cfgs = create_peermgr_cfgs(2);
 		cfgs[0].routing_handler.request_full_sync.store(true, Ordering::Release);
@@ -4457,11 +4455,19 @@ mod tests {
 		// At this point we should have sent channel announcements up to roughly SCID 150. Now
 		// build an updated update for SCID 100 and SCID 5000 and make sure only the one for SCID
 		// 100 gets forwarded
-		let msg_100 = get_dummy_channel_update(100);
-		let msg_ev_100 = MessageSendEvent::BroadcastChannelUpdate { msg: msg_100.clone() };
+		let msg_100 = test_utils::get_dummy_channel_update(100);
+		let msg_ev_100 = MessageSendEvent::BroadcastChannelUpdate {
+			msg: msg_100.clone(),
+			node_id_1: NodeId::from_slice(&[2; 33]).unwrap(),
+			node_id_2: NodeId::from_slice(&[3; 33]).unwrap(),
+		};
 
-		let msg_5000 = get_dummy_channel_update(5000);
-		let msg_ev_5000 = MessageSendEvent::BroadcastChannelUpdate { msg: msg_5000 };
+		let msg_5000 = test_utils::get_dummy_channel_update(5000);
+		let msg_ev_5000 = MessageSendEvent::BroadcastChannelUpdate {
+			msg: msg_5000,
+			node_id_1: NodeId::from_slice(&[2; 33]).unwrap(),
+			node_id_2: NodeId::from_slice(&[3; 33]).unwrap(),
+		};
 
 		fd_a.hang_writes.store(true, Ordering::Relaxed);
 

lightning/src/ln/shutdown_tests.rs

@@ -1402,7 +1402,7 @@ fn do_test_closing_signed_reinit_timeout(timeout_step: TimeoutStep) {
 		let events = nodes[1].node.get_and_clear_pending_msg_events();
 		assert_eq!(events.len(), 1);
 		match events[0] {
-			MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
+			MessageSendEvent::BroadcastChannelUpdate { ref msg, .. } => {
 				assert_eq!(msg.contents.channel_flags & 2, 2);
 			},
 			_ => panic!("Unexpected event"),