Skip to content

Commit 6c62d46

Browse files
tankyleotankyleo
committed
Reorder the operations in ChannelContext::build_commitment_transaction
This reorganizes the logic in `ChannelContext::build_commitment_transaction` to clearly separate the sorting of HTLCs for a commitment transaction based on their state from the trimming of HTLCs based on the broadcaster's dust limit. In the future, transaction builders will decide how to handle HTLCs below the dust limit, but they will not decide which HTLCs to include based on their state.
1 parent 30ab527 commit 6c62d46

File tree

1 file changed

+81
-65
lines changed

1 file changed

+81
-65
lines changed

‎lightning/src/ln/channel.rs

Lines changed: 81 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -3590,9 +3590,11 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
35903590
fn build_commitment_transaction<L: Deref>(&self, funding: &FundingScope, commitment_number: u64, per_commitment_point: &PublicKey, local: bool, generated_by_local: bool, logger: &L) -> CommitmentStats
35913591
where L::Target: Logger
35923592
{
3593-
let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new();
35943593
let num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len();
3595-
let mut included_non_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs);
3594+
3595+
// This will contain all the htlcs included on the commitment transaction due to their state, both dust, and non-dust.
3596+
// Non-dust htlcs will come first, with their output indices populated, then dust htlcs, with their output indices set to `None`.
3597+
let mut htlcs_in_tx: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs);
35963598

35973599
let broadcaster_dust_limit_satoshis = if local { self.holder_dust_limit_satoshis } else { self.counterparty_dust_limit_satoshis };
35983600
let mut remote_htlc_total_msat = 0;
@@ -3630,42 +3632,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
36303632
}
36313633
}
36323634

