Send channel_update messages to direct peers on private channels

If we are a public node and have a private channel, our
counterparty needs to know the fees which we will charge to forward
payments to them. Without sending them a channel_update, they have
no way to learn that information, resulting in the channel being
effectively useless for outbound-from-us payments.

This commit fixes our lack of channel_update messages to private
channel counterparties, ensuring we always send them a
channel_update after the channel funding is confirmed.

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -571,7 +571,12 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 						events::MessageSendEvent::SendFundingLocked { .. } => continue,
 						events::MessageSendEvent::SendAnnouncementSignatures { .. } => continue,
 						events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => continue,
-						_ => panic!("Unhandled message event"),
+						events::MessageSendEvent::SendChannelUpdate { ref node_id, ref msg } => {
+							assert_eq!(msg.contents.flags & 2, 0); // The disable bit must never be set!
+							if Some(*node_id) == expect_drop_id { panic!("peer_disconnected should drop msgs bound for the disconnected peer"); }
+							*node_id == a_id
+						},
+						_ => panic!("Unhandled message event {:?}", event),
 					};
 					if push_a { ba_events.push(event); } else { bc_events.push(event); }
 				}
@@ -692,7 +697,16 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 							// Can be generated due to a payment forward being rejected due to a
 							// channel having previously failed a monitor update
 						},
-						_ => panic!("Unhandled message event"),
+						events::MessageSendEvent::SendChannelUpdate { ref msg, .. } => {
+							// When we reconnect we will resend a channel_update to make sure our
+							// counterparty has the latest parameters for receiving payments
+							// through us. We do, however, check that the message does not include
+							// the "disabled" bit, as we should never ever have a channel which is
+							// disabled when we send such an update (or it may indicate channel
+							// force-close which we should detect as an error).
+							assert_eq!(msg.contents.flags & 2, 0);
+						},
+						_ => panic!("Unhandled message event {:?}", event),
 					}
 					if $limit_events != ProcessMessages::AllMessages {
 						break;
@@ -722,6 +736,9 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 							events::MessageSendEvent::SendFundingLocked { .. } => {},
 							events::MessageSendEvent::SendAnnouncementSignatures { .. } => {},
 							events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => {},
+							events::MessageSendEvent::SendChannelUpdate { ref msg, .. } => {
+								assert_eq!(msg.contents.flags & 2, 0); // The disable bit must never be set!
+							},
 							_ => panic!("Unhandled message event"),
 						}
 					}
@@ -737,6 +754,9 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 							events::MessageSendEvent::SendFundingLocked { .. } => {},
 							events::MessageSendEvent::SendAnnouncementSignatures { .. } => {},
 							events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => {},
+							events::MessageSendEvent::SendChannelUpdate { ref msg, .. } => {
+								assert_eq!(msg.contents.flags & 2, 0); // The disable bit must never be set!
+							},
 							_ => panic!("Unhandled message event"),
 						}
 					}

lightning-background-processor/src/lib.rs

@@ -443,9 +443,13 @@ mod tests {
 
 		// Confirm the funding transaction.
 		confirm_transaction(&mut nodes[0], &funding_tx);
+		let as_funding = get_event_msg!(nodes[0], MessageSendEvent::SendFundingLocked, nodes[1].node.get_our_node_id());
 		confirm_transaction(&mut nodes[1], &funding_tx);
-		nodes[0].node.handle_funding_locked(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingLocked, nodes[0].node.get_our_node_id()));
-		nodes[1].node.handle_funding_locked(&nodes[0].node.get_our_node_id(), &get_event_msg!(nodes[0], MessageSendEvent::SendFundingLocked, nodes[1].node.get_our_node_id()));
+		let bs_funding = get_event_msg!(nodes[1], MessageSendEvent::SendFundingLocked, nodes[0].node.get_our_node_id());
+		nodes[0].node.handle_funding_locked(&nodes[1].node.get_our_node_id(), &bs_funding);
+		let _as_channel_update = get_event_msg!(nodes[0], MessageSendEvent::SendChannelUpdate, nodes[1].node.get_our_node_id());
+		nodes[1].node.handle_funding_locked(&nodes[0].node.get_our_node_id(), &as_funding);
+		let _bs_channel_update = get_event_msg!(nodes[1], MessageSendEvent::SendChannelUpdate, nodes[0].node.get_our_node_id());
 
 		assert!(bg_processor.stop().is_ok());
 

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -1158,7 +1158,10 @@ fn test_monitor_update_fail_reestablish() {
 	nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &bs_reestablish);
 
 	nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &as_reestablish);
