Merge pull request #2686 from jkczyz/2023-10-onion-message-fuzz
[rust-lightning] / lightning / src / ln / channel.rs
index 6a468f2d6fe9f71209691edfa7f0318a349d084a..33a46084abb4d1bba4bf3de72168c4d1b3933b7d 100644 (file)
@@ -543,18 +543,17 @@ pub(super) struct ReestablishResponses {
        pub shutdown_msg: Option<msgs::Shutdown>,
 }
 
-/// The return type of `force_shutdown`
-///
-/// Contains a tuple with the following:
-/// - An optional (counterparty_node_id, funding_txo, [`ChannelMonitorUpdate`]) tuple
-/// - A list of HTLCs to fail back in the form of the (source, payment hash, and this channel's
-/// counterparty_node_id and channel_id).
-/// - An optional transaction id identifying a corresponding batch funding transaction.
-pub(crate) type ShutdownResult = (
-       Option<(PublicKey, OutPoint, ChannelMonitorUpdate)>,
-       Vec<(HTLCSource, PaymentHash, PublicKey, ChannelId)>,
-       Option<Txid>
-);
+/// The result of a shutdown that should be handled.
+#[must_use]
+pub(crate) struct ShutdownResult {
+       /// A channel monitor update to apply.
+       pub(crate) monitor_update: Option<(PublicKey, OutPoint, ChannelMonitorUpdate)>,
+       /// A list of dropped outbound HTLCs that can safely be failed backwards immediately.
+       pub(crate) dropped_outbound_htlcs: Vec<(HTLCSource, PaymentHash, PublicKey, ChannelId)>,
+       /// An unbroadcasted batch funding transaction id. The closure of this channel should be
+       /// propagated to the remainder of the batch.
+       pub(crate) unbroadcasted_batch_funding_txid: Option<Txid>,
+}
 
 /// If the majority of the channels funds are to the fundee and the initiator holds only just
 /// enough funds to cover their reserve value, channels are at risk of getting "stuck". Because the
@@ -724,7 +723,7 @@ pub(super) struct ChannelContext<SP: Deref> where SP::Target: SignerProvider {
 
        cur_holder_commitment_transaction_number: u64,
        cur_counterparty_commitment_transaction_number: u64,
-       value_to_self_msat: u64, // Excluding all pending_htlcs, excluding fees
+       value_to_self_msat: u64, // Excluding all pending_htlcs, fees, and anchor outputs
        pending_inbound_htlcs: Vec<InboundHTLCOutput>,
        pending_outbound_htlcs: Vec<OutboundHTLCOutput>,
        holding_cell_htlc_updates: Vec<HTLCUpdateAwaitingACK>,
@@ -1673,6 +1672,11 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 
                let mut available_capacity_msat = outbound_capacity_msat;
 
+               let anchor_outputs_value_msat = if context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
+                       ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000
+               } else {
+                       0
+               };
                if context.is_outbound() {
                        // We should mind channel commit tx fee when computing how much of the available capacity
                        // can be used in the next htlc. Mirrors the logic in send_htlc.
@@ -1687,14 +1691,19 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
                        }
 
                        let htlc_above_dust = HTLCCandidate::new(real_dust_limit_timeout_sat * 1000, HTLCInitiator::LocalOffered);
-                       let max_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * context.next_local_commit_tx_fee_msat(htlc_above_dust, Some(()));
+                       let mut max_reserved_commit_tx_fee_msat = context.next_local_commit_tx_fee_msat(htlc_above_dust, Some(()));
                        let htlc_dust = HTLCCandidate::new(real_dust_limit_timeout_sat * 1000 - 1, HTLCInitiator::LocalOffered);
-                       let min_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * context.next_local_commit_tx_fee_msat(htlc_dust, Some(()));
+                       let mut min_reserved_commit_tx_fee_msat = context.next_local_commit_tx_fee_msat(htlc_dust, Some(()));
+                       if !context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
+                               max_reserved_commit_tx_fee_msat *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE;
+                               min_reserved_commit_tx_fee_msat *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE;
+                       }
 
                        // We will first subtract the fee as if we were above-dust. Then, if the resulting
                        // value ends up being below dust, we have this fee available again. In that case,
                        // match the value to right-below-dust.
