Put a 10_000vByte cap on `HolderHTLCOutput` 0FC package templates #4129

tankyleo · 2025-09-25T20:25:26Z

Otherwise, we could potentially hit the max 10_000vB size limit on V3 transactions (BIP 431 rule 4) if we aggregate enough HTLC-claims together.

ldk-reviews-bot · 2025-09-25T20:25:28Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-09-25T20:35:30Z

Codecov Report

❌ Patch coverage is 97.37374% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.83%. Comparing base (19a9dbd) to head (386075d).
⚠️ Report is 80 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/events/bump_transaction/mod.rs	95.78%	4 Missing and 4 partials ⚠️
lightning/src/ln/zero_fee_commitment_tests.rs	97.92%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4129      +/-   ##
==========================================
+ Coverage   88.63%   88.83%   +0.20%     
==========================================
  Files         180      180              
  Lines      134878   137390    +2512     
  Branches   134878   137390    +2512     
==========================================
+ Hits       119543   122048    +2505     
+ Misses      12567    12529      -38     
- Partials     2768     2813      +45

Flag	Coverage Δ
fuzzing	`21.50% <0.00%> (-0.26%)`	⬇️
tests	`88.67% <97.37%> (+0.20%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

TheBlueMatt · 2025-09-25T22:21:25Z

lightning/src/chain/package.rs

+						// See rust-bitcoin to_vbytes_ceil
+						let self_vbytes = (self.weight() + 3) / 4; // This is the weight of the witnesses alone, we need to add more here
+						let other_vbytes = (other.weight() + 3) / 4;
+						// What is a good offset to use here to leave room for the user-provided input-output pair?


Hmmmmmmmmm, I wonder if we shouldn't just not merge and then do the work in the events::bump_transaction module and add like a needs_v3_transaction: Option<()> in the event? That way its at least pretty explicit.

You mean aggregate in bump_transaction ? How about restrict ourselves to aggregate max 25 HTLCs ? This should be good for any conceivable user-provided input-output pair.

No as in break an aggregation in bump_transaction

ldk-reviews-bot · 2025-09-30T14:48:43Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt · 2025-10-06T15:12:49Z

lightning/src/events/bump_transaction/mod.rs

+					// Cap the size of transactions claiming `HolderHTLCOutput` in 0FC channels.
+					// Otherwise, we could hit the max 10_000vB size limit on V3 transactions
+					// (BIP 431 rule 4).
+					25


I guess this is okay, but I thought the point of pulling this out into the bump_transaction logic is so that we could do the split after we see the inputs the user wants to contribute so that we can get close to 10k without exceeding it, rather than picking some random "max overhead" constant.

Thanks yea let me know the commit below sounds

carlaKC

approach lgtm

lightning/src/events/bump_transaction/mod.rs

carlaKC · 2025-10-09T15:19:38Z

lightning/src/events/bump_transaction/mod.rs

+			let expected_signed_tx_weight = unsigned_tx_weight + total_satisfaction_weight;
+			if expected_signed_tx_weight >= max_weight {
+				let extra_weight = expected_signed_tx_weight - max_weight;
+				let htlcs_to_remove = (extra_weight / chan_utils::htlc_timeout_tx_weight(channel_type)


Not critical, but I think these weights include transaction overhead (version etc) so we'll overcount by 40 wu per htlc?

Good catch, we should just use the weight of the HTLC input-output pair

thanks ! yes this likely resolves the weird + 2 I've got below

lightning/src/ln/zero_fee_commitment_tests.rs

wpaulino · 2025-10-09T17:48:06Z

lightning/src/events/bump_transaction/mod.rs

+			if expected_signed_tx_weight >= max_weight {
+				let extra_weight = expected_signed_tx_weight - max_weight;
+				let htlcs_to_remove = (extra_weight / chan_utils::htlc_timeout_tx_weight(channel_type)
+					// If we remove extra_weight / timeout_weight + 1 we sometimes still land above max_weight


I assume this wouldn't be an issue if we just round up the integer division?

likely it's because htlc_timeout_tx_weight is the weight of the full-transaction, but we want to only count the bytes from the single input-output pair

wpaulino · 2025-10-09T17:50:42Z

lightning/src/events/bump_transaction/mod.rs

+			// (BIP 431 rule 4).
+			chan_utils::TRUC_MAX_WEIGHT
+		} else {
+			u64::MAX


Might as well cap this to the max weight we can relay?

wpaulino · 2025-10-09T17:56:35Z

lightning/src/events/bump_transaction/mod.rs

+				engine.input(&htlc.commitment_txid.to_byte_array());
+				engine.input(&htlc.htlc.transaction_output_index.unwrap().to_be_bytes());
+			}
+			let utxo_id = ClaimId(Sha256::from_engine(engine).to_byte_array());


Wouldn't we want to reuse the same ID until we move on to the next batch?

Yea, was just wondering this - should we use the one from the event at least until we split the batch, which would work in the common case. Might want to sort the outputs so that we use them in a deterministic order (I imagine they are in the event, but...)

addressed below, don't quiet yet sort the HTLCs, need to look into whether they are already deterministic in the event.

TheBlueMatt · 2025-10-09T18:38:35Z

lightning/src/events/bump_transaction/mod.rs

+				});
+				htlc_tx.input.push(htlc_input);
+				let htlc_output = htlc_descriptor.tx_output(&self.secp);
+				htlc_tx.output.push(htlc_output);


Do we want to break out of this loop early if we get close to the max_weight? Would mean generating the claim ID during this loop rather than separately but that's trivial. Would avoid an extra coin-selection pass in a few cases, but its not critical.

addressed below

tankyleo · 2025-10-10T17:18:23Z

@wpaulino Carla raised this question: what do we do for the 1000vB limit on the anchor transaction ? For now I opted for documentation + debug assertion in 952a6b1
Since it's a much tighter limit that the 10_000vB max for HTLC claims, I was wondering if we add some kind of weight budget to the select_confirmed_utxos call. Or maybe add a different call entirely for this specific case ?

carlaKC

I was wondering if we add some kind of weight budget to the select_confirmed_utxos call.

This is something I was thinking about when reviewing this PR, seems reasonable for the anchor bump case where we don't have any outputs that we could trim.

For HTLC batching, the current approach of selecting a set of HTLCs we think will fit still seems appropriate, but a weight limit would be nice to have.

carlaKC · 2025-10-14T18:02:25Z

lightning/src/ln/chan_utils.rs

+	const TXIN_WEIGHT: u64 = 41 * WITNESS_SCALE_FACTOR as u64;
+	const TXOUT_WEIGHT: u64 = 43 * WITNESS_SCALE_FACTOR as u64;
+	let witness_weight =
+		if channel_type_features.supports_anchors_zero_fee_htlc_tx() { 327 } else { 324 };


nit: could use HTLC_SUCCESS_INPUT_KEYED_ANCHOR_WITNESS_WEIGHT and HTLC_SUCCESS_INPUT_P2A_ANCHOR_WITNESS_WEIGHT here (+ the timeout versions in htlc_timeout_input_output_pair_weight).

thanks again went ahead and cleanup the constants, see below

lightning/src/events/bump_transaction/mod.rs

ldk-reviews-bot · 2025-10-15T00:01:02Z

🔔 2nd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tankyleo · 2025-10-15T06:09:02Z

I added a max_tx_weight constraint to the public API at fn select_confirmed_utxos.

Bitcoin Core's fundrawtransaction RPC which we use as reference has a max_tx_weight parameter.
User can explicitly return an error if they can't satisfy this limit, and avoid unnecessarily locking utxos, see select_confirmed_utxos_internal.
Can communicate the TRUC_CHILD_MAX_WEIGHT limit to the user when funding a V3 anchor tx.

lightning/src/events/bump_transaction/mod.rs

TheBlueMatt · 2025-10-15T14:10:23Z

lightning/src/events/bump_transaction/mod.rs

 	///    provided, in which case a zero-value empty OP_RETURN output can be used instead.
 	/// 3. Enough inputs must be selected/contributed for the resulting transaction (including the
 	///    inputs and outputs noted above) to meet `target_feerate_sat_per_1000_weight`.
+	/// 4. The final transaction must have a weight smaller than `max_tx_weight`.


This could probably use additional docs. Something about it being okay to return an Err if it can't be met and that it'll be driven again.

tankyleo · 2025-10-15T15:56:56Z

BDK does not yet have an equivalent max_tx_weight parameter, I can file an issue there if we do go for this parameter on our own API.

TheBlueMatt · 2025-10-15T16:51:16Z

BDK does not yet have an equivalent max_tx_weight parameter, I can file an issue there if we do go for this parameter on our own API.

Yea, it would be nice, but also I think its okay, it should be rare for the coin selector to give us 11KvB in inputs...

tankyleo · 2025-10-15T17:15:14Z

BDK does not yet have an equivalent max_tx_weight parameter, I can file an issue there if we do go for this parameter on our own API.

Yea, it would be nice, but also I think its okay, it should be rare for the coin selector to give us 11KvB in inputs...

The bottleneck is the anchor tx - that one is limited to 1000vB. Should be fine for most single-sig wallets.

wpaulino · 2025-10-15T16:48:52Z

lightning/src/ln/chan_utils.rs

+pub const TRUC_CHILD_MAX_WEIGHT: u64 = 1000 * WITNESS_SCALE_FACTOR as u64;
+
+/// The maximum weight of any standard transaction, see bitcoin/src/policy/policy.h
+pub const MAX_STANDARD_TX_WEIGHT: u64 = 400_000;


Already defined as bitcoin::policy::MAX_STANDARD_TX_WEIGHT

wpaulino · 2025-10-15T16:55:11Z

lightning/src/ln/chan_utils.rs

 }

+/// Gets the weight of a single input-output pair in HTLC-success transactions
+pub fn htlc_success_input_output_pair_weight(channel_type_features: &ChannelTypeFeatures) -> u64 {


This method is only intended to be used for anchor channels but the name doesn't make that clear

aggregated_htlc_success_input_output_pair_weight ?

wpaulino · 2025-10-15T17:01:43Z

lightning/src/events/bump_transaction/mod.rs

+		let change_script = self.source.get_change_script().await?;
+		let max_utxo_selection_weight = max_tx_weight
+			- preexisting_tx_weight
+			- (change_script.len() as u64 + 8 + 1) * WITNESS_SCALE_FACTOR as u64;


I understand we're being conservative here by considering the change script weight even though we may not end up having one. Unfortunately, calling this too early before we know if we need it could lead to address inflation.

wpaulino · 2025-10-15T17:08:23Z

lightning/src/events/bump_transaction/mod.rs

 			}
-			if selected_amount < target_amount_sat + total_fees {
+			if selected_amount < target_amount_sat + total_fees
+				|| selected_utxos_weight > max_utxo_selection_weight


Would be nice to have weight condition logged separately

I'm going to keep a single log that mentions both conditions. For example we could fail to meet the target feerate condition because we've been trimming UTXOs to satisfy the max weight condition.

wpaulino · 2025-10-15T17:14:53Z

lightning/src/events/bump_transaction/mod.rs

+					let min_weight_to_remove = USER_COINS_WEIGHT_BUDGET;
+					let htlcs_to_remove = min_weight_to_remove
+						/ chan_utils::htlc_timeout_input_output_pair_weight(channel_type)
+						+ 1;


The idea here was to remove at least USER_COINS_WEIGHT_BUDGET from the total preexisting tx size.

wpaulino · 2025-10-15T17:18:11Z

lightning/src/events/bump_transaction/mod.rs

+			let utxo_id = if broadcasted_htlcs == 0 {
+				claim_id


It's not just the first case. For every time we start a new batch, we should reuse claim_id until we move on to the next.

wpaulino · 2025-10-15T17:27:57Z

lightning/src/ln/zero_fee_commitment_tests.rs

 }
+
+#[test]
+fn test_htlc_claim_chunking() {


I'd also like to see tests for when too many inputs are contributed so we have to lower the batch size

ldk-reviews-bot · 2025-10-18T00:01:06Z

🔔 1st Reminder

Hey @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-10-20T00:01:39Z

🔔 2nd Reminder

Hey @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

lightning/src/events/bump_transaction/mod.rs

carlaKC · 2025-10-21T13:42:04Z

lightning/src/events/bump_transaction/mod.rs

 			}
+			while !selected_utxos.is_empty()
+				&& selected_amount - selected_utxos.front().unwrap().0.output.value
+					>= target_amount_sat + total_fees


Should this be total_fees - selected_utxos.front().unwrap().1 to also remove the fees for this utxo if we're going to drop it?

yes ! thank you

lightning/src/events/bump_transaction/mod.rs

carlaKC

lgtm, two non-blocking micro-nits

carlaKC · 2025-10-21T19:48:26Z

lightning/src/events/bump_transaction/mod.rs

+			.ok_or_else(|| {
+				log_debug!(
+					self.logger,
+					"max_tx_weight is too small to accomodate the preexisting tx weight plus a P2WSH/P2TR output"


s/accomodate/accommodate

carlaKC · 2025-10-21T19:49:50Z

lightning/src/events/bump_transaction/mod.rs

+	/// 4. The final transaction must have a weight smaller than `max_tx_weight`; if this
+	///    constraint can't be met, return an `Err`. In the case of counterparty-signed HTLC
+	///    transactions, we will remove a chunk of HTLCs and try your algorithm again. Anchor
+	///    transactions cannot be trimmed, so if you fail to meet the `max_tx_weight` we will


nit: "fail to meet max_tx_weight" makes it sound like this is a goal that we want to reach, rather than a limit we should stay under.

Otherwise, we could hit the max 10_000vB size limit on V3 transactions (BIP 431 rule 4). Also introduce a `max_tx_weight` parameter to `select_confirmed_utxos`. This constraint makes sure anchor and HTLC transactions in 0FC channels satisfy the `TRUC_MAX_WEIGHT` and the `TRUC_CHILD_MAX_WEIGHT` maximums. Expand the coin-selection algorithm provided for any `T: WalletSource` to satisfy this new constraint.

ldk-reviews-bot · 2025-10-23T22:05:53Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tankyleo requested review from TheBlueMatt and carlaKC September 25, 2025 20:26

TheBlueMatt added this to the 0.2 milestone Sep 25, 2025

TheBlueMatt reviewed Sep 25, 2025

View reviewed changes

tankyleo force-pushed the 2025-09-cap-0fc-holder-htlc-txs branch 2 times, most recently from 63e8f1c to db273bb Compare September 26, 2025 11:54

tankyleo requested review from TheBlueMatt and removed request for TheBlueMatt and carlaKC September 26, 2025 13:04

tankyleo force-pushed the 2025-09-cap-0fc-holder-htlc-txs branch from db273bb to 107eb25 Compare September 28, 2025 14:24

tankyleo requested a review from TheBlueMatt September 28, 2025 14:47

tankyleo removed the request for review from TheBlueMatt October 1, 2025 07:29

TheBlueMatt reviewed Oct 6, 2025

View reviewed changes

tankyleo force-pushed the 2025-09-cap-0fc-holder-htlc-txs branch from 107eb25 to c222c41 Compare October 9, 2025 14:15

tankyleo marked this pull request as ready for review October 9, 2025 14:17

ldk-reviews-bot requested a review from jkczyz October 9, 2025 14:17

tankyleo requested review from TheBlueMatt and removed request for jkczyz October 9, 2025 14:17

carlaKC self-requested a review October 9, 2025 14:41

TheBlueMatt added the backport 0.2 label Oct 9, 2025

carlaKC reviewed Oct 9, 2025

View reviewed changes

wpaulino reviewed Oct 9, 2025

View reviewed changes

TheBlueMatt reviewed Oct 9, 2025

View reviewed changes

tankyleo force-pushed the 2025-09-cap-0fc-holder-htlc-txs branch from 8d2b31c to 4d54289 Compare October 10, 2025 17:13

tankyleo requested review from carlaKC and wpaulino October 10, 2025 17:16

carlaKC previously approved these changes Oct 14, 2025

View reviewed changes

tankyleo dismissed carlaKC’s stale review via 8614904 October 14, 2025 20:04

tankyleo force-pushed the 2025-09-cap-0fc-holder-htlc-txs branch from 952a6b1 to 8614904 Compare October 14, 2025 20:04

tankyleo force-pushed the 2025-09-cap-0fc-holder-htlc-txs branch from 8614904 to dcc361d Compare October 15, 2025 05:53

tankyleo force-pushed the 2025-09-cap-0fc-holder-htlc-txs branch 3 times, most recently from 3fadf50 to 837b9cc Compare October 15, 2025 06:57

tankyleo requested review from TheBlueMatt and carlaKC October 15, 2025 07:01

TheBlueMatt reviewed Oct 15, 2025

View reviewed changes

lightning/src/events/bump_transaction/mod.rs Show resolved Hide resolved

TheBlueMatt reviewed Oct 15, 2025

View reviewed changes

wpaulino reviewed Oct 15, 2025

View reviewed changes

tankyleo added this to Weekly Goals Oct 16, 2025

tankyleo force-pushed the 2025-09-cap-0fc-holder-htlc-txs branch from 3ed04aa to 92d1ce7 Compare October 21, 2025 07:57

carlaKC reviewed Oct 21, 2025

View reviewed changes

tankyleo requested a review from carlaKC October 21, 2025 18:28

carlaKC previously approved these changes Oct 21, 2025

View reviewed changes

tankyleo dismissed carlaKC’s stale review via 386075d October 21, 2025 21:55

tankyleo force-pushed the 2025-09-cap-0fc-holder-htlc-txs branch from bdfedc4 to 386075d Compare October 21, 2025 21:55

tankyleo requested a review from TheBlueMatt October 21, 2025 22:04

tankyleo mentioned this pull request Oct 22, 2025

[TRUC Transactions]: Specify the Maximum Acceptable Weight When Building a Transaction bitcoindevkit/bdk_wallet#332

Open

Put a 10_000vByte cap on HolderHTLCOutput 0FC package templates #4129

Are you sure you want to change the base?

Put a 10_000vByte cap on HolderHTLCOutput 0FC package templates #4129

Uh oh!

Conversation

tankyleo commented Sep 25, 2025

Uh oh!

ldk-reviews-bot commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Sep 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlaKC left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlaKC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldk-reviews-bot commented Oct 15, 2025

Uh oh!

tankyleo commented Oct 15, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo commented Oct 15, 2025

Uh oh!

TheBlueMatt commented Oct 15, 2025

Uh oh!

tankyleo commented Oct 15, 2025

Uh oh!

Choose a reason for hiding this comment

Put a 10_000vByte cap on `HolderHTLCOutput` 0FC package templates #4129

Put a 10_000vByte cap on `HolderHTLCOutput` 0FC package templates #4129

ldk-reviews-bot commented Sep 25, 2025 •

edited

Loading

codecov bot commented Sep 25, 2025 •

edited

Loading

tankyleo commented Oct 10, 2025 •

edited

Loading