Require option_shutdown_anysegwit feature

Author: jkczyz

lightning/src/ln/features.rs

@@ -123,7 +123,7 @@ mod sealed {
 			// Byte 2
 			,
 			// Byte 3
-			,
+			ShutdownAnySegwit,
 		],
 		optional_features: [
 			// Byte 0
@@ -133,7 +133,7 @@ mod sealed {
 			// Byte 2
 			BasicMPP,
 			// Byte 3
-			ShutdownAnySegwit,
+			,
 		],
 	});
 	define_context!(NodeContext {
@@ -145,7 +145,7 @@ mod sealed {
 			// Byte 2
 			,
 			// Byte 3
-			,
+			ShutdownAnySegwit,
 			// Byte 4
 			,
 			// Byte 5
@@ -161,7 +161,7 @@ mod sealed {
 			// Byte 2
 			BasicMPP,
 			// Byte 3
-			ShutdownAnySegwit,
+			,
 			// Byte 4
 			,
 			// Byte 5
@@ -684,11 +684,6 @@ impl<T: sealed::ShutdownAnySegwit> Features<T> {
 	pub(crate) fn supports_shutdown_anysegwit(&self) -> bool {
 		<T as sealed::ShutdownAnySegwit>::supports_feature(&self.flags)
 	}
-	#[cfg(test)]
-	pub(crate) fn clear_shutdown_anysegwit(mut self) -> Self {
-		<T as sealed::ShutdownAnySegwit>::clear_bits(&mut self.flags);
-		self
-	}
 }
 
 impl<T: sealed::Context> Writeable for Features<T> {
@@ -805,12 +800,12 @@ mod tests {
 			// - option_data_loss_protect
 			// - var_onion_optin (req) | static_remote_key (req) | payment_secret(req)
 			// - basic_mpp
-			// - opt_shutdown_anysegwit
+			// - opt_shutdown_anysegwit (req)
 			assert_eq!(node_features.flags.len(), 4);
 			assert_eq!(node_features.flags[0], 0b00000010);
 			assert_eq!(node_features.flags[1], 0b01010001);
 			assert_eq!(node_features.flags[2], 0b00000010);
-			assert_eq!(node_features.flags[3], 0b00001000);
+			assert_eq!(node_features.flags[3], 0b00000100);
 		}
 
 		// Check that cleared flags are kept blank when converting back:

lightning/src/ln/functional_tests.rs

@@ -7518,24 +7518,20 @@ fn test_upfront_shutdown_script() {
 }
 
 #[test]
-fn test_upfront_shutdown_script_unsupport_segwit() {
-	// We test that channel is closed early
-	// if a segwit program is passed as upfront shutdown script,
-	// but the peer does not support segwit.
+fn test_invalid_upfront_shutdown_script() {
 	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);
 
 	nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None).unwrap();
 
+	// Use a segwit v0 script with an unsupported witness program
 	let mut open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
-	open_channel.shutdown_scriptpubkey = Present(Builder::new().push_int(16)
+	open_channel.shutdown_scriptpubkey = Present(Builder::new().push_int(0)
 		.push_slice(&[0, 0])
 		.into_script());
-
-	let features = InitFeatures::known().clear_shutdown_anysegwit();
-	nodes[0].node.handle_open_channel(&nodes[0].node.get_our_node_id(), features, &open_channel);
+	nodes[0].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &open_channel);
 
 	let events = nodes[0].node.get_and_clear_pending_msg_events();
 	assert_eq!(events.len(), 1);
@@ -7549,7 +7545,7 @@ fn test_upfront_shutdown_script_unsupport_segwit() {
 }
 
 #[test]
-fn test_shutdown_script_any_segwit_allowed() {
+fn test_segwit_v0_shutdown_script() {
 	let mut config = UserConfig::default();
 	config.channel_options.announced_channel = true;
 	config.peer_channel_config_limits.force_announced_channel_preference = false;
@@ -7560,14 +7556,16 @@ fn test_shutdown_script_any_segwit_allowed() {
 	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &user_cfgs);
 	let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
 
-	//// We test if the remote peer accepts opt_shutdown_anysegwit, a witness program can be used on shutdown
 	let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1000000, 1000000, InitFeatures::known(), InitFeatures::known());
 	nodes[1].node.close_channel(&OutPoint { txid: chan.3.txid(), index: 0 }.to_channel_id()).unwrap();
+
+	// Use a segwit v0 script supported even without opt_shutdown_anysegwit
 	let mut node_0_shutdown = get_event_msg!(nodes[1], MessageSendEvent::SendShutdown, nodes[0].node.get_our_node_id());
-	node_0_shutdown.scriptpubkey = Builder::new().push_int(16)
-		.push_slice(&[0, 0])
+	node_0_shutdown.scriptpubkey = Builder::new().push_int(0)
+		.push_slice(&[0; 20])
 		.into_script();
 	nodes[0].node.handle_shutdown(&nodes[1].node.get_our_node_id(), &InitFeatures::known(), &node_0_shutdown);
+
 	let events = nodes[0].node.get_and_clear_pending_msg_events();
 	assert_eq!(events.len(), 2);
 	match events[0] {
@@ -7581,7 +7579,7 @@ fn test_shutdown_script_any_segwit_allowed() {
 }
 
 #[test]
-fn test_shutdown_script_any_segwit_not_allowed() {
+fn test_anysegwit_shutdown_script() {
 	let mut config = UserConfig::default();
 	config.channel_options.announced_channel = true;
 	config.peer_channel_config_limits.force_announced_channel_preference = false;
@@ -7592,30 +7590,30 @@ fn test_shutdown_script_any_segwit_not_allowed() {
 	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &user_cfgs);
 	let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
 
-	//// We test that if the remote peer does not accept opt_shutdown_anysegwit, the witness program cannot be used on shutdown
 	let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1000000, 1000000, InitFeatures::known(), InitFeatures::known());
 	nodes[1].node.close_channel(&OutPoint { txid: chan.3.txid(), index: 0 }.to_channel_id()).unwrap();
+
+	// Use a non-v0 segwit script supported by opt_shutdown_anysegwit
 	let mut node_0_shutdown = get_event_msg!(nodes[1], MessageSendEvent::SendShutdown, nodes[0].node.get_our_node_id());
-	// Make an any segwit version script
 	node_0_shutdown.scriptpubkey = Builder::new().push_int(16)
 		.push_slice(&[0, 0])
 		.into_script();
-	let flags_no = InitFeatures::known().clear_shutdown_anysegwit();
-	nodes[0].node.handle_shutdown(&nodes[1].node.get_our_node_id(), &flags_no, &node_0_shutdown);
+	nodes[0].node.handle_shutdown(&nodes[1].node.get_our_node_id(), &InitFeatures::known(), &node_0_shutdown);
+
 	let events = nodes[0].node.get_and_clear_pending_msg_events();
 	assert_eq!(events.len(), 2);
+	match events[0] {
+		MessageSendEvent::SendShutdown { node_id, .. } => { assert_eq!(node_id, nodes[1].node.get_our_node_id()) }
+		_ => panic!("Unexpected event"),
+	}
 	match events[1] {
-		MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { ref msg }, node_id } => {
-			assert_eq!(node_id, nodes[1].node.get_our_node_id());
-			assert_eq!(msg.data, "Got a nonstandard scriptpubkey (60020000) from remote peer".to_owned())
-		},
+		MessageSendEvent::SendClosingSigned { node_id, .. } => { assert_eq!(node_id, nodes[1].node.get_our_node_id()) }
 		_ => panic!("Unexpected event"),
 	}
-	check_added_monitors!(nodes[0], 1);
 }
 
 #[test]
-fn test_shutdown_script_segwit_but_not_anysegwit() {
+fn test_invalid_shutdown_script() {
 	let mut config = UserConfig::default();
 	config.channel_options.announced_channel = true;
 	config.peer_channel_config_limits.force_announced_channel_preference = false;
@@ -7626,15 +7624,16 @@ fn test_shutdown_script_segwit_but_not_anysegwit() {
 	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &user_cfgs);
 	let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
 
-	//// We test that if shutdown any segwit is supported and we send a witness script with 0 version, this is not accepted
 	let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1000000, 1000000, InitFeatures::known(), InitFeatures::known());
 	nodes[1].node.close_channel(&OutPoint { txid: chan.3.txid(), index: 0 }.to_channel_id()).unwrap();
+
+	// Use a segwit v0 script with an unsupported witness program
 	let mut node_0_shutdown = get_event_msg!(nodes[1], MessageSendEvent::SendShutdown, nodes[0].node.get_our_node_id());
-	// Make a segwit script that is not a valid as any segwit
 	node_0_shutdown.scriptpubkey = Builder::new().push_int(0)
 		.push_slice(&[0, 0])
 		.into_script();
 	nodes[0].node.handle_shutdown(&nodes[1].node.get_our_node_id(), &InitFeatures::known(), &node_0_shutdown);
+
 	let events = nodes[0].node.get_and_clear_pending_msg_events();
 	assert_eq!(events.len(), 2);
 	match events[1] {

lightning/src/ln/peer_handler.rs

@@ -897,6 +897,10 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
 					log_debug!(self.logger, "Peer {} does not support static remote key, disconnecting with no_connection_possible", log_pubkey!(peer.their_node_id.unwrap()));
 					return Err(PeerHandleError{ no_connection_possible: true }.into());
 				}
+				if !msg.features.supports_shutdown_anysegwit() {
+					log_debug!(self.logger, "Peer {} does not support option_shutdown_anysegwit, disconnecting with no_connection_possible", log_pubkey!(peer.their_node_id.unwrap()));
+					return Err(PeerHandleError{ no_connection_possible: true }.into());
+				}
 
 				self.message_handler.route_handler.sync_routing_table(&peer.their_node_id.unwrap(), &msg);