Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 8bc75b9

Browse files
committedApr 14, 2025·
Remove exclusive reference and generic from CommitmentTransaction API
This commit reworks how channel is told about which HTLCs were assigned which index upon the build of a `CommitmentTransaction`. Previously, `CommitmentTransaction` was given an exclusive reference to channel's HTLC-source table, and `CommitmentTransaction` populated the transaction output indices in that table via this exclusive reference. As a result, the public API of `CommitmentTransaction` included a generic parameter, and an exclusive reference. We remove both of these in preparation for the upcoming `TxBuilder` trait. This cleans up the API, and makes it more bindings-friendly. Henceforth, channel populates the HTLC-source table via a brute-force search of each htlc in `CommitmentTransaction`. This is an O(n^2) operation, but n is small enough that we ignore the performance hit. We also take this opportunity to cleanup how we build and sort the commitment transaction outputs together with the htlc output data in `CommitmentTransaction`. The goal is to keep the number of vector allocations to a minimum; we now only allocate a single vector to hold the transaction outputs, and do all the sorting in-place.
1 parent ef0fcab commit 8bc75b9

File tree

6 files changed

+274
-158
lines changed

6 files changed

+274
-158
lines changed
 

‎lightning/src/chain/channelmonitor.rs

Lines changed: 27 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3588,15 +3588,15 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
35883588
to_broadcaster_value,
35893589
to_countersignatory_value,
35903590
)| {
3591-
let htlc_outputs = vec![];
3591+
let nondust_htlcs = vec![];
35923592

35933593
let commitment_tx = self.build_counterparty_commitment_tx(
35943594
INITIAL_COMMITMENT_NUMBER,
35953595
&their_per_commitment_point,
35963596
to_broadcaster_value,
35973597
to_countersignatory_value,
35983598
feerate_per_kw,
3599-
htlc_outputs,
3599+
nondust_htlcs,
36003600
);
36013601
// Take the opportunity to populate this recently introduced field
36023602
self.initial_counterparty_commitment_tx = Some(commitment_tx.clone());
@@ -3609,11 +3609,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
36093609
fn build_counterparty_commitment_tx(
36103610
&self, commitment_number: u64, their_per_commitment_point: &PublicKey,
36113611
to_broadcaster_value: u64, to_countersignatory_value: u64, feerate_per_kw: u32,
3612-
mut nondust_htlcs: Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>
3612+
nondust_htlcs: Vec<HTLCOutputInCommitment>
36133613
) -> CommitmentTransaction {
36143614
let channel_parameters = &self.funding.channel_parameters.as_counterparty_broadcastable();
3615-
CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number, their_per_commitment_point,
3616-
to_broadcaster_value, to_countersignatory_value, feerate_per_kw, &mut nondust_htlcs, channel_parameters, &self.onchain_tx_handler.secp_ctx)
3615+
CommitmentTransaction::new(commitment_number, their_per_commitment_point,
3616+
to_broadcaster_value, to_countersignatory_value, feerate_per_kw, nondust_htlcs, channel_parameters, &self.onchain_tx_handler.secp_ctx)
36173617
}
36183618

