Clean up PackageTemplate::get_height_timer to consider type

`PackageTemplate::get_height_timer` is used to decide when to next
bump our feerate on claims which need to make it on chain within
some window. It does so by comparing the current height with some
deadline and increasing the bump rate as the deadline approaches.

However, the deadline used is the `counterparty_spendable_height`,
which is the height at which the counterparty might be able to
spend the same output, irrespective of why. This doesn't make sense
for all output types, for example outbound HTLCs are spendable by
our counteraprty immediately (by revealing the preimage), but we
don't need to get our HTLC timeout claims confirmed immedaitely,
as we actually have `MIN_CLTV_EXPIRY` blocks before the inbound
edge of a forwarded HTLC becomes claimable by our (other)
counterparty.

Thus, here, we adapt `get_height_timer` to look at the type of
output being claimed, and adjust the rate at which we bump the fee
according to the real deadline.

Author: TheBlueMatt

lightning/src/chain/package.rs

@@ -28,6 +28,7 @@ use crate::ln::types::PaymentPreimage;
 use crate::ln::chan_utils::{self, TxCreationKeys, HTLCOutputInCommitment};
 use crate::ln::features::ChannelTypeFeatures;
 use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint};
+use crate::ln::channelmanager::MIN_CLTV_EXPIRY_DELTA;
 use crate::ln::msgs::DecodeError;
 use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, MIN_RELAY_FEE_SAT_PER_1000_WEIGHT, compute_feerate_sat_per_1000_weight, FEERATE_FLOOR_SATS_PER_KW};
 use crate::chain::transaction::MaybeSignedTransaction;
@@ -104,8 +105,13 @@ pub(crate) fn verify_channel_type_features(channel_type_features: &Option<Channe
 // number_of_witness_elements + sig_length + revocation_sig + true_length + op_true + witness_script_length + witness_script
 pub(crate) const WEIGHT_REVOKED_OUTPUT: u64 = 1 + 1 + 73 + 1 + 1 + 1 + 77;
 
+#[cfg(not(test))]
 /// Height delay at which transactions are fee-bumped/rebroadcasted with a low priority.
 const LOW_FREQUENCY_BUMP_INTERVAL: u32 = 15;
+#[cfg(test)]
+/// Height delay at which transactions are fee-bumped/rebroadcasted with a low priority.
+pub(crate) const LOW_FREQUENCY_BUMP_INTERVAL: u32 = 15;
+
 /// Height delay at which transactions are fee-bumped/rebroadcasted with a middle priority.
 const MIDDLE_FREQUENCY_BUMP_INTERVAL: u32 = 3;
 /// Height delay at which transactions are fee-bumped/rebroadcasted with a high priority.
@@ -945,18 +951,83 @@ impl PackageTemplate {
 			return None;
 		} else { panic!("API Error: Package must not be inputs empty"); }
 	}
