From 1c825e1f1898f8cfeda492f0335286a550380213 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 18 Sep 2024 16:00:20 +0000 Subject: [PATCH] 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. --- lightning/src/chain/package.rs | 91 +++++++++++++++++++++++++--- lightning/src/ln/functional_tests.rs | 4 +- 2 files changed, 83 insertions(+), 12 deletions(-) diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 2dcaa8800..f6809551b 100644 --- a/lightning/src/chain/package.rs +++ b/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 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.htlc.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), + ); + }, + 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 diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 22b7df1ed..c2a32d3c9 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -2809,7 +2809,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); @@ -7803,7 +7803,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); -- 2.39.5