Merge pull request #2024 from TheBlueMatt/2023-02-6conf-pub-hints

Include a route hint for public, not-yet-announced channels

Author: TheBlueMatt

lightning-invoice/src/utils.rs

@@ -512,8 +512,10 @@ fn _create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_has
 /// * Always select the channel with the highest inbound capacity per counterparty node
 /// * Prefer channels with capacity at least `min_inbound_capacity_msat` and where the channel
 ///   `is_usable` (i.e. the peer is connected).
-/// * If any public channel exists, the returned `RouteHint`s will be empty, and the sender will
-///   need to find the path by looking at the public channels instead
+/// * If any public channel exists, only public [`RouteHint`]s will be returned.
+/// * If any public, announced, channel exists (i.e. a channel with 7+ confs, to ensure the
+///   announcement has had a chance to propagate), no [`RouteHint`]s will be returned, as the
+///   sender is expected to find the path by looking at the public channels instead.
 fn filter_channels<L: Deref>(
 	channels: Vec<ChannelDetails>, min_inbound_capacity_msat: Option<u64>, logger: &L
 ) -> Vec<RouteHint> where L::Target: Logger {
@@ -522,6 +524,7 @@ fn filter_channels<L: Deref>(
 	let mut min_capacity_channel_exists = false;
 	let mut online_channel_exists = false;
 	let mut online_min_capacity_channel_exists = false;
+	let mut has_pub_unconf_chan = false;
 
 	log_trace!(logger, "Considering {} channels for invoice route hints", channels.len());
 	for channel in channels.into_iter().filter(|chan| chan.is_channel_ready) {
@@ -531,11 +534,18 @@ fn filter_channels<L: Deref>(
 		}
 
 		if channel.is_public {
-			// If any public channel exists, return no hints and let the sender
-			// look at the public channels instead.
-			log_trace!(logger, "Not including channels in invoice route hints on account of public channel {}",
-				log_bytes!(channel.channel_id));
-			return vec![]
+			if channel.confirmations.is_some() && channel.confirmations < Some(7) {
+				// If we have a public channel, but it doesn't have enough confirmations to (yet)
+				// be in the public network graph (and have gotten a chance to propagate), include
+				// route hints but only for public channels to protect private channel privacy.
+				has_pub_unconf_chan = true;
+			} else {
+				// If any public channel exists, return no hints and let the sender
+				// look at the public channels instead.
+				log_trace!(logger, "Not including channels in invoice route hints on account of public channel {}",
+					log_bytes!(channel.channel_id));
+				return vec![]
+			}
 		}
 
 		if channel.inbound_capacity_msat >= min_inbound_capacity {
@@ -557,20 +567,32 @@ fn filter_channels<L: Deref>(
 		match filtered_channels.entry(channel.counterparty.node_id) {
 			hash_map::Entry::Occupied(mut entry) => {
 				let current_max_capacity = entry.get().inbound_capacity_msat;
-				if channel.inbound_capacity_msat < current_max_capacity {
+				// If this channel is public and the previous channel is not, ensure we replace the
+				// previous channel to avoid announcing non-public channels.
+				let new_now_public = channel.is_public && !entry.get().is_public;
+				// If the public-ness of the channel has not changed (in which case simply defer to
+				// `new_now_public), and this channel has a greater capacity, prefer to announce
+				// this channel.
+				let new_higher_capacity = channel.is_public == entry.get().is_public &&
+					channel.inbound_capacity_msat > current_max_capacity;
+				if new_now_public || new_higher_capacity {
+					log_trace!(logger,
+						"Preferring counterparty {} channel {} (SCID {:?}, {} msats) over {} (SCID {:?}, {} msats) for invoice route hints",
+						log_pubkey!(channel.counterparty.node_id),
+						log_bytes!(channel.channel_id), channel.short_channel_id,
+						channel.inbound_capacity_msat,
+						log_bytes!(entry.get().channel_id), entry.get().short_channel_id,
+						current_max_capacity);
+					entry.insert(channel);
+				} else {
 					log_trace!(logger,
-						"Preferring counterparty {} channel {} ({} msats) over {} ({} msats) for invoice route hints",
+						"Preferring counterparty {} channel {} (SCID {:?}, {} msats) over {} (SCID {:?}, {} msats) for invoice route hints",
 						log_pubkey!(channel.counterparty.node_id),
-						log_bytes!(entry.get().channel_id), current_max_capacity,
-						log_bytes!(channel.channel_id), channel.inbound_capacity_msat);
-					continue;
+						log_bytes!(entry.get().channel_id), entry.get().short_channel_id,
+						current_max_capacity,
+						log_bytes!(channel.channel_id), channel.short_channel_id,
+						channel.inbound_capacity_msat);
 				}
-				log_trace!(logger,
-					"Preferring counterparty {} channel {} ({} msats) over {} ({} msats) for invoice route hints",
-					log_pubkey!(channel.counterparty.node_id),
-					log_bytes!(channel.channel_id), channel.inbound_capacity_msat,
-					log_bytes!(entry.get().channel_id), current_max_capacity);
-				entry.insert(channel);
 			}
 			hash_map::Entry::Vacant(entry) => {
 				entry.insert(channel);
@@ -600,7 +622,12 @@ fn filter_channels<L: Deref>(
 		.map(|(_, channel)| channel)
 		.filter(|channel| {
 			let has_enough_capacity = channel.inbound_capacity_msat >= min_inbound_capacity;
-			let include_channel = if online_min_capacity_channel_exists {
+			let include_channel = if has_pub_unconf_chan {
+				// If we have a public channel, but it doesn't have enough confirmations to (yet)
+				// be in the public network graph (and have gotten a chance to propagate), include
+				// route hints but only for public channels to protect private channel privacy.
+				channel.is_public
+			} else if online_min_capacity_channel_exists {
 				has_enough_capacity && channel.is_usable
 			} else if min_capacity_channel_exists && online_channel_exists {
 				// If there are some online channels and some min_capacity channels, but no
@@ -620,7 +647,7 @@ fn filter_channels<L: Deref>(
 				log_trace!(logger, "Ignoring channel {} without enough capacity for invoice route hints",
 					log_bytes!(channel.channel_id));
 			} else {
-				debug_assert!(!channel.is_usable);
+				debug_assert!(!channel.is_usable || (has_pub_unconf_chan && !channel.is_public));
 				log_trace!(logger, "Ignoring channel {} with disconnected peer",
 					log_bytes!(channel.channel_id));
 			}
@@ -789,6 +816,63 @@ mod test {
 		assert_eq!(invoice.payment_hash(), &sha256::Hash::from_slice(&payment_hash.0[..]).unwrap());
 	}
 
+	#[test]
+	fn test_hints_has_only_public_confd_channels() {
+		let chanmon_cfgs = create_chanmon_cfgs(2);
+		let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+		let mut config = test_default_channel_config();
+		config.channel_handshake_config.minimum_depth = 1;
+		let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(config), Some(config)]);
+		let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+		// Create a private channel with lots of capacity and a lower value public channel (without
+		// confirming the funding tx yet).
+		let unannounced_scid = create_unannounced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 0);
+		let conf_tx = create_chan_between_nodes_with_value_init(&nodes[0], &nodes[1], 10_000, 0);
+
+		// Before the channel is available, we should include the unannounced_scid.
+		let mut scid_aliases = HashSet::new();
+		scid_aliases.insert(unannounced_scid.0.short_channel_id_alias.unwrap());
+		match_invoice_routes(Some(5000), &nodes[1], scid_aliases.clone());
+
+		// However after we mine the funding tx and exchange channel_ready messages for the public
+		// channel we'll immediately switch to including it as a route hint, even though it isn't
+		// yet announced.
+		let pub_channel_scid = mine_transaction(&nodes[0], &conf_tx);
+		let node_a_pub_channel_ready = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReady, nodes[1].node.get_our_node_id());
+		nodes[1].node.handle_channel_ready(&nodes[0].node.get_our_node_id(), &node_a_pub_channel_ready);
+
+		assert_eq!(mine_transaction(&nodes[1], &conf_tx), pub_channel_scid);
+		let events = nodes[1].node.get_and_clear_pending_msg_events();
+		assert_eq!(events.len(), 2);
+		if let MessageSendEvent::SendChannelReady { msg, .. } = &events[0] {
+			nodes[0].node.handle_channel_ready(&nodes[1].node.get_our_node_id(), msg);
+		} else { panic!(); }
+		if let MessageSendEvent::SendChannelUpdate { msg, .. } = &events[1] {
+			nodes[0].node.handle_channel_update(&nodes[1].node.get_our_node_id(), msg);
+		} else { panic!(); }
+
+		nodes[1].node.handle_channel_update(&nodes[0].node.get_our_node_id(), &get_event_msg!(nodes[0], MessageSendEvent::SendChannelUpdate, nodes[1].node.get_our_node_id()));
+
+		expect_channel_ready_event(&nodes[0], &nodes[1].node.get_our_node_id());
+		expect_channel_ready_event(&nodes[1], &nodes[0].node.get_our_node_id());
+
+		scid_aliases.clear();
+		scid_aliases.insert(node_a_pub_channel_ready.short_channel_id_alias.unwrap());
+		match_invoice_routes(Some(5000), &nodes[1], scid_aliases.clone());
+		// This also applies even if the amount is more than the payment amount, to ensure users
+		// dont screw up their privacy.
+		match_invoice_routes(Some(50_000_000), &nodes[1], scid_aliases.clone());
+
+		// The same remains true until the channel has 7 confirmations, at which point we include
+		// no hints.
+		connect_blocks(&nodes[1], 5);
+		match_invoice_routes(Some(5000), &nodes[1], scid_aliases.clone());
+		connect_blocks(&nodes[1], 1);
+		get_event_msg!(nodes[1], MessageSendEvent::SendAnnouncementSignatures, nodes[0].node.get_our_node_id());
+		match_invoice_routes(Some(5000), &nodes[1], HashSet::new());
+	}
+
 	#[test]
 	fn test_hints_includes_single_channels_to_nodes() {
 		let chanmon_cfgs = create_chanmon_cfgs(3);

lightning/src/ln/functional_test_utils.rs

@@ -63,9 +63,12 @@ pub fn confirm_transaction<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, tx: &Tran
 	scid
 }
 /// Mine a single block containing the given transaction
-pub fn mine_transaction<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, tx: &Transaction) {
+///
+/// Returns the SCID a channel confirmed in the given transaction will have, assuming the funding
+/// output is the 1st output in the transaction.
+pub fn mine_transaction<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, tx: &Transaction) -> u64 {
 	let height = node.best_block_info().1 + 1;
-	confirm_transaction_at(node, tx, height);
+	confirm_transaction_at(node, tx, height)
 }
 /// Mine a single block containing the given transactions
 pub fn mine_transactions<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, txn: &[&Transaction]) {
@@ -1098,6 +1101,10 @@ pub fn create_unannounced_chan_between_nodes_with_value<'a, 'b, 'c, 'd>(nodes: &
 	nodes[a].node.handle_funding_signed(&nodes[b].node.get_our_node_id(), &cs_funding_signed);
 	check_added_monitors!(nodes[a], 1);
 
+	assert_eq!(nodes[a].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1);
+	assert_eq!(nodes[a].tx_broadcaster.txn_broadcasted.lock().unwrap()[0], tx);
+	nodes[a].tx_broadcaster.txn_broadcasted.lock().unwrap().clear();
+
 	let conf_height = core::cmp::max(nodes[a].best_block_info().1 + 1, nodes[b].best_block_info().1 + 1);
 	confirm_transaction_at(&nodes[a], &tx, conf_height);
 	connect_blocks(&nodes[a], CHAN_CONFIRM_DEPTH - 1);