36193619
fn counterparty_commitment_txs_from_update(&self, update: &ChannelMonitorUpdate) -> Vec<CommitmentTransaction> {
@@ -3629,7 +3629,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
36293629
to_countersignatory_value_sat: Some(to_countersignatory_value) } => {
36303630

36313631
let nondust_htlcs = htlc_outputs.iter().filter_map(|(htlc, _)| {
3632-
htlc.transaction_output_index.map(|_| (htlc.clone(), None))
3632+
htlc.transaction_output_index.map(|_| htlc).cloned()
36333633
}).collect::<Vec<_>>();
36343634

36353635
let commitment_tx = self.build_counterparty_commitment_tx(commitment_number,
@@ -5620,21 +5620,21 @@ mod tests {
56205620
{
56215621
let mut res = Vec::new();
56225622
for (idx, preimage) in $preimages_slice.iter().enumerate() {
5623-
res.push((HTLCOutputInCommitment {
5623+
res.push(HTLCOutputInCommitment {
56245624
offered: true,
56255625
amount_msat: 0,
56265626
cltv_expiry: 0,
56275627
payment_hash: preimage.1.clone(),
56285628
transaction_output_index: Some(idx as u32),
5629-
}, ()));
5629+
});
56305630
}
56315631
res
56325632
}
56335633
}
56345634
}
56355635
macro_rules! preimages_slice_to_htlc_outputs {
56365636
($preimages_slice: expr) => {
5637-
preimages_slice_to_htlcs!($preimages_slice).into_iter().map(|(htlc, _)| (htlc, None)).collect()
5637+
preimages_slice_to_htlcs!($preimages_slice).into_iter().map(|htlc| (htlc, None)).collect()
56385638
}
56395639
}
56405640
let dummy_sig = crate::crypto::utils::sign(&secp_ctx,
@@ -5690,15 +5690,17 @@ mod tests {
56905690
let best_block = BestBlock::from_network(Network::Testnet);
56915691
let monitor = ChannelMonitor::new(
56925692
Secp256k1::new(), keys, Some(shutdown_script.into_inner()), 0, &ScriptBuf::new(),
5693-
&channel_parameters, true, 0, HolderCommitmentTransaction::dummy(0, &mut Vec::new()),
5693+
&channel_parameters, true, 0, HolderCommitmentTransaction::dummy(0, Vec::new()),
56945694
best_block, dummy_key, channel_id,
56955695
);
56965696

5697-
let mut htlcs = preimages_slice_to_htlcs!(preimages[0..10]);
5698-
let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs);
5697+
let nondust_htlcs = preimages_slice_to_htlcs!(preimages[0..10]);
5698+
let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, nondust_htlcs);
5699+
// These HTLCs now have their output indices assigned
5700+
let nondust_htlcs = dummy_commitment_tx.nondust_htlcs();
56995701

57005702
monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(),
5701-
htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), Some(dummy_source.clone()))).collect());
5703+
nondust_htlcs.into_iter().map(|htlc| (htlc.clone(), Some(dummy_sig), Some(dummy_source.clone()))).collect());
57025704
monitor.provide_latest_counterparty_commitment_tx(Txid::from_byte_array(Sha256::hash(b"1").to_byte_array()),
57035705
preimages_slice_to_htlc_outputs!(preimages[5..15]), 281474976710655, dummy_key, &logger);
57045706
monitor.provide_latest_counterparty_commitment_tx(Txid::from_byte_array(Sha256::hash(b"2").to_byte_array()),
@@ -5733,21 +5735,25 @@ mod tests {
57335735

57345736
// Now update holder commitment tx info, pruning only element 18 as we still care about the
57355737
// previous commitment tx's preimages too
5736-
let mut htlcs = preimages_slice_to_htlcs!(preimages[0..5]);
5737-
let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs);
5738+
let nondust_htlcs = preimages_slice_to_htlcs!(preimages[0..5]);
5739+
let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, nondust_htlcs);
5740+
// These HTLCs now have their output indices assigned
5741+
let nondust_htlcs = dummy_commitment_tx.nondust_htlcs();
57385742
monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(),
5739-
htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), Some(dummy_source.clone()))).collect());
5743+
nondust_htlcs.into_iter().map(|htlc| (htlc.clone(), Some(dummy_sig), Some(dummy_source.clone()))).collect());
57405744
secret[0..32].clone_from_slice(&<Vec<u8>>::from_hex("2273e227a5b7449b6e70f1fb4652864038b1cbf9cd7c043a7d6456b7fc275ad8").unwrap());
57415745
monitor.provide_secret(281474976710653, secret.clone()).unwrap();
57425746
assert_eq!(monitor.inner.lock().unwrap().payment_preimages.len(), 12);
57435747
test_preimages_exist!(&preimages[0..10], monitor);
57445748
test_preimages_exist!(&preimages[18..20], monitor);
57455749

