Skip to content

Commit 570ca77

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 ca3fa4e commit 570ca77

File tree

1 file changed

+87
-65
lines changed

1 file changed

+87
-65
lines changed

‎lightning/src/ln/channel.rs

Lines changed: 87 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -3413,9 +3413,16 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
34133413
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
34143414
where L::Target: Logger
34153415
{
3416-
let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new();
34173416
let num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len();
3418-
let mut included_non_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs);
3417+
3418+
// This will contain all the htlcs included on the commitment transaction due to their state, both dust, and non-dust.
3419+
// Non-dust htlcs will come first, with their output indices populated, then dust htlcs, with their output indices set to `None`.
3420+
let mut htlcs_in_tx: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs);
3421+
3422+
// We allocate this vector because we need to count the number of non-dust htlcs and calculate the total fee of the transaction
3423+
// before calling `CommitmentTransaction::new`.
3424+
// We could drop this vector and create two iterators: one to count the number of non-dust htlcs, and another to pass to `CommitmentTransaction::new`
3425+
let mut included_non_dust_htlcs: Vec<&mut HTLCOutputInCommitment> = Vec::with_capacity(num_htlcs);
34193426

34203427
let broadcaster_dust_limit_satoshis = if local { self.holder_dust_limit_satoshis } else { self.counterparty_dust_limit_satoshis };
34213428
let mut remote_htlc_total_msat = 0;
@@ -3453,42 +3460,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
34533460
}
34543461
}
34553462

