diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index f709183f2d1..a74cb7d7685 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(); @@ -3098,8 +3098,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()); @@ -3109,7 +3109,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`. @@ -3129,7 +3129,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; } @@ -3137,10 +3137,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)); @@ -3955,9 +3955,9 @@ impl ChannelMonitorImpl { &self, holder_tx: &HolderCommitmentTransaction, ) -> Vec { let tx = holder_tx.trust(); - let mut htlcs = Vec::with_capacity(holder_tx.htlcs().len()); - debug_assert_eq!(holder_tx.htlcs().len(), holder_tx.counterparty_htlc_sigs.len()); - for (htlc, counterparty_sig) in holder_tx.htlcs().iter().zip(holder_tx.counterparty_htlc_sigs.iter()) { + let mut htlcs = Vec::with_capacity(holder_tx.nondust_htlcs().len()); + debug_assert_eq!(holder_tx.nondust_htlcs().len(), holder_tx.counterparty_htlc_sigs.len()); + for (htlc, counterparty_sig) in holder_tx.nondust_htlcs().iter().zip(holder_tx.counterparty_htlc_sigs.iter()) { assert!(htlc.transaction_output_index.is_some(), "Expected transaction output index for non-dust HTLC"); let preimage = if htlc.offered { @@ -4026,9 +4026,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, @@ -4121,7 +4121,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); @@ -4135,7 +4135,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); diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 645296d6411..92a48034b1d 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -688,7 +688,7 @@ impl OnchainTxHandler { OnchainClaim::Event(ClaimEvent::BumpCommitment { package_target_feerate_sat_per_1000_weight, commitment_tx: tx, - pending_nondust_htlcs: holder_commitment.htlcs().to_vec(), + pending_nondust_htlcs: holder_commitment.nondust_htlcs().to_vec(), commitment_tx_fee_satoshis: fee_sat, anchor_output_idx: idx, channel_parameters: channel_parameters.clone(), @@ -1339,7 +1339,7 @@ mod tests { let holder_commit = tx_handler.current_holder_commitment_tx(); let holder_commit_txid = holder_commit.trust().txid(); let mut requests = Vec::new(); - for (htlc, counterparty_sig) in holder_commit.htlcs().iter().zip(holder_commit.counterparty_htlc_sigs.iter()) { + for (htlc, counterparty_sig) in holder_commit.nondust_htlcs().iter().zip(holder_commit.counterparty_htlc_sigs.iter()) { requests.push(PackageTemplate::build_package( holder_commit_txid, htlc.transaction_output_index.unwrap(), diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 14a884ca6f6..cee72e081a6 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -472,7 +472,7 @@ impl HolderHTLCOutput { } let (htlc, counterparty_sig) = - trusted_tx.htlcs().iter().zip(holder_commitment.counterparty_htlc_sigs.iter()) + trusted_tx.nondust_htlcs().iter().zip(holder_commitment.counterparty_htlc_sigs.iter()) .find(|(htlc, _)| htlc.transaction_output_index.unwrap() == outp.vout) .unwrap(); diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 8d59304d566..c978680a620 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -1430,7 +1430,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 +1446,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 +1468,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 +1486,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,7 +1503,7 @@ 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()) }) } @@ -1526,7 +1526,7 @@ impl CommitmentTransaction { 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::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters); let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(commitment_number, channel_parameters); let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs); @@ -1537,7 +1537,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 { @@ -1558,7 +1558,7 @@ impl CommitmentTransaction { 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); - let mut htlcs_with_aux = self.htlcs.iter().map(|h| (h.clone(), ())).collect(); + let mut htlcs_with_aux = self.nondust_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 transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs); @@ -1653,7 +1653,7 @@ impl CommitmentTransaction { } } - let mut htlcs = Vec::with_capacity(htlcs_with_aux.len()); + let mut nondust_htlcs = Vec::with_capacity(htlcs_with_aux.len()); for (htlc, _) in htlcs_with_aux { let script = get_htlc_redeemscript(htlc, channel_type, keys); let txout = TxOut { @@ -1683,11 +1683,11 @@ impl CommitmentTransaction { 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()); + nondust_htlcs.push(htlc.clone()); } outputs.push(out.0); } - (outputs, htlcs) + (outputs, nondust_htlcs) } fn internal_build_inputs(commitment_number: u64, channel_parameters: &DirectedChannelTransactionParameters) -> (u64, Vec) { @@ -1746,8 +1746,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. @@ -1831,10 +1831,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); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index d5cccc0bc1f..4b60b83aaf1 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,23 +339,50 @@ impl From<&OutboundHTLCState> for OutboundHTLCStateDetails { } } -#[derive(Clone)] -#[cfg_attr(test, derive(Debug, PartialEq))] -enum OutboundHTLCOutcome { - /// LDK version 0.0.105+ will always fill in the preimage here. - Success(Option), - Failure(HTLCFailReason), +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 From> for OutboundHTLCOutcome { - fn from(o: Option) -> Self { - match o { - None => OutboundHTLCOutcome::Success(None), - Some(r) => OutboundHTLCOutcome::Failure(r) +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)) => { + Some(*preimage) + }, + _ => None, } } } +#[derive(Clone)] +#[cfg_attr(test, derive(Debug, PartialEq))] +enum OutboundHTLCOutcome { + /// 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(PaymentPreimage), + Failure(HTLCFailReason), +} + impl<'a> Into> for &'a OutboundHTLCOutcome { fn into(self) -> Option<&'a HTLCFailReason> { match self { @@ -329,6 +405,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 +977,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 +986,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 +2221,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 +2261,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 +3743,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 +3756,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 +3765,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 +3787,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 +3811,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 +3840,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 +3863,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 +3916,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 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); + + 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); + 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); + included_dust_htlcs.push((htlc_in_tx, $source)); + } 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); + included_non_dust_htlcs.push((htlc_in_tx, $source)); + } + } + } + + 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; @@ -3974,14 +4061,8 @@ impl ChannelContext where SP::Target: SignerProvider { 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, - }; - CommitmentData { + tx, stats, htlcs_included, inbound_htlc_preimages, @@ -4773,7 +4854,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) => { @@ -5709,20 +5790,15 @@ impl FundedChannel where /// Marks an outbound HTLC which we have received update_fail/fulfill/malformed #[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"); + fn mark_outbound_htlc_removed(&mut self, htlc_id: u64, outcome: OutboundHTLCOutcome) -> Result<&OutboundHTLCOutput, ChannelError> { for htlc in self.context.pending_outbound_htlcs.iter_mut() { if htlc.htlc_id == htlc_id { - let outcome = match check_preimage { - None => fail_reason.into(), - Some(payment_preimage) => { - let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).to_byte_array()); - if payment_hash != htlc.payment_hash { - return Err(ChannelError::close(format!("Remote tried to fulfill HTLC ({}) with an incorrect preimage", htlc_id))); - } - OutboundHTLCOutcome::Success(Some(payment_preimage)) + if let OutboundHTLCOutcome::Success(ref payment_preimage) = outcome { + let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).to_byte_array()); + if payment_hash != htlc.payment_hash { + return Err(ChannelError::close(format!("Remote tried to fulfill HTLC ({}) with an incorrect preimage", htlc_id))); } - }; + } match htlc.state { OutboundHTLCState::LocalAnnounced(_) => return Err(ChannelError::close(format!("Remote tried to fulfill/fail HTLC ({}) before it had been committed", htlc_id))), @@ -5749,7 +5825,7 @@ impl FundedChannel where return Err(ChannelError::close("Peer sent update_fulfill_htlc when we needed a channel_reestablish".to_owned())); } - self.mark_outbound_htlc_removed(msg.htlc_id, Some(msg.payment_preimage), None).map(|htlc| (htlc.source.clone(), htlc.amount_msat, htlc.skimmed_fee_msat)) + self.mark_outbound_htlc_removed(msg.htlc_id, OutboundHTLCOutcome::Success(msg.payment_preimage)).map(|htlc| (htlc.source.clone(), htlc.amount_msat, htlc.skimmed_fee_msat)) } pub fn update_fail_htlc(&mut self, msg: &msgs::UpdateFailHTLC, fail_reason: HTLCFailReason) -> Result<(), ChannelError> { @@ -5763,7 +5839,7 @@ impl FundedChannel where return Err(ChannelError::close("Peer sent update_fail_htlc when we needed a channel_reestablish".to_owned())); } - self.mark_outbound_htlc_removed(msg.htlc_id, None, Some(fail_reason))?; + self.mark_outbound_htlc_removed(msg.htlc_id, OutboundHTLCOutcome::Failure(fail_reason))?; Ok(()) } @@ -5778,7 +5854,7 @@ impl FundedChannel where return Err(ChannelError::close("Peer sent update_fail_malformed_htlc when we needed a channel_reestablish".to_owned())); } - self.mark_outbound_htlc_removed(msg.htlc_id, None, Some(fail_reason))?; + self.mark_outbound_htlc_removed(msg.htlc_id, OutboundHTLCOutcome::Failure(fail_reason))?; Ok(()) } @@ -5924,10 +6000,10 @@ impl FundedChannel where if let &mut OutboundHTLCState::RemoteRemoved(ref mut outcome) = &mut htlc.state { log_trace!(logger, "Updating HTLC {} to AwaitingRemoteRevokeToRemove due to commitment_signed in channel {}.", &htlc.payment_hash, &self.context.channel_id); - // Grab the preimage, if it exists, instead of cloning - let mut reason = OutboundHTLCOutcome::Success(None); + // Swap against a dummy variant to avoid a potentially expensive clone of `OutboundHTLCOutcome::Failure(HTLCFailReason)` + let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32])); mem::swap(outcome, &mut reason); - if let OutboundHTLCOutcome::Success(Some(preimage)) = reason { + if let OutboundHTLCOutcome::Success(preimage) = reason { // If a user (a) receives an HTLC claim using LDK 0.0.104 or before, then (b) // upgrades to LDK 0.0.114 or later before the HTLC is fully resolved, we could // have a `Success(None)` reason. In this case we could forget some HTLC @@ -6345,8 +6421,8 @@ impl FundedChannel where } if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut outcome) = &mut htlc.state { log_trace!(logger, " ...promoting outbound AwaitingRemoteRevokeToRemove {} to AwaitingRemovedRemoteRevoke", &htlc.payment_hash); - // Grab the preimage, if it exists, instead of cloning - let mut reason = OutboundHTLCOutcome::Success(None); + // Swap against a dummy variant to avoid a potentially expensive clone of `OutboundHTLCOutcome::Failure(HTLCFailReason)` + let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32])); mem::swap(outcome, &mut reason); htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(reason); require_commitment = true; @@ -6557,8 +6633,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 +6948,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 @@ -8921,8 +8997,8 @@ impl FundedChannel where for htlc in self.context.pending_outbound_htlcs.iter_mut() { if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut outcome) = &mut htlc.state { log_trace!(logger, " ...promoting outbound AwaitingRemoteRevokeToRemove {} to AwaitingRemovedRemoteRevoke", &htlc.payment_hash); - // Grab the preimage, if it exists, instead of cloning - let mut reason = OutboundHTLCOutcome::Success(None); + // Swap against a dummy variant to avoid a potentially expensive clone of `OutboundHTLCOutcome::Failure(HTLCFailReason)` + let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32])); mem::swap(outcome, &mut reason); htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(reason); } @@ -8939,10 +9015,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`, @@ -8988,7 +9064,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))] { @@ -9001,7 +9077,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); } } @@ -9039,7 +9115,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) => { @@ -9063,8 +9139,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)), @@ -9532,7 +9608,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) => { @@ -10547,7 +10623,9 @@ impl Writeable for FundedChannel where SP::Target: SignerProvider } } - let mut preimages: Vec<&Option> = vec![]; + // The elements of this vector will always be `Some` starting in 0.2, + // but we still serialize the option to maintain backwards compatibility + let mut preimages: Vec> = vec![]; let mut pending_outbound_skimmed_fees: Vec> = Vec::new(); let mut pending_outbound_blinding_points: Vec> = Vec::new(); @@ -10574,7 +10652,7 @@ impl Writeable for FundedChannel where SP::Target: SignerProvider &OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref outcome) => { 3u8.write(writer)?; if let OutboundHTLCOutcome::Success(preimage) = outcome { - preimages.push(preimage); + preimages.push(Some(preimage)); } let reason: Option<&HTLCFailReason> = outcome.into(); reason.write(writer)?; @@ -10582,7 +10660,7 @@ impl Writeable for FundedChannel where SP::Target: SignerProvider &OutboundHTLCState::AwaitingRemovedRemoteRevoke(ref outcome) => { 4u8.write(writer)?; if let OutboundHTLCOutcome::Success(preimage) = outcome { - preimages.push(preimage); + preimages.push(Some(preimage)); } let reason: Option<&HTLCFailReason> = outcome.into(); reason.write(writer)?; @@ -10921,15 +10999,30 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, &'c Channel 1 => OutboundHTLCState::Committed, 2 => { let option: Option = Readable::read(reader)?; - OutboundHTLCState::RemoteRemoved(option.into()) + let outcome = match option { + Some(r) => OutboundHTLCOutcome::Failure(r), + // Initialize this variant with a dummy preimage, the actual preimage will be filled in further down + None => OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32])), + }; + OutboundHTLCState::RemoteRemoved(outcome) }, 3 => { let option: Option = Readable::read(reader)?; - OutboundHTLCState::AwaitingRemoteRevokeToRemove(option.into()) + let outcome = match option { + Some(r) => OutboundHTLCOutcome::Failure(r), + // Initialize this variant with a dummy preimage, the actual preimage will be filled in further down + None => OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32])), + }; + OutboundHTLCState::AwaitingRemoteRevokeToRemove(outcome) }, 4 => { let option: Option = Readable::read(reader)?; - OutboundHTLCState::AwaitingRemovedRemoteRevoke(option.into()) + let outcome = match option { + Some(r) => OutboundHTLCOutcome::Failure(r), + // Initialize this variant with a dummy preimage, the actual preimage will be filled in further down + None => OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32])), + }; + OutboundHTLCState::AwaitingRemovedRemoteRevoke(outcome) }, _ => return Err(DecodeError::InvalidValue), }, @@ -11076,7 +11169,9 @@ 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; + // Starting in 0.2, all the elements in this vector will be `Some`, but they are still + // serialized as options to maintain backwards compatibility + 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. @@ -11130,7 +11225,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), @@ -11158,23 +11253,27 @@ 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 &mut htlc.state { + OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(ref mut preimage)) => { + // This variant was initialized like this further above + debug_assert_eq!(preimage, &PaymentPreimage([0u8; 32])); + // Flatten and unwrap the preimage; they are always set starting in 0.2. + *preimage = iter.next().flatten().ok_or(DecodeError::InvalidValue)?; } + OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(ref mut preimage)) => { + // This variant was initialized like this further above + debug_assert_eq!(preimage, &PaymentPreimage([0u8; 32])); + // Flatten and unwrap the preimage; they are always set starting in 0.2. + *preimage = iter.next().flatten().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(); @@ -12262,7 +12361,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(); @@ -12277,10 +12376,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(), @@ -12303,7 +12402,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/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(