]> git.bitcoin.ninja Git - rust-lightning/commitdiff
Clean up `PackageTemplate::get_height_timer` to consider type
authorMatt Corallo <git@bluematt.me>
Wed, 18 Sep 2024 16:00:20 +0000 (16:00 +0000)
committerMatt Corallo <git@bluematt.me>
Thu, 17 Oct 2024 18:34:33 +0000 (18:34 +0000)
`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
lightning/src/ln/functional_tests.rs
lightning/src/ln/monitor_tests.rs
lightning/src/ln/reorg_tests.rs

index 47fa48b63738c9d3b7e9dae7a09e7fb1ad4df496..db652820524c280147158ac90a9c5c60c13835a2 100644 (file)
@@ -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
index 31346c6b78b72b5e9ad4dfabffd380babc65e119..9fe3def09c5efff691aef626acfdbcd9a5fe8b40 100644 (file)
@@ -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);
index 947ec4a1304294c1a816930171e6273a6dbadbf4..dfc6c02b61a1769b29a810d02d8347d69513657e 100644 (file)
@@ -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);
 }
index 5d4f1540fb995a66a2a3f50e050ff583e35236e0..a05e40300fc1113f6768ade9dca7600dc18bd28d 100644 (file)
@@ -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);