Cleanup `PackageTemplate`a bit #3297

TheBlueMatt · 2024-09-06T01:04:42Z

Digging into better aggregation turned up a bunch of nonsense in PackageTemplate that is gonna lead to us breaking things in the future, so since I actually understand how the hell it works I figured I should clean it up a bit so someone else can understand it.

codecov · 2024-09-06T19:24:38Z

Codecov Report

Attention: Patch coverage is 99.28571% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.99%. Comparing base (bc1931b) to head (58d0ca3).
Report is 28 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/chain/onchaintx.rs	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3297      +/-   ##
==========================================
+ Coverage   89.62%   89.99%   +0.37%     
==========================================
  Files         127      127              
  Lines      103517   107521    +4004     
  Branches   103517   107521    +4004     
==========================================
+ Hits        92780    96768    +3988     
- Misses       8041     8145     +104     
+ Partials     2696     2608      -88

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

wvanlint · 2024-09-18T00:23:56Z

lightning/src/chain/package.rs

+	///
+	/// This is an important limit for aggregation as after this height our counterparty may be
+	/// able to pin transactions spending this output in the mempool.
+	pub(crate) fn counterparty_spendable_height(&self) -> u32 {
 		self.soonest_conf_deadline


Perhaps we can rename the variable as well?

I was also confused why it was not an Option, aren't there cases where the counterparty might be able to claim it immediately? However, the build_package calls don't seem to discriminate between those.

There's always some height at which a counterparty could claim an output, assuming they have the cryptographic material with which to do so, though at least for our outputs maybe we shouldn't set one (because our counterparty can only claim if its a stale output).

But, yes, the variable should be renamed, added two commits to clean more stuff up....

There's always some height at which a counterparty could claim an output, assuming they have the cryptographic material with which to do so, though at least for our outputs maybe we shouldn't set one (because our counterparty can only claim if its a stale output).

True, as in 0 or current_height for outputs without any CSV/CLTV locks? I was wondering if it was clearer to talk about the presence of any of those locks, but it's not really necessary.

I was also confused because the build_package call for holder HTLC outputs seems to always set counterparty_spendable_height to htlc.cltv_expiry, regardless if it was offered/received:

rust-lightning/lightning/src/chain/channelmonitor.rs

Lines 3668 to 3689 in cdd1298

let htlc_output = if htlc.offered {

let htlc_output = HolderHTLCOutput::build_offered(

htlc.amount_msat, htlc.cltv_expiry, self.onchain_tx_handler.channel_type_features().clone()

);

htlc_output

} else {

let payment_preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.payment_hash) {

preimage.clone()

} else {

// We can't build an HTLC-Success transaction without the preimage

continue;

};

let htlc_output = HolderHTLCOutput::build_accepted(

payment_preimage, htlc.amount_msat, self.onchain_tx_handler.channel_type_features().clone()

);

htlc_output

};

let htlc_package = PackageTemplate::build_package(

holder_tx.txid, transaction_output_index,

PackageSolvingData::HolderHTLCOutput(htlc_output),

htlc.cltv_expiry, conf_height

);

True, as in 0 or current_height for outputs without any CSV/CLTV locks? I was wondering if it was clearer to talk about the presence of any of those locks, but it's not really necessary.

Yea, it might be. I think its really only for our to_self output, and I'm not sure about changing it all for that...

I was also confused because the build_package call for holder HTLC outputs seems to always set counterparty_spendable_height to htlc.cltv_expiry, regardless if it was offered/received:

Ugh, yea, that's a bug too....this code is a mess.

lightning/src/chain/package.rs

wvanlint · 2024-09-18T17:51:27Z

lightning/src/chain/package.rs

+	///
+	/// This is an important limit for aggregation as after this height our counterparty may be
+	/// able to pin transactions spending this output in the mempool.
+	pub(crate) fn counterparty_spendable_height(&self) -> u32 {
 		self.soonest_conf_deadline


There's always some height at which a counterparty could claim an output, assuming they have the cryptographic material with which to do so, though at least for our outputs maybe we shouldn't set one (because our counterparty can only claim if its a stale output).

True, as in 0 or current_height for outputs without any CSV/CLTV locks? I was wondering if it was clearer to talk about the presence of any of those locks, but it's not really necessary.

I was also confused because the build_package call for holder HTLC outputs seems to always set counterparty_spendable_height to htlc.cltv_expiry, regardless if it was offered/received:

rust-lightning/lightning/src/chain/channelmonitor.rs

Lines 3668 to 3689 in cdd1298

let htlc_output = if htlc.offered {

let htlc_output = HolderHTLCOutput::build_offered(

htlc.amount_msat, htlc.cltv_expiry, self.onchain_tx_handler.channel_type_features().clone()

);

htlc_output

} else {

let payment_preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.payment_hash) {

preimage.clone()

} else {

// We can't build an HTLC-Success transaction without the preimage

continue;

};

let htlc_output = HolderHTLCOutput::build_accepted(

payment_preimage, htlc.amount_msat, self.onchain_tx_handler.channel_type_features().clone()

);

htlc_output

};

