Merge pull request #4340 from Thrishalmadasu/issue-1672-dont-forward-bit

Set dont_forward on private channel updates and add tests

Author: TheBlueMatt

lightning/src/ln/channelmanager.rs

@@ -5088,7 +5088,7 @@ impl<
 			chain_hash: self.chain_hash,
 			short_channel_id,
 			timestamp: chan.context.get_update_time_counter(),
-			message_flags: 1, // Only must_be_one
+			message_flags: 1 | if !chan.context.should_announce() { 1 << 1 } else { 0 }, // must_be_one + dont_forward
 			channel_flags: (!were_node_one) as u8 | ((!enabled as u8) << 1),
 			cltv_expiry_delta: chan.context.get_cltv_expiry_delta(),
 			htlc_minimum_msat: chan.context.get_counterparty_htlc_minimum_msat(),
@@ -12274,6 +12274,9 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 			Some((cp_id, chan_id)) => (cp_id.clone(), chan_id.clone()),
 			None => {
 				// It's not a local channel
+				if msg.contents.message_flags & (1 << 1) != 0 {
+					log_debug!(self.logger, "Received channel_update for unknown channel {} with dont_forward set. You may wish to check if an incorrect tx_index was passed to chain::Confirm::transactions_confirmed.", msg.contents.short_channel_id);
+				}
 				return Ok(NotifyOption::SkipPersistNoEvents)
 			}
 		};

lightning/src/ln/priv_short_conf_tests.rs

@@ -1520,3 +1520,133 @@ fn test_0conf_ann_sigs_racing_conf() {
 	let as_announcement = nodes[0].node.get_and_clear_pending_msg_events();
 	assert_eq!(as_announcement.len(), 1);
 }
+
+#[test]
+fn test_channel_update_dont_forward_flag() {
+	// Test that the `dont_forward` bit (bit 1 of message_flags) is set correctly:
+	// - For private channels: message_flags should have bit 1 set (value 3 = must_be_one + dont_forward)
+	// - For public channels: message_flags should NOT have bit 1 set (value 1 = must_be_one only)
+	let chanmon_cfgs = create_chanmon_cfgs(3);
+	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+	let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+	let node_b_id = nodes[1].node.get_our_node_id();
+	let node_c_id = nodes[2].node.get_our_node_id();
+
+	// Create a public (announced) channel between nodes[0] and nodes[1]
+	create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 500_000_000);
+
+	// Create a private (unannounced) channel between nodes[1] and nodes[2]
+	create_unannounced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 500_000_000);
+
+	// Get the channel details for both channels
+	let public_channel = nodes[0]
+		.node
+		.list_channels()
+		.into_iter()
+		.find(|c| c.counterparty.node_id == node_b_id)
+		.unwrap();
+	let private_channel = nodes[1]
+		.node
+		.list_channels()
+		.into_iter()
+		.find(|c| c.counterparty.node_id == node_c_id)
+		.unwrap();
+
+	// Verify is_announced correctly reflects the channel type
+	assert!(public_channel.is_announced, "Public channel should have is_announced = true");
+	assert!(!private_channel.is_announced, "Private channel should have is_announced = false");
+
+	// Trigger channel_update by changing config on the public channel
+	let mut new_config = public_channel.config.unwrap();
+	new_config.forwarding_fee_base_msat += 10;
+	nodes[0]
+		.node
+		.update_channel_config(&node_b_id, &[public_channel.channel_id], &new_config)
+		.unwrap();
+
+	// Get the channel_update for the public channel and verify dont_forward is NOT set
+	let events = nodes[0].node.get_and_clear_pending_msg_events();
+	let public_channel_update = events
+		.iter()
+		.find_map(|e| {
+			if let MessageSendEvent::BroadcastChannelUpdate { ref msg, .. } = e {
+				Some(msg.clone())
+			} else {
+				None
+			}
+		})
+		.expect("Expected BroadcastChannelUpdate for public channel");
+	// message_flags should be 1 (only must_be_one bit set, dont_forward NOT set)
+	assert_eq!(
+		public_channel_update.contents.message_flags & (1 << 1),
+		0,
+		"Public channel update should NOT have dont_forward bit set"
+	);
+	assert_eq!(
+		public_channel_update.contents.message_flags & 1,
+		1,
+		"Public channel update should have must_be_one bit set"
+	);
+
+	// Trigger channel_update by changing config on the private channel
+	let mut new_config = private_channel.config.unwrap();
+	new_config.forwarding_fee_base_msat += 10;
+	nodes[1]
+		.node
+		.update_channel_config(&node_c_id, &[private_channel.channel_id], &new_config)
+		.unwrap();
+
+	// Get the channel_update for the private channel and verify dont_forward IS set
+	let private_channel_update =
+		get_event_msg!(nodes[1], MessageSendEvent::SendChannelUpdate, node_c_id);
+	// message_flags should have dont_forward bit set
+	assert_ne!(
+		private_channel_update.contents.message_flags & (1 << 1),
+		0,
+		"Private channel update should have dont_forward bit set"
+	);
+	assert_eq!(
+		private_channel_update.contents.message_flags & 1,
+		1,
+		"Private channel update should have must_be_one bit set"
+	);
+}
+
+#[test]
+fn test_unknown_channel_update_with_dont_forward_logs_debug() {
+	use bitcoin::constants::ChainHash;
+	use bitcoin::secp256k1::ecdsa::Signature;
+	use bitcoin::secp256k1::ffi::Signature as FFISignature;
+	use bitcoin::Network;
+
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let unknown_scid = 42;
+	let msg = msgs::ChannelUpdate {
+		signature: Signature::from(unsafe { FFISignature::new() }),
+		contents: msgs::UnsignedChannelUpdate {
+			chain_hash: ChainHash::using_genesis_block(Network::Testnet),
+			short_channel_id: unknown_scid,
+			timestamp: 0,
+			message_flags: 1 | (1 << 1), // must_be_one + dont_forward
+			channel_flags: 0,
+			cltv_expiry_delta: 0,
+			htlc_minimum_msat: 0,
+			htlc_maximum_msat: msgs::MAX_VALUE_MSAT,
+			fee_base_msat: 0,
+			fee_proportional_millionths: 0,
+			excess_data: Vec::new(),
+		},
+	};
+
+	nodes[0].node.handle_channel_update(nodes[1].node.get_our_node_id(), &msg);
+	nodes[0].logger.assert_log_contains(
+		"lightning::ln::channelmanager",
+		"Received channel_update for unknown channel",
+		1,
+	);
+}