-	/// In LN, output claimed are time-sensitive, which means we have to spend them before reaching some timelock expiration. At in-channel
-	/// output detection, we generate a first version of a claim tx and associate to it a height timer. A height timer is an absolute block
-	/// height that once reached we should generate a new bumped "version" of the claim tx to be sure that we safely claim outputs before
-	/// that our counterparty can do so. If timelock expires soon, height timer is going to be scaled down in consequence to increase
-	/// frequency of the bump and so increase our bets of success.
+	/// Gets the next height at which we should fee-bump this package, assuming we can do so and
+	/// the package is last fee-bumped at `current_height`.
+	///
+	/// As the deadline with which to get a claim confirmed approaches, the rate at which the timer
+	/// ticks increases.
 	pub(crate) fn get_height_timer(&self, current_height: u32) -> u32 {
-		if self.soonest_conf_deadline <= current_height + MIDDLE_FREQUENCY_BUMP_INTERVAL {
-			return current_height + HIGH_FREQUENCY_BUMP_INTERVAL
-		} else if self.soonest_conf_deadline - current_height <= LOW_FREQUENCY_BUMP_INTERVAL {
-			return current_height + MIDDLE_FREQUENCY_BUMP_INTERVAL
+		let mut height_timer = current_height + LOW_FREQUENCY_BUMP_INTERVAL;
+		let timer_for_target_conf = |target_conf| -> u32 {
+			if target_conf <= current_height + MIDDLE_FREQUENCY_BUMP_INTERVAL {
+				current_height + HIGH_FREQUENCY_BUMP_INTERVAL
+			} else if target_conf <= current_height + LOW_FREQUENCY_BUMP_INTERVAL {
+				current_height + MIDDLE_FREQUENCY_BUMP_INTERVAL
+			} else {
+				current_height + LOW_FREQUENCY_BUMP_INTERVAL
+			}
+		};
+		for (_, input) in self.inputs.iter() {
+			match input {
+				PackageSolvingData::RevokedOutput(_) => {
+					// Revoked Outputs will become spendable by our counterparty at the height
+					// where the CSV expires, which is also our `soonest_conf_deadline`.
+					height_timer = cmp::min(
+						height_timer,
+						timer_for_target_conf(self.soonest_conf_deadline),
+					);
+				},
+				PackageSolvingData::RevokedHTLCOutput(_) => {
+					// Revoked HTLC Outputs may be spendable by our counterparty right now, but
+					// after they spend them they still have to wait for an additional CSV delta
+					// before they can claim the full funds. Thus, we leave the timer at
+					// `LOW_FREQUENCY_BUMP_INTERVAL` until the HTLC output is spent, creating a
+					// `RevokedOutput`.
+				},
+				PackageSolvingData::CounterpartyOfferedHTLCOutput(outp) => {
+					// Incoming HTLCs being claimed by preimage should be claimed by the time their
+					// CLTV unlocks.
+					height_timer = cmp::min(
+						height_timer,
+						timer_for_target_conf(outp.htlc.cltv_expiry),
+					);
+				},
+				PackageSolvingData::HolderHTLCOutput(outp) if outp.preimage.is_some() => {
+					// We have the same deadline here as for `CounterpartyOfferedHTLCOutput`. Note
+					// that `outp.cltv_expiry` is always 0 in this case, but
+					// `soonest_conf_deadline` holds the real HTLC expiry.
+					height_timer = cmp::min(
+						height_timer,
+						timer_for_target_conf(self.soonest_conf_deadline),
+					);
+				},
+				PackageSolvingData::CounterpartyReceivedHTLCOutput(outp) => {
+					// Outgoing HTLCs being claimed through their timeout should be claimed fast
+					// enough to allow us to claim before the CLTV lock expires on the inbound
+					// edge (assuming the HTLC was forwarded).
+					height_timer = cmp::min(
+						height_timer,
+						timer_for_target_conf(outp.htlc.cltv_expiry + MIN_CLTV_EXPIRY_DELTA as u32),
+					);
+				},
+				PackageSolvingData::HolderHTLCOutput(outp) => {
+					// We have the same deadline for holder timeout claims as for
+					// `CounterpartyReceivedHTLCOutput`
+					height_timer = cmp::min(
+						height_timer,
+						timer_for_target_conf(outp.cltv_expiry + MIN_CLTV_EXPIRY_DELTA as u32),
+					);
+				},
+				PackageSolvingData::HolderFundingOutput(_) => {
+					// We should apply a smart heuristic here based on the HTLCs in the commitment
+					// transaction, but we don't currently have that information available so
+					// instead we just bump once per block.
+					height_timer =
+						cmp::min(height_timer, current_height + HIGH_FREQUENCY_BUMP_INTERVAL);
+				},
+			}
 		}
-		current_height + LOW_FREQUENCY_BUMP_INTERVAL
+		height_timer
 	}
 
 	/// Returns value in satoshis to be included as package outgoing output amount and feerate

lightning/src/ln/functional_tests.rs

@@ -2811,7 +2811,7 @@ fn claim_htlc_outputs_single_tx() {
 
 		// Filter out any non justice transactions.
 		node_txn.retain(|tx| tx.input[0].previous_output.txid == revoked_local_txn[0].compute_txid());
-		assert!(node_txn.len() > 3);
+		assert!(node_txn.len() >= 3);
 
 		assert_eq!(node_txn[0].input.len(), 1);
 		assert_eq!(node_txn[1].input.len(), 1);
@@ -7805,7 +7805,7 @@ fn test_bump_penalty_txn_on_remote_commitment() {
 	assert_ne!(feerate_preimage, 0);
 
 	// After exhaustion of height timer, new bumped claim txn should have been broadcast, check it
-	connect_blocks(&nodes[1], 1);
+	connect_blocks(&nodes[1], crate::chain::package::LOW_FREQUENCY_BUMP_INTERVAL);
 	{
 		let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
 		assert_eq!(node_txn.len(), 1);

lightning/src/ln/monitor_tests.rs

@@ -2299,17 +2299,13 @@ fn do_test_restored_packages_retry(check_old_monitor_retries_after_upgrade: bool
 	}
 
 	// Connecting more blocks should result in the HTLC transactions being rebroadcast.
-	connect_blocks(&nodes[0], 6);
+	connect_blocks(&nodes[0], crate::chain::package::LOW_FREQUENCY_BUMP_INTERVAL);
 	if check_old_monitor_retries_after_upgrade {
 		check_added_monitors(&nodes[0], 1);
 	}
 	{
 		let txn = nodes[0].tx_broadcaster.txn_broadcast();
-		if !nodes[0].connect_style.borrow().skips_blocks() {
-			assert_eq!(txn.len(), 6);
-		} else {
-			assert!(txn.len() < 6);
-		}
+		assert_eq!(txn.len(), 1);
 		for tx in txn {
 			assert_eq!(tx.input.len(), htlc_timeout_tx.input.len());
 			assert_eq!(tx.output.len(), htlc_timeout_tx.output.len());
@@ -2423,9 +2419,9 @@ fn do_test_monitor_rebroadcast_pending_claims(anchors: bool) {
 	connect_blocks(&nodes[0], 1);
 	check_htlc_retry(true, false);
 
-	// Connect one more block, expecting a retry with a fee bump. Unfortunately, we cannot bump HTLC
-	// transactions pre-anchors.
-	connect_blocks(&nodes[0], 1);
+	// Connect a few more blocks, expecting a retry with a fee bump. Unfortunately, we cannot bump
+	// HTLC transactions pre-anchors.
+	connect_blocks(&nodes[0], crate::chain::package::LOW_FREQUENCY_BUMP_INTERVAL);
 	check_htlc_retry(true, anchors);
 
 	// Trigger a call and we should have another retry, but without a bump.
@@ -2437,20 +2433,13 @@ fn do_test_monitor_rebroadcast_pending_claims(anchors: bool) {
 	nodes[0].chain_monitor.chain_monitor.rebroadcast_pending_claims();
 	check_htlc_retry(true, anchors);
 
-	// Connect one more block, expecting a retry with a fee bump. Unfortunately, we cannot bump HTLC
-	// transactions pre-anchors.
-	connect_blocks(&nodes[0], 1);
+	// Connect a few more blocks, expecting a retry with a fee bump. Unfortunately, we cannot bump
+	// HTLC transactions pre-anchors.
+	connect_blocks(&nodes[0], crate::chain::package::LOW_FREQUENCY_BUMP_INTERVAL);
 	let htlc_tx = check_htlc_retry(true, anchors).unwrap();
 
 	// Mine the HTLC transaction to ensure we don't retry claims while they're confirmed.
 	mine_transaction(&nodes[0], &htlc_tx);
-	// If we have a `ConnectStyle` that advertises the new block first without the transactions,
-	// we'll receive an extra bumped claim.
-	if nodes[0].connect_style.borrow().updates_best_block_first() {
-		nodes[0].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.compute_txid(), vout: 0 }, coinbase_tx.output[0].value);
-		nodes[0].wallet_source.remove_utxo(bitcoin::OutPoint { txid: htlc_tx.compute_txid(), vout: 1 });
-		check_htlc_retry(true, anchors);
-	}
 	nodes[0].chain_monitor.chain_monitor.rebroadcast_pending_claims();
 	check_htlc_retry(false, false);
 }

lightning/src/ln/reorg_tests.rs

@@ -847,9 +847,9 @@ fn do_test_retries_own_commitment_broadcast_after_reorg(anchors: bool, revoked_c
 	{
 		let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
 		if nodes[0].connect_style.borrow().updates_best_block_first() {
-			// `commitment_a` and `htlc_timeout_a` are rebroadcast because the best block was
-			// updated prior to seeing `commitment_b`.
-			assert_eq!(txn.len(), if anchors { 2 } else { 3 });
+			// `commitment_a` is rebroadcast because the best block was updated prior to seeing
+			// `commitment_b`.
+			assert_eq!(txn.len(), 2);
 			check_spends!(txn.last().unwrap(), commitment_b);
 		} else {
 			assert_eq!(txn.len(), 1);