From 303a0c9a4e59afb0583bb3835da992b484c843a6 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 6 Sep 2024 00:33:45 +0000 Subject: [PATCH] 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 86f0d3de5..a53b94ad2 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 1b570d18e..47fa48b63 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)); } } -- 2.39.5