Use correct to_remote script in counterparty commitments

lightning/src/chain/channelmonitor.rs

@@ -22,12 +22,11 @@
 
 use bitcoin::blockdata::block::BlockHeader;
 use bitcoin::blockdata::transaction::{OutPoint as BitcoinOutPoint, TxOut, Transaction};
-use bitcoin::blockdata::script::{Script, Builder};
-use bitcoin::blockdata::opcodes;
+use bitcoin::blockdata::script::Script;
 
 use bitcoin::hashes::Hash;
 use bitcoin::hashes::sha256::Hash as Sha256;
-use bitcoin::hash_types::{Txid, BlockHash, WPubkeyHash};
+use bitcoin::hash_types::{Txid, BlockHash};
 
 use bitcoin::secp256k1::{Secp256k1, ecdsa::Signature};
 use bitcoin::secp256k1::{SecretKey, PublicKey};
@@ -1141,8 +1140,9 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
 	                  best_block: BestBlock, counterparty_node_id: PublicKey) -> ChannelMonitor<Signer> {
 
 		assert!(commitment_transaction_number_obscure_factor <= (1 << 48));
-		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();
+		let counterparty_payment_script = chan_utils::get_counterparty_payment_script(
+			&channel_parameters.channel_type_features, &keys.pubkeys().payment_point
+		);
 
 		let counterparty_channel_parameters = channel_parameters.counterparty_parameters.as_ref().unwrap();
 		let counterparty_delayed_payment_base_key = counterparty_channel_parameters.pubkeys.delayed_payment_basepoint;
@@ -1696,6 +1696,16 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
 			Vec::new()
 		}
 	}
+
+	#[cfg(test)]
+	pub fn get_counterparty_payment_script(&self) -> Script{
+		self.inner.lock().unwrap().counterparty_payment_script.clone()
+	}
+
+	#[cfg(test)]
+	pub fn set_counterparty_payment_script(&self, script: Script) {
+		self.inner.lock().unwrap().counterparty_payment_script = script;
+	}
 }
 
 impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
@@ -2263,6 +2273,7 @@ macro_rules! fail_unbroadcast_htlcs {
 
 #[cfg(test)]
 pub fn deliberately_bogus_accepted_htlc_witness_program() -> Vec<u8> {
+	use bitcoin::blockdata::opcodes;
 	let mut ret = [opcodes::all::OP_NOP.to_u8(); 136];
 	ret[131] = opcodes::all::OP_DROP.to_u8();
 	ret[132] = opcodes::all::OP_DROP.to_u8();
@@ -4042,6 +4053,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					output: outp.clone(),
 					channel_keys_id: self.channel_keys_id,
 					channel_value_satoshis: self.channel_value_satoshis,
+					channel_transaction_parameters: Some(self.onchain_tx_handler.channel_transaction_parameters.clone()),
 				}));
 			}
 			if self.shutdown_script.as_ref() == Some(&outp.script_pubkey) {
@@ -4144,7 +4156,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
 			1 => { None },
 			_ => return Err(DecodeError::InvalidValue),
 		};
-		let counterparty_payment_script = Readable::read(reader)?;
+		let mut counterparty_payment_script: Script = Readable::read(reader)?;
 		let shutdown_script = {
 			let script = <Script as Readable>::read(reader)?;
 			if script.is_empty() { None } else { Some(script) }
@@ -4345,6 +4357,17 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
 			(17, initial_counterparty_commitment_info, option),
 		});
 
