From: Matt Corallo Date: Wed, 18 Sep 2024 16:00:20 +0000 (+0000) Subject: Clean up `PackageTemplate::get_height_timer` to consider type X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=be915872c5d2d03fa45052c8567253d68303d222;p=rust-lightning 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. --- diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 47fa48b63..db6528205 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.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 diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 31346c6b7..9fe3def09 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/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); diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 947ec4a13..dfc6c02b6 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/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); } diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index 5d4f1540f..a05e40300 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/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);