-                       let mut capacity_minus_commitment_fee_msat: i64 = (available_capacity_msat as i64) - (max_reserved_commit_tx_fee_msat as i64);
+                       let mut capacity_minus_commitment_fee_msat: i64 = available_capacity_msat as i64 -
+                               max_reserved_commit_tx_fee_msat as i64 - anchor_outputs_value_msat as i64;
                        if capacity_minus_commitment_fee_msat < (real_dust_limit_timeout_sat as i64) * 1000 {
                                let one_htlc_difference_msat = max_reserved_commit_tx_fee_msat - min_reserved_commit_tx_fee_msat;
                                debug_assert!(one_htlc_difference_msat != 0);
@@ -1719,7 +1728,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
                        let remote_balance_msat = (context.channel_value_satoshis * 1000 - context.value_to_self_msat)
                                .saturating_sub(inbound_stats.pending_htlcs_value_msat);
 
-                       if remote_balance_msat < max_reserved_commit_tx_fee_msat + holder_selected_chan_reserve_msat {
+                       if remote_balance_msat < max_reserved_commit_tx_fee_msat + holder_selected_chan_reserve_msat + anchor_outputs_value_msat {
                                // If another HTLC's fee would reduce the remote's balance below the reserve limit
                                // we've selected for them, we can only send dust HTLCs.
                                available_capacity_msat = cmp::min(available_capacity_msat, real_dust_limit_success_sat * 1000 - 1);
@@ -2064,7 +2073,11 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 
                self.channel_state = ChannelState::ShutdownComplete as u32;
                self.update_time_counter += 1;
-               (monitor_update, dropped_outbound_htlcs, unbroadcasted_batch_funding_txid)
+               ShutdownResult {
+                       monitor_update,
+                       dropped_outbound_htlcs,
+                       unbroadcasted_batch_funding_txid,
+               }
        }
 }
 
@@ -2119,7 +2132,7 @@ fn commit_tx_fee_sat(feerate_per_kw: u32, num_htlcs: usize, channel_type_feature
 
 // Get the fee cost in MSATS of a commitment tx with a given number of HTLC outputs.
 // Note that num_htlcs should not include dust HTLCs.
-fn commit_tx_fee_msat(feerate_per_kw: u32, num_htlcs: usize, channel_type_features: &ChannelTypeFeatures) -> u64 {
+pub(crate) fn commit_tx_fee_msat(feerate_per_kw: u32, num_htlcs: usize, channel_type_features: &ChannelTypeFeatures) -> u64 {
        // Note that we need to divide before multiplying to round properly,
        // since the lowest denomination of bitcoin on-chain is the satoshi.
        (commitment_tx_base_weight(channel_type_features) + num_htlcs as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC) * feerate_per_kw as u64 / 1000 * 1000
@@ -2766,6 +2779,7 @@ impl<SP: Deref> Channel<SP> where
                if inbound_stats.pending_htlcs_value_msat + msg.amount_msat > self.context.holder_max_htlc_value_in_flight_msat {
                        return Err(ChannelError::Close(format!("Remote HTLC add would put them over our max HTLC value ({})", self.context.holder_max_htlc_value_in_flight_msat)));
                }
+
                // Check holder_selected_channel_reserve_satoshis (we're getting paid, so they have to at least meet
                // the reserve_satoshis we told them to always have as direct payment so that they lose
                // something if we punish them for broadcasting an old state).
@@ -2825,30 +2839,40 @@ impl<SP: Deref> Channel<SP> where
 
                // Check that the remote can afford to pay for this HTLC on-chain at the current
                // feerate_per_kw, while maintaining their channel reserve (as required by the spec).
-               let remote_commit_tx_fee_msat = if self.context.is_outbound() { 0 } else {
-                       let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
-                       self.context.next_remote_commit_tx_fee_msat(htlc_candidate, None) // Don't include the extra fee spike buffer HTLC in calculations
-               };
-               if pending_remote_value_msat - msg.amount_msat < remote_commit_tx_fee_msat {
-                       return Err(ChannelError::Close("Remote HTLC add would not leave enough to pay for fees".to_owned()));
-               };
-
-               if pending_remote_value_msat - msg.amount_msat - remote_commit_tx_fee_msat < self.context.holder_selected_channel_reserve_satoshis * 1000 {
-                       return Err(ChannelError::Close("Remote HTLC add would put them under remote reserve value".to_owned()));
+               {
+                       let remote_commit_tx_fee_msat = if self.context.is_outbound() { 0 } else {
+                               let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
+                               self.context.next_remote_commit_tx_fee_msat(htlc_candidate, None) // Don't include the extra fee spike buffer HTLC in calculations
+                       };
+                       let anchor_outputs_value_msat = if !self.context.is_outbound() && self.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
+                               ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000
+                       } else {
+                               0
+                       };
+                       if pending_remote_value_msat.saturating_sub(msg.amount_msat).saturating_sub(anchor_outputs_value_msat) < remote_commit_tx_fee_msat {
+                               return Err(ChannelError::Close("Remote HTLC add would not leave enough to pay for fees".to_owned()));
+                       };
+                       if pending_remote_value_msat.saturating_sub(msg.amount_msat).saturating_sub(remote_commit_tx_fee_msat).saturating_sub(anchor_outputs_value_msat) < self.context.holder_selected_channel_reserve_satoshis * 1000 {
+                               return Err(ChannelError::Close("Remote HTLC add would put them under remote reserve value".to_owned()));
+                       }
                }
 
+               let anchor_outputs_value_msat = if self.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
+                       ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000
+               } else {
+                       0
+               };
                if !self.context.is_outbound() {
-                       // `2 *` and `Some(())` is for the fee spike buffer we keep for the remote. This deviates from
-                       // the spec because in the spec, the fee spike buffer requirement doesn't exist on the
-                       // receiver's side, only on the sender's.
-                       // Note that when we eventually remove support for fee updates and switch to anchor output
-                       // fees, we will drop the `2 *`, since we no longer be as sensitive to fee spikes. But, keep
-                       // the extra htlc when calculating the next remote commitment transaction fee as we should
-                       // still be able to afford adding this HTLC plus one more future HTLC, regardless of being
-                       // sensitive to fee spikes.
+                       // `Some(())` is for the fee spike buffer we keep for the remote. This deviates from
+                       // the spec because the fee spike buffer requirement doesn't exist on the receiver's
+                       // side, only on the sender's. Note that with anchor outputs we are no longer as
+                       // sensitive to fee spikes, so we need to account for them.
                        let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
-                       let remote_fee_cost_incl_stuck_buffer_msat = 2 * self.context.next_remote_commit_tx_fee_msat(htlc_candidate, Some(()));
-                       if pending_remote_value_msat - msg.amount_msat - self.context.holder_selected_channel_reserve_satoshis * 1000 < remote_fee_cost_incl_stuck_buffer_msat {
+                       let mut remote_fee_cost_incl_stuck_buffer_msat = self.context.next_remote_commit_tx_fee_msat(htlc_candidate, Some(()));
+                       if !self.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
+                               remote_fee_cost_incl_stuck_buffer_msat *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE;
+                       }
+                       if pending_remote_value_msat.saturating_sub(msg.amount_msat).saturating_sub(self.context.holder_selected_channel_reserve_satoshis * 1000).saturating_sub(anchor_outputs_value_msat) < remote_fee_cost_incl_stuck_buffer_msat {
                                // Note that if the pending_forward_status is not updated here, then it's because we're already failing
                                // the HTLC, i.e. its status is already set to failing.
                                log_info!(logger, "Attempting to fail HTLC due to fee spike buffer violation in channel {}. Rebalancing is required.", &self.context.channel_id());
@@ -2858,7 +2882,7 @@ impl<SP: Deref> Channel<SP> where
                        // Check that they won't violate our local required channel reserve by adding this HTLC.
                        let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
                        let local_commit_tx_fee_msat = self.context.next_local_commit_tx_fee_msat(htlc_candidate, None);
-                       if self.context.value_to_self_msat < self.context.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + local_commit_tx_fee_msat {
+                       if self.context.value_to_self_msat < self.context.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + local_commit_tx_fee_msat + anchor_outputs_value_msat {
                                return Err(ChannelError::Close("Cannot accept HTLC that would put our balance under counterparty-announced channel reserve value".to_owned()));
                        }
                }
@@ -4213,18 +4237,18 @@ impl<SP: Deref> Channel<SP> where
 
        pub fn maybe_propose_closing_signed<F: Deref, L: Deref>(
                &mut self, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
-               -> Result<(Option<msgs::ClosingSigned>, Option<Transaction>), ChannelError>
+               -> Result<(Option<msgs::ClosingSigned>, Option<Transaction>, Option<ShutdownResult>), ChannelError>
                where F::Target: FeeEstimator, L::Target: Logger
        {
                if self.context.last_sent_closing_fee.is_some() || !self.closing_negotiation_ready() {
-                       return Ok((None, None));
+                       return Ok((None, None, None));
                }
 
                if !self.context.is_outbound() {
                        if let Some(msg) = &self.context.pending_counterparty_closing_signed.take() {
                                return self.closing_signed(fee_estimator, &msg);
                        }
-                       return Ok((None, None));
+                       return Ok((None, None, None));
                }
 
                let (our_min_fee, our_max_fee) = self.calculate_closing_fee_limits(fee_estimator);
@@ -4249,7 +4273,7 @@ impl<SP: Deref> Channel<SP> where
                                                min_fee_satoshis: our_min_fee,
                                                max_fee_satoshis: our_max_fee,
                                        }),
-                               }), None))
+                               }), None, None))
                        }
                }
        }
