f - Check KeysInterface::get_shutdown_scriptpubkey

Check that the user-provided shutdown_scriptpubkey is compatible with
the counterparty's features (option_shutdown_anysegwit).

TODO: Implement for ChannelManager::close_channel
TODO: Add unit tests

Author: jkczyz

lightning/src/ln/channel.rs

@@ -567,7 +567,7 @@ impl<Signer: Sign> Channel<Signer> {
 	}
 
 	// Constructors:
-	pub fn new_outbound<K: Deref, F: Deref>(fee_estimator: &F, keys_provider: &K, counterparty_node_id: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_id: u64, config: &UserConfig) -> Result<Channel<Signer>, APIError>
+	pub fn new_outbound<K: Deref, F: Deref>(fee_estimator: &F, keys_provider: &K, counterparty_node_id: PublicKey, their_features: InitFeatures, channel_value_satoshis: u64, push_msat: u64, user_id: u64, config: &UserConfig) -> Result<Channel<Signer>, APIError>
 	where K::Target: KeysInterface<Signer = Signer>,
 	      F::Target: FeeEstimator,
 	{
@@ -597,9 +597,13 @@ impl<Signer: Sign> Channel<Signer> {
 
 		let shutdown_scriptpubkey = if config.channel_options.commit_upfront_shutdown_pubkey {
 			Some(keys_provider.get_shutdown_scriptpubkey())
-		} else {
-			None
-		};
+		} else { None };
+
+		if let Some(shutdown_scriptpubkey) = &shutdown_scriptpubkey {
+			if !shutdown_scriptpubkey.is_compatible(&their_features) {
+				return Err(APIError::APIMisuseError { err: format!("Provided a scriptpubkey format not accepted by peer. script: ({})", shutdown_scriptpubkey.clone().into_inner().to_bytes().to_hex()) });
+			}
+		}
 
 		Ok(Channel {
 			user_id,
@@ -843,14 +847,18 @@ impl<Signer: Sign> Channel<Signer> {
 			}
 		} else { None };
 
-		let mut secp_ctx = Secp256k1::new();
-		secp_ctx.seeded_randomize(&keys_provider.get_secure_random_bytes());
-
 		let shutdown_scriptpubkey = if config.channel_options.commit_upfront_shutdown_pubkey {
 			Some(keys_provider.get_shutdown_scriptpubkey())
-		} else {
-			None
-		};
+		} else { None };
+
+		if let Some(shutdown_scriptpubkey) = &shutdown_scriptpubkey {
+			if !shutdown_scriptpubkey.is_compatible(&their_features) {
+				return Err(ChannelError::Close(format!("Provided a scriptpubkey format not accepted by peer. script: ({})", shutdown_scriptpubkey.clone().into_inner().to_bytes().to_hex())));
+			}
+		}
+
+		let mut secp_ctx = Secp256k1::new();
+		secp_ctx.seeded_randomize(&keys_provider.get_secure_random_bytes());
 
 		let chan = Channel {
 			user_id,
@@ -3262,6 +3270,23 @@ impl<Signer: Sign> Channel<Signer> {
 			self.counterparty_shutdown_scriptpubkey = Some(shutdown_scriptpubkey);
 		}
 
+		// If we have any LocalAnnounced updates we'll probably just get back a update_fail_htlc
+		// immediately after the commitment dance, but we can send a Shutdown cause we won't send
+		// any further commitment updates after we set LocalShutdownSent.
+		let send_shutdown = (self.channel_state & ChannelState::LocalShutdownSent as u32) != ChannelState::LocalShutdownSent as u32;
+
+		let shutdown_scriptpubkey = match self.shutdown_scriptpubkey {
+			Some(_) => None,
+			None => {
+				assert!(send_shutdown);
+				let shutdown_scriptpubkey = keys_provider.get_shutdown_scriptpubkey();
+				if !shutdown_scriptpubkey.is_compatible(their_features) {
+					return Err(ChannelError::Close(format!("Provided a scriptpubkey format not accepted by peer. script: ({})", shutdown_scriptpubkey.clone().into_inner().to_bytes().to_hex())));
+				}
+				Some(shutdown_scriptpubkey)
+			},
+		};
+
 		// From here on out, we may not fail!
 
 		self.channel_state |= ChannelState::RemoteShutdownSent as u32;
@@ -3282,15 +3307,9 @@ impl<Signer: Sign> Channel<Signer> {
 			}
 		});
 
