Minor updates after feedback

* An empty script is only allowed when sent as upfront shutdown script,
so make sure that check is only done for accept/open_channel situations.
* Instead of reimplementing a variant of is_witness_script,
just call it and verify that the witness version is not 0.
* Fixed an outdated comment after a test copy/paste.

Author: galderz

lightning/src/ln/channel.rs

@@ -45,6 +45,7 @@ use std::ops::Deref;
 #[cfg(any(test, feature = "fuzztarget"))]
 use std::sync::Mutex;
 use bitcoin::hashes::hex::ToHex;
+use bitcoin::blockdata::opcodes::all::OP_PUSHBYTES_0;
 
 #[cfg(test)]
 pub struct ChannelValueStat {
@@ -737,12 +738,12 @@ impl<Signer: Sign> Channel<Signer> {
 		let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() {
 			match &msg.shutdown_scriptpubkey {
 				&OptionalField::Present(ref script) => {
-					if is_unsupported_shutdown_script(&their_features, script) {
-						return Err(ChannelError::Close(format!("Peer is signaling upfront_shutdown but has provided a non-accepted scriptpubkey format. script: ({})", script.to_bytes().to_hex())));
-					}
-
+					// Peer is signaling upfront_shutdown and has opt-out with a 0-length script. We don't enforce anything
 					if script.len() == 0 {
 						None
+					// Peer is signaling upfront_shutdown and has provided a non-accepted scriptpubkey format. Fail the channel
+					} else if is_unsupported_shutdown_script(&their_features, script) {
+						return Err(ChannelError::Close(format!("Peer is signaling upfront_shutdown but has provided a non-accepted scriptpubkey format. script: ({})", script.to_bytes().to_hex())));
 					} else {
 						Some(script.clone())
 					}
@@ -1438,12 +1439,12 @@ impl<Signer: Sign> Channel<Signer> {
 		let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() {
 			match &msg.shutdown_scriptpubkey {
 				&OptionalField::Present(ref script) => {
-					if is_unsupported_shutdown_script(&their_features, script) {
-						return Err(ChannelError::Close(format!("Peer is signaling upfront_shutdown but has provided a non-accepted scriptpubkey format. script: ({})", script.to_bytes().to_hex())));
-					}
-
+					// Peer is signaling upfront_shutdown and has opt-out with a 0-length script. We don't enforce anything
 					if script.len() == 0 {
 						None
+					// Peer is signaling upfront_shutdown and has provided a non-accepted scriptpubkey format. Fail the channel
+					} else if is_unsupported_shutdown_script(&their_features, script) {
+						return Err(ChannelError::Close(format!("Peer is signaling upfront_shutdown but has provided a non-accepted scriptpubkey format. script: ({})", script.to_bytes().to_hex())));
 					} else {
 						Some(script.clone())
 					}
@@ -4211,40 +4212,16 @@ impl<Signer: Sign> Channel<Signer> {
 fn is_unsupported_shutdown_script(their_features: &InitFeatures, script: &Script) -> bool {
 	// We restrain shutdown scripts to standards forms to avoid transactions not propagating on the p2p tx-relay network
 
-	// Providing an empty is supported
-	let script_lenght = script.len();
-	if script_lenght == 0 {
-		return false;
-	}
-
 	// BOLT 2 says we must only send a scriptpubkey of certain standard forms,
 	// which for a a BIP-141-compliant witness program is at max 42 bytes in length.
 	// So don't let the remote peer feed us some super fee-heavy script.
-	let is_script_too_long = script_lenght > 42;
+	let is_script_too_long = script.len() > 42;
 	if is_script_too_long {
 		return true;
 	}
 
-	if their_features.supports_shutdown_anysegwit() {
-		// A scriptPubKey (or redeemScript as defined in BIP16/P2SH) that consists of a 1-byte
-		// push opcode (for 1 to 16) followed by a data push between 2 and 40 bytes gets a new
-		// special meaning.
-		let min_vernum: u8 = opcodes::all::OP_PUSHNUM_1.into_u8();
-		let max_vernum: u8 = opcodes::all::OP_PUSHNUM_16.into_u8();
-		let script_bytes = script.as_bytes();
-		let is_anysegwit = script.len() >= 3
-			&& script.len() <= 42
-			// PUSHNUM_1-PUSHNUM_16
-			&& (script_bytes[0] >= min_vernum && script_bytes[0] <= max_vernum)
-			// Second byte push opcode 2-40 bytes
-			&& script_bytes[1] >= opcodes::all::OP_PUSHBYTES_2.into_u8()
-			&& script_bytes[1] <= opcodes::all::OP_PUSHBYTES_40.into_u8()
-			// Check that the rest of the script has the correct size
-			&& script.len() - 2 == script_bytes[1] as usize;
-
-		if is_anysegwit {
-			return false;
-		}
+	if their_features.supports_shutdown_anysegwit() && script.is_witness_program() && script.as_bytes()[0] != OP_PUSHBYTES_0.into_u8() {
+		return false;
 	}
 
 	return !script.is_p2pkh() && !script.is_p2sh() && !script.is_v0_p2wpkh() && !script.is_v0_p2wsh()

lightning/src/ln/functional_tests.rs

@@ -7369,7 +7369,7 @@ 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 the remote peer does not accept opt_shutdown_anysegwit, the witness program cannot be used on shutdown
+	//// 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();
 	let mut node_0_shutdown = get_event_msg!(nodes[1], MessageSendEvent::SendShutdown, nodes[0].node.get_our_node_id());