+		// Monitors for anchor outputs channels opened in v0.0.116 suffered from a bug in which the
+		// wrong `counterparty_payment_script` was being tracked. Fix it now on deserialization to
+		// give them a chance to recognize the spendable output.
+		if onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() &&
+			counterparty_payment_script.is_v0_p2wpkh()
+		{
+			let payment_point = onchain_tx_handler.channel_transaction_parameters.holder_pubkeys.payment_point;
+			counterparty_payment_script =
+				chan_utils::get_to_countersignatory_with_anchors_redeemscript(&payment_point).to_v0_p2wsh();
+		}
+
 		Ok((best_block.block_hash(), ChannelMonitor::from_impl(ChannelMonitorImpl {
 			latest_update_id,
 			commitment_transaction_number_obscure_factor,

lightning/src/events/bump_transaction.rs

@@ -26,7 +26,7 @@ use crate::ln::chan_utils::{
 use crate::ln::features::ChannelTypeFeatures;
 use crate::ln::PaymentPreimage;
 use crate::prelude::*;
-use crate::sign::{EcdsaChannelSigner, SignerProvider, WriteableEcdsaChannelSigner};
+use crate::sign::{EcdsaChannelSigner, SignerProvider, WriteableEcdsaChannelSigner, P2WPKH_WITNESS_WEIGHT};
 use crate::sync::Mutex;
 use crate::util::logger::Logger;
 
@@ -384,12 +384,6 @@ pub struct Utxo {
 }
 
 impl Utxo {
-	const P2WPKH_WITNESS_WEIGHT: u64 = 1 /* num stack items */ +
-		1 /* sig length */ +
-		73 /* sig including sighash flag */ +
-		1 /* pubkey length */ +
-		33 /* pubkey */;
-
 	/// Returns a `Utxo` with the `satisfaction_weight` estimate for a legacy P2PKH output.
 	pub fn new_p2pkh(outpoint: OutPoint, value: u64, pubkey_hash: &PubkeyHash) -> Self {
 		let script_sig_size = 1 /* script_sig length */ +
@@ -419,7 +413,7 @@ impl Utxo {
 				value,
 				script_pubkey: Script::new_p2sh(&Script::new_v0_p2wpkh(pubkey_hash).script_hash()),
 			},
-			satisfaction_weight: script_sig_size * WITNESS_SCALE_FACTOR as u64 + Self::P2WPKH_WITNESS_WEIGHT,
+			satisfaction_weight: script_sig_size * WITNESS_SCALE_FACTOR as u64 + P2WPKH_WITNESS_WEIGHT,
 		}
 	}
 
@@ -431,7 +425,7 @@ impl Utxo {
 				value,
 				script_pubkey: Script::new_v0_p2wpkh(pubkey_hash),
 			},
-			satisfaction_weight: EMPTY_SCRIPT_SIG_WEIGHT + Self::P2WPKH_WITNESS_WEIGHT,
+			satisfaction_weight: EMPTY_SCRIPT_SIG_WEIGHT + P2WPKH_WITNESS_WEIGHT,
 		}
 	}
 }

lightning/src/ln/chan_utils.rs

@@ -19,7 +19,7 @@ use bitcoin::util::address::Payload;
 use bitcoin::hashes::{Hash, HashEngine};
 use bitcoin::hashes::sha256::Hash as Sha256;
 use bitcoin::hashes::ripemd160::Hash as Ripemd160;
-use bitcoin::hash_types::{Txid, PubkeyHash};
+use bitcoin::hash_types::{Txid, PubkeyHash, WPubkeyHash};
 
 use crate::chain::chaininterface::fee_for_weight;
 use crate::chain::package::WEIGHT_REVOKED_OUTPUT;
@@ -475,7 +475,7 @@ impl_writeable_tlv_based!(TxCreationKeys, {
 });
 
 /// One counterparty's public keys which do not change over the life of a channel.
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, Hash, PartialEq, Eq)]
 pub struct ChannelPublicKeys {
 	/// The public key which is used to sign all commitment transactions, as it appears in the
 	/// on-chain channel lock-in 2-of-2 multisig output.
@@ -556,6 +556,16 @@ pub fn get_revokeable_redeemscript(revocation_key: &PublicKey, contest_delay: u1
 	res
 }
 
+/// Returns the script for the counterparty's output on a holder's commitment transaction based on
+/// the channel type.
+pub fn get_counterparty_payment_script(channel_type_features: &ChannelTypeFeatures, payment_key: &PublicKey) -> Script {
+	if channel_type_features.supports_anchors_zero_fee_htlc_tx() {
+		get_to_countersignatory_with_anchors_redeemscript(payment_key).to_v0_p2wsh()
+	} else {
+		Script::new_v0_p2wpkh(&WPubkeyHash::hash(&payment_key.serialize()))
+	}
+}
+
 /// Information about an HTLC as it appears in a commitment transaction
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct HTLCOutputInCommitment {
@@ -853,7 +863,7 @@ pub fn build_anchor_input_witness(funding_key: &PublicKey, funding_sig: &Signatu
 ///
 /// Normally, this is converted to the broadcaster/countersignatory-organized DirectedChannelTransactionParameters
 /// before use, via the as_holder_broadcastable and as_counterparty_broadcastable functions.
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, Hash, PartialEq, Eq)]
 pub struct ChannelTransactionParameters {
 	/// Holder public keys
 	pub holder_pubkeys: ChannelPublicKeys,
@@ -873,7 +883,7 @@ pub struct ChannelTransactionParameters {
 }
 
 /// Late-bound per-channel counterparty data used to build transactions.
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, Hash, PartialEq, Eq)]
 pub struct CounterpartyChannelTransactionParameters {
 	/// Counter-party public keys
 	pub pubkeys: ChannelPublicKeys,

lightning/src/ln/monitor_tests.rs

@@ -2287,16 +2287,20 @@ fn test_anchors_aggregated_revoked_htlc_tx() {
 
 	assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());
 	let spendable_output_events = nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events();
-	assert_eq!(spendable_output_events.len(), 2);
-	for event in spendable_output_events.iter() {
+	assert_eq!(spendable_output_events.len(), 4);
+	for event in spendable_output_events {
 		if let Event::SpendableOutputs { outputs, channel_id } = event {
 			assert_eq!(outputs.len(), 1);
 			assert!(vec![chan_b.2, chan_a.2].contains(&channel_id.unwrap()));
 			let spend_tx = nodes[0].keys_manager.backing.spend_spendable_outputs(
 				&[&outputs[0]], Vec::new(), Script::new_op_return(&[]), 253, None, &Secp256k1::new(),
 			).unwrap();
 
-			check_spends!(spend_tx, revoked_claim_transactions.get(&spend_tx.input[0].previous_output.txid).unwrap());
+			if let SpendableOutputDescriptor::StaticPaymentOutput(_) = &outputs[0] {
+				check_spends!(spend_tx, &revoked_commitment_a, &revoked_commitment_b);
+			} else {
+				check_spends!(spend_tx, revoked_claim_transactions.get(&spend_tx.input[0].previous_output.txid).unwrap());
+			}
 		} else {
 			panic!("unexpected event");
 		}
@@ -2314,3 +2318,90 @@ fn test_anchors_aggregated_revoked_htlc_tx() {
 	// revoked commitment which Bob has the preimage for.
 	assert_eq!(nodes[1].chain_monitor.chain_monitor.get_claimable_balances(&[]).len(), 6);
 }
+
+fn do_test_anchors_monitor_fixes_counterparty_payment_script_on_reload(confirm_commitment_before_reload: bool) {
+	// Tests that we'll fix a ChannelMonitor's `counterparty_payment_script` for an anchor outputs
+	// channel upon deserialization.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let persister;
+	let chain_monitor;
+	let mut user_config = test_default_channel_config();
+	user_config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true;
+	user_config.manually_accept_inbound_channels = true;
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(user_config), Some(user_config)]);
+	let node_deserialized;
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let (_, _, chan_id, funding_tx) = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 50_000_000);
+
+	// Set the monitor's `counterparty_payment_script` to a dummy P2WPKH script.
+	let secp = Secp256k1::new();
+	let privkey = bitcoin::PrivateKey::from_slice(&[1; 32], bitcoin::Network::Testnet).unwrap();
+	let pubkey = bitcoin::PublicKey::from_private_key(&secp, &privkey);
+	let p2wpkh_script = Script::new_v0_p2wpkh(&pubkey.wpubkey_hash().unwrap());
+	get_monitor!(nodes[1], chan_id).set_counterparty_payment_script(p2wpkh_script.clone());
+	assert_eq!(get_monitor!(nodes[1], chan_id).get_counterparty_payment_script(), p2wpkh_script);
+
+	// Confirm the counterparty's commitment and reload the monitor (either before or after) such
+	// that we arrive at the correct `counterparty_payment_script` after the reload.
+	nodes[0].node.force_close_broadcasting_latest_txn(&chan_id, &nodes[1].node.get_our_node_id()).unwrap();
+	check_added_monitors(&nodes[0], 1);
+	check_closed_broadcast(&nodes[0], 1, true);
+	check_closed_event!(&nodes[0], 1, ClosureReason::HolderForceClosed, false,
+		 [nodes[1].node.get_our_node_id()], 100000);
+
+	let commitment_tx = {
+		let mut txn = nodes[0].tx_broadcaster.unique_txn_broadcast();
+		assert_eq!(txn.len(), 1);
+		assert_eq!(txn[0].output.len(), 4);
+		check_spends!(txn[0], funding_tx);
+		txn.pop().unwrap()
+	};
+
+	mine_transaction(&nodes[0], &commitment_tx);
+	let commitment_tx_conf_height = if confirm_commitment_before_reload {
+		// We should expect our round trip serialization check to fail as we're writing the monitor
+		// with the incorrect P2WPKH script but reading it with the correct P2WSH script.
+		*nodes[1].chain_monitor.expect_monitor_round_trip_fail.lock().unwrap() = Some(chan_id);
+		let commitment_tx_conf_height = block_from_scid(&mine_transaction(&nodes[1], &commitment_tx));
+		let serialized_monitor = get_monitor!(nodes[1], chan_id).encode();
+		reload_node!(nodes[1], user_config, &nodes[1].node.encode(), &[&serialized_monitor], persister, chain_monitor, node_deserialized);
+		commitment_tx_conf_height
+	} else {
+		let serialized_monitor = get_monitor!(nodes[1], chan_id).encode();
+		reload_node!(nodes[1], user_config, &nodes[1].node.encode(), &[&serialized_monitor], persister, chain_monitor, node_deserialized);
+		let commitment_tx_conf_height = block_from_scid(&mine_transaction(&nodes[1], &commitment_tx));
+		check_added_monitors(&nodes[1], 1);
+		check_closed_broadcast(&nodes[1], 1, true);
+		commitment_tx_conf_height
+	};
+	check_closed_event!(&nodes[1], 1, ClosureReason::CommitmentTxConfirmed, false,
+		 [nodes[0].node.get_our_node_id()], 100000);
+	assert!(get_monitor!(nodes[1], chan_id).get_counterparty_payment_script().is_v0_p2wsh());
+
+	connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
+	connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
+
+	if confirm_commitment_before_reload {
+		// If we saw the commitment before our `counterparty_payment_script` was fixed, we'll never
+		// get the spendable output event for the `to_remote` output, so we'll need to get it
+		// manually via `get_spendable_outputs`.
+		check_added_monitors(&nodes[1], 1);
+		let outputs = get_monitor!(nodes[1], chan_id).get_spendable_outputs(&commitment_tx, commitment_tx_conf_height);
+		assert_eq!(outputs.len(), 1);
+		let spend_tx = nodes[1].keys_manager.backing.spend_spendable_outputs(
+			&[&outputs[0]], Vec::new(), Builder::new().push_opcode(opcodes::all::OP_RETURN).into_script(),
+			253, None, &secp
+		).unwrap();
+		check_spends!(spend_tx, &commitment_tx);
+	} else {
+		test_spendable_output(&nodes[1], &commitment_tx);
+	}
+}
+
+#[test]
+fn test_anchors_monitor_fixes_counterparty_payment_script_on_reload() {
+	do_test_anchors_monitor_fixes_counterparty_payment_script_on_reload(false);
+	do_test_anchors_monitor_fixes_counterparty_payment_script_on_reload(true);
+}