Make PackageTemplate::height_timer non-optional

Now that we leverage a package's `height_timer` even for untractable
packages, there's no need to have it be an `Option` anymore. We aim to
not break compatibility by keeping the deserialization of such as an
`option`, and use the package's `height_original` when not present. This
allows us to retry packages from older `ChannelMonitor` versions that
have had a failed initial package broadcast.

Author: wpaulino

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)));
 			}
@@ -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]