-f Move timelock from OnchainRequest to PackageTemplate

Author: Antoine Riard

lightning/src/chain/channelmonitor.rs

@@ -1462,8 +1462,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 			for (idx, outp) in tx.output.iter().enumerate() {
 				if outp.script_pubkey == revokeable_p2wsh {
 					let revk_outp = RevokedOutput::build(per_commitment_point, per_commitment_key, self.counterparty_tx_cache.counterparty_delayed_payment_base_key, self.counterparty_tx_cache.counterparty_htlc_base_key, InputDescriptors::RevokedOutput, outp.value, None, self.counterparty_tx_cache.on_counterparty_tx_csv);
-					let justice_package = PackageTemplate::build_package(commitment_txid, idx as u32, PackageSolvingData::RevokedOutput(revk_outp), PackageMalleability::Malleable);
-					claimable_outpoints.push(OnchainRequest { aggregation: true, feerate_previous: 0, height_timer: None, absolute_timelock: height + self.counterparty_tx_cache.on_counterparty_tx_csv as u32, height_original: height, content: justice_package});
+					let justice_package = PackageTemplate::build_package(commitment_txid, idx as u32, PackageSolvingData::RevokedOutput(revk_outp), PackageMalleability::Malleable, height + self.counterparty_tx_cache.on_counterparty_tx_csv as u32);
+					claimable_outpoints.push(OnchainRequest { aggregation: true, feerate_previous: 0, height_timer: None, height_original: height, content: justice_package});
 				}
 			}
 
@@ -1476,8 +1476,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 							return (claimable_outpoints, (commitment_txid, watch_outputs)); // Corrupted per_commitment_data, fuck this user
 						}
 						let revk_outp = RevokedOutput::build(per_commitment_point, per_commitment_key, self.counterparty_tx_cache.counterparty_delayed_payment_base_key, self.counterparty_tx_cache.counterparty_htlc_base_key, if htlc.offered { InputDescriptors::RevokedOfferedHTLC } else { InputDescriptors::RevokedReceivedHTLC }, htlc.amount_msat / 1000, Some(htlc.clone()), self.counterparty_tx_cache.on_counterparty_tx_csv);
-						let justice_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, PackageSolvingData::RevokedOutput(revk_outp), PackageMalleability::Malleable);
-						claimable_outpoints.push(OnchainRequest { aggregation: true, feerate_previous: 0, height_timer: None, absolute_timelock: htlc.cltv_expiry, height_original: height, content: justice_package});
+						let justice_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, PackageSolvingData::RevokedOutput(revk_outp), PackageMalleability::Malleable, htlc.cltv_expiry);
+						claimable_outpoints.push(OnchainRequest { aggregation: true, feerate_previous: 0, height_timer: None, height_original: height, content: justice_package});
 					}
 				}
 			}
@@ -1626,8 +1626,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 							let preimage = if htlc.offered { if let Some(p) = self.payment_preimages.get(&htlc.payment_hash) { Some(*p) } else { None } } else { None };
 							if preimage.is_some() || !htlc.offered {
 								let counterparty_htlc_outp = CounterpartyHTLCOutput::build(*revocation_point, self.counterparty_tx_cache.counterparty_delayed_payment_base_key, self.counterparty_tx_cache.counterparty_htlc_base_key, preimage, htlc.clone());
-								let counterparty_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, PackageSolvingData::CounterpartyHTLCOutput(counterparty_htlc_outp), PackageMalleability::Malleable);
-								claimable_outpoints.push(OnchainRequest { aggregation: if !htlc.offered { false } else { true }, feerate_previous: 0, height_timer: None, absolute_timelock: htlc.cltv_expiry, height_original: /*XXX(ariard) Option<height> */ 0, content: counterparty_package });
+								let counterparty_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, PackageSolvingData::CounterpartyHTLCOutput(counterparty_htlc_outp), PackageMalleability::Malleable, htlc.cltv_expiry);
+								claimable_outpoints.push(OnchainRequest { aggregation: if !htlc.offered { false } else { true }, feerate_previous: 0, height_timer: None, height_original: /*XXX(ariard) Option<height> */ 0, content: counterparty_package });
 							}
 						}
 					}
