Merge pull request #1019 from jkczyz/2021-07-shutdown-pubkey

Fetch shutdown script based on `commit_upfront_shutdown_pubkey`

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -39,6 +39,7 @@ use lightning::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
 use lightning::ln::channelmanager::{ChainParameters, ChannelManager, PaymentSendFailure, ChannelManagerReadArgs};
 use lightning::ln::features::{ChannelFeatures, InitFeatures, NodeFeatures};
 use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, DecodeError, UpdateAddHTLC, Init};
+use lightning::ln::script::ShutdownScript;
 use lightning::util::enforcing_trait_impls::{EnforcingSigner, INITIAL_REVOKED_COMMITMENT_NUMBER};
 use lightning::util::errors::APIError;
 use lightning::util::events;
@@ -164,9 +165,11 @@ impl KeysInterface for KeyProvider {
 		Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&our_channel_monitor_claim_key_hash[..]).into_script()
 	}
 
-	fn get_shutdown_pubkey(&self) -> PublicKey {
+	fn get_shutdown_scriptpubkey(&self) -> ShutdownScript {
 		let secp_ctx = Secp256k1::signing_only();
-		PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[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, 3, self.node_id]).unwrap())
+		let secret_key = SecretKey::from_slice(&[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, 3, self.node_id]).unwrap();
+		let pubkey_hash = WPubkeyHash::hash(&PublicKey::from_secret_key(&secp_ctx, &secret_key).serialize());
+		ShutdownScript::new_p2wpkh(&pubkey_hash)
 	}
 
 	fn get_channel_signer(&self, _inbound: bool, channel_value_satoshis: u64) -> EnforcingSigner {
@@ -250,6 +253,7 @@ fn check_api_err(api_err: APIError) {
 		APIError::MonitorUpdateFailed => {
 			// We can (obviously) temp-fail a monitor update
 		},
+		APIError::IncompatibleShutdownScript { .. } => panic!("Cannot send an incompatible shutdown script"),
 	}
 }
 #[inline]