-	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+	assert_eq!(
+		get_event_msg!(nodes[0], MessageSendEvent::SendChannelUpdate, nodes[1].node.get_our_node_id())
+			.contents.flags & 2, 0); // The "disabled" bit should be unset as we just reconnected
+
 	nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
 	check_added_monitors!(nodes[1], 1);
 
@@ -1172,10 +1175,15 @@ fn test_monitor_update_fail_reestablish() {
 	assert!(bs_reestablish == get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id()));
 
 	nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &bs_reestablish);
+	assert_eq!(
+		get_event_msg!(nodes[0], MessageSendEvent::SendChannelUpdate, nodes[1].node.get_our_node_id())
+			.contents.flags & 2, 0); // The "disabled" bit should be unset as we just reconnected
 
 	nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &as_reestablish);
 	check_added_monitors!(nodes[1], 0);
-	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+	assert_eq!(
+		get_event_msg!(nodes[1], MessageSendEvent::SendChannelUpdate, nodes[0].node.get_our_node_id())
+			.contents.flags & 2, 0); // The "disabled" bit should be unset as we just reconnected
 
 	*nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Ok(()));
 	let (outpoint, latest_update) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_1.2).unwrap().clone();
@@ -1352,14 +1360,14 @@ fn claim_while_disconnected_monitor_update_fail() {
 	let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
 
 	nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &bs_reconnect);
-	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+	let _as_channel_update = get_event_msg!(nodes[0], MessageSendEvent::SendChannelUpdate, nodes[1].node.get_our_node_id());
 
 	// Now deliver a's reestablish, freeing the claim from the holding cell, but fail the monitor
 	// update.
 	*nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::TemporaryFailure));
 
 	nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &as_reconnect);
-	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+	let _bs_channel_update = get_event_msg!(nodes[1], MessageSendEvent::SendChannelUpdate, nodes[0].node.get_our_node_id());
 	nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
 	check_added_monitors!(nodes[1], 1);
 	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
@@ -1492,7 +1500,9 @@ fn monitor_failed_no_reestablish_response() {
 	let bs_reconnect = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
 
 	nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &as_reconnect);
+	let _bs_channel_update = get_event_msg!(nodes[1], MessageSendEvent::SendChannelUpdate, nodes[0].node.get_our_node_id());
 	nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &bs_reconnect);
+	let _as_channel_update = get_event_msg!(nodes[0], MessageSendEvent::SendChannelUpdate, nodes[1].node.get_our_node_id());
 
 	*nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Ok(()));
 	let (outpoint, latest_update) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();

lightning/src/ln/channelmanager.rs

@@ -1598,6 +1598,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				action: msgs::ErrorAction::IgnoreError
 			});
 		}
+		log_trace!(self.logger, "Attempting to generate broadcast channel update for channel {}", log_bytes!(chan.channel_id()));
 		self.get_channel_update_for_unicast(chan)
 	}
 
