[Custom Transactions] Provide Built Counterparty Commitment Transactions To `ChannelMonitor` #3654

tankyleo · 2025-03-07T21:25:36Z

Provide built counterparty commitment transactions to `ChannelMonitor`

`LatestCounterpartyCommitmentTXInfo` provides `ChannelMonitor` with the
data it needs to build counterparty commitment transactions.
`ChannelMonitor` then rebuilds commitment transactions from these pieces
of data when requested.

This commit adds a new variant to `ChannelMonitorUpdateStep` called
`LatestCounterpartyCommitmentTX`, which will provide fully built
commitment transactions to `ChannelMonitor`. When `ChannelMonitor` is
asked for counterparty commitment transactions, it will not rebuild
them, but just clone them.

As a result, this new variant eliminates any calls to the upcoming
`TxBuilder` trait in `ChannelMonitor`, which avoids adding a generic
parameter on `ChannelMonitor`. This commit also stomps any bugs that
might come from passing around disparate pieces of data to re-build
commitment transactions from scratch.

We also add a `htlc_outputs` field to the variant to include the dust
HTLCs, as well as all the HTLC sources. This means that this new variant
will store non-dust HTLCs both in the `htlc_outputs` field, and in the
`commitment_tx` field. This is ok for now as the number of HTLCs
per commitment nowadays is very low.

We add code to handle this new variant, but refrain from
immediately setting it. This will allow us to atomically switch from
`LatestCounterpartyCommitmentTXInfo` to `LatestCounterpartyCommitmentTX`
in the future while still allowing downgrades.

This was previously part of #3606

ldk-reviews-bot · 2025-03-07T21:25:38Z

👋 I see @valentinewallace was un-assigned.
If you'd like another reviewer assignemnt, please click here.

codecov · 2025-03-08T20:07:33Z

Codecov Report

Attention: Patch coverage is 82.22222% with 8 lines in your changes missing coverage. Please review.

Project coverage is 89.23%. Comparing base (4c43a5b) to head (29a8b9f).

Files with missing lines	Patch %	Lines
lightning/src/chain/channelmonitor.rs	81.57%	7 Missing ⚠️
lightning/src/ln/channel.rs	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3654      +/-   ##
==========================================
+ Coverage   89.18%   89.23%   +0.04%     
==========================================
  Files         155      155              
  Lines      119274   119280       +6     
  Branches   119274   119280       +6     
==========================================
+ Hits       106379   106440      +61     
+ Misses      10290    10241      -49     
+ Partials     2605     2599       -6

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

ldk-reviews-bot · 2025-03-10T17:04:29Z

🔔 1st Reminder

Hey @valentinewallace! 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.

wpaulino

LGTM

wpaulino · 2025-03-10T18:16:34Z

lightning/src/chain/channelmonitor.rs

+				let htlc_outputs = vec![];
+
+				let commitment_tx = self.build_counterparty_commitment_tx(INITIAL_COMMITMENT_NUMBER,
+					&their_per_commitment_point, to_broadcaster_value, to_countersignatory_value,
+					feerate_per_kw, htlc_outputs);
+				// Take the opportunity to populate this recently introduced field
+				self.initial_counterparty_commitment_tx = Some(commitment_tx.clone());
+				commitment_tx


Nit: indentation seems off here

Thank you I addressed this below, ran the block through fmt.

wpaulino · 2025-03-10T18:17:27Z

lightning/src/chain/channelmonitor.rs

