Merge pull request #3144 from TheBlueMatt/2024-06-message-flags

(Re-)add handling for `ChannelUpdate::message_flags`

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -1069,7 +1069,7 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 						events::MessageSendEvent::SendChannelReady { .. } => continue,
 						events::MessageSendEvent::SendAnnouncementSignatures { .. } => continue,
 						events::MessageSendEvent::SendChannelUpdate { ref node_id, ref msg } => {
-							assert_eq!(msg.contents.flags & 2, 0); // The disable bit must never be set!
+							assert_eq!(msg.contents.channel_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
 						},
@@ -1207,7 +1207,7 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 							// 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);
+							assert_eq!(msg.contents.channel_flags & 2, 0);
 						},
 						_ => if out.may_fail.load(atomic::Ordering::Acquire) {
 							return;
@@ -1249,7 +1249,7 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 							events::MessageSendEvent::SendChannelReady { .. } => {},
 							events::MessageSendEvent::SendAnnouncementSignatures { .. } => {},
 							events::MessageSendEvent::SendChannelUpdate { ref msg, .. } => {
-								assert_eq!(msg.contents.flags & 2, 0); // The disable bit must never be set!
+								assert_eq!(msg.contents.channel_flags & 2, 0); // The disable bit must never be set!
 							},
 							_ => {
 								if out.may_fail.load(atomic::Ordering::Acquire) {
@@ -1275,7 +1275,7 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 							events::MessageSendEvent::SendChannelReady { .. } => {},
 							events::MessageSendEvent::SendAnnouncementSignatures { .. } => {},
 							events::MessageSendEvent::SendChannelUpdate { ref msg, .. } => {
-								assert_eq!(msg.contents.flags & 2, 0); // The disable bit must never be set!
+								assert_eq!(msg.contents.channel_flags & 2, 0); // The disable bit must never be set!
 							},
 							_ => {
 								if out.may_fail.load(atomic::Ordering::Acquire) {

fuzz/src/full_stack.rs

@@ -1779,7 +1779,7 @@ mod tests {
 
 		let log_entries = logger.lines.lock().unwrap();
 		assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Sending message to all peers except Some(PublicKey(0000000000000000000000000000000000000000000000000000000000000002ff00000000000000000000000000000000000000000000000000000000000002)) or the announced channel's counterparties: ChannelAnnouncement { node_signature_1: 3026020200b202200303030303030303030303030303030303030303030303030303030303030303, node_signature_2: 3026020200b202200202020202020202020202020202020202020202020202020202020202020202, bitcoin_signature_1: 3026020200b202200303030303030303030303030303030303030303030303030303030303030303, bitcoin_signature_2: 3026020200b202200202020202020202020202020202020202020202020202020202020202020202, contents: UnsignedChannelAnnouncement { features: [], chain_hash: 6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000, short_channel_id: 42, node_id_1: NodeId(030303030303030303030303030303030303030303030303030303030303030303), node_id_2: NodeId(020202020202020202020202020202020202020202020202020202020202020202), bitcoin_key_1: NodeId(030303030303030303030303030303030303030303030303030303030303030303), bitcoin_key_2: NodeId(020202020202020202020202020202020202020202020202020202020202020202), excess_data: [] } }".to_string())), Some(&1));
-		assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Sending message to all peers except Some(PublicKey(0000000000000000000000000000000000000000000000000000000000000002ff00000000000000000000000000000000000000000000000000000000000002)): ChannelUpdate { signature: 3026020200a602200303030303030303030303030303030303030303030303030303030303030303, contents: UnsignedChannelUpdate { chain_hash: 6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000, short_channel_id: 42, timestamp: 44, flags: 0, cltv_expiry_delta: 40, htlc_minimum_msat: 0, htlc_maximum_msat: 100000000, fee_base_msat: 0, fee_proportional_millionths: 0, excess_data: [] } }".to_string())), Some(&1));
+		assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Sending message to all peers except Some(PublicKey(0000000000000000000000000000000000000000000000000000000000000002ff00000000000000000000000000000000000000000000000000000000000002)): ChannelUpdate { signature: 3026020200a602200303030303030303030303030303030303030303030303030303030303030303, contents: UnsignedChannelUpdate { chain_hash: 6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000, short_channel_id: 42, timestamp: 44, message_flags: 1, channel_flags: 0, cltv_expiry_delta: 40, htlc_minimum_msat: 0, htlc_maximum_msat: 100000000, fee_base_msat: 0, fee_proportional_millionths: 0, excess_data: [] } }".to_string())), Some(&1));
 		assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Sending message to all peers except Some(PublicKey(0000000000000000000000000000000000000000000000000000000000000002ff00000000000000000000000000000000000000000000000000000000000002)) or the announced node: NodeAnnouncement { signature: 302502012802200303030303030303030303030303030303030303030303030303030303030303, contents: UnsignedNodeAnnouncement { features: [], timestamp: 43, node_id: NodeId(030303030303030303030303030303030303030303030303030303030303030303), rgb: [0, 0, 0], alias: NodeAlias([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, 0]), addresses: [], excess_address_data: [], excess_data: [] } }".to_string())), Some(&1));
 	}
 }

fuzz/src/msg_targets/gen_target.sh

@@ -39,14 +39,17 @@ GEN_TEST lightning::ln::msgs::ChannelReestablish test_msg_simple ""
 
 GEN_TEST lightning::ln::msgs::DecodedOnionErrorPacket test_msg ""
 
+# Gossip messages need to use `test_msg_exact` to ensure that messages
+# round-trip exactly when doing signature validation.
 GEN_TEST lightning::ln::msgs::ChannelAnnouncement test_msg_exact ""
 GEN_TEST lightning::ln::msgs::NodeAnnouncement test_msg_exact ""
+GEN_TEST lightning::ln::msgs::ChannelUpdate test_msg_exact ""
+
 GEN_TEST lightning::ln::msgs::QueryShortChannelIds test_msg ""
 GEN_TEST lightning::ln::msgs::ReplyChannelRange test_msg ""
 
 GEN_TEST lightning::ln::msgs::ErrorMessage test_msg_hole ", 32, 2"
 GEN_TEST lightning::ln::msgs::WarningMessage test_msg_hole ", 32, 2"
-GEN_TEST lightning::ln::msgs::ChannelUpdate test_msg_hole ", 108, 1"
 
 GEN_TEST lightning::ln::channel_state::ChannelDetails test_msg_simple ""
 

fuzz/src/msg_targets/mod.rs

@@ -25,11 +25,11 @@ pub mod msg_channel_reestablish;
 pub mod msg_decoded_onion_error_packet;
 pub mod msg_channel_announcement;
 pub mod msg_node_announcement;
+pub mod msg_channel_update;
 pub mod msg_query_short_channel_ids;
 pub mod msg_reply_channel_range;
 pub mod msg_error_message;
 pub mod msg_warning_message;
-pub mod msg_channel_update;
 pub mod msg_channel_details;
 pub mod msg_open_channel_v2;
 pub mod msg_accept_channel_v2;

fuzz/src/msg_targets/msg_channel_update.rs

@@ -17,11 +17,11 @@ use crate::utils::test_logger;
 
 #[inline]
 pub fn msg_channel_update_test<Out: test_logger::Output>(data: &[u8], _out: Out) {
-	test_msg_hole!(lightning::ln::msgs::ChannelUpdate, data, 108, 1);
+	test_msg_exact!(lightning::ln::msgs::ChannelUpdate, data);
 }
 
 #[no_mangle]
 pub extern "C" fn msg_channel_update_run(data: *const u8, datalen: usize) {
 	let data = unsafe { std::slice::from_raw_parts(data, datalen) };
-	test_msg_hole!(lightning::ln::msgs::ChannelUpdate, data, 108, 1);
+	test_msg_exact!(lightning::ln::msgs::ChannelUpdate, data);
 }

lightning-rapid-gossip-sync/src/processing.rs

@@ -370,7 +370,8 @@ where
 				chain_hash,
 				short_channel_id,
 				timestamp: backdated_timestamp,
-				flags: standard_channel_flags,
+				message_flags: 1, // Only must_be_one
+				channel_flags: standard_channel_flags,
 				cltv_expiry_delta: default_cltv_expiry_delta,
 				htlc_minimum_msat: default_htlc_minimum_msat,
 				htlc_maximum_msat: default_htlc_maximum_msat,

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -1130,7 +1130,7 @@ fn test_monitor_update_fail_reestablish() {
 	nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &as_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
+			.contents.channel_flags & 2, 0); // The "disabled" bit should be unset as we just reconnected
 
 	nodes[1].node.get_and_clear_pending_msg_events(); // Free the holding cell
 	check_added_monitors!(nodes[1], 1);
@@ -1151,13 +1151,13 @@ fn test_monitor_update_fail_reestablish() {
 	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
+			.contents.channel_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_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
+			.contents.channel_flags & 2, 0); // The "disabled" bit should be unset as we just reconnected
 
 	chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
 	let (outpoint, latest_update, _) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_1.2).unwrap().clone();

lightning/src/ln/channel.rs

@@ -9895,7 +9895,8 @@ mod tests {
 				chain_hash,
 				short_channel_id: 0,
 				timestamp: 0,
-				flags: 0,
+				message_flags: 1, // Only must_be_one
+				channel_flags: 0,
 				cltv_expiry_delta: 100,
 				htlc_minimum_msat: 5,
 				htlc_maximum_msat: MAX_VALUE_MSAT,

lightning/src/ln/channelmanager.rs

@@ -3506,7 +3506,7 @@ where
 			// peer has been disabled for some time), return `channel_disabled`,
 			// otherwise return `temporary_channel_failure`.
 			let chan_update_opt = self.get_channel_update_for_onion(next_packet.outgoing_scid, chan).ok();
-			if chan_update_opt.as_ref().map(|u| u.contents.flags & 2 == 2).unwrap_or(false) {
+			if chan_update_opt.as_ref().map(|u| u.contents.channel_flags & 2 == 2).unwrap_or(false) {
 				return Err(("Forwarding channel has been disconnected for some time.", 0x1000 | 20, chan_update_opt));
 			} else {
 				return Err(("Forwarding channel is not in a ready state.", 0x1000 | 7, chan_update_opt));
@@ -3785,7 +3785,8 @@ where
 			chain_hash: self.chain_hash,
 			short_channel_id,
 			timestamp: chan.context.get_update_time_counter(),
-			flags: (!were_node_one) as u8 | ((!enabled as u8) << 1),
+			message_flags: 1, // Only must_be_one
+			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(),
 			htlc_maximum_msat: chan.context.get_announced_htlc_max_msat(),
@@ -7921,7 +7922,7 @@ where
 						return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a channel_update for a channel from the wrong node - it shouldn't know about our private channels!".to_owned(), chan_id));
 					}
 					let were_node_one = self.get_our_node_id().serialize()[..] < chan.context.get_counterparty_node_id().serialize()[..];
-					let msg_from_node_one = msg.contents.flags & 1 == 0;
+					let msg_from_node_one = msg.contents.channel_flags & 1 == 0;
 					if were_node_one == msg_from_node_one {
 						return Ok(NotifyOption::SkipPersistNoEvents);
 					} else {
@@ -12262,8 +12263,8 @@ mod tests {
 		// update message and would always update the local fee info, even if our peer was
 		// (spuriously) forwarding us our own channel_update.
 		let as_node_one = nodes[0].node.get_our_node_id().serialize()[..] < nodes[1].node.get_our_node_id().serialize()[..];
-		let as_update = if as_node_one == (chan.0.contents.flags & 1 == 0 /* chan.0 is from node one */) { &chan.0 } else { &chan.1 };
-		let bs_update = if as_node_one == (chan.0.contents.flags & 1 == 0 /* chan.0 is from node one */) { &chan.1 } else { &chan.0 };
+		let as_update = if as_node_one == (chan.0.contents.channel_flags & 1 == 0 /* chan.0 is from node one */) { &chan.0 } else { &chan.1 };
+		let bs_update = if as_node_one == (chan.0.contents.channel_flags & 1 == 0 /* chan.0 is from node one */) { &chan.1 } else { &chan.0 };
 
 		// First deliver each peers' own message, checking that the node doesn't need to be
 		// persisted and that its channel info remains the same.

lightning/src/ln/functional_test_utils.rs

@@ -1554,7 +1554,7 @@ macro_rules! get_closing_signed_broadcast {
 			assert!(events.len() == 1 || events.len() == 2);
 			(match events[events.len() - 1] {
 				MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
-					assert_eq!(msg.contents.flags & 2, 2);
+					assert_eq!(msg.contents.channel_flags & 2, 2);
 					msg.clone()
 				},
 				_ => panic!("Unexpected event"),
@@ -1613,7 +1613,7 @@ pub fn check_closed_broadcast(node: &Node, num_channels: usize, with_error_msg:
 	msg_events.into_iter().filter_map(|msg_event| {
 		match msg_event {
 			MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
-				assert_eq!(msg.contents.flags & 2, 2);
+				assert_eq!(msg.contents.channel_flags & 2, 2);
 				None
 			},
 			MessageSendEvent::HandleError { action: msgs::ErrorAction::SendErrorMessage { msg }, node_id: _ } => {

lightning/src/ln/functional_tests.rs

@@ -7405,7 +7405,7 @@ fn test_announce_disable_channels() {
 	for e in msg_events {
 		match e {
 			MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
-				assert_eq!(msg.contents.flags & (1<<1), 1<<1); // The "channel disabled" bit should be set
+				assert_eq!(msg.contents.channel_flags & (1<<1), 1<<1); // The "channel disabled" bit should be set
 				// Check that each channel gets updated exactly once
 				if chans_disabled.insert(msg.contents.short_channel_id, msg.contents.timestamp).is_some() {
 					panic!("Generated ChannelUpdate for wrong chan!");
@@ -7452,7 +7452,7 @@ fn test_announce_disable_channels() {
 	for e in msg_events {
 		match e {
 			MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
-				assert_eq!(msg.contents.flags & (1<<1), 0); // The "channel disabled" bit should be off
+				assert_eq!(msg.contents.channel_flags & (1<<1), 0); // The "channel disabled" bit should be off
 				match chans_disabled.remove(&msg.contents.short_channel_id) {
 					// Each update should have a higher timestamp than the previous one, replacing
 					// the old one.
@@ -9398,13 +9398,13 @@ fn test_error_chans_closed() {
 	assert_eq!(events.len(), 2);
 	match events[0] {
 		MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
-			assert_eq!(msg.contents.flags & 2, 2);
+			assert_eq!(msg.contents.channel_flags & 2, 2);
 		},
 		_ => panic!("Unexpected event"),
 	}
 	match events[1] {
 		MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
-			assert_eq!(msg.contents.flags & 2, 2);
+			assert_eq!(msg.contents.channel_flags & 2, 2);
 		},
 		_ => panic!("Unexpected event"),
 	}

lightning/src/ln/msgs.rs

@@ -1244,8 +1244,11 @@ pub struct UnsignedChannelUpdate {
 	pub short_channel_id: u64,
 	/// A strictly monotonic announcement counter, with gaps allowed, specific to this channel
 	pub timestamp: u32,
-	/// Channel flags
-	pub flags: u8,
+	/// Flags pertaining to this message.
+	pub message_flags: u8,
+	/// Flags pertaining to the channel, including to which direction in the channel this update
+	/// applies and whether the direction is currently able to forward HTLCs.
+	pub channel_flags: u8,
 	/// The number of blocks such that if:
 	/// `incoming_htlc.cltv_expiry < outgoing_htlc.cltv_expiry + cltv_expiry_delta`
 	/// then we need to fail the HTLC backwards. When forwarding an HTLC, `cltv_expiry_delta` determines
@@ -2896,13 +2899,13 @@ impl_writeable!(ChannelAnnouncement, {
 
 impl Writeable for UnsignedChannelUpdate {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
-		// `message_flags` used to indicate presence of `htlc_maximum_msat`, but was deprecated in the spec.
-		const MESSAGE_FLAGS: u8 = 1;
 		self.chain_hash.write(w)?;
 		self.short_channel_id.write(w)?;
 		self.timestamp.write(w)?;
-		let all_flags = self.flags as u16 | ((MESSAGE_FLAGS as u16) << 8);
-		all_flags.write(w)?;
+		// Thw low bit of message_flags used to indicate the presence of `htlc_maximum_msat`, and
+		// now must be set
+		(self.message_flags | 1).write(w)?;
+		self.channel_flags.write(w)?;
 		self.cltv_expiry_delta.write(w)?;
 		self.htlc_minimum_msat.write(w)?;
 		self.fee_base_msat.write(w)?;
@@ -2915,22 +2918,26 @@ impl Writeable for UnsignedChannelUpdate {
 
 impl Readable for UnsignedChannelUpdate {
 	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
-		Ok(Self {
+		let res = Self {
 			chain_hash: Readable::read(r)?,
 			short_channel_id: Readable::read(r)?,
 			timestamp: Readable::read(r)?,
-			flags: {
-				let flags: u16 = Readable::read(r)?;
-				// Note: we ignore the `message_flags` for now, since it was deprecated by the spec.
-				flags as u8
-			},
+			message_flags: Readable::read(r)?,
+			channel_flags: Readable::read(r)?,
 			cltv_expiry_delta: Readable::read(r)?,
 			htlc_minimum_msat: Readable::read(r)?,
 			fee_base_msat: Readable::read(r)?,
 			fee_proportional_millionths: Readable::read(r)?,
 			htlc_maximum_msat: Readable::read(r)?,
 			excess_data: read_to_end(r)?,
-		})
+		};
+		if res.message_flags & 1 != 1 {
+			// The `must_be_one` flag should be set (historically it indicated the presence of the
+			// `htlc_maximum_msat` field, which is now required).
+			Err(DecodeError::InvalidValue)
+		} else {
+			Ok(res)
+		}
 	}
 }
 
@@ -3526,7 +3533,8 @@ mod tests {
 			chain_hash: ChainHash::using_genesis_block(Network::Bitcoin),
 			short_channel_id: 2316138423780173,
 			timestamp: 20190119,
-			flags: if direction { 1 } else { 0 } | if disable { 1 << 1 } else { 0 },
+			message_flags: 1, // Only must_be_one
+			channel_flags: if direction { 1 } else { 0 } | if disable { 1 << 1 } else { 0 },
 			cltv_expiry_delta: 144,
 			htlc_minimum_msat: 1000000,
 			htlc_maximum_msat: 131355275467161,

lightning/src/ln/onion_route_tests.rs

@@ -238,7 +238,8 @@ impl msgs::ChannelUpdate {
 				chain_hash: ChainHash::from(BlockHash::hash(&vec![0u8][..]).as_ref()),
 				short_channel_id,
 				timestamp: 0,
-				flags: 0,
+				message_flags: 1, // Only must_be_one
+				channel_flags: 0,
 				cltv_expiry_delta: 0,
 				htlc_minimum_msat: 0,
 				htlc_maximum_msat: msgs::MAX_VALUE_MSAT,

lightning/src/ln/priv_short_conf_tests.rs

@@ -508,7 +508,8 @@ fn test_scid_alias_returned() {
 		chain_hash: ChainHash::using_genesis_block(Network::Testnet),
 		short_channel_id: last_hop[0].inbound_scid_alias.unwrap(),
 		timestamp: 21,
-		flags: 1,
+		message_flags: 1, // Only must_be_one
+		channel_flags: 1,
 		cltv_expiry_delta: accept_forward_cfg.channel_config.cltv_expiry_delta,
 		htlc_minimum_msat: 1_000,
 		htlc_maximum_msat: 1_000_000, // Defaults to 10% of the channel value

lightning/src/ln/shutdown_tests.rs

@@ -577,7 +577,7 @@ fn do_test_shutdown_rebroadcast(recv_count: u8) {
 		node_0_2nd_shutdown
 	} else {
 		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.
+		assert_eq!(node_0_chan_update.contents.channel_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(), &node_1_2nd_shutdown);
 		get_event_msg!(nodes[0], MessageSendEvent::SendShutdown, nodes[1].node.get_our_node_id())
 	};
@@ -1188,7 +1188,7 @@ fn do_test_closing_signed_reinit_timeout(timeout_step: TimeoutStep) {
 		assert_eq!(events.len(), 1);
 		match events[0] {
 			MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
-				assert_eq!(msg.contents.flags & 2, 2);
+				assert_eq!(msg.contents.channel_flags & 2, 2);
 			},
 			_ => panic!("Unexpected event"),
 		}