lightning/src/routing/gossip.rs

@@ -551,6 +551,14 @@ impl<G: Deref<Target = NetworkGraph<L>>, U: UtxoLookup, L: Logger> RoutingMessag
 	fn handle_channel_update(
 		&self, _their_node_id: Option<PublicKey>, msg: &msgs::ChannelUpdate,
 	) -> Result<Option<(NodeId, NodeId)>, LightningError> {
+		// Ignore channel updates with dont_forward bit set - these are for private channels
+		// and shouldn't be gossiped or stored in the network graph
+		if msg.contents.message_flags & (1 << 1) != 0 {
+			return Err(LightningError {
+				err: "Ignoring channel_update with dont_forward bit set".to_owned(),
+				action: ErrorAction::IgnoreAndLog(Level::Debug),
+			});
+		}
 		match self.network_graph.update_channel(msg) {
 			Ok(nodes) if msg.contents.excess_data.len() <= MAX_EXCESS_BYTES_FOR_RELAY => Ok(nodes),
 			Ok(_) => Ok(None),
@@ -3294,6 +3302,65 @@ pub(crate) mod tests {
 		};
 	}
 
+	#[test]
+	fn handling_channel_update_with_dont_forward_flag() {
+		// Test that channel updates with the dont_forward bit set are rejected
+		let secp_ctx = Secp256k1::new();
+		let logger = test_utils::TestLogger::new();
+		let chain_source = test_utils::TestChainSource::new(Network::Testnet);
+		let network_graph = NetworkGraph::new(Network::Testnet, &logger);
+		let gossip_sync = P2PGossipSync::new(&network_graph, Some(&chain_source), &logger);
+
+		let node_1_privkey = &SecretKey::from_slice(&[42; 32]).unwrap();
+		let node_1_pubkey = PublicKey::from_secret_key(&secp_ctx, node_1_privkey);
+		let node_2_privkey = &SecretKey::from_slice(&[41; 32]).unwrap();
+
+		// First announce a channel so we have something to update
+		let good_script = get_channel_script(&secp_ctx);
+		*chain_source.utxo_ret.lock().unwrap() = UtxoResult::Sync(Ok(TxOut {
+			value: Amount::from_sat(1000_000),
+			script_pubkey: good_script.clone(),
+		}));
+
+		let valid_channel_announcement =
+			get_signed_channel_announcement(|_| {}, node_1_privkey, node_2_privkey, &secp_ctx);
+		gossip_sync
+			.handle_channel_announcement(Some(node_1_pubkey), &valid_channel_announcement)
+			.unwrap();
+
+		// Create a channel update with dont_forward bit set (bit 1 of message_flags)
+		let dont_forward_update = get_signed_channel_update(
+			|unsigned_channel_update| {
+				unsigned_channel_update.message_flags = 1 | (1 << 1); // must_be_one + dont_forward
+			},
+			node_1_privkey,
+			&secp_ctx,
+		);
+
+		// The update should be rejected because dont_forward is set
+		match gossip_sync.handle_channel_update(Some(node_1_pubkey), &dont_forward_update) {
+			Ok(_) => panic!("Expected channel update with dont_forward to be rejected"),
+			Err(e) => {
+				assert_eq!(e.err, "Ignoring channel_update with dont_forward bit set");
+				match e.action {
+					crate::ln::msgs::ErrorAction::IgnoreAndLog(level) => {
+						assert_eq!(level, crate::util::logger::Level::Debug)
+					},
+					_ => panic!("Expected IgnoreAndLog action"),
+				}
+			},
+		};
+
+		// Verify the update was not applied to the network graph
+		let channels = network_graph.read_only();
+		let channel =
+			channels.channels().get(&valid_channel_announcement.contents.short_channel_id).unwrap();
+		assert!(
+			channel.one_to_two.is_none(),
+			"Channel update with dont_forward should not be stored in network graph"
+		);
+	}
+
 	#[test]
 	fn handling_network_update() {
 		let logger = test_utils::TestLogger::new();