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

fuzz/src/full_stack.rs

@@ -415,7 +415,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 		chan_handler: channelmanager.clone(),
 		route_handler: gossip_sync.clone(),
 		onion_message_handler: IgnoringMessageHandler {},
-	}, our_network_key, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 15, 0], Arc::clone(&logger), IgnoringMessageHandler{}));
+	}, our_network_key, 0, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 15, 0], Arc::clone(&logger), IgnoringMessageHandler{}));
 
 	let mut should_forward = false;
 	let mut payments_received: Vec<PaymentHash> = Vec::new();

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 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

@@ -54,7 +54,6 @@ use chain::keysinterface::{Sign, KeysInterface, KeysManager, InMemorySigner, Rec
 use util::config::{UserConfig, ChannelConfig};
 use util::events::{EventHandler, EventsProvider, MessageSendEvent, MessageSendEventsProvider, ClosureReason, HTLCDestination};
 use util::{byte_utils, events};
-use util::crypto::sign;
 use util::wakers::{Future, Notifier};
 use util::scid_utils::fake_scid;
 use util::ser::{BigSize, FixedLengthReader, Readable, ReadableArgs, MaybeReadable, Writeable, Writer, VecWriter};
@@ -764,10 +763,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 +1612,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()),
@@ -2928,80 +2922,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		})
 	}
 
-	#[allow(dead_code)]
-	// Messages of up to 64KB should never end up more than half full with addresses, as that would
-	// be absurd. We ensure this by checking that at least 100 (our stated public contract on when
-	// broadcast_node_announcement panics) of the maximum-length addresses would fit in a 64KB
-	// message...
-	const HALF_MESSAGE_IS_ADDRS: u32 = ::core::u16::MAX as u32 / (NetAddress::MAX_LEN as u32 + 1) / 2;
-	#[deny(const_err)]
-	#[allow(dead_code)]
-	// ...by failing to compile if the number of addresses that would be half of a message is
-	// 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
@@ -5780,7 +5700,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| {
@@ -6132,7 +6051,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,
@@ -6237,6 +6155,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 provided_node_features(&self) -> NodeFeatures {
+		NodeFeatures::known()
+	}
 }
 
 const SERIALIZATION_VERSION: u8 = 1;
@@ -6659,7 +6581,10 @@ 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)?;
+		// Prior to 0.0.111 we tracked node_announcement serials here, however that now happens in
+		// `PeerManager`, and thus we simply write the `highest_seen_timestamp` twice, which is
+		// likely to be identical.
+		(self.highest_seen_timestamp.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)?;
@@ -6978,7 +6903,7 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 			}
 		}
 
-		let last_node_announcement_serial: u32 = Readable::read(reader)?;
+		let _last_node_announcement_serial: u32 = Readable::read(reader)?; // Only used < 0.0.111
 		let highest_seen_timestamp: u32 = Readable::read(reader)?;
 
 		let pending_inbound_payment_count: u64 = Readable::read(reader)?;
@@ -7239,7 +7164,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

@@ -892,34 +892,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 provided_node_features(&self) -> NodeFeatures;
 }
 
 /// A trait to describe an object which can receive routing messages.

lightning/src/ln/payment_tests.rs

@@ -676,7 +676,7 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) {
 	// Now nodes[1] should send a channel reestablish, which nodes[0] will respond to with an
 	// error, as the channel has hit the chain.
 	nodes[1].node.peer_connected(&nodes[0].node.get_our_node_id(), &msgs::Init { features: InitFeatures::known(), remote_network_address: None });
-	let bs_reestablish = get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, nodes[0].node.get_our_node_id());
+	let bs_reestablish = get_chan_reestablish_msgs!(nodes[1], nodes[0]).pop().unwrap();
 	nodes[0].node.handle_channel_reestablish(&nodes[1].node.get_our_node_id(), &bs_reestablish);
 	let as_err = nodes[0].node.get_and_clear_pending_msg_events();
 	assert_eq!(as_err.len(), 1);