Merge pull request #2203 from wpaulino/retry-untractable-packages

Retry untractable packages

Author: TheBlueMatt

lightning/src/chain/onchaintx.rs

@@ -489,7 +489,7 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 	///
 	/// Panics if there are signing errors, because signing operations in reaction to on-chain
 	/// events are not expected to fail, and if they do, we may lose funds.
-	fn generate_claim<F: Deref, L: Deref>(&mut self, cur_height: u32, cached_request: &PackageTemplate, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L) -> Option<(Option<u32>, u64, OnchainClaim)>
+	fn generate_claim<F: Deref, L: Deref>(&mut self, cur_height: u32, cached_request: &PackageTemplate, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L) -> Option<(u32, u64, OnchainClaim)>
 		where F::Target: FeeEstimator,
 					L::Target: Logger,
 	{
@@ -533,7 +533,7 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> 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(cached_request.get_height_timer(cur_height));
+		let new_timer = cached_request.get_height_timer(cur_height);
 		if cached_request.is_malleable() {
 			#[cfg(anchors)]
 			{ // Attributes are not allowed on if expressions on our current MSRV of 1.41.
@@ -565,7 +565,7 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 				let transaction = cached_request.finalize_malleable_package(
 					cur_height, self, output_value, self.destination_script.clone(), logger
 				).unwrap();
-				log_trace!(logger, "...with timer {} and feerate {}", new_timer.unwrap(), new_feerate);
+				log_trace!(logger, "...with timer {} and feerate {}", new_timer, new_feerate);
 				assert!(predicted_weight >= transaction.weight());
 				return Some((new_timer, new_feerate, OnchainClaim::Tx(transaction)));
 			}
@@ -583,7 +583,7 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 				None => return None,
 			};
 			if !cached_request.requires_external_funding() {
-				return Some((None, 0, OnchainClaim::Tx(tx)));
+				return Some((new_timer, 0, OnchainClaim::Tx(tx)));
 			}
 			#[cfg(anchors)]
 			return inputs.find_map(|input| match input {
@@ -616,7 +616,7 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 						// attempt to broadcast the transaction with its current fee rate and hope
 						// it confirms. This is essentially the same behavior as a commitment
 						// transaction without anchor outputs.
-						None => Some((None, 0, OnchainClaim::Tx(tx.clone()))),
+						None => Some((new_timer, 0, OnchainClaim::Tx(tx.clone()))),
 					}
 				},
 				_ => {
@@ -885,10 +885,8 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
 
 		// Check if any pending claim request must be rescheduled
 		for (package_id, request) in self.pending_claim_requests.iter() {
-			if let Some(h) = request.timer() {
-				if cur_height >= h {
-					bump_candidates.insert(*package_id, request.clone());
-				}
+			if cur_height >= request.timer() {
+				bump_candidates.insert(*package_id, request.clone());
 			}
 		}
 

lightning/src/chain/package.rs

@@ -538,7 +538,7 @@ pub struct PackageTemplate {
 	feerate_previous: u64,
 	// Cache of next height at which fee-bumping and rebroadcast will be attempted. In
 	// the future, we might abstract it to an observed mempool fluctuation.
-	height_timer: Option<u32>,
+	height_timer: u32,
 	// Confirmation height of the claimed outputs set transaction. In case of reorg reaching
 	// it, we wipe out and forget the package.
 	height_original: u32,
@@ -557,13 +557,10 @@ impl PackageTemplate {
 	pub(crate) fn set_feerate(&mut self, new_feerate: u64) {
 		self.feerate_previous = new_feerate;
 	}
-	pub(crate) fn timer(&self) -> Option<u32> {
-		if let Some(ref timer) = self.height_timer {
-			return Some(*timer);
-		}
-		None
+	pub(crate) fn timer(&self) -> u32 {
+		self.height_timer
 	}
-	pub(crate) fn set_timer(&mut self, new_timer: Option<u32>) {
+	pub(crate) fn set_timer(&mut self, new_timer: u32) {
 		self.height_timer = new_timer;
 	}
 	pub(crate) fn outpoints(&self) -> Vec<&BitcoinOutPoint> {
@@ -837,7 +834,7 @@ impl PackageTemplate {
 			soonest_conf_deadline,
 			aggregable,
 			feerate_previous: 0,
-			height_timer: None,
+			height_timer: height_original,
 			height_original,
 		}
 	}
@@ -854,7 +851,7 @@ impl Writeable for PackageTemplate {
 			(0, self.soonest_conf_deadline, required),
 			(2, self.feerate_previous, required),
 			(4, self.height_original, required),
-			(6, self.height_timer, option)
+			(6, self.height_timer, required)
 		});
 		Ok(())
 	}
@@ -893,13 +890,16 @@ impl Readable for PackageTemplate {
 			(4, height_original, required),
 			(6, height_timer, option),
 		});
+		if height_timer.is_none() {
+			height_timer = Some(height_original);
+		}
 		Ok(PackageTemplate {
 			inputs,
 			malleability,
 			soonest_conf_deadline,
 			aggregable,
 			feerate_previous,
-			height_timer,
+			height_timer: height_timer.unwrap(),
 			height_original,
 		})
 	}
@@ -1177,12 +1177,9 @@ mod tests {
 		let revk_outp = dumb_revk_output!(secp_ctx);
 
 		let mut package = PackageTemplate::build_package(txid, 0, revk_outp, 1000, true, 100);
-		let timer_none = package.timer();
-		assert!(timer_none.is_none());
-		package.set_timer(Some(100));
-		if let Some(timer_some) = package.timer() {
-			assert_eq!(timer_some, 100);
-		} else { panic!() }
+		assert_eq!(package.timer(), 100);
+		package.set_timer(101);
+		assert_eq!(package.timer(), 101);
 	}
 
 	#[test]

lightning/src/ln/functional_test_utils.rs

@@ -137,6 +137,20 @@ pub enum ConnectStyle {
 }
 
 impl ConnectStyle {
+	pub fn skips_blocks(&self) -> bool {
+		match self {
+			ConnectStyle::BestBlockFirst => false,
+			ConnectStyle::BestBlockFirstSkippingBlocks => true,
+			ConnectStyle::BestBlockFirstReorgsOnlyTip => true,
+			ConnectStyle::TransactionsFirst => false,
+			ConnectStyle::TransactionsFirstSkippingBlocks => true,
+			ConnectStyle::TransactionsDuplicativelyFirstSkippingBlocks => true,
+			ConnectStyle::HighlyRedundantTransactionsFirstSkippingBlocks => true,
+			ConnectStyle::TransactionsFirstReorgsOnlyTip => true,
+			ConnectStyle::FullBlockViaListen => false,
+		}
+	}
+
 	fn random_style() -> ConnectStyle {
 		#[cfg(feature = "std")] {
 			use core::hash::{BuildHasher, Hasher};
@@ -164,12 +178,7 @@ impl ConnectStyle {
 }
 
 pub fn connect_blocks<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, depth: u32) -> BlockHash {
-	let skip_intermediaries = match *node.connect_style.borrow() {
-		ConnectStyle::BestBlockFirstSkippingBlocks|ConnectStyle::TransactionsFirstSkippingBlocks|
-			ConnectStyle::TransactionsDuplicativelyFirstSkippingBlocks|ConnectStyle::HighlyRedundantTransactionsFirstSkippingBlocks|
-			ConnectStyle::BestBlockFirstReorgsOnlyTip|ConnectStyle::TransactionsFirstReorgsOnlyTip => true,
-		_ => false,
-	};
+	let skip_intermediaries = node.connect_style.borrow().skips_blocks();
 
 	let height = node.best_block_info().1 + 1;
 	let mut block = Block {
@@ -2535,6 +2544,8 @@ pub enum HTLCType { NONE, TIMEOUT, SUCCESS }
 /// also fail.
 pub fn test_txn_broadcast<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, chan: &(msgs::ChannelUpdate, msgs::ChannelUpdate, [u8; 32], Transaction), commitment_tx: Option<Transaction>, has_htlc_tx: HTLCType) -> Vec<Transaction>  {
 	let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap();
+	let mut txn_seen = HashSet::new();
+	node_txn.retain(|tx| txn_seen.insert(tx.txid()));
 	assert!(node_txn.len() >= if commitment_tx.is_some() { 0 } else { 1 } + if has_htlc_tx == HTLCType::NONE { 0 } else { 1 });
 
 	let mut res = Vec::with_capacity(2);
@@ -2598,22 +2609,23 @@ pub fn test_revoked_htlc_claim_txn_broadcast<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>
 
 pub fn check_preimage_claim<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, prev_txn: &Vec<Transaction>) -> Vec<Transaction>  {
 	let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap();
+	let mut txn_seen = HashSet::new();
+	node_txn.retain(|tx| txn_seen.insert(tx.txid()));
 
-	assert!(node_txn.len() >= 1);
-	assert_eq!(node_txn[0].input.len(), 1);
 	let mut found_prev = false;
-
-	for tx in prev_txn {
-		if node_txn[0].input[0].previous_output.txid == tx.txid() {
-			check_spends!(node_txn[0], tx);
-			let mut iter = node_txn[0].input[0].witness.iter();
-			iter.next().expect("expected 3 witness items");
-			iter.next().expect("expected 3 witness items");
-			assert!(iter.next().expect("expected 3 witness items").len() > 106); // must spend an htlc output
-			assert_eq!(tx.input.len(), 1); // must spend a commitment tx
-
-			found_prev = true;
-			break;
+	for prev_tx in prev_txn {
+		for tx in &*node_txn {
+			if tx.input[0].previous_output.txid == prev_tx.txid() {
+				check_spends!(tx, prev_tx);
+				let mut iter = tx.input[0].witness.iter();
+				iter.next().expect("expected 3 witness items");
+				iter.next().expect("expected 3 witness items");
+				assert!(iter.next().expect("expected 3 witness items").len() > 106); // must spend an htlc output
+				assert_eq!(tx.input.len(), 1); // must spend a commitment tx
+
+				found_prev = true;
+				break;
+			}
 		}
 	}
 	assert!(found_prev);