diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 5ed524ae764..ac3a1f66030 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -317,7 +317,7 @@ impl HolderCommitment { let delayed_payment_key = &tx_keys.broadcaster_delayed_payment_key; let per_commitment_point = &tx_keys.per_commitment_point; - let mut nondust_htlcs = self.tx.htlcs().iter().zip(self.tx.counterparty_htlc_sigs.iter()); + let mut nondust_htlcs = self.tx.nondust_htlcs().iter().zip(self.tx.counterparty_htlc_sigs.iter()); let mut sources = self.nondust_htlc_sources.iter(); // Use an iterator to write `htlc_outputs` to avoid allocations. @@ -937,7 +937,7 @@ impl TryFrom<(HolderCommitmentTransaction, HolderSignedTx)> for HolderCommitment // HTLC sources, separately. All offered, non-dust HTLCs must have a source available. let mut missing_nondust_source = false; - let mut nondust_htlc_sources = Vec::with_capacity(holder_commitment_tx.htlcs().len()); + let mut nondust_htlc_sources = Vec::with_capacity(holder_commitment_tx.nondust_htlcs().len()); let dust_htlcs = holder_signed_tx.htlc_outputs.into_iter().filter_map(|(htlc, _, source)| { // Filter our non-dust HTLCs, while at the same time pushing their sources into // `nondust_htlc_sources`. @@ -967,16 +967,16 @@ impl TryFrom<(HolderCommitmentTransaction, HolderSignedTx)> for HolderCommitment impl HolderCommitment { fn has_htlcs(&self) -> bool { - self.tx.htlcs().len() > 0 || self.dust_htlcs.len() > 0 + self.tx.nondust_htlcs().len() > 0 || self.dust_htlcs.len() > 0 } fn htlcs(&self) -> impl Iterator { - self.tx.htlcs().iter().chain(self.dust_htlcs.iter().map(|(htlc, _)| htlc)) + self.tx.nondust_htlcs().iter().chain(self.dust_htlcs.iter().map(|(htlc, _)| htlc)) } fn htlcs_with_sources(&self) -> impl Iterator)> { let mut sources = self.nondust_htlc_sources.iter(); - let nondust_htlcs = self.tx.htlcs().iter().map(move |htlc| { + let nondust_htlcs = self.tx.nondust_htlcs().iter().map(move |htlc| { let mut source = None; if htlc.offered && htlc.transaction_output_index.is_some() { source = sources.next(); @@ -3093,8 +3093,8 @@ impl ChannelMonitorImpl { // If we have non-dust HTLCs in htlc_outputs, ensure they match the HTLCs in the // `holder_commitment_tx`. In the future, we'll no longer provide the redundant data // and just pass in source data via `nondust_htlc_sources`. - debug_assert_eq!(htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).count(), holder_commitment_tx.trust().htlcs().len()); - for (a, b) in htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).map(|(h, _, _)| h).zip(holder_commitment_tx.trust().htlcs().iter()) { + debug_assert_eq!(htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).count(), holder_commitment_tx.trust().nondust_htlcs().len()); + for (a, b) in htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).map(|(h, _, _)| h).zip(holder_commitment_tx.trust().nondust_htlcs().iter()) { debug_assert_eq!(a, b); } debug_assert_eq!(htlc_outputs.iter().filter(|(_, s, _)| s.is_some()).count(), holder_commitment_tx.counterparty_htlc_sigs.len()); @@ -3104,7 +3104,7 @@ impl ChannelMonitorImpl { // Backfill the non-dust HTLC sources. debug_assert!(nondust_htlc_sources.is_empty()); - nondust_htlc_sources.reserve_exact(holder_commitment_tx.htlcs().len()); + nondust_htlc_sources.reserve_exact(holder_commitment_tx.nondust_htlcs().len()); let dust_htlcs = htlc_outputs.into_iter().filter_map(|(htlc, _, source)| { // Filter our non-dust HTLCs, while at the same time pushing their sources into // `nondust_htlc_sources`. @@ -3124,7 +3124,7 @@ impl ChannelMonitorImpl { // `nondust_htlc_sources` and the `holder_commitment_tx` { let mut prev = -1; - for htlc in holder_commitment_tx.trust().htlcs().iter() { + for htlc in holder_commitment_tx.trust().nondust_htlcs().iter() { assert!(htlc.transaction_output_index.unwrap() as i32 > prev); prev = htlc.transaction_output_index.unwrap() as i32; } @@ -3132,10 +3132,10 @@ impl ChannelMonitorImpl { debug_assert!(htlc_outputs.iter().all(|(htlc, _, _)| htlc.transaction_output_index.is_none())); debug_assert!(htlc_outputs.iter().all(|(_, sig_opt, _)| sig_opt.is_none())); - debug_assert_eq!(holder_commitment_tx.trust().htlcs().len(), holder_commitment_tx.counterparty_htlc_sigs.len()); + debug_assert_eq!(holder_commitment_tx.trust().nondust_htlcs().len(), holder_commitment_tx.counterparty_htlc_sigs.len()); let mut sources = nondust_htlc_sources.iter(); - for htlc in holder_commitment_tx.trust().htlcs().iter() { + for htlc in holder_commitment_tx.trust().nondust_htlcs().iter() { if htlc.offered { let source = sources.next().expect("Non-dust HTLC sources didn't match commitment tx"); assert!(source.possibly_matches_output(htlc)); @@ -3526,7 +3526,7 @@ impl ChannelMonitorImpl { let counterparty_node_id = self.counterparty_node_id; let commitment_txid = commitment_tx.compute_txid(); debug_assert_eq!(self.funding.current_holder_commitment.tx.trust().txid(), commitment_txid); - let pending_htlcs = self.funding.current_holder_commitment.tx.trust().htlcs().to_vec(); + let pending_htlcs = self.funding.current_holder_commitment.tx.trust().nondust_htlcs().to_vec(); let channel_value_satoshis = self.funding.channel_parameters.channel_value_satoshis; let commitment_tx_fee_satoshis = channel_value_satoshis - commitment_tx.output.iter().fold(0u64, |sum, output| sum + output.value.to_sat()); @@ -3598,7 +3598,7 @@ impl ChannelMonitorImpl { to_broadcaster_value, to_countersignatory_value, )| { - let htlc_outputs = vec![]; + let nondust_htlcs = vec![]; let commitment_tx = self.build_counterparty_commitment_tx( INITIAL_COMMITMENT_NUMBER, @@ -3606,7 +3606,7 @@ impl ChannelMonitorImpl { to_broadcaster_value, to_countersignatory_value, feerate_per_kw, - htlc_outputs, + &nondust_htlcs, ); // Take the opportunity to populate this recently introduced field self.initial_counterparty_commitment_tx = Some(commitment_tx.clone()); @@ -3619,11 +3619,11 @@ impl ChannelMonitorImpl { fn build_counterparty_commitment_tx( &self, commitment_number: u64, their_per_commitment_point: &PublicKey, to_broadcaster_value: u64, to_countersignatory_value: u64, feerate_per_kw: u32, - mut nondust_htlcs: Vec<(HTLCOutputInCommitment, Option>)> + nondust_htlcs: &Vec ) -> CommitmentTransaction { let channel_parameters = &self.funding.channel_parameters.as_counterparty_broadcastable(); - CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number, their_per_commitment_point, - to_broadcaster_value, to_countersignatory_value, feerate_per_kw, &mut nondust_htlcs, channel_parameters, &self.onchain_tx_handler.secp_ctx) + CommitmentTransaction::new(commitment_number, their_per_commitment_point, + to_broadcaster_value, to_countersignatory_value, feerate_per_kw, nondust_htlcs, channel_parameters, &self.onchain_tx_handler.secp_ctx) } fn counterparty_commitment_txs_from_update(&self, update: &ChannelMonitorUpdate) -> Vec { @@ -3639,12 +3639,12 @@ impl ChannelMonitorImpl { to_countersignatory_value_sat: Some(to_countersignatory_value) } => { let nondust_htlcs = htlc_outputs.iter().filter_map(|(htlc, _)| { - htlc.transaction_output_index.map(|_| (htlc.clone(), None)) + htlc.transaction_output_index.map(|_| htlc).cloned() }).collect::>(); let commitment_tx = self.build_counterparty_commitment_tx(commitment_number, &their_per_commitment_point, to_broadcaster_value, - to_countersignatory_value, feerate_per_kw, nondust_htlcs); + to_countersignatory_value, feerate_per_kw, &nondust_htlcs); debug_assert_eq!(commitment_tx.trust().txid(), commitment_txid); @@ -3973,7 +3973,7 @@ impl ChannelMonitorImpl { fn get_broadcasted_holder_claims( &self, holder_tx: &HolderCommitmentTransaction, conf_height: u32, ) -> (Vec, Option<(ScriptBuf, PublicKey, RevocationKey)>) { - let mut claim_requests = Vec::with_capacity(holder_tx.htlcs().len()); + let mut claim_requests = Vec::with_capacity(holder_tx.nondust_htlcs().len()); let tx = holder_tx.trust(); let keys = tx.keys(); @@ -3985,7 +3985,7 @@ impl ChannelMonitorImpl { )); let txid = tx.txid(); - for htlc in holder_tx.htlcs() { + for htlc in holder_tx.nondust_htlcs() { if let Some(transaction_output_index) = htlc.transaction_output_index { let (htlc_output, counterparty_spendable_height) = if htlc.offered { let htlc_output = HolderHTLCOutput::build_offered( @@ -4020,9 +4020,9 @@ impl ChannelMonitorImpl { // Returns holder HTLC outputs to watch and react to in case of spending. fn get_broadcasted_holder_watch_outputs(&self, holder_tx: &HolderCommitmentTransaction) -> Vec<(u32, TxOut)> { - let mut watch_outputs = Vec::with_capacity(holder_tx.htlcs().len()); + let mut watch_outputs = Vec::with_capacity(holder_tx.nondust_htlcs().len()); let tx = holder_tx.trust(); - for htlc in holder_tx.htlcs() { + for htlc in holder_tx.nondust_htlcs() { if let Some(transaction_output_index) = htlc.transaction_output_index { watch_outputs.push(( transaction_output_index, @@ -4115,7 +4115,7 @@ impl ChannelMonitorImpl { let txid = self.funding.current_holder_commitment.tx.trust().txid(); log_trace!(logger, "Canceling claims for previously broadcast holder commitment {}", txid); let mut outpoint = BitcoinOutPoint { txid, vout: 0 }; - for htlc in self.funding.current_holder_commitment.tx.htlcs() { + for htlc in self.funding.current_holder_commitment.tx.nondust_htlcs() { if let Some(vout) = htlc.transaction_output_index { outpoint.vout = vout; self.onchain_tx_handler.abandon_claim(&outpoint); @@ -4129,7 +4129,7 @@ impl ChannelMonitorImpl { if txid != *confirmed_commitment_txid { log_trace!(logger, "Canceling claims for previously broadcast holder commitment {}", txid); let mut outpoint = BitcoinOutPoint { txid, vout: 0 }; - for htlc in prev_holder_commitment.tx.htlcs() { + for htlc in prev_holder_commitment.tx.nondust_htlcs() { if let Some(vout) = htlc.transaction_output_index { outpoint.vout = vout; self.onchain_tx_handler.abandon_claim(&outpoint); @@ -4155,7 +4155,7 @@ impl ChannelMonitorImpl { if self.channel_type_features().supports_anchors_zero_fee_htlc_tx() { return holder_transactions; } - for htlc in self.funding.current_holder_commitment.tx.htlcs() { + for htlc in self.funding.current_holder_commitment.tx.nondust_htlcs() { if let Some(vout) = htlc.transaction_output_index { let preimage = if !htlc.offered { if let Some((preimage, _)) = self.payment_preimages.get(&htlc.payment_hash) { Some(preimage.clone()) } else { @@ -5595,13 +5595,13 @@ mod tests { { let mut res = Vec::new(); for (idx, preimage) in $preimages_slice.iter().enumerate() { - res.push((HTLCOutputInCommitment { + res.push(HTLCOutputInCommitment { offered: true, amount_msat: 0, cltv_expiry: 0, payment_hash: preimage.1.clone(), transaction_output_index: Some(idx as u32), - }, ())); + }); } res } @@ -5609,7 +5609,7 @@ mod tests { } macro_rules! preimages_slice_to_htlc_outputs { ($preimages_slice: expr) => { - preimages_slice_to_htlcs!($preimages_slice).into_iter().map(|(htlc, _)| (htlc, None)).collect() + preimages_slice_to_htlcs!($preimages_slice).into_iter().map(|htlc| (htlc, None)).collect() } } let dummy_sig = crate::crypto::utils::sign(&secp_ctx, @@ -5665,15 +5665,17 @@ mod tests { let best_block = BestBlock::from_network(Network::Testnet); let monitor = ChannelMonitor::new( Secp256k1::new(), keys, Some(shutdown_script.into_inner()), 0, &ScriptBuf::new(), - &channel_parameters, true, 0, HolderCommitmentTransaction::dummy(0, &mut Vec::new()), + &channel_parameters, true, 0, HolderCommitmentTransaction::dummy(0, &Vec::new()), best_block, dummy_key, channel_id, ); - let mut htlcs = preimages_slice_to_htlcs!(preimages[0..10]); - let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs); + let nondust_htlcs = preimages_slice_to_htlcs!(preimages[0..10]); + let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &nondust_htlcs); + // These htlcs now have their output indices assigned + let nondust_htlcs = dummy_commitment_tx.nondust_htlcs(); monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(), - htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), Some(dummy_source.clone()))).collect()); + nondust_htlcs.into_iter().map(|htlc| (htlc.clone(), Some(dummy_sig), Some(dummy_source.clone()))).collect()); monitor.provide_latest_counterparty_commitment_tx(Txid::from_byte_array(Sha256::hash(b"1").to_byte_array()), preimages_slice_to_htlc_outputs!(preimages[5..15]), 281474976710655, dummy_key, &logger); monitor.provide_latest_counterparty_commitment_tx(Txid::from_byte_array(Sha256::hash(b"2").to_byte_array()), @@ -5708,10 +5710,12 @@ mod tests { // Now update holder commitment tx info, pruning only element 18 as we still care about the // previous commitment tx's preimages too - let mut htlcs = preimages_slice_to_htlcs!(preimages[0..5]); - let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs); + let nondust_htlcs = preimages_slice_to_htlcs!(preimages[0..5]); + let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &nondust_htlcs); + // These htlcs now have their output indices assigned + let nondust_htlcs = dummy_commitment_tx.nondust_htlcs(); monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(), - htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), Some(dummy_source.clone()))).collect()); + nondust_htlcs.into_iter().map(|htlc| (htlc.clone(), Some(dummy_sig), Some(dummy_source.clone()))).collect()); secret[0..32].clone_from_slice(&>::from_hex("2273e227a5b7449b6e70f1fb4652864038b1cbf9cd7c043a7d6456b7fc275ad8").unwrap()); monitor.provide_secret(281474976710653, secret.clone()).unwrap(); assert_eq!(monitor.inner.lock().unwrap().payment_preimages.len(), 12); @@ -5719,10 +5723,12 @@ mod tests { test_preimages_exist!(&preimages[18..20], monitor); // But if we do it again, we'll prune 5-10 - let mut htlcs = preimages_slice_to_htlcs!(preimages[0..3]); - let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs); - monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx, - htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), Some(dummy_source.clone()))).collect()); + let nondust_htlcs = preimages_slice_to_htlcs!(preimages[0..3]); + let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &nondust_htlcs); + // These htlcs now have their output indices assigned + let nondust_htlcs = dummy_commitment_tx.nondust_htlcs(); + monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(), + nondust_htlcs.into_iter().map(|htlc| (htlc.clone(), Some(dummy_sig), Some(dummy_source.clone()))).collect()); secret[0..32].clone_from_slice(&>::from_hex("27cddaa5624534cb6cb9d7da077cf2b22ab21e9b506fd4998a51d54502e99116").unwrap()); monitor.provide_secret(281474976710652, secret.clone()).unwrap(); assert_eq!(monitor.inner.lock().unwrap().payment_preimages.len(), 5); @@ -5917,7 +5923,7 @@ mod tests { let best_block = BestBlock::from_network(Network::Testnet); let monitor = ChannelMonitor::new( Secp256k1::new(), keys, Some(shutdown_script.into_inner()), 0, &ScriptBuf::new(), - &channel_parameters, true, 0, HolderCommitmentTransaction::dummy(0, &mut Vec::new()), + &channel_parameters, true, 0, HolderCommitmentTransaction::dummy(0, &Vec::new()), best_block, dummy_key, channel_id, ); diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 5ec0713bae6..be6608b57e7 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -1205,7 +1205,7 @@ impl OnchainTxHandler { if trusted_tx.txid() != outp.txid { return None; } - let (htlc_idx, htlc) = trusted_tx.htlcs().iter().enumerate() + let (htlc_idx, htlc) = trusted_tx.nondust_htlcs().iter().enumerate() .find(|(_, htlc)| htlc.transaction_output_index.unwrap() == outp.vout) .unwrap(); let counterparty_htlc_sig = holder_commitment.counterparty_htlc_sigs[htlc_idx]; @@ -1248,7 +1248,7 @@ impl OnchainTxHandler { if outp.txid != trusted_tx.txid() { return None; } - trusted_tx.htlcs().iter().enumerate() + trusted_tx.nondust_htlcs().iter().enumerate() .find(|(_, htlc)| if let Some(output_index) = htlc.transaction_output_index { output_index == outp.vout } else { @@ -1359,29 +1359,28 @@ mod tests { // Create an OnchainTxHandler for a commitment containing HTLCs with CLTV expiries of 0, 1, // and 2 blocks. - let mut htlcs = Vec::new(); + let mut nondust_htlcs = Vec::new(); for i in 0..3 { let preimage = PaymentPreimage([i; 32]); let hash = PaymentHash(Sha256::hash(&preimage.0[..]).to_byte_array()); - htlcs.push(( + nondust_htlcs.push( HTLCOutputInCommitment { offered: true, amount_msat: 10000, cltv_expiry: i as u32, payment_hash: hash, transaction_output_index: Some(i as u32), - }, - (), - )); + } + ); } - let holder_commit = HolderCommitmentTransaction::dummy(1000000, &mut htlcs); + let holder_commit = HolderCommitmentTransaction::dummy(1000000, &nondust_htlcs); let mut tx_handler = OnchainTxHandler::new( 1000000, [0; 32], ScriptBuf::new(), signer, chan_params, - holder_commit, + holder_commit.clone(), secp_ctx, ); @@ -1400,7 +1399,7 @@ mod tests { // Request claiming of each HTLC on the holder's commitment, with current block height 1. let holder_commit_txid = tx_handler.get_unsigned_holder_commitment_tx().compute_txid(); let mut requests = Vec::new(); - for (htlc, _) in htlcs { + for htlc in holder_commit.nondust_htlcs() { requests.push(PackageTemplate::build_package( holder_commit_txid, htlc.transaction_output_index.unwrap(), diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 93fd78fbd32..3404075b3a0 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -614,6 +614,13 @@ impl HTLCOutputInCommitment { pub const fn to_bitcoin_amount(&self) -> Amount { Amount::from_sat(self.amount_msat / 1000) } + + pub(crate) fn is_data_equal(&self, other: &HTLCOutputInCommitment) -> bool { + self.offered == other.offered + && self.amount_msat == other.amount_msat + && self.cltv_expiry == other.cltv_expiry + && self.payment_hash == other.payment_hash + } } impl_writeable_tlv_based!(HTLCOutputInCommitment, { @@ -1182,7 +1189,7 @@ impl_writeable_tlv_based!(HolderCommitmentTransaction, { impl HolderCommitmentTransaction { #[cfg(test)] - pub fn dummy(channel_value_satoshis: u64, htlcs: &mut Vec<(HTLCOutputInCommitment, ())>) -> Self { + pub fn dummy(channel_value_satoshis: u64, nondust_htlcs: &Vec) -> Self { let secp_ctx = Secp256k1::new(); let dummy_key = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let dummy_sig = sign(&secp_ctx, &secp256k1::Message::from_digest([42; 32]), &SecretKey::from_slice(&[42; 32]).unwrap()); @@ -1205,11 +1212,10 @@ impl HolderCommitmentTransaction { channel_value_satoshis, }; let mut counterparty_htlc_sigs = Vec::new(); - for _ in 0..htlcs.len() { + for _ in 0..nondust_htlcs.len() { counterparty_htlc_sigs.push(dummy_sig); } - let inner = CommitmentTransaction::new_with_auxiliary_htlc_data(0, &dummy_key, 0, 0, 0, htlcs, &channel_parameters.as_counterparty_broadcastable(), &secp_ctx); - htlcs.sort_by_key(|htlc| htlc.0.transaction_output_index); + let inner = CommitmentTransaction::new(0, &dummy_key, 0, 0, 0, nondust_htlcs, &channel_parameters.as_counterparty_broadcastable(), &secp_ctx); HolderCommitmentTransaction { inner, counterparty_sig: dummy_sig, @@ -1430,7 +1436,7 @@ pub struct CommitmentTransaction { feerate_per_kw: u32, // The set of non-dust HTLCs included in the commitment. They must be sorted in increasing // output index order. - htlcs: Vec, + nondust_htlcs: Vec, // Note that on upgrades, some features of existing outputs may be missed. channel_type_features: ChannelTypeFeatures, // A cache of the parties' pubkeys required to construct the transaction, see doc for trust() @@ -1446,7 +1452,7 @@ impl PartialEq for CommitmentTransaction { self.to_broadcaster_value_sat == o.to_broadcaster_value_sat && self.to_countersignatory_value_sat == o.to_countersignatory_value_sat && self.feerate_per_kw == o.feerate_per_kw && - self.htlcs == o.htlcs && + self.nondust_htlcs == o.nondust_htlcs && self.channel_type_features == o.channel_type_features && self.keys == o.keys; if eq { @@ -1468,7 +1474,7 @@ impl Writeable for CommitmentTransaction { (6, self.feerate_per_kw, required), (8, self.keys, required), (10, self.built, required), - (12, self.htlcs, required_vec), + (12, self.nondust_htlcs, required_vec), (14, legacy_deserialization_prevention_marker, option), (15, self.channel_type_features, required), }); @@ -1486,7 +1492,7 @@ impl Readable for CommitmentTransaction { (6, feerate_per_kw, required), (8, keys, required), (10, built, required), - (12, htlcs, required_vec), + (12, nondust_htlcs, required_vec), (14, _legacy_deserialization_prevention_marker, (option, explicit_type: ())), (15, channel_type_features, option), }); @@ -1503,32 +1509,23 @@ impl Readable for CommitmentTransaction { feerate_per_kw: feerate_per_kw.0.unwrap(), keys: keys.0.unwrap(), built: built.0.unwrap(), - htlcs, + nondust_htlcs, channel_type_features: channel_type_features.unwrap_or(ChannelTypeFeatures::only_static_remote_key()) }) } } impl CommitmentTransaction { - /// Construct an object of the class while assigning transaction output indices to HTLCs. - /// - /// Populates HTLCOutputInCommitment.transaction_output_index in htlcs_with_aux. - /// - /// The generic T allows the caller to match the HTLC output index with auxiliary data. - /// This auxiliary data is not stored in this object. - /// /// Only include HTLCs that are above the dust limit for the channel. - /// - /// This is not exported to bindings users due to the generic though we likely should expose a version without - pub fn new_with_auxiliary_htlc_data(commitment_number: u64, per_commitment_point: &PublicKey, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, feerate_per_kw: u32, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters, secp_ctx: &Secp256k1) -> CommitmentTransaction { + /// The broadcaster and countersignatory amounts MUST be EITHER 0 or ABOVE DUST. + pub fn new(commitment_number: u64, per_commitment_point: &PublicKey, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, feerate_per_kw: u32, nondust_htlcs: &Vec, channel_parameters: &DirectedChannelTransactionParameters, secp_ctx: &Secp256k1) -> CommitmentTransaction { let to_broadcaster_value_sat = Amount::from_sat(to_broadcaster_value_sat); let to_countersignatory_value_sat = Amount::from_sat(to_countersignatory_value_sat); let keys = TxCreationKeys::from_channel_static_keys(per_commitment_point, channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), secp_ctx); - // Sort outputs and populate output indices while keeping track of the auxiliary data - let (outputs, htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters); + let (outputs, nondust_htlcs) = Self::build_outputs_and_htlcs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, nondust_htlcs, channel_parameters); - let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(commitment_number, channel_parameters); + let (obscured_commitment_transaction_number, txins) = Self::build_inputs(commitment_number, channel_parameters); let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs); let txid = transaction.compute_txid(); CommitmentTransaction { @@ -1537,7 +1534,7 @@ impl CommitmentTransaction { to_countersignatory_value_sat, to_broadcaster_delay: Some(channel_parameters.contest_delay()), feerate_per_kw, - htlcs, + nondust_htlcs, channel_type_features: channel_parameters.channel_type_features().clone(), keys, built: BuiltCommitmentTransaction { @@ -1555,11 +1552,11 @@ impl CommitmentTransaction { self } - fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters) -> BuiltCommitmentTransaction { - let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(self.commitment_number, channel_parameters); + fn rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters) -> BuiltCommitmentTransaction { + let (obscured_commitment_transaction_number, txins) = Self::build_inputs(self.commitment_number, channel_parameters); - let mut htlcs_with_aux = self.htlcs.iter().map(|h| (h.clone(), ())).collect(); - let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &mut htlcs_with_aux, channel_parameters); + let txout_htlc_pairs: Vec<(TxOut, Option<&HTLCOutputInCommitment>)> = Self::build_txout_htlc_pairs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &self.nondust_htlcs, channel_parameters); + let outputs = txout_htlc_pairs.into_iter().map(|(output, _)| output).collect(); let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs); let txid = transaction.compute_txid(); @@ -1579,25 +1576,44 @@ impl CommitmentTransaction { } } - // Builds the set of outputs for the commitment transaction. There will be an output per - // non-dust HTLC, one output for the holder, another for the counterparty, and possibly anchor - // output(s). + // This function splits the txout-HTLC pairs into two vectors, one for the txouts, and another for the HTLCs. + // As it pushes each HTLC into the HTLC vector, this function also populates the output index of that HTLC. + fn build_outputs_and_htlcs(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, nondust_htlcs: &Vec, channel_parameters: &DirectedChannelTransactionParameters) -> (Vec, Vec) { + let txout_htlc_pairs: Vec<(TxOut, Option<&HTLCOutputInCommitment>)> = Self::build_txout_htlc_pairs(keys, to_broadcaster_value_sat, to_countersignatory_value_sat, &nondust_htlcs, channel_parameters); + let mut outputs: Vec = Vec::with_capacity(txout_htlc_pairs.len()); + let mut nondust_htlcs: Vec = Vec::with_capacity(txout_htlc_pairs.len()); + for (idx, (output, htlc)) in txout_htlc_pairs.into_iter().enumerate() { + if let Some(htlc) = htlc { + let mut htlc = htlc.clone(); + htlc.transaction_output_index = Some(idx as u32); + nondust_htlcs.push(htlc); + } + outputs.push(output); + } + (outputs, nondust_htlcs) + } + + // Builds the set of outputs for the commitment transaction. Each HTLC output is paired with its corresponding data. + // There will be an output for all passed HTLCs, since they are all non-dust. + // Then depending on amounts, and features, this function assigns holder, counterparty, and anchor outputs. // // The set of outputs are sorted according to BIP-69 ordering, guaranteeing that HTLCs are // returned in increasing output index order. // // This is used in two cases: - // - initial sorting of outputs / HTLCs in the constructor, in which case T is auxiliary data the - // caller needs to have sorted together with the HTLCs so it can keep track of the output index - // - building of a bitcoin transaction during a verify() call, in which case T is just () - fn internal_build_outputs(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters) -> (Vec, Vec) { + // - initial sorting of outputs / HTLCs in the constructor, via `build_outputs_and_htlcs` + // - building of a bitcoin transaction during a verify() call + // + // Note that the HTLC data in the second column of the returned table is NOT populated with the + // output indices of the HTLCs; this is done in `build_outputs_and_htlcs`. + fn build_txout_htlc_pairs<'a>(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, nondust_htlcs: &'a Vec, channel_parameters: &DirectedChannelTransactionParameters) -> Vec<(TxOut, Option<&'a HTLCOutputInCommitment>)> { let countersignatory_payment_point = &channel_parameters.countersignatory_pubkeys().payment_point; let countersignatory_funding_key = &channel_parameters.countersignatory_pubkeys().funding_pubkey; let broadcaster_funding_key = &channel_parameters.broadcaster_pubkeys().funding_pubkey; let channel_type = channel_parameters.channel_type_features(); let contest_delay = channel_parameters.contest_delay(); - let mut txouts: Vec<(TxOut, Option<&mut HTLCOutputInCommitment>)> = Vec::new(); + let mut txout_htlc_pairs: Vec<(TxOut, Option<&HTLCOutputInCommitment>)> = Vec::with_capacity(nondust_htlcs.len() + 4); if to_countersignatory_value_sat > Amount::ZERO { let script = if channel_type.supports_anchors_zero_fee_htlc_tx() { @@ -1605,7 +1621,7 @@ impl CommitmentTransaction { } else { ScriptBuf::new_p2wpkh(&Hash160::hash(&countersignatory_payment_point.serialize()).into()) }; - txouts.push(( + txout_htlc_pairs.push(( TxOut { script_pubkey: script.clone(), value: to_countersignatory_value_sat, @@ -1620,7 +1636,7 @@ impl CommitmentTransaction { contest_delay, &keys.broadcaster_delayed_payment_key, ); - txouts.push(( + txout_htlc_pairs.push(( TxOut { script_pubkey: redeem_script.to_p2wsh(), value: to_broadcaster_value_sat, @@ -1630,9 +1646,9 @@ impl CommitmentTransaction { } if channel_type.supports_anchors_zero_fee_htlc_tx() { - if to_broadcaster_value_sat > Amount::ZERO || !htlcs_with_aux.is_empty() { + if to_broadcaster_value_sat > Amount::ZERO || !nondust_htlcs.is_empty() { let anchor_script = get_keyed_anchor_redeemscript(broadcaster_funding_key); - txouts.push(( + txout_htlc_pairs.push(( TxOut { script_pubkey: anchor_script.to_p2wsh(), value: Amount::from_sat(ANCHOR_OUTPUT_VALUE_SATOSHI), @@ -1641,9 +1657,9 @@ impl CommitmentTransaction { )); } - if to_countersignatory_value_sat > Amount::ZERO || !htlcs_with_aux.is_empty() { + if to_countersignatory_value_sat > Amount::ZERO || !nondust_htlcs.is_empty() { let anchor_script = get_keyed_anchor_redeemscript(countersignatory_funding_key); - txouts.push(( + txout_htlc_pairs.push(( TxOut { script_pubkey: anchor_script.to_p2wsh(), value: Amount::from_sat(ANCHOR_OUTPUT_VALUE_SATOSHI), @@ -1653,44 +1669,35 @@ impl CommitmentTransaction { } } - let mut htlcs = Vec::with_capacity(htlcs_with_aux.len()); - for (htlc, _) in htlcs_with_aux { + for htlc in nondust_htlcs { let script = get_htlc_redeemscript(htlc, channel_type, keys); let txout = TxOut { script_pubkey: script.to_p2wsh(), value: htlc.to_bitcoin_amount(), }; - txouts.push((txout, Some(htlc))); + txout_htlc_pairs.push((txout, Some(htlc))); } // Sort output in BIP-69 order (amount, scriptPubkey). Tie-breaks based on HTLC // CLTV expiration height. - sort_outputs(&mut txouts, |a, b| { - if let &Some(ref a_htlcout) = a { - if let &Some(ref b_htlcout) = b { - a_htlcout.cltv_expiry.cmp(&b_htlcout.cltv_expiry) + sort_outputs(&mut txout_htlc_pairs, |a: &Option<&HTLCOutputInCommitment>, b: &Option<&HTLCOutputInCommitment>| { + if let Some(a) = a { + if let Some(b) = b { + a.cltv_expiry.cmp(&b.cltv_expiry) // Note that due to hash collisions, we have to have a fallback comparison // here for fuzzing mode (otherwise at least chanmon_fail_consistency // may fail)! - .then(a_htlcout.payment_hash.0.cmp(&b_htlcout.payment_hash.0)) + .then(a.payment_hash.cmp(&b.payment_hash)) // For non-HTLC outputs, if they're copying our SPK we don't really care if we // close the channel due to mismatches - they're doing something dumb: } else { cmp::Ordering::Equal } } else { cmp::Ordering::Equal } }); - let mut outputs = Vec::with_capacity(txouts.len()); - for (idx, out) in txouts.drain(..).enumerate() { - if let Some(htlc) = out.1 { - htlc.transaction_output_index = Some(idx as u32); - htlcs.push(htlc.clone()); - } - outputs.push(out.0); - } - (outputs, htlcs) + txout_htlc_pairs } - fn internal_build_inputs(commitment_number: u64, channel_parameters: &DirectedChannelTransactionParameters) -> (u64, Vec) { + fn build_inputs(commitment_number: u64, channel_parameters: &DirectedChannelTransactionParameters) -> (u64, Vec) { let broadcaster_pubkeys = channel_parameters.broadcaster_pubkeys(); let countersignatory_pubkeys = channel_parameters.countersignatory_pubkeys(); let commitment_transaction_number_obscure_factor = get_commitment_transaction_number_obscure_factor( @@ -1746,8 +1753,8 @@ impl CommitmentTransaction { /// /// This is not exported to bindings users as we cannot currently convert Vec references to/from C, though we should /// expose a less effecient version which creates a Vec of references in the future. - pub fn htlcs(&self) -> &Vec { - &self.htlcs + pub fn nondust_htlcs(&self) -> &Vec { + &self.nondust_htlcs } /// Trust our pre-built transaction and derived transaction creation public keys. @@ -1773,7 +1780,7 @@ impl CommitmentTransaction { if keys != self.keys { return Err(()); } - let tx = self.internal_rebuild_transaction(&keys, channel_parameters); + let tx = self.rebuild_transaction(&keys, channel_parameters); if self.built.transaction != tx.transaction || self.built.txid != tx.txid { return Err(()); } @@ -1831,10 +1838,10 @@ impl<'a> TrustedCommitmentTransaction<'a> { let inner = self.inner; let keys = &inner.keys; let txid = inner.built.txid; - let mut ret = Vec::with_capacity(inner.htlcs.len()); + let mut ret = Vec::with_capacity(inner.nondust_htlcs.len()); let holder_htlc_key = derive_private_key(secp_ctx, &inner.keys.per_commitment_point, htlc_base_key); - for this_htlc in inner.htlcs.iter() { + for this_htlc in inner.nondust_htlcs.iter() { assert!(this_htlc.transaction_output_index.is_some()); let htlc_tx = build_htlc_transaction(&txid, inner.feerate_per_kw, channel_parameters.contest_delay(), &this_htlc, &self.channel_type_features, &keys.broadcaster_delayed_payment_key, &keys.revocation_key); @@ -1852,7 +1859,7 @@ impl<'a> TrustedCommitmentTransaction<'a> { preimage: &Option, ) -> Transaction { let keys = &self.inner.keys; - let this_htlc = &self.inner.htlcs[htlc_index]; + let this_htlc = &self.inner.nondust_htlcs[htlc_index]; assert!(this_htlc.transaction_output_index.is_some()); // if we don't have preimage for an HTLC-Success, we can't generate an HTLC transaction. if !this_htlc.offered && preimage.is_none() { unreachable!(); } @@ -1874,7 +1881,7 @@ impl<'a> TrustedCommitmentTransaction<'a> { ) -> Witness { let keys = &self.inner.keys; let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys( - &self.inner.htlcs[htlc_index], &self.channel_type_features, &keys.broadcaster_htlc_key, + &self.inner.nondust_htlcs[htlc_index], &self.channel_type_features, &keys.broadcaster_htlc_key, &keys.countersignatory_htlc_key, &keys.revocation_key ); build_htlc_input_witness( @@ -1998,7 +2005,7 @@ mod tests { commitment_number: u64, per_commitment_point: PublicKey, feerate_per_kw: u32, - htlcs_with_aux: Vec<(HTLCOutputInCommitment, ())>, + nondust_htlcs: Vec, channel_parameters: ChannelTransactionParameters, counterparty_pubkeys: ChannelPublicKeys, secp_ctx: Secp256k1::, @@ -2026,13 +2033,13 @@ mod tests { channel_type_features: ChannelTypeFeatures::only_static_remote_key(), channel_value_satoshis: 3000, }; - let htlcs_with_aux = Vec::new(); + let nondust_htlcs = Vec::new(); Self { commitment_number: 0, per_commitment_point, feerate_per_kw: 1, - htlcs_with_aux, + nondust_htlcs, channel_parameters, counterparty_pubkeys, secp_ctx, @@ -2040,9 +2047,9 @@ mod tests { } fn build(&mut self, to_broadcaster_sats: u64, to_countersignatory_sats: u64) -> CommitmentTransaction { - CommitmentTransaction::new_with_auxiliary_htlc_data( + CommitmentTransaction::new( self.commitment_number, &self.per_commitment_point, to_broadcaster_sats, to_countersignatory_sats, self.feerate_per_kw, - &mut self.htlcs_with_aux, &self.channel_parameters.as_holder_broadcastable(), &self.secp_ctx + &self.nondust_htlcs, &self.channel_parameters.as_holder_broadcastable(), &self.secp_ctx ) } } @@ -2088,7 +2095,7 @@ mod tests { // Generate broadcaster output and received and offered HTLC outputs, w/o anchors builder.channel_parameters.channel_type_features = ChannelTypeFeatures::only_static_remote_key(); - builder.htlcs_with_aux = vec![(received_htlc.clone(), ()), (offered_htlc.clone(), ())]; + builder.nondust_htlcs = vec![received_htlc.clone(), offered_htlc.clone()]; let tx = builder.build(3000, 0); let keys = tx.trust().keys(); assert_eq!(tx.built.transaction.output.len(), 3); @@ -2101,7 +2108,7 @@ mod tests { // Generate broadcaster output and received and offered HTLC outputs, with anchors builder.channel_parameters.channel_type_features = ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies(); - builder.htlcs_with_aux = vec![(received_htlc.clone(), ()), (offered_htlc.clone(), ())]; + builder.nondust_htlcs = vec![received_htlc.clone(), offered_htlc.clone()]; let tx = builder.build(3000, 0); assert_eq!(tx.built.transaction.output.len(), 5); assert_eq!(tx.built.transaction.output[2].script_pubkey, get_htlc_redeemscript(&received_htlc, &ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies(), &keys).to_p2wsh()); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 005ec566f0e..aae37977262 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -226,6 +226,37 @@ impl From<&InboundHTLCState> for Option { } } +impl fmt::Display for InboundHTLCState { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + InboundHTLCState::RemoteAnnounced(_) => write!(f, "RemoteAnnounced"), + InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => write!(f, "AwaitingRemoteRevokeToAnnounce"), + InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => write!(f, "AwaitingAnnouncedRemoteRevoke"), + InboundHTLCState::Committed => write!(f, "Committed"), + InboundHTLCState::LocalRemoved(_) => write!(f, "LocalRemoved"), + } + } +} + +impl InboundHTLCState { + fn included_in_commitment(&self, generated_by_local: bool) -> bool { + match self { + InboundHTLCState::RemoteAnnounced(_) => !generated_by_local, + InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => !generated_by_local, + InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => true, + InboundHTLCState::Committed => true, + InboundHTLCState::LocalRemoved(_) => !generated_by_local, + } + } + + fn preimage(&self) -> Option { + match self { + InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(preimage)) => Some(*preimage), + _ => None, + } + } +} + struct InboundHTLCOutput { htlc_id: u64, amount_msat: u64, @@ -234,6 +265,24 @@ struct InboundHTLCOutput { state: InboundHTLCState, } +impl InboundHTLCOutput { + fn is_dust(&self, local: bool, feerate_per_kw: u32, broadcaster_dust_limit_sat: u64, features: &ChannelTypeFeatures) -> bool { + let htlc_tx_fee_sat = if features.supports_anchors_zero_fee_htlc_tx() { + 0 + } else { + let htlc_tx_weight = if !local { + // this is an offered htlc + htlc_timeout_tx_weight(features) + } else { + htlc_success_tx_weight(features) + }; + // As required by the spec, round down + feerate_per_kw as u64 * htlc_tx_weight / 1000 + }; + self.amount_msat / 1000 < broadcaster_dust_limit_sat + htlc_tx_fee_sat + } +} + #[cfg_attr(test, derive(Clone, Debug, PartialEq))] enum OutboundHTLCState { /// Added by us and included in a commitment_signed (if we were AwaitingRemoteRevoke when we @@ -290,10 +339,49 @@ impl From<&OutboundHTLCState> for OutboundHTLCStateDetails { } } +impl fmt::Display for OutboundHTLCState { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + OutboundHTLCState::LocalAnnounced(_) => write!(f, "LocalAnnounced"), + OutboundHTLCState::Committed => write!(f, "Committed"), + OutboundHTLCState::RemoteRemoved(_) => write!(f, "RemoteRemoved"), + OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) => write!(f, "AwaitingRemoteRevokeToRemove"), + OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) => write!(f, "AwaitingRemovedRemoteRevoke"), + } + } +} + +impl OutboundHTLCState { + fn included_in_commitment(&self, generated_by_local: bool) -> bool { + match self { + OutboundHTLCState::LocalAnnounced(_) => generated_by_local, + OutboundHTLCState::Committed => true, + OutboundHTLCState::RemoteRemoved(_) => generated_by_local, + OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) => generated_by_local, + OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) => false, + } + } + + fn preimage(&self) -> Option { + match self { + OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(preimage)) | + OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(preimage)) | + OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(preimage)) => { + debug_assert!(preimage.is_some()); + *preimage + }, + _ => None, + } + } +} + #[derive(Clone)] #[cfg_attr(test, derive(Debug, PartialEq))] enum OutboundHTLCOutcome { - /// LDK version 0.0.105+ will always fill in the preimage here. + /// Except briefly during deserialization and state transitions between success states, + /// we require all success states to hold their corresponding preimage. + /// We started always filling in the preimages here in 0.0.105, and the requirement + /// that the preimages always be filled in was added in 0.2. Success(Option), Failure(HTLCFailReason), } @@ -329,6 +417,24 @@ struct OutboundHTLCOutput { send_timestamp: Option, } +impl OutboundHTLCOutput { + fn is_dust(&self, local: bool, feerate_per_kw: u32, broadcaster_dust_limit_sat: u64, features: &ChannelTypeFeatures) -> bool { + let htlc_tx_fee_sat = if features.supports_anchors_zero_fee_htlc_tx() { + 0 + } else { + let htlc_tx_weight = if local { + // this is an offered htlc + htlc_timeout_tx_weight(features) + } else { + htlc_success_tx_weight(features) + }; + // As required by the spec, round down + feerate_per_kw as u64 * htlc_tx_weight / 1000 + }; + self.amount_msat / 1000 < broadcaster_dust_limit_sat + htlc_tx_fee_sat + } +} + /// See AwaitingRemoteRevoke ChannelState for more info #[cfg_attr(test, derive(Clone, Debug, PartialEq))] enum HTLCUpdateAwaitingACK { @@ -883,6 +989,7 @@ struct HTLCStats { /// A struct gathering data on a commitment, either local or remote. struct CommitmentData<'a> { + tx: CommitmentTransaction, stats: CommitmentStats, htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, // the list of HTLCs (dust HTLCs *included*) which were not ignored when building the transaction outbound_htlc_preimages: Vec, // preimages for successful offered HTLCs since last commitment @@ -891,10 +998,12 @@ struct CommitmentData<'a> { /// A struct gathering stats on a commitment transaction, either local or remote. struct CommitmentStats { - tx: CommitmentTransaction, // the transaction info + feerate_per_kw: u32, // the feerate of the commitment transaction total_fee_sat: u64, // the total fee included in the transaction - local_balance_before_fee_msat: u64, // local balance before fees *not* considering dust limits - remote_balance_before_fee_msat: u64, // remote balance before fees *not* considering dust limits + total_anchors_sat: u64, // the sum of the anchors' amounts + broadcaster_dust_limit_sat: u64, // the broadcaster's dust limit + local_balance_before_fee_anchors_msat: u64, // local balance before fees and anchors *not* considering dust limits + remote_balance_before_fee_anchors_msat: u64, // remote balance before fees and anchors *not* considering dust limits } /// Used when calculating whether we or the remote can afford an additional HTLC. @@ -2124,7 +2233,7 @@ trait InitialRemoteCommitmentReceiver where SP::Target: SignerProvide let commitment_data = self.context().build_commitment_transaction(self.funding(), holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(), true, false, logger); - let initial_commitment_tx = commitment_data.stats.tx; + let initial_commitment_tx = commitment_data.tx; let trusted_tx = initial_commitment_tx.trust(); let initial_commitment_bitcoin_tx = trusted_tx.built_transaction(); let sighash = initial_commitment_bitcoin_tx.get_sighash_all(&funding_script, self.funding().get_value_satoshis()); @@ -2164,7 +2273,7 @@ trait InitialRemoteCommitmentReceiver where SP::Target: SignerProvide let commitment_data = context.build_commitment_transaction(self.funding(), context.cur_counterparty_commitment_transaction_number, &context.counterparty_cur_commitment_point.unwrap(), false, false, logger); - let counterparty_initial_commitment_tx = commitment_data.stats.tx; + let counterparty_initial_commitment_tx = commitment_data.tx; let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust(); let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction(); @@ -3646,7 +3755,7 @@ impl ChannelContext where SP::Target: SignerProvider { holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(), true, false, logger); let commitment_txid = { - let trusted_tx = commitment_data.stats.tx.trust(); + let trusted_tx = commitment_data.tx.trust(); let bitcoin_tx = trusted_tx.built_transaction(); let sighash = bitcoin_tx.get_sighash_all(&funding_script, funding.get_value_satoshis()); @@ -3659,7 +3768,6 @@ impl ChannelContext where SP::Target: SignerProvider { } bitcoin_tx.txid }; - let htlcs_cloned: Vec<_> = commitment_data.htlcs_included.iter().map(|htlc| (htlc.0.clone(), htlc.1.map(|h| h.clone()))).collect(); // If our counterparty updated the channel fee in this commitment transaction, check that // they can actually afford the new fee now. @@ -3669,7 +3777,7 @@ impl ChannelContext where SP::Target: SignerProvider { if update_fee { debug_assert!(!funding.is_outbound()); let counterparty_reserve_we_require_msat = funding.holder_selected_channel_reserve_satoshis * 1000; - if commitment_data.stats.remote_balance_before_fee_msat < commitment_data.stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat { + if commitment_data.stats.remote_balance_before_fee_anchors_msat < commitment_data.stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat { return Err(ChannelError::close("Funding remote cannot afford proposed new fee".to_owned())); } } @@ -3691,16 +3799,16 @@ impl ChannelContext where SP::Target: SignerProvider { } } - if msg.htlc_signatures.len() != commitment_data.stats.tx.htlcs().len() { - return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_data.stats.tx.htlcs().len()))); + if msg.htlc_signatures.len() != commitment_data.tx.nondust_htlcs().len() { + return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_data.tx.nondust_htlcs().len()))); } - let holder_keys = commitment_data.stats.tx.trust().keys(); - let mut nondust_htlc_sources = Vec::with_capacity(commitment_data.stats.tx.htlcs().len()); - let mut dust_htlcs = Vec::with_capacity(htlcs_cloned.len() - commitment_data.stats.tx.htlcs().len()); - for (idx, (htlc, mut source_opt)) in htlcs_cloned.into_iter().enumerate() { + let holder_keys = commitment_data.tx.trust().keys(); + let mut nondust_htlc_sources = Vec::with_capacity(commitment_data.tx.nondust_htlcs().len()); + let mut dust_htlcs = Vec::with_capacity(commitment_data.htlcs_included.len() - commitment_data.tx.nondust_htlcs().len()); + for (idx, (htlc, mut source_opt)) in commitment_data.htlcs_included.into_iter().enumerate() { if let Some(_) = htlc.transaction_output_index { - let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_data.stats.tx.feerate_per_kw(), + let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_data.tx.feerate_per_kw(), funding.get_counterparty_selected_contest_delay().unwrap(), &htlc, funding.get_channel_type(), &holder_keys.broadcaster_delayed_payment_key, &holder_keys.revocation_key); @@ -3715,19 +3823,19 @@ impl ChannelContext where SP::Target: SignerProvider { } if htlc.offered { if let Some(source) = source_opt.take() { - nondust_htlc_sources.push(source); + nondust_htlc_sources.push(source.clone()); } else { panic!("Missing outbound HTLC source"); } } } else { - dust_htlcs.push((htlc, None, source_opt.take())); + dust_htlcs.push((htlc, None, source_opt.take().cloned())); } debug_assert!(source_opt.is_none(), "HTLCSource should have been put somewhere"); } let holder_commitment_tx = HolderCommitmentTransaction::new( - commitment_data.stats.tx, + commitment_data.tx, msg.signature, msg.htlc_signatures.clone(), &funding.get_holder_pubkeys().funding_pubkey, @@ -3744,28 +3852,12 @@ impl ChannelContext where SP::Target: SignerProvider { }) } - /// Transaction nomenclature is somewhat confusing here as there are many different cases - a - /// transaction is referred to as "a's transaction" implying that a will be able to broadcast - /// the transaction. Thus, b will generally be sending a signature over such a transaction to - /// a, and a can revoke the transaction by providing b the relevant per_commitment_secret. As - /// such, a transaction is generally the result of b increasing the amount paid to a (or adding - /// an HTLC to a). - /// @local is used only to convert relevant internal structures which refer to remote vs local - /// to decide value of outputs and direction of HTLCs. - /// @generated_by_local is used to determine *which* HTLCs to include - noting that the HTLC - /// state may indicate that one peer has informed the other that they'd like to add an HTLC but - /// have not yet committed it. Such HTLCs will only be included in transactions which are being - /// generated by the peer which proposed adding the HTLCs, and thus we need to understand both - /// which peer generated this transaction and "to whom" this transaction flows. + /// Builds stats on a potential commitment transaction build, without actually building the + /// commitment transaction. See `build_commitment_transaction` for further docs. #[inline] - fn build_commitment_transaction(&self, funding: &FundingScope, commitment_number: u64, per_commitment_point: &PublicKey, local: bool, generated_by_local: bool, logger: &L) -> CommitmentData - where L::Target: Logger - { - let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new(); - let num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len(); - let mut included_non_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs); - - let broadcaster_dust_limit_satoshis = if local { self.holder_dust_limit_satoshis } else { self.counterparty_dust_limit_satoshis }; + fn build_commitment_stats(&self, funding: &FundingScope, local: bool, generated_by_local: bool) -> CommitmentStats { + let broadcaster_dust_limit_sat = if local { self.holder_dust_limit_satoshis } else { self.counterparty_dust_limit_satoshis }; + let mut non_dust_htlc_count = 0; let mut remote_htlc_total_msat = 0; let mut local_htlc_total_msat = 0; let mut value_to_self_msat_offset = 0; @@ -3783,122 +3875,31 @@ impl ChannelContext where SP::Target: SignerProvider { } } - log_trace!(logger, "Building commitment transaction number {} (really {} xor {}) for channel {} for {}, generated by {} with fee {}...", - commitment_number, (INITIAL_COMMITMENT_NUMBER - commitment_number), - get_commitment_transaction_number_obscure_factor(&funding.get_holder_pubkeys().payment_point, &funding.get_counterparty_pubkeys().payment_point, funding.is_outbound()), - &self.channel_id, - if local { "us" } else { "remote" }, if generated_by_local { "us" } else { "remote" }, feerate_per_kw); - - macro_rules! get_htlc_in_commitment { - ($htlc: expr, $offered: expr) => { - HTLCOutputInCommitment { - offered: $offered, - amount_msat: $htlc.amount_msat, - cltv_expiry: $htlc.cltv_expiry, - payment_hash: $htlc.payment_hash, - transaction_output_index: None - } - } - } - - macro_rules! add_htlc_output { - ($htlc: expr, $outbound: expr, $source: expr, $state_name: expr) => { - if $outbound == local { // "offered HTLC output" - let htlc_in_tx = get_htlc_in_commitment!($htlc, true); - let htlc_tx_fee = if funding.get_channel_type().supports_anchors_zero_fee_htlc_tx() { - 0 - } else { - feerate_per_kw as u64 * htlc_timeout_tx_weight(funding.get_channel_type()) / 1000 - }; - if $htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee { - log_trace!(logger, " ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat); - included_non_dust_htlcs.push((htlc_in_tx, $source)); - } else { - log_trace!(logger, " ...including {} {} dust HTLC {} (hash {}) with value {} due to dust limit", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat); - included_dust_htlcs.push((htlc_in_tx, $source)); - } - } else { - let htlc_in_tx = get_htlc_in_commitment!($htlc, false); - let htlc_tx_fee = if funding.get_channel_type().supports_anchors_zero_fee_htlc_tx() { - 0 - } else { - feerate_per_kw as u64 * htlc_success_tx_weight(funding.get_channel_type()) / 1000 - }; - if $htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee { - log_trace!(logger, " ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat); - included_non_dust_htlcs.push((htlc_in_tx, $source)); - } else { - log_trace!(logger, " ...including {} {} dust HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat); - included_dust_htlcs.push((htlc_in_tx, $source)); - } + for htlc in self.pending_inbound_htlcs.iter() { + if htlc.state.included_in_commitment(generated_by_local) { + if !htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) { + non_dust_htlc_count += 1; } - } - } - - let mut inbound_htlc_preimages: Vec = Vec::new(); - - for ref htlc in self.pending_inbound_htlcs.iter() { - let (include, state_name) = match htlc.state { - InboundHTLCState::RemoteAnnounced(_) => (!generated_by_local, "RemoteAnnounced"), - InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => (!generated_by_local, "AwaitingRemoteRevokeToAnnounce"), - InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => (true, "AwaitingAnnouncedRemoteRevoke"), - InboundHTLCState::Committed => (true, "Committed"), - InboundHTLCState::LocalRemoved(_) => (!generated_by_local, "LocalRemoved"), - }; - - if include { - add_htlc_output!(htlc, false, None, state_name); remote_htlc_total_msat += htlc.amount_msat; } else { - log_trace!(logger, " ...not including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name); - match &htlc.state { - &InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(preimage)) => { - inbound_htlc_preimages.push(preimage); - value_to_self_msat_offset += htlc.amount_msat as i64; - }, - _ => {}, + if htlc.state.preimage().is_some() { + value_to_self_msat_offset += htlc.amount_msat as i64; } } - } - - - let mut outbound_htlc_preimages: Vec = Vec::new(); - - for ref htlc in self.pending_outbound_htlcs.iter() { - let (include, state_name) = match htlc.state { - OutboundHTLCState::LocalAnnounced(_) => (generated_by_local, "LocalAnnounced"), - OutboundHTLCState::Committed => (true, "Committed"), - OutboundHTLCState::RemoteRemoved(_) => (generated_by_local, "RemoteRemoved"), - OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) => (generated_by_local, "AwaitingRemoteRevokeToRemove"), - OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) => (false, "AwaitingRemovedRemoteRevoke"), - }; - - let preimage_opt = match htlc.state { - OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(p)) => p, - OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(p)) => p, - OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(p)) => p, - _ => None, - }; - - if let Some(preimage) = preimage_opt { - outbound_htlc_preimages.push(preimage); - } + }; - if include { - add_htlc_output!(htlc, true, Some(&htlc.source), state_name); + for htlc in self.pending_outbound_htlcs.iter() { + if htlc.state.included_in_commitment(generated_by_local) { + if !htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) { + non_dust_htlc_count += 1; + } local_htlc_total_msat += htlc.amount_msat; } else { - log_trace!(logger, " ...not including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name); - match htlc.state { - OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) | - OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) | - OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(_)) => { - value_to_self_msat_offset -= htlc.amount_msat as i64; - }, - _ => {}, + if htlc.state.preimage().is_some() { + value_to_self_msat_offset -= htlc.amount_msat as i64; } } - } + }; // # Panics // @@ -3927,31 +3928,129 @@ impl ChannelContext where SP::Target: SignerProvider { broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat); } + let total_fee_sat = commit_tx_fee_sat(feerate_per_kw, non_dust_htlc_count, &funding.channel_transaction_parameters.channel_type_features); + let total_anchors_sat = if funding.channel_transaction_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 }; + + CommitmentStats { + feerate_per_kw, + total_fee_sat, + total_anchors_sat, + broadcaster_dust_limit_sat, + local_balance_before_fee_anchors_msat: value_to_self_msat, + remote_balance_before_fee_anchors_msat: value_to_remote_msat, + } + } + + /// Transaction nomenclature is somewhat confusing here as there are many different cases - a + /// transaction is referred to as "a's transaction" implying that a will be able to broadcast + /// the transaction. Thus, b will generally be sending a signature over such a transaction to + /// a, and a can revoke the transaction by providing b the relevant per_commitment_secret. As + /// such, a transaction is generally the result of b increasing the amount paid to a (or adding + /// an HTLC to a). + /// @local is used only to convert relevant internal structures which refer to remote vs local + /// to decide value of outputs and direction of HTLCs. + /// @generated_by_local is used to determine *which* HTLCs to include - noting that the HTLC + /// state may indicate that one peer has informed the other that they'd like to add an HTLC but + /// have not yet committed it. Such HTLCs will only be included in transactions which are being + /// generated by the peer which proposed adding the HTLCs, and thus we need to understand both + /// which peer generated this transaction and "to whom" this transaction flows. + #[inline] + fn build_commitment_transaction(&self, funding: &FundingScope, commitment_number: u64, per_commitment_point: &PublicKey, local: bool, generated_by_local: bool, logger: &L) -> CommitmentData + where L::Target: Logger + { + let stats = self.build_commitment_stats(funding, local, generated_by_local); + let CommitmentStats { + feerate_per_kw, + total_fee_sat, + total_anchors_sat, + broadcaster_dust_limit_sat, + local_balance_before_fee_anchors_msat, + remote_balance_before_fee_anchors_msat + } = stats; + + let num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len(); + let mut htlcs_included: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs); + let mut nondust_htlcs: Vec = Vec::with_capacity(num_htlcs); + + log_trace!(logger, "Building commitment transaction number {} (really {} xor {}) for channel {} for {}, generated by {} with fee {}...", + commitment_number, (INITIAL_COMMITMENT_NUMBER - commitment_number), + get_commitment_transaction_number_obscure_factor(&funding.get_holder_pubkeys().payment_point, &funding.get_counterparty_pubkeys().payment_point, funding.is_outbound()), + self.channel_id, + if local { "us" } else { "remote" }, if generated_by_local { "us" } else { "remote" }, feerate_per_kw); + + macro_rules! get_htlc_in_commitment { + ($htlc: expr, $offered: expr) => { + HTLCOutputInCommitment { + offered: $offered, + amount_msat: $htlc.amount_msat, + cltv_expiry: $htlc.cltv_expiry, + payment_hash: $htlc.payment_hash, + transaction_output_index: None + } + } + } + + macro_rules! add_htlc_output { + ($htlc: expr, $outbound: expr, $source: expr) => { + let htlc_in_tx = get_htlc_in_commitment!($htlc, $outbound == local); + htlcs_included.push((htlc_in_tx.clone(), $source)); + if $htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) { + log_trace!(logger, " ...including {} {} dust HTLC {} (hash {}) with value {} due to dust limit", if $outbound { "outbound" } else { "inbound" }, $htlc.state, $htlc.htlc_id, $htlc.payment_hash, $htlc.amount_msat); + } else { + log_trace!(logger, " ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $htlc.state, $htlc.htlc_id, $htlc.payment_hash, $htlc.amount_msat); + nondust_htlcs.push(htlc_in_tx); + } + } + } + + let mut inbound_htlc_preimages: Vec = Vec::new(); + let mut outbound_htlc_preimages: Vec = Vec::new(); + + for htlc in self.pending_inbound_htlcs.iter() { + if htlc.state.included_in_commitment(generated_by_local) { + add_htlc_output!(htlc, false, None); + } else { + log_trace!(logger, " ...not including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, htlc.payment_hash, htlc.amount_msat, htlc.state); + if let Some(preimage) = htlc.state.preimage() { + inbound_htlc_preimages.push(preimage); + } + } + }; + + for htlc in self.pending_outbound_htlcs.iter() { + if let Some(preimage) = htlc.state.preimage() { + outbound_htlc_preimages.push(preimage); + } + if htlc.state.included_in_commitment(generated_by_local) { + add_htlc_output!(htlc, true, Some(&htlc.source)); + } else { + log_trace!(logger, " ...not including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, htlc.payment_hash, htlc.amount_msat, htlc.state); + } + }; + // We MUST use saturating subs here, as the funder's balance is not guaranteed to be greater - // than or equal to the sum of `total_fee_sat` and `anchors_val`. + // than or equal to the sum of `total_fee_sat` and `total_anchors_sat`. // // This is because when the remote party sends an `update_fee` message, we build the new // commitment transaction *before* checking whether the remote party's balance is enough to // cover the total fee and the anchors. - let total_fee_sat = commit_tx_fee_sat(feerate_per_kw, included_non_dust_htlcs.len(), funding.get_channel_type()); - let anchors_val = if funding.get_channel_type().supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 }; let (value_to_self, value_to_remote) = if funding.is_outbound() { - ((value_to_self_msat / 1000).saturating_sub(anchors_val).saturating_sub(total_fee_sat), value_to_remote_msat / 1000) + ((local_balance_before_fee_anchors_msat / 1000).saturating_sub(total_anchors_sat).saturating_sub(total_fee_sat), remote_balance_before_fee_anchors_msat / 1000) } else { - (value_to_self_msat / 1000, (value_to_remote_msat / 1000).saturating_sub(anchors_val).saturating_sub(total_fee_sat)) + (local_balance_before_fee_anchors_msat / 1000, (remote_balance_before_fee_anchors_msat / 1000).saturating_sub(total_anchors_sat).saturating_sub(total_fee_sat)) }; let mut to_broadcaster_value_sat = if local { value_to_self } else { value_to_remote }; let mut to_countersignatory_value_sat = if local { value_to_remote } else { value_to_self }; - if to_broadcaster_value_sat >= broadcaster_dust_limit_satoshis { + if to_broadcaster_value_sat >= broadcaster_dust_limit_sat { log_trace!(logger, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, to_broadcaster_value_sat); } else { to_broadcaster_value_sat = 0; } - if to_countersignatory_value_sat >= broadcaster_dust_limit_satoshis { + if to_countersignatory_value_sat >= broadcaster_dust_limit_sat { log_trace!(logger, " ...including {} output with value {}", if local { "to_remote" } else { "to_local" }, to_countersignatory_value_sat); } else { to_countersignatory_value_sat = 0; @@ -3960,28 +4059,48 @@ impl ChannelContext where SP::Target: SignerProvider { let channel_parameters = if local { funding.channel_transaction_parameters.as_holder_broadcastable() } else { funding.channel_transaction_parameters.as_counterparty_broadcastable() }; - let tx = CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number, - per_commitment_point, - to_broadcaster_value_sat, - to_countersignatory_value_sat, - feerate_per_kw, - &mut included_non_dust_htlcs, - &channel_parameters, - &self.secp_ctx, + let tx = CommitmentTransaction::new( + commitment_number, + per_commitment_point, + to_broadcaster_value_sat, + to_countersignatory_value_sat, + feerate_per_kw, + &nondust_htlcs, + &channel_parameters, + &self.secp_ctx, ); - let mut htlcs_included = included_non_dust_htlcs; - // The unwrap is safe, because all non-dust HTLCs have been assigned an output index - htlcs_included.sort_unstable_by_key(|h| h.0.transaction_output_index.unwrap()); - htlcs_included.append(&mut included_dust_htlcs); - let stats = CommitmentStats { - tx, - total_fee_sat, - local_balance_before_fee_msat: value_to_self_msat, - remote_balance_before_fee_msat: value_to_remote_msat, - }; + // This populates the htlc-source table with the indices from the htlcs in the commitment + // transaction. + // + // This brute-force search is O(n^2) over ~1k HTLCs in the worst case. This case is very + // rare (non-existent?) at the moment. + for nondust_htlc in tx.nondust_htlcs() { + let htlc = htlcs_included + .iter_mut() + .filter(|(htlc, _source)| htlc.transaction_output_index.is_none()) + .filter_map(|(htlc, _source)| { + if htlc.is_data_equal(nondust_htlc) { + Some(htlc) + } else { + None + } + }) + .nth(0) + .unwrap(); + htlc.transaction_output_index = Some(nondust_htlc.transaction_output_index.unwrap()); + } + htlcs_included.sort_unstable_by(|(htlc_a, _), (htlc_b, _)| { + match (htlc_a.transaction_output_index, htlc_b.transaction_output_index) { + // `None` is smaller than `Some`, but we want `Some` ordered before `None` in the vector + (None, Some(_)) => cmp::Ordering::Greater, + (Some(_), None) => cmp::Ordering::Less, + (l, r) => cmp::Ord::cmp(&l, &r), + } + }); CommitmentData { + tx, stats, htlcs_included, inbound_htlc_preimages, @@ -4773,7 +4892,7 @@ impl ChannelContext where SP::Target: SignerProvider { let commitment_data = self.build_commitment_transaction(funding, self.cur_counterparty_commitment_transaction_number, &self.counterparty_cur_commitment_point.unwrap(), false, false, logger); - let counterparty_initial_commitment_tx = commitment_data.stats.tx; + let counterparty_initial_commitment_tx = commitment_data.tx; match self.holder_signer { // TODO (taproot|arik): move match into calling method for Taproot ChannelSignerType::Ecdsa(ref ecdsa) => { @@ -5711,6 +5830,7 @@ impl FundedChannel where #[inline] fn mark_outbound_htlc_removed(&mut self, htlc_id: u64, check_preimage: Option, fail_reason: Option) -> Result<&OutboundHTLCOutput, ChannelError> { assert!(!(check_preimage.is_some() && fail_reason.is_some()), "cannot fail while we have a preimage"); + assert!(!(check_preimage.is_none() && fail_reason.is_none()), "success states must hold their corresponding preimage"); for htlc in self.context.pending_outbound_htlcs.iter_mut() { if htlc.htlc_id == htlc_id { let outcome = match check_preimage { @@ -6557,8 +6677,8 @@ impl FundedChannel where &self.funding, self.holder_commitment_point.transaction_number(), &self.holder_commitment_point.current_point(), true, true, logger, ); - let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_data.stats.tx.htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.funding.get_channel_type()) * 1000; - let holder_balance_msat = commitment_data.stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat; + let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_data.tx.nondust_htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.funding.get_channel_type()) * 1000; + let holder_balance_msat = commitment_data.stats.local_balance_before_fee_anchors_msat - htlc_stats.outbound_holding_cell_msat; if holder_balance_msat < buffer_fee_msat + self.funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 { //TODO: auto-close after a number of failures? log_debug!(logger, "Cannot afford to send new feerate at {}", feerate_per_kw); @@ -6872,7 +6992,7 @@ impl FundedChannel where let commitment_data = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number + 1, &self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger); - let counterparty_initial_commitment_tx = commitment_data.stats.tx; + let counterparty_initial_commitment_tx = commitment_data.tx; self.context.get_funding_signed_msg(&self.funding.channel_transaction_parameters, logger, counterparty_initial_commitment_tx) } else { None }; // Provide a `channel_ready` message if we need to, but only if we're _not_ still pending @@ -8942,10 +9062,10 @@ impl FundedChannel where let mut updates = Vec::with_capacity(self.pending_funding.len() + 1); for funding in core::iter::once(&self.funding).chain(self.pending_funding.iter()) { - let (mut htlcs_ref, counterparty_commitment_tx) = + let (htlcs_ref, counterparty_commitment_tx) = self.build_commitment_no_state_update(funding, logger); let htlc_outputs: Vec<(HTLCOutputInCommitment, Option>)> = - htlcs_ref.drain(..).map(|(htlc, htlc_source)| (htlc, htlc_source.map(|source_ref| Box::new(source_ref.clone())))).collect(); + htlcs_ref.into_iter().map(|(htlc, htlc_source)| (htlc, htlc_source.map(|source_ref| Box::new(source_ref.clone())))).collect(); if self.pending_funding.is_empty() { // Soon, we will switch this to `LatestCounterpartyCommitmentTX`, @@ -8991,7 +9111,7 @@ impl FundedChannel where funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger, ); - let counterparty_commitment_tx = commitment_data.stats.tx; + let counterparty_commitment_tx = commitment_data.tx; #[cfg(any(test, fuzzing))] { @@ -9004,7 +9124,7 @@ impl FundedChannel where && info.next_holder_htlc_id == self.context.next_holder_htlc_id && info.next_counterparty_htlc_id == self.context.next_counterparty_htlc_id && info.feerate == self.context.feerate_per_kw { - let actual_fee = commit_tx_fee_sat(self.context.feerate_per_kw, counterparty_commitment_tx.htlcs().len(), self.funding.get_channel_type()) * 1000; + let actual_fee = commit_tx_fee_sat(self.context.feerate_per_kw, counterparty_commitment_tx.nondust_htlcs().len(), self.funding.get_channel_type()) * 1000; assert_eq!(actual_fee, info.fee); } } @@ -9042,7 +9162,7 @@ impl FundedChannel where funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger, ); - let counterparty_commitment_tx = commitment_data.stats.tx; + let counterparty_commitment_tx = commitment_data.tx; match &self.context.holder_signer { ChannelSignerType::Ecdsa(ecdsa) => { @@ -9066,8 +9186,8 @@ impl FundedChannel where log_bytes!(signature.serialize_compact()[..]), &self.context.channel_id()); let counterparty_keys = trusted_tx.keys(); - debug_assert_eq!(htlc_signatures.len(), trusted_tx.htlcs().len()); - for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(trusted_tx.htlcs()) { + debug_assert_eq!(htlc_signatures.len(), trusted_tx.nondust_htlcs().len()); + for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(trusted_tx.nondust_htlcs()) { log_trace!(logger, "Signed remote HTLC tx {} with redeemscript {} with pubkey {} -> {} in channel {}", encode::serialize_hex(&chan_utils::build_htlc_transaction(&trusted_tx.txid(), trusted_tx.feerate_per_kw(), funding.get_holder_selected_contest_delay(), htlc, funding.get_channel_type(), &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)), encode::serialize_hex(&chan_utils::get_htlc_redeemscript(&htlc, funding.get_channel_type(), &counterparty_keys)), @@ -9535,7 +9655,7 @@ impl OutboundV1Channel where SP::Target: SignerProvider { let commitment_data = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger); - let counterparty_initial_commitment_tx = commitment_data.stats.tx; + let counterparty_initial_commitment_tx = commitment_data.tx; let signature = match &self.context.holder_signer { // TODO (taproot|arik): move match into calling method for Taproot ChannelSignerType::Ecdsa(ecdsa) => { @@ -10577,6 +10697,7 @@ impl Writeable for FundedChannel where SP::Target: SignerProvider &OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref outcome) => { 3u8.write(writer)?; if let OutboundHTLCOutcome::Success(preimage) = outcome { + debug_assert!(preimage.is_some(), "success states must hold their corresponding preimage"); preimages.push(preimage); } let reason: Option<&HTLCFailReason> = outcome.into(); @@ -10585,6 +10706,7 @@ impl Writeable for FundedChannel where SP::Target: SignerProvider &OutboundHTLCState::AwaitingRemovedRemoteRevoke(ref outcome) => { 4u8.write(writer)?; if let OutboundHTLCOutcome::Success(preimage) = outcome { + debug_assert!(preimage.is_some(), "success states must hold their corresponding preimage"); preimages.push(preimage); } let reason: Option<&HTLCFailReason> = outcome.into(); @@ -11079,7 +11201,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel // only, so we default to that if none was written. let mut channel_type = Some(ChannelTypeFeatures::only_static_remote_key()); let mut channel_creation_height = 0u32; - let mut preimages_opt: Option>> = None; + let mut preimages: Vec> = Vec::new(); // If we read an old Channel, for simplicity we just treat it as "we never sent an // AnnouncementSignatures" which implies we'll re-send it on reconnect, but that's fine. @@ -11133,7 +11255,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel (10, monitor_pending_update_adds, option), // Added in 0.0.122 (11, monitor_pending_finalized_fulfills, optional_vec), (13, channel_creation_height, required), - (15, preimages_opt, optional_vec), + (15, preimages, required_vec), // The preimages transitioned from optional to required in 0.2 (17, announcement_sigs_state, required), (19, latest_inbound_scid_alias, option), (21, outbound_scid_alias, required), @@ -11161,23 +11283,21 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel let holder_signer = signer_provider.derive_channel_signer(channel_keys_id); - if let Some(preimages) = preimages_opt { - let mut iter = preimages.into_iter(); - for htlc in pending_outbound_htlcs.iter_mut() { - match &htlc.state { - OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(None)) => { - htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(iter.next().ok_or(DecodeError::InvalidValue)?)); - } - OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(None)) => { - htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(iter.next().ok_or(DecodeError::InvalidValue)?)); - } - _ => {} + let mut iter = preimages.into_iter(); + for htlc in pending_outbound_htlcs.iter_mut() { + match &htlc.state { + OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(None)) => { + htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(iter.next().ok_or(DecodeError::InvalidValue)?)); } + OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(None)) => { + htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(iter.next().ok_or(DecodeError::InvalidValue)?)); + } + _ => {} } - // We expect all preimages to be consumed above - if iter.next().is_some() { - return Err(DecodeError::InvalidValue); - } + } + // We expect all preimages to be consumed above + if iter.next().is_some() { + return Err(DecodeError::InvalidValue); } let chan_features = channel_type.unwrap(); @@ -12265,7 +12385,7 @@ mod tests { } ) => { { let commitment_data = chan.context.build_commitment_transaction(&chan.funding, 0xffffffffffff - 42, &per_commitment_point, true, false, &logger); - let commitment_tx = commitment_data.stats.tx; + let commitment_tx = commitment_data.tx; let trusted_tx = commitment_tx.trust(); let unsigned_tx = trusted_tx.built_transaction(); let redeemscript = chan.funding.get_funding_redeemscript(); @@ -12280,10 +12400,10 @@ mod tests { counterparty_htlc_sigs.clear(); // Don't warn about excess mut for no-HTLC calls $({ let remote_signature = Signature::from_der(&>::from_hex($counterparty_htlc_sig_hex).unwrap()[..]).unwrap(); - per_htlc.push((commitment_tx.htlcs()[$htlc_idx].clone(), Some(remote_signature))); + per_htlc.push((commitment_tx.nondust_htlcs()[$htlc_idx].clone(), Some(remote_signature))); counterparty_htlc_sigs.push(remote_signature); })* - assert_eq!(commitment_tx.htlcs().len(), per_htlc.len()); + assert_eq!(commitment_tx.nondust_htlcs().len(), per_htlc.len()); let holder_commitment_tx = HolderCommitmentTransaction::new( commitment_tx.clone(), @@ -12306,7 +12426,7 @@ mod tests { log_trace!(logger, "verifying htlc {}", $htlc_idx); let remote_signature = Signature::from_der(&>::from_hex($counterparty_htlc_sig_hex).unwrap()[..]).unwrap(); - let ref htlc = commitment_tx.htlcs()[$htlc_idx]; + let ref htlc = commitment_tx.nondust_htlcs()[$htlc_idx]; let keys = commitment_tx.trust().keys(); let mut htlc_tx = chan_utils::build_htlc_transaction(&unsigned_tx.txid, chan.context.feerate_per_kw, chan.funding.get_counterparty_selected_contest_delay().unwrap(), diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 4aaa1a3f31e..0e4d5384b4e 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -746,14 +746,14 @@ pub fn test_update_fee_that_funder_cannot_afford() { let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); let local_chan = local_chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap(); let local_chan_signer = local_chan.get_signer(); - let mut htlcs: Vec<(HTLCOutputInCommitment, ())> = vec![]; - let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data( + let nondust_htlcs: Vec = vec![]; + let commitment_tx = CommitmentTransaction::new( INITIAL_COMMITMENT_NUMBER - 1, &remote_point, push_sats, channel_value - push_sats - commit_tx_fee_msat(non_buffer_feerate + 4, 0, &channel_type_features) / 1000, non_buffer_feerate + 4, - &mut htlcs, + &nondust_htlcs, &local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable(), &secp_ctx, ); @@ -831,8 +831,8 @@ pub fn test_update_fee_that_saturates_subs() { let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); let local_chan = local_chan_lock.channel_by_id.get(&chan_id).and_then(Channel::as_funded).unwrap(); let local_chan_signer = local_chan.get_signer(); - let mut htlcs: Vec<(HTLCOutputInCommitment, ())> = vec![]; - let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data( + let nondust_htlcs: Vec = vec![]; + let commitment_tx = CommitmentTransaction::new( INITIAL_COMMITMENT_NUMBER, &remote_point, 8500, @@ -840,7 +840,7 @@ pub fn test_update_fee_that_saturates_subs() { // he will do a saturating subtraction of the total fees from node 0's balance. 0, FEERATE, - &mut htlcs, + &nondust_htlcs, &local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable(), &secp_ctx, ); @@ -1565,13 +1565,13 @@ pub fn test_fee_spike_violation_fails_htlc() { let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); let local_chan = local_chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap(); let local_chan_signer = local_chan.get_signer(); - let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data( + let commitment_tx = CommitmentTransaction::new( commitment_number, &remote_point, 95000, local_chan_balance, feerate_per_kw, - &mut vec![(accepted_htlc_info, ())], + &vec![accepted_htlc_info], &local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable(), &secp_ctx, ); diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs index df79df6bab8..eb3d57e6dec 100644 --- a/lightning/src/sign/mod.rs +++ b/lightning/src/sign/mod.rs @@ -1364,8 +1364,8 @@ impl EcdsaChannelSigner for InMemorySigner { ); let commitment_txid = built_tx.txid; - let mut htlc_sigs = Vec::with_capacity(commitment_tx.htlcs().len()); - for htlc in commitment_tx.htlcs() { + let mut htlc_sigs = Vec::with_capacity(commitment_tx.nondust_htlcs().len()); + for htlc in commitment_tx.nondust_htlcs() { let holder_selected_contest_delay = channel_parameters.holder_selected_contest_delay; let chan_type = &channel_parameters.channel_type_features; let htlc_tx = chan_utils::build_htlc_transaction(