let htlc_package = PackageTemplate::build_package(

holder_tx.txid, transaction_output_index,

PackageSolvingData::HolderHTLCOutput(htlc_output),

htlc.cltv_expiry, conf_height

);

wvanlint · 2024-09-18T20:51:52Z

lightning/src/chain/channelmonitor.rs

@@ -3598,7 +3607,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 								self.counterparty_commitment_params.counterparty_htlc_base_key,
 								htlc.clone(), self.onchain_tx_handler.channel_type_features().clone()))
 					};
-					let counterparty_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, counterparty_htlc_outp, htlc.cltv_expiry, 0);
+					let counterparty_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, counterparty_htlc_outp, htlc.cltv_expiry);


Should the counterparty_spendable_height be adjusted here as well based on whether the htlc is offered or not?

Ideally yea, but passing the current height through to here is very annoying, and really it doesn't matter - if its a timeout claim our counterparty can claim it now, but we can't so having a counterparty_claimable_height of when we can both claim it is fine.

wvanlint · 2024-09-18T20:53:06Z

lightning/src/chain/channelmonitor.rs

+							commitment_txid,
+							transaction_output_index,
+							PackageSolvingData::RevokedHTLCOutput(revk_htlc_outp),
+							htlc.cltv_expiry,


Should the counterparty_spendable_height be adjusted here as well based on whether the htlc is offered or not?

Yea, sorry, fought with this for a while and think we should do it in a separate PR, it causes a bunch of test changes.

Would be good to track this feedback somewhere

lightning/src/chain/onchaintx.rs

lightning/src/chain/package.rs

arik-so · 2024-10-08T17:46:46Z

lightning/src/chain/package.rs

