Move broadcast_node_announcement to PeerManager

Some `NodeFeatures` will, in the future, represent features which
are not enabled by the `ChannelManager`, but by other message
handlers handlers. Thus, it doesn't make sense to determine the
node feature bits in the `ChannelManager`.

The simplest fix for this is to change to generating the
node_announcement in `PeerManager`, asking all the connected
handlers which feature bits they support and simply OR'ing them
together. While this may not be sufficient in the future as it
doesn't consider feature bit dependencies, support for those could
be handled at the feature level in the future.

This commit moves the `broadcast_node_announcement` function to
`PeerHandler` but does not yet implement feature OR'ing.

Author: TheBlueMatt

lightning-background-processor/src/lib.rs

@@ -746,7 +746,7 @@ mod tests {
 			let p2p_gossip_sync = Arc::new(P2PGossipSync::new(network_graph.clone(), Some(chain_source.clone()), logger.clone()));
 			let rapid_gossip_sync = Arc::new(RapidGossipSync::new(network_graph.clone()));
 			let msg_handler = MessageHandler { chan_handler: Arc::new(test_utils::TestChannelMessageHandler::new()), route_handler: Arc::new(test_utils::TestRoutingMessageHandler::new()), onion_message_handler: IgnoringMessageHandler{}};
-			let peer_manager = Arc::new(PeerManager::new(msg_handler, keys_manager.get_node_secret(Recipient::Node).unwrap(), &seed, logger.clone(), IgnoringMessageHandler{}));
+			let peer_manager = Arc::new(PeerManager::new(msg_handler, keys_manager.get_node_secret(Recipient::Node).unwrap(), 0, &seed, logger.clone(), IgnoringMessageHandler{}));
 			let scorer = Arc::new(Mutex::new(test_utils::TestScorer::with_penalty(0)));
 			let node = Node { node: manager, p2p_gossip_sync, rapid_gossip_sync, peer_manager, chain_monitor, persister, tx_broadcaster, network_graph, logger, best_block, scorer };
 			nodes.push(node);

lightning-net-tokio/src/lib.rs

@@ -546,6 +546,7 @@ mod tests {
 	use lightning::ln::features::*;
 	use lightning::ln::msgs::*;
 	use lightning::ln::peer_handler::{MessageHandler, PeerManager};
+	use lightning::ln::features::NodeFeatures;
 	use lightning::util::events::*;
 	use bitcoin::secp256k1::{Secp256k1, SecretKey, PublicKey};
 
@@ -612,6 +613,7 @@ mod tests {
 		}
 		fn handle_channel_reestablish(&self, _their_node_id: &PublicKey, _msg: &ChannelReestablish) {}
 		fn handle_error(&self, _their_node_id: &PublicKey, _msg: &ErrorMessage) {}
+		fn get_provided_node_features(&self) -> NodeFeatures { NodeFeatures::known() }
 	}
 	impl MessageSendEventsProvider for MsgHandler {
 		fn get_and_clear_pending_msg_events(&self) -> Vec<MessageSendEvent> {
@@ -657,7 +659,7 @@ mod tests {
 			chan_handler: Arc::clone(&a_handler),
 			route_handler: Arc::clone(&a_handler),
 			onion_message_handler: Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{}),
-		}, a_key.clone(), &[1; 32], Arc::new(TestLogger()), Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{})));
+		}, a_key.clone(), 0, &[1; 32], Arc::new(TestLogger()), Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{})));
 
 		let (b_connected_sender, mut b_connected) = mpsc::channel(1);
 		let (b_disconnected_sender, mut b_disconnected) = mpsc::channel(1);
@@ -672,7 +674,7 @@ mod tests {
 			chan_handler: Arc::clone(&b_handler),
 			route_handler: Arc::clone(&b_handler),
 			onion_message_handler: Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{}),
-		}, b_key.clone(), &[2; 32], Arc::new(TestLogger()), Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{})));
+		}, b_key.clone(), 0, &[2; 32], Arc::new(TestLogger()), Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{})));
 
 		// We bind on localhost, hoping the environment is properly configured with a local
 		// address. This may not always be the case in containers and the like, so if this test is
@@ -725,7 +727,7 @@ mod tests {
 			chan_handler: Arc::new(lightning::ln::peer_handler::ErroringMessageHandler::new()),
 			onion_message_handler: Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{}),
 			route_handler: Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{}),
