From 2f549eeb80b497492fd4914d8ae7d41ebb2060f6 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 5 Sep 2024 21:06:16 +0000 Subject: [PATCH 1/8] Rename claim cleaning match bool for accuracy We don't actually care if a confirmed transaction claimed other outputs, only that it claimed a superset of the outputs in the pending claim we're looking at. Thus, the variable to detect that is renamed `is_claim_subset_of_tx` instead of `are_sets_equal`. --- lightning/src/chain/onchaintx.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 7ac8f9a63f6..e1f3ebf049b 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -885,15 +885,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 +916,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; From a19edbc32dcbadd1d50d963238f343fa1a2e2355 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 6 Sep 2024 00:25:00 +0000 Subject: [PATCH 2/8] Rename `PackageTemplate::timelock` `counteraprty_spendable_height` This function was very confusing - its used to determine by when we have to stop aggregating this claim with others as it starts to be at risk of pinning due to the counterparty's ability to spend the output. It is not ever used as a timelock for a transaction, and thus its name is very confusing. Instead we rename it `counterparty_spendable_height`. --- lightning/src/chain/onchaintx.rs | 5 ++--- lightning/src/chain/package.rs | 6 +++++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index e1f3ebf049b..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); diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 0ff7fd186cd..52e1c23945d 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -755,7 +755,11 @@ impl PackageTemplate { pub(crate) fn is_malleable(&self) -> bool { self.malleability == PackageMalleability::Malleable } - pub(crate) fn timelock(&self) -> u32 { + /// 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.soonest_conf_deadline } pub(crate) fn aggregable(&self) -> bool { From 8c4794df87543ccc0bb1e2f2e946d26bdc7d42a0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 5 Sep 2024 23:48:02 +0000 Subject: [PATCH 3/8] Drop unused `PackageTemplate::height_original` This has never been used, and its set to a fixed value of zero for HTLCs on local commitment transactions making it impossible to rely on so might as well remove it. --- lightning/src/chain/package.rs | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 52e1c23945d..1b570d18e41 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -746,9 +746,6 @@ pub struct PackageTemplate { // Cache of next height at which fee-bumping and rebroadcast will be attempted. In // the future, we might abstract it to an observed mempool fluctuation. height_timer: u32, - // Confirmation height of the claimed outputs set transaction. In case of reorg reaching - // it, we wipe out and forget the package. - height_original: u32, } impl PackageTemplate { @@ -791,7 +788,6 @@ impl PackageTemplate { 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 { @@ -801,7 +797,6 @@ impl PackageTemplate { aggregable, feerate_previous, height_timer, - height_original, }); return false; } @@ -820,7 +815,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"); } @@ -1035,7 +1029,7 @@ 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, soonest_conf_deadline: u32, first_bump: u32) -> Self { let (malleability, aggregable) = PackageSolvingData::map_output_type_flags(&input_solving_data); let inputs = vec![(BitcoinOutPoint { txid, vout }, input_solving_data)]; PackageTemplate { @@ -1044,8 +1038,7 @@ impl PackageTemplate { soonest_conf_deadline, aggregable, feerate_previous: 0, - height_timer: height_original, - height_original, + height_timer: first_bump, } } } @@ -1060,7 +1053,8 @@ impl Writeable for PackageTemplate { write_tlv_fields!(writer, { (0, self.soonest_conf_deadline, 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(()) @@ -1082,24 +1076,20 @@ impl Readable for PackageTemplate { let mut soonest_conf_deadline = 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), (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, aggregable, feerate_previous, - height_timer: height_timer.unwrap(), - height_original, + height_timer: height_timer.unwrap_or(0), }) } } @@ -1376,7 +1366,6 @@ mod tests { 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); } From 303a0c9a4e59afb0583bb3835da992b484c843a6 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 6 Sep 2024 00:33:45 +0000 Subject: [PATCH 4/8] Stop passing current height to `PackageTemplate::build_package` Now that we don't store the confirmation height of the inputs being spent, passing the current height to `PackageTemplate::build_package` is useless - we only use it to set the height at which we should next bump the fee, but we just want it to be "next block", so we might as well use `0` and avoid the extra argument. Further, in one case we were already passing `0`, so passing the argument is just confusing as we can't rely on it being set. Note that this does remove an assertion that we never merge packages that were crated at different heights, and in the future we may wish to do that (as there's no specific reason not to), but we do not currently change the behavior. --- lightning/src/chain/channelmonitor.rs | 25 +++++++---- lightning/src/chain/package.rs | 60 +++++++++++---------------- 2 files changed, 41 insertions(+), 44 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 86f0d3de5ed..a53b94ad2c9 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() { @@ -3657,7 +3666,7 @@ impl ChannelMonitorImpl { // Returns (1) `PackageTemplate`s that can be given to the OnchainTxHandler, so that the handler can // broadcast transactions claiming holder HTLC commitment outputs and (2) a holder revokable // script so we can detect whether a holder transaction has been seen on-chain. - fn get_broadcasted_holder_claims(&self, holder_tx: &HolderSignedTx, conf_height: u32) -> (Vec, Option<(ScriptBuf, PublicKey, RevocationKey)>) { + fn get_broadcasted_holder_claims(&self, holder_tx: &HolderSignedTx, _conf_height: u32) -> (Vec, Option<(ScriptBuf, PublicKey, RevocationKey)>) { let mut claim_requests = Vec::with_capacity(holder_tx.htlc_outputs.len()); let redeemscript = chan_utils::get_revokeable_redeemscript(&holder_tx.revocation_key, self.on_holder_tx_csv, &holder_tx.delayed_payment_key); @@ -3685,7 +3694,7 @@ impl ChannelMonitorImpl { let htlc_package = PackageTemplate::build_package( holder_tx.txid, transaction_output_index, PackageSolvingData::HolderHTLCOutput(htlc_output), - htlc.cltv_expiry, conf_height + htlc.cltv_expiry, ); claim_requests.push(htlc_package); } diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 1b570d18e41..47fa48b6373 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -1029,7 +1029,7 @@ impl PackageTemplate { }).is_some() } - pub (crate) fn build_package(txid: Txid, vout: u32, input_solving_data: PackageSolvingData, soonest_conf_deadline: u32, first_bump: u32) -> Self { + pub (crate) fn build_package(txid: Txid, vout: u32, input_solving_data: PackageSolvingData, soonest_conf_deadline: u32) -> Self { let (malleability, aggregable) = PackageSolvingData::map_output_type_flags(&input_solving_data); let inputs = vec![(BitcoinOutPoint { txid, vout }, input_solving_data)]; PackageTemplate { @@ -1038,7 +1038,7 @@ impl PackageTemplate { soonest_conf_deadline, aggregable, feerate_previous: 0, - height_timer: first_bump, + height_timer: 0, } } } @@ -1253,18 +1253,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() { @@ -1273,8 +1261,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); } @@ -1286,8 +1274,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); } @@ -1299,8 +1287,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); } @@ -1312,8 +1300,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); } @@ -1324,9 +1312,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); } @@ -1338,8 +1326,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); } @@ -1351,9 +1339,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); @@ -1375,7 +1363,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()); } @@ -1386,8 +1374,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); } @@ -1398,7 +1386,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); } @@ -1412,14 +1400,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)); } } @@ -1427,7 +1415,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)); } } From be915872c5d2d03fa45052c8567253d68303d222 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 18 Sep 2024 16:00:20 +0000 Subject: [PATCH 5/8] 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 +- lightning/src/ln/monitor_tests.rs | 27 +++------ lightning/src/ln/reorg_tests.rs | 6 +- 4 files changed, 94 insertions(+), 34 deletions(-) diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 47fa48b6373..db652820524 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 31346c6b78b..9fe3def09c5 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 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); From d557334e3106915efea84359fcd4238df1378706 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 18 Sep 2024 16:48:24 +0000 Subject: [PATCH 6/8] Rename `soonest_conf_deadline` to `counterparty_spendable_height` This renames the field in `PackageTemplate` which describes the height at which a counterparty can make a claim to an output to match its actual use. Previously it had been set based on when a counterparty can claim an output but also used for other purposes. In the previous commit we cleaned up its use for fee-bumping-rate, so here we can rename it as it is now only used as the `counteraprty_spendable_height`. --- lightning/src/chain/package.rs | 43 ++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index db652820524..aefd1c5deb7 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -731,10 +731,14 @@ pub struct PackageTemplate { // Untractable packages have been counter-signed and thus imply that we can't aggregate // them without breaking signatures. Fee-bumping strategy will also rely on CPFP. malleability: PackageMalleability, - // Block height after which the earlier-output belonging to this package is mature for a - // competing claim by the counterparty. As our chain tip becomes nearer from the timelock, - // the fee-bumping frequency will increase. See `OnchainTxHandler::get_height_timer`. - soonest_conf_deadline: u32, + /// Block height at which our counterparty can potentially claim this output as well (assuming + /// they have the keys or information required to do so). + /// + /// This is used primarily by external consumers to decide when an output becomes "pinnable" + /// because the counterparty can potentially spend it. It is also used internally by + /// [`Self::get_height_timer`] to identify when an output must be claimed by, depending on the + /// type of output. + counterparty_spendable_height: u32, // Determines if this package can be aggregated. // Timelocked outputs belonging to the same transaction might have differing // satisfying heights. Picking up the later height among the output set would be a valid @@ -763,7 +767,7 @@ impl PackageTemplate { /// 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.soonest_conf_deadline + self.counterparty_spendable_height } pub(crate) fn aggregable(&self) -> bool { self.aggregable @@ -790,7 +794,6 @@ 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; @@ -799,7 +802,7 @@ impl PackageTemplate { 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, @@ -836,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; @@ -971,10 +974,10 @@ impl PackageTemplate { 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`. + // where the CSV expires, which is also our `counterparty_spendable_height`. height_timer = cmp::min( height_timer, - timer_for_target_conf(self.soonest_conf_deadline), + timer_for_target_conf(self.counterparty_spendable_height), ); }, PackageSolvingData::RevokedHTLCOutput(_) => { @@ -995,10 +998,10 @@ impl PackageTemplate { 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. + // `counterparty_spendable_height` holds the real HTLC expiry. height_timer = cmp::min( height_timer, - timer_for_target_conf(self.soonest_conf_deadline), + timer_for_target_conf(self.counterparty_spendable_height), ); }, PackageSolvingData::CounterpartyReceivedHTLCOutput(outp) => { @@ -1100,13 +1103,13 @@ impl PackageTemplate { }).is_some() } - pub (crate) fn build_package(txid: Txid, vout: u32, input_solving_data: PackageSolvingData, soonest_conf_deadline: 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: 0, @@ -1122,7 +1125,7 @@ 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), // Prior to 0.1, the height at which the package's inputs were mined, but was always unused (4, 0u32, required), @@ -1144,12 +1147,12 @@ 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: Option = None; read_tlv_fields!(reader, { - (0, soonest_conf_deadline, required), + (0, counterparty_spendable_height, required), (2, feerate_previous, required), (4, _height_original, option), // Written with a dummy value since 0.1 (6, height_timer, option), @@ -1157,7 +1160,7 @@ impl Readable for PackageTemplate { Ok(PackageTemplate { inputs, malleability, - soonest_conf_deadline, + counterparty_spendable_height, aggregable, feerate_previous, height_timer: height_timer.unwrap_or(0), @@ -1421,7 +1424,7 @@ 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); From 6ae33dc1e09e3305d2e3a12d81b398a2a9c23d23 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 18 Sep 2024 18:20:46 +0000 Subject: [PATCH 7/8] Set correct `counterparty_spendable_height` for outb local HTLCs For outbound HTLCs, the counterparty can spend the output immediately. This fixes the `counterparty_spendable_height` in the `PackageTemplate` claiming outbound HTLCs on local commitment transactions, which was previously spuriously set to the HTLC timeout (at which point *we* can claim the HTLC). --- lightning/src/chain/channelmonitor.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index a53b94ad2c9..21a1f8be2be 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3666,7 +3666,7 @@ impl ChannelMonitorImpl { // Returns (1) `PackageTemplate`s that can be given to the OnchainTxHandler, so that the handler can // broadcast transactions claiming holder HTLC commitment outputs and (2) a holder revokable // script so we can detect whether a holder transaction has been seen on-chain. - fn get_broadcasted_holder_claims(&self, holder_tx: &HolderSignedTx, _conf_height: u32) -> (Vec, Option<(ScriptBuf, PublicKey, RevocationKey)>) { + fn get_broadcasted_holder_claims(&self, holder_tx: &HolderSignedTx, conf_height: u32) -> (Vec, Option<(ScriptBuf, PublicKey, RevocationKey)>) { let mut claim_requests = Vec::with_capacity(holder_tx.htlc_outputs.len()); let redeemscript = chan_utils::get_revokeable_redeemscript(&holder_tx.revocation_key, self.on_holder_tx_csv, &holder_tx.delayed_payment_key); @@ -3674,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() @@ -3689,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, + counterparty_spendable_height, ); claim_requests.push(htlc_package); } From 20db790c53bd57d0fc4e07c92724d11fc7fb35a2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 18 Oct 2024 15:57:25 +0000 Subject: [PATCH 8/8] Add a test for the fee-bump rate of timeout HTLC claims on cp txn In a previous commit we updated the fee-bump-rate of claims against HTLC timeouts on counterparty commitment transactions so that instead of immediately attempting to bump every block we consider the fact that we actually have at least `MIN_CLTV_EXPIRY_DELTA` blocks to do so, and bumping at the appropriate rate given that. Here we test that by adding an extra check to an existing test that we do not bump in the very next block after the HTLC timeout claim was initially broadcasted. --- lightning/src/ln/functional_tests.rs | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 9fe3def09c5..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}; @@ -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);