+		let timer_for_target_conf = |target_conf| -> u32 {
+			if target_conf <= current_height + MIDDLE_FREQUENCY_BUMP_INTERVAL {
+				current_height + HIGH_FREQUENCY_BUMP_INTERVAL
+			} else if target_conf <= current_height + LOW_FREQUENCY_BUMP_INTERVAL {


if target_conf is lower than current_height + LOW_FREQUENCY_BUMP_INTERVAL, isn't it necessarily also lower than current_height + MIDDLE_FREQUENCY_BUMP_INTERVAL? So can the second clause even ever trigger?

Ha, indeed, it cannot. Looks like this is an existing bug.

just to double-check, this is meant to be fixed in a followup, not here, right? If so, on my end the PR looks ready.

Yea, my initial patch was wrong and mislead me into thinking updating tests was easy, but it breaks 9 tests and we should land this PR...The issue dates back to 757bcc2 but also the logic here isn't just broken, its also much too naive, we should rethink it entirely.

Added to #3372

arik-so · 2024-10-09T07:36:57Z

I'm sure you already noticed, but CI's failing :/

TheBlueMatt · 2024-10-10T23:39:11Z

I had not, thanks. Fixed.

lightning/src/chain/onchaintx.rs

tankyleo · 2024-10-12T03:03:32Z

lightning/src/chain/channelmonitor.rs

@@ -3657,7 +3666,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 	// Returns (1) `PackageTemplate`s that can be given to the OnchainTxHandler, so that the handler can
 	// broadcast transactions claiming holder HTLC commitment outputs and (2) a holder revokable
 	// script so we can detect whether a holder transaction has been seen on-chain.
-	fn get_broadcasted_holder_claims(&self, holder_tx: &HolderSignedTx, conf_height: u32) -> (Vec<PackageTemplate>, Option<(ScriptBuf, PublicKey, RevocationKey)>) {
+	fn get_broadcasted_holder_claims(&self, holder_tx: &HolderSignedTx, _conf_height: u32) -> (Vec<PackageTemplate>, Option<(ScriptBuf, PublicKey, RevocationKey)>) {


nit: are you keeping conf_height around for use in this function in the future ?

Yes, later in my current patchset it is used.

lightning/src/chain/onchaintx.rs

valentinewallace · 2024-10-15T20:27:36Z

lightning/src/chain/package.rs

+	// Block height at which our counterparty can potentially claim this output as well (assuming
+	// they have the keys or information required to do so).
+	// For HTLCs we're claiming with a preimage and revoked outputs, we need to get our spend
+	// on-chain before this height.


I think it would be helpful to retain the reference to OnchainTxHandler::get_height_timer from the previous comment.

This value is now not about the fee-bumping, though? get_height_timer uses this value in a few places because its an easy way to figure out the target, but this value is really about the package merging decision based on if this output is potentially pinnable by our counterparty.

lightning/src/chain/package.rs

valentinewallace · 2024-10-15T21:08:06Z

lightning/src/chain/channelmonitor.rs

+							commitment_txid,
+							transaction_output_index,
+							PackageSolvingData::RevokedHTLCOutput(revk_htlc_outp),
+							htlc.cltv_expiry,


Would be good to track this feedback somewhere

valentinewallace · 2024-10-15T21:12:56Z

lightning/src/chain/package.rs

@@ -1038,7 +1038,7 @@ impl PackageTemplate {
 			soonest_conf_deadline,
 			aggregable,
 			feerate_previous: 0,
-			height_timer: first_bump,
+			height_timer: 0,


next_fee_bump_height seems like a more descriptive name than height_timer, though if we're setting it to 0 that's a bit inaccurate. It seems like the docs for height_timer could use a note that it can be 0 as well. It looks like the "timer" phrasing is used in a lot of places right now, though.

Yea, there's still more cleanup that should probably happen here, but this PR already kinda got away from me, and there's at least two followups pending...

valentinewallace · 2024-10-15T22:01:05Z

lightning/src/chain/package.rs

+					);
+				},
+				PackageSolvingData::HolderHTLCOutput(outp) if outp.preimage.is_some() => {
+					// We have the same deadline here as for `CounterpartyOfferedHTLCOutput`. Note


This comment is a bit confusing as we seem to calculate the deadline differently for CounterpartyOfferedHTLCOutputs, i.e. using outp.htlc.cltv_expiry vs self.soonest_conf_deadline.

The following sentence explains why - outp.cltv_expiry is actually always 0 due to an unrelated bug, so we can't rely on it :(

lightning/src/chain/package.rs

valentinewallace · 2024-10-16T14:33:55Z

lightning/src/chain/channelmonitor.rs

 					let htlc_output = HolderHTLCOutput::build_offered(
 						htlc.amount_msat, htlc.cltv_expiry, self.onchain_tx_handler.channel_type_features().clone()
 					);
-					htlc_output
+					(htlc_output, conf_height)


Missing test coverage for this change

Right, several of the "fix the value we pass" commits don't have test coverage because we don't rely on the value being the new correct one (and can't, old monitors may still be around which don't have the corrected value).

valentinewallace

I'm good with squashing this

lightning/src/chain/package.rs

We don't actually care if a confirmed transaction claimed other outputs, only that it claimed a superset of the outputs in the pending claim we're looking at. Thus, the variable to detect that is renamed `is_claim_subset_of_tx` instead of `are_sets_equal`.

This function was very confusing - its used to determine by when we have to stop aggregating this claim with others as it starts to be at risk of pinning due to the counterparty's ability to spend the output. It is not ever used as a timelock for a transaction, and thus its name is very confusing. Instead we rename it `counterparty_spendable_height`.

valentinewallace

I think basically landable on my end, I'm good with continuing to squash

lightning/src/chain/package.rs

valentinewallace · 2024-10-17T15:13:29Z

lightning/src/chain/package.rs

+					// `CounterpartyReceivedHTLCOutput`
+					height_timer = cmp::min(
+						height_timer,
+						timer_for_target_conf(outp.cltv_expiry + MIN_CLTV_EXPIRY_DELTA as u32),


Missing test coverage for the added + MIN_CLTV_EXPIRY_DELTA here

Hmm? If I drop the MIN_CLTV_EXPIRY_DELTA in the PackageSolvingData::HolderHTLCOutput (without a preimage) I see three tests fail.

Oops, I meant to leave this comment on PackageSolvingData::CounterpartyReceivedHTLCOutput.

Added test coverage.

valentinewallace · 2024-10-17T15:18:46Z

lightning/src/chain/package.rs

+					// We have the same deadline for holder timeout claims as for
+					// `CounterpartyReceivedHTLCOutput`


Is it possible that this isn't a holder timeout claim, but instead a preimage claim where we haven't received the preimage yet (but might in the future)? Wondering if it's possible that outp.cltv_expiry would be 0 here.

By the time we turn a spend into a PackageTemplate we have some ability to claim it. If the HTLC is inbound (ie its possible to claim with a preimage) we can only end up here with the preimage set.

This has never been used, and its set to a fixed value of zero for HTLCs on local commitment transactions making it impossible to rely on so might as well remove it.

Now that we don't store the confirmation height of the inputs being spent, passing the current height to `PackageTemplate::build_package` is useless - we only use it to set the height at which we should next bump the fee, but we just want it to be "next block", so we might as well use `0` and avoid the extra argument. Further, in one case we were already passing `0`, so passing the argument is just confusing as we can't rely on it being set. Note that this does remove an assertion that we never merge packages that were crated at different heights, and in the future we may wish to do that (as there's no specific reason not to), but we do not currently change the behavior.

`PackageTemplate::get_height_timer` is used to decide when to next bump our feerate on claims which need to make it on chain within some window. It does so by comparing the current height with some deadline and increasing the bump rate as the deadline approaches. However, the deadline used is the `counterparty_spendable_height`, which is the height at which the counterparty might be able to spend the same output, irrespective of why. This doesn't make sense for all output types, for example outbound HTLCs are spendable by our counteraprty immediately (by revealing the preimage), but we don't need to get our HTLC timeout claims confirmed immedaitely, as we actually have `MIN_CLTV_EXPIRY` blocks before the inbound edge of a forwarded HTLC becomes claimable by our (other) counterparty. Thus, here, we adapt `get_height_timer` to look at the type of output being claimed, and adjust the rate at which we bump the fee according to the real deadline.

This renames the field in `PackageTemplate` which describes the height at which a counterparty can make a claim to an output to match its actual use. Previously it had been set based on when a counterparty can claim an output but also used for other purposes. In the previous commit we cleaned up its use for fee-bumping-rate, so here we can rename it as it is now only used as the `counteraprty_spendable_height`.

valentinewallace · 2024-10-18T16:17:33Z

lightning/src/chain/package.rs

-					height_timer =
-						cmp::min(height_timer, current_height + HIGH_FREQUENCY_BUMP_INTERVAL);
+					height_timer = current_height + HIGH_FREQUENCY_BUMP_INTERVAL;


This fixup should've been applied to a prior commit

Actually, I'm just gonna back this change out. We're iterating a loop and trying to find the minimum, so its weird that sometimes we don't call cmp::min, even if its the case that we're setting the minimum value in that case.

For outbound HTLCs, the counterparty can spend the output immediately. This fixes the `counterparty_spendable_height` in the `PackageTemplate` claiming outbound HTLCs on local commitment transactions, which was previously spuriously set to the HTLC timeout (at which point *we* can claim the HTLC).

In a previous commit we updated the fee-bump-rate of claims against HTLC timeouts on counterparty commitment transactions so that instead of immediately attempting to bump every block we consider the fact that we actually have at least `MIN_CLTV_EXPIRY_DELTA` blocks to do so, and bumping at the appropriate rate given that. Here we test that by adding an extra check to an existing test that we do not bump in the very next block after the HTLC timeout claim was initially broadcasted.

TheBlueMatt force-pushed the 2024-09-cleanup-package branch 2 times, most recently from 1668dbe to 04c46a5 Compare September 6, 2024 19:16

wvanlint reviewed Sep 18, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-09-cleanup-package branch from fd97a6b to e46b1a4 Compare September 18, 2024 20:30

wvanlint reviewed Sep 18, 2024

View reviewed changes

wvanlint approved these changes Sep 19, 2024

View reviewed changes

arik-so reviewed Sep 24, 2024

View reviewed changes

lightning/src/chain/onchaintx.rs Outdated Show resolved Hide resolved

lightning/src/chain/onchaintx.rs Outdated Show resolved Hide resolved

lightning/src/chain/package.rs Show resolved Hide resolved

wvanlint mentioned this pull request Sep 25, 2024

Batch on-chain claims more aggressively per channel #3340

Merged

TheBlueMatt force-pushed the 2024-09-cleanup-package branch from e46b1a4 to 7284666 Compare October 3, 2024 20:48

arik-so reviewed Oct 8, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-09-cleanup-package branch from 7284666 to 544d5ce Compare October 8, 2024 20:08

TheBlueMatt force-pushed the 2024-09-cleanup-package branch from 544d5ce to 5be3cde Compare October 10, 2024 23:38

tankyleo reviewed Oct 12, 2024

View reviewed changes

lightning/src/chain/onchaintx.rs Outdated Show resolved Hide resolved

tankyleo reviewed Oct 12, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-09-cleanup-package branch from 5be3cde to fac44b8 Compare October 15, 2024 15:44

valentinewallace reviewed Oct 15, 2024

View reviewed changes

valentinewallace reviewed Oct 16, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-09-cleanup-package branch from fac44b8 to 630bc52 Compare October 16, 2024 18:22

TheBlueMatt mentioned this pull request Oct 16, 2024

Further PackageTemplate cleanups #3372

Open

4 tasks

TheBlueMatt force-pushed the 2024-09-cleanup-package branch from 630bc52 to ba0ac67 Compare October 16, 2024 18:28

valentinewallace reviewed Oct 16, 2024

View reviewed changes

lightning/src/chain/package.rs Show resolved Hide resolved

lightning/src/chain/package.rs Outdated Show resolved Hide resolved

TheBlueMatt added 2 commits October 16, 2024 21:04

TheBlueMatt force-pushed the 2024-09-cleanup-package branch from ba0ac67 to 58d0ca3 Compare October 16, 2024 21:08

valentinewallace reviewed Oct 17, 2024

View reviewed changes

Drop unused PackageTemplate::height_original

8c4794d

This has never been used, and its set to a fixed value of zero for HTLCs on local commitment transactions making it impossible to rely on so might as well remove it.

TheBlueMatt added 3 commits October 17, 2024 18:34

TheBlueMatt force-pushed the 2024-09-cleanup-package branch from 58d0ca3 to 158005c Compare October 17, 2024 18:34

arik-so previously approved these changes Oct 18, 2024

View reviewed changes

TheBlueMatt dismissed arik-so’s stale review via cb27257 October 18, 2024 15:59

valentinewallace previously approved these changes Oct 18, 2024

View reviewed changes

TheBlueMatt added 2 commits October 18, 2024 16:23

TheBlueMatt dismissed valentinewallace’s stale review via 20db790 October 18, 2024 16:23

TheBlueMatt force-pushed the 2024-09-cleanup-package branch from cb27257 to 20db790 Compare October 18, 2024 16:23

valentinewallace approved these changes Oct 18, 2024

View reviewed changes

arik-so approved these changes Oct 18, 2024

View reviewed changes

TheBlueMatt merged commit c5be7aa into lightningdevkit:main Oct 18, 2024
18 of 21 checks passed

	let htlc_output = if htlc.offered {
	let htlc_output = HolderHTLCOutput::build_offered(
	htlc.amount_msat, htlc.cltv_expiry, self.onchain_tx_handler.channel_type_features().clone()
	);
	htlc_output
	} else {
	let payment_preimage = if let Some(preimage) = self.payment_preimages.get(&htlc.payment_hash) {
	preimage.clone()
	} else {
	// We can't build an HTLC-Success transaction without the preimage
	continue;
	};
	let htlc_output = HolderHTLCOutput::build_accepted(
	payment_preimage, htlc.amount_msat, self.onchain_tx_handler.channel_type_features().clone()
	);
	htlc_output
	};
	let htlc_package = PackageTemplate::build_package(
	holder_tx.txid, transaction_output_index,
	PackageSolvingData::HolderHTLCOutput(htlc_output),
	htlc.cltv_expiry, conf_height
	);

		// We have the same deadline for holder timeout claims as for
		// `CounterpartyReceivedHTLCOutput`

Cleanup PackageTemplatea bit #3297

Cleanup PackageTemplatea bit #3297

Conversation

TheBlueMatt commented Sep 6, 2024

codecov bot commented Sep 6, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

TheBlueMatt Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arik-so commented Oct 9, 2024

TheBlueMatt commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cleanup `PackageTemplate`a bit #3297

Cleanup `PackageTemplate`a bit #3297

codecov bot commented Sep 6, 2024 •

edited

Loading

TheBlueMatt Sep 18, 2024 •

edited

Loading

TheBlueMatt Oct 18, 2024 •

edited

Loading