(Re-)add handling for ChannelUpdate::message_flags

When the `htlc_maximum_msat` field was made mandatory in
`ChannelUpdate` (in b0e8b73) we
started ignoring the `message_flags` field entirely and always
writing `1`. The comment updates indicated that the `message_flags`
field was deprecated, but this is not true - only the
`htlc_maximum_msat` indicator bit was deprecated, requiring it to
be 1.

If a node creates a `channel_update` with `message_flags` bits set
other than the low bit, this will cause us to spuriously reject
the message with an invalid signature error as we will check the
message against the wrong hash.

With today's current spec this is totally fine - the only other bit
defined for `message_flags` is the `dont_forward` bit, which when
set indicates we shouldn't accept the message into our gossip store
anyway (though this may lead to spurious `warning` messages being
sent to peers). However, in the future this may mean we start
rejecting valid `channel_update`s if new bits are defiend.

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));
 	}
 }

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

@@ -9894,7 +9894,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

@@ -3505,7 +3505,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));
@@ -3784,7 +3784,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(),
@@ -7920,7 +7921,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 {
@@ -12236,8 +12237,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

@@ -7403,7 +7403,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!");
@@ -7450,7 +7450,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.
@@ -9396,13 +9396,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

@@ -1227,8 +1227,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
@@ -2866,13 +2869,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)?;
@@ -2885,22 +2888,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)
+		}
 	}
 }
 
@@ -3496,7 +3503,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,