Add support for opt_shutdown_anysegwit feature #780

* Implemented protocol.
* Made feature optional.
* Verify that the default value is true.
* Verify that on shutdown,
if Channel.supports_shutdown_anysegwit is enabled,
the script can be a witness program.
* Added a test that verifies that a scriptpubkey
for an unreleased segwit version is handled successfully.
* Added a test that verifies that
if node has op_shutdown_anysegwit disabled,
a scriptpubkey with an unreleased segwit version on shutdown
throws an error.
* Added peer InitFeatures to handle_shutdown
* Check if shutdown script is valid when given upfront.
* Added a test to verify that an invalid test results in error.
* Added a test to check that if a segwit script with version 0 is provided,
the updated anysegwit check detects it and returns unsupported.
* 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.

Author: galderz

lightning-net-tokio/src/lib.rs

@@ -551,7 +551,7 @@ mod tests {
 		fn handle_funding_created(&self, _their_node_id: &PublicKey, _msg: &FundingCreated) {}
 		fn handle_funding_signed(&self, _their_node_id: &PublicKey, _msg: &FundingSigned) {}
 		fn handle_funding_locked(&self, _their_node_id: &PublicKey, _msg: &FundingLocked) {}
-		fn handle_shutdown(&self, _their_node_id: &PublicKey, _msg: &Shutdown) {}
+		fn handle_shutdown(&self, _their_node_id: &PublicKey, _their_features: &InitFeatures, _msg: &Shutdown) {}
 		fn handle_closing_signed(&self, _their_node_id: &PublicKey, _msg: &ClosingSigned) {}
 		fn handle_update_add_htlc(&self, _their_node_id: &PublicKey, _msg: &UpdateAddHTLC) {}
 		fn handle_update_fulfill_htlc(&self, _their_node_id: &PublicKey, _msg: &UpdateFulfillHTLC) {}

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,15 +738,14 @@ impl<Signer: Sign> Channel<Signer> {
 		let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() {
 			match &msg.shutdown_scriptpubkey {
 				&OptionalField::Present(ref script) => {
-					// Peer is signaling upfront_shutdown and has provided a non-accepted scriptpubkey format. We enforce it while receiving shutdown msg
-					if script.is_p2pkh() || script.is_p2sh() || script.is_v0_p2wsh() || script.is_v0_p2wpkh() {
-						Some(script.clone())
 					// Peer is signaling upfront_shutdown and has opt-out with a 0-length script. We don't enforce anything
-					} else if script.len() == 0 {
+					if script.len() == 0 {
 						None
 					// Peer is signaling upfront_shutdown and has provided a non-accepted scriptpubkey format. Fail the channel
-					} else {
+					} 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())
 					}
 				},
 				// Peer is signaling upfront shutdown but don't opt-out with correct mechanism (a.k.a 0-length script). Peer looks buggy, we fail the channel
@@ -1439,15 +1439,14 @@ impl<Signer: Sign> Channel<Signer> {
 		let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() {
 			match &msg.shutdown_scriptpubkey {
 				&OptionalField::Present(ref script) => {
-					// Peer is signaling upfront_shutdown and has provided a non-accepted scriptpubkey format. We enforce it while receiving shutdown msg
-					if script.is_p2pkh() || script.is_p2sh() || script.is_v0_p2wsh() || script.is_v0_p2wpkh() {
-						Some(script.clone())
 					// Peer is signaling upfront_shutdown and has opt-out with a 0-length script. We don't enforce anything
-					} else if script.len() == 0 {
+					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 {
-						return Err(ChannelError::Close(format!("Peer is signaling upfront_shutdown but has provided a non-accepted scriptpubkey format. scriptpubkey: ({})", script.to_bytes().to_hex())));
+						Some(script.clone())
 					}
 				},
 				// Peer is signaling upfront shutdown but don't opt-out with correct mechanism (a.k.a 0-length script). Peer looks buggy, we fail the channel
@@ -3066,7 +3065,7 @@ impl<Signer: Sign> Channel<Signer> {
 		})
 	}
 
-	pub fn shutdown<F: Deref>(&mut self, fee_estimator: &F, msg: &msgs::Shutdown) -> Result<(Option<msgs::Shutdown>, Option<msgs::ClosingSigned>, Vec<(HTLCSource, PaymentHash)>), ChannelError>
+	pub fn shutdown<F: Deref>(&mut self, fee_estimator: &F, their_features: &InitFeatures, msg: &msgs::Shutdown) -> Result<(Option<msgs::Shutdown>, Option<msgs::ClosingSigned>, Vec<(HTLCSource, PaymentHash)>), ChannelError>
 		where F::Target: FeeEstimator
 	{
 		if self.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 {
@@ -3085,14 +3084,7 @@ impl<Signer: Sign> Channel<Signer> {
 		}
 		assert_eq!(self.channel_state & ChannelState::ShutdownComplete as u32, 0);
 
-		// BOLT 2 says we must only send a scriptpubkey of certain standard forms, which are up to
-		// 34 bytes in length, so don't let the remote peer feed us some super fee-heavy script.
-		if self.is_outbound() && msg.scriptpubkey.len() > 34 {
-			return Err(ChannelError::Close(format!("Got counterparty shutdown_scriptpubkey ({}) of absurd length from remote peer", msg.scriptpubkey.to_bytes().to_hex())));
-		}
-
-		//Check counterparty_shutdown_scriptpubkey form as BOLT says we must
-		if !msg.scriptpubkey.is_p2pkh() && !msg.scriptpubkey.is_p2sh() && !msg.scriptpubkey.is_v0_p2wpkh() && !msg.scriptpubkey.is_v0_p2wsh() {
+		if is_unsupported_shutdown_script(&their_features, &msg.scriptpubkey) {
 			return Err(ChannelError::Close(format!("Got a nonstandard scriptpubkey ({}) from remote peer", msg.scriptpubkey.to_bytes().to_hex())));
 		}
 
@@ -4217,6 +4209,24 @@ 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
+
+	// 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.len() > 42;
+	if is_script_too_long {
+		return true;
+	}
+
+	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()
+}
+
 const SERIALIZATION_VERSION: u8 = 1;
 const MIN_SERIALIZATION_VERSION: u8 = 1;
 

lightning/src/ln/channelmanager.rs

@@ -2507,7 +2507,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		}
 	}
 
-	fn internal_shutdown(&self, counterparty_node_id: &PublicKey, msg: &msgs::Shutdown) -> Result<(), MsgHandleErrInternal> {
+	fn internal_shutdown(&self, counterparty_node_id: &PublicKey, their_features: &InitFeatures, msg: &msgs::Shutdown) -> Result<(), MsgHandleErrInternal> {
 		let (mut dropped_htlcs, chan_option) = {
 			let mut channel_state_lock = self.channel_state.lock().unwrap();
 			let channel_state = &mut *channel_state_lock;
@@ -2517,7 +2517,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					if chan_entry.get().get_counterparty_node_id() != *counterparty_node_id {
 						return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id));
 					}
-					let (shutdown, closing_signed, dropped_htlcs) = try_chan_entry!(self, chan_entry.get_mut().shutdown(&self.fee_estimator, &msg), channel_state, chan_entry);
+					let (shutdown, closing_signed, dropped_htlcs) = try_chan_entry!(self, chan_entry.get_mut().shutdown(&self.fee_estimator, &their_features, &msg), channel_state, chan_entry);
 					if let Some(msg) = shutdown {
 						channel_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
 							node_id: counterparty_node_id.clone(),
@@ -3351,9 +3351,9 @@ impl<Signer: Sign, M: Deref + Sync + Send, T: Deref + Sync + Send, K: Deref + Sy
 		let _ = handle_error!(self, self.internal_funding_locked(counterparty_node_id, msg), *counterparty_node_id);
 	}
 
-	fn handle_shutdown(&self, counterparty_node_id: &PublicKey, msg: &msgs::Shutdown) {
+	fn handle_shutdown(&self, counterparty_node_id: &PublicKey, their_features: &InitFeatures, msg: &msgs::Shutdown) {
 		let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
-		let _ = handle_error!(self, self.internal_shutdown(counterparty_node_id, msg), *counterparty_node_id);
+		let _ = handle_error!(self, self.internal_shutdown(counterparty_node_id, their_features, msg), *counterparty_node_id);
 	}
 
 	fn handle_closing_signed(&self, counterparty_node_id: &PublicKey, msg: &msgs::ClosingSigned) {

lightning/src/ln/features.rs

@@ -100,6 +100,8 @@ mod sealed {
 			StaticRemoteKey,
 			// Byte 2
 			,
+			// Byte 3
+			,
 		],
 		optional_features: [
 			// Byte 0
@@ -108,6 +110,8 @@ mod sealed {
 			VariableLengthOnion | PaymentSecret,
 			// Byte 2
 			BasicMPP,
+			// Byte 3
+			ShutdownAnySegwit,
 		],
 	});
 	define_context!(NodeContext {
@@ -118,6 +122,8 @@ mod sealed {
 			StaticRemoteKey,
 			// Byte 2
 			,
+			// Byte 3
+			,
 		],
 		optional_features: [
 			// Byte 0
@@ -126,6 +132,8 @@ mod sealed {
 			VariableLengthOnion | PaymentSecret,
 			// Byte 2
 			BasicMPP,
+			// Byte 3
+			ShutdownAnySegwit,
 		],
 	});
 	define_context!(ChannelContext {
@@ -252,6 +260,8 @@ mod sealed {
 		"Feature flags for `payment_secret`.");
 	define_feature!(17, BasicMPP, [InitContext, NodeContext],
 		"Feature flags for `basic_mpp`.");
+	define_feature!(27, ShutdownAnySegwit, [InitContext, NodeContext],
+		"Feature flags for `opt_shutdown_anysegwit`.");
 
 	#[cfg(test)]
 	define_context!(TestingContext {
@@ -549,6 +559,17 @@ impl<T: sealed::BasicMPP> Features<T> {
 	}
 }
 
+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> {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
 		w.size_hint(self.flags.len() + 2);
@@ -619,6 +640,9 @@ mod tests {
 		assert!(!InitFeatures::known().requires_basic_mpp());
 		assert!(!NodeFeatures::known().requires_basic_mpp());
 
+		assert!(InitFeatures::known().supports_shutdown_anysegwit());
+		assert!(NodeFeatures::known().supports_shutdown_anysegwit());
+
 		let mut init_features = InitFeatures::known();
 		assert!(init_features.initial_routing_sync());
 		init_features.clear_initial_routing_sync();
@@ -657,10 +681,12 @@ mod tests {
 			// - option_data_loss_protect
 			// - var_onion_optin | static_remote_key (req) | payment_secret
 			// - basic_mpp
-			assert_eq!(node_features.flags.len(), 3);
+			// - opt_shutdown_anysegwit
+			assert_eq!(node_features.flags.len(), 4);
 			assert_eq!(node_features.flags[0], 0b00000010);
 			assert_eq!(node_features.flags[1], 0b10010010);
 			assert_eq!(node_features.flags[2], 0b00000010);
+			assert_eq!(node_features.flags[3], 0b00001000);
 		}
 
 		// Check that cleared flags are kept blank when converting back:

lightning/src/ln/functional_test_utils.rs

@@ -604,7 +604,7 @@ pub fn close_channel<'a, 'b, 'c>(outbound_node: &Node<'a, 'b, 'c>, inbound_node:
 	let (tx_a, tx_b);
 
 	node_a.close_channel(channel_id).unwrap();
-	node_b.handle_shutdown(&node_a.get_our_node_id(), &get_event_msg!(struct_a, MessageSendEvent::SendShutdown, node_b.get_our_node_id()));
+	node_b.handle_shutdown(&node_a.get_our_node_id(), &InitFeatures::known(), &get_event_msg!(struct_a, MessageSendEvent::SendShutdown, node_b.get_our_node_id()));
 
 	let events_1 = node_b.get_and_clear_pending_msg_events();
 	assert!(events_1.len() >= 1);
@@ -629,7 +629,7 @@ pub fn close_channel<'a, 'b, 'c>(outbound_node: &Node<'a, 'b, 'c>, inbound_node:
 		})
 	};
 
-	node_a.handle_shutdown(&node_b.get_our_node_id(), &shutdown_b);
+	node_a.handle_shutdown(&node_b.get_our_node_id(), &InitFeatures::known(), &shutdown_b);
 	let (as_update, bs_update) = if close_inbound_first {
 		assert!(node_a.get_and_clear_pending_msg_events().is_empty());
 		node_a.handle_closing_signed(&node_b.get_our_node_id(), &closing_signed_b.unwrap());