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.

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

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.

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 91.22%. Comparing base (d35239c) to head (e46b1a4).
Report is 104 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.85%   91.22%   +1.36%     
==========================================
  Files         126      126              
  Lines      104145   114759   +10614     
  Branches   104145   114759   +10614     
==========================================
+ Hits        93577   104685   +11108     
+ Misses       7894     7480     -414     
+ Partials     2674     2594      -80

☔ 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

);

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

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

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?

TheBlueMatt added 3 commits September 5, 2024 23:53

Drop unused PackageTemplate::height_original

3ea6449

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 force-pushed the 2024-09-cleanup-package branch from 1ef27be to 3ea6449 Compare September 6, 2024 17:58

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

wvanlint reviewed Sep 18, 2024

View reviewed changes

TheBlueMatt added 6 commits September 18, 2024 19:17

f re-add height param to get_broadcasted_holder_claims

e3142e5

f add missing CLTV_EXPIRY_DELTA offset

78fa3d4

f correct comment

9c0bac6

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup `PackageTemplate`a bit #3297

Cleanup `PackageTemplate`a bit #3297

TheBlueMatt commented Sep 6, 2024

codecov bot commented Sep 6, 2024 •

edited

Loading

wvanlint Sep 18, 2024

TheBlueMatt Sep 18, 2024 •

edited

Loading

TheBlueMatt Sep 18, 2024

wvanlint Sep 18, 2024

TheBlueMatt Sep 18, 2024

TheBlueMatt Sep 18, 2024

wvanlint Sep 18, 2024

wvanlint Sep 18, 2024

TheBlueMatt Sep 19, 2024

wvanlint Sep 18, 2024

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

Cleanup PackageTemplatea bit #3297

Are you sure you want to change the base?

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

Cleanup `PackageTemplate`a bit #3297

Cleanup `PackageTemplate`a bit #3297

codecov bot commented Sep 6, 2024 •

edited

Loading

TheBlueMatt Sep 18, 2024 •

edited

Loading