@@ -1659,8 +1659,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 
 		log_trace!(logger, "Counterparty HTLC broadcast {}:{}", htlc_txid, 0);
 		let revk_outp = RevokedOutput::build(per_commitment_point, per_commitment_key, self.counterparty_tx_cache.counterparty_delayed_payment_base_key, self.counterparty_tx_cache.counterparty_htlc_base_key, InputDescriptors::RevokedOutput, tx.output[0].value, None, self.counterparty_tx_cache.on_counterparty_tx_csv);
-		let justice_package = PackageTemplate::build_package(htlc_txid, 0, PackageSolvingData::RevokedOutput(revk_outp), PackageMalleability::Malleable);
-		let claimable_outpoints = vec!(OnchainRequest { aggregation: true, feerate_previous: 0, height_timer: None, absolute_timelock: height + self.counterparty_tx_cache.on_counterparty_tx_csv as u32, height_original: height, content: justice_package });
+		let justice_package = PackageTemplate::build_package(htlc_txid, 0, PackageSolvingData::RevokedOutput(revk_outp), PackageMalleability::Malleable, height + self.counterparty_tx_cache.on_counterparty_tx_csv as u32);
+		let claimable_outpoints = vec!(OnchainRequest { aggregation: true, feerate_previous: 0, height_timer: None, height_original: height, content: justice_package });
 		let outputs = vec![(0, tx.output[0].clone())];
 		(claimable_outpoints, Some((htlc_txid, outputs)))
 	}
@@ -1684,8 +1684,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 						continue;
 					}
 				} else { None }, htlc.amount_msat);
-				let htlc_package = PackageTemplate::build_package(holder_tx.txid, transaction_output_index, PackageSolvingData::HolderHTLCOutput(htlc_output), PackageMalleability::Untractable);
-				claim_requests.push(OnchainRequest { aggregation: false, feerate_previous: 0, height_timer: None, absolute_timelock: height, height_original: height, content: htlc_package });
+				let htlc_package = PackageTemplate::build_package(holder_tx.txid, transaction_output_index, PackageSolvingData::HolderHTLCOutput(htlc_output), PackageMalleability::Untractable, height);
+				claim_requests.push(OnchainRequest { aggregation: false, feerate_previous: 0, height_timer: None, height_original: height, content: htlc_package });
 			}
 		}
 