@@ -1607,6 +1608,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	/// provided evidence that they know about the existence of the channel.
 	/// May be called with channel_state already locked!
 	fn get_channel_update_for_unicast(&self, chan: &Channel<Signer>) -> Result<msgs::ChannelUpdate, LightningError> {
+		log_trace!(self.logger, "Attempting to generate channel update for channel {}", log_bytes!(chan.channel_id()));
 		let short_channel_id = match chan.get_short_channel_id() {
 			None => return Err(LightningError{err: "Channel not yet established".to_owned(), action: msgs::ErrorAction::IgnoreError}),
 			Some(id) => id,
@@ -2789,7 +2791,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	pub fn channel_monitor_updated(&self, funding_txo: &OutPoint, highest_applied_update_id: u64) {
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
 
-		let (mut pending_failures, chan_restoration_res) = {
+		let chan_restoration_res;
+		let mut pending_failures = {
 			let mut channel_lock = self.channel_state.lock().unwrap();
 			let channel_state = &mut *channel_lock;
 			let mut channel = match channel_state.by_id.entry(funding_txo.to_channel_id()) {
@@ -2801,7 +2804,21 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			}
 
 			let (raa, commitment_update, order, pending_forwards, pending_failures, funding_broadcastable, funding_locked) = channel.get_mut().monitor_updating_restored(&self.logger);
-			(pending_failures, handle_chan_restoration_locked!(self, channel_lock, channel_state, channel, raa, commitment_update, order, None, pending_forwards, funding_broadcastable, funding_locked))
+			let channel_update = if funding_locked.is_some() && channel.get().is_usable() && !channel.get().should_announce() {
+				// We only send a channel_update in the case where we are just now sending a
+				// funding_locked and the channel is in a usable state. Further, we rely on the
+				// normal announcement_signatures process to send a channel_update for public
+				// channels, only generating a unicast channel_update if this is a private channel.
+				Some(events::MessageSendEvent::SendChannelUpdate {
+					node_id: channel.get().get_counterparty_node_id(),
+					msg: self.get_channel_update_for_unicast(channel.get()).unwrap(),
+				})
+			} else { None };
+			chan_restoration_res = handle_chan_restoration_locked!(self, channel_lock, channel_state, channel, raa, commitment_update, order, None, pending_forwards, funding_broadcastable, funding_locked);
+			if let Some(upd) = channel_update {
+				channel_state.pending_msg_events.push(upd);
+			}
+			pending_failures
 		};
 		post_handle_chan_restoration!(self, chan_restoration_res);
 		for failure in pending_failures.drain(..) {
@@ -2964,6 +2981,11 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 						node_id: counterparty_node_id.clone(),
 						msg: announcement_sigs,
 					});
+				} else if chan.get().is_usable() {
+					channel_state.pending_msg_events.push(events::MessageSendEvent::SendChannelUpdate {
+						node_id: counterparty_node_id.clone(),
+						msg: self.get_channel_update_for_unicast(chan.get()).unwrap(),
+					});
 				}
 				Ok(())
 			},
@@ -3394,7 +3416,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	}
 
 	fn internal_channel_reestablish(&self, counterparty_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(), MsgHandleErrInternal> {
-		let (htlcs_failed_forward, need_lnd_workaround, chan_restoration_res) = {
+		let chan_restoration_res;
+		let (htlcs_failed_forward, need_lnd_workaround) = {
 			let mut channel_state_lock = self.channel_state.lock().unwrap();
 			let channel_state = &mut *channel_state_lock;
 
@@ -3409,15 +3432,27 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					// add-HTLCs on disconnect, we may be handed HTLCs to fail backwards here.
 					let (funding_locked, revoke_and_ack, commitment_update, monitor_update_opt, order, htlcs_failed_forward, shutdown) =
 						try_chan_entry!(self, chan.get_mut().channel_reestablish(msg, &self.logger), channel_state, chan);
+					let mut channel_update = None;
 					if let Some(msg) = shutdown {
 						channel_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
 							node_id: counterparty_node_id.clone(),
 							msg,
 						});
+					} else if chan.get().is_usable() {
+						// 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.
+						channel_update = Some(events::MessageSendEvent::SendChannelUpdate {
+							node_id: chan.get().get_counterparty_node_id(),
+							msg: self.get_channel_update_for_unicast(chan.get()).unwrap(),
+						});
 					}
 					let need_lnd_workaround = chan.get_mut().workaround_lnd_bug_4006.take();
-					(htlcs_failed_forward, need_lnd_workaround,
-						handle_chan_restoration_locked!(self, channel_state_lock, channel_state, chan, revoke_and_ack, commitment_update, order, monitor_update_opt, Vec::new(), None, funding_locked))
+					chan_restoration_res = handle_chan_restoration_locked!(self, channel_state_lock, channel_state, chan, revoke_and_ack, commitment_update, order, monitor_update_opt, Vec::new(), None, funding_locked);
+					if let Some(upd) = channel_update {
+						channel_state.pending_msg_events.push(upd);
+					}
+					(htlcs_failed_forward, need_lnd_workaround)
 				},
 				hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
 			}
