diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 4f279cf6548..7e29565de1c 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -216,9 +216,16 @@ impl_writeable_tlv_based!(HTLCUpdate, { (4, payment_preimage, option), }); -/// If an HTLC expires within this many blocks, don't try to claim it in a shared transaction, -/// instead claiming it in its own individual transaction. -pub(crate) const CLTV_SHARED_CLAIM_BUFFER: u32 = 12; +/// If an output goes from claimable only by us to claimable by us or our counterparty within this +/// many blocks, we consider it pinnable for the purposes of aggregating claims in a single +/// transaction. +pub(crate) const COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE: u32 = 12; + +/// When we go to force-close a channel because an HTLC is expiring, we should ensure that the +/// HTLC(s) expiring are not considered pinnable, allowing us to aggregate them with other HTLC(s) +/// expiring at the same time. +const _: () = assert!(CLTV_CLAIM_BUFFER > COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE); + /// If an HTLC expires within this many blocks, force-close the channel to broadcast the /// HTLC-Success transaction. /// In other words, this is an upper bound on how many blocks we think it can take us to get a diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index f19198330d2..96e5c832983 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -31,7 +31,7 @@ use crate::types::payment::PaymentPreimage; use crate::ln::chan_utils::{self, ChannelTransactionParameters, HTLCOutputInCommitment, HolderCommitmentTransaction}; use crate::chain::ClaimId; use crate::chain::chaininterface::{FeeEstimator, BroadcasterInterface, LowerBoundedFeeEstimator}; -use crate::chain::channelmonitor::{ANTI_REORG_DELAY, CLTV_SHARED_CLAIM_BUFFER}; +use crate::chain::channelmonitor::ANTI_REORG_DELAY; use crate::chain::package::{PackageSolvingData, PackageTemplate}; use crate::chain::transaction::MaybeSignedTransaction; use crate::util::logger::Logger; @@ -726,7 +726,7 @@ impl OnchainTxHandler { /// does not need to equal the current blockchain tip height, which should be provided via /// `cur_height`, however it must never be higher than `cur_height`. pub(super) fn update_claims_view_from_requests( - &mut self, requests: Vec, conf_height: u32, cur_height: u32, + &mut self, mut requests: Vec, conf_height: u32, cur_height: u32, broadcaster: &B, conf_target: ConfirmationTarget, fee_estimator: &LowerBoundedFeeEstimator, logger: &L ) where @@ -737,49 +737,67 @@ impl OnchainTxHandler { log_debug!(logger, "Updating claims view at height {} with {} claim requests", cur_height, requests.len()); } - let mut preprocessed_requests = Vec::with_capacity(requests.len()); - let mut aggregated_request = None; - - // Try to aggregate outputs if their timelock expiration isn't imminent (package timelock - // <= CLTV_SHARED_CLAIM_BUFFER) and they don't require an immediate nLockTime (aggregable). - for req in requests { - // Don't claim a outpoint twice that would be bad for privacy and may uselessly lock a CPFP input for a while - if let Some(_) = self.claimable_outpoints.get(req.outpoints()[0]) { - log_info!(logger, "Ignoring second claim for outpoint {}:{}, already registered its claiming request", req.outpoints()[0].txid, req.outpoints()[0].vout); + // First drop any duplicate claims. + requests.retain(|req| { + debug_assert_eq!( + req.outpoints().len(), + 1, + "Claims passed to `update_claims_view_from_requests` should not be aggregated" + ); + let mut all_outpoints_claiming = true; + for outpoint in req.outpoints() { + if self.claimable_outpoints.get(outpoint).is_none() { + all_outpoints_claiming = false; + } + } + if all_outpoints_claiming { + log_info!(logger, "Ignoring second claim for outpoint {}:{}, already registered its claiming request", + req.outpoints()[0].txid, req.outpoints()[0].vout); + false } else { let timelocked_equivalent_package = self.locktimed_packages.iter().map(|v| v.1.iter()).flatten() .find(|locked_package| locked_package.outpoints() == req.outpoints()); if let Some(package) = timelocked_equivalent_package { log_info!(logger, "Ignoring second claim for outpoint {}:{}, we already have one which we're waiting on a timelock at {} for.", req.outpoints()[0].txid, req.outpoints()[0].vout, package.package_locktime(cur_height)); - continue; + false + } else { + true } + } + }); - let package_locktime = req.package_locktime(cur_height); - if package_locktime > cur_height + 1 { - log_info!(logger, "Delaying claim of package until its timelock at {} (current height {}), the following outpoints are spent:", package_locktime, cur_height); - for outpoint in req.outpoints() { - log_info!(logger, " Outpoint {}", outpoint); + // Then try to maximally aggregate `requests`. + for i in (1..requests.len()).rev() { + for j in 0..i { + if requests[i].can_merge_with(&requests[j], cur_height) { + let merge = requests.remove(i); + if let Err(rejected) = requests[j].merge_package(merge, cur_height) { + debug_assert!(false, "Merging package should not be rejected after verifying can_merge_with."); + requests.insert(i, rejected); + } else { + break; } - self.locktimed_packages.entry(package_locktime).or_default().push(req); - continue; } + } + } - 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); - } else { - aggregated_request.as_mut().unwrap().merge_package(req); + // Finally, split requests into timelocked ones and immediately-spendable ones. + let mut preprocessed_requests = Vec::with_capacity(requests.len()); + for req in requests { + let package_locktime = req.package_locktime(cur_height); + if package_locktime > cur_height + 1 { + log_info!(logger, "Delaying claim of package until its timelock at {} (current height {}), the following outpoints are spent:", package_locktime, cur_height); + for outpoint in req.outpoints() { + log_info!(logger, " Outpoint {}", outpoint); } + self.locktimed_packages.entry(package_locktime).or_default().push(req); + } else { + preprocessed_requests.push(req); } } - if let Some(req) = aggregated_request { - preprocessed_requests.push(req); - } - // Claim everything up to and including `cur_height` + // Claim everything up to and including `cur_height`. let remaining_locked_packages = self.locktimed_packages.split_off(&(cur_height + 1)); if !self.locktimed_packages.is_empty() { log_debug!(logger, @@ -1088,7 +1106,7 @@ impl OnchainTxHandler { OnchainEvent::ContentiousOutpoint { package } => { if let Some(pending_claim) = self.claimable_outpoints.get(package.outpoints()[0]) { if let Some(request) = self.pending_claim_requests.get_mut(&pending_claim.0) { - request.merge_package(package); + assert!(request.merge_package(package, height).is_ok()); // Using a HashMap guarantee us than if we have multiple outpoints getting // resurrected only one bump claim tx is going to be broadcast bump_candidates.insert(pending_claim.clone(), request.clone()); diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index f5745e93dc3..53bba3a754b 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -30,6 +30,7 @@ use crate::types::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::channelmonitor::COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE; 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; use crate::sign::ecdsa::EcdsaChannelSigner; @@ -39,7 +40,6 @@ use crate::util::ser::{Readable, Writer, Writeable, RequiredWrapper}; use crate::io; use core::cmp; -use core::mem; use core::ops::Deref; #[allow(unused_imports)] @@ -543,25 +543,38 @@ impl PackageSolvingData { PackageSolvingData::HolderFundingOutput(..) => unreachable!(), } } - fn is_compatible(&self, input: &PackageSolvingData) -> bool { + + /// Checks if this and `other` are spending types of inputs which could have descended from the + /// same commitment transaction(s) and thus could both be spent without requiring a + /// double-spend. + fn is_possibly_from_same_tx_tree(&self, other: &PackageSolvingData) -> bool { match self { - PackageSolvingData::RevokedOutput(..) => { - match input { - PackageSolvingData::RevokedHTLCOutput(..) => { true }, - PackageSolvingData::RevokedOutput(..) => { true }, - _ => { false } + PackageSolvingData::RevokedOutput(_)|PackageSolvingData::RevokedHTLCOutput(_) => { + match other { + PackageSolvingData::RevokedOutput(_)| + PackageSolvingData::RevokedHTLCOutput(_) => true, + _ => false, + } + }, + PackageSolvingData::CounterpartyOfferedHTLCOutput(_)| + PackageSolvingData::CounterpartyReceivedHTLCOutput(_) => { + match other { + PackageSolvingData::CounterpartyOfferedHTLCOutput(_)| + PackageSolvingData::CounterpartyReceivedHTLCOutput(_) => true, + _ => false, } }, - PackageSolvingData::RevokedHTLCOutput(..) => { - match input { - PackageSolvingData::RevokedOutput(..) => { true }, - PackageSolvingData::RevokedHTLCOutput(..) => { true }, - _ => { false } + PackageSolvingData::HolderHTLCOutput(_)| + PackageSolvingData::HolderFundingOutput(_) => { + match other { + PackageSolvingData::HolderHTLCOutput(_)| + PackageSolvingData::HolderFundingOutput(_) => true, + _ => false, } }, - _ => { mem::discriminant(self) == mem::discriminant(&input) } } } + fn as_tx_input(&self, previous_output: BitcoinOutPoint) -> TxIn { let sequence = match self { PackageSolvingData::RevokedOutput(_) => Sequence::ENABLE_RBF_NO_LOCKTIME, @@ -655,43 +668,53 @@ impl PackageSolvingData { _ => { panic!("API Error!"); } } } - fn absolute_tx_timelock(&self, current_height: u32) -> u32 { - // We use `current_height` as our default locktime to discourage fee sniping and because - // transactions with it always propagate. - let absolute_timelock = match self { - PackageSolvingData::RevokedOutput(_) => current_height, - PackageSolvingData::RevokedHTLCOutput(_) => current_height, - PackageSolvingData::CounterpartyOfferedHTLCOutput(_) => current_height, - PackageSolvingData::CounterpartyReceivedHTLCOutput(ref outp) => cmp::max(outp.htlc.cltv_expiry, current_height), - // HTLC timeout/success transactions rely on a fixed timelock due to the counterparty's - // signature. + /// Some output types are locked with CHECKLOCKTIMEVERIFY and the spending transaction must + /// have a minimum locktime, which is returned here. + fn minimum_locktime(&self) -> Option { + match self { + PackageSolvingData::CounterpartyReceivedHTLCOutput(ref outp) => Some(outp.htlc.cltv_expiry), + _ => None, + } + } + /// Some output types are pre-signed in such a way that the spending transaction must have an + /// exact locktime. This returns that locktime for such outputs. + fn signed_locktime(&self) -> Option { + match self { PackageSolvingData::HolderHTLCOutput(ref outp) => { if outp.preimage.is_some() { debug_assert_eq!(outp.cltv_expiry, 0); } - outp.cltv_expiry + Some(outp.cltv_expiry) }, - PackageSolvingData::HolderFundingOutput(_) => current_height, - }; - absolute_timelock - } - - fn map_output_type_flags(&self) -> (PackageMalleability, bool) { - // Post-anchor, aggregation of outputs of different types is unsafe. See https://github.com/lightning/bolts/pull/803. - let (malleability, aggregable) = match self { - PackageSolvingData::RevokedOutput(RevokedOutput { is_counterparty_balance_on_anchors: Some(()), .. }) => { (PackageMalleability::Malleable, false) }, - PackageSolvingData::RevokedOutput(RevokedOutput { is_counterparty_balance_on_anchors: None, .. }) => { (PackageMalleability::Malleable, true) }, - PackageSolvingData::RevokedHTLCOutput(..) => { (PackageMalleability::Malleable, true) }, - PackageSolvingData::CounterpartyOfferedHTLCOutput(..) => { (PackageMalleability::Malleable, true) }, - PackageSolvingData::CounterpartyReceivedHTLCOutput(..) => { (PackageMalleability::Malleable, false) }, - PackageSolvingData::HolderHTLCOutput(ref outp) => if outp.channel_type_features.supports_anchors_zero_fee_htlc_tx() { - (PackageMalleability::Malleable, outp.preimage.is_some()) - } else { - (PackageMalleability::Untractable, false) + _ => None, + } + } + + fn map_output_type_flags(&self) -> PackageMalleability { + // We classify claims into not-mergeable (i.e. transactions that have to be broadcasted + // as-is) or merge-able (i.e. transactions we can merge with others and claim in batches), + // which we then sub-categorize into pinnable (where our counterparty could potentially + // also claim the transaction right now) or unpinnable (where only we can claim this + // output). We assume we are claiming in a timely manner. + match self { + PackageSolvingData::RevokedOutput(RevokedOutput { .. }) => + PackageMalleability::Malleable(AggregationCluster::Unpinnable), + PackageSolvingData::RevokedHTLCOutput(..) => + PackageMalleability::Malleable(AggregationCluster::Pinnable), + PackageSolvingData::CounterpartyOfferedHTLCOutput(..) => + PackageMalleability::Malleable(AggregationCluster::Unpinnable), + PackageSolvingData::CounterpartyReceivedHTLCOutput(..) => + PackageMalleability::Malleable(AggregationCluster::Pinnable), + PackageSolvingData::HolderHTLCOutput(ref outp) if outp.channel_type_features.supports_anchors_zero_fee_htlc_tx() => { + if outp.preimage.is_some() { + PackageMalleability::Malleable(AggregationCluster::Unpinnable) + } else { + PackageMalleability::Malleable(AggregationCluster::Pinnable) + } }, - PackageSolvingData::HolderFundingOutput(..) => { (PackageMalleability::Untractable, false) }, - }; - (malleability, aggregable) + PackageSolvingData::HolderHTLCOutput(..) => PackageMalleability::Untractable, + PackageSolvingData::HolderFundingOutput(..) => PackageMalleability::Untractable, + } } } @@ -704,11 +727,25 @@ impl_writeable_tlv_based_enum_legacy!(PackageSolvingData, ; (5, HolderFundingOutput), ); +/// We aggregate claims into clusters based on if we think the output is potentially pinnable by +/// our counterparty and whether the CLTVs required make sense to aggregate into one claim. +/// That way we avoid claiming in too many discrete transactions while also avoiding +/// unnecessarily exposing ourselves to pinning attacks or delaying claims when we could have +/// claimed at least part of the available outputs quickly and without risk. +#[derive(Copy, Clone, PartialEq, Eq)] +enum AggregationCluster { + /// Our counterparty can potentially claim this output. + Pinnable, + /// We are the only party that can claim these funds, thus we believe they are not pinnable + /// until they reach a CLTV/CSV expiry where our counterparty could also claim them. + Unpinnable, +} + /// A malleable package might be aggregated with other packages to save on fees. /// A untractable package has been counter-signed and aggregable will break cached counterparty signatures. -#[derive(Clone, PartialEq, Eq)] -pub(crate) enum PackageMalleability { - Malleable, +#[derive(Copy, Clone, PartialEq, Eq)] +enum PackageMalleability { + Malleable(AggregationCluster), Untractable, } @@ -739,16 +776,6 @@ pub struct PackageTemplate { /// [`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 - // aggregable strategy but it comes with at least 2 trade-offs : - // * earlier-output fund are going to take longer to come back - // * CLTV delta backing up a corresponding HTLC on an upstream channel could be swallowed - // by the requirement of the later-output part of the set - // For now, we mark such timelocked outputs as non-aggregable, though we might introduce - // smarter aggregable strategy in the future. - aggregable: bool, // Cache of package feerate committed at previous (re)broadcast. If bumping resources // (either claimed output value or external utxo), it will keep increasing until holder // or counterparty successful claim. @@ -759,8 +786,74 @@ pub struct PackageTemplate { } impl PackageTemplate { + pub(crate) fn can_merge_with(&self, other: &PackageTemplate, cur_height: u32) -> bool { + match (self.malleability, other.malleability) { + (PackageMalleability::Untractable, _) => false, + (_, PackageMalleability::Untractable) => false, + (PackageMalleability::Malleable(self_cluster), PackageMalleability::Malleable(other_cluster)) => { + if self.inputs.is_empty() { + return false; + } + if other.inputs.is_empty() { + return false; + } + + // First check the types of the inputs and don't merge if they are possibly claiming + // from different commitment transactions at the same time. + // This shouldn't ever happen, but if we do end up with packages trying to claim + // funds from two different commitment transactions (which cannot possibly be + // on-chain at the same time), we definitely shouldn't merge them. + #[cfg(debug_assertions)] + { + for i in 0..self.inputs.len() { + for j in 0..i { + debug_assert!(self.inputs[i].1.is_possibly_from_same_tx_tree(&self.inputs[j].1)); + } + } + for i in 0..other.inputs.len() { + for j in 0..i { + assert!(other.inputs[i].1.is_possibly_from_same_tx_tree(&other.inputs[j].1)); + } + } + } + if !self.inputs[0].1.is_possibly_from_same_tx_tree(&other.inputs[0].1) { + debug_assert!(false, "We shouldn't have packages from different tx trees"); + return false; + } + + // Check if the packages have signed locktimes. If they do, we only want to aggregate + // packages with the same, signed locktime. + if self.signed_locktime() != other.signed_locktime() { + return false; + } + // Check if the two packages have compatible minimum locktimes. + if self.package_locktime(cur_height) != other.package_locktime(cur_height) { + return false; + } + + // Now check that we only merge packages if they are both unpinnable or both + // pinnable. + let self_pinnable = self_cluster == AggregationCluster::Pinnable || + self.counterparty_spendable_height() <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE; + let other_pinnable = other_cluster == AggregationCluster::Pinnable || + other.counterparty_spendable_height() <= cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE; + if self_pinnable && other_pinnable { + return true; + } + + let self_unpinnable = self_cluster == AggregationCluster::Unpinnable && + self.counterparty_spendable_height() > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE; + let other_unpinnable = other_cluster == AggregationCluster::Unpinnable && + other.counterparty_spendable_height() > cur_height + COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE; + if self_unpinnable && other_unpinnable { + return true; + } + false + }, + } + } pub(crate) fn is_malleable(&self) -> bool { - self.malleability == PackageMalleability::Malleable + matches!(self.malleability, PackageMalleability::Malleable(..)) } /// The height at which our counterparty may be able to spend this output. /// @@ -769,9 +862,6 @@ impl PackageTemplate { pub(crate) fn counterparty_spendable_height(&self) -> u32 { self.counterparty_spendable_height } - pub(crate) fn aggregable(&self) -> bool { - self.aggregable - } pub(crate) fn previous_feerate(&self) -> u64 { self.feerate_previous } @@ -792,18 +882,16 @@ impl PackageTemplate { } pub(crate) fn split_package(&mut self, split_outp: &BitcoinOutPoint) -> Option { match self.malleability { - PackageMalleability::Malleable => { + PackageMalleability::Malleable(cluster) => { let mut split_package = None; - let aggregable = self.aggregable; let feerate_previous = self.feerate_previous; let height_timer = self.height_timer; self.inputs.retain(|outp| { if *split_outp == outp.0 { split_package = Some(PackageTemplate { inputs: vec![(outp.0, outp.1.clone())], - malleability: PackageMalleability::Malleable, + malleability: PackageMalleability::Malleable(cluster), counterparty_spendable_height: self.counterparty_spendable_height, - aggregable, feerate_previous, height_timer, }); @@ -823,18 +911,10 @@ impl PackageTemplate { } } } - pub(crate) fn merge_package(&mut self, mut merge_from: PackageTemplate) { - if self.malleability == PackageMalleability::Untractable || merge_from.malleability == PackageMalleability::Untractable { - panic!("Merging template on untractable packages"); - } - if !self.aggregable || !merge_from.aggregable { - panic!("Merging non aggregatable packages"); + pub(crate) fn merge_package(&mut self, mut merge_from: PackageTemplate, cur_height: u32) -> Result<(), PackageTemplate> { + if !self.can_merge_with(&merge_from, cur_height) { + return Err(merge_from); } - if let Some((_, lead_input)) = self.inputs.first() { - for (_, v) in merge_from.inputs.iter() { - if !lead_input.is_compatible(v) { panic!("Merging outputs from differing types !"); } - } - } else { panic!("Merging template on an empty package"); } for (k, v) in merge_from.inputs.drain(..) { self.inputs.push((k, v)); } @@ -846,6 +926,7 @@ impl PackageTemplate { self.feerate_previous = merge_from.feerate_previous; } self.height_timer = cmp::min(self.height_timer, merge_from.height_timer); + Ok(()) } /// Gets the amount of all outptus being spent by this package, only valid for malleable /// packages. @@ -856,36 +937,23 @@ impl PackageTemplate { } amounts } - pub(crate) fn package_locktime(&self, current_height: u32) -> u32 { - let locktime = self.inputs.iter().map(|(_, outp)| outp.absolute_tx_timelock(current_height)) - .max().expect("There must always be at least one output to spend in a PackageTemplate"); - - // If we ever try to aggregate a `HolderHTLCOutput`s with another output type, we'll likely - // end up with an incorrect transaction locktime since the counterparty has included it in - // its HTLC signature. This should never happen unless we decide to aggregate outputs across - // different channel commitments. - #[cfg(debug_assertions)] { - if self.inputs.iter().any(|(_, outp)| - if let PackageSolvingData::HolderHTLCOutput(outp) = outp { - outp.preimage.is_some() - } else { - false - } - ) { - debug_assert_eq!(locktime, 0); - }; - for timeout_htlc_expiry in self.inputs.iter().filter_map(|(_, outp)| - if let PackageSolvingData::HolderHTLCOutput(outp) = outp { - if outp.preimage.is_none() { - Some(outp.cltv_expiry) - } else { None } - } else { None } - ) { - debug_assert_eq!(locktime, timeout_htlc_expiry); - } + fn signed_locktime(&self) -> Option { + let signed_locktime = self.inputs.iter().find_map(|(_, outp)| outp.signed_locktime()); + #[cfg(debug_assertions)] + for (_, outp) in &self.inputs { + debug_assert!(outp.signed_locktime().is_none() || outp.signed_locktime() == signed_locktime); } + signed_locktime + } + pub(crate) fn package_locktime(&self, current_height: u32) -> u32 { + let minimum_locktime = self.inputs.iter().filter_map(|(_, outp)| outp.minimum_locktime()).max(); - locktime + if let Some(signed_locktime) = self.signed_locktime() { + debug_assert!(minimum_locktime.is_none()); + signed_locktime + } else { + core::cmp::max(current_height, minimum_locktime.unwrap_or(0)) + } } pub(crate) fn package_weight(&self, destination_script: &Script) -> u64 { let mut inputs_weight = 0; @@ -1042,7 +1110,8 @@ impl PackageTemplate { ) -> Option<(u64, u64)> where F::Target: FeeEstimator, { - debug_assert!(self.malleability == PackageMalleability::Malleable, "The package output is fixed for non-malleable packages"); + debug_assert!(matches!(self.malleability, PackageMalleability::Malleable(..)), + "The package output is fixed for non-malleable packages"); let input_amounts = self.package_amount(); assert!(dust_limit_sats as i64 > 0, "Output script must be broadcastable/have a 'real' dust limit."); // If old feerate is 0, first iteration of this claim, use normal fee calculation @@ -1104,13 +1173,12 @@ impl PackageTemplate { } 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 malleability = PackageSolvingData::map_output_type_flags(&input_solving_data); let inputs = vec![(BitcoinOutPoint { txid, vout }, input_solving_data)]; PackageTemplate { inputs, malleability, counterparty_spendable_height, - aggregable, feerate_previous: 0, height_timer: 0, } @@ -1144,7 +1212,7 @@ impl Readable for PackageTemplate { let rev_outp = Readable::read(reader)?; inputs.push((outpoint, rev_outp)); } - let (malleability, aggregable) = if let Some((_, lead_input)) = inputs.first() { + let malleability = if let Some((_, lead_input)) = inputs.first() { PackageSolvingData::map_output_type_flags(&lead_input) } else { return Err(DecodeError::InvalidValue); }; let mut counterparty_spendable_height = 0; @@ -1161,7 +1229,6 @@ impl Readable for PackageTemplate { inputs, malleability, counterparty_spendable_height, - aggregable, feerate_previous, height_timer: height_timer.unwrap_or(0), }) @@ -1210,7 +1277,7 @@ where F::Target: FeeEstimator, { // If old feerate inferior to actual one given back by Fee Estimator, use it to compute new fee... - let (new_fee, new_feerate) = if let Some((new_fee, new_feerate)) = + let (new_fee, new_feerate) = if let Some((new_fee, new_feerate)) = compute_fee_from_spent_amounts(input_amounts, predicted_weight, conf_target, fee_estimator, logger) { match feerate_strategy { @@ -1264,16 +1331,19 @@ where #[cfg(test)] mod tests { - use crate::chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderHTLCOutput, PackageTemplate, PackageSolvingData, RevokedOutput, WEIGHT_REVOKED_OUTPUT, weight_offered_htlc, weight_received_htlc}; + use crate::chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput, HolderHTLCOutput, PackageTemplate, PackageSolvingData, RevokedHTLCOutput, RevokedOutput, WEIGHT_REVOKED_OUTPUT, weight_offered_htlc, weight_received_htlc}; use crate::chain::Txid; use crate::ln::chan_utils::HTLCOutputInCommitment; use crate::types::payment::{PaymentPreimage, PaymentHash}; use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint}; + use bitcoin::absolute::LockTime; use bitcoin::amount::Amount; use bitcoin::constants::WITNESS_SCALE_FACTOR; use bitcoin::script::ScriptBuf; use bitcoin::transaction::OutPoint as BitcoinOutPoint; + use bitcoin::transaction::Version; + use bitcoin::{Transaction, TxOut}; use bitcoin::hex::FromHex; @@ -1281,151 +1351,254 @@ mod tests { use bitcoin::secp256k1::Secp256k1; use crate::types::features::ChannelTypeFeatures; - use std::str::FromStr; + fn fake_txid(n: u64) -> Txid { + Transaction { + version: Version(0), + lock_time: LockTime::ZERO, + input: vec![], + output: vec![TxOut { + value: Amount::from_sat(n), + script_pubkey: ScriptBuf::new(), + }], + }.compute_txid() + } macro_rules! dumb_revk_output { - ($secp_ctx: expr, $is_counterparty_balance_on_anchors: expr) => { + ($is_counterparty_balance_on_anchors: expr) => { { + let secp_ctx = Secp256k1::new(); let dumb_scalar = SecretKey::from_slice(&>::from_hex("0101010101010101010101010101010101010101010101010101010101010101").unwrap()[..]).unwrap(); - let dumb_point = PublicKey::from_secret_key(&$secp_ctx, &dumb_scalar); + let dumb_point = PublicKey::from_secret_key(&secp_ctx, &dumb_scalar); PackageSolvingData::RevokedOutput(RevokedOutput::build(dumb_point, DelayedPaymentBasepoint::from(dumb_point), HtlcBasepoint::from(dumb_point), dumb_scalar, Amount::ZERO, 0, $is_counterparty_balance_on_anchors)) } } } - macro_rules! dumb_counterparty_output { - ($secp_ctx: expr, $amt: expr, $opt_anchors: expr) => { + macro_rules! dumb_revk_htlc_output { + () => { { + let secp_ctx = Secp256k1::new(); let dumb_scalar = SecretKey::from_slice(&>::from_hex("0101010101010101010101010101010101010101010101010101010101010101").unwrap()[..]).unwrap(); - let dumb_point = PublicKey::from_secret_key(&$secp_ctx, &dumb_scalar); + let dumb_point = PublicKey::from_secret_key(&secp_ctx, &dumb_scalar); let hash = PaymentHash([1; 32]); - let htlc = HTLCOutputInCommitment { offered: true, amount_msat: $amt, cltv_expiry: 0, payment_hash: hash, transaction_output_index: None }; - PackageSolvingData::CounterpartyReceivedHTLCOutput(CounterpartyReceivedHTLCOutput::build(dumb_point, DelayedPaymentBasepoint::from(dumb_point), HtlcBasepoint::from(dumb_point), htlc, $opt_anchors)) + let htlc = HTLCOutputInCommitment { offered: false, amount_msat: 1_000_000, cltv_expiry: 0, payment_hash: hash, transaction_output_index: None }; + PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput::build(dumb_point, DelayedPaymentBasepoint::from(dumb_point), HtlcBasepoint::from(dumb_point), dumb_scalar, 1_000_000 / 1_000, htlc, &ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies())) + } + } + } + + macro_rules! dumb_counterparty_received_output { + ($amt: expr, $expiry: expr, $features: expr) => { + { + let secp_ctx = Secp256k1::new(); + let dumb_scalar = SecretKey::from_slice(&>::from_hex("0101010101010101010101010101010101010101010101010101010101010101").unwrap()[..]).unwrap(); + let dumb_point = PublicKey::from_secret_key(&secp_ctx, &dumb_scalar); + let hash = PaymentHash([1; 32]); + let htlc = HTLCOutputInCommitment { offered: true, amount_msat: $amt, cltv_expiry: $expiry, payment_hash: hash, transaction_output_index: None }; + PackageSolvingData::CounterpartyReceivedHTLCOutput(CounterpartyReceivedHTLCOutput::build(dumb_point, DelayedPaymentBasepoint::from(dumb_point), HtlcBasepoint::from(dumb_point), htlc, $features)) } } } macro_rules! dumb_counterparty_offered_output { - ($secp_ctx: expr, $amt: expr, $opt_anchors: expr) => { + ($amt: expr, $features: expr) => { { + let secp_ctx = Secp256k1::new(); let dumb_scalar = SecretKey::from_slice(&>::from_hex("0101010101010101010101010101010101010101010101010101010101010101").unwrap()[..]).unwrap(); - let dumb_point = PublicKey::from_secret_key(&$secp_ctx, &dumb_scalar); + let dumb_point = PublicKey::from_secret_key(&secp_ctx, &dumb_scalar); let hash = PaymentHash([1; 32]); let preimage = PaymentPreimage([2;32]); - let htlc = HTLCOutputInCommitment { offered: false, amount_msat: $amt, cltv_expiry: 1000, payment_hash: hash, transaction_output_index: None }; - PackageSolvingData::CounterpartyOfferedHTLCOutput(CounterpartyOfferedHTLCOutput::build(dumb_point, DelayedPaymentBasepoint::from(dumb_point), HtlcBasepoint::from(dumb_point), preimage, htlc, $opt_anchors)) + let htlc = HTLCOutputInCommitment { offered: false, amount_msat: $amt, cltv_expiry: 0, payment_hash: hash, transaction_output_index: None }; + PackageSolvingData::CounterpartyOfferedHTLCOutput(CounterpartyOfferedHTLCOutput::build(dumb_point, DelayedPaymentBasepoint::from(dumb_point), HtlcBasepoint::from(dumb_point), preimage, htlc, $features)) } } } - macro_rules! dumb_htlc_output { - () => { + macro_rules! dumb_accepted_htlc_output { + ($features: expr) => { { let preimage = PaymentPreimage([2;32]); - PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build_accepted(preimage, 0, ChannelTypeFeatures::only_static_remote_key())) + PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build_accepted(preimage, 0, $features)) } } } - #[test] - #[should_panic] - fn test_package_untractable_merge_to() { - let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap(); - let secp_ctx = Secp256k1::new(); - let revk_outp = dumb_revk_output!(secp_ctx, false); - let htlc_outp = dumb_htlc_output!(); + macro_rules! dumb_offered_htlc_output { + ($cltv_expiry: expr, $features: expr) => { + { + PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build_offered(0, $cltv_expiry, $features)) + } + } + } - 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); + macro_rules! dumb_funding_output { + () => { + PackageSolvingData::HolderFundingOutput(HolderFundingOutput::build(ScriptBuf::new(), 0, ChannelTypeFeatures::only_static_remote_key())) + } } #[test] - #[should_panic] - fn test_package_untractable_merge_from() { - let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap(); - let secp_ctx = Secp256k1::new(); - let htlc_outp = dumb_htlc_output!(); - let revk_outp = dumb_revk_output!(secp_ctx, false); + fn test_merge_package_untractable_funding_output() { + let funding_outp = dumb_funding_output!(); + let htlc_outp = dumb_accepted_htlc_output!(ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()); + + let mut untractable_package = PackageTemplate::build_package(fake_txid(1), 0, funding_outp.clone(), 0); + let mut malleable_package = PackageTemplate::build_package(fake_txid(2), 0, htlc_outp.clone(), 1100); - 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); + assert!(!untractable_package.can_merge_with(&malleable_package, 1000)); + assert!(untractable_package.merge_package(malleable_package.clone(), 1000).is_err()); + + assert!(!malleable_package.can_merge_with(&untractable_package, 1000)); + assert!(malleable_package.merge_package(untractable_package.clone(), 1000).is_err()); } #[test] - #[should_panic] - fn test_package_noaggregation_to() { - let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap(); - let secp_ctx = Secp256k1::new(); - let revk_outp = dumb_revk_output!(secp_ctx, false); - let revk_outp_counterparty_balance = dumb_revk_output!(secp_ctx, true); + fn test_merge_empty_package() { + let revk_outp = dumb_revk_htlc_output!(); - 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); + let mut empty_package = PackageTemplate::build_package(fake_txid(1), 0, revk_outp.clone(), 0); + empty_package.inputs = vec![]; + let mut package = PackageTemplate::build_package(fake_txid(1), 1, revk_outp.clone(), 1100); + assert!(empty_package.merge_package(package.clone(), 1000).is_err()); + assert!(package.merge_package(empty_package.clone(), 1000).is_err()); } #[test] - #[should_panic] - fn test_package_noaggregation_from() { - let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap(); - let secp_ctx = Secp256k1::new(); - let revk_outp = dumb_revk_output!(secp_ctx, false); - let revk_outp_counterparty_balance = dumb_revk_output!(secp_ctx, true); + fn test_merge_package_different_signed_locktimes() { + // Malleable HTLC transactions are signed over the locktime, and can't be aggregated with + // different locktimes. + let offered_htlc_1 = dumb_offered_htlc_output!(900, ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()); + let offered_htlc_2 = dumb_offered_htlc_output!(901, ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()); + let accepted_htlc = dumb_accepted_htlc_output!(ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()); + + let mut offered_htlc_1_package = PackageTemplate::build_package(fake_txid(1), 0, offered_htlc_1.clone(), 0); + let mut offered_htlc_2_package = PackageTemplate::build_package(fake_txid(1), 1, offered_htlc_2.clone(), 0); + let mut accepted_htlc_package = PackageTemplate::build_package(fake_txid(1), 2, accepted_htlc.clone(), 1001); + + assert!(!offered_htlc_2_package.can_merge_with(&offered_htlc_1_package, 1000)); + assert!(offered_htlc_2_package.merge_package(offered_htlc_1_package.clone(), 1000).is_err()); + assert!(!offered_htlc_1_package.can_merge_with(&offered_htlc_2_package, 1000)); + assert!(offered_htlc_1_package.merge_package(offered_htlc_2_package.clone(), 1000).is_err()); - 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); + assert!(!accepted_htlc_package.can_merge_with(&offered_htlc_1_package, 1000)); + assert!(accepted_htlc_package.merge_package(offered_htlc_1_package.clone(), 1000).is_err()); + assert!(!offered_htlc_1_package.can_merge_with(&accepted_htlc_package, 1000)); + assert!(offered_htlc_1_package.merge_package(accepted_htlc_package.clone(), 1000).is_err()); } #[test] - #[should_panic] - fn test_package_empty() { - let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap(); - let secp_ctx = Secp256k1::new(); - let revk_outp = dumb_revk_output!(secp_ctx, false); + fn test_merge_package_different_effective_locktimes() { + // Spends of outputs can have different minimum locktimes, and are not mergeable if they are in the + // future. + let old_outp_1 = dumb_counterparty_received_output!(1_000_000, 900, ChannelTypeFeatures::only_static_remote_key()); + let old_outp_2 = dumb_counterparty_received_output!(1_000_000, 901, ChannelTypeFeatures::only_static_remote_key()); + let future_outp_1 = dumb_counterparty_received_output!(1_000_000, 1001, ChannelTypeFeatures::only_static_remote_key()); + let future_outp_2 = dumb_counterparty_received_output!(1_000_000, 1002, ChannelTypeFeatures::only_static_remote_key()); + + let old_outp_1_package = PackageTemplate::build_package(fake_txid(1), 0, old_outp_1.clone(), 0); + let old_outp_2_package = PackageTemplate::build_package(fake_txid(1), 1, old_outp_2.clone(), 0); + let future_outp_1_package = PackageTemplate::build_package(fake_txid(1), 2, future_outp_1.clone(), 0); + let future_outp_2_package = PackageTemplate::build_package(fake_txid(1), 3, future_outp_2.clone(), 0); + + assert!(old_outp_1_package.can_merge_with(&old_outp_2_package, 1000)); + assert!(old_outp_2_package.can_merge_with(&old_outp_1_package, 1000)); + assert!(old_outp_1_package.clone().merge_package(old_outp_2_package.clone(), 1000).is_ok()); + assert!(old_outp_2_package.clone().merge_package(old_outp_1_package.clone(), 1000).is_ok()); + + assert!(!future_outp_1_package.can_merge_with(&future_outp_2_package, 1000)); + assert!(!future_outp_2_package.can_merge_with(&future_outp_1_package, 1000)); + assert!(future_outp_1_package.clone().merge_package(future_outp_2_package.clone(), 1000).is_err()); + assert!(future_outp_2_package.clone().merge_package(future_outp_1_package.clone(), 1000).is_err()); + } - 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); - empty_package.merge_package(package); + #[test] + fn test_merge_package_holder_htlc_output_clusters() { + // Signed locktimes of 0. + let unpinnable_1 = dumb_accepted_htlc_output!(ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()); + let unpinnable_2 = dumb_accepted_htlc_output!(ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()); + let considered_pinnable = dumb_accepted_htlc_output!(ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()); + // Signed locktimes of 1000. + let pinnable_1 = dumb_offered_htlc_output!(1000, ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()); + let pinnable_2 = dumb_offered_htlc_output!(1000, ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()); + + let mut unpinnable_1_package = PackageTemplate::build_package(fake_txid(1), 0, unpinnable_1.clone(), 1100); + let mut unpinnable_2_package = PackageTemplate::build_package(fake_txid(1), 1, unpinnable_2.clone(), 1100); + let mut considered_pinnable_package = PackageTemplate::build_package(fake_txid(1), 2, considered_pinnable.clone(), 1001); + let mut pinnable_1_package = PackageTemplate::build_package(fake_txid(1), 3, pinnable_1.clone(), 0); + let mut pinnable_2_package = PackageTemplate::build_package(fake_txid(1), 4, pinnable_2.clone(), 0); + + // Unpinnable with signed locktimes of 0. + let unpinnable_cluster = [&mut unpinnable_1_package, &mut unpinnable_2_package]; + // Pinnables with signed locktime of 1000. + let pinnable_cluster = [&mut pinnable_1_package, &mut pinnable_2_package]; + // Pinnable with signed locktime of 0. + let considered_pinnable_cluster = [&mut considered_pinnable_package]; + // Pinnable and unpinnable malleable packages are kept separate. A package is considered + // unpinnable if it can only be claimed by the counterparty a given amount of time in the + // future. + let clusters = [unpinnable_cluster.as_slice(), pinnable_cluster.as_slice(), considered_pinnable_cluster.as_slice()]; + + for a in 0..clusters.len() { + for b in 0..clusters.len() { + for i in 0..clusters[a].len() { + for j in 0..clusters[b].len() { + if a != b { + assert!(!clusters[a][i].can_merge_with(clusters[b][j], 1000)); + } else { + if i != j { + assert!(clusters[a][i].can_merge_with(clusters[b][j], 1000)); + } + } + } + } + } + } + + let mut packages = vec![ + unpinnable_1_package, unpinnable_2_package, considered_pinnable_package, + pinnable_1_package, pinnable_2_package, + ]; + for i in (1..packages.len()).rev() { + for j in 0..i { + if packages[i].can_merge_with(&packages[j], 1000) { + let merge = packages.remove(i); + assert!(packages[j].merge_package(merge, 1000).is_ok()); + } + } + } + assert_eq!(packages.len(), 3); } #[test] #[should_panic] - fn test_package_differing_categories() { - let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap(); - let secp_ctx = Secp256k1::new(); - let revk_outp = dumb_revk_output!(secp_ctx, false); - let counterparty_outp = dumb_counterparty_output!(secp_ctx, 0, ChannelTypeFeatures::only_static_remote_key()); + fn test_merge_package_different_tx_trees() { + let offered_htlc = dumb_offered_htlc_output!(900, ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()); + let mut offered_htlc_package = PackageTemplate::build_package(fake_txid(1), 0, offered_htlc.clone(), 0); + let counterparty_received_htlc = dumb_counterparty_received_output!(1_000_000, 900, ChannelTypeFeatures::only_static_remote_key()); + let counterparty_received_htlc_package = PackageTemplate::build_package(fake_txid(2), 0, counterparty_received_htlc.clone(), 0); - 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); + assert!(!offered_htlc_package.can_merge_with(&counterparty_received_htlc_package, 1000)); + assert!(offered_htlc_package.merge_package(counterparty_received_htlc_package.clone(), 1000).is_err()); } #[test] fn test_package_split_malleable() { - let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap(); - let secp_ctx = Secp256k1::new(); - let revk_outp_one = dumb_revk_output!(secp_ctx, false); - 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); - 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); + let revk_outp_one = dumb_revk_output!(false); + let revk_outp_two = dumb_revk_output!(false); + let revk_outp_three = dumb_revk_output!(false); + + let mut package_one = PackageTemplate::build_package(fake_txid(1), 0, revk_outp_one, 1100); + let package_two = PackageTemplate::build_package(fake_txid(1), 1, revk_outp_two, 1100); + let package_three = PackageTemplate::build_package(fake_txid(1), 2, revk_outp_three, 1100); + + assert!(package_one.merge_package(package_two, 1000).is_ok()); + assert!(package_one.merge_package(package_three, 1000).is_ok()); assert_eq!(package_one.outpoints().len(), 3); - if let Some(split_package) = package_one.split_package(&BitcoinOutPoint { txid, vout: 1 }) { + if let Some(split_package) = package_one.split_package(&BitcoinOutPoint { txid: fake_txid(1), vout: 1 }) { // Packages attributes should be identical assert!(split_package.is_malleable()); 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); } else { panic!(); } @@ -1434,21 +1607,18 @@ mod tests { #[test] fn test_package_split_untractable() { - let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap(); - let htlc_outp_one = dumb_htlc_output!(); + let htlc_outp_one = dumb_accepted_htlc_output!(ChannelTypeFeatures::only_static_remote_key()); - let mut package_one = PackageTemplate::build_package(txid, 0, htlc_outp_one, 1000); - let ret_split = package_one.split_package(&BitcoinOutPoint { txid, vout: 0}); + let mut package_one = PackageTemplate::build_package(fake_txid(1), 0, htlc_outp_one, 1000); + let ret_split = package_one.split_package(&BitcoinOutPoint { txid: fake_txid(1), vout: 0 }); assert!(ret_split.is_none()); } #[test] fn test_package_timer() { - let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap(); - let secp_ctx = Secp256k1::new(); - let revk_outp = dumb_revk_output!(secp_ctx, false); + let revk_outp = dumb_revk_output!(false); - let mut package = PackageTemplate::build_package(txid, 0, revk_outp, 1000); + let mut package = PackageTemplate::build_package(fake_txid(1), 0, revk_outp, 1000); assert_eq!(package.timer(), 0); package.set_timer(101); assert_eq!(package.timer(), 101); @@ -1456,40 +1626,35 @@ mod tests { #[test] fn test_package_amounts() { - let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap(); - let secp_ctx = Secp256k1::new(); - let counterparty_outp = dumb_counterparty_output!(secp_ctx, 1_000_000, ChannelTypeFeatures::only_static_remote_key()); + let counterparty_outp = dumb_counterparty_received_output!(1_000_000, 1000, ChannelTypeFeatures::only_static_remote_key()); - let package = PackageTemplate::build_package(txid, 0, counterparty_outp, 1000); + let package = PackageTemplate::build_package(fake_txid(1), 0, counterparty_outp, 1000); assert_eq!(package.package_amount(), 1000); } #[test] fn test_package_weight() { - let txid = Txid::from_str("c2d4449afa8d26140898dd54d3390b057ba2a5afcf03ba29d7dc0d8b9ffe966e").unwrap(); - let secp_ctx = Secp256k1::new(); - // (nVersion (4) + nLocktime (4) + count_tx_in (1) + prevout (36) + sequence (4) + script_length (1) + count_tx_out (1) + value (8) + var_int (1)) * WITNESS_SCALE_FACTOR + witness marker (2) let weight_sans_output = (4 + 4 + 1 + 36 + 4 + 1 + 1 + 8 + 1) * WITNESS_SCALE_FACTOR as u64 + 2; { - let revk_outp = dumb_revk_output!(secp_ctx, false); - let package = PackageTemplate::build_package(txid, 0, revk_outp, 0); + let revk_outp = dumb_revk_output!(false); + let package = PackageTemplate::build_package(fake_txid(1), 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); + let counterparty_outp = dumb_counterparty_received_output!(1_000_000, 1000, channel_type_features.clone()); + let package = PackageTemplate::build_package(fake_txid(1), 0, counterparty_outp, 1000); assert_eq!(package.package_weight(&ScriptBuf::new()), weight_sans_output + weight_received_htlc(channel_type_features)); } } { 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); + let counterparty_outp = dumb_counterparty_offered_output!(1_000_000, channel_type_features.clone()); + let package = PackageTemplate::build_package(fake_txid(1), 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 dc8edacb85e..24a882ca670 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -2513,10 +2513,15 @@ fn test_justice_tx_htlc_timeout() { mine_transaction(&nodes[1], &revoked_local_txn[0]); { let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert_eq!(node_txn.len(), 1); // ChannelMonitor: penalty tx - assert_eq!(node_txn[0].input.len(), 2); // We should claim the revoked output and the HTLC output - check_spends!(node_txn[0], revoked_local_txn[0]); - node_txn.swap_remove(0); + // The unpinnable, revoked to_self output, and the pinnable, revoked htlc output will + // be claimed in separate transactions. + assert_eq!(node_txn.len(), 2); + for tx in node_txn.iter() { + assert_eq!(tx.input.len(), 1); + check_spends!(tx, revoked_local_txn[0]); + } + assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output); + node_txn.clear(); } check_added_monitors!(nodes[1], 1); check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed, [nodes[0].node.get_our_node_id()], 100000); @@ -2694,7 +2699,7 @@ fn do_test_forming_justice_tx_from_monitor_updates(broadcast_initial_commitment: #[test] -fn claim_htlc_outputs_shared_tx() { +fn claim_htlc_outputs() { // Node revoked old state, htlcs haven't time out yet, claim them in shared justice tx let mut chanmon_cfgs = create_chanmon_cfgs(2); chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true; @@ -2721,7 +2726,7 @@ fn claim_htlc_outputs_shared_tx() { assert_eq!(revoked_local_txn[1].input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); // HTLC-Timeout check_spends!(revoked_local_txn[1], revoked_local_txn[0]); - //Revoke the old state + // Revoke the old state. claim_payment(&nodes[0], &vec!(&nodes[1])[..], payment_preimage_1); { @@ -2735,97 +2740,19 @@ fn claim_htlc_outputs_shared_tx() { assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); - assert_eq!(node_txn.len(), 1); // ChannelMonitor: penalty tx - - assert_eq!(node_txn[0].input.len(), 3); // Claim the revoked output + both revoked HTLC outputs - check_spends!(node_txn[0], revoked_local_txn[0]); - - let mut witness_lens = BTreeSet::new(); - witness_lens.insert(node_txn[0].input[0].witness.last().unwrap().len()); - witness_lens.insert(node_txn[0].input[1].witness.last().unwrap().len()); - witness_lens.insert(node_txn[0].input[2].witness.last().unwrap().len()); - assert_eq!(witness_lens.len(), 3); - assert_eq!(*witness_lens.iter().skip(0).next().unwrap(), 77); // revoked to_local - assert_eq!(*witness_lens.iter().skip(1).next().unwrap(), OFFERED_HTLC_SCRIPT_WEIGHT); // revoked offered HTLC - assert_eq!(*witness_lens.iter().skip(2).next().unwrap(), ACCEPTED_HTLC_SCRIPT_WEIGHT); // revoked received HTLC - - // Finally, mine the penalty transaction and check that we get an HTLC failure after - // ANTI_REORG_DELAY confirmations. - mine_transaction(&nodes[1], &node_txn[0]); - connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); - expect_payment_failed!(nodes[1], payment_hash_2, false); - } - get_announce_close_broadcast_events(&nodes, 0, 1); - assert_eq!(nodes[0].node.list_channels().len(), 0); - assert_eq!(nodes[1].node.list_channels().len(), 0); -} - -#[test] -fn claim_htlc_outputs_single_tx() { - // Node revoked old state, htlcs have timed out, claim each of them in separated justice tx - let mut chanmon_cfgs = create_chanmon_cfgs(2); - chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true; - let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); - let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); - let nodes = create_network(2, &node_cfgs, &node_chanmgrs); - - let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1); - - // Rebalance the network to generate htlc in the two directions - send_payment(&nodes[0], &[&nodes[1]], 8_000_000); - // node[0] is gonna to revoke an old state thus node[1] should be able to claim both offered/received HTLC outputs on top of commitment tx, but this - // time as two different claim transactions as we're gonna to timeout htlc with given a high current height - let payment_preimage_1 = route_payment(&nodes[0], &[&nodes[1]], 3_000_000).0; - let (_payment_preimage_2, payment_hash_2, ..) = route_payment(&nodes[1], &[&nodes[0]], 3_000_000); - - // Get the will-be-revoked local txn from node[0] - let revoked_local_txn = get_local_commitment_txn!(nodes[0], chan_1.2); - - //Revoke the old state - claim_payment(&nodes[0], &vec!(&nodes[1])[..], payment_preimage_1); - - { - confirm_transaction_at(&nodes[0], &revoked_local_txn[0], 100); - check_added_monitors!(nodes[0], 1); - confirm_transaction_at(&nodes[1], &revoked_local_txn[0], 100); - check_added_monitors!(nodes[1], 1); - check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed, [nodes[0].node.get_our_node_id()], 100000); - let mut events = nodes[0].node.get_and_clear_pending_events(); - expect_pending_htlcs_forwardable_conditions(events[0..2].to_vec(), &[HTLCDestination::FailedPayment { payment_hash: payment_hash_2 }]); - match events.last().unwrap() { - Event::ChannelClosed { reason: ClosureReason::CommitmentTxConfirmed, .. } => {} - _ => panic!("Unexpected event"), - } - - connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); - assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); - - let mut node_txn = nodes[1].tx_broadcaster.txn_broadcast(); - - // Check the pair local commitment and HTLC-timeout broadcast due to HTLC expiration - assert_eq!(node_txn[0].input.len(), 1); - check_spends!(node_txn[0], chan_1.3); - assert_eq!(node_txn[1].input.len(), 1); - let witness_script = node_txn[1].input[0].witness.last().unwrap(); - assert_eq!(witness_script.len(), OFFERED_HTLC_SCRIPT_WEIGHT); //Spending an offered htlc output - check_spends!(node_txn[1], node_txn[0]); - - // 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_eq!(node_txn[0].input.len(), 1); - assert_eq!(node_txn[1].input.len(), 1); - assert_eq!(node_txn[2].input.len(), 1); - + assert_eq!(node_txn.len(), 2); // Two penalty transactions: + assert_eq!(node_txn[0].input.len(), 1); // Claims the unpinnable, revoked output. + assert_eq!(node_txn[1].input.len(), 2); // Claims both pinnable, revoked HTLC outputs separately. check_spends!(node_txn[0], revoked_local_txn[0]); check_spends!(node_txn[1], revoked_local_txn[0]); - check_spends!(node_txn[2], revoked_local_txn[0]); + assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output); + assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[1].previous_output); + assert_ne!(node_txn[1].input[0].previous_output, node_txn[1].input[1].previous_output); let mut witness_lens = BTreeSet::new(); witness_lens.insert(node_txn[0].input[0].witness.last().unwrap().len()); witness_lens.insert(node_txn[1].input[0].witness.last().unwrap().len()); - witness_lens.insert(node_txn[2].input[0].witness.last().unwrap().len()); + witness_lens.insert(node_txn[1].input[1].witness.last().unwrap().len()); assert_eq!(witness_lens.len(), 3); assert_eq!(*witness_lens.iter().skip(0).next().unwrap(), 77); // revoked to_local assert_eq!(*witness_lens.iter().skip(1).next().unwrap(), OFFERED_HTLC_SCRIPT_WEIGHT); // revoked offered HTLC @@ -2835,7 +2762,6 @@ fn claim_htlc_outputs_single_tx() { // ANTI_REORG_DELAY confirmations. mine_transaction(&nodes[1], &node_txn[0]); mine_transaction(&nodes[1], &node_txn[1]); - mine_transaction(&nodes[1], &node_txn[2]); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); expect_payment_failed!(nodes[1], payment_hash_2, false); } @@ -2991,23 +2917,24 @@ fn test_htlc_on_chain_success() { macro_rules! check_tx_local_broadcast { ($node: expr, $htlc_offered: expr, $commitment_tx: expr) => { { let mut node_txn = $node.tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert_eq!(node_txn.len(), 2); - // Node[1]: 2 * HTLC-timeout tx - // Node[0]: 2 * HTLC-timeout tx - check_spends!(node_txn[0], $commitment_tx); - check_spends!(node_txn[1], $commitment_tx); - assert_ne!(node_txn[0].lock_time, LockTime::ZERO); - assert_ne!(node_txn[1].lock_time, LockTime::ZERO); + // HTLC timeout claims for non-anchor channels are only aggregated when claimed from the + // remote commitment transaction. if $htlc_offered { - assert_eq!(node_txn[0].input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); - assert_eq!(node_txn[1].input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); - assert!(node_txn[0].output[0].script_pubkey.is_p2wsh()); // revokeable output - assert!(node_txn[1].output[0].script_pubkey.is_p2wsh()); // revokeable output + assert_eq!(node_txn.len(), 2); + for tx in node_txn.iter() { + check_spends!(tx, $commitment_tx); + assert_ne!(tx.lock_time, LockTime::ZERO); + assert_eq!(tx.input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); + assert!(tx.output[0].script_pubkey.is_p2wsh()); // revokeable output + } + assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output); } else { + assert_eq!(node_txn.len(), 1); + check_spends!(node_txn[0], $commitment_tx); + assert_ne!(node_txn[0].lock_time, LockTime::ZERO); assert_eq!(node_txn[0].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); - assert_eq!(node_txn[1].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); assert!(node_txn[0].output[0].script_pubkey.is_p2wpkh()); // direct payment - assert!(node_txn[1].output[0].script_pubkey.is_p2wpkh()); // direct payment + assert_ne!(node_txn[0].input[0].previous_output, node_txn[0].input[1].previous_output); } node_txn.clear(); } } @@ -3024,23 +2951,20 @@ fn test_htlc_on_chain_success() { check_added_monitors!(nodes[1], 1); check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed, [nodes[0].node.get_our_node_id()], 100000); let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().clone(); - assert!(node_txn.len() == 1 || node_txn.len() == 3); // HTLC-Success, 2* RBF bumps of above HTLC txn + assert!(node_txn.len() == 1 || node_txn.len() == 2); // HTLC-Success, RBF bump of above aggregated HTLC txn let commitment_spend = if node_txn.len() == 1 { &node_txn[0] } else { // Certain `ConnectStyle`s will cause RBF bumps of the previous HTLC transaction to be broadcast. // FullBlockViaListen + assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output); if node_txn[0].input[0].previous_output.txid == node_a_commitment_tx[0].compute_txid() { check_spends!(node_txn[1], commitment_tx[0]); - check_spends!(node_txn[2], commitment_tx[0]); - assert_ne!(node_txn[1].input[0].previous_output.vout, node_txn[2].input[0].previous_output.vout); &node_txn[0] } else { check_spends!(node_txn[0], commitment_tx[0]); - check_spends!(node_txn[1], commitment_tx[0]); - assert_ne!(node_txn[0].input[0].previous_output.vout, node_txn[1].input[0].previous_output.vout); - &node_txn[2] + &node_txn[1] } }; @@ -4764,10 +4688,15 @@ fn test_static_spendable_outputs_justice_tx_revoked_commitment_tx() { check_added_monitors!(nodes[1], 1); check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed, [nodes[0].node.get_our_node_id()], 100000); + // The unpinnable, revoked to_self output and the pinnable, revoked HTLC output will be claimed + // in separate transactions. let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().clone(); - assert_eq!(node_txn.len(), 1); - assert_eq!(node_txn[0].input.len(), 2); - check_spends!(node_txn[0], revoked_local_txn[0]); + assert_eq!(node_txn.len(), 2); + for tx in node_txn.iter() { + assert_eq!(tx.input.len(), 1); + check_spends!(tx, revoked_local_txn[0]); + } + assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output); mine_transaction(&nodes[1], &node_txn[0]); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); @@ -4815,31 +4744,32 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() { check_added_monitors!(nodes[1], 1); check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed, [nodes[0].node.get_our_node_id()], 100000); + // There will be 2 justice transactions: + // - One on the unpinnable, revoked to_self output on the commitment transaction and on + // the unpinnable, revoked to_self output on the HTLC-timeout transaction. + // - One on the pinnable, revoked HTLC output on the commitment transaction. + // The latter transaction will become out-of-date as it spends the output already spent by + // revoked_htlc_txn[0]. That's OK, we'll spend with valid transactions next. let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().clone(); - assert_eq!(node_txn.len(), 2); // ChannelMonitor: bogus justice tx, justice tx on revoked outputs - // The first transaction generated is bogus - it spends both outputs of revoked_local_txn[0] - // including the one already spent by revoked_htlc_txn[1]. That's OK, we'll spend with valid - // transactions next... - assert_eq!(node_txn[0].input.len(), 3); + assert_eq!(node_txn.len(), 2); + assert_eq!(node_txn[0].input.len(), 2); check_spends!(node_txn[0], revoked_local_txn[0], revoked_htlc_txn[0]); + assert_ne!(node_txn[0].input[0].previous_output, node_txn[0].input[1].previous_output); - assert_eq!(node_txn[1].input.len(), 2); - check_spends!(node_txn[1], revoked_local_txn[0], revoked_htlc_txn[0]); - if node_txn[1].input[1].previous_output.txid == revoked_htlc_txn[0].compute_txid() { - assert_ne!(node_txn[1].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output); - } else { - assert_eq!(node_txn[1].input[0].previous_output.txid, revoked_htlc_txn[0].compute_txid()); - assert_ne!(node_txn[1].input[1].previous_output, revoked_htlc_txn[0].input[0].previous_output); - } + assert_eq!(node_txn[1].input.len(), 1); + check_spends!(node_txn[1], revoked_local_txn[0]); + assert_eq!(node_txn[1].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output); + assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output); + assert_ne!(node_txn[0].input[1].previous_output, node_txn[1].input[0].previous_output); - mine_transaction(&nodes[1], &node_txn[1]); + mine_transaction(&nodes[1], &node_txn[0]); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); // Check B's ChannelMonitor was able to generate the right spendable output descriptor let spend_txn = check_spendable_outputs!(nodes[1], node_cfgs[1].keys_manager); assert_eq!(spend_txn.len(), 1); assert_eq!(spend_txn[0].input.len(), 1); - check_spends!(spend_txn[0], node_txn[1]); + check_spends!(spend_txn[0], node_txn[0]); } #[test] @@ -4885,21 +4815,15 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_success_tx() { check_added_monitors!(nodes[0], 1); check_closed_event!(nodes[0], 1, ClosureReason::CommitmentTxConfirmed, [nodes[1].node.get_our_node_id()], 100000); + // There will be 2 justice transactions, one on the revoked HTLC output on the commitment + // transaction, and one on the revoked to_self output on the HTLC-success transaction. let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clone(); - assert_eq!(node_txn.len(), 2); // ChannelMonitor: justice tx on revoked commitment, justice tx on revoked HTLC-success - - // The first transaction generated is bogus - it spends both outputs of revoked_local_txn[0] - // including the one already spent by revoked_htlc_txn[0]. That's OK, we'll spend with valid - // transactions next... - assert_eq!(node_txn[0].input.len(), 2); - check_spends!(node_txn[0], revoked_local_txn[0], revoked_htlc_txn[0]); - if node_txn[0].input[1].previous_output.txid == revoked_htlc_txn[0].compute_txid() { - assert_eq!(node_txn[0].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output); - } else { - assert_eq!(node_txn[0].input[0].previous_output.txid, revoked_htlc_txn[0].compute_txid()); - assert_eq!(node_txn[0].input[1].previous_output, revoked_htlc_txn[0].input[0].previous_output); - } + assert_eq!(node_txn.len(), 2); + // The first transaction generated will become out-of-date as it spends the output already spent + // by revoked_htlc_txn[0]. That's OK, we'll spend with valid transactions next... + assert_eq!(node_txn[0].input.len(), 1); + check_spends!(node_txn[0], revoked_local_txn[0]); assert_eq!(node_txn[1].input.len(), 1); check_spends!(node_txn[1], revoked_htlc_txn[0]); @@ -7506,14 +7430,6 @@ fn test_bump_penalty_txn_on_revoked_commitment() { assert_eq!(revoked_txn[0].output.len(), 4); assert_eq!(revoked_txn[0].input.len(), 1); assert_eq!(revoked_txn[0].input[0].previous_output.txid, chan.3.compute_txid()); - let revoked_txid = revoked_txn[0].compute_txid(); - - let mut penalty_sum = 0; - for outp in revoked_txn[0].output.iter() { - if outp.script_pubkey.is_p2wsh() { - penalty_sum += outp.value.to_sat(); - } - } // Connect blocks to change height_timer range to see if we use right soonest_timelock let header_114 = connect_blocks(&nodes[1], 14); @@ -7523,66 +7439,64 @@ fn test_bump_penalty_txn_on_revoked_commitment() { connect_block(&nodes[1], &create_dummy_block(header_114, 42, vec![revoked_txn[0].clone()])); check_added_monitors!(nodes[1], 1); - // One or more justice tx should have been broadcast, check it - let penalty_1; - let feerate_1; - { - let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert_eq!(node_txn.len(), 1); // justice tx (broadcasted from ChannelMonitor) - assert_eq!(node_txn[0].input.len(), 3); // Penalty txn claims to_local, offered_htlc and received_htlc outputs - assert_eq!(node_txn[0].output.len(), 1); - check_spends!(node_txn[0], revoked_txn[0]); - let fee_1 = penalty_sum - node_txn[0].output[0].value.to_sat(); - feerate_1 = fee_1 * 1000 / node_txn[0].weight().to_wu(); - penalty_1 = node_txn[0].compute_txid(); - node_txn.clear(); - }; + macro_rules! check_broadcasted_txn { + ($penalty_txids:ident, $fee_rates:ident) => { + let mut $penalty_txids = new_hash_map(); + let mut $fee_rates = new_hash_map(); + { + let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap(); + // 2 justice txs can be broadcasted from ChannelMonitor: + // - 1 unpinnable, revoked to_self output. + // - 2 pinnable, revoked HTLC outputs. + // Note that the revoked HTLC output has a slow timer, as we can always claim + // from the second stage HTLC transaction. + assert_eq!(node_txn.len(), 2); + for tx in node_txn.iter() { + assert!(tx.input.len() == 1 || tx.input.len() == 2); + assert_eq!(tx.output.len(), 1); + check_spends!(tx, revoked_txn[0]); + let total_input: u64 = tx.input.iter().map(|i| revoked_txn[0].output[i.previous_output.vout as usize].value.to_sat()).sum(); + let fee_rate: u64 = (total_input - tx.output[0].value.to_sat()) * 1000 / tx.weight().to_wu(); + assert_ne!(fee_rate, 0); + for input in &tx.input { + $fee_rates.insert(input.previous_output, fee_rate); + $penalty_txids.insert(input.previous_output, tx.compute_txid()); + } + } + assert_eq!($fee_rates.len(), 3); + assert_eq!($penalty_txids.len(), 3); + node_txn.clear(); + } + } + } + + // One or more justice tx should have been broadcast, check it. + check_broadcasted_txn!(penalty_txids_1, fee_rates_1); - // After exhaustion of height timer, a new bumped justice tx should have been broadcast, check it + // After 15 blocks, the height timer for both the to_self claim and HTLC claims should be triggered, + // and new bumped justice transactions should have been broadcast. connect_blocks(&nodes[1], 15); - let mut penalty_2 = penalty_1; - let mut feerate_2 = 0; - { - let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert_eq!(node_txn.len(), 1); - if node_txn[0].input[0].previous_output.txid == revoked_txid { - assert_eq!(node_txn[0].input.len(), 3); // Penalty txn claims to_local, offered_htlc and received_htlc outputs - assert_eq!(node_txn[0].output.len(), 1); - check_spends!(node_txn[0], revoked_txn[0]); - penalty_2 = node_txn[0].compute_txid(); - // Verify new bumped tx is different from last claiming transaction, we don't want spurrious rebroadcast - assert_ne!(penalty_2, penalty_1); - let fee_2 = penalty_sum - node_txn[0].output[0].value.to_sat(); - feerate_2 = fee_2 * 1000 / node_txn[0].weight().to_wu(); - // Verify 25% bump heuristic - assert!(feerate_2 * 100 >= feerate_1 * 125); - node_txn.clear(); - } + check_broadcasted_txn!(penalty_txids_2, fee_rates_2); + // Verify new bumped tx is different from last claiming transaction, we don't want spurious rebroadcasts. + for (outpoint, txid) in &penalty_txids_2 { + assert_ne!(txid, &penalty_txids_1[outpoint]); + } + // Verify 25% bump heuristic. + for (outpoint, fee_rate) in &fee_rates_2 { + assert!(fee_rate * 100 >= fee_rates_1[outpoint] * 125); } - assert_ne!(feerate_2, 0); - // After exhaustion of height timer for a 2nd time, a new bumped justice tx should have been broadcast, check it - connect_blocks(&nodes[1], 1); - let penalty_3; - let mut feerate_3 = 0; - { - let mut node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert_eq!(node_txn.len(), 1); - if node_txn[0].input[0].previous_output.txid == revoked_txid { - assert_eq!(node_txn[0].input.len(), 3); // Penalty txn claims to_local, offered_htlc and received_htlc outputs - assert_eq!(node_txn[0].output.len(), 1); - check_spends!(node_txn[0], revoked_txn[0]); - penalty_3 = node_txn[0].compute_txid(); - // Verify new bumped tx is different from last claiming transaction, we don't want spurrious rebroadcast - assert_ne!(penalty_3, penalty_2); - let fee_3 = penalty_sum - node_txn[0].output[0].value.to_sat(); - feerate_3 = fee_3 * 1000 / node_txn[0].weight().to_wu(); - // Verify 25% bump heuristic - assert!(feerate_3 * 100 >= feerate_2 * 125); - node_txn.clear(); - } + // After another 15 blocks, the height timers should be triggered again. + connect_blocks(&nodes[1], 15); + check_broadcasted_txn!(penalty_txids_3, fee_rates_3); + // Verify new bumped tx is different from last claiming transaction, we don't want spurious rebroadcasts. + for (outpoint, txid) in &penalty_txids_3 { + assert_ne!(txid, &penalty_txids_1[outpoint]); + } + // Verify 25% bump heuristic. + for (outpoint, fee_rate) in &fee_rates_3 { + assert!(fee_rate * 100 >= fee_rates_2[outpoint] * 125); } - assert_ne!(feerate_3, 0); nodes[1].node.get_and_clear_pending_events(); nodes[1].node.get_and_clear_pending_msg_events(); @@ -7662,41 +7576,42 @@ fn test_bump_penalty_txn_on_revoked_htlcs() { let penalty_txn; { let mut node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert_eq!(node_txn.len(), 4); // 3 penalty txn on revoked commitment tx + 1 penalty tnx on revoked HTLC txn + // 1 penalty transaction of the to_remote output on the revoked commitment tx, + // 1 aggregated penalty transaction of the htlc outputs on the revoked commitment tx, + // 1 aggregated penalty transaction of the two revoked HTLC txs. + assert_eq!(node_txn.len(), 3); // Verify claim tx are spending revoked HTLC txn // node_txn 0-2 each spend a separate revoked output from revoked_local_txn[0] - // Note that node_txn[0] and node_txn[1] are bogus - they double spend the revoked_htlc_txn + // Note that node_txn[1] and node_txn[2] are bogus - they double spend the revoked_htlc_txn // which are included in the same block (they are broadcasted because we scan the // transactions linearly and generate claims as we go, they likely should be removed in the // future). assert_eq!(node_txn[0].input.len(), 1); check_spends!(node_txn[0], revoked_local_txn[0]); - assert_eq!(node_txn[1].input.len(), 1); + assert_eq!(node_txn[1].input.len(), 2); check_spends!(node_txn[1], revoked_local_txn[0]); - assert_eq!(node_txn[2].input.len(), 1); - check_spends!(node_txn[2], revoked_local_txn[0]); // Each of the three justice transactions claim a separate (single) output of the three // available, which we check here: assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output); - assert_ne!(node_txn[0].input[0].previous_output, node_txn[2].input[0].previous_output); - assert_ne!(node_txn[1].input[0].previous_output, node_txn[2].input[0].previous_output); + assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[1].previous_output); + assert_ne!(node_txn[1].input[0].previous_output, node_txn[1].input[1].previous_output); - assert_eq!(node_txn[0].input[0].previous_output, revoked_htlc_txn[1].input[0].previous_output); - assert_eq!(node_txn[1].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output); + assert_eq!(node_txn[1].input[0].previous_output, revoked_htlc_txn[1].input[0].previous_output); + assert_eq!(node_txn[1].input[1].previous_output, revoked_htlc_txn[0].input[0].previous_output); // node_txn[3] spends the revoked outputs from the revoked_htlc_txn (which only have one // output, checked above). - assert_eq!(node_txn[3].input.len(), 2); - assert_eq!(node_txn[3].output.len(), 1); - check_spends!(node_txn[3], revoked_htlc_txn[0], revoked_htlc_txn[1]); + assert_eq!(node_txn[2].input.len(), 2); + assert_eq!(node_txn[2].output.len(), 1); + check_spends!(node_txn[2], revoked_htlc_txn[0], revoked_htlc_txn[1]); - first = node_txn[3].compute_txid(); + first = node_txn[2].compute_txid(); // Store both feerates for later comparison - let fee_1 = revoked_htlc_txn[0].output[0].value + revoked_htlc_txn[1].output[0].value - node_txn[3].output[0].value; - feerate_1 = fee_1 * 1000 / node_txn[3].weight().to_wu(); - penalty_txn = vec![node_txn[2].clone()]; + let fee_1 = revoked_htlc_txn[0].output[0].value + revoked_htlc_txn[1].output[0].value - node_txn[2].output[0].value; + feerate_1 = fee_1 * 1000 / node_txn[2].weight().to_wu(); + penalty_txn = vec![node_txn[0].clone()]; node_txn.clear(); } @@ -7902,7 +7817,7 @@ fn test_counterparty_raa_skip_no_crash() { #[test] fn test_bump_txn_sanitize_tracking_maps() { - // Sanitizing pendning_claim_request and claimable_outpoints used to be buggy, + // Sanitizing pending_claim_request and claimable_outpoints used to be buggy, // verify we clean then right after expiration of ANTI_REORG_DELAY. let chanmon_cfgs = create_chanmon_cfgs(2); @@ -7933,11 +7848,15 @@ fn test_bump_txn_sanitize_tracking_maps() { check_closed_event!(nodes[0], 1, ClosureReason::CommitmentTxConfirmed, [nodes[1].node.get_our_node_id()], 1000000); let penalty_txn = { let mut node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert_eq!(node_txn.len(), 3); //ChannelMonitor: justice txn * 3 + assert_eq!(node_txn.len(), 2); //ChannelMonitor: justice txn * 2 check_spends!(node_txn[0], revoked_local_txn[0]); + assert_eq!(node_txn[0].input.len(), 1); check_spends!(node_txn[1], revoked_local_txn[0]); - check_spends!(node_txn[2], revoked_local_txn[0]); - let penalty_txn = vec![node_txn[0].clone(), node_txn[1].clone(), node_txn[2].clone()]; + assert_eq!(node_txn[1].input.len(), 2); + assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output); + assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[1].previous_output); + assert_ne!(node_txn[1].input[0].previous_output, node_txn[1].input[1].previous_output); + let penalty_txn = vec![node_txn[0].clone(), node_txn[1].clone()]; node_txn.clear(); penalty_txn }; diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 99ad31d3583..caf5d6bd252 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -114,7 +114,7 @@ fn test_spendable_output<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, spendable_t #[test] fn revoked_output_htlc_resolution_timing() { // Tests that HTLCs which were present in a broadcasted remote revoked commitment transaction - // are resolved only after a spend of the HTLC output reaches six confirmations. Preivously + // are resolved only after a spend of the HTLC output reaches six confirmations. Previously // they would resolve after the revoked commitment transaction itself reaches six // confirmations. let chanmon_cfgs = create_chanmon_cfgs(2); @@ -131,7 +131,7 @@ fn revoked_output_htlc_resolution_timing() { let revoked_local_txn = get_local_commitment_txn!(nodes[0], chan.2); assert_eq!(revoked_local_txn.len(), 1); - // Route a dust payment to revoke the above commitment transaction + // Route a dust payment to revoke the above commitment transaction. route_payment(&nodes[0], &[&nodes[1]], 1_000); // Confirm the revoked commitment transaction, closing the channel. @@ -140,9 +140,15 @@ fn revoked_output_htlc_resolution_timing() { check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed, [nodes[0].node.get_our_node_id()], 1000000); check_closed_broadcast!(nodes[1], true); + // Two justice transactions will be broadcast, one on the unpinnable, revoked to_self output, + // and one on the pinnable revoked HTLC output. let bs_spend_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); - assert_eq!(bs_spend_txn.len(), 1); - check_spends!(bs_spend_txn[0], revoked_local_txn[0]); + assert_eq!(bs_spend_txn.len(), 2); + for tx in bs_spend_txn.iter() { + assert_eq!(tx.input.len(), 1); + check_spends!(tx, revoked_local_txn[0]); + } + assert_ne!(bs_spend_txn[0].input[0].previous_output, bs_spend_txn[1].input[0].previous_output); // After the commitment transaction confirms, we should still wait on the HTLC spend // transaction to confirm before resolving the HTLC. @@ -151,7 +157,7 @@ fn revoked_output_htlc_resolution_timing() { assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); // Spend the HTLC output, generating a HTLC failure event after ANTI_REORG_DELAY confirmations. - mine_transaction(&nodes[1], &bs_spend_txn[0]); + mine_transactions(&nodes[1], &[&bs_spend_txn[0], &bs_spend_txn[1]]); assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); @@ -656,20 +662,23 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) { // When the HTLC timeout output is spendable in the next block, A should broadcast it connect_blocks(&nodes[0], htlc_cltv_timeout - nodes[0].best_block_info().1); let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); - assert_eq!(a_broadcast_txn.len(), 2); - assert_eq!(a_broadcast_txn[0].input.len(), 1); + // Aggregated claim transaction. + assert_eq!(a_broadcast_txn.len(), 1); check_spends!(a_broadcast_txn[0], remote_txn[0]); - assert_eq!(a_broadcast_txn[1].input.len(), 1); - check_spends!(a_broadcast_txn[1], remote_txn[0]); - assert_ne!(a_broadcast_txn[0].input[0].previous_output.vout, - a_broadcast_txn[1].input[0].previous_output.vout); + assert_eq!(a_broadcast_txn[0].input.len(), 2); + assert_ne!(a_broadcast_txn[0].input[0].previous_output.vout, a_broadcast_txn[0].input[1].previous_output.vout); // a_broadcast_txn [0] and [1] should spend the HTLC outputs of the commitment tx - assert_eq!(remote_txn[0].output[a_broadcast_txn[0].input[0].previous_output.vout as usize].value.to_sat(), 3_000); - assert_eq!(remote_txn[0].output[a_broadcast_txn[1].input[0].previous_output.vout as usize].value.to_sat(), 4_000); + assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 3_000)); + assert!(a_broadcast_txn[0].input.iter().any(|input| remote_txn[0].output[input.previous_output.vout as usize].value.to_sat() == 4_000)); + + // Confirm node B's claim for node A to remove that claim from the aggregated claim transaction. + mine_transaction(&nodes[0], &b_broadcast_txn[0]); + let a_broadcast_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + let a_htlc_timeout_tx = a_broadcast_txn.into_iter().last().unwrap(); // Once the HTLC-Timeout transaction confirms, A will no longer consider the HTLC // "MaybeClaimable", but instead move it to "AwaitingConfirmations". - mine_transaction(&nodes[0], &a_broadcast_txn[1]); + mine_transaction(&nodes[0], &a_htlc_timeout_tx); assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); assert_eq!(vec![Balance::ClaimableAwaitingConfirmations { amount_satoshis: 4_000, @@ -684,7 +693,7 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) { nodes[0].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()); expect_payment_failed!(nodes[0], timeout_payment_hash, false); - test_spendable_output(&nodes[0], &a_broadcast_txn[1], false); + test_spendable_output(&nodes[0], &a_htlc_timeout_tx, false); // Node B will no longer consider the HTLC "contentious" after the HTLC claim transaction // confirms, and consider it simply "awaiting confirmations". Note that it has to wait for the @@ -726,7 +735,7 @@ fn do_test_claim_value_force_close(anchors: bool, prev_commitment_tx: bool) { // Finally, mine the HTLC timeout transaction that A broadcasted (even though B should be able // to claim this HTLC with the preimage it knows!). It will remain listed as a claimable HTLC // until ANTI_REORG_DELAY confirmations on the spend. - mine_transaction(&nodes[1], &a_broadcast_txn[1]); + mine_transaction(&nodes[1], &a_htlc_timeout_tx); assert_eq!(vec![received_htlc_timeout_claiming_balance.clone()], nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); @@ -893,12 +902,47 @@ fn do_test_balances_on_local_commitment_htlcs(anchors: bool) { }, htlc_balance_known_preimage.clone(), htlc_balance_unknown_preimage.clone()]), sorted_vec(nodes[0].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); if anchors { - handle_bump_htlc_event(&nodes[0], 2); + handle_bump_htlc_event(&nodes[0], 1); + } + let mut timeout_htlc_txn = nodes[0].tx_broadcaster.unique_txn_broadcast(); + if anchors { + // Aggregated HTLC timeouts. + assert_eq!(timeout_htlc_txn.len(), 1); + check_spends!(timeout_htlc_txn[0], commitment_tx, coinbase_tx); + // One input from the commitment transaction for each HTLC, and one input to provide fees. + assert_eq!(timeout_htlc_txn[0].input.len(), 3); + assert_eq!(timeout_htlc_txn[0].input[0].witness.last().unwrap().len(), chan_utils::OFFERED_HTLC_SCRIPT_WEIGHT_ANCHORS); + assert_eq!(timeout_htlc_txn[0].input[1].witness.last().unwrap().len(), chan_utils::OFFERED_HTLC_SCRIPT_WEIGHT_ANCHORS); + } else { + assert_eq!(timeout_htlc_txn.len(), 2); + check_spends!(timeout_htlc_txn[0], commitment_tx); + check_spends!(timeout_htlc_txn[1], commitment_tx); + assert_eq!(timeout_htlc_txn[0].input[0].witness.last().unwrap().len(), chan_utils::OFFERED_HTLC_SCRIPT_WEIGHT); + assert_eq!(timeout_htlc_txn[1].input[0].witness.last().unwrap().len(), chan_utils::OFFERED_HTLC_SCRIPT_WEIGHT); + } + + // Now confirm nodes[1]'s HTLC claim, giving nodes[0] the preimage. Note that the "maybe + // claimable" balance remains until we see ANTI_REORG_DELAY blocks. + mine_transaction(&nodes[0], &bs_htlc_claim_txn[0]); + expect_payment_sent(&nodes[0], payment_preimage_2, None, true, false); + assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations { + amount_satoshis: 1_000_000 - 10_000 - 20_000 - commitment_tx_fee - anchor_outputs_value, + confirmation_height: node_a_commitment_claimable, + source: BalanceSource::HolderForceClosed, + }, htlc_balance_known_preimage.clone(), htlc_balance_unknown_preimage.clone()]), + sorted_vec(nodes[0].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); + + if anchors { + // The HTLC timeout claim corresponding to the counterparty preimage claim is removed from the + // aggregated package. + handle_bump_htlc_event(&nodes[0], 1); + timeout_htlc_txn = nodes[0].tx_broadcaster.unique_txn_broadcast(); + assert_eq!(timeout_htlc_txn.len(), 1); + check_spends!(timeout_htlc_txn[0], commitment_tx, coinbase_tx); + // One input from the commitment transaction for the HTLC, and one input to provide fees. + assert_eq!(timeout_htlc_txn[0].input.len(), 2); + assert_eq!(timeout_htlc_txn[0].input[0].witness.last().unwrap().len(), chan_utils::OFFERED_HTLC_SCRIPT_WEIGHT_ANCHORS); } - let timeout_htlc_txn = nodes[0].tx_broadcaster.unique_txn_broadcast(); - assert_eq!(timeout_htlc_txn.len(), 2); - check_spends!(timeout_htlc_txn[0], commitment_tx, coinbase_tx); - check_spends!(timeout_htlc_txn[1], commitment_tx, coinbase_tx); // Now confirm nodes[0]'s HTLC-Timeout transaction, which changes the claimable balance to an // "awaiting confirmations" one. @@ -918,21 +962,6 @@ fn do_test_balances_on_local_commitment_htlcs(anchors: bool) { }, htlc_balance_unknown_preimage.clone()]), sorted_vec(nodes[0].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); - // Now confirm nodes[1]'s HTLC claim, giving nodes[0] the preimage. Note that the "maybe - // claimable" balance remains until we see ANTI_REORG_DELAY blocks. - mine_transaction(&nodes[0], &bs_htlc_claim_txn[0]); - expect_payment_sent(&nodes[0], payment_preimage_2, None, true, false); - assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations { - amount_satoshis: 1_000_000 - 10_000 - 20_000 - commitment_tx_fee - anchor_outputs_value, - confirmation_height: node_a_commitment_claimable, - source: BalanceSource::HolderForceClosed, - }, Balance::ClaimableAwaitingConfirmations { - amount_satoshis: 10_000, - confirmation_height: node_a_htlc_claimable, - source: BalanceSource::Htlc, - }, htlc_balance_unknown_preimage.clone()]), - sorted_vec(nodes[0].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); - // Finally make the HTLC transactions have ANTI_REORG_DELAY blocks. This call previously // panicked as described in the test introduction. This will remove the "maybe claimable" // spendable output as nodes[1] has fully claimed the second HTLC. @@ -1234,15 +1263,6 @@ fn test_no_preimage_inbound_htlc_balances() { assert!(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances().is_empty()); } -fn sorted_vec_with_additions(v_orig: &Vec, extra_ts: &[&T]) -> Vec { - let mut v = v_orig.clone(); - for t in extra_ts { - v.push((*t).clone()); - } - v.sort_unstable(); - v -} - fn do_test_revoked_counterparty_commitment_balances(anchors: bool, confirm_htlc_spend_first: bool) { // Tests `get_claimable_balances` for revoked counterparty commitment transactions. let mut chanmon_cfgs = create_chanmon_cfgs(2); @@ -1341,152 +1361,149 @@ fn do_test_revoked_counterparty_commitment_balances(anchors: bool, confirm_htlc_ // Prior to channel closure, B considers the preimage HTLC as its own, and otherwise only // lists the two on-chain timeout-able HTLCs as claimable balances. - assert_eq!(sorted_vec(vec![Balance::ClaimableOnChannelClose { - amount_satoshis: 100_000 - 5_000 - 4_000 - 3 - 2_000 + 3_000 - 1 /* rounded up msat parts of HTLCs */, - transaction_fee_satoshis: 0, - outbound_payment_htlc_rounded_msat: 3200, - outbound_forwarded_htlc_rounded_msat: 0, - inbound_claiming_htlc_rounded_msat: 100, - inbound_htlc_rounded_msat: 0, - }, Balance::MaybeTimeoutClaimableHTLC { - amount_satoshis: 2_000, - claimable_height: missing_htlc_cltv_timeout, - payment_hash: missing_htlc_payment_hash, - outbound_payment: true, - }, Balance::MaybeTimeoutClaimableHTLC { - amount_satoshis: 4_000, - claimable_height: htlc_cltv_timeout, - payment_hash: timeout_payment_hash, - outbound_payment: true, - }, Balance::MaybeTimeoutClaimableHTLC { - amount_satoshis: 5_000, - claimable_height: live_htlc_cltv_timeout, - payment_hash: live_payment_hash, - outbound_payment: true, - }]), - sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); + assert_eq!( + sorted_vec(vec![ + Balance::ClaimableOnChannelClose { + amount_satoshis: 100_000 - 5_000 - 4_000 - 3 - 2_000 + 3_000 - 1 /* rounded up msat parts of HTLCs */, + transaction_fee_satoshis: 0, + outbound_payment_htlc_rounded_msat: 3200, + outbound_forwarded_htlc_rounded_msat: 0, + inbound_claiming_htlc_rounded_msat: 100, + inbound_htlc_rounded_msat: 0, + }, Balance::MaybeTimeoutClaimableHTLC { + amount_satoshis: 2_000, + claimable_height: missing_htlc_cltv_timeout, + payment_hash: missing_htlc_payment_hash, + outbound_payment: true, + }, Balance::MaybeTimeoutClaimableHTLC { + amount_satoshis: 4_000, + claimable_height: htlc_cltv_timeout, + payment_hash: timeout_payment_hash, + outbound_payment: true, + }, Balance::MaybeTimeoutClaimableHTLC { + amount_satoshis: 5_000, + claimable_height: live_htlc_cltv_timeout, + payment_hash: live_payment_hash, + outbound_payment: true, + }, + ]), + sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()), + ); mine_transaction(&nodes[1], &as_revoked_txn[0]); let mut claim_txn: Vec<_> = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().drain(..).filter(|tx| tx.input.iter().any(|inp| inp.previous_output.txid == as_revoked_txn[0].compute_txid())).collect(); - // Currently the revoked commitment is claimed in four transactions as the HTLCs all expire - // quite soon. - assert_eq!(claim_txn.len(), 4); + // Currently, the revoked commitment is claimed in two batches based on pinnability. + assert_eq!(claim_txn.len(), 2); claim_txn.sort_unstable_by_key(|tx| tx.output.iter().map(|output| output.value.to_sat()).sum::()); // The following constants were determined experimentally - const BS_TO_SELF_CLAIM_EXP_WEIGHT: u64 = 483; - let outbound_htlc_claim_exp_weight: u64 = if anchors { 574 } else { 571 }; - let inbound_htlc_claim_exp_weight: u64 = if anchors { 582 } else { 578 }; + let anchor_outputs_value = if anchors { channel::ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 }; + let commitment_tx_fee = chan_feerate * + (chan_utils::commitment_tx_base_weight(&channel_type_features) + 3 * chan_utils::COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000; + let pinnable_weight = if anchors { 1398 } else { 1389 }; + let unpinnable_weight = 483; // Check that the weight is close to the expected weight. Note that signature sizes vary // somewhat so it may not always be exact. - fuzzy_assert_eq(claim_txn[0].weight().to_wu(), outbound_htlc_claim_exp_weight); - fuzzy_assert_eq(claim_txn[1].weight().to_wu(), inbound_htlc_claim_exp_weight); - fuzzy_assert_eq(claim_txn[2].weight().to_wu(), inbound_htlc_claim_exp_weight); - fuzzy_assert_eq(claim_txn[3].weight().to_wu(), BS_TO_SELF_CLAIM_EXP_WEIGHT); - - let commitment_tx_fee = chan_feerate * - (chan_utils::commitment_tx_base_weight(&channel_type_features) + 3 * chan_utils::COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000; - let anchor_outputs_value = if anchors { channel::ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 }; - let inbound_htlc_claim_fee = chan_feerate * inbound_htlc_claim_exp_weight / 1000; - let outbound_htlc_claim_fee = chan_feerate * outbound_htlc_claim_exp_weight / 1000; - let to_self_claim_fee = chan_feerate * claim_txn[3].weight().to_wu() / 1000; - - // The expected balance for the next three checks, with the largest-HTLC and to_self output - // claim balances separated out. - let expected_balance_b = vec![Balance::ClaimableAwaitingConfirmations { - // to_remote output in A's revoked commitment - amount_satoshis: 100_000 - 5_000 - 4_000 - 3 - 1 /* rounded up msat parts of HTLCs */, - confirmation_height: nodes[1].best_block_info().1 + 5, - source: BalanceSource::CounterpartyForceClosed, - }, Balance::CounterpartyRevokedOutputClaimable { - amount_satoshis: 3_000, - }, Balance::CounterpartyRevokedOutputClaimable { - amount_satoshis: 4_000, - }]; - + fuzzy_assert_eq(claim_txn[0].weight().to_wu(), pinnable_weight); + fuzzy_assert_eq(claim_txn[1].weight().to_wu(), unpinnable_weight); + let pinnable_fee = claim_txn[0].input.iter().map(|txin| { + assert_eq!(txin.previous_output.txid, as_revoked_txn[0].compute_txid()); + as_revoked_txn[0].output[txin.previous_output.vout as usize].value.to_sat() + }).sum::() - claim_txn[0].output.iter().map(|txout| txout.value.to_sat()).sum::(); + let unpinnable_fee = claim_txn[1].input.iter().map(|txin| { + assert_eq!(txin.previous_output.txid, as_revoked_txn[0].compute_txid()); + as_revoked_txn[0].output[txin.previous_output.vout as usize].value.to_sat() + }).sum::() - claim_txn[1].output.iter().map(|txout| txout.value.to_sat()).sum::(); + + // The expected balances for the next three checks. + let to_remote_balance = Balance::ClaimableAwaitingConfirmations { + // to_remote output in A's revoked commitment + amount_satoshis: 100_000 - 5_000 - 4_000 - 3 - 1 /* rounded up msat parts of HTLCs */, + confirmation_height: nodes[1].best_block_info().1 + 5, + source: BalanceSource::CounterpartyForceClosed, + }; + let htlc_unclaimed_balance = |amount: u64| Balance::CounterpartyRevokedOutputClaimable { + amount_satoshis: amount, + }; let to_self_unclaimed_balance = Balance::CounterpartyRevokedOutputClaimable { amount_satoshis: 1_000_000 - 100_000 - 3_000 - commitment_tx_fee - anchor_outputs_value - 1 /* rounded up msat parts of HTLCs */, }; let to_self_claimed_avail_height; - let largest_htlc_unclaimed_balance = Balance::CounterpartyRevokedOutputClaimable { - amount_satoshis: 5_000, - }; let largest_htlc_claimed_avail_height; // Once the channel has been closed by A, B now considers all of the commitment transactions' // outputs as `CounterpartyRevokedOutputClaimable`. - assert_eq!(sorted_vec_with_additions(&expected_balance_b, &[&to_self_unclaimed_balance, &largest_htlc_unclaimed_balance]), - sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); + assert_eq!( + sorted_vec(vec![ + to_remote_balance.clone(), + to_self_unclaimed_balance.clone(), + htlc_unclaimed_balance(3_000), + htlc_unclaimed_balance(4_000), + htlc_unclaimed_balance(5_000), + ]), + sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()), + ); if confirm_htlc_spend_first { - mine_transaction(&nodes[1], &claim_txn[2]); + mine_transaction(&nodes[1], &claim_txn[0]); largest_htlc_claimed_avail_height = nodes[1].best_block_info().1 + 5; to_self_claimed_avail_height = nodes[1].best_block_info().1 + 6; // will be claimed in the next block } else { // Connect the to_self output claim, taking all of A's non-HTLC funds - mine_transaction(&nodes[1], &claim_txn[3]); + mine_transaction(&nodes[1], &claim_txn[1]); to_self_claimed_avail_height = nodes[1].best_block_info().1 + 5; largest_htlc_claimed_avail_height = nodes[1].best_block_info().1 + 6; // will be claimed in the next block } - let largest_htlc_claimed_balance = Balance::ClaimableAwaitingConfirmations { - amount_satoshis: 5_000 - inbound_htlc_claim_fee, + let pinnable_claimed_balance = Balance::ClaimableAwaitingConfirmations { + amount_satoshis: 5_000 + 3_000 + 4_000 - pinnable_fee, confirmation_height: largest_htlc_claimed_avail_height, source: BalanceSource::CounterpartyForceClosed, }; - let to_self_claimed_balance = Balance::ClaimableAwaitingConfirmations { - amount_satoshis: 1_000_000 - 100_000 - 3_000 - commitment_tx_fee - anchor_outputs_value - to_self_claim_fee - 1 /* rounded up msat parts of HTLCs */, + let unpinnable_claimed_balance = Balance::ClaimableAwaitingConfirmations { + amount_satoshis: 1_000_000 - 100_000 - 3_000 - commitment_tx_fee - anchor_outputs_value - unpinnable_fee - 1 /* rounded up msat parts of HTLCs */, confirmation_height: to_self_claimed_avail_height, source: BalanceSource::CounterpartyForceClosed, }; if confirm_htlc_spend_first { - assert_eq!(sorted_vec_with_additions(&expected_balance_b, &[&to_self_unclaimed_balance, &largest_htlc_claimed_balance]), - sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); + assert_eq!( + sorted_vec(vec![ + to_remote_balance.clone(), + pinnable_claimed_balance.clone(), + to_self_unclaimed_balance.clone(), + ]), + sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()), + ); } else { - assert_eq!(sorted_vec_with_additions(&expected_balance_b, &[&to_self_claimed_balance, &largest_htlc_unclaimed_balance]), - sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); + assert_eq!( + sorted_vec(vec![ + to_remote_balance.clone(), + unpinnable_claimed_balance.clone(), + htlc_unclaimed_balance(3_000), + htlc_unclaimed_balance(4_000), + htlc_unclaimed_balance(5_000), + ]), + sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()), + ); } if confirm_htlc_spend_first { - mine_transaction(&nodes[1], &claim_txn[3]); + mine_transaction(&nodes[1], &claim_txn[1]); } else { - mine_transaction(&nodes[1], &claim_txn[2]); + mine_transaction(&nodes[1], &claim_txn[0]); } - assert_eq!(sorted_vec_with_additions(&expected_balance_b, &[&to_self_claimed_balance, &largest_htlc_claimed_balance]), - sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); - - // Finally, connect the last two remaining HTLC spends and check that they move to - // `ClaimableAwaitingConfirmations` - mine_transaction(&nodes[1], &claim_txn[0]); - mine_transaction(&nodes[1], &claim_txn[1]); - - assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations { - // to_remote output in A's revoked commitment - amount_satoshis: 100_000 - 5_000 - 4_000 - 3 - 1 /* rounded up msat parts of HTLCs */, - confirmation_height: nodes[1].best_block_info().1 + 1, - source: BalanceSource::CounterpartyForceClosed, - }, Balance::ClaimableAwaitingConfirmations { - amount_satoshis: 1_000_000 - 100_000 - 3_000 - commitment_tx_fee - anchor_outputs_value - to_self_claim_fee - 1 /* rounded up msat parts of HTLCs */, - confirmation_height: to_self_claimed_avail_height, - source: BalanceSource::CounterpartyForceClosed, - }, Balance::ClaimableAwaitingConfirmations { - amount_satoshis: 3_000 - outbound_htlc_claim_fee, - confirmation_height: nodes[1].best_block_info().1 + 4, - source: BalanceSource::CounterpartyForceClosed, - }, Balance::ClaimableAwaitingConfirmations { - amount_satoshis: 4_000 - inbound_htlc_claim_fee, - confirmation_height: nodes[1].best_block_info().1 + 5, - source: BalanceSource::CounterpartyForceClosed, - }, Balance::ClaimableAwaitingConfirmations { - amount_satoshis: 5_000 - inbound_htlc_claim_fee, - confirmation_height: largest_htlc_claimed_avail_height, - source: BalanceSource::CounterpartyForceClosed, - }]), - sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); + assert_eq!( + sorted_vec(vec![ + to_remote_balance.clone(), + pinnable_claimed_balance.clone(), + unpinnable_claimed_balance.clone(), + ]), + sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()), + ); - connect_blocks(&nodes[1], 1); + connect_blocks(&nodes[1], 3); test_spendable_output(&nodes[1], &as_revoked_txn[0], false); let mut payment_failed_events = nodes[1].node.get_and_clear_pending_events(); @@ -1496,15 +1513,27 @@ fn do_test_revoked_counterparty_commitment_balances(anchors: bool, confirm_htlc_ dust_payment_hash, false, PaymentFailedConditions::new()); connect_blocks(&nodes[1], 1); - test_spendable_output(&nodes[1], &claim_txn[if confirm_htlc_spend_first { 2 } else { 3 }], false); - connect_blocks(&nodes[1], 1); - test_spendable_output(&nodes[1], &claim_txn[if confirm_htlc_spend_first { 3 } else { 2 }], false); - expect_payment_failed!(nodes[1], live_payment_hash, false); - connect_blocks(&nodes[1], 1); - test_spendable_output(&nodes[1], &claim_txn[0], false); + if confirm_htlc_spend_first { + test_spendable_output(&nodes[1], &claim_txn[0], false); + let mut payment_failed_events = nodes[1].node.get_and_clear_pending_events(); + expect_payment_failed_conditions_event(payment_failed_events[..2].to_vec(), + live_payment_hash, false, PaymentFailedConditions::new()); + expect_payment_failed_conditions_event(payment_failed_events[2..].to_vec(), + timeout_payment_hash, false, PaymentFailedConditions::new()); + } else { + test_spendable_output(&nodes[1], &claim_txn[1], false); + } connect_blocks(&nodes[1], 1); - test_spendable_output(&nodes[1], &claim_txn[1], false); - expect_payment_failed!(nodes[1], timeout_payment_hash, false); + if confirm_htlc_spend_first { + test_spendable_output(&nodes[1], &claim_txn[1], false); + } else { + test_spendable_output(&nodes[1], &claim_txn[0], false); + let mut payment_failed_events = nodes[1].node.get_and_clear_pending_events(); + expect_payment_failed_conditions_event(payment_failed_events[..2].to_vec(), + live_payment_hash, false, PaymentFailedConditions::new()); + expect_payment_failed_conditions_event(payment_failed_events[2..].to_vec(), + timeout_payment_hash, false, PaymentFailedConditions::new()); + } assert_eq!(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances(), Vec::new()); // Ensure that even if we connect more blocks, potentially replaying the entire chain if we're @@ -1630,24 +1659,17 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) { let revoked_to_self_claim = { let mut as_commitment_claim_txn = nodes[0].tx_broadcaster.txn_broadcast(); - assert_eq!(as_commitment_claim_txn.len(), if anchors { 2 } else { 1 }); - if anchors { - assert_eq!(as_commitment_claim_txn[0].input.len(), 1); - assert_eq!(as_commitment_claim_txn[0].input[0].previous_output.vout, 4); // Separate to_remote claim - check_spends!(as_commitment_claim_txn[0], revoked_local_txn[0]); - assert_eq!(as_commitment_claim_txn[1].input.len(), 2); - assert_eq!(as_commitment_claim_txn[1].input[0].previous_output.vout, 2); - assert_eq!(as_commitment_claim_txn[1].input[1].previous_output.vout, 3); - check_spends!(as_commitment_claim_txn[1], revoked_local_txn[0]); - Some(as_commitment_claim_txn.remove(0)) - } else { - assert_eq!(as_commitment_claim_txn[0].input.len(), 3); - assert_eq!(as_commitment_claim_txn[0].input[0].previous_output.vout, 2); - assert_eq!(as_commitment_claim_txn[0].input[1].previous_output.vout, 0); - assert_eq!(as_commitment_claim_txn[0].input[2].previous_output.vout, 1); - check_spends!(as_commitment_claim_txn[0], revoked_local_txn[0]); - None - } + assert_eq!(as_commitment_claim_txn.len(), 2); + // One unpinnable revoked to_self output. + assert_eq!(as_commitment_claim_txn[0].input.len(), 1); + // Two pinnable revoked HTLC outputs. + assert_eq!(as_commitment_claim_txn[1].input.len(), 2); + check_spends!(as_commitment_claim_txn[0], revoked_local_txn[0]); + check_spends!(as_commitment_claim_txn[1], revoked_local_txn[0]); + assert_ne!(as_commitment_claim_txn[0].input[0].previous_output, as_commitment_claim_txn[1].input[0].previous_output); + assert_ne!(as_commitment_claim_txn[0].input[0].previous_output, as_commitment_claim_txn[1].input[1].previous_output); + assert_ne!(as_commitment_claim_txn[1].input[0].previous_output, as_commitment_claim_txn[1].input[1].previous_output); + as_commitment_claim_txn.remove(0) }; // The next two checks have the same balance set for A - even though we confirm a revoked HTLC @@ -1678,12 +1700,8 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) { assert_eq!(as_htlc_claim_tx[0].input.len(), 1); check_spends!(as_htlc_claim_tx[0], revoked_htlc_success); // A has to generate a new claim for the remaining revoked outputs (which no longer includes the - // spent HTLC output) - assert_eq!(as_htlc_claim_tx[1].input.len(), if anchors { 1 } else { 2 }); - assert_eq!(as_htlc_claim_tx[1].input[0].previous_output.vout, 2); - if !anchors { - assert_eq!(as_htlc_claim_tx[1].input[1].previous_output.vout, 0); - } + // spent HTLC output). + assert_eq!(as_htlc_claim_tx[1].input.len(), 1); check_spends!(as_htlc_claim_tx[1], revoked_local_txn[0]); assert_eq!(as_balances, @@ -1761,23 +1779,12 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) { } mine_transaction(&nodes[0], &revoked_htlc_timeout); - let (revoked_htlc_timeout_claim, revoked_to_self_claim) = { + let revoked_htlc_timeout_claim = { let mut as_second_htlc_claim_tx = nodes[0].tx_broadcaster.txn_broadcast(); - assert_eq!(as_second_htlc_claim_tx.len(), if anchors { 1 } else { 2 }); - if anchors { - assert_eq!(as_second_htlc_claim_tx[0].input.len(), 1); - assert_eq!(as_second_htlc_claim_tx[0].input[0].previous_output.vout, 0); - check_spends!(as_second_htlc_claim_tx[0], revoked_htlc_timeout); - (as_second_htlc_claim_tx.remove(0), revoked_to_self_claim.unwrap()) - } else { - assert_eq!(as_second_htlc_claim_tx[0].input.len(), 1); - assert_eq!(as_second_htlc_claim_tx[0].input[0].previous_output.vout, 0); - check_spends!(as_second_htlc_claim_tx[0], revoked_htlc_timeout); - assert_eq!(as_second_htlc_claim_tx[1].input.len(), 1); - assert_eq!(as_second_htlc_claim_tx[1].input[0].previous_output.vout, 2); - check_spends!(as_second_htlc_claim_tx[1], revoked_local_txn[0]); - (as_second_htlc_claim_tx.remove(0), as_second_htlc_claim_tx.remove(0)) - } + assert_eq!(as_second_htlc_claim_tx.len(), 1); + assert_eq!(as_second_htlc_claim_tx[0].input.len(), 1); + check_spends!(as_second_htlc_claim_tx[0], revoked_htlc_timeout); + as_second_htlc_claim_tx.remove(0) }; // Connect blocks to finalize the HTLC resolution with the HTLC-Timeout transaction. In a @@ -1942,24 +1949,17 @@ fn do_test_revoked_counterparty_aggregated_claims(anchors: bool) { check_added_monitors!(nodes[1], 1); let mut claim_txn = nodes[1].tx_broadcaster.txn_broadcast(); - assert_eq!(claim_txn.len(), if anchors { 2 } else { 1 }); - let revoked_to_self_claim = if anchors { - assert_eq!(claim_txn[0].input.len(), 1); - assert_eq!(claim_txn[0].input[0].previous_output.vout, 5); // Separate to_remote claim - check_spends!(claim_txn[0], as_revoked_txn[0]); - assert_eq!(claim_txn[1].input.len(), 2); - assert_eq!(claim_txn[1].input[0].previous_output.vout, 2); - assert_eq!(claim_txn[1].input[1].previous_output.vout, 3); - check_spends!(claim_txn[1], as_revoked_txn[0]); - Some(claim_txn.remove(0)) - } else { - assert_eq!(claim_txn[0].input.len(), 3); - assert_eq!(claim_txn[0].input[0].previous_output.vout, 3); - assert_eq!(claim_txn[0].input[1].previous_output.vout, 0); - assert_eq!(claim_txn[0].input[2].previous_output.vout, 1); - check_spends!(claim_txn[0], as_revoked_txn[0]); - None - }; + assert_eq!(claim_txn.len(), 2); + // One unpinnable revoked to_self output. + assert_eq!(claim_txn[0].input.len(), 1); + // Two pinnable revoked HTLC outputs. + assert_eq!(claim_txn[1].input.len(), 2); + check_spends!(claim_txn[0], as_revoked_txn[0]); + check_spends!(claim_txn[1], as_revoked_txn[0]); + assert_ne!(claim_txn[0].input[0].previous_output, claim_txn[1].input[0].previous_output); + assert_ne!(claim_txn[0].input[0].previous_output, claim_txn[1].input[1].previous_output); + assert_ne!(claim_txn[1].input[0].previous_output, claim_txn[1].input[1].previous_output); + let to_remote_maturity = nodes[1].best_block_info().1 + ANTI_REORG_DELAY - 1; @@ -2010,15 +2010,12 @@ fn do_test_revoked_counterparty_aggregated_claims(anchors: bool) { assert_eq!(claim_txn_2[0].input[0].previous_output.vout, 0); check_spends!(claim_txn_2[0], &htlc_success_claim); assert_eq!(claim_txn_2[1].input.len(), 1); - assert_eq!(claim_txn_2[1].input[0].previous_output.vout, 3); check_spends!(claim_txn_2[1], as_revoked_txn[0]); } else { assert_eq!(claim_txn_2[0].input.len(), 1); assert_eq!(claim_txn_2[0].input[0].previous_output.vout, 0); check_spends!(claim_txn_2[0], as_revoked_txn[1]); - assert_eq!(claim_txn_2[1].input.len(), 2); - assert_eq!(claim_txn_2[1].input[0].previous_output.vout, 3); - assert_eq!(claim_txn_2[1].input[1].previous_output.vout, 1); + assert_eq!(claim_txn_2[1].input.len(), 1); check_spends!(claim_txn_2[1], as_revoked_txn[0]); } @@ -2082,52 +2079,40 @@ fn do_test_revoked_counterparty_aggregated_claims(anchors: bool) { }]), sorted_vec(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances())); - if anchors { - mine_transactions(&nodes[1], &[&claim_txn_2[1], revoked_to_self_claim.as_ref().unwrap()]); - } else { - mine_transaction(&nodes[1], &claim_txn_2[1]); - } + mine_transactions(&nodes[1], &[&claim_txn[0], &claim_txn_2[1]]); let rest_claim_maturity = nodes[1].best_block_info().1 + ANTI_REORG_DELAY - 1; - if anchors { - assert_eq!(vec![Balance::ClaimableAwaitingConfirmations { - amount_satoshis: claim_txn_2[1].output[0].value.to_sat(), - confirmation_height: rest_claim_maturity, - source: BalanceSource::CounterpartyForceClosed, - }, Balance::ClaimableAwaitingConfirmations { - amount_satoshis: revoked_to_self_claim.as_ref().unwrap().output[0].value.to_sat(), + assert_eq!( + vec![ + Balance::ClaimableAwaitingConfirmations { + amount_satoshis: claim_txn[0].output[0].value.to_sat(), confirmation_height: rest_claim_maturity, source: BalanceSource::CounterpartyForceClosed, - }], - nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()); - } else { - assert_eq!(vec![Balance::ClaimableAwaitingConfirmations { + }, + Balance::ClaimableAwaitingConfirmations { amount_satoshis: claim_txn_2[1].output[0].value.to_sat(), confirmation_height: rest_claim_maturity, source: BalanceSource::CounterpartyForceClosed, - }], - nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()); - } + }, + ], + nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances()); assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); // We shouldn't fail the payment until we spend the output connect_blocks(&nodes[1], 5); expect_payment_failed!(nodes[1], revoked_payment_hash, false); - if anchors { - let events = nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events(); - assert_eq!(events.len(), 2); - for (i, event) in events.into_iter().enumerate() { - if let Event::SpendableOutputs { outputs, .. } = event { - assert_eq!(outputs.len(), 1); - let spend_tx = nodes[1].keys_manager.backing.spend_spendable_outputs( - &[&outputs[0]], Vec::new(), Builder::new().push_opcode(opcodes::all::OP_RETURN).into_script(), - 253, None, &Secp256k1::new() - ).unwrap(); - check_spends!(spend_tx, if i == 0 { &claim_txn_2[1] } else { revoked_to_self_claim.as_ref().unwrap() }); - } else { panic!(); } + let spendable_output_events = nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events(); + assert_eq!(spendable_output_events.len(), 2); + for event in spendable_output_events { + if let Event::SpendableOutputs { outputs, channel_id: _ } = event { + assert_eq!(outputs.len(), 1); + let spend_tx = nodes[1].keys_manager.backing.spend_spendable_outputs( + &[&outputs[0]], Vec::new(), ScriptBuf::new_op_return(&[]), 253, None, &Secp256k1::new(), + ).unwrap(); + check_spends!(spend_tx, &claim_txn[0], &claim_txn_2[1]); + } else { + panic!("unexpected event"); } - } else { - test_spendable_output(&nodes[1], &claim_txn_2[1], false); } assert!(nodes[1].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances().is_empty()); @@ -2580,21 +2565,18 @@ fn do_test_yield_anchors_events(have_htlcs: bool) { { let mut txn = nodes[1].tx_broadcaster.unique_txn_broadcast(); - assert_eq!(txn.len(), if nodes[1].connect_style.borrow().updates_best_block_first() { 3 } else { 2 }); - - let htlc_preimage_tx = txn.pop().unwrap(); - assert_eq!(htlc_preimage_tx.input.len(), 1); - assert_eq!(htlc_preimage_tx.input[0].previous_output.vout, 3); - check_spends!(htlc_preimage_tx, commitment_tx); - - let htlc_timeout_tx = txn.pop().unwrap(); - assert_eq!(htlc_timeout_tx.input.len(), 1); - assert_eq!(htlc_timeout_tx.input[0].previous_output.vout, 2); - check_spends!(htlc_timeout_tx, commitment_tx); - - if let Some(new_commitment_tx) = txn.pop() { + // Both HTLC claims are pinnable at this point, + // and will be broadcast in a single transaction. + assert_eq!(txn.len(), if nodes[1].connect_style.borrow().updates_best_block_first() { 2 } else { 1 }); + if nodes[1].connect_style.borrow().updates_best_block_first() { + let new_commitment_tx = txn.remove(0); check_spends!(new_commitment_tx, funding_tx); } + let htlc_claim_tx = txn.pop().unwrap(); + assert_eq!(htlc_claim_tx.input.len(), 2); + assert_eq!(htlc_claim_tx.input[0].previous_output.vout, 2); + assert_eq!(htlc_claim_tx.input[1].previous_output.vout, 3); + check_spends!(htlc_claim_tx, commitment_tx); } let mut holder_events = nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events(); @@ -2765,23 +2747,31 @@ fn test_anchors_aggregated_revoked_htlc_tx() { check_closed_event!(&nodes[0], 2, ClosureReason::CommitmentTxConfirmed, [nodes[1].node.get_our_node_id(); 2], 1000000); // Alice should detect the confirmed revoked commitments, and attempt to claim all of the - // revoked outputs. + // revoked outputs in aggregated transactions per channel, grouped into pinnable and unpinnable + // clusters: the unpinnable, revoked to_self outputs and the pinnable, revoked HTLC outputs. { let txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); assert_eq!(txn.len(), 4); - let (revoked_htlc_claim_a, revoked_htlc_claim_b) = if txn[0].input[0].previous_output.txid == revoked_commitment_txs[0].compute_txid() { - (if txn[0].input.len() == 2 { &txn[0] } else { &txn[1] }, if txn[2].input.len() == 2 { &txn[2] } else { &txn[3] }) - } else { - (if txn[2].input.len() == 2 { &txn[2] } else { &txn[3] }, if txn[0].input.len() == 2 { &txn[0] } else { &txn[1] }) - }; - - assert_eq!(revoked_htlc_claim_a.input.len(), 2); // Spends both HTLC outputs - assert_eq!(revoked_htlc_claim_a.output.len(), 1); - check_spends!(revoked_htlc_claim_a, revoked_commitment_txs[0]); - assert_eq!(revoked_htlc_claim_b.input.len(), 2); // Spends both HTLC outputs - assert_eq!(revoked_htlc_claim_b.output.len(), 1); - check_spends!(revoked_htlc_claim_b, revoked_commitment_txs[1]); + let revoked_claims_a: Vec<_> = txn.clone().into_iter().filter(|tx| { + tx.input[0].previous_output.txid == revoked_commitment_txs[0].compute_txid() + }).collect(); + let revoked_claims_b: Vec<_> = txn.clone().into_iter().filter(|tx| { + tx.input[0].previous_output.txid == revoked_commitment_txs[1].compute_txid() + }).collect(); + + assert_eq!(revoked_claims_a.len(), 2); + assert_eq!(revoked_claims_a.iter().map(|tx| tx.input.len()).sum::(), 3); + for tx in &revoked_claims_a { + check_spends!(tx, revoked_commitment_txs[0]); + assert_eq!(tx.output.len(), 1); + } + assert_eq!(revoked_claims_b.len(), 2); + assert_eq!(revoked_claims_b.iter().map(|tx| tx.input.len()).sum::(), 3); + for tx in &revoked_claims_b { + check_spends!(tx, revoked_commitment_txs[1]); + assert_eq!(tx.output.len(), 1); + } } // Since Bob was able to confirm his revoked commitment, he'll now try to claim the HTLCs @@ -2873,6 +2863,7 @@ fn test_anchors_aggregated_revoked_htlc_tx() { // the second level instead. let revoked_claim_transactions = { let txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + // Each channel has 1 adjusted claim of the HTLC revocations. assert_eq!(txn.len(), 2); let revoked_htlc_claims = txn.iter().filter(|tx| @@ -2910,6 +2901,9 @@ fn test_anchors_aggregated_revoked_htlc_tx() { assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty()); let spendable_output_events = nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events(); + // The spendable outputs for each channel consist of: + // - 1 aggregated claim of the HTLC revocations. + // - 1 static to_remote output. assert_eq!(spendable_output_events.len(), 4); for event in spendable_output_events { if let Event::SpendableOutputs { outputs, channel_id } = event {