From 0fe90c6f7c3325935b18dbc809be14afd8fe067f Mon Sep 17 00:00:00 2001 From: Willem Van Lint Date: Fri, 20 Sep 2024 15:01:36 -0700 Subject: [PATCH] Batch on-chain claims more aggressively per channel When batch claiming was first added, it was only done so for claims which were not pinnable, i.e. those which can only be claimed by us. This was the conservative choice - pinning of outputs claimed by a batch would leave the entire batch unable to confirm on-chain. However, if pinning is considered an attack that can be executed with a high probability of success, then there is no reason not to batch claims of pinnable outputs together, separate from unpinnable outputs. Whether specific outputs are pinnable can change over time - those that are not pinnable will eventually become pinnable at the height at which our counterparty can spend them. Outputs are treated as pinnable if they're within `COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE` of that height. Aside from outputs being pinnable or not, locktimes are also a factor for batching claims. HTLC-timeout claims have locktimes fixed by the counterparty's signature and thus can only be aggregated with other HTLCs of the same CLTV, which we have to check for. The complexity required here is worth it - aggregation can save users a significant amount of fees in the case of a force-closure, and directly impacts the number of UTXOs needed as a reserve for anchors. Co-authored-by: Matt Corallo --- lightning/src/chain/channelmonitor.rs | 13 +- lightning/src/chain/onchaintx.rs | 4 +- lightning/src/chain/package.rs | 523 ++++++++++++++++-------- lightning/src/ln/functional_tests.rs | 363 +++++++---------- lightning/src/ln/monitor_tests.rs | 550 +++++++++++++------------- 5 files changed, 774 insertions(+), 679 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 4f279cf65..7e29565de 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 0b9f97107..96e5c8329 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -772,7 +772,7 @@ impl OnchainTxHandler { 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) { + 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 { @@ -1106,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) { - assert!(request.merge_package(package).is_ok()); + 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 bd0bf66a0..53bba3a75 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -30,7 +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::CLTV_SHARED_CLAIM_BUFFER; +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; @@ -40,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)] @@ -544,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::RevokedHTLCOutput(..) => { - match input { - PackageSolvingData::RevokedOutput(..) => { true }, - PackageSolvingData::RevokedHTLCOutput(..) => { true }, - _ => { false } + PackageSolvingData::CounterpartyOfferedHTLCOutput(_)| + PackageSolvingData::CounterpartyReceivedHTLCOutput(_) => { + match other { + PackageSolvingData::CounterpartyOfferedHTLCOutput(_)| + PackageSolvingData::CounterpartyReceivedHTLCOutput(_) => 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, @@ -678,22 +690,31 @@ impl PackageSolvingData { } } - 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) + 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, + } } } @@ -706,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, } @@ -741,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. @@ -762,13 +787,73 @@ pub struct PackageTemplate { impl PackageTemplate { pub(crate) fn can_merge_with(&self, other: &PackageTemplate, cur_height: u32) -> bool { - self.aggregable() && other.aggregable() && - self.package_locktime(cur_height) == other.package_locktime(cur_height) && - self.counterparty_spendable_height() > cur_height + CLTV_SHARED_CLAIM_BUFFER && - other.counterparty_spendable_height() > cur_height + CLTV_SHARED_CLAIM_BUFFER + 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. /// @@ -777,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 } @@ -800,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, }); @@ -831,18 +911,10 @@ impl PackageTemplate { } } } - pub(crate) fn merge_package(&mut self, mut merge_from: PackageTemplate) -> Result<(), PackageTemplate> { - if self.malleability == PackageMalleability::Untractable || merge_from.malleability == PackageMalleability::Untractable { + 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 !self.aggregable || !merge_from.aggregable { - 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) { return Err(merge_from); } - } - } else { return Err(merge_from); } for (k, v) in merge_from.inputs.drain(..) { self.inputs.push((k, v)); } @@ -1038,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 @@ -1100,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, } @@ -1140,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; @@ -1157,7 +1229,6 @@ impl Readable for PackageTemplate { inputs, malleability, counterparty_spendable_height, - aggregable, feerate_previous, height_timer: height_timer.unwrap_or(0), }) @@ -1260,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; @@ -1277,145 +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] - 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); - assert!(untractable_package.merge_package(malleable_package).is_err()); + macro_rules! dumb_funding_output { + () => { + PackageSolvingData::HolderFundingOutput(HolderFundingOutput::build(ScriptBuf::new(), 0, ChannelTypeFeatures::only_static_remote_key())) + } } #[test] - 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 malleable_package = PackageTemplate::build_package(txid, 0, htlc_outp.clone(), 1000); - let untractable_package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000); - assert!(malleable_package.merge_package(untractable_package).is_err()); + 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); + + 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] - 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); - assert!(noaggregation_package.merge_package(aggregation_package).is_err()); - } - - #[test] - 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); - - 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); - assert!(aggregation_package.merge_package(noaggregation_package).is_err()); - } - - #[test] - 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); - - let mut empty_package = PackageTemplate::build_package(txid, 0, revk_outp.clone(), 1000); + let mut empty_package = PackageTemplate::build_package(fake_txid(1), 0, revk_outp.clone(), 0); empty_package.inputs = vec![]; - let package = PackageTemplate::build_package(txid, 1, revk_outp.clone(), 1000); - assert!(empty_package.merge_package(package).is_err()); + 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] - 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_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 revoked_package = PackageTemplate::build_package(txid, 0, revk_outp, 1000); - let counterparty_package = PackageTemplate::build_package(txid, 1, counterparty_outp, 1000); - assert!(revoked_package.merge_package(counterparty_package).is_err()); + 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()); + + 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] + 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()); + } + + #[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_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); + + 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 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(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); + 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).is_ok()); - assert!(package_one.merge_package(package_three).is_ok()); + 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!(); } @@ -1424,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); @@ -1446,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 f5b137833..24a882ca6 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(); + } + } + } - // After exhaustion of height timer, a new bumped justice tx should have been broadcast, check it + // One or more justice tx should have been broadcast, check it. + check_broadcasted_txn!(penalty_txids_1, fee_rates_1); + + // 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,7 +7576,10 @@ 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] @@ -7672,30 +7589,28 @@ fn test_bump_penalty_txn_on_revoked_htlcs() { // 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[1].input[0].previous_output, revoked_htlc_txn[1].input[0].previous_output); - assert_eq!(node_txn[2].input[0].previous_output, revoked_htlc_txn[0].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(); + 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 d18f16235..caf5d6bd2 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())); + 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()), + ); - // 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())); - - 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); + 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[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); - 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!(as_commitment_claim_txn[1].input.iter().any(|inp| inp.previous_output.vout == 2)); - assert!(as_commitment_claim_txn[1].input.iter().any(|inp| inp.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!(as_commitment_claim_txn[0].input.iter().any(|inp| inp.previous_output.vout == 2)); - assert!(as_commitment_claim_txn[0].input.iter().any(|inp| inp.previous_output.vout == 0)); - assert!(as_commitment_claim_txn[0].input.iter().any(|inp| inp.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!(claim_txn[0].input.iter().any(|inp| inp.previous_output.vout == 3)); - assert!(claim_txn[0].input.iter().any(|inp| inp.previous_output.vout == 0)); - assert!(claim_txn[0].input.iter().any(|inp| inp.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 { + assert_eq!( + vec![ + Balance::ClaimableAwaitingConfirmations { + amount_satoshis: claim_txn[0].output[0].value.to_sat(), + confirmation_height: rest_claim_maturity, + source: BalanceSource::CounterpartyForceClosed, + }, + 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(), - 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 { - 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] }) - }; + 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_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]); + 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 {