diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 86f0d3de5ed..21a1f8be2be 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3018,7 +3018,7 @@ impl ChannelMonitorImpl { let commitment_package = PackageTemplate::build_package( self.funding_info.0.txid.clone(), self.funding_info.0.index as u32, PackageSolvingData::HolderFundingOutput(funding_outp), - self.best_block.height, self.best_block.height + self.best_block.height, ); let mut claimable_outpoints = vec![commitment_package]; let event = MonitorEvent::HolderForceClosedWithInfo { @@ -3041,7 +3041,7 @@ impl ChannelMonitorImpl { // assuming it gets confirmed in the next block. Sadly, we have code which considers // "not yet confirmed" things as discardable, so we cannot do that here. let (mut new_outpoints, _) = self.get_broadcasted_holder_claims( - &self.current_holder_commitment_tx, self.best_block.height + &self.current_holder_commitment_tx, self.best_block.height, ); let unsigned_commitment_tx = self.onchain_tx_handler.get_unsigned_holder_commitment_tx(); let new_outputs = self.get_broadcasted_holder_watch_outputs( @@ -3459,7 +3459,11 @@ impl ChannelMonitorImpl { for (idx, outp) in tx.output.iter().enumerate() { if outp.script_pubkey == revokeable_p2wsh { let revk_outp = RevokedOutput::build(per_commitment_point, self.counterparty_commitment_params.counterparty_delayed_payment_base_key, self.counterparty_commitment_params.counterparty_htlc_base_key, per_commitment_key, outp.value, self.counterparty_commitment_params.on_counterparty_tx_csv, self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx()); - let justice_package = PackageTemplate::build_package(commitment_txid, idx as u32, PackageSolvingData::RevokedOutput(revk_outp), height + self.counterparty_commitment_params.on_counterparty_tx_csv as u32, height); + let justice_package = PackageTemplate::build_package( + commitment_txid, idx as u32, + PackageSolvingData::RevokedOutput(revk_outp), + height + self.counterparty_commitment_params.on_counterparty_tx_csv as u32, + ); claimable_outpoints.push(justice_package); to_counterparty_output_info = Some((idx.try_into().expect("Txn can't have more than 2^32 outputs"), outp.value)); @@ -3476,7 +3480,12 @@ impl ChannelMonitorImpl { return (claimable_outpoints, to_counterparty_output_info); } let revk_htlc_outp = RevokedHTLCOutput::build(per_commitment_point, self.counterparty_commitment_params.counterparty_delayed_payment_base_key, self.counterparty_commitment_params.counterparty_htlc_base_key, per_commitment_key, htlc.amount_msat / 1000, htlc.clone(), &self.onchain_tx_handler.channel_transaction_parameters.channel_type_features); - let justice_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, PackageSolvingData::RevokedHTLCOutput(revk_htlc_outp), htlc.cltv_expiry, height); + let justice_package = PackageTemplate::build_package( + commitment_txid, + transaction_output_index, + PackageSolvingData::RevokedHTLCOutput(revk_htlc_outp), + htlc.cltv_expiry, + ); claimable_outpoints.push(justice_package); } } @@ -3598,7 +3607,7 @@ impl ChannelMonitorImpl { self.counterparty_commitment_params.counterparty_htlc_base_key, htlc.clone(), self.onchain_tx_handler.channel_type_features().clone())) }; - let counterparty_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, counterparty_htlc_outp, htlc.cltv_expiry, 0); + let counterparty_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, counterparty_htlc_outp, htlc.cltv_expiry); claimable_outpoints.push(counterparty_package); } } @@ -3642,7 +3651,7 @@ impl ChannelMonitorImpl { ); let justice_package = PackageTemplate::build_package( htlc_txid, idx as u32, PackageSolvingData::RevokedOutput(revk_outp), - height + self.counterparty_commitment_params.on_counterparty_tx_csv as u32, height + height + self.counterparty_commitment_params.on_counterparty_tx_csv as u32, ); claimable_outpoints.push(justice_package); if outputs_to_watch.is_none() { @@ -3665,11 +3674,11 @@ impl ChannelMonitorImpl { for &(ref htlc, _, _) in holder_tx.htlc_outputs.iter() { if let Some(transaction_output_index) = htlc.transaction_output_index { - let htlc_output = if htlc.offered { + let (htlc_output, counterparty_spendable_height) = if htlc.offered { let htlc_output = HolderHTLCOutput::build_offered( htlc.amount_msat, htlc.cltv_expiry, self.onchain_tx_handler.channel_type_features().clone() ); - htlc_output + (htlc_output, conf_height) } else { let payment_preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.payment_hash) { preimage.clone() @@ -3680,12 +3689,12 @@ impl ChannelMonitorImpl { let htlc_output = HolderHTLCOutput::build_accepted( payment_preimage, htlc.amount_msat, self.onchain_tx_handler.channel_type_features().clone() ); - htlc_output + (htlc_output, htlc.cltv_expiry) }; let htlc_package = PackageTemplate::build_package( holder_tx.txid, transaction_output_index, PackageSolvingData::HolderHTLCOutput(htlc_output), - htlc.cltv_expiry, conf_height + counterparty_spendable_height, ); claim_requests.push(htlc_package); } diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 7ac8f9a63f6..6039b7e5d49 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -765,9 +765,8 @@ impl OnchainTxHandler { continue; } - log_trace!(logger, "Test if outpoint can be aggregated with expiration {} against {}", req.timelock(), cur_height + CLTV_SHARED_CLAIM_BUFFER); - if req.timelock() <= cur_height + CLTV_SHARED_CLAIM_BUFFER || !req.aggregable() { - // Don't aggregate if outpoint package timelock is soon or marked as non-aggregable + log_trace!(logger, "Test if outpoint which our counterparty can spend at {} can be aggregated based on aggregation limit {}", req.counterparty_spendable_height(), cur_height + CLTV_SHARED_CLAIM_BUFFER); + if req.counterparty_spendable_height() <= cur_height + CLTV_SHARED_CLAIM_BUFFER || !req.aggregable() { preprocessed_requests.push(req); } else if aggregated_request.is_none() { aggregated_request = Some(req); @@ -885,15 +884,16 @@ impl OnchainTxHandler { if let Some((claim_id, _)) = self.claimable_outpoints.get(&inp.previous_output) { // If outpoint has claim request pending on it... if let Some(request) = self.pending_claim_requests.get_mut(claim_id) { - //... we need to verify equality between transaction outpoints and claim request - // outpoints to know if transaction is the original claim or a bumped one issued - // by us. - let mut are_sets_equal = true; + //... we need to check if the pending claim was for a subset of the outputs + // spent by the confirmed transaction. If so, we can drop the pending claim + // after ANTI_REORG_DELAY blocks, otherwise we need to split it and retry + // claiming the remaining outputs. + let mut is_claim_subset_of_tx = true; let mut tx_inputs = tx.input.iter().map(|input| &input.previous_output).collect::>(); tx_inputs.sort_unstable(); for request_input in request.outpoints() { if tx_inputs.binary_search(&request_input).is_err() { - are_sets_equal = false; + is_claim_subset_of_tx = false; break; } } @@ -915,7 +915,7 @@ impl OnchainTxHandler { // If this is our transaction (or our counterparty spent all the outputs // before we could anyway with same inputs order than us), wait for // ANTI_REORG_DELAY and clean the RBF tracking map. - if are_sets_equal { + if is_claim_subset_of_tx { clean_claim_request_after_safety_delay!(); } else { // If false, generate new claim request with update outpoint set let mut at_least_one_drop = false; diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 0ff7fd186cd..aefd1c5deb7 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 bool { self.malleability == PackageMalleability::Malleable } - pub(crate) fn timelock(&self) -> u32 { - self.soonest_conf_deadline + /// The height at which our counterparty may be able to spend this output. + /// + /// This is an important limit for aggregation as after this height our counterparty may be + /// able to pin transactions spending this output in the mempool. + pub(crate) fn counterparty_spendable_height(&self) -> u32 { + self.counterparty_spendable_height } pub(crate) fn aggregable(&self) -> bool { self.aggregable @@ -783,21 +794,18 @@ impl PackageTemplate { match self.malleability { PackageMalleability::Malleable => { let mut split_package = None; - let timelock = self.soonest_conf_deadline; let aggregable = self.aggregable; let feerate_previous = self.feerate_previous; let height_timer = self.height_timer; - let height_original = self.height_original; self.inputs.retain(|outp| { if *split_outp == outp.0 { split_package = Some(PackageTemplate { inputs: vec![(outp.0, outp.1.clone())], malleability: PackageMalleability::Malleable, - soonest_conf_deadline: timelock, + counterparty_spendable_height: self.counterparty_spendable_height, aggregable, feerate_previous, height_timer, - height_original, }); return false; } @@ -816,7 +824,6 @@ impl PackageTemplate { } } pub(crate) fn merge_package(&mut self, mut merge_from: PackageTemplate) { - assert_eq!(self.height_original, merge_from.height_original); if self.malleability == PackageMalleability::Untractable || merge_from.malleability == PackageMalleability::Untractable { panic!("Merging template on untractable packages"); } @@ -832,8 +839,8 @@ impl PackageTemplate { self.inputs.push((k, v)); } //TODO: verify coverage and sanity? - if self.soonest_conf_deadline > merge_from.soonest_conf_deadline { - self.soonest_conf_deadline = merge_from.soonest_conf_deadline; + if self.counterparty_spendable_height > merge_from.counterparty_spendable_height { + self.counterparty_spendable_height = merge_from.counterparty_spendable_height; } if self.feerate_previous > merge_from.feerate_previous { self.feerate_previous = merge_from.feerate_previous; @@ -947,18 +954,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 `counterparty_spendable_height`. + height_timer = cmp::min( + height_timer, + timer_for_target_conf(self.counterparty_spendable_height), + ); + }, + 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 + // `counterparty_spendable_height` holds the real HTLC expiry. + height_timer = cmp::min( + height_timer, + timer_for_target_conf(self.counterparty_spendable_height), + ); + }, + 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 @@ -1031,17 +1103,16 @@ impl PackageTemplate { }).is_some() } - pub (crate) fn build_package(txid: Txid, vout: u32, input_solving_data: PackageSolvingData, soonest_conf_deadline: u32, height_original: u32) -> Self { + pub (crate) fn build_package(txid: Txid, vout: u32, input_solving_data: PackageSolvingData, counterparty_spendable_height: u32) -> Self { let (malleability, aggregable) = PackageSolvingData::map_output_type_flags(&input_solving_data); let inputs = vec![(BitcoinOutPoint { txid, vout }, input_solving_data)]; PackageTemplate { inputs, malleability, - soonest_conf_deadline, + counterparty_spendable_height, aggregable, feerate_previous: 0, - height_timer: height_original, - height_original, + height_timer: 0, } } } @@ -1054,9 +1125,10 @@ impl Writeable for PackageTemplate { rev_outp.write(writer)?; } write_tlv_fields!(writer, { - (0, self.soonest_conf_deadline, required), + (0, self.counterparty_spendable_height, required), (2, self.feerate_previous, required), - (4, self.height_original, required), + // Prior to 0.1, the height at which the package's inputs were mined, but was always unused + (4, 0u32, required), (6, self.height_timer, required) }); Ok(()) @@ -1075,27 +1147,23 @@ impl Readable for PackageTemplate { let (malleability, aggregable) = if let Some((_, lead_input)) = inputs.first() { PackageSolvingData::map_output_type_flags(&lead_input) } else { return Err(DecodeError::InvalidValue); }; - let mut soonest_conf_deadline = 0; + let mut counterparty_spendable_height = 0; let mut feerate_previous = 0; let mut height_timer = None; - let mut height_original = 0; + let mut _height_original: Option = None; read_tlv_fields!(reader, { - (0, soonest_conf_deadline, required), + (0, counterparty_spendable_height, required), (2, feerate_previous, required), - (4, height_original, required), + (4, _height_original, option), // Written with a dummy value since 0.1 (6, height_timer, option), }); - if height_timer.is_none() { - height_timer = Some(height_original); - } Ok(PackageTemplate { inputs, malleability, - soonest_conf_deadline, + counterparty_spendable_height, aggregable, feerate_previous, - height_timer: height_timer.unwrap(), - height_original, + height_timer: height_timer.unwrap_or(0), }) } } @@ -1259,18 +1327,6 @@ mod tests { } } - #[test] - #[should_panic] - fn test_package_differing_heights() { - let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap(); - let secp_ctx = Secp256k1::new(); - let revk_outp = dumb_revk_output!(secp_ctx, false); - - let mut package_one_hundred = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000, 100); - let package_two_hundred = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000, 200); - package_one_hundred.merge_package(package_two_hundred); - } - #[test] #[should_panic] fn test_package_untractable_merge_to() { @@ -1279,8 +1335,8 @@ mod tests { let revk_outp = dumb_revk_output!(secp_ctx, false); let htlc_outp = dumb_htlc_output!(); - let mut untractable_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000, 100); - let malleable_package = PackageTemplate::build_package(txid, 1, htlc_outp.clone(), 1000, 100); + let mut untractable_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000); + let malleable_package = PackageTemplate::build_package(txid, 1, htlc_outp.clone(), 1000); untractable_package.merge_package(malleable_package); } @@ -1292,8 +1348,8 @@ mod tests { let htlc_outp = dumb_htlc_output!(); let revk_outp = dumb_revk_output!(secp_ctx, false); - let mut malleable_package = PackageTemplate::build_package(txid, 0, htlc_outp.clone(), 1000, 100); - let untractable_package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000, 100); + let mut malleable_package = PackageTemplate::build_package(txid, 0, htlc_outp.clone(), 1000); + let untractable_package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000); malleable_package.merge_package(untractable_package); } @@ -1305,8 +1361,8 @@ mod tests { let revk_outp = dumb_revk_output!(secp_ctx, false); let revk_outp_counterparty_balance = dumb_revk_output!(secp_ctx, true); - let mut noaggregation_package = PackageTemplate::build_package(txid, 0, revk_outp_counterparty_balance.clone(), 1000, 100); - let aggregation_package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000, 100); + let mut noaggregation_package = PackageTemplate::build_package(txid, 0, revk_outp_counterparty_balance.clone(), 1000); + let aggregation_package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000); noaggregation_package.merge_package(aggregation_package); } @@ -1318,8 +1374,8 @@ mod tests { let revk_outp = dumb_revk_output!(secp_ctx, false); let revk_outp_counterparty_balance = dumb_revk_output!(secp_ctx, true); - let mut aggregation_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000, 100); - let noaggregation_package = PackageTemplate::build_package(txid, 1, revk_outp_counterparty_balance.clone(), 1000, 100); + let mut aggregation_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000); + let noaggregation_package = PackageTemplate::build_package(txid, 1, revk_outp_counterparty_balance.clone(), 1000); aggregation_package.merge_package(noaggregation_package); } @@ -1330,9 +1386,9 @@ mod tests { let secp_ctx = Secp256k1::new(); let revk_outp = dumb_revk_output!(secp_ctx, false); - let mut empty_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000, 100); + let mut empty_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000); empty_package.inputs = vec![]; - let package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000, 100); + let package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000); empty_package.merge_package(package); } @@ -1344,8 +1400,8 @@ mod tests { let revk_outp = dumb_revk_output!(secp_ctx, false); let counterparty_outp = dumb_counterparty_output!(secp_ctx, 0, ChannelTypeFeatures::only_static_remote_key()); - let mut revoked_package = PackageTemplate::build_package(txid, 0, revk_outp, 1000, 100); - let counterparty_package = PackageTemplate::build_package(txid, 1, counterparty_outp, 1000, 100); + let mut revoked_package = PackageTemplate::build_package(txid, 0, revk_outp, 1000); + let counterparty_package = PackageTemplate::build_package(txid, 1, counterparty_outp, 1000); revoked_package.merge_package(counterparty_package); } @@ -1357,9 +1413,9 @@ mod tests { let revk_outp_two = dumb_revk_output!(secp_ctx, false); let revk_outp_three = dumb_revk_output!(secp_ctx, false); - let mut package_one = PackageTemplate::build_package(txid, 0, revk_outp_one, 1000, 100); - let package_two = PackageTemplate::build_package(txid, 1, revk_outp_two, 1000, 100); - let package_three = PackageTemplate::build_package(txid, 2, revk_outp_three, 1000, 100); + let mut package_one = PackageTemplate::build_package(txid, 0, revk_outp_one, 1000); + let package_two = PackageTemplate::build_package(txid, 1, revk_outp_two, 1000); + let package_three = PackageTemplate::build_package(txid, 2, revk_outp_three, 1000); package_one.merge_package(package_two); package_one.merge_package(package_three); @@ -1368,11 +1424,10 @@ mod tests { if let Some(split_package) = package_one.split_package(&BitcoinOutPoint { txid, vout: 1 }) { // Packages attributes should be identical assert!(split_package.is_malleable()); - assert_eq!(split_package.soonest_conf_deadline, package_one.soonest_conf_deadline); + assert_eq!(split_package.counterparty_spendable_height, package_one.counterparty_spendable_height); assert_eq!(split_package.aggregable, package_one.aggregable); assert_eq!(split_package.feerate_previous, package_one.feerate_previous); assert_eq!(split_package.height_timer, package_one.height_timer); - assert_eq!(split_package.height_original, package_one.height_original); } else { panic!(); } assert_eq!(package_one.outpoints().len(), 2); } @@ -1382,7 +1437,7 @@ mod tests { let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap(); let htlc_outp_one = dumb_htlc_output!(); - let mut package_one = PackageTemplate::build_package(txid, 0, htlc_outp_one, 1000, 100); + let mut package_one = PackageTemplate::build_package(txid, 0, htlc_outp_one, 1000); let ret_split = package_one.split_package(&BitcoinOutPoint { txid, vout: 0}); assert!(ret_split.is_none()); } @@ -1393,8 +1448,8 @@ mod tests { let secp_ctx = Secp256k1::new(); let revk_outp = dumb_revk_output!(secp_ctx, false); - let mut package = PackageTemplate::build_package(txid, 0, revk_outp, 1000, 100); - assert_eq!(package.timer(), 100); + let mut package = PackageTemplate::build_package(txid, 0, revk_outp, 1000); + assert_eq!(package.timer(), 0); package.set_timer(101); assert_eq!(package.timer(), 101); } @@ -1405,7 +1460,7 @@ mod tests { let secp_ctx = Secp256k1::new(); let counterparty_outp = dumb_counterparty_output!(secp_ctx, 1_000_000, ChannelTypeFeatures::only_static_remote_key()); - let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000, 100); + let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000); assert_eq!(package.package_amount(), 1000); } @@ -1419,14 +1474,14 @@ mod tests { { let revk_outp = dumb_revk_output!(secp_ctx, false); - let package = PackageTemplate::build_package(txid, 0, revk_outp, 0, 100); + let package = PackageTemplate::build_package(txid, 0, revk_outp, 0); assert_eq!(package.package_weight(&ScriptBuf::new()), weight_sans_output + WEIGHT_REVOKED_OUTPUT); } { for channel_type_features in [ChannelTypeFeatures::only_static_remote_key(), ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()].iter() { let counterparty_outp = dumb_counterparty_output!(secp_ctx, 1_000_000, channel_type_features.clone()); - let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000, 100); + let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000); assert_eq!(package.package_weight(&ScriptBuf::new()), weight_sans_output + weight_received_htlc(channel_type_features)); } } @@ -1434,7 +1489,7 @@ mod tests { { for channel_type_features in [ChannelTypeFeatures::only_static_remote_key(), ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()].iter() { let counterparty_outp = dumb_counterparty_offered_output!(secp_ctx, 1_000_000, channel_type_features.clone()); - let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000, 100); + let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000); assert_eq!(package.package_weight(&ScriptBuf::new()), weight_sans_output + weight_offered_htlc(channel_type_features)); } } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 31346c6b78b..ae9ea040aa7 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -15,7 +15,7 @@ use crate::chain; use crate::chain::{ChannelMonitorUpdateStatus, Confirm, Listen, Watch}; use crate::chain::chaininterface::LowerBoundedFeeEstimator; use crate::chain::channelmonitor; -use crate::chain::channelmonitor::{CLOSED_CHANNEL_UPDATE_ID, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY}; +use crate::chain::channelmonitor::{Balance, CLOSED_CHANNEL_UPDATE_ID, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY}; use crate::chain::transaction::OutPoint; use crate::sign::{ecdsa::EcdsaChannelSigner, EntropySource, OutputSpender, SignerProvider}; use crate::events::{Event, FundingInfo, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose, ClosureReason, HTLCDestination, PaymentFailureReason}; @@ -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); @@ -3138,19 +3138,28 @@ fn do_test_htlc_on_chain_timeout(connect_style: ConnectStyle) { mine_transaction(&nodes[1], &commitment_tx[0]); check_closed_event!(&nodes[1], 1, ClosureReason::CommitmentTxConfirmed, false , [nodes[2].node.get_our_node_id()], 100000); - connect_blocks(&nodes[1], 200 - nodes[2].best_block_info().1); + let htlc_expiry = get_monitor!(nodes[1], chan_2.2).get_claimable_balances().iter().filter_map(|bal| + if let Balance::MaybeTimeoutClaimableHTLC { claimable_height, .. } = bal { + Some(*claimable_height) + } else { + None + } + ).next().unwrap(); + connect_blocks(&nodes[1], htlc_expiry - nodes[1].best_block_info().1); let timeout_tx = { let mut txn = nodes[1].tx_broadcaster.txn_broadcast(); - if nodes[1].connect_style.borrow().skips_blocks() { - assert_eq!(txn.len(), 1); - } else { - assert_eq!(txn.len(), 3); // Two extra fee bumps for timeout transaction - } + assert_eq!(txn.len(), 1); txn.iter().for_each(|tx| check_spends!(tx, commitment_tx[0])); assert_eq!(txn[0].clone().input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); txn.remove(0) }; + // Make sure that if we connect only one block we aren't aggressively fee-bumping the HTLC + // claim which was only just broadcasted (and as at least `MIN_CLTV_EXPIRY_DELTA` blocks to + // confirm). + connect_blocks(&nodes[1], 1); + assert_eq!(nodes[1].tx_broadcaster.txn_broadcast().len(), 0); + mine_transaction(&nodes[1], &timeout_tx); check_added_monitors!(nodes[1], 1); check_closed_broadcast!(nodes[1], true); @@ -7805,7 +7814,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 947ec4a1304..dfc6c02b61a 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 5d4f1540fb9..a05e40300fc 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);