@@ -3970,6 +4005,12 @@ where
 								node_id: channel.get_counterparty_node_id(),
 								msg: announcement_sigs,
 							});
+						} else if channel.is_usable() {
+							log_trace!(self.logger, "Sending funding_locked WITHOUT announcement_signatures but with private channel_update for our counterparty on channel {}", log_bytes!(channel.channel_id()));
+							pending_msg_events.push(events::MessageSendEvent::SendChannelUpdate {
+								node_id: channel.get_counterparty_node_id(),
+								msg: self.get_channel_update_for_unicast(channel).unwrap(),
+							});
 						} else {
 							log_trace!(self.logger, "Sending funding_locked WITHOUT announcement_signatures for {}", log_bytes!(channel.channel_id()));
 						}
@@ -4209,6 +4250,7 @@ impl<Signer: Sign, M: Deref , T: Deref , K: Deref , F: Deref , L: Deref >
 					&events::MessageSendEvent::BroadcastChannelAnnouncement { .. } => true,
 					&events::MessageSendEvent::BroadcastNodeAnnouncement { .. } => true,
 					&events::MessageSendEvent::BroadcastChannelUpdate { .. } => true,
+					&events::MessageSendEvent::SendChannelUpdate { ref node_id, .. } => node_id != counterparty_node_id,
 					&events::MessageSendEvent::HandleError { ref node_id, .. } => node_id != counterparty_node_id,
 					&events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => true,
 					&events::MessageSendEvent::SendChannelRangeQuery { .. } => false,
