Skip to content

Commit 6317aa5

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 8189283 commit 6317aa5

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
@@ -3461,9 +3461,11 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
34613461
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
34623462
where L::Target: Logger
34633463
{
3464-
let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new();
34653464
let num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len();
3466-
let mut included_non_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs);
3465+
3466+
// This will contain all the htlcs included on the commitment transaction due to their state, both dust, and non-dust.
3467+
// Non-dust htlcs will come first, with their output indices populated, then dust htlcs, with their output indices set to `None`.
3468+
let mut htlcs_in_tx: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs);
34673469

34683470
let broadcaster_dust_limit_satoshis = if local { self.holder_dust_limit_satoshis } else { self.counterparty_dust_limit_satoshis };
34693471
let mut remote_htlc_total_msat = 0;
@@ -3501,42 +3503,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
35013503
}
35023504
}
35033505

3504-
macro_rules! add_htlc_output {
3505-
($htlc: expr, $outbound: expr, $source: expr, $state_name: expr) => {
3506-
if $outbound == local { // "offered HTLC output"
3507-
let htlc_in_tx = get_htlc_in_commitment!($htlc, true);
3508-
let htlc_tx_fee = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
3509-
0
3510-
} else {
3511-
feerate_per_kw as u64 * htlc_timeout_tx_weight(self.get_channel_type()) / 1000
3512-
};
3513-
if $htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee {
3514-
log_trace!(logger, " ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat);
3515-
included_non_dust_htlcs.push((htlc_in_tx, $source));
3516-
} else {
3517-
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);
3518-
included_dust_htlcs.push((htlc_in_tx, $source));
3519-
}
3520-
} else {
3521-
let htlc_in_tx = get_htlc_in_commitment!($htlc, false);
3522-
let htlc_tx_fee = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
3523-
0
3524-
} else {
3525-
feerate_per_kw as u64 * htlc_success_tx_weight(self.get_channel_type()) / 1000
3526-
};
3527-
if $htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee {
3528-
log_trace!(logger, " ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat);
3529-
included_non_dust_htlcs.push((htlc_in_tx, $source));
3530-
} else {
3531-
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);
3532-
included_dust_htlcs.push((htlc_in_tx, $source));
3533-
}
3534-
}
3535-
}
3536-
}
3537-
3506+
// Read the state of htlcs and determine their inclusion in the commitment transaction
35383507
let mut inbound_htlc_preimages: Vec<PaymentPreimage> = Vec::new();
3539-
35403508
for ref htlc in self.pending_inbound_htlcs.iter() {
35413509
let (include, state_name) = match htlc.state {
35423510
InboundHTLCState::RemoteAnnounced(_) => (!generated_by_local, "RemoteAnnounced"),
@@ -3547,8 +3515,9 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
35473515
};
35483516

35493517
if include {
3550-
add_htlc_output!(htlc, false, None, state_name);
3551-
remote_htlc_total_msat += htlc.amount_msat;
3518+
log_trace!(logger, " ...including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
3519+
let htlc = get_htlc_in_commitment!(htlc, !local);
3520+
htlcs_in_tx.push((htlc, None));
35523521
} else {
35533522
log_trace!(logger, " ...not including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
35543523
match &htlc.state {
@@ -3563,7 +3532,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
35633532

35643533

35653534
let mut outbound_htlc_preimages: Vec<PaymentPreimage> = Vec::new();
3566-
35673535
for ref htlc in self.pending_outbound_htlcs.iter() {
35683536
let (include, state_name) = match htlc.state {
35693537
OutboundHTLCState::LocalAnnounced(_) => (generated_by_local, "LocalAnnounced"),
@@ -3585,8 +3553,9 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
35853553
}
35863554

35873555
if include {
3588-
add_htlc_output!(htlc, true, Some(&htlc.source), state_name);
3589-
local_htlc_total_msat += htlc.amount_msat;
3556+
log_trace!(logger, " ...including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
3557+
let htlc_in_tx = get_htlc_in_commitment!(htlc, local);
3558+
htlcs_in_tx.push((htlc_in_tx, Some(&htlc.source)));
35903559
} else {
35913560
log_trace!(logger, " ...not including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
35923561
match htlc.state {
@@ -3600,6 +3569,46 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
36003569
}
36013570
}
36023571

3572+
// Trim dust htlcs
3573+
let mut included_non_dust_htlcs: Vec<_> = htlcs_in_tx.iter_mut().map(|(htlc, _)| htlc).collect();
3574+
included_non_dust_htlcs.retain(|htlc| {
3575+
let outbound = local == htlc.offered;
3576+
if outbound {
3577+
local_htlc_total_msat += htlc.amount_msat;
3578+
} else {
3579+
remote_htlc_total_msat += htlc.amount_msat;
3580+
}
3581+
let htlc_tx_fee = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
3582+
0
3583+
} else {
3584+
feerate_per_kw as u64
3585+
* if htlc.offered {
3586+
chan_utils::htlc_timeout_tx_weight(self.get_channel_type())
3587+
} else {
3588+
chan_utils::htlc_success_tx_weight(self.get_channel_type())
3589+
} / 1000
3590+
};
3591+
if htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee {
3592+
log_trace!(
3593+
logger,
3594+
" ...creating output for {} non-dust HTLC (hash {}) with value {}",
3595+
if outbound { "outbound" } else { "inbound" },
3596+
htlc.payment_hash,
3597+
htlc.amount_msat
3598+
);
3599+
true
3600+
} else {
3601+
log_trace!(
3602+
logger,
3603+
" ...trimming {} HTLC (hash {}) with value {} due to dust limit",
3604+
if outbound { "outbound" } else { "inbound" },
3605+
htlc.payment_hash,
3606+
htlc.amount_msat
3607+
);
3608+
false
3609+
}
3610+
});
3611+
36033612
// TODO: When MSRV >= 1.66.0, use u64::checked_add_signed
36043613
let mut value_to_self_msat = u64::try_from(funding.value_to_self_msat as i64 + value_to_self_msat_offset).unwrap();
36053614
// Note that in case they have several just-awaiting-last-RAA fulfills in-progress (ie
@@ -3610,21 +3619,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
36103619
value_to_self_msat = u64::checked_sub(value_to_self_msat, local_htlc_total_msat).unwrap();
36113620
value_to_remote_msat = u64::checked_sub(value_to_remote_msat, remote_htlc_total_msat).unwrap();
36123621

3613-
#[cfg(debug_assertions)]
3614-
{
3615-
// Make sure that the to_self/to_remote is always either past the appropriate
3616-
// channel_reserve *or* it is making progress towards it.
3617-
let mut broadcaster_max_commitment_tx_output = if generated_by_local {
3618-
funding.holder_max_commitment_tx_output.lock().unwrap()
3619-
} else {
3620-
funding.counterparty_max_commitment_tx_output.lock().unwrap()
3621-
};
3622-
debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat || value_to_self_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap());
3623-
broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat);
3624-
debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat || value_to_remote_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis);
3625-
broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat);
3626-
}
3627-
36283622
let total_fee_sat = commit_tx_fee_sat(feerate_per_kw, included_non_dust_htlcs.len(), &funding.channel_transaction_parameters.channel_type_features);
36293623
let anchors_val = if funding.channel_transaction_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 };
36303624
let (value_to_self, value_to_remote) = if funding.is_outbound() {
@@ -3637,14 +3631,16 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
36373631
let mut value_to_b = if local { value_to_remote } else { value_to_self };
36383632

36393633
if value_to_a >= broadcaster_dust_limit_satoshis {
3640-
log_trace!(logger, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a);
3634+
log_trace!(logger, " ...creating {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a);
36413635
} else {
3636+
log_trace!(logger, " ...trimming {} output with value {} due to dust limit", if local { "to_local" } else { "to_remote" }, value_to_a);
36423637
value_to_a = 0;
36433638
}
36443639

36453640
if value_to_b >= broadcaster_dust_limit_satoshis {
3646-
log_trace!(logger, " ...including {} output with value {}", if local { "to_remote" } else { "to_local" }, value_to_b);
3641+
log_trace!(logger, " ...creating {} output with value {}", if local { "to_remote" } else { "to_local" }, value_to_b);
36473642
} else {
3643+
log_trace!(logger, " ...trimming {} output with value {} due to dust limit", if local { "to_remote" } else { "to_local" }, value_to_b);
36483644
value_to_b = 0;
36493645
}
36503646

@@ -3658,21 +3654,41 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
36583654
value_to_a,
36593655
value_to_b,
36603656
feerate_per_kw,
3661-
included_non_dust_htlcs.iter_mut().map(|(htlc, _)| htlc).collect(),
3657+
included_non_dust_htlcs,
36623658
&channel_parameters,
36633659
&self.secp_ctx,
36643660
);
3665-
let mut htlcs_included = included_non_dust_htlcs;
3666-
// The unwrap is safe, because all non-dust HTLCs have been assigned an output index
3667-
htlcs_included.sort_unstable_by_key(|h| h.0.transaction_output_index.unwrap());
3668-
htlcs_included.append(&mut included_dust_htlcs);
3661+
3662+
#[cfg(debug_assertions)]
3663+
{
3664+
// Make sure that the to_self/to_remote is always either past the appropriate
3665+
// channel_reserve *or* it is making progress towards it.
3666+
let mut broadcaster_max_commitment_tx_output = if generated_by_local {
3667+
funding.holder_max_commitment_tx_output.lock().unwrap()
3668+
} else {
3669+
funding.counterparty_max_commitment_tx_output.lock().unwrap()
3670+
};
3671+
debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat || value_to_self_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap());
3672+
broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat);
3673+
debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat || value_to_remote_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis);
3674+
broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat);
3675+
}
3676+
3677+
htlcs_in_tx.sort_unstable_by(|(htlc_a, _), (htlc_b, _)| {
3678+
match (htlc_a.transaction_output_index, htlc_b.transaction_output_index) {
3679+
// `None` is smaller than `Some`, but we want `Some` ordered before `None` in the vector
3680+
(None, Some(_)) => cmp::Ordering::Greater,
3681+
(Some(_), None) => cmp::Ordering::Less,
3682+
(l, r) => cmp::Ord::cmp(&l, &r),
3683+
}
3684+
});
36693685

36703686
CommitmentStats {
36713687
tx,
36723688
feerate_per_kw,
36733689
total_fee_sat,
36743690
num_nondust_htlcs,
3675-
htlcs_included,
3691+
htlcs_included: htlcs_in_tx,
36763692
local_balance_msat: value_to_self_msat,
36773693
remote_balance_msat: value_to_remote_msat,
36783694
inbound_htlc_preimages,

0 commit comments

Comments
 (0)