@@ -393,6 +397,9 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 	let mut channel_txn = Vec::new();
 	macro_rules! make_channel {
 		($source: expr, $dest: expr, $chan_id: expr) => { {
+			$source.peer_connected(&$dest.get_our_node_id(), &Init { features: InitFeatures::known() });
+			$dest.peer_connected(&$source.get_our_node_id(), &Init { features: InitFeatures::known() });
+
 			$source.create_channel($dest.get_our_node_id(), 100_000, 42, 0, None).unwrap();
 			let open_channel = {
 				let events = $source.get_and_clear_pending_msg_events();

fuzz/src/full_stack.rs

@@ -36,6 +36,7 @@ use lightning::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
 use lightning::ln::channelmanager::{ChainParameters, ChannelManager};
 use lightning::ln::peer_handler::{MessageHandler,PeerManager,SocketDescriptor};
 use lightning::ln::msgs::DecodeError;
+use lightning::ln::script::ShutdownScript;
 use lightning::routing::router::get_route;
 use lightning::routing::network_graph::NetGraphMsgHandler;
 use lightning::util::config::UserConfig;
@@ -271,9 +272,11 @@ impl KeysInterface for KeyProvider {
 		Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&our_channel_monitor_claim_key_hash[..]).into_script()
 	}
 
-	fn get_shutdown_pubkey(&self) -> PublicKey {
+	fn get_shutdown_scriptpubkey(&self) -> ShutdownScript {
 		let secp_ctx = Secp256k1::signing_only();
-		PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[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, 1]).unwrap())
+		let secret_key = SecretKey::from_slice(&[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, 1]).unwrap();
+		let pubkey_hash = WPubkeyHash::hash(&PublicKey::from_secret_key(&secp_ctx, &secret_key).serialize());
+		ShutdownScript::new_p2wpkh(&pubkey_hash)
 	}
 
 	fn get_channel_signer(&self, inbound: bool, channel_value_satoshis: u64) -> EnforcingSigner {

lightning-background-processor/src/lib.rs

@@ -243,7 +243,7 @@ mod tests {
 	use lightning::get_event_msg;
 	use lightning::ln::channelmanager::{BREAKDOWN_TIMEOUT, ChainParameters, ChannelManager, SimpleArcChannelManager};
 	use lightning::ln::features::InitFeatures;
-	use lightning::ln::msgs::ChannelMessageHandler;
+	use lightning::ln::msgs::{ChannelMessageHandler, Init};
 	use lightning::ln::peer_handler::{PeerManager, MessageHandler, SocketDescriptor};
 	use lightning::util::config::UserConfig;
 	use lightning::util::events::{Event, MessageSendEventsProvider, MessageSendEvent};
@@ -317,6 +317,14 @@ mod tests {
 			let node = Node { node: manager, peer_manager, chain_monitor, persister, tx_broadcaster, logger, best_block };
 			nodes.push(node);
 		}
+
+		for i in 0..num_nodes {
+			for j in (i+1)..num_nodes {
+				nodes[i].node.peer_connected(&nodes[j].node.get_our_node_id(), &Init { features: InitFeatures::known() });
+				nodes[j].node.peer_connected(&nodes[i].node.get_our_node_id(), &Init { features: InitFeatures::known() });
+			}
+		}
+
 		nodes
 	}
 

lightning/src/chain/channelmonitor.rs

@@ -438,6 +438,9 @@ pub(crate) enum ChannelMonitorUpdateStep {
 		/// think we've fallen behind!
 		should_broadcast: bool,
 	},
+	ShutdownScript {
+		scriptpubkey: Script,
+	},
 }
 
 impl_writeable_tlv_based_enum!(ChannelMonitorUpdateStep,
@@ -461,6 +464,9 @@ impl_writeable_tlv_based_enum!(ChannelMonitorUpdateStep,
 	(4, ChannelForceClosed) => {
 		(0, should_broadcast, required),
 	},
+	(5, ShutdownScript) => {
+		(0, scriptpubkey, required),
+	},
 ;);
 
 /// A ChannelMonitor handles chain events (blocks connected and disconnected) and generates
@@ -493,7 +499,7 @@ pub(crate) struct ChannelMonitorImpl<Signer: Sign> {
 	destination_script: Script,
 	broadcasted_holder_revokable_script: Option<(Script, PublicKey, PublicKey)>,
 	counterparty_payment_script: Script,
-	shutdown_script: Script,
+	shutdown_script: Option<Script>,
 
 	channel_keys_id: [u8; 32],
 	holder_revocation_basepoint: PublicKey,
@@ -669,7 +675,10 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
 		}
 
 		self.counterparty_payment_script.write(writer)?;
-		self.shutdown_script.write(writer)?;
+		match &self.shutdown_script {
+			Some(script) => script.write(writer)?,
+			None => Script::new().write(writer)?,
+		}
 
 		self.channel_keys_id.write(writer)?;
 		self.holder_revocation_basepoint.write(writer)?;
@@ -799,7 +808,7 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
 }
 
 impl<Signer: Sign> ChannelMonitor<Signer> {
-	pub(crate) fn new(secp_ctx: Secp256k1<secp256k1::All>, keys: Signer, shutdown_pubkey: &PublicKey,
+	pub(crate) fn new(secp_ctx: Secp256k1<secp256k1::All>, keys: Signer, shutdown_script: Option<Script>,
 	                  on_counterparty_tx_csv: u16, destination_script: &Script, funding_info: (OutPoint, Script),
 	                  channel_parameters: &ChannelTransactionParameters,
 	                  funding_redeemscript: Script, channel_value_satoshis: u64,
@@ -808,8 +817,6 @@ impl<Signer: Sign> ChannelMonitor<Signer> {
 	                  best_block: BestBlock) -> ChannelMonitor<Signer> {
 
 		assert!(commitment_transaction_number_obscure_factor <= (1 << 48));
-		let our_channel_close_key_hash = WPubkeyHash::hash(&shutdown_pubkey.serialize());
-		let shutdown_script = Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&our_channel_close_key_hash[..]).into_script();
 		let payment_key_hash = WPubkeyHash::hash(&keys.pubkeys().payment_point.serialize());
 		let counterparty_payment_script = Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&payment_key_hash[..]).into_script();
 
@@ -1436,7 +1443,13 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 						// shouldn't print the scary warning above.
 						log_info!(logger, "Channel off-chain state closed after we broadcasted our latest commitment transaction.");
 					}
-				}
+				},
+				ChannelMonitorUpdateStep::ShutdownScript { scriptpubkey } => {
+					log_trace!(logger, "Updating ChannelMonitor with shutdown script");
+					if let Some(shutdown_script) = self.shutdown_script.replace(scriptpubkey.clone()) {
+						panic!("Attempted to replace shutdown script {} with {}", shutdown_script, scriptpubkey);
+					}
+				},
 			}
 		}
 		self.latest_update_id = updates.update_id;
