-f Dry-up aggregation 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, 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});
+					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, true);
+					claimable_outpoints.push(OnchainRequest { 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, htlc.cltv_expiry);
-						claimable_outpoints.push(OnchainRequest { aggregation: true, feerate_previous: 0, height_timer: None, 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, true);
+						claimable_outpoints.push(OnchainRequest { 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, 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 });
+								let counterparty_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, PackageSolvingData::CounterpartyHTLCOutput(counterparty_htlc_outp), PackageMalleability::Malleable, htlc.cltv_expiry, if !htlc.offered { false } else { true });
+								claimable_outpoints.push(OnchainRequest { 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, 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 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, true);
+		let claimable_outpoints = vec!(OnchainRequest { 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, height);
-				claim_requests.push(OnchainRequest { aggregation: false, feerate_previous: 0, height_timer: None, 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, false);
+				claim_requests.push(OnchainRequest { 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, height);
-			claimable_outpoints.push(OnchainRequest { aggregation: false, feerate_previous: 0, height_timer: None, 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, false);
+			claimable_outpoints.push(OnchainRequest { 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));
@@ -2664,9 +2664,9 @@ mod tests {
 	use hex;
 	use chain::channelmonitor::ChannelMonitor;
 	use chain::transaction::OutPoint;
+	use chain::onchain_utils;
+	use chain::onchain_utils::InputDescriptors;
 	use ln::channelmanager::{PaymentPreimage, PaymentHash};
-	use ln::onchain_utils;
-	use ln::onchain_utils::InputDescriptors;
 	use ln::chan_utils;
 	use ln::chan_utils::{HTLCOutputInCommitment, ChannelPublicKeys, ChannelTransactionParameters, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters};
 	use util::test_utils::{TestLogger, TestBroadcaster, TestFeeEstimator};

lightning/src/chain/onchain_utils.rs

@@ -482,6 +482,13 @@ pub(crate) struct PackageTemplate {
 	// Block height before which claiming is exclusive to one party,
 	// after reaching it, claiming may be contentious.
 	absolute_timelock: u32,
+	// Timeout tx must have nLocktime set which means aggregating multiple
+	// ones must take the higher nLocktime among them to satisfy all of them.
+	// Sadly it has few pitfalls, a) it takes longuer to get fund back b) CLTV_DELTA
+	// of a sooner-HTLC could be swallowed by the highest nLocktime of the HTLC set.
+	// To simplify we mark them as non-aggregable.
+	aggregation: bool,
+
 }
 
 impl PackageTemplate {
@@ -491,6 +498,9 @@ impl PackageTemplate {
 	pub(crate) fn timelock(&self) -> u32 {
 		self.absolute_timelock
 	}
+	pub(crate) fn aggregation(&self) -> bool {
+		self.aggregation
+	}
 	pub(crate) fn outpoints(&self) -> Vec<&BitcoinOutPoint> {
 		let mut outpoints = Vec::with_capacity(self.inputs.len());
 		for (o, _) in self.inputs.iter() {
@@ -503,12 +513,14 @@ impl PackageTemplate {
 			PackageMalleability::Malleable => {
 				let mut split_package = None;
 				let timelock = self.absolute_timelock;
+				let aggregation = self.aggregation;
 				self.inputs.retain(|outp| {
 					if *split_outp == outp.0 {
 						split_package = Some(PackageTemplate {
 							inputs: vec![(outp.0, outp.1.clone())],
 							malleability: PackageMalleability::Malleable,
 							absolute_timelock: timelock,
+							aggregation,
 						});
 						return false;
 					}
@@ -530,6 +542,9 @@ impl PackageTemplate {
 		if self.malleability == PackageMalleability::Untractable || merge_from.malleability == PackageMalleability::Untractable {
 			panic!("Merging template on untractable packages");
 		}
+		if !self.aggregation || !merge_from.aggregation {
+			panic!("Merging non aggregatable packages");
+		}
 		let lead_category;
 		if let Some((_, lead_input)) = self.inputs.first() {
 			lead_category = lead_input.category();
@@ -614,13 +629,14 @@ impl PackageTemplate {
 			},
 		}
 	}
-	pub (crate) fn build_package(txid: Txid, vout: u32, input_solving_data: PackageSolvingData, malleability: PackageMalleability, absolute_timelock: u32) -> Self {
+	pub (crate) fn build_package(txid: Txid, vout: u32, input_solving_data: PackageSolvingData, malleability: PackageMalleability, absolute_timelock: u32, aggregation: bool) -> Self {
 		let mut inputs = Vec::with_capacity(1);
 		inputs.push((BitcoinOutPoint { txid, vout }, input_solving_data));
 		PackageTemplate {
 			inputs,
 			malleability,
-			absolute_timelock
+			absolute_timelock,
+			aggregation
 		}
 	}
 }
@@ -634,6 +650,7 @@ impl Writeable for PackageTemplate {
 		}
 		self.malleability.write(writer)?;
 		self.absolute_timelock.write(writer)?;
+		self.aggregation.write(writer)?;
 		Ok(())
 	}
 }
@@ -649,10 +666,12 @@ impl Readable for PackageTemplate {
 		}
 		let malleability = Readable::read(reader)?;
 		let absolute_timelock = Readable::read(reader)?;
+		let aggregation = Readable::read(reader)?;
 		Ok(PackageTemplate {
 			inputs,
 			malleability,
-			absolute_timelock
+			absolute_timelock,
+			aggregation
 		})
 	}
 }
@@ -676,12 +695,6 @@ impl Readable for PackageTemplate {
 /// Content embeds transactions elements to generate transaction. See PackageTemplate.
 #[derive(PartialEq, Clone)]
 pub struct OnchainRequest {
-	// Timeout tx must have nLocktime set which means aggregating multiple
-	// ones must take the higher nLocktime among them to satisfy all of them.
-	// Sadly it has few pitfalls, a) it takes longuer to get fund back b) CLTV_DELTA
-	// of a sooner-HTLC could be swallowed by the highest nLocktime of the HTLC set.
-	// Do simplify we mark them as non-aggregable.
-	pub(crate) aggregation: bool,
 	// Based feerate of previous broadcast. If resources available (either
 	// output value or utxo bumping).
 	pub(crate) feerate_previous: u64,
@@ -703,7 +716,6 @@ impl OnchainRequest {
 
 impl Writeable for OnchainRequest {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
-		self.aggregation.write(writer)?;
 		self.feerate_previous.write(writer)?;
 		self.height_timer.write(writer)?;
 		self.height_original.write(writer)?;
@@ -715,14 +727,12 @@ impl Writeable for OnchainRequest {
 
 impl Readable for OnchainRequest {
 	fn read<R: ::std::io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
-		let aggregation = Readable::read(reader)?;
 		let feerate_previous = Readable::read(reader)?;
 		let height_timer = Readable::read(reader)?;
 		let height_original = Readable::read(reader)?;
 		let content = Readable::read(reader)?;
 
 		Ok(OnchainRequest {
-			aggregation,
 			feerate_previous,
 			height_timer,
 			height_original,

lightning/src/chain/onchaintx.rs

@@ -375,7 +375,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
 			// 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.content.timelock(), height + CLTV_SHARED_CLAIM_BUFFER);
-				if req.content.timelock() <= height + CLTV_SHARED_CLAIM_BUFFER || !req.aggregation {
+				if req.content.timelock() <= height + CLTV_SHARED_CLAIM_BUFFER || !req.content.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() {