@@ -1893,8 +1893,8 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 		let should_broadcast = self.would_broadcast_at_height(height, &logger);
 		if should_broadcast {
 			let funding_outp = HolderFundingOutput::build(self.funding_redeemscript.clone());
-			let commitment_package = PackageTemplate::build_package(self.funding_info.0.txid.clone(), self.funding_info.0.index as u32, PackageSolvingData::HolderFundingOutput(funding_outp), PackageMalleability::Untractable);
-			claimable_outpoints.push(OnchainRequest { aggregation: false, feerate_previous: 0, height_timer: None, absolute_timelock: height, height_original: height, content: commitment_package });
+			let commitment_package = PackageTemplate::build_package(self.funding_info.0.txid.clone(), self.funding_info.0.index as u32, PackageSolvingData::HolderFundingOutput(funding_outp), PackageMalleability::Untractable, height);
+			claimable_outpoints.push(OnchainRequest { aggregation: false, feerate_previous: 0, height_timer: None, height_original: height, content: commitment_package });
 		}
 		if should_broadcast {
 			self.pending_monitor_events.push(MonitorEvent::CommitmentTxBroadcasted(self.funding_info.0));

lightning/src/chain/onchain_utils.rs

@@ -479,12 +479,18 @@ impl Readable for PackageMalleability {
 pub(crate) struct PackageTemplate {
 	inputs: Vec<(BitcoinOutPoint, PackageSolvingData)>,
 	malleability: PackageMalleability,
+	// Block height before which claiming is exclusive to one party,
+	// after reaching it, claiming may be contentious.
+	absolute_timelock: u32,
 }
 
 impl PackageTemplate {
 	pub(crate) fn is_malleable(&self) -> bool {
 		self.malleability == PackageMalleability::Malleable
 	}
+	pub(crate) fn timelock(&self) -> u32 {
+		self.absolute_timelock
+	}
 	pub(crate) fn outpoints(&self) -> Vec<&BitcoinOutPoint> {
 		let mut outpoints = Vec::with_capacity(self.inputs.len());
 		for (o, _) in self.inputs.iter() {
@@ -496,11 +502,13 @@ impl PackageTemplate {
 		match self.malleability {
 			PackageMalleability::Malleable => {
 				let mut split_package = None;
+				let timelock = self.absolute_timelock;
 				self.inputs.retain(|outp| {
 					if *split_outp == outp.0 {
 						split_package = Some(PackageTemplate {
 							inputs: vec![(outp.0, outp.1.clone())],
-							malleability: PackageMalleability::Malleable
+							malleability: PackageMalleability::Malleable,
+							absolute_timelock: timelock,
 						});
 						return false;
 					}
@@ -530,6 +538,10 @@ impl PackageTemplate {
 			if lead_category != v.category() { panic!("Merging outputs from differing categories !"); }
 			self.inputs.push((k, v));
 		}
+		//TODO: verify coverage and sanity?
+		if self.absolute_timelock > merge_from.absolute_timelock {
+			self.absolute_timelock = merge_from.absolute_timelock;
+		}
 	}
 	pub(crate) fn package_amount(&self) -> u64 {
 		let mut amounts = 0;
@@ -602,12 +614,13 @@ impl PackageTemplate {
 			},
 		}
 	}
-	pub (crate) fn build_package(txid: Txid, vout: u32, input_solving_data: PackageSolvingData, malleability: PackageMalleability) -> Self {
+	pub (crate) fn build_package(txid: Txid, vout: u32, input_solving_data: PackageSolvingData, malleability: PackageMalleability, absolute_timelock: u32) -> Self {
 		let mut inputs = Vec::with_capacity(1);
 		inputs.push((BitcoinOutPoint { txid, vout }, input_solving_data));
 		PackageTemplate {
 			inputs,
-			malleability
+			malleability,
+			absolute_timelock
 		}
 	}
 }
@@ -620,6 +633,7 @@ impl Writeable for PackageTemplate {
 			rev_outp.write(writer)?;
 		}
 		self.malleability.write(writer)?;
+		self.absolute_timelock.write(writer)?;
 		Ok(())
 	}
 }
@@ -634,9 +648,11 @@ impl Readable for PackageTemplate {
 			inputs.push((outpoint, rev_outp));
 		}
 		let malleability = Readable::read(reader)?;
+		let absolute_timelock = Readable::read(reader)?;
 		Ok(PackageTemplate {
 			inputs,
-			malleability
+			malleability,
+			absolute_timelock
 		})
 	}
 }
@@ -672,9 +688,6 @@ pub struct OnchainRequest {
 	// At every block tick, used to check if pending claiming tx is taking too
 	// much time for confirmation and we need to bump it.
 	pub(crate) height_timer: Option<u32>,
-	// Block height before which claiming is exclusive to one party,
-	// after reaching it, claiming may be contentious.
-	pub(crate) absolute_timelock: u32,
 	// Tracked in case of reorg to wipe out now-superflous request.
 	pub(crate) height_original: u32,
 	// Content of request.
@@ -683,18 +696,7 @@ pub struct OnchainRequest {
 
 impl OnchainRequest {
 	pub(crate) fn request_merge(&mut self, req: OnchainRequest) {
-		// We init default onchain request with first merge content
-		if self.absolute_timelock == ::std::u32::MAX {
-			println!("Init merging {}", req.height_original);
-			self.height_original = req.height_original;
-			self.content = req.content;
-			self.absolute_timelock = req.absolute_timelock;
-			return;
-		}
 		assert_eq!(self.height_original, req.height_original);
-		if self.absolute_timelock > req.absolute_timelock {
-			self.absolute_timelock = req.absolute_timelock;
-		}
 		self.content.merge_package(req.content);
 	}
 }
@@ -704,7 +706,6 @@ impl Writeable for OnchainRequest {
 		self.aggregation.write(writer)?;
 		self.feerate_previous.write(writer)?;
 		self.height_timer.write(writer)?;
-		self.absolute_timelock.write(writer)?;
 		self.height_original.write(writer)?;
 		self.content.write(writer)?;
 
@@ -717,15 +718,13 @@ impl Readable for OnchainRequest {
 		let aggregation = Readable::read(reader)?;
 		let feerate_previous = Readable::read(reader)?;
 		let height_timer = Readable::read(reader)?;
-		let absolute_timelock = Readable::read(reader)?;
 		let height_original = Readable::read(reader)?;
 		let content = Readable::read(reader)?;
 
 		Ok(OnchainRequest {
 			aggregation,
 			feerate_previous,
 			height_timer,
-			absolute_timelock,
 			height_original,
 			content
 		})

lightning/src/chain/onchaintx.rs

@@ -332,7 +332,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
 
 		// Compute new height timer to decide when we need to regenerate a new bumped version of the claim tx (if we
 		// didn't receive confirmation of it before, or not enough reorg-safe depth on top of it).
-		let new_timer = Some(Self::get_height_timer(height, cached_request.absolute_timelock));
+		let new_timer = Some(Self::get_height_timer(height, cached_request.content.timelock()));
 		let amt = cached_request.content.package_amount();
 		if cached_request.content.is_malleable() {
 			let predicted_weight = cached_request.content.package_weight(&self.destination_script);
@@ -369,14 +369,14 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
 		let mut preprocessed_requests = Vec::with_capacity(requests.len());
 		let mut aggregated_request = None;
 
-		// Try to aggregate outputs if their timelock expiration isn't imminent (absolute_timelock
+		// Try to aggregate outputs if their timelock expiration isn't imminent (package timelock
 		// <= CLTV_SHARED_CLAIM_BUFFER) and they don't require an immediate nLockTime (aggregable).
 		for req in requests {
 			// Don't claim a outpoint twice that would be bad for privacy and may uselessly lock a CPFP input for a while
 			if let Some(_) = self.claimable_outpoints.get(req.content.outpoints()[0]) { log_trace!(logger, "Bouncing off outpoint {}:{}, already registered its claiming request", req.content.outpoints()[0].txid, req.content.outpoints()[0].vout); } else {
-				log_trace!(logger, "Test if outpoint can be aggregated with expiration {} against {}", req.absolute_timelock, height + CLTV_SHARED_CLAIM_BUFFER);
-				if req.absolute_timelock <= height + CLTV_SHARED_CLAIM_BUFFER || !req.aggregation {
-					// Don't aggregate if outpoint absolute timelock is soon or marked as non-aggregable
+				log_trace!(logger, "Test if outpoint can be aggregated with expiration {} against {}", req.content.timelock(), height + CLTV_SHARED_CLAIM_BUFFER);
+				if req.content.timelock() <= height + CLTV_SHARED_CLAIM_BUFFER || !req.aggregation {
+					// Don't aggregate if outpoint package timelock is soon or marked as non-aggregable
 					preprocessed_requests.push(req);
 				} else if aggregated_request.is_none() {
 					aggregated_request = Some(req);