Replace use of HolderSignedTx with HolderCommitment #3664

wpaulino · 2025-03-12T17:41:09Z

As we move to a custom commitment transactions world, we'll no longer be able to rely on HolderSignedTx and should instead track the actual commitment transaction itself. This commit does so by introducing a new HolderCommitment struct, which we'll use as a replacement to HolderSignedTx to hold all relevant holder commitment data, including the HolderCommitmentTransaction, without data duplication.

Luckily, this is a backwards and forwards compatible change due to the OnchainTxHandler tracking the latest HolderCommitmentTransactions. In the future, we aim to remove them from the OnchainTxHandler entirely and begin storing them at the ChannelMonitor level.

Aside from how the data is represented internally, there shouldn't be any functional changes within this patch.

Depends on #3663.

ldk-reviews-bot · 2025-03-12T17:41:12Z

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

TheBlueMatt · 2025-03-12T18:19:40Z

lightning/src/ln/channel.rs

@@ -3531,23 +3531,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 			return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_stats.num_nondust_htlcs)));
 		}

-		// Up to LDK 0.0.115, HTLC information was required to be duplicated in the


Hmm, did we fuck this up? Is there no way to indicate to old LDK to decline to decode the update now that its no longer actually compatible? At a minimum we need release notes but kinda annoying that we'll just silently corrupt state if a user downgrades to 0.0.115.

Yeah... I think we would've needed to add an optional even TLV back then that only gets set when the sources are provided separately. Release notes will have to do for now.

TheBlueMatt · 2025-03-12T18:27:00Z

lightning/src/ln/channel.rs

+					if let Some(source) = source_opt.take() {
+						nondust_htlc_sources.push(source);
+					} else {
+						debug_assert!(false, "Missing outbound HTLC source");


This should probably be a hard assert, right? And also probably we should change the types to avoid it?

Change the types on what? The source still needs to be optional in case the HTLC is not offered by the holder.

TheBlueMatt · 2025-03-12T18:31:47Z

lightning/src/chain/channelmonitor.rs

 					PackageSolvingData::HolderHTLCOutput(htlc_output),
 					counterparty_spendable_height,
 				);
 				claim_requests.push(htlc_package);