@@ -4398,7 +4422,7 @@ impl<SP: Deref> Channel<SP> where
 
        pub fn closing_signed<F: Deref>(
                &mut self, fee_estimator: &LowerBoundedFeeEstimator<F>, msg: &msgs::ClosingSigned)
-               -> Result<(Option<msgs::ClosingSigned>, Option<Transaction>), ChannelError>
+               -> Result<(Option<msgs::ClosingSigned>, Option<Transaction>, Option<ShutdownResult>), ChannelError>
                where F::Target: FeeEstimator
        {
                if self.context.channel_state & BOTH_SIDES_SHUTDOWN_MASK != BOTH_SIDES_SHUTDOWN_MASK {
@@ -4420,7 +4444,7 @@ impl<SP: Deref> Channel<SP> where
 
                if self.context.channel_state & ChannelState::MonitorUpdateInProgress as u32 != 0 {
                        self.context.pending_counterparty_closing_signed = Some(msg.clone());
-                       return Ok((None, None));
+                       return Ok((None, None, None));
                }
 
                let funding_redeemscript = self.context.get_funding_redeemscript();
@@ -4450,10 +4474,15 @@ impl<SP: Deref> Channel<SP> where
                assert!(self.context.shutdown_scriptpubkey.is_some());
                if let Some((last_fee, sig)) = self.context.last_sent_closing_fee {
                        if last_fee == msg.fee_satoshis {
+                               let shutdown_result = ShutdownResult {
+                                       monitor_update: None,
+                                       dropped_outbound_htlcs: Vec::new(),
+                                       unbroadcasted_batch_funding_txid: self.context.unbroadcasted_batch_funding_txid(),
+                               };
                                let tx = self.build_signed_closing_transaction(&mut closing_tx, &msg.signature, &sig);
                                self.context.channel_state = ChannelState::ShutdownComplete as u32;
                                self.context.update_time_counter += 1;
-                               return Ok((None, Some(tx)));
+                               return Ok((None, Some(tx), Some(shutdown_result)));
                        }
                }
 
@@ -4472,13 +4501,19 @@ impl<SP: Deref> Channel<SP> where
                                                let sig = ecdsa
                                                        .sign_closing_transaction(&closing_tx, &self.context.secp_ctx)
                                                        .map_err(|_| ChannelError::Close("External signer refused to sign closing transaction".to_owned()))?;
-
-                                               let signed_tx = if $new_fee == msg.fee_satoshis {
+                                               let (signed_tx, shutdown_result) = if $new_fee == msg.fee_satoshis {
+                                                       let shutdown_result = ShutdownResult {
+                                                               monitor_update: None,
+                                                               dropped_outbound_htlcs: Vec::new(),
+                                                               unbroadcasted_batch_funding_txid: self.context.unbroadcasted_batch_funding_txid(),
+                                                       };
                                                        self.context.channel_state = ChannelState::ShutdownComplete as u32;
                                                        self.context.update_time_counter += 1;
                                                        let tx = self.build_signed_closing_transaction(&closing_tx, &msg.signature, &sig);
-                                                       Some(tx)
-                                               } else { None };
+                                                       (Some(tx), Some(shutdown_result))
+                                               } else {
+                                                       (None, None)
+                                               };
 
                                                self.context.last_sent_closing_fee = Some((used_fee, sig.clone()));
                                                Ok((Some(msgs::ClosingSigned {
@@ -4489,7 +4524,7 @@ impl<SP: Deref> Channel<SP> where
                                                                min_fee_satoshis: our_min_fee,
                                                                max_fee_satoshis: our_max_fee,
                                                        }),
-                                               }), signed_tx))
+                                               }), signed_tx, shutdown_result))
                                        }
                                }
                        }