-		}, a_key, &[1; 32], Arc::new(TestLogger()), Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{})));
+		}, a_key, 0, &[1; 32], Arc::new(TestLogger()), Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{})));
 
 		// Make two connections, one for an inbound and one for an outbound connection
 		let conn_a = {

lightning/src/ln/channelmanager.rs

@@ -764,10 +764,6 @@ pub struct ChannelManager<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref,
 	/// keeping additional state.
 	probing_cookie_secret: [u8; 32],
 
-	/// Used to track the last value sent in a node_announcement "timestamp" field. We ensure this
-	/// value increases strictly since we don't assume access to a time source.
-	last_node_announcement_serial: AtomicUsize,
-
 	/// The highest block timestamp we've seen, which is usually a good guess at the current time.
 	/// Assuming most miners are generating blocks with reasonable timestamps, this shouldn't be
 	/// very far in the past, and can only ever be up to two hours in the future.
@@ -1617,7 +1613,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 
 			probing_cookie_secret: keys_manager.get_secure_random_bytes(),
 
-			last_node_announcement_serial: AtomicUsize::new(0),
 			highest_seen_timestamp: AtomicUsize::new(0),
 
 			per_peer_state: RwLock::new(HashMap::new()),
@@ -2940,68 +2935,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	// smaller than 100:
 	const STATIC_ASSERT: u32 = Self::HALF_MESSAGE_IS_ADDRS - 100;
 
-	/// Regenerates channel_announcements and generates a signed node_announcement from the given
-	/// arguments, providing them in corresponding events via
-	/// [`get_and_clear_pending_msg_events`], if at least one public channel has been confirmed
-	/// on-chain. This effectively re-broadcasts all channel announcements and sends our node
-	/// announcement to ensure that the lightning P2P network is aware of the channels we have and
-	/// our network addresses.
-	///
-	/// `rgb` is a node "color" and `alias` is a printable human-readable string to describe this
-	/// node to humans. They carry no in-protocol meaning.
-	///
-	/// `addresses` represent the set (possibly empty) of socket addresses on which this node
-	/// accepts incoming connections. These will be included in the node_announcement, publicly
-	/// tying these addresses together and to this node. If you wish to preserve user privacy,
-	/// addresses should likely contain only Tor Onion addresses.
-	///
-	/// Panics if `addresses` is absurdly large (more than 100).
-	///
-	/// [`get_and_clear_pending_msg_events`]: MessageSendEventsProvider::get_and_clear_pending_msg_events
-	pub fn broadcast_node_announcement(&self, rgb: [u8; 3], alias: [u8; 32], mut addresses: Vec<NetAddress>) {
-		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
-
-		if addresses.len() > 100 {
-			panic!("More than half the message size was taken up by public addresses!");
-		}
-
-		// While all existing nodes handle unsorted addresses just fine, the spec requires that
-		// addresses be sorted for future compatibility.
-		addresses.sort_by_key(|addr| addr.get_id());
-
-		let announcement = msgs::UnsignedNodeAnnouncement {
-			features: NodeFeatures::known(),
-			timestamp: self.last_node_announcement_serial.fetch_add(1, Ordering::AcqRel) as u32,
-			node_id: self.get_our_node_id(),
-			rgb, alias, addresses,
-			excess_address_data: Vec::new(),
-			excess_data: Vec::new(),
-		};
-		let msghash = hash_to_message!(&Sha256dHash::hash(&announcement.encode()[..])[..]);
-		let node_announce_sig = sign(&self.secp_ctx, &msghash, &self.our_network_key);
-
-		let mut channel_state_lock = self.channel_state.lock().unwrap();
-		let channel_state = &mut *channel_state_lock;
-
-		let mut announced_chans = false;
-		for (_, chan) in channel_state.by_id.iter() {
-			if chan.get_signed_channel_announcement(self.get_our_node_id(), self.genesis_hash.clone(), self.best_block.read().unwrap().height()).is_some()
-				&& self.get_channel_update_for_broadcast(chan).is_ok()
-			{
-				announced_chans = true;
-			}
-		}
-
-		if announced_chans {
-			channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastNodeAnnouncement {
-				msg: msgs::NodeAnnouncement {
-					signature: node_announce_sig,
-					contents: announcement
-				},
-			});
-		}
-	}
-
 	/// Atomically updates the [`ChannelConfig`] for the given channels.
 	///
 	/// Once the updates are applied, each eligible channel (advertised with a known short channel
@@ -5784,7 +5717,6 @@ where
 				}
 			}
 		}