3456-
macro_rules! add_htlc_output {
3457-
($htlc: expr, $outbound: expr, $source: expr, $state_name: expr) => {
3458-
if $outbound == local { // "offered HTLC output"
3459-
let htlc_in_tx = get_htlc_in_commitment!($htlc, true);
3460-
let htlc_tx_fee = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
3461-
0
3462-
} else {
3463-
feerate_per_kw as u64 * htlc_timeout_tx_weight(self.get_channel_type()) / 1000
3464-
};
3465-
if $htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee {
3466-
log_trace!(logger, " ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat);
3467-
included_non_dust_htlcs.push((htlc_in_tx, $source));
3468-
} else {
3469-
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);
3470-
included_dust_htlcs.push((htlc_in_tx, $source));
3471-
}
3472-
} else {
3473-
let htlc_in_tx = get_htlc_in_commitment!($htlc, false);
3474-
let htlc_tx_fee = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
3475-
0
3476-
} else {
3477-
feerate_per_kw as u64 * htlc_success_tx_weight(self.get_channel_type()) / 1000
3478-
};
3479-
if $htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee {
3480-
log_trace!(logger, " ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, &$htlc.payment_hash, $htlc.amount_msat);
3481-
included_non_dust_htlcs.push((htlc_in_tx, $source));
3482-
} else {
3483-
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);
3484-
included_dust_htlcs.push((htlc_in_tx, $source));
3485-
}
3486-
}
3487-
}
3488-
}
3489-
3463+
// Read the state of htlcs and determine their inclusion in the commitment transaction
34903464
let mut inbound_htlc_preimages: Vec<PaymentPreimage> = Vec::new();
3491-
34923465
for ref htlc in self.pending_inbound_htlcs.iter() {
34933466
let (include, state_name) = match htlc.state {
34943467
InboundHTLCState::RemoteAnnounced(_) => (!generated_by_local, "RemoteAnnounced"),
@@ -3499,8 +3472,9 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
34993472
};
35003473

35013474
if include {
3502-
add_htlc_output!(htlc, false, None, state_name);
3503-
remote_htlc_total_msat += htlc.amount_msat;
3475+
log_trace!(logger, " ...including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
3476+
let htlc = get_htlc_in_commitment!(htlc, !local);
3477+
htlcs_in_tx.push((htlc, None));
35043478
} else {
35053479
log_trace!(logger, " ...not including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
35063480
match &htlc.state {
@@ -3515,7 +3489,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
35153489

35163490

35173491
let mut outbound_htlc_preimages: Vec<PaymentPreimage> = Vec::new();
3518-
35193492
for ref htlc in self.pending_outbound_htlcs.iter() {
35203493
let (include, state_name) = match htlc.state {
35213494
OutboundHTLCState::LocalAnnounced(_) => (generated_by_local, "LocalAnnounced"),
@@ -3537,8 +3510,9 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
35373510
}
35383511

35393512
if include {
3540-
add_htlc_output!(htlc, true, Some(&htlc.source), state_name);
3541-
local_htlc_total_msat += htlc.amount_msat;
3513+
log_trace!(logger, " ...including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
3514+
let htlc_in_tx = get_htlc_in_commitment!(htlc, local);
3515+
htlcs_in_tx.push((htlc_in_tx, Some(&htlc.source)));
35423516
} else {
35433517
log_trace!(logger, " ...not including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
35443518
match htlc.state {
@@ -3552,6 +3526,47 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
35523526
}
35533527
}
35543528

3529+
// Trim dust htlcs
3530+
for (htlc, _) in htlcs_in_tx.iter_mut() {
3531+
if htlc.offered {
3532+
let outbound = local;
3533+
if outbound {
3534+
local_htlc_total_msat += htlc.amount_msat;
3535+
} else {
3536+
remote_htlc_total_msat += htlc.amount_msat;
3537+
}
3538+
let htlc_tx_fee = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
3539+
0
3540+
} else {
3541+
feerate_per_kw as u64 * htlc_timeout_tx_weight(self.get_channel_type()) / 1000
3542+
};
3543+
if htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee {
3544+
log_trace!(logger, " ...creating output for {} non-dust HTLC (hash {}) with value {}", if outbound { "outbound" } else { "inbound" }, htlc.payment_hash, htlc.amount_msat);
3545+
included_non_dust_htlcs.push(htlc);
3546+
} else {
3547+
log_trace!(logger, " ...trimming {} HTLC (hash {}) with value {} due to dust limit", if outbound { "outbound" } else { "inbound" }, htlc.payment_hash, htlc.amount_msat);
3548+
}
3549+
} else {
3550+
let outbound = !local;
3551+
if outbound {
3552+
local_htlc_total_msat += htlc.amount_msat;
3553+
} else {
3554+
remote_htlc_total_msat += htlc.amount_msat;
3555+
}
3556+
let htlc_tx_fee = if self.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
3557+
0
3558+
} else {
3559+
feerate_per_kw as u64 * htlc_success_tx_weight(self.get_channel_type()) / 1000
3560+
};
3561+
if htlc.amount_msat / 1000 >= broadcaster_dust_limit_satoshis + htlc_tx_fee {
3562+
log_trace!(logger, " ...creating output for {} non-dust HTLC (hash {}) with value {}", if outbound { "outbound" } else { "inbound" }, htlc.payment_hash, htlc.amount_msat);
3563+
included_non_dust_htlcs.push(htlc);
3564+
} else {
3565+
log_trace!(logger, " ...trimming {} HTLC (hash {}) with value {} due to dust limit", if outbound { "outbound" } else { "inbound" }, htlc.payment_hash, htlc.amount_msat);
3566+
}
3567+
}
3568+
}
3569+
35553570
// TODO: When MSRV >= 1.66.0, use u64::checked_add_signed
35563571
let mut value_to_self_msat = u64::try_from(funding.value_to_self_msat as i64 + value_to_self_msat_offset).unwrap();
35573572
// Note that in case they have several just-awaiting-last-RAA fulfills in-progress (ie
@@ -3562,21 +3577,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
35623577
value_to_self_msat = u64::checked_sub(value_to_self_msat, local_htlc_total_msat).unwrap();
35633578
value_to_remote_msat = u64::checked_sub(value_to_remote_msat, remote_htlc_total_msat).unwrap();
35643579

3565-
#[cfg(debug_assertions)]
3566-
{
3567-
// Make sure that the to_self/to_remote is always either past the appropriate
3568-
// channel_reserve *or* it is making progress towards it.
3569-
let mut broadcaster_max_commitment_tx_output = if generated_by_local {
3570-
funding.holder_max_commitment_tx_output.lock().unwrap()
3571-
} else {
3572-
funding.counterparty_max_commitment_tx_output.lock().unwrap()
3573-
};
3574-
debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat || value_to_self_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap());
3575-
broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat);
3576-
debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat || value_to_remote_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis);
3577-
broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat);
3578-
}
3579-
35803580
let total_fee_sat = commit_tx_fee_sat(feerate_per_kw, included_non_dust_htlcs.len(), &self.channel_transaction_parameters.channel_type_features);
35813581
let anchors_val = if self.channel_transaction_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 };
35823582
let (value_to_self, value_to_remote) = if self.is_outbound() {
@@ -3589,14 +3589,16 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
35893589
let mut value_to_b = if local { value_to_remote } else { value_to_self };
35903590

35913591
if value_to_a >= broadcaster_dust_limit_satoshis {
3592-
log_trace!(logger, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a);
3592+
log_trace!(logger, " ...creating {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a);
35933593
} else {
3594+
log_trace!(logger, " ...trimming {} output with value {} due to dust limit", if local { "to_local" } else { "to_remote" }, value_to_a);
35943595
value_to_a = 0;
35953596
}
35963597

35973598
if value_to_b >= broadcaster_dust_limit_satoshis {
3598-
log_trace!(logger, " ...including {} output with value {}", if local { "to_remote" } else { "to_local" }, value_to_b);
3599+
log_trace!(logger, " ...creating {} output with value {}", if local { "to_remote" } else { "to_local" }, value_to_b);
35993600
} else {
3601+
log_trace!(logger, " ...trimming {} output with value {} due to dust limit", if local { "to_remote" } else { "to_local" }, value_to_b);
36003602
value_to_b = 0;
36013603
}
36023604

@@ -3610,21 +3612,41 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
36103612
value_to_a,
36113613
value_to_b,
36123614
feerate_per_kw,
3613-
included_non_dust_htlcs.iter_mut().map(|(htlc, _)| htlc),
3615+
included_non_dust_htlcs.into_iter(),
36143616
&channel_parameters,
36153617
&self.secp_ctx,
36163618
);
3617-
let mut htlcs_included = included_non_dust_htlcs;
3618-
// The unwrap is safe, because all non-dust HTLCs have been assigned an output index
3619-
htlcs_included.sort_unstable_by_key(|h| h.0.transaction_output_index.unwrap());
3620-
htlcs_included.append(&mut included_dust_htlcs);
3619+
3620+
#[cfg(debug_assertions)]
3621+
{
3622+
// Make sure that the to_self/to_remote is always either past the appropriate
3623+
// channel_reserve *or* it is making progress towards it.
3624+
let mut broadcaster_max_commitment_tx_output = if generated_by_local {
3625+
funding.holder_max_commitment_tx_output.lock().unwrap()
3626+
} else {
3627+
funding.counterparty_max_commitment_tx_output.lock().unwrap()
3628+
};
3629+
debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat || value_to_self_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap());
3630+
broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat);
3631+
debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat || value_to_remote_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis);
3632+
broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat);
3633+
}
3634+
3635+
htlcs_in_tx.sort_unstable_by(|(htlc_a, _), (htlc_b, _)| {
3636+
match (htlc_a.transaction_output_index, htlc_b.transaction_output_index) {
3637+
// `None` is smaller than `Some`, but we want `Some` ordered before `None` in the vector
3638+
(None, Some(_)) => cmp::Ordering::Greater,
3639+
(Some(_), None) => cmp::Ordering::Less,
3640+
(l, r) => cmp::Ord::cmp(&l, &r),
3641+
}
3642+
});
36213643

36223644
CommitmentStats {
36233645
tx,
36243646
feerate_per_kw,
36253647
total_fee_sat,
36263648
num_nondust_htlcs,
3627-
htlcs_included,
3649+
htlcs_included: htlcs_in_tx,
36283650
local_balance_msat: value_to_self_msat,
36293651
remote_balance_msat: value_to_remote_msat,
36303652
inbound_htlc_preimages,

0 commit comments

Comments
 (0)