Use counterparty txid instead of tx in channelmonitor update

Author: devrandom

lightning/src/chain/channelmonitor.rs

@@ -27,7 +27,6 @@ use bitcoin::blockdata::transaction::{TxOut,Transaction};
 use bitcoin::blockdata::transaction::OutPoint as BitcoinOutPoint;
 use bitcoin::blockdata::script::{Script, Builder};
 use bitcoin::blockdata::opcodes;
-use bitcoin::consensus::encode;
 
 use bitcoin::hashes::Hash;
 use bitcoin::hashes::sha256::Hash as Sha256;
@@ -493,7 +492,7 @@ pub(crate) enum ChannelMonitorUpdateStep {
 		htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Signature>, Option<HTLCSource>)>,
 	},
 	LatestCounterpartyCommitmentTXInfo {
-		unsigned_commitment_tx: Transaction, // TODO: We should actually only need the txid here
+		commitment_txid: Txid,
 		htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>,
 		commitment_number: u64,
 		their_revocation_point: PublicKey,
@@ -527,9 +526,9 @@ impl Writeable for ChannelMonitorUpdateStep {
 					source.write(w)?;
 				}
 			}
-			&ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { ref unsigned_commitment_tx, ref htlc_outputs, ref commitment_number, ref their_revocation_point } => {
+			&ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { commitment_txid, ref htlc_outputs, ref commitment_number, ref their_revocation_point } => {
 				1u8.write(w)?;
-				unsigned_commitment_tx.write(w)?;
+				commitment_txid.write(w)?;
 				commitment_number.write(w)?;
 				their_revocation_point.write(w)?;
 				(htlc_outputs.len() as u64).write(w)?;
@@ -573,7 +572,7 @@ impl Readable for ChannelMonitorUpdateStep {
 			},
 			1u8 => {
 				Ok(ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo {
-					unsigned_commitment_tx: Readable::read(r)?,
+					commitment_txid: Readable::read(r)?,
 					commitment_number: Readable::read(r)?,
 					their_revocation_point: Readable::read(r)?,
 					htlc_outputs: {
@@ -1097,7 +1096,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 	/// The monitor watches for it to be broadcasted and then uses the HTLC information (and
 	/// possibly future revocation/preimage information) to claim outputs where possible.
 	/// We cache also the mapping hash:commitment number to lighten pruning of old preimages by watchtowers.
-	pub(crate) fn provide_latest_counterparty_commitment_tx<L: Deref>(&mut self, unsigned_commitment_tx: &Transaction, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>, commitment_number: u64, their_revocation_point: PublicKey, logger: &L) where L::Target: Logger {
+	pub(crate) fn provide_latest_counterparty_commitment_tx<L: Deref>(&mut self, txid: Txid, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>, commitment_number: u64, their_revocation_point: PublicKey, logger: &L) where L::Target: Logger {
 		// TODO: Encrypt the htlc_outputs data with the single-hash of the commitment transaction
 		// so that a remote monitor doesn't learn anything unless there is a malicious close.
 		// (only maybe, sadly we cant do the same for local info, as we need to be aware of
@@ -1106,12 +1105,12 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 			self.counterparty_hash_commitment_number.insert(htlc.payment_hash, commitment_number);
 		}
 
-		let new_txid = unsigned_commitment_tx.txid();
-		log_trace!(logger, "Tracking new counterparty commitment transaction with txid {} at commitment number {} with {} HTLC outputs", new_txid, commitment_number, htlc_outputs.len());
-		log_trace!(logger, "New potential counterparty commitment transaction: {}", encode::serialize_hex(unsigned_commitment_tx));
+		log_trace!(logger, "Tracking new counterparty commitment transaction with txid {} at commitment number {} with {} HTLC outputs", txid, commitment_number, htlc_outputs.len());
+		// TODO do we really want to log the transaction hex?
+		// log_trace!(logger, "New potential counterparty commitment transaction: {}", encode::serialize_hex(unsigned_commitment_tx));
 		self.prev_counterparty_commitment_txid = self.current_counterparty_commitment_txid.take();
-		self.current_counterparty_commitment_txid = Some(new_txid);
-		self.counterparty_claimable_outpoints.insert(new_txid, htlc_outputs.clone());
+		self.current_counterparty_commitment_txid = Some(txid);
+		self.counterparty_claimable_outpoints.insert(txid, htlc_outputs.clone());
 		self.current_counterparty_commitment_number = commitment_number;
 		//TODO: Merge this into the other per-counterparty-transaction output storage stuff
 		match self.their_cur_revocation_points {
@@ -1138,7 +1137,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 				htlcs.push(htlc.0);
 			}
 		}
-		self.counterparty_tx_cache.per_htlc.insert(new_txid, htlcs);
+		self.counterparty_tx_cache.per_htlc.insert(txid, htlcs);
 	}
 
 	/// Informs this monitor of the latest holder (ie broadcastable) commitment transaction. The
@@ -1258,9 +1257,9 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 					if self.lockdown_from_offchain { panic!(); }
 					self.provide_latest_holder_commitment_tx(commitment_info.clone(), htlc_outputs.clone())?
 				},
-				ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { unsigned_commitment_tx, htlc_outputs, commitment_number, their_revocation_point } => {
+				ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { commitment_txid, htlc_outputs, commitment_number, their_revocation_point } => {
 					log_trace!(logger, "Updating ChannelMonitor with latest counterparty commitment transaction info");
-					self.provide_latest_counterparty_commitment_tx(&unsigned_commitment_tx, htlc_outputs.clone(), *commitment_number, *their_revocation_point, logger)
+					self.provide_latest_counterparty_commitment_tx(*commitment_txid, htlc_outputs.clone(), *commitment_number, *their_revocation_point, logger)
 				},
 				ChannelMonitorUpdateStep::PaymentPreimage { payment_preimage } => {
 					log_trace!(logger, "Updating ChannelMonitor with payment preimage");
@@ -2707,10 +2706,11 @@ mod tests {
 		                                      HolderCommitmentTransaction::dummy());
 
 		monitor.provide_latest_holder_commitment_tx(HolderCommitmentTransaction::dummy(), preimages_to_holder_htlcs!(preimages[0..10])).unwrap();
-		monitor.provide_latest_counterparty_commitment_tx(&dummy_tx, preimages_slice_to_htlc_outputs!(preimages[5..15]), 281474976710655, dummy_key, &logger);
-		monitor.provide_latest_counterparty_commitment_tx(&dummy_tx, preimages_slice_to_htlc_outputs!(preimages[15..20]), 281474976710654, dummy_key, &logger);
-		monitor.provide_latest_counterparty_commitment_tx(&dummy_tx, preimages_slice_to_htlc_outputs!(preimages[17..20]), 281474976710653, dummy_key, &logger);
-		monitor.provide_latest_counterparty_commitment_tx(&dummy_tx, preimages_slice_to_htlc_outputs!(preimages[18..20]), 281474976710652, dummy_key, &logger);
+		let dummy_txid = dummy_tx.txid();
+		monitor.provide_latest_counterparty_commitment_tx(dummy_txid, preimages_slice_to_htlc_outputs!(preimages[5..15]), 281474976710655, dummy_key, &logger);
+		monitor.provide_latest_counterparty_commitment_tx(dummy_txid, preimages_slice_to_htlc_outputs!(preimages[15..20]), 281474976710654, dummy_key, &logger);
+		monitor.provide_latest_counterparty_commitment_tx(dummy_txid, preimages_slice_to_htlc_outputs!(preimages[17..20]), 281474976710653, dummy_key, &logger);
+		monitor.provide_latest_counterparty_commitment_tx(dummy_txid, preimages_slice_to_htlc_outputs!(preimages[18..20]), 281474976710652, dummy_key, &logger);
 		for &(ref preimage, ref hash) in preimages.iter() {
 			monitor.provide_payment_preimage(hash, preimage, &broadcaster, &fee_estimator, &logger);
 		}

lightning/src/ln/chan_utils.rs

@@ -656,8 +656,6 @@ impl<'a> DirectedChannelTransactionParameters<'a> {
 
 /// This class tracks the per-transaction information needed to build a holder's commitment transaction.
 /// It is used for holder transactions that we sign only when needed.
-///
-/// TODO(devrandom): take over signing functionality from HolderCommitmentTransactionForSigning
 #[derive(Clone)]
 pub struct HolderCommitmentTransaction {
 	/// The non-party-specific transaction information

lightning/src/ln/channel.rs

@@ -1460,7 +1460,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		Ok(())
 	}
 
-	fn funding_created_signature<L: Deref>(&mut self, sig: &Signature, logger: &L) -> Result<(Transaction, CommitmentTransaction, Signature), ChannelError> where L::Target: Logger {
+	fn funding_created_signature<L: Deref>(&mut self, sig: &Signature, logger: &L) -> Result<(Txid, CommitmentTransaction, Signature), ChannelError> where L::Target: Logger {
 		let funding_script = self.get_funding_redeemscript();
 
 		let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number)?;
@@ -1474,12 +1474,12 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		let counterparty_keys = self.build_remote_transaction_keys()?;
 		let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, self.feerate_per_kw, logger).0;
 
-		let counterparty_initial_commitment_bitcoin_tx = counterparty_initial_commitment_tx.build(&self.get_channel_parameters(false), &self.secp_ctx).unwrap().0;
+		let counterparty_initial_commitment_txid = counterparty_initial_commitment_tx.calculate_txid(&self.get_channel_parameters(false), &self.secp_ctx);
 		let counterparty_signature = self.holder_keys.sign_counterparty_commitment(&counterparty_initial_commitment_tx, &self.secp_ctx)
 				.map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?.0;
 
 		// We sign "counterparty" commitment transaction, allowing them to broadcast the tx if they wish.
-		Ok((counterparty_initial_commitment_bitcoin_tx, initial_commitment_tx, counterparty_signature))
+		Ok((counterparty_initial_commitment_txid, initial_commitment_tx, counterparty_signature))
 	}
 
 	// Create a directed version of the channel static fields for the holder or the counterparty.
@@ -1512,7 +1512,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		self.channel_transaction_parameters.funding_outpoint = Some(funding_txo);
 		self.holder_keys.ready_channel(&self.channel_transaction_parameters);
 
-		let (counterparty_initial_commitment_bitcoin_tx, initial_commitment_tx, signature) = match self.funding_created_signature(&msg.signature, logger) {
+		let (counterparty_initial_commitment_txid, initial_commitment_tx, signature) = match self.funding_created_signature(&msg.signature, logger) {
 			Ok(res) => res,
 			Err(e) => {
 				self.channel_transaction_parameters.funding_outpoint = None;
@@ -1540,7 +1540,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		                                              self.get_commitment_transaction_number_obscure_factor(),
 		                                              holder_commitment_tx);
 
-		channel_monitor.provide_latest_counterparty_commitment_tx(&counterparty_initial_commitment_bitcoin_tx, Vec::new(), self.cur_counterparty_commitment_transaction_number, self.counterparty_cur_commitment_point.unwrap(), logger);
+		channel_monitor.provide_latest_counterparty_commitment_tx(counterparty_initial_commitment_txid, Vec::new(), self.cur_counterparty_commitment_transaction_number, self.counterparty_cur_commitment_point.unwrap(), logger);
 
 		self.channel_state = ChannelState::FundingSent as u32;
 		self.channel_id = funding_txo.to_channel_id();
@@ -1573,7 +1573,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		let counterparty_keys = self.build_remote_transaction_keys()?;
 		let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, self.feerate_per_kw, logger).0;
 		// TODO this is not integration tested
-		let counterparty_initial_commitment_bitcoin_tx = counterparty_initial_commitment_tx.build(&self.get_channel_parameters(false), &self.secp_ctx).unwrap().0;
+		let counterparty_initial_commitment_txid = counterparty_initial_commitment_tx.calculate_txid(&self.get_channel_parameters(false), &self.secp_ctx);
 
 		let holder_keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number)?;
 		let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &holder_keys, true, false, self.feerate_per_kw, logger).0;
@@ -1602,7 +1602,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		                                              self.get_commitment_transaction_number_obscure_factor(),
 		                                              holder_commitment_tx);
 
-		channel_monitor.provide_latest_counterparty_commitment_tx(&counterparty_initial_commitment_bitcoin_tx, Vec::new(), self.cur_counterparty_commitment_transaction_number, self.counterparty_cur_commitment_point.unwrap(), logger);
+		channel_monitor.provide_latest_counterparty_commitment_tx(counterparty_initial_commitment_txid, Vec::new(), self.cur_counterparty_commitment_transaction_number, self.counterparty_cur_commitment_point.unwrap(), logger);
 
 		assert_eq!(self.channel_state & (ChannelState::MonitorUpdateFailed as u32), 0); // We have no had any monitor(s) yet to fail update!
 		self.channel_state = ChannelState::FundingSent as u32;
@@ -3849,7 +3849,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		}
 		self.resend_order = RAACommitmentOrder::RevokeAndACKFirst;
 
-		let (res, counterparty_commitment_tx, htlcs) = match self.send_commitment_no_state_update(logger) {
+		let (res, counterparty_commitment_txid, htlcs) = match self.send_commitment_no_state_update(logger) {
 			Ok((res, (counterparty_commitment_tx, mut htlcs))) => {
 				// Update state now that we've passed all the can-fail calls...
 				let htlcs_no_ref: Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)> =
@@ -3863,7 +3863,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		let monitor_update = ChannelMonitorUpdate {
 			update_id: self.latest_monitor_update_id,
 			updates: vec![ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo {
-				unsigned_commitment_tx: counterparty_commitment_tx.clone(),
+				commitment_txid: counterparty_commitment_txid,
 				htlc_outputs: htlcs.clone(),
 				commitment_number: self.cur_counterparty_commitment_transaction_number,
 				their_revocation_point: self.counterparty_cur_commitment_point.unwrap()
@@ -3875,7 +3875,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 	/// Only fails in case of bad keys. Used for channel_reestablish commitment_signed generation
 	/// when we shouldn't change HTLC/channel state.
-	fn send_commitment_no_state_update<L: Deref>(&self, logger: &L) -> Result<(msgs::CommitmentSigned, (Transaction, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>)), ChannelError> where L::Target: Logger {
+	fn send_commitment_no_state_update<L: Deref>(&self, logger: &L) -> Result<(msgs::CommitmentSigned, (Txid, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>)), ChannelError> where L::Target: Logger {
 		let mut feerate_per_kw = self.feerate_per_kw;
 		if let Some(feerate) = self.pending_update_fee {
 			if self.is_outbound() {
@@ -3885,7 +3885,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 		let counterparty_keys = self.build_remote_transaction_keys()?;
 		let counterparty_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, true, feerate_per_kw, logger);
-		let counterparty_commitment_bitcoin_tx = counterparty_commitment_tx.0.build(&self.get_channel_parameters(false), &self.secp_ctx).unwrap().0;
+		let counterparty_commitment_txid = counterparty_commitment_tx.0.calculate_txid(&self.get_channel_parameters(false), &self.secp_ctx);
 		let (signature, htlc_signatures);
 
 		{
@@ -3900,13 +3900,13 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			htlc_signatures = res.1;
 
 			log_trace!(logger, "Signed remote commitment tx {} with redeemscript {} -> {}",
-				encode::serialize_hex(&counterparty_commitment_bitcoin_tx),
+				&counterparty_commitment_txid,
 				encode::serialize_hex(&self.get_funding_redeemscript()),
 				log_bytes!(signature.serialize_compact()[..]));
 
 			for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(htlcs) {
 				log_trace!(logger, "Signed remote HTLC tx {} with redeemscript {} with pubkey {} -> {}",
-					encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_bitcoin_tx.txid(), feerate_per_kw, self.get_holder_selected_contest_delay(), htlc, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)),
+					encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_txid, feerate_per_kw, self.get_holder_selected_contest_delay(), htlc, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)),
 					encode::serialize_hex(&chan_utils::get_htlc_redeemscript(&htlc, &counterparty_keys)),
 					log_bytes!(counterparty_keys.broadcaster_htlc_key.serialize()),
 					log_bytes!(htlc_sig.serialize_compact()[..]));
@@ -3917,7 +3917,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			channel_id: self.channel_id,
 			signature,
 			htlc_signatures,
-		}, (counterparty_commitment_bitcoin_tx, counterparty_commitment_tx.2)))
+		}, (counterparty_commitment_txid, counterparty_commitment_tx.2)))
 	}
 
 	/// Adds a pending outbound HTLC to this channel, and creates a signed commitment transaction