+			} else {
+				debug_assert!(false, "Expected transaction output index for non-dust HTLC");


Ugh, we really need to change the types on htlc info so that we don't have these checks everywhere...if we're gonna separate tracking of dust from non-dust we should use different types...

Yeah, unfortunately there are still areas where we track them both together in a single Vec, so I don't think we can get there quite yet as of this PR.

lightning/src/ln/chan_utils.rs

TheBlueMatt · 2025-03-12T18:40:01Z

lightning/src/chain/channelmonitor.rs

 			writer.write_all(&[1; 1])?;
-			prev_holder_tx.write(writer)?;
+			HolderSignedTx::from(prev_holder_commitment).write(writer)?;


Bleh, can we just implement a write_as_legacy?

Just to avoid the from magic? Because we still need that further below to make sure the HolderCommitmentTransaction we got from the OnchainTxHandler maps to the HolderSignedTx we read.

In all of those cases there's no reason we need to be cloning the HTLC vecs and sources. I'd really prefer to avoid that. Plus we also panic in the conversion while trying to check if the data we read was valid, which is generally something we try to avoid.

TheBlueMatt · 2025-03-12T18:43:39Z

lightning/src/chain/channelmonitor.rs

+			let holder_commitment = HolderCommitment::try_from((
+				current_holder_commitment_tx, &current_holder_signed_tx,
+			)).map_err(|_| DecodeError::InvalidValue)?;
+			if HolderSignedTx::from(&holder_commitment) != current_holder_signed_tx {


What's gonna happen here? Are we just gonna keep writing the legacy versions and converting, or should we have some upgrade path?

Was planning to address this in a follow-up PR that removes them from OnchainTxHandler such that we can no longer rely on getting them from there and instead store them as a new TLV in the monitor.

ldk-reviews-bot · 2025-03-12T18:43:44Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

TheBlueMatt · 2025-03-12T19:26:53Z

This does seem like the kind of thing that might want an 👋 upgrade test 👋

codecov · 2025-03-12T19:57:56Z

Codecov Report

Attention: Patch coverage is 90.09901% with 30 lines in your changes missing coverage. Please review.

Project coverage is 89.20%. Comparing base (030a784) to head (a891a04).

Files with missing lines	Patch %	Lines
lightning/src/chain/channelmonitor.rs	90.00%	18 Missing and 11 partials ⚠️
lightning/src/ln/channel.rs	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3664      +/-   ##
==========================================
- Coverage   89.25%   89.20%   -0.06%     
==========================================
  Files         155      155              
  Lines      119959   120054      +95     
  Branches   119959   120054      +95     
==========================================
+ Hits       107069   107090      +21     
- Misses      10276    10332      +56     
- Partials     2614     2632      +18

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

wpaulino · 2025-03-12T22:55:53Z

This does seem like the kind of thing that might want an 👋 upgrade test 👋

Will need to wait for #3584 first

dunxen · 2025-03-17T12:16:45Z

Needs rebase.

tankyleo

Didn't make it through everything, some initial comments.

I'm also unclear how this helps with custom commitment transactions in particular ? Thanks for clarifying here.

lightning/src/ln/channel.rs

lightning/src/chain/channelmonitor.rs

jkczyz

Made it through everything. Nothing out of the ordinary pops out aside from what other reviewers have raised.

I'm also unclear how this helps with custom commitment transactions in particular ? Thanks for clarifying here.

Will let @wpaulino confirm, but IIUC prior to this PR we were only storing parts of the commitment transaction, so anything additional in a custom one would be missing.

lightning/src/chain/channelmonitor.rs

lightning/src/ln/channelmanager.rs

wpaulino · 2025-03-17T23:56:49Z

IIUC prior to this PR we were only storing parts of the commitment transaction, so anything additional in a custom one would be missing.

Correct. We were already tracking the actual commitment transaction at the OnchainTxHandler, so HolderSignedTx was redundant. The plan is to move the commitment transaction storage from OnchainTxHandler -> ChannelMonitor.

tankyleo · 2025-03-18T02:32:42Z

IIUC prior to this PR we were only storing parts of the commitment transaction, so anything additional in a custom one would be missing.

Correct. We were already tracking the actual commitment transaction at the OnchainTxHandler, so HolderSignedTx was redundant. The plan is to move the commitment transaction storage from OnchainTxHandler -> ChannelMonitor.

Ok! From my side here's how I've been approaching custom transactions in channel monitor. Many details I haven't worked out yet but big picture:

While storing the fully built holder commitment transactions is ok as we only store two, we will likely not store all the previous counterparty commitment transactions (even custom ones), as the storage cost would be too high.
For this problem, me and Matt have been going for a roundtrip to some signer-like object to generate the claims.
ie "hey signer ! here's a commitment tx that confirmed, what can we claim out of this?"
this could work for all the counterparty transactions (revoked and non-revoked), and I've been making it work for holder transactions too.

Experimental API currently looks like this:

	/// Returns claims on a counterparty commitment transaction
	fn generate_claims_from_counterparty_tx(&self,
		per_commitment_point: &PublicKey,
		channel_parameters: &ChannelTransactionParameters,
		tx: &Transaction,
		per_commitment_claimable_data: &Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>,
		payment_preimages: &HashMap<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)>,
		secp_ctx: &Secp256k1<secp256k1::All>,
	) -> (Vec<PackageTemplate>, CommitmentTxCounterpartyOutputInfo);

	/// Returns claims on a counterparty's revoked commitment transaction
	fn generate_claims_from_revoked_tx(
		&self,
		per_commitment_key: &SecretKey,
		channel_parameters: &ChannelTransactionParameters,
		tx: &Transaction,
		per_commitment_claimable_data: &Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>,
		height: u32,
		secp_ctx: &Secp256k1<secp256k1::All>,
	) -> (Vec<PackageTemplate>, CommitmentTxCounterpartyOutputInfo);

	/// Returns claims on a holder's commitment transaction
	fn generate_claims_from_holder_tx(
		&self,
		holder_tx: &HolderSignedTx,
		conf_height: u32,
		channel_parameters: &ChannelTransactionParameters,
		payment_preimages: &HashMap<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)>,
		secp_ctx: &Secp256k1<secp256k1::All>,
	) -> Vec<PackageTemplate>;

