X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=blobdiff_plain;f=lightning%2Fsrc%2Fchain%2Fpackage.rs;h=e8886e5a2aa49958fcac012529a3ae4160348230;hb=ba1349982ba28657c9e2d03a5b02c3ecc054b5cc;hp=5537a56f2f3898bcee470ae1fae87cbf44c5767c;hpb=394491115c2117cdf6afe98c286742ec49478011;p=rust-lightning

diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs
index 5537a56f..e8886e5a 100644
--- a/lightning/src/chain/package.rs
+++ b/lightning/src/chain/package.rs
@@ -434,7 +434,6 @@ impl PackageSolvingData {
 				let chan_keys = TxCreationKeys::derive_new(&onchain_handler.secp_ctx, &outp.per_commitment_point, &outp.counterparty_delayed_payment_base_key, &outp.counterparty_htlc_base_key, &onchain_handler.signer.pubkeys().revocation_basepoint, &onchain_handler.signer.pubkeys().htlc_basepoint);
 				let witness_script = chan_utils::get_htlc_redeemscript_with_explicit_keys(&outp.htlc, onchain_handler.opt_anchors(), &chan_keys.broadcaster_htlc_key, &chan_keys.countersignatory_htlc_key, &chan_keys.revocation_key);
 
-				bumped_tx.lock_time = PackedLockTime(outp.htlc.cltv_expiry); // Right now we don't aggregate time-locked transaction, if we do we should set lock_time before to avoid breaking hash computation
 				if let Ok(sig) = onchain_handler.signer.sign_counterparty_htlc_transaction(&bumped_tx, i, &outp.htlc.amount_msat / 1000, &outp.per_commitment_point, &outp.htlc, &onchain_handler.secp_ctx) {
 					let mut ser_sig = sig.serialize_der().to_vec();
 					ser_sig.push(EcdsaSighashType::All as u8);
@@ -460,18 +459,23 @@ impl PackageSolvingData {
 			_ => { panic!("API Error!"); }
 		}
 	}
-	fn absolute_tx_timelock(&self, output_conf_height: u32) -> u32 {
-		// Get the absolute timelock at which this output can be spent given the height at which
-		// this output was confirmed. We use `output_conf_height + 1` as a safe default as we can
-		// be confirmed in the next block and transactions with time lock `current_height + 1`
-		// always propagate.
+	fn absolute_tx_timelock(&self, current_height: u32) -> u32 {
+		// We use `current_height + 1` as our default locktime to discourage fee sniping and because
+		// transactions with it always propagate.
 		let absolute_timelock = match self {
-			PackageSolvingData::RevokedOutput(_) => output_conf_height + 1,
-			PackageSolvingData::RevokedHTLCOutput(_) => output_conf_height + 1,
-			PackageSolvingData::CounterpartyOfferedHTLCOutput(_) => output_conf_height + 1,
-			PackageSolvingData::CounterpartyReceivedHTLCOutput(ref outp) => cmp::max(outp.htlc.cltv_expiry, output_conf_height + 1),
-			PackageSolvingData::HolderHTLCOutput(ref outp) => cmp::max(outp.cltv_expiry, output_conf_height + 1),
-			PackageSolvingData::HolderFundingOutput(_) => output_conf_height + 1,
+			PackageSolvingData::RevokedOutput(_) => current_height + 1,
+			PackageSolvingData::RevokedHTLCOutput(_) => current_height + 1,
+			PackageSolvingData::CounterpartyOfferedHTLCOutput(_) => current_height + 1,
+			PackageSolvingData::CounterpartyReceivedHTLCOutput(ref outp) => cmp::max(outp.htlc.cltv_expiry, current_height + 1),
+			// HTLC timeout/success transactions rely on a fixed timelock due to the counterparty's
+			// signature.
+			PackageSolvingData::HolderHTLCOutput(ref outp) => {
+				if outp.preimage.is_some() {
+					debug_assert_eq!(outp.cltv_expiry, 0);
+				}
+				outp.cltv_expiry
+			},
+			PackageSolvingData::HolderFundingOutput(_) => current_height + 1,
 		};
 		absolute_timelock
 	}
@@ -638,9 +642,36 @@ impl PackageTemplate {
 		}
 		amounts
 	}
-	pub(crate) fn package_timelock(&self) -> u32 {
-		self.inputs.iter().map(|(_, outp)| outp.absolute_tx_timelock(self.height_original))
-			.max().expect("There must always be at least one output to spend in a PackageTemplate")
+	pub(crate) fn package_locktime(&self, current_height: u32) -> u32 {
+		let locktime = self.inputs.iter().map(|(_, outp)| outp.absolute_tx_timelock(current_height))
+			.max().expect("There must always be at least one output to spend in a PackageTemplate");
+
+		// If we ever try to aggregate a `HolderHTLCOutput`s with another output type, we'll likely
+		// end up with an incorrect transaction locktime since the counterparty has included it in
+		// its HTLC signature. This should never happen unless we decide to aggregate outputs across
+		// different channel commitments.
+		#[cfg(debug_assertions)] {
+			if self.inputs.iter().any(|(_, outp)|
+				if let PackageSolvingData::HolderHTLCOutput(outp) = outp {
+					outp.preimage.is_some()
+				} else {
+					false
+				}
+			) {
+				debug_assert_eq!(locktime, 0);
+			};
+			for timeout_htlc_expiry in self.inputs.iter().filter_map(|(_, outp)|
+				if let PackageSolvingData::HolderHTLCOutput(outp) = outp {
+					if outp.preimage.is_none() {
+						Some(outp.cltv_expiry)
+					} else { None }
+				} else { None }
+			) {
+				debug_assert_eq!(locktime, timeout_htlc_expiry);
+			}
+		}
+
+		locktime
 	}
 	pub(crate) fn package_weight(&self, destination_script: &Script) -> usize {
 		let mut inputs_weight = 0;
@@ -676,12 +707,13 @@ impl PackageTemplate {
 		htlcs
 	}
 	pub(crate) fn finalize_malleable_package<L: Deref, Signer: WriteableEcdsaChannelSigner>(
-		&self, onchain_handler: &mut OnchainTxHandler<Signer>, value: u64, destination_script: Script, logger: &L
+		&self, current_height: u32, onchain_handler: &mut OnchainTxHandler<Signer>, value: u64,
+		destination_script: Script, logger: &L
 	) -> Option<Transaction> where L::Target: Logger {
 		debug_assert!(self.is_malleable());
 		let mut bumped_tx = Transaction {
 			version: 2,
-			lock_time: PackedLockTime::ZERO,
+			lock_time: PackedLockTime(self.package_locktime(current_height)),
 			input: vec![],
 			output: vec![TxOut {
 				script_pubkey: destination_script,