3633-
macro_rules! add_htlc_output {
3634-
($htlc: expr, $outbound: expr, $source: expr, $state_name: expr) => {
3635-
if $outbound == local { // "offered HTLC output"
3636-
let htlc_in_tx = get_htlc_in_commitment!($htlc, true);
3637-
let htlc_tx_fee = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
3638-
0
3639-
} else {
3640-
feerate_per_kw as u64 * htlc_timeout_tx_weight(self.get_channel_type()) / 1000
3641-
};
3642-
if $htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee {
3643-
log_trace!(logger, " ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat);
3644-
included_non_dust_htlcs.push((htlc_in_tx, $source));
3645-
} else {
3646-
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);
3647-
included_dust_htlcs.push((htlc_in_tx, $source));
3648-
}
3649-
} else {
3650-
let htlc_in_tx = get_htlc_in_commitment!($htlc, false);
3651-
let htlc_tx_fee = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
3652-
0
3653-
} else {
3654-
feerate_per_kw as u64 * htlc_success_tx_weight(self.get_channel_type()) / 1000
3655-
};
3656-
if $htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee {
3657-
log_trace!(logger, " ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat);
3658-
included_non_dust_htlcs.push((htlc_in_tx, $source));
3659-
} else {
3660-
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);
3661-
included_dust_htlcs.push((htlc_in_tx, $source));
3662-
}
3663-
}
3664-
}
3665-
}
3666-
3635+
// Read the state of htlcs and determine their inclusion in the commitment transaction
36673636
let mut inbound_htlc_preimages: Vec<PaymentPreimage> = Vec::new();
3668-
36693637
for ref htlc in self.pending_inbound_htlcs.iter() {
36703638
let (include, state_name) = match htlc.state {
36713639
InboundHTLCState::RemoteAnnounced(_) => (!generated_by_local, "RemoteAnnounced"),
@@ -3676,8 +3644,9 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
36763644
};
36773645

36783646
if include {
3679-
add_htlc_output!(htlc, false, None, state_name);
3680-
remote_htlc_total_msat += htlc.amount_msat;
3647+
log_trace!(logger, " ...including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
3648+
let htlc = get_htlc_in_commitment!(htlc, !local);
3649+
htlcs_in_tx.push((htlc, None));
36813650
} else {
36823651
log_trace!(logger, " ...not including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
36833652
match &htlc.state {
@@ -3692,7 +3661,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
36923661

36933662

36943663
let mut outbound_htlc_preimages: Vec<PaymentPreimage> = Vec::new();
3695-
36963664
for ref htlc in self.pending_outbound_htlcs.iter() {
36973665
let (include, state_name) = match htlc.state {
36983666
OutboundHTLCState::LocalAnnounced(_) => (generated_by_local, "LocalAnnounced"),
@@ -3714,8 +3682,9 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
37143682
}
37153683

37163684
if include {
3717-
add_htlc_output!(htlc, true, Some(&htlc.source), state_name);
3718-
local_htlc_total_msat += htlc.amount_msat;
3685+
log_trace!(logger, " ...including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
3686+
let htlc_in_tx = get_htlc_in_commitment!(htlc, local);
3687+
htlcs_in_tx.push((htlc_in_tx, Some(&htlc.source)));
37193688
} else {
37203689
log_trace!(logger, " ...not including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
37213690
match htlc.state {
@@ -3729,6 +3698,46 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
37293698
}
37303699
}
37313700

3701+
// Trim dust htlcs
3702+
let mut included_non_dust_htlcs: Vec<_> = htlcs_in_tx.iter_mut().map(|(htlc, _)| htlc).collect();
3703+
included_non_dust_htlcs.retain(|htlc| {
3704+
let outbound = local == htlc.offered;
3705+
if outbound {
3706+
local_htlc_total_msat += htlc.amount_msat;
3707+
} else {
3708+
remote_htlc_total_msat += htlc.amount_msat;
3709+
}
3710+
let htlc_tx_fee = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
3711+
0
3712+
} else {
3713+
feerate_per_kw as u64
3714+
* if htlc.offered {
3715+
chan_utils::htlc_timeout_tx_weight(self.get_channel_type())
3716+
} else {
3717+
chan_utils::htlc_success_tx_weight(self.get_channel_type())
3718+
} / 1000
3719+
};
3720+
if htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee {
3721+
log_trace!(
3722+
logger,
3723+
" ...creating output for {} non-dust HTLC (hash {}) with value {}",
3724+
if outbound { "outbound" } else { "inbound" },
3725+
htlc.payment_hash,
3726+
htlc.amount_msat
3727+
);
3728+
true
3729+
} else {
3730+
log_trace!(
3731+
logger,
3732+
" ...trimming {} HTLC (hash {}) with value {} due to dust limit",
3733+
if outbound { "outbound" } else { "inbound" },
3734+
htlc.payment_hash,
3735+
htlc.amount_msat
3736+
);
3737+
false
3738+
}
3739+
});
3740+
37323741
// TODO: When MSRV >= 1.66.0, use u64::checked_add_signed
37333742
let mut value_to_self_msat = u64::try_from(funding.value_to_self_msat as i64 + value_to_self_msat_offset).unwrap();
37343743
// Note that in case they have several just-awaiting-last-RAA fulfills in-progress (ie
@@ -3739,21 +3748,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
37393748
value_to_self_msat = u64::checked_sub(value_to_self_msat, local_htlc_total_msat).unwrap();
37403749
value_to_remote_msat = u64::checked_sub(value_to_remote_msat, remote_htlc_total_msat).unwrap();
37413750

3742-
#[cfg(debug_assertions)]
3743-
{
3744-
// Make sure that the to_self/to_remote is always either past the appropriate
3745-
// channel_reserve *or* it is making progress towards it.
3746-
let mut broadcaster_max_commitment_tx_output = if generated_by_local {
3747-
funding.holder_max_commitment_tx_output.lock().unwrap()
3748-
} else {
3749-
funding.counterparty_max_commitment_tx_output.lock().unwrap()
3750-
};
3751-
debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat || value_to_self_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap());
3752-
broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat);
3753-
debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat || value_to_remote_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis);
3754-
broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat);
3755-
}
3756-
37573751
let total_fee_sat = commit_tx_fee_sat(feerate_per_kw, included_non_dust_htlcs.len(), &funding.channel_transaction_parameters.channel_type_features);
37583752
let anchors_val = if funding.channel_transaction_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 };
37593753
let (value_to_self, value_to_remote) = if funding.is_outbound() {
@@ -3766,14 +3760,16 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
37663760
let mut value_to_b = if local { value_to_remote } else { value_to_self };
37673761

37683762
if value_to_a >= broadcaster_dust_limit_satoshis {
3769-
log_trace!(logger, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a);
3763+
log_trace!(logger, " ...creating {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a);
37703764
} else {
3765+
log_trace!(logger, " ...trimming {} output with value {} due to dust limit", if local { "to_local" } else { "to_remote" }, value_to_a);
37713766
value_to_a = 0;
37723767
}
37733768

37743769
if value_to_b >= broadcaster_dust_limit_satoshis {
3775-
log_trace!(logger, " ...including {} output with value {}", if local { "to_remote" } else { "to_local" }, value_to_b);
3770+
log_trace!(logger, " ...creating {} output with value {}", if local { "to_remote" } else { "to_local" }, value_to_b);
37763771
} else {
3772+
log_trace!(logger, " ...trimming {} output with value {} due to dust limit", if local { "to_remote" } else { "to_local" }, value_to_b);
37773773
value_to_b = 0;
37783774
}
37793775

@@ -3787,21 +3783,41 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
37873783
value_to_a,
37883784
value_to_b,
37893785
feerate_per_kw,
3790-
included_non_dust_htlcs.iter_mut().map(|(htlc, _)| htlc).collect(),
3786+
included_non_dust_htlcs,
37913787
&channel_parameters,
37923788
&self.secp_ctx,
37933789
);
3794-
let mut htlcs_included = included_non_dust_htlcs;
3795-
// The unwrap is safe, because all non-dust HTLCs have been assigned an output index
3796-
htlcs_included.sort_unstable_by_key(|h| h.0.transaction_output_index.unwrap());
3797-
htlcs_included.append(&mut included_dust_htlcs);
3790+
3791+
#[cfg(debug_assertions)]
3792+
{
3793+
// Make sure that the to_self/to_remote is always either past the appropriate
3794+
// channel_reserve *or* it is making progress towards it.
3795+
let mut broadcaster_max_commitment_tx_output = if generated_by_local {
3796+
funding.holder_max_commitment_tx_output.lock().unwrap()
3797+
} else {
3798+
funding.counterparty_max_commitment_tx_output.lock().unwrap()
3799+
};
3800+
debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat || value_to_self_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap());
3801+
broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat);
3802+
debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat || value_to_remote_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis);
3803+
broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat);
3804+
}
3805+
3806+
htlcs_in_tx.sort_unstable_by(|(htlc_a, _), (htlc_b, _)| {
3807+
match (htlc_a.transaction_output_index, htlc_b.transaction_output_index) {
3808+
// `None` is smaller than `Some`, but we want `Some` ordered before `None` in the vector
3809+
(None, Some(_)) => cmp::Ordering::Greater,
3810+
(Some(_), None) => cmp::Ordering::Less,
3811+
(l, r) => cmp::Ord::cmp(&l, &r),
3812+
}
3813+
});
37983814

37993815
CommitmentStats {
38003816
tx,
38013817
feerate_per_kw,
38023818
total_fee_sat,
38033819
num_nondust_htlcs,
3804-
htlcs_included,
3820+
htlcs_included: htlcs_in_tx,
38053821
local_balance_msat: value_to_self_msat,
38063822
remote_balance_msat: value_to_remote_msat,
38073823
inbound_htlc_preimages,

0 commit comments

Comments
 (0)