-		// If we have any LocalAnnounced updates we'll probably just get back a update_fail_htlc
-		// immediately after the commitment dance, but we can send a Shutdown cause we won't send
-		// any further commitment updates after we set LocalShutdownSent.
-		let send_shutdown = (self.channel_state & ChannelState::LocalShutdownSent as u32) != ChannelState::LocalShutdownSent as u32;
-		let monitor_update = match self.shutdown_scriptpubkey {
-			Some(_) => None,
-			None => {
-				assert!(send_shutdown);
-				self.shutdown_scriptpubkey = Some(keys_provider.get_shutdown_scriptpubkey());
+		let monitor_update = match shutdown_scriptpubkey {
+			Some(shutdown_scriptpubkey) => {
+				self.shutdown_scriptpubkey = Some(shutdown_scriptpubkey);
 				self.latest_monitor_update_id += 1;
 				Some(ChannelMonitorUpdate {
 					update_id: self.latest_monitor_update_id,
@@ -3299,6 +3318,7 @@ impl<Signer: Sign> Channel<Signer> {
 					}],
 				})
 			},
+			None => None,
 		};
 		let shutdown = if send_shutdown {
 			Some(msgs::Shutdown {
@@ -5212,7 +5232,7 @@ mod tests {
 
 		let node_a_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
 		let config = UserConfig::default();
-		let node_a_chan = Channel::<EnforcingSigner>::new_outbound(&&fee_est, &&keys_provider, node_a_node_id, 10000000, 100000, 42, &config).unwrap();
+		let node_a_chan = Channel::<EnforcingSigner>::new_outbound(&&fee_est, &&keys_provider, node_a_node_id, InitFeatures::known(), 10000000, 100000, 42, &config).unwrap();
 
 		// Now change the fee so we can check that the fee in the open_channel message is the
 		// same as the old fee.
@@ -5237,7 +5257,7 @@ mod tests {
 		// Create Node A's channel pointing to Node B's pubkey
 		let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
 		let config = UserConfig::default();
-		let mut node_a_chan = Channel::<EnforcingSigner>::new_outbound(&&feeest, &&keys_provider, node_b_node_id, 10000000, 100000, 42, &config).unwrap();
+		let mut node_a_chan = Channel::<EnforcingSigner>::new_outbound(&&feeest, &&keys_provider, node_b_node_id, InitFeatures::known(), 10000000, 100000, 42, &config).unwrap();
 
 		// Create Node B's channel by receiving Node A's open_channel message
 		// Make sure A's dust limit is as we expect.
@@ -5304,7 +5324,7 @@ mod tests {
 
 		let node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
 		let config = UserConfig::default();
-		let mut chan = Channel::<EnforcingSigner>::new_outbound(&&fee_est, &&keys_provider, node_id, 10000000, 100000, 42, &config).unwrap();
+		let mut chan = Channel::<EnforcingSigner>::new_outbound(&&fee_est, &&keys_provider, node_id, InitFeatures::known(), 10000000, 100000, 42, &config).unwrap();
 
 		let commitment_tx_fee_0_htlcs = chan.commit_tx_fee_msat(0);
 		let commitment_tx_fee_1_htlc = chan.commit_tx_fee_msat(1);
@@ -5353,7 +5373,7 @@ mod tests {
 		// Create Node A's channel pointing to Node B's pubkey
 		let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
 		let config = UserConfig::default();
-		let mut node_a_chan = Channel::<EnforcingSigner>::new_outbound(&&feeest, &&keys_provider, node_b_node_id, 10000000, 100000, 42, &config).unwrap();
+		let mut node_a_chan = Channel::<EnforcingSigner>::new_outbound(&&feeest, &&keys_provider, node_b_node_id, InitFeatures::known(), 10000000, 100000, 42, &config).unwrap();
 
 		// Create Node B's channel by receiving Node A's open_channel message
 		let open_channel_msg = node_a_chan.get_open_channel(chain_hash);
@@ -5415,7 +5435,7 @@ mod tests {
 		// Create a channel.
 		let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
 		let config = UserConfig::default();
-		let mut node_a_chan = Channel::<EnforcingSigner>::new_outbound(&&feeest, &&keys_provider, node_b_node_id, 10000000, 100000, 42, &config).unwrap();
+		let mut node_a_chan = Channel::<EnforcingSigner>::new_outbound(&&feeest, &&keys_provider, node_b_node_id, InitFeatures::known(), 10000000, 100000, 42, &config).unwrap();
 		assert!(node_a_chan.counterparty_forwarding_info.is_none());
 		assert_eq!(node_a_chan.holder_htlc_minimum_msat, 1); // the default
 		assert!(node_a_chan.counterparty_forwarding_info().is_none());
@@ -5479,7 +5499,7 @@ mod tests {
 		let counterparty_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
 		let mut config = UserConfig::default();
 		config.channel_options.announced_channel = false;
-		let mut chan = Channel::<InMemorySigner>::new_outbound(&&feeest, &&keys_provider, counterparty_node_id, 10_000_000, 100000, 42, &config).unwrap(); // Nothing uses their network key in this test
+		let mut chan = Channel::<InMemorySigner>::new_outbound(&&feeest, &&keys_provider, counterparty_node_id, InitFeatures::known(), 10_000_000, 100000, 42, &config).unwrap(); // Nothing uses their network key in this test
 		chan.holder_dust_limit_satoshis = 546;
 		chan.counterparty_selected_channel_reserve_satoshis = Some(0); // Filled in in accept_channel
 

lightning/src/ln/channelmanager.rs

@@ -1156,8 +1156,13 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			return Err(APIError::APIMisuseError { err: format!("Channel value must be at least 1000 satoshis. It was {}", channel_value_satoshis) });
 		}
 
+		let their_features = {
+			let per_peer_state = self.per_peer_state.read().unwrap();
+			let peer_state = per_peer_state.get(&their_network_key).unwrap().lock().unwrap();
+			peer_state.latest_features.clone()
+		};
 		let config = if override_config.is_some() { override_config.as_ref().unwrap() } else { &self.default_configuration };
-		let channel = Channel::new_outbound(&self.fee_estimator, &self.keys_manager, their_network_key, channel_value_satoshis, push_msat, user_id, config)?;
+		let channel = Channel::new_outbound(&self.fee_estimator, &self.keys_manager, their_network_key, their_features, channel_value_satoshis, push_msat, user_id, config)?;
 		let res = channel.get_open_channel(self.genesis_hash.clone());
 
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);

lightning/src/ln/functional_tests.rs

@@ -7748,7 +7748,7 @@ fn test_user_configurable_csv_delay() {
 	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
 	// We test config.our_to_self > BREAKDOWN_TIMEOUT is enforced in Channel::new_outbound()
-	if let Err(error) = Channel::new_outbound(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), 1000000, 1000000, 0, &low_our_to_self_config) {
+	if let Err(error) = Channel::new_outbound(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), InitFeatures::known(), 1000000, 1000000, 0, &low_our_to_self_config) {
 		match error {
 			APIError::APIMisuseError { err } => { assert!(regex::Regex::new(r"Configured with an unreasonable our_to_self_delay \(\d+\) putting user funds at risks").unwrap().is_match(err.as_str())); },
 			_ => panic!("Unexpected event"),