@@ -5567,7 +5602,7 @@ impl<SP: Deref> Channel<SP> where
        /// [`ChannelMonitorUpdate`] will be returned).
        pub fn get_shutdown(&mut self, signer_provider: &SP, their_features: &InitFeatures,
                target_feerate_sats_per_kw: Option<u32>, override_shutdown_script: Option<ShutdownScript>)
-       -> Result<(msgs::Shutdown, Option<ChannelMonitorUpdate>, Vec<(HTLCSource, PaymentHash)>), APIError>
+       -> Result<(msgs::Shutdown, Option<ChannelMonitorUpdate>, Vec<(HTLCSource, PaymentHash)>, Option<ShutdownResult>), APIError>
        {
                for htlc in self.context.pending_outbound_htlcs.iter() {
                        if let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
@@ -5622,11 +5657,18 @@ impl<SP: Deref> Channel<SP> where
 
                // From here on out, we may not fail!
                self.context.target_closing_feerate_sats_per_kw = target_feerate_sats_per_kw;
-               if self.context.channel_state & !STATE_FLAGS < ChannelState::FundingSent as u32 {
+               let shutdown_result = if self.context.channel_state & !STATE_FLAGS < ChannelState::FundingSent as u32 {
+                       let shutdown_result = ShutdownResult {
+                               monitor_update: None,
+                               dropped_outbound_htlcs: Vec::new(),
+                               unbroadcasted_batch_funding_txid: self.context.unbroadcasted_batch_funding_txid(),
+                       };
                        self.context.channel_state = ChannelState::ShutdownComplete as u32;
+                       Some(shutdown_result)
                } else {
                        self.context.channel_state |= ChannelState::LocalShutdownSent as u32;
-               }
+                       None
+               };
                self.context.update_time_counter += 1;
 
                let monitor_update = if update_shutdown_script {
@@ -5662,7 +5704,7 @@ impl<SP: Deref> Channel<SP> where
                debug_assert!(!self.is_shutdown() || monitor_update.is_none(),
                        "we can't both complete shutdown and return a monitor update");
 
-               Ok((shutdown, monitor_update, dropped_outbound_htlcs))
+               Ok((shutdown, monitor_update, dropped_outbound_htlcs, shutdown_result))
        }
 
        pub fn inflight_htlc_sources(&self) -> impl Iterator<Item=(&HTLCSource, &PaymentHash)> {
@@ -5721,16 +5763,16 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
                let channel_type = Self::get_initial_channel_type(&config, their_features);
                debug_assert!(channel_type.is_subset(&channelmanager::provided_channel_type_features(&config)));
 
-               let commitment_conf_target = if channel_type.supports_anchors_zero_fee_htlc_tx() {
-                       ConfirmationTarget::AnchorChannelFee
+               let (commitment_conf_target, anchor_outputs_value_msat)  = if channel_type.supports_anchors_zero_fee_htlc_tx() {
+                       (ConfirmationTarget::AnchorChannelFee, ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000)
                } else {
-                       ConfirmationTarget::NonAnchorChannelFee
+                       (ConfirmationTarget::NonAnchorChannelFee, 0)
                };
                let commitment_feerate = fee_estimator.bounded_sat_per_1000_weight(commitment_conf_target);
 
                let value_to_self_msat = channel_value_satoshis * 1000 - push_msat;
                let commitment_tx_fee = commit_tx_fee_msat(commitment_feerate, MIN_AFFORDABLE_HTLC_COUNT, &channel_type);
-               if value_to_self_msat < commitment_tx_fee {
+               if value_to_self_msat.saturating_sub(anchor_outputs_value_msat) < commitment_tx_fee {
                        return Err(APIError::APIMisuseError{ err: format!("Funding amount ({}) can't even pay fee for initial commitment transaction fee of {}.", value_to_self_msat / 1000, commitment_tx_fee / 1000) });
                }
 
@@ -6347,13 +6389,18 @@ impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {
 
                // check if the funder's amount for the initial commitment tx is sufficient
                // for full fee payment plus a few HTLCs to ensure the channel will be useful.
+               let anchor_outputs_value = if channel_type.supports_anchors_zero_fee_htlc_tx() {
+                       ANCHOR_OUTPUT_VALUE_SATOSHI * 2
+               } else {
+                       0
+               };
                let funders_amount_msat = msg.funding_satoshis * 1000 - msg.push_msat;
                let commitment_tx_fee = commit_tx_fee_msat(msg.feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT, &channel_type) / 1000;
-               if funders_amount_msat / 1000 < commitment_tx_fee {
-                       return Err(ChannelError::Close(format!("Funding amount ({} sats) can't even pay fee for initial commitment transaction fee of {} sats.", funders_amount_msat / 1000, commitment_tx_fee)));
+               if (funders_amount_msat / 1000).saturating_sub(anchor_outputs_value) < commitment_tx_fee {
+                       return Err(ChannelError::Close(format!("Funding amount ({} sats) can't even pay fee for initial commitment transaction fee of {} sats.", (funders_amount_msat / 1000).saturating_sub(anchor_outputs_value), commitment_tx_fee)));
                }
 
-               let to_remote_satoshis = funders_amount_msat / 1000 - commitment_tx_fee;
+               let to_remote_satoshis = funders_amount_msat / 1000 - commitment_tx_fee - anchor_outputs_value;
                // While it's reasonable for us to not meet the channel reserve initially (if they don't
                // want to push much to us), our counterparty should always have more than our reserve.
                if to_remote_satoshis < holder_selected_channel_reserve_satoshis {
@@ -8179,7 +8226,7 @@ mod tests {
                use bitcoin::hashes::hex::FromHex;
                use bitcoin::hash_types::Txid;
                use bitcoin::secp256k1::Message;
-               use crate::sign::EcdsaChannelSigner;
+               use crate::sign::{ChannelDerivationParameters, HTLCDescriptor, EcdsaChannelSigner};
                use crate::ln::PaymentPreimage;
                use crate::ln::channel::{HTLCOutputInCommitment ,TxCreationKeys};
                use crate::ln::chan_utils::{ChannelPublicKeys, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters};
@@ -8304,7 +8351,7 @@ mod tests {
                                        &chan.context.holder_signer.as_ref().pubkeys().funding_pubkey,
                                        chan.context.counterparty_funding_pubkey()
                                );
-                               let (holder_sig, htlc_sigs) = signer.sign_holder_commitment_and_htlcs(&holder_commitment_tx, &secp_ctx).unwrap();
+                               let holder_sig = signer.sign_holder_commitment(&holder_commitment_tx, &secp_ctx).unwrap();
                                assert_eq!(Signature::from_der(&hex::decode($sig_hex).unwrap()[..]).unwrap(), holder_sig, "holder_sig");
 
                                let funding_redeemscript = chan.context.get_funding_redeemscript();
@@ -8312,14 +8359,14 @@ mod tests {
                                assert_eq!(serialize(&tx)[..], hex::decode($tx_hex).unwrap()[..], "tx");
 
                                // ((htlc, counterparty_sig), (index, holder_sig))
-                               let mut htlc_sig_iter = holder_commitment_tx.htlcs().iter().zip(&holder_commitment_tx.counterparty_htlc_sigs).zip(htlc_sigs.iter().enumerate());
+                               let mut htlc_counterparty_sig_iter = holder_commitment_tx.counterparty_htlc_sigs.iter();
 
                                $({
                                        log_trace!(logger, "verifying htlc {}", $htlc_idx);
                                        let remote_signature = Signature::from_der(&hex::decode($counterparty_htlc_sig_hex).unwrap()[..]).unwrap();
 
                                        let ref htlc = htlcs[$htlc_idx];
-                                       let htlc_tx = chan_utils::build_htlc_transaction(&unsigned_tx.txid, chan.context.feerate_per_kw,
+                                       let mut htlc_tx = chan_utils::build_htlc_transaction(&unsigned_tx.txid, chan.context.feerate_per_kw,
                                                chan.context.get_counterparty_selected_contest_delay().unwrap(),
                                                &htlc, $opt_anchors, &keys.broadcaster_delayed_payment_key, &keys.revocation_key);
                                        let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, $opt_anchors, &keys);
@@ -8339,20 +8386,32 @@ mod tests {
                                                assert!(preimage.is_some());
                                        }
 
-                                       let htlc_sig = htlc_sig_iter.next().unwrap();
+                                       let htlc_counterparty_sig = htlc_counterparty_sig_iter.next().unwrap();
+                                       let htlc_holder_sig = signer.sign_holder_htlc_transaction(&htlc_tx, 0, &HTLCDescriptor {
+                                               channel_derivation_parameters: ChannelDerivationParameters {
+                                                       value_satoshis: chan.context.channel_value_satoshis,
+                                                       keys_id: chan.context.channel_keys_id,
+                                                       transaction_parameters: chan.context.channel_transaction_parameters.clone(),
+                                               },
+                                               commitment_txid: trusted_tx.txid(),
+                                               per_commitment_number: trusted_tx.commitment_number(),
+                                               per_commitment_point: trusted_tx.per_commitment_point(),
+                                               feerate_per_kw: trusted_tx.feerate_per_kw(),
+                                               htlc: htlc.clone(),
+                                               preimage: preimage.clone(),
+                                               counterparty_sig: *htlc_counterparty_sig,
+                                       }, &secp_ctx).unwrap();
                                        let num_anchors = if $opt_anchors.supports_anchors_zero_fee_htlc_tx() { 2 } else { 0 };
-                                       assert_eq!((htlc_sig.0).0.transaction_output_index, Some($htlc_idx + num_anchors), "output index");
+                                       assert_eq!(htlc.transaction_output_index, Some($htlc_idx + num_anchors), "output index");
 
                                        let signature = Signature::from_der(&hex::decode($htlc_sig_hex).unwrap()[..]).unwrap();
-                                       assert_eq!(signature, *(htlc_sig.1).1, "htlc sig");
-                                       let index = (htlc_sig.1).0;
-                                       let channel_parameters = chan.context.channel_transaction_parameters.as_holder_broadcastable();
+                                       assert_eq!(signature, htlc_holder_sig, "htlc sig");
                                        let trusted_tx = holder_commitment_tx.trust();
-                                       log_trace!(logger, "htlc_tx = {}", hex::encode(serialize(&trusted_tx.get_signed_htlc_tx(&channel_parameters, index, &(htlc_sig.0).1, (htlc_sig.1).1, &preimage))));
-                                       assert_eq!(serialize(&trusted_tx.get_signed_htlc_tx(&channel_parameters, index, &(htlc_sig.0).1, (htlc_sig.1).1, &preimage))[..],
-                                                       hex::decode($htlc_tx_hex).unwrap()[..], "htlc tx");
+                                       htlc_tx.input[0].witness = trusted_tx.build_htlc_input_witness($htlc_idx, htlc_counterparty_sig, &htlc_holder_sig, &preimage);
+                                       log_trace!(logger, "htlc_tx = {}", hex::encode(serialize(&htlc_tx)));
+                                       assert_eq!(serialize(&htlc_tx)[..], hex::decode($htlc_tx_hex).unwrap()[..], "htlc tx");
                                })*
-                               assert!(htlc_sig_iter.next().is_none());
+                               assert!(htlc_counterparty_sig_iter.next().is_none());
                        } }
                }