-		max_time!(self.last_node_announcement_serial);
 		max_time!(self.highest_seen_timestamp);
 		let mut payment_secrets = self.pending_inbound_payments.lock().unwrap();
 		payment_secrets.retain(|_, inbound_payment| {
@@ -6136,7 +6068,6 @@ impl<Signer: Sign, M: Deref , T: Deref , K: Deref , F: Deref , L: Deref >
 					&events::MessageSendEvent::SendChannelReestablish { ref node_id, .. } => node_id != counterparty_node_id,
 					&events::MessageSendEvent::SendChannelAnnouncement { ref node_id, .. } => node_id != counterparty_node_id,
 					&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,
@@ -6241,6 +6172,10 @@ impl<Signer: Sign, M: Deref , T: Deref , K: Deref , F: Deref , L: Deref >
 			let _ = self.force_close_channel_with_peer(&msg.channel_id, counterparty_node_id, Some(&msg.data), true);
 		}
 	}
+
+	fn get_provided_node_features(&self) -> NodeFeatures {
+		NodeFeatures::known()
+	}
 }
 
 const SERIALIZATION_VERSION: u8 = 1;
@@ -6663,7 +6598,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable f
 			}
 		}
 
-		(self.last_node_announcement_serial.load(Ordering::Acquire) as u32).write(writer)?;
 		(self.highest_seen_timestamp.load(Ordering::Acquire) as u32).write(writer)?;
 
 		(pending_inbound_payments.len() as u64).write(writer)?;
@@ -6982,7 +6916,6 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 			}
 		}
 
-		let last_node_announcement_serial: u32 = Readable::read(reader)?;
 		let highest_seen_timestamp: u32 = Readable::read(reader)?;
 
 		let pending_inbound_payment_count: u64 = Readable::read(reader)?;
@@ -7243,7 +7176,6 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 			our_network_pubkey,
 			secp_ctx,
 
-			last_node_announcement_serial: AtomicUsize::new(last_node_announcement_serial as usize),
 			highest_seen_timestamp: AtomicUsize::new(highest_seen_timestamp as usize),
 
 			per_peer_state: RwLock::new(per_peer_state),

lightning/src/ln/functional_test_utils.rs

@@ -826,34 +826,10 @@ pub fn create_unannounced_chan_between_nodes_with_value<'a, 'b, 'c, 'd>(nodes: &
 }
 
 pub fn update_nodes_with_chan_announce<'a, 'b, 'c, 'd>(nodes: &'a Vec<Node<'b, 'c, 'd>>, a: usize, b: usize, ann: &msgs::ChannelAnnouncement, upd_1: &msgs::ChannelUpdate, upd_2: &msgs::ChannelUpdate) {
-	nodes[a].node.broadcast_node_announcement([0, 0, 0], [0; 32], Vec::new());
-	let a_events = nodes[a].node.get_and_clear_pending_msg_events();
-	assert_eq!(a_events.len(), 1);
-
-	let a_node_announcement = match a_events.last().unwrap() {
-		MessageSendEvent::BroadcastNodeAnnouncement { ref msg } => {
-			(*msg).clone()
-		},
-		_ => panic!("Unexpected event"),
-	};
-
-	nodes[b].node.broadcast_node_announcement([1, 1, 1], [1; 32], Vec::new());
-	let b_events = nodes[b].node.get_and_clear_pending_msg_events();
-	assert_eq!(b_events.len(), 1);
-
-	let b_node_announcement = match b_events.last().unwrap() {
-		MessageSendEvent::BroadcastNodeAnnouncement { ref msg } => {
-			(*msg).clone()
-		},
-		_ => panic!("Unexpected event"),
-	};
-
 	for node in nodes {
 		assert!(node.gossip_sync.handle_channel_announcement(ann).unwrap());
 		node.gossip_sync.handle_channel_update(upd_1).unwrap();
 		node.gossip_sync.handle_channel_update(upd_2).unwrap();
-		node.gossip_sync.handle_node_announcement(&a_node_announcement).unwrap();
-		node.gossip_sync.handle_node_announcement(&b_node_announcement).unwrap();
 
 		// Note that channel_updates are also delivered to ChannelManagers to ensure we have
 		// forwarding info for local channels even if its not accepted in the network graph.

lightning/src/ln/msgs.rs

@@ -896,6 +896,12 @@ pub trait ChannelMessageHandler : MessageSendEventsProvider {
 	// Error:
 	/// Handle an incoming error message from the given peer.
 	fn handle_error(&self, their_node_id: &PublicKey, msg: &ErrorMessage);
+
+	// Handler information:
+	/// Gets the node feature flags which this handler itself supports. All available handlers are
+	/// queried similarly and their feature flags are OR'd together to form the [`NodeFeatures`]
+	/// which are broadcasted in our node_announcement message.
+	fn get_provided_node_features(&self) -> NodeFeatures;
 }
 
 /// A trait to describe an object which can receive routing messages.