@@ -5042,7 +5084,19 @@ pub mod bench {
 		Listen::block_connected(&node_b, &block, 1);
 
 		node_a.handle_funding_locked(&node_b.get_our_node_id(), &get_event_msg!(node_b_holder, MessageSendEvent::SendFundingLocked, node_a.get_our_node_id()));
-		node_b.handle_funding_locked(&node_a.get_our_node_id(), &get_event_msg!(node_a_holder, MessageSendEvent::SendFundingLocked, node_b.get_our_node_id()));
+		let msg_events = node_a.get_and_clear_pending_msg_events();
+		assert_eq!(msg_events.len(), 2);
+		match msg_events[0] {
+			MessageSendEvent::SendFundingLocked { ref msg, .. } => {
+				node_b.handle_funding_locked(&node_a.get_our_node_id(), msg);
+				get_event_msg!(node_b_holder, MessageSendEvent::SendChannelUpdate, node_a.get_our_node_id());
+			},
+			_ => panic!(),
+		}
+		match msg_events[1] {
+			MessageSendEvent::SendChannelUpdate { .. } => {},
+			_ => panic!(),
+		}
 
 		let dummy_graph = NetworkGraph::new(genesis_hash);
 

lightning/src/ln/functional_test_utils.rs

@@ -1583,18 +1583,20 @@ macro_rules! handle_chan_reestablish_msgs {
 			let mut revoke_and_ack = None;
 			let mut commitment_update = None;
 			let order = if let Some(ev) = msg_events.get(idx) {
-				idx += 1;
 				match ev {
 					&MessageSendEvent::SendRevokeAndACK { ref node_id, ref msg } => {
 						assert_eq!(*node_id, $dst_node.node.get_our_node_id());
 						revoke_and_ack = Some(msg.clone());
+						idx += 1;
 						RAACommitmentOrder::RevokeAndACKFirst
 					},
 					&MessageSendEvent::UpdateHTLCs { ref node_id, ref updates } => {
 						assert_eq!(*node_id, $dst_node.node.get_our_node_id());
 						commitment_update = Some(updates.clone());
+						idx += 1;
 						RAACommitmentOrder::CommitmentFirst
 					},
+					&MessageSendEvent::SendChannelUpdate { .. } => RAACommitmentOrder::CommitmentFirst,
 					_ => panic!("Unexpected event"),
 				}
 			} else {
@@ -1607,16 +1609,24 @@ macro_rules! handle_chan_reestablish_msgs {
 						assert_eq!(*node_id, $dst_node.node.get_our_node_id());
 						assert!(revoke_and_ack.is_none());
 						revoke_and_ack = Some(msg.clone());
+						idx += 1;
 					},
 					&MessageSendEvent::UpdateHTLCs { ref node_id, ref updates } => {
 						assert_eq!(*node_id, $dst_node.node.get_our_node_id());
 						assert!(commitment_update.is_none());
 						commitment_update = Some(updates.clone());
+						idx += 1;
 					},
+					&MessageSendEvent::SendChannelUpdate { .. } => {},
 					_ => panic!("Unexpected event"),
 				}
 			}
 
+			if let Some(&MessageSendEvent::SendChannelUpdate { ref node_id, ref msg }) = msg_events.get(idx) {
+				assert_eq!(*node_id, $dst_node.node.get_our_node_id());
+				assert_eq!(msg.contents.flags & 2, 0); // "disabled" flag must not be set as we just reconnected.
+			}
+
 			(funding_locked, revoke_and_ack, commitment_update, order)
 		}
 	}

lightning/src/ln/functional_tests.rs

@@ -1039,7 +1039,8 @@ fn do_test_shutdown_rebroadcast(recv_count: u8) {
 		nodes[0].node.handle_shutdown(&nodes[1].node.get_our_node_id(), &InitFeatures::known(), &node_1_2nd_shutdown);
 		node_0_2nd_shutdown
 	} else {
-		assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+		let node_0_chan_update = get_event_msg!(nodes[0], MessageSendEvent::SendChannelUpdate, nodes[1].node.get_our_node_id());
+		assert_eq!(node_0_chan_update.contents.flags & 2, 0); // "disabled" flag must not be set as we just reconnected.
 		nodes[0].node.handle_shutdown(&nodes[1].node.get_our_node_id(), &InitFeatures::known(), &node_1_2nd_shutdown);
 		get_event_msg!(nodes[0], MessageSendEvent::SendShutdown, nodes[1].node.get_our_node_id())
 	};

lightning/src/ln/peer_handler.rs

@@ -1264,6 +1264,12 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
 							self.forward_broadcast_msg(peers, &wire::Message::ChannelUpdate(msg), None);
 						}
 					},
+					MessageSendEvent::SendChannelUpdate { ref node_id, ref msg } => {
+						log_trace!(self.logger, "Handling SendChannelUpdate event in peer_handler for node {} for channel {}",
+								log_pubkey!(node_id), msg.contents.short_channel_id);
+						let peer = get_peer_for_forwarding!(node_id);
+						peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg)));
+					},
 					MessageSendEvent::PaymentFailureNetworkUpdate { ref update } => {
 						self.message_handler.route_handler.handle_htlc_fail_channel_update(update);
 					},