57465750
// But if we do it again, we'll prune 5-10
5747-
let mut htlcs = preimages_slice_to_htlcs!(preimages[0..3]);
5748-
let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, &mut htlcs);
5749-
monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx,
5750-
htlcs.into_iter().map(|(htlc, _)| (htlc, Some(dummy_sig), Some(dummy_source.clone()))).collect());
5751+
let nondust_htlcs = preimages_slice_to_htlcs!(preimages[0..3]);
5752+
let dummy_commitment_tx = HolderCommitmentTransaction::dummy(0, nondust_htlcs);
5753+
// These HTLCs now have their output indices assigned
5754+
let nondust_htlcs = dummy_commitment_tx.nondust_htlcs();
5755+
monitor.provide_latest_holder_commitment_tx(dummy_commitment_tx.clone(),
5756+
nondust_htlcs.into_iter().map(|htlc| (htlc.clone(), Some(dummy_sig), Some(dummy_source.clone()))).collect());
57515757
secret[0..32].clone_from_slice(&<Vec<u8>>::from_hex("27cddaa5624534cb6cb9d7da077cf2b22ab21e9b506fd4998a51d54502e99116").unwrap());
57525758
monitor.provide_secret(281474976710652, secret.clone()).unwrap();
57535759
assert_eq!(monitor.inner.lock().unwrap().payment_preimages.len(), 5);
@@ -5942,7 +5948,7 @@ mod tests {
59425948
let best_block = BestBlock::from_network(Network::Testnet);
59435949
let monitor = ChannelMonitor::new(
59445950
Secp256k1::new(), keys, Some(shutdown_script.into_inner()), 0, &ScriptBuf::new(),
5945-
&channel_parameters, true, 0, HolderCommitmentTransaction::dummy(0, &mut Vec::new()),
5951+
&channel_parameters, true, 0, HolderCommitmentTransaction::dummy(0, Vec::new()),
59465952
best_block, dummy_key, channel_id,
59475953
);
59485954

‎lightning/src/chain/onchaintx.rs

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1296,22 +1296,21 @@ mod tests {
12961296

12971297
// Create an OnchainTxHandler for a commitment containing HTLCs with CLTV expiries of 0, 1,
12981298
// and 2 blocks.
1299-
let mut htlcs = Vec::new();
1299+
let mut nondust_htlcs = Vec::new();
13001300
for i in 0..3 {
13011301
let preimage = PaymentPreimage([i; 32]);
13021302
let hash = PaymentHash(Sha256::hash(&preimage.0[..]).to_byte_array());
1303-
htlcs.push((
1303+
nondust_htlcs.push(
13041304
HTLCOutputInCommitment {
13051305
offered: true,
13061306
amount_msat: 10000,
13071307
cltv_expiry: i as u32,
13081308
payment_hash: hash,
13091309
transaction_output_index: Some(i as u32),
1310-
},
1311-
(),
1312-
));
1310+
}
1311+
);
13131312
}
1314-
let holder_commit = HolderCommitmentTransaction::dummy(1000000, &mut htlcs);
1313+
let holder_commit = HolderCommitmentTransaction::dummy(1000000, nondust_htlcs);
13151314
let destination_script = ScriptBuf::new();
13161315
let mut tx_handler = OnchainTxHandler::new(
13171316
1000000,

‎lightning/src/chain/package.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1712,7 +1712,7 @@ mod tests {
17121712
payment_hash: PaymentHash::from(preimage),
17131713
transaction_output_index: None,
17141714
};
1715-
let commitment_tx = HolderCommitmentTransaction::dummy(0, &mut vec![(htlc.clone(), ())]);
1715+
let commitment_tx = HolderCommitmentTransaction::dummy(0, vec![htlc.clone()]);
17161716
let trusted_tx = commitment_tx.trust();
17171717
PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build(
17181718
HTLCDescriptor {
@@ -1746,7 +1746,7 @@ mod tests {
17461746
payment_hash: PaymentHash::from(PaymentPreimage([2;32])),
17471747
transaction_output_index: None,
17481748
};
1749-
let commitment_tx = HolderCommitmentTransaction::dummy(0, &mut vec![(htlc.clone(), ())]);
1749+
let commitment_tx = HolderCommitmentTransaction::dummy(0, vec![htlc.clone()]);
17501750
let trusted_tx = commitment_tx.trust();
17511751
PackageSolvingData::HolderHTLCOutput(HolderHTLCOutput::build(
17521752
HTLCDescriptor {
@@ -1770,7 +1770,7 @@ mod tests {
17701770

17711771
macro_rules! dumb_funding_output {
17721772
() => {{
1773-
let commitment_tx = HolderCommitmentTransaction::dummy(0, &mut Vec::new());
1773+
let commitment_tx = HolderCommitmentTransaction::dummy(0, Vec::new());
17741774
let mut channel_parameters = ChannelTransactionParameters::test_dummy(0);
17751775
channel_parameters.channel_type_features = ChannelTypeFeatures::only_static_remote_key();
17761776
PackageSolvingData::HolderFundingOutput(HolderFundingOutput::build(

‎lightning/src/ln/chan_utils.rs

Lines changed: 187 additions & 104 deletions
Large diffs are not rendered by default.

‎lightning/src/ln/channel.rs

Lines changed: 44 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3956,9 +3956,9 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
39563956
remote_balance_before_fee_anchors_msat
39573957
} = stats;
39583958

3959-
let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new();
39603959
let num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len();
3961-
let mut included_non_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs);
3960+
let mut htlcs_included: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs);
3961+
let mut nondust_htlcs: Vec<HTLCOutputInCommitment> = Vec::with_capacity(num_htlcs);
39623962

39633963
log_trace!(logger, "Building commitment transaction number {} (really {} xor {}) for channel {} for {}, generated by {} with fee {}...",
39643964
commitment_number, (INITIAL_COMMITMENT_NUMBER - commitment_number),
@@ -3981,12 +3981,12 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
39813981
macro_rules! add_htlc_output {
39823982
($htlc: expr, $outbound: expr, $source: expr) => {
39833983
let htlc_in_tx = get_htlc_in_commitment!($htlc, $outbound == local);
3984+
htlcs_included.push((htlc_in_tx.clone(), $source));
39843985
if $htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) {
39853986
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);
3986-
included_dust_htlcs.push((htlc_in_tx, $source));
39873987
} else {
39883988
log_trace!(logger, " ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $htlc.state, $htlc.htlc_id, $htlc.payment_hash, $htlc.amount_msat);
3989-
included_non_dust_htlcs.push((htlc_in_tx, $source));
3989+
nondust_htlcs.push(htlc_in_tx);
39903990
}
39913991
}
39923992
}
@@ -4047,19 +4047,47 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
40474047
let channel_parameters =
40484048
if local { funding.channel_transaction_parameters.as_holder_broadcastable() }
40494049
else { funding.channel_transaction_parameters.as_counterparty_broadcastable() };
4050-
let tx = CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number,
4051-
per_commitment_point,
4052-
to_broadcaster_value_sat,
4053-
to_countersignatory_value_sat,
4054-
feerate_per_kw,
4055-
&mut included_non_dust_htlcs,
4056-
&channel_parameters,
4057-
&self.secp_ctx,
4050+
let tx = CommitmentTransaction::new(
4051+
commitment_number,
4052+
per_commitment_point,
4053+
to_broadcaster_value_sat,
4054+
to_countersignatory_value_sat,
4055+
feerate_per_kw,
4056+
nondust_htlcs,
4057+
&channel_parameters,
4058+
&self.secp_ctx,
40584059
);
4059-
let mut htlcs_included = included_non_dust_htlcs;
4060-
// The unwrap is safe, because all non-dust HTLCs have been assigned an output index
4061-
htlcs_included.sort_unstable_by_key(|h| h.0.transaction_output_index.unwrap());
4062-
htlcs_included.append(&mut included_dust_htlcs);
4060+
4061+
// This populates the HTLC-source table with the indices from the HTLCs in the commitment
4062+
// transaction.
4063+
//
4064+
// This brute-force search is O(n^2) over ~1k HTLCs in the worst case. This case is very
4065+
// rare at the moment.
4066+
for nondust_htlc in tx.nondust_htlcs() {
4067+
let htlc = htlcs_included
4068+
.iter_mut()
4069+
.filter(|(htlc, _source)| htlc.transaction_output_index.is_none())
4070+
.find_map(|(htlc, _source)| {
4071+
if htlc.is_data_equal(nondust_htlc) {
4072+
Some(htlc)
4073+
} else {
4074+
None
4075+
}
4076+
})
4077+
.unwrap();
4078+
htlc.transaction_output_index = Some(nondust_htlc.transaction_output_index.unwrap());
4079+
}
4080+
4081+
// This places the non-dust HTLC-source pairs first, in the order they appear in the
4082+
// commitment transaction, followed by the dust HTLC-source pairs.
4083+
htlcs_included.sort_unstable_by(|(htlc_a, _), (htlc_b, _)| {
4084+
match (htlc_a.transaction_output_index, htlc_b.transaction_output_index) {
4085+
// `None` is smaller than `Some`, but we want `Some` ordered before `None` in the vector
4086+
(None, Some(_)) => cmp::Ordering::Greater,
4087+
(Some(_), None) => cmp::Ordering::Less,
4088+
(l, r) => cmp::Ord::cmp(&l, &r),
4089+
}
4090+
});
40634091

40644092
CommitmentData {
40654093
tx,

‎lightning/src/ln/functional_tests.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -746,14 +746,14 @@ pub fn test_update_fee_that_funder_cannot_afford() {
746746
let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
747747
let local_chan = local_chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap();
748748
let local_chan_signer = local_chan.get_signer();
749-
let mut htlcs: Vec<(HTLCOutputInCommitment, ())> = vec![];
750-
let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data(
749+
let nondust_htlcs: Vec<HTLCOutputInCommitment> = vec![];
750+
let commitment_tx = CommitmentTransaction::new(
751751
INITIAL_COMMITMENT_NUMBER - 1,
752752
&remote_point,
753753
push_sats,
754754
channel_value - push_sats - commit_tx_fee_msat(non_buffer_feerate + 4, 0, &channel_type_features) / 1000,
755755
non_buffer_feerate + 4,
756-
&mut htlcs,
756+
nondust_htlcs,
757757
&local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable(),
758758
&secp_ctx,
759759
);
@@ -831,16 +831,16 @@ pub fn test_update_fee_that_saturates_subs() {
831831
let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
832832
let local_chan = local_chan_lock.channel_by_id.get(&chan_id).and_then(Channel::as_funded).unwrap();
833833
let local_chan_signer = local_chan.get_signer();
834-
let mut htlcs: Vec<(HTLCOutputInCommitment, ())> = vec![];
835-
let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data(
834+
let nondust_htlcs: Vec<HTLCOutputInCommitment> = vec![];
835+
let commitment_tx = CommitmentTransaction::new(
836836
INITIAL_COMMITMENT_NUMBER,
837837
&remote_point,
838838
8500,
839839
// Set a zero balance here: this is the transaction that node 1 will expect a signature for, as
840840
// he will do a saturating subtraction of the total fees from node 0's balance.
841841
0,
842842
FEERATE,
843-
&mut htlcs,
843+
nondust_htlcs,
844844
&local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable(),
845845
&secp_ctx,
846846
);
@@ -1565,13 +1565,13 @@ pub fn test_fee_spike_violation_fails_htlc() {
15651565
let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
15661566
let local_chan = local_chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap();
15671567
let local_chan_signer = local_chan.get_signer();
1568-
let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data(
1568+
let commitment_tx = CommitmentTransaction::new(
15691569
commitment_number,
15701570
&remote_point,
15711571
95000,
15721572
local_chan_balance,
15731573
feerate_per_kw,
1574-
&mut vec![(accepted_htlc_info, ())],
1574+
vec![accepted_htlc_info],
15751575
&local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable(),
15761576
&secp_ctx,
15771577
);

0 commit comments

Comments
 (0)
Please sign in to comment.