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.24771% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.76%. Comparing base (19a9dbd) to head (c222c41).
⚠️ Report is 32 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/events/bump_transaction/mod.rs	96.63%	0 Missing and 4 partials ⚠️
lightning/src/ln/zero_fee_commitment_tests.rs	97.97%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4129      +/-   ##
==========================================
+ Coverage   88.63%   88.76%   +0.13%     
==========================================
  Files         180      180              
  Lines      134878   135660     +782     
  Branches   134878   135660     +782     
==========================================
+ Hits       119543   120425     +882     
+ Misses      12567    12467     -100     
  Partials     2768     2768

Flag	Coverage Δ
fuzzing	`21.71% <0.00%> (-0.05%)`	⬇️
tests	`88.61% <97.24%> (+0.14%)`	⬆️

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

Otherwise, we could hit the max 10_000vB size limit on V3 transactions (BIP 431 rule 4).

carlaKC

approach lgtm

carlaKC · 2025-10-09T14:22:29Z

lightning/src/events/bump_transaction/mod.rs

+			// This matches the scheme used in `onchaintx.rs`, so for non-0fc-channels,
+			// this should match the `ClaimId` of the claim generated in `onchaintx.rs`.
+			let mut engine = Sha256::engine();
+			for htlc in htlcs {
+				engine.input(&htlc.commitment_txid.to_byte_array());
+				engine.input(&htlc.htlc.transaction_output_index.unwrap().to_be_bytes());
+			}


nit: pull out into ClaimID::from_prev_outs to de-duplicate?

carlaKC · 2025-10-09T14:28:59Z

lightning/src/events/bump_transaction/mod.rs

+			let unsigned_tx_weight =
+				htlc_tx.weight().to_wu() - (htlc_tx.input.len() as u64 * EMPTY_SCRIPT_SIG_WEIGHT);
+			let expected_signed_tx_weight = unsigned_tx_weight + total_satisfaction_weight;
+			if expected_signed_tx_weight >= max_weight {


Should this just be >? We're allowed to be on exactly 10,000 vbytes, just no larger.

I considered this, but went for the more conservative approach here.

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

carlaKC · 2025-10-09T17:45:35Z

lightning/src/ln/zero_fee_commitment_tests.rs

+	let _node_a_id = nodes[0].node.get_our_node_id();
+	let _node_b_id = nodes[1].node.get_our_node_id();


nit: unused vars unnecessary?

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...)

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.

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

Implement Holder HTLC claim chunking for 0FC channels

c222c41

Otherwise, we could hit the max 10_000vB size limit on V3 transactions (BIP 431 rule 4).

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

		let _node_a_id = nodes[0].node.get_our_node_id();
		let _node_b_id = nodes[1].node.get_our_node_id();

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

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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