What's missing:

When we force close, we want to generate claims even though our holder commit tx hasn't confirmed yet.
When we learn a preimage, we want to generate claims against already confirmed outputs.
Generate claims on htlc transactions.

Let me know if this syncs up with what you were thinking @wpaulino happy to chat here or elsewhere.

This branch here contains this API together with an implementation that passes the test suite:

https://github.com/tankyleo/rust-lightning/tree/check

jkczyz

LGTM aside from pending discussions. Please re-request a review once those are resolved.

wpaulino · 2025-03-18T17:04:31Z

@tankyleo your proposal seems fine so far. The only change as a result of this PR would be to the function argument in generate_claims_from_holder_tx, we would've probably wanted to provide the HolderCommitmentTransaction there to begin with anyway. The rest is mostly unrelated to this change, so happy to chat about this work offline.

tankyleo · 2025-03-18T17:07:54Z

The only change as a result of this PR would be to the function argument in generate_claims_from_holder_tx, we would've probably wanted to provide the HolderCommitmentTransaction there to begin with anyway.

Yes agreed

The rest is mostly unrelated to this change, so happy to chat about this work offline.

Sounds good

TheBlueMatt · 2025-03-25T12:42:56Z

lightning/src/ln/channel.rs

@@ -3593,7 +3577,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {

 		Ok(LatestHolderCommitmentTXInfo {
 			commitment_tx: holder_commitment_tx,
-			htlc_outputs: htlcs_and_sigs,
+			htlc_outputs: dust_htlcs,


Can we rename this now that its something different?

It might still have the old meaning when we read. Would you rather change it now anyway or wait until we know for sure we'll only ever find dust_htlcs?

Maybe mostly_dust_htlcs? :). I mean we can certainly do it later, just seems easy for a bug to slip in because of a bad name.

I'd rather defer it. I don't think a bug could happen where non-dust HTLCs are included because we still have code to support that, and the current name already assumed dust HTLCs must be included.

TheBlueMatt · 2025-03-25T13:04:48Z

lightning/src/chain/channelmonitor.rs

 			writer.write_all(&[1; 1])?;
-			prev_holder_tx.write(writer)?;
+			HolderSignedTx::from(prev_holder_commitment).write(writer)?;


In all of those cases there's no reason we need to be cloning the HTLC vecs and sources. I'd really prefer to avoid that. Plus we also panic in the conversion while trying to check if the data we read was valid, which is generally something we try to avoid.

Currently, non-dust HTLCs are duplicated across the commitment transaction itself, and the full set of HTLCs (dust & non-dust) along with their `HTLCSource` considered in the commitment transaction. As of v0.0.15, we've had support for providing non-dust HTLC sources separately such that we no longer track duplicate non-dust HTLC data, but only enabled it under testing environments. This commit enables it such that it always happens. Note that we still need to support reading `ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo` updates that did not separate the non-dust HTLC sources in case they were written in an older version and they've yet to be processed.

wpaulino · 2025-03-25T17:04:50Z

Had to rebase due to a conflict with #3606 and implemented write_as_legacy.

lightning/src/chain/channelmonitor.rs

As we move to a custom commitment transactions world, we'll no longer be able to rely on `HolderSignedTx` and should instead track the actual commitment transaction itself. This commit does so by introducing a new `HolderCommitment` struct, which we'll use as a replacement to `HolderSignedTx` to hold all relevant holder commitment data, including the `HolderCommitmentTransaction`, without data duplication. Luckily, this is a backwards and forwards compatible change due to the `OnchainTxHandler` tracking the latest `HolderCommitmentTransaction`s. In the future, we aim to remove them from the `OnchainTxHandler` entirely and begin storing them at the `ChannelMonitor` level. Aside from how the data is represented internally, there shouldn't be any functional changes within this patch.

wpaulino added the weekly goal Someone wants to land this this week label Mar 12, 2025

wpaulino requested a review from TheBlueMatt March 12, 2025 17:41

TheBlueMatt reviewed Mar 12, 2025

View reviewed changes

wpaulino force-pushed the deprecate-holder-signed-tx branch from 2884bad to fdc1706 Compare March 12, 2025 23:04

wpaulino force-pushed the deprecate-holder-signed-tx branch from fdc1706 to 73f82fb Compare March 17, 2025 16:53

wpaulino requested review from jkczyz and TheBlueMatt March 17, 2025 17:03

tankyleo reviewed Mar 17, 2025

View reviewed changes

jkczyz reviewed Mar 17, 2025

View reviewed changes

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

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

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the deprecate-holder-signed-tx branch from 73f82fb to 24dffdb Compare March 17, 2025 23:55

jkczyz reviewed Mar 18, 2025

View reviewed changes

wpaulino requested a review from jkczyz March 18, 2025 17:01

jkczyz previously approved these changes Mar 18, 2025

View reviewed changes

TheBlueMatt removed their request for review March 18, 2025 19:13

wpaulino requested a review from TheBlueMatt March 24, 2025 22:37

TheBlueMatt reviewed Mar 25, 2025

View reviewed changes

wpaulino dismissed jkczyz’s stale review via 41d9463 March 25, 2025 17:03

wpaulino force-pushed the deprecate-holder-signed-tx branch from 24dffdb to 41d9463 Compare March 25, 2025 17:03

wpaulino force-pushed the deprecate-holder-signed-tx branch from 41d9463 to 7f626c8 Compare March 25, 2025 17:03

wpaulino requested a review from TheBlueMatt March 25, 2025 17:05

TheBlueMatt reviewed Mar 25, 2025

View reviewed changes

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

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

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

wpaulino added 2 commits March 25, 2025 15:30

Remove dead code after deprecating HolderSignedTx

a891a04

wpaulino force-pushed the deprecate-holder-signed-tx branch from 7f626c8 to a891a04 Compare March 25, 2025 22:31

wpaulino requested a review from TheBlueMatt March 25, 2025 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace use of HolderSignedTx with HolderCommitment #3664

Replace use of HolderSignedTx with HolderCommitment #3664

wpaulino commented Mar 12, 2025

ldk-reviews-bot commented Mar 12, 2025 •

edited

Loading

TheBlueMatt Mar 12, 2025

wpaulino Mar 12, 2025

TheBlueMatt Mar 12, 2025

wpaulino Mar 12, 2025

TheBlueMatt Mar 12, 2025

wpaulino Mar 12, 2025

TheBlueMatt Mar 12, 2025

wpaulino Mar 12, 2025

TheBlueMatt Mar 25, 2025

TheBlueMatt Mar 12, 2025

wpaulino Mar 12, 2025

ldk-reviews-bot commented Mar 12, 2025

TheBlueMatt commented Mar 12, 2025

codecov bot commented Mar 12, 2025 •

edited

Loading

wpaulino commented Mar 12, 2025

dunxen commented Mar 17, 2025

tankyleo left a comment

jkczyz left a comment

wpaulino commented Mar 17, 2025

tankyleo commented Mar 18, 2025 •

edited

Loading

jkczyz left a comment

wpaulino commented Mar 18, 2025

tankyleo commented Mar 18, 2025

TheBlueMatt Mar 25, 2025

wpaulino Mar 25, 2025

TheBlueMatt Mar 25, 2025

wpaulino Mar 25, 2025

TheBlueMatt Mar 25, 2025

wpaulino commented Mar 25, 2025

Replace use of HolderSignedTx with HolderCommitment #3664

Are you sure you want to change the base?

Replace use of HolderSignedTx with HolderCommitment #3664

Conversation

wpaulino commented Mar 12, 2025

ldk-reviews-bot commented Mar 12, 2025 • 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

ldk-reviews-bot commented Mar 12, 2025

TheBlueMatt commented Mar 12, 2025

codecov bot commented Mar 12, 2025 • edited Loading

Codecov Report

wpaulino commented Mar 12, 2025

dunxen commented Mar 17, 2025

tankyleo left a comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

wpaulino commented Mar 17, 2025

tankyleo commented Mar 18, 2025 • edited Loading

jkczyz left a comment

Choose a reason for hiding this comment

wpaulino commented Mar 18, 2025

tankyleo commented Mar 18, 2025

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

wpaulino commented Mar 25, 2025

ldk-reviews-bot commented Mar 12, 2025 •

edited

Loading

codecov bot commented Mar 12, 2025 •

edited

Loading

tankyleo commented Mar 18, 2025 •

edited

Loading