+				&ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTX { htlc_outputs: _,
+					ref commitment_tx,


Suggested change

&ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTX { htlc_outputs: _,

ref commitment_tx,

&ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTX {

htlc_outputs: _, ref commitment_tx,

Thank you, addressed below.

jkczyz · 2025-03-10T18:35:54Z

lightning/src/chain/channelmonitor.rs

-		to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, logger: &L,
-	)
-	where L::Target: Logger
+		&self, commitment_tx: CommitmentTransaction, logger: &L,


Let's update the docs accordingly.

Thanks ! Addressed below.

jkczyz · 2025-03-10T18:49:14Z

lightning/src/chain/channelmonitor.rs

+		self.initial_counterparty_commitment_tx.clone().or_else(|| {
+			// This provides forward compatibility; an old monitor will not contain the full
+			// transaction; only enough information to rebuild it
+			self.initial_counterparty_commitment_info.map(


Could we ever return None now? It seems before we couldn't.

Before we could return None at this call self.initial_counterparty_commitment_info?;

Ah, for reason I was unaware that ? worked with Option return types.

`LatestCounterpartyCommitmentTXInfo` provides `ChannelMonitor` with the data it needs to build counterparty commitment transactions. `ChannelMonitor` then rebuilds commitment transactions from these pieces of data when requested. This commit adds a new variant to `ChannelMonitorUpdateStep` called `LatestCounterpartyCommitmentTX`, which will provide fully built commitment transactions to `ChannelMonitor`. When `ChannelMonitor` is asked for counterparty commitment transactions, it will not rebuild them, but just clone them. As a result, this new variant eliminates any calls to the upcoming `TxBuilder` trait in `ChannelMonitor`, which avoids adding a generic parameter on `ChannelMonitor`. This commit also stomps any bugs that might come from passing around disparate pieces of data to re-build commitment transactions from scratch. We also add a `htlc_outputs` field to the variant to include the dust HTLCs, as well as all the HTLC sources. This means that this new variant will store non-dust HTLCs both in the `htlc_outputs` field, and in the `commitment_tx` field. This is ok for now as the number of HTLCs per commitment nowadays is very low. We add code to handle this new variant, but refrain from immediately setting it. This will allow us to atomically switch from `LatestCounterpartyCommitmentTXInfo` to `LatestCounterpartyCommitmentTX` in the future while still allowing downgrades.

TheBlueMatt · 2025-03-11T20:03:36Z

lightning/src/chain/channelmonitor.rs

 				ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { commitment_txid, htlc_outputs, commitment_number, their_per_commitment_point, .. } => {
 					log_trace!(logger, "Updating ChannelMonitor with latest counterparty commitment transaction info");
 					self.provide_latest_counterparty_commitment_tx(*commitment_txid, htlc_outputs.clone(), *commitment_number, *their_per_commitment_point, logger)
 				},
+				ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTX { htlc_outputs, commitment_tx } => {
+					log_trace!(logger, "Updating ChannelMonitor with latest counterparty commitment transaction info");
+					self.provide_latest_counterparty_commitment_tx(commitment_tx.trust().txid(), htlc_outputs.clone(), commitment_tx.commitment_number(), commitment_tx.per_commitment_point(), logger)


Should we handle having both the legacy and new version? I guess i was hoping we wouldn't have to wait a release or two for this stuff (which we have to do if we're trying to bridge versions here).

We could -- I think it was done that way first in #3606 but I was concerned about the extra storage cost. We're planning to use this new variant (along with a similar one for latest holder commitment) for spliced channels as a way to move to the new format sooner since you can't downgrade with a pending splice anyway.

I think it was done that way first in #3606 but I was concerned about the extra storage cost.

Right yes we previously added another field to the legacy variant that contained the full transaction. This made most of the fields in the legacy variant redundant so we went for a new variant.

Then to alleviate the storage cost we decided against immediately setting this new variant.

Yea, okay, that all makes sense, just annoying that we have to wait a release or two to ship things. Alas.

tankyleo changed the title ~~Provide built commitment transactions to ChannelMonitor~~ Provide built counterparty commitment transactions to ChannelMonitor Mar 7, 2025

ldk-reviews-bot requested a review from valentinewallace March 7, 2025 21:35

tankyleo force-pushed the channel-monitor-updates branch from ad98735 to 9f88056 Compare March 8, 2025 19:38

tankyleo changed the title ~~Provide built counterparty commitment transactions to ChannelMonitor~~ Provide Built Counterparty Commitment Transactions To ChannelMonitor Mar 8, 2025

tankyleo force-pushed the channel-monitor-updates branch 2 times, most recently from 6e70599 to 52a2eb8 Compare March 8, 2025 19:58

tankyleo force-pushed the channel-monitor-updates branch from 52a2eb8 to 29a8b9f Compare March 8, 2025 20:12

tankyleo changed the title ~~Provide Built Counterparty Commitment Transactions To ChannelMonitor~~ [Custom Transactions] Provide Built Counterparty Commitment Transactions To ChannelMonitor Mar 10, 2025

tankyleo mentioned this pull request Mar 10, 2025

Custom Transactions Tracking Issue #3603

Open

21 tasks

wpaulino reviewed Mar 10, 2025

View reviewed changes

wpaulino requested review from jkczyz and removed request for valentinewallace March 10, 2025 18:20

tankyleo force-pushed the channel-monitor-updates branch from 29a8b9f to e1b5fe0 Compare March 10, 2025 18:40

jkczyz reviewed Mar 10, 2025

View reviewed changes

tankyleo force-pushed the channel-monitor-updates branch from e1b5fe0 to b8a03cd Compare March 10, 2025 19:16

jkczyz approved these changes Mar 10, 2025

View reviewed changes

wpaulino approved these changes Mar 10, 2025

View reviewed changes

wpaulino merged commit 00ee0ef into lightningdevkit:main Mar 11, 2025
23 of 27 checks passed

TheBlueMatt reviewed Mar 11, 2025

View reviewed changes

		&ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTX { htlc_outputs: _,
		ref commitment_tx,

[Custom Transactions] Provide Built Counterparty Commitment Transactions To ChannelMonitor #3654

[Custom Transactions] Provide Built Counterparty Commitment Transactions To ChannelMonitor #3654

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

ldk-reviews-bot commented Mar 10, 2025

Uh oh!

wpaulino 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[Custom Transactions] Provide Built Counterparty Commitment Transactions To `ChannelMonitor` #3654

[Custom Transactions] Provide Built Counterparty Commitment Transactions To `ChannelMonitor` #3654

tankyleo commented Mar 7, 2025 •

edited

Loading

ldk-reviews-bot commented Mar 7, 2025 •

edited

Loading

codecov bot commented Mar 8, 2025 •

edited

Loading

tankyleo Mar 11, 2025 •

edited

Loading