@@ -2485,7 +2498,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 				}));
 				break;
 			}
-			if outp.script_pubkey == self.shutdown_script {
+			if self.shutdown_script.as_ref() == Some(&outp.script_pubkey) {
 				spendable_output = Some(SpendableOutputDescriptor::StaticOutput {
 					outpoint: OutPoint { txid: tx.txid(), index: i as u16 },
 					output: outp.clone(),
@@ -2622,7 +2635,10 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
 			_ => return Err(DecodeError::InvalidValue),
 		};
 		let counterparty_payment_script = Readable::read(reader)?;
-		let shutdown_script = Readable::read(reader)?;
+		let shutdown_script = {
+			let script = <Script as Readable>::read(reader)?;
+			if script.is_empty() { None } else { Some(script) }
+		};
 
 		let channel_keys_id = Readable::read(reader)?;
 		let holder_revocation_basepoint = Readable::read(reader)?;
@@ -2854,6 +2870,7 @@ mod tests {
 	use ln::{PaymentPreimage, PaymentHash};
 	use ln::chan_utils;
 	use ln::chan_utils::{HTLCOutputInCommitment, ChannelPublicKeys, ChannelTransactionParameters, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters};
+	use ln::script::ShutdownScript;
 	use util::test_utils::{TestLogger, TestBroadcaster, TestFeeEstimator};
 	use bitcoin::secp256k1::key::{SecretKey,PublicKey};
 	use bitcoin::secp256k1::Secp256k1;
@@ -2947,9 +2964,10 @@ mod tests {
 		};
 		// Prune with one old state and a holder commitment tx holding a few overlaps with the
 		// old state.
+		let shutdown_pubkey = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
 		let best_block = BestBlock::from_genesis(Network::Testnet);
 		let monitor = ChannelMonitor::new(Secp256k1::new(), keys,
-		                                  &PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()), 0, &Script::new(),
+		                                  Some(ShutdownScript::new_p2wpkh_from_pubkey(shutdown_pubkey).into_inner()), 0, &Script::new(),
 		                                  (OutPoint { txid: Txid::from_slice(&[43; 32]).unwrap(), index: 0 }, Script::new()),
 		                                  &channel_parameters,
 		                                  Script::new(), 46, 0,

lightning/src/chain/keysinterface.rs

@@ -36,6 +36,7 @@ use chain::transaction::OutPoint;
 use ln::chan_utils;
 use ln::chan_utils::{HTLCOutputInCommitment, make_funding_redeemscript, ChannelPublicKeys, HolderCommitmentTransaction, ChannelTransactionParameters, CommitmentTransaction};
 use ln::msgs::UnsignedChannelAnnouncement;
+use ln::script::ShutdownScript;
 
 use prelude::*;
 use core::sync::atomic::{AtomicUsize, Ordering};
@@ -118,8 +119,8 @@ impl_writeable_tlv_based!(StaticPaymentOutputDescriptor, {
 #[derive(Clone, Debug, PartialEq)]
 pub enum SpendableOutputDescriptor {
 	/// An output to a script which was provided via KeysInterface directly, either from
-	/// `get_destination_script()` or `get_shutdown_pubkey()`, thus you should already know how to
-	/// spend it. No secret keys are provided as rust-lightning was never given any key.
+	/// `get_destination_script()` or `get_shutdown_scriptpubkey()`, thus you should already know
+	/// how to spend it. No secret keys are provided as rust-lightning was never given any key.
 	/// These may include outputs from a transaction punishing our counterparty or claiming an HTLC
 	/// on-chain using the payment preimage or after it has timed out.
 	StaticOutput {
@@ -351,12 +352,11 @@ pub trait KeysInterface {
 	/// This method should return a different value each time it is called, to avoid linking
 	/// on-chain funds across channels as controlled to the same user.
 	fn get_destination_script(&self) -> Script;
-	/// Get a public key which we will send funds to (in the form of a P2WPKH output) when closing
-	/// a channel.
+	/// Get a script pubkey which we will send funds to when closing a channel.
 	///
 	/// This method should return a different value each time it is called, to avoid linking
 	/// on-chain funds across channels as controlled to the same user.
-	fn get_shutdown_pubkey(&self) -> PublicKey;
+	fn get_shutdown_scriptpubkey(&self) -> ShutdownScript;
 	/// Get a new set of Sign for per-channel secrets. These MUST be unique even if you
 	/// restarted with some stale data!
 	///
@@ -1013,8 +1013,8 @@ impl KeysInterface for KeysManager {
 		self.destination_script.clone()
 	}
 
-	fn get_shutdown_pubkey(&self) -> PublicKey {
-		self.shutdown_pubkey.clone()
+	fn get_shutdown_scriptpubkey(&self) -> ShutdownScript {
+		ShutdownScript::new_p2wpkh_from_pubkey(self.shutdown_pubkey.clone())
 	}
 
 	fn get_channel_signer(&self, _inbound: bool, channel_value_satoshis: u64) -> Self::Signer {

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -2369,3 +2369,66 @@ fn test_reconnect_dup_htlc_claims() {
 	do_test_reconnect_dup_htlc_claims(HTLCStatusAtDupClaim::HoldingCell, true);
 	do_test_reconnect_dup_htlc_claims(HTLCStatusAtDupClaim::Cleared, true);
 }
+
+#[test]
+fn test_temporary_error_during_shutdown() {
+	// Test that temporary failures when updating the monitor's shutdown script do not prevent
+	// cooperative close.
+	let mut config = test_default_channel_config();
+	config.channel_options.commit_upfront_shutdown_pubkey = false;
+
+	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, &[Some(config), Some(config)]);
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let (_, _, channel_id, funding_tx) = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
+
+	*nodes[0].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::TemporaryFailure));
+	*nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::TemporaryFailure));
+	close_channel(&nodes[0], &nodes[1], &channel_id, funding_tx, false);
+	check_added_monitors!(nodes[0], 1);
+	check_added_monitors!(nodes[1], 1);
+}
+
+#[test]
+fn test_permanent_error_during_sending_shutdown() {
+	// Test that permanent failures when updating the monitor's shutdown script result in a force
+	// close when initiating a cooperative close.
+	let mut config = test_default_channel_config();
+	config.channel_options.commit_upfront_shutdown_pubkey = false;
+
+	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, &[Some(config), None]);
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).2;
+	*nodes[0].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::PermanentFailure));
+
+	assert!(nodes[0].node.close_channel(&channel_id).is_ok());
+	check_closed_broadcast!(nodes[0], true);
+	check_added_monitors!(nodes[0], 2);
+}
+
+#[test]
+fn test_permanent_error_during_handling_shutdown() {
+	// Test that permanent failures when updating the monitor's shutdown script result in a force
+	// close when handling a cooperative close.
+	let mut config = test_default_channel_config();
+	config.channel_options.commit_upfront_shutdown_pubkey = false;
+
+	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, Some(config)]);
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let channel_id = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).2;
+	*nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::PermanentFailure));
+
+	assert!(nodes[0].node.close_channel(&channel_id).is_ok());
+	let shutdown = get_event_msg!(nodes[0], MessageSendEvent::SendShutdown, nodes[1].node.get_our_node_id());
+	nodes[1].node.handle_shutdown(&nodes[0].node.get_our_node_id(), &InitFeatures::known(), &shutdown);
+	check_closed_broadcast!(nodes[1], true);
+	check_added_monitors!(nodes[1], 2);
+}