Create a single P2A anchor on commitment transactions in 0FC channels by tankyleo · Pull Request #4053 · lightningdevkit/rust-lightning

tankyleo · 2025-09-04T07:24:58Z

Create a single P2A anchor on commitment transactions in 0FC channels

Zero-fee commitment channels replace today's existing `to_local_anchor`
and `to_remote_anchor` outputs with a single `shared_anchor` output.

Use `SINGLE|ACP` sighash on signatures from peer in 0FC channels

Part of #3789

ldk-reviews-bot · 2025-09-04T07:25:01Z

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

tankyleo · 2025-09-04T07:32:30Z

Still owe some tests, but here's the API I currently have in mind.

codecov · 2025-09-04T07:34:39Z

Codecov Report

❌ Patch coverage is 97.10145% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.76%. Comparing base (9ddd25b) to head (538c4cc).
⚠️ Report is 58 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/chain/channelmonitor.rs	60.00%	2 Missing ⚠️
lightning/src/ln/channel.rs	97.40%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #4053    +/-   ##
========================================
  Coverage   88.76%   88.76%            
========================================
  Files         176      176            
  Lines      129357   129611   +254     
  Branches   129357   129611   +254     
========================================
+ Hits       114823   115051   +228     
- Misses      11929    11953    +24     
- Partials     2605     2607     +2

Flag	Coverage Δ
fuzzing	`22.09% <78.83%> (+0.09%)`	⬆️
tests	`88.60% <97.10%> (+<0.01%)`	⬆️

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-04T14:35:13Z

lightning/src/chain/channelmonitor.rs

-		// We populate this field for downgrades
-		self.initial_counterparty_commitment_info = Some((commitment_tx.per_commitment_point(),
-			commitment_tx.feerate_per_kw(), commitment_tx.to_broadcaster_value_sat(), commitment_tx.to_countersignatory_value_sat()));
+		// Only populate `initial_counterparty_commitment_info` in non-0FC channels


Can you update the comment on initial_counterparty_commitment_info now that its not always set?

TheBlueMatt · 2025-09-04T14:48:50Z

lightning/src/ln/chan_utils.rs

 			(8, self.keys, required),
 			(10, self.built, required),
 			(12, self.nondust_htlcs, required_vec),
+			(13, self.trimmed_sum_sat, option),


Hmm? The commit messages is wrong, this should be even to make sure old clients fail to deserialize.

Discussed offline: old clients will fail to deserialize when they read the unknown 0FC feature bit in the channel_type_features member.

TheBlueMatt · 2025-09-04T14:49:42Z

lightning/src/ln/chan_utils.rs

 	// output index order.
 	nondust_htlcs: Vec<HTLCOutputInCommitment>,
+	// The sum of the values of all outputs that were trimmed to fees
+	trimmed_sum_sat: Option<Amount>, // Added in 0.2


ISTM this doesn't have to be an Option - just default to 0 if it isn't there on load and skip writing if its zero (which we need to do anyway as the current constructor will always set Some and so we'll currently always write).

TheBlueMatt · 2025-09-04T14:51:02Z

lightning/src/ln/channel.rs


-		let update = if self.pending_funding.is_empty() {
+		let update = if self.pending_funding.is_empty()
+			&& !self.funding.get_channel_type().supports_anchor_zero_fee_commitments()


I mean, that's fine, but why bother? Is it just to push migration faster by using the new type for cases where we know no downgrade is possible anyway?

Mentioned offline: LatestCounterpartyCommitmentTXInfo does not include enough information to build a 0FC commitment transaction from it, as is done in ChannelMonitor.counterparty_commitment_txs_from_update. We have to use the LatestCounterpartyCommitment variant.

TheBlueMatt · 2025-09-04T15:25:01Z

lightning/src/ln/chan_utils.rs

@@ -1538,6 +1544,8 @@ pub struct CommitmentTransaction {
 	// The set of non-dust HTLCs included in the commitment. They must be sorted in increasing
 	// output index order.
 	nondust_htlcs: Vec<HTLCOutputInCommitment>,


While we're here can we rename the feerate_per_kw field/accessor method in CommitmentTransaction to negotiated_feerate_per_kw or so? Its confusing that it does not actually refer to the feerate that the commitment transaction has (even if that already was true due to dust rounding).

ldk-reviews-bot · 2025-09-04T15:25:10Z

👋 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

LGTM, feel free to squash.

tankyleo · 2025-09-05T17:15:38Z

Squashed, some small edits to the commit messages, no code changes.

Zero-fee commitment channels replace today's existing `to_local_anchor` and `to_remote_anchor` outputs with a single `shared_anchor` output. Co-authored-by: Matt Corallo <[email protected]>

Use `negotiated_feerate_per_kw()` to underscore that this is the feerate we negotiated with our peer, not the actual feerate of the commitment transaction. The feerate of the actual commitment transaction may be higher.

Co-authored-by: Matt Corallo <[email protected]>

tankyleo · 2025-09-07T21:25:47Z

@TheBlueMatt sorry for the rugpull here I looked at the dev branch Carla shared, and turns out ChannelTransactionParameters now has channel_value_satoshis, so we can determine the value of the anchor from the existing data alone :) Should have checked earlier for sure, simplifies things quite a bit.

ldk-reviews-bot · 2025-09-08T00:00:31Z

🔔 1st Reminder

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

ldk-reviews-bot · 2025-09-08T00:00:33Z

🔔 1st Reminder

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

Co-authored-by: Matt Corallo <[email protected]>

graphite-app · 2025-09-08T00:40:52Z

lightning/src/ln/chan_utils.rs

+				// These subtractions panic on underflow, but this should never happen
+				let trimmed_sum_sat = channel_value_satoshis - nondust_htlcs_value_sum_sat - to_broadcaster_value_sat - to_countersignatory_value_sat;


The subtraction operation here could potentially panic if the sum of the values being subtracted exceeds channel_value_satoshis. While the comment acknowledges this shouldn't happen, it would be safer to use checked arithmetic or validate the constraint before performing the subtraction to prevent a potential DoS vulnerability in edge cases.

Consider using checked_sub or similar methods to handle this gracefully:

let trimmed_sum_sat = channel_value_satoshis .checked_sub(nondust_htlcs_value_sum_sat) .and_then(|result| result.checked_sub(to_broadcaster_value_sat)) .and_then(|result| result.checked_sub(to_countersignatory_value_sat)) .unwrap_or(Amount::ZERO); // Or handle the error case appropriately

This would make the code more robust against unexpected inputs while maintaining the same functionality.

Suggested change

// These subtractions panic on underflow, but this should never happen

let trimmed_sum_sat = channel_value_satoshis - nondust_htlcs_value_sum_sat - to_broadcaster_value_sat - to_countersignatory_value_sat;

// Use checked subtraction to avoid potential panics

let trimmed_sum_sat = channel_value_satoshis

.checked_sub(nondust_htlcs_value_sum_sat)

.and_then(|result| result.checked_sub(to_broadcaster_value_sat))

.and_then(|result| result.checked_sub(to_countersignatory_value_sat))

.unwrap_or_else(|| {

// This should never happen, but we handle it gracefully instead of panicking

debug_assert!(false, "Channel value calculation underflowed");

0

});

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

TheBlueMatt

This LGTM. WDYT @wpaulino

tankyleo requested a review from TheBlueMatt September 4, 2025 07:27

tankyleo added this to Weekly Goals Sep 4, 2025

tankyleo self-assigned this Sep 4, 2025

tankyleo moved this to Goal: Merge in Weekly Goals Sep 4, 2025

TheBlueMatt reviewed Sep 4, 2025

View reviewed changes

tankyleo requested a review from TheBlueMatt September 4, 2025 22:44

tankyleo force-pushed the 2025-09-p2a-anchor branch from f4f603a to d14ea23 Compare September 5, 2025 00:37

TheBlueMatt reviewed Sep 5, 2025

View reviewed changes

tankyleo force-pushed the 2025-09-p2a-anchor branch from d14ea23 to 889ca0b Compare September 5, 2025 17:13

tankyleo requested review from TheBlueMatt and wpaulino September 5, 2025 17:15

tankyleo and others added 4 commits September 6, 2025 00:40

Create a single P2A anchor on commitment transactions in 0FC channels

9008409

Zero-fee commitment channels replace today's existing `to_local_anchor` and `to_remote_anchor` outputs with a single `shared_anchor` output. Co-authored-by: Matt Corallo <[email protected]>

Rename CommitmentTransaction.feerate_per_kw()

e921bf6

Use `negotiated_feerate_per_kw()` to underscore that this is the feerate we negotiated with our peer, not the actual feerate of the commitment transaction. The feerate of the actual commitment transaction may be higher.

Run fmt on ChannelContext::validate_commitment_signed

c944b24

Use SINGLE|ACP sighash on signatures from peer in 0FC channels

1cef4c3

Co-authored-by: Matt Corallo <[email protected]>

tankyleo force-pushed the 2025-09-p2a-anchor branch from 889ca0b to 1cef4c3 Compare September 6, 2025 01:08

Set version 3 on transactions for HTLC signatures in 0FC channels

538c4cc

Co-authored-by: Matt Corallo <[email protected]>

tankyleo force-pushed the 2025-09-p2a-anchor branch from 0a0306e to 538c4cc Compare September 8, 2025 00:28

graphite-app bot reviewed Sep 8, 2025

View reviewed changes

TheBlueMatt reviewed Sep 8, 2025

View reviewed changes

wpaulino approved these changes Sep 9, 2025

View reviewed changes

tankyleo requested a review from TheBlueMatt September 9, 2025 16:29

TheBlueMatt approved these changes Sep 9, 2025

View reviewed changes

TheBlueMatt merged commit 867f084 into lightningdevkit:main Sep 9, 2025
25 checks passed

github-project-automation bot moved this from Goal: Merge to Done in Weekly Goals Sep 9, 2025

notmandatory mentioned this pull request Sep 25, 2025

Socratic Seminar #36 Topics NashBitDevs/nashbitdevs.org#107

Closed

brh28 mentioned this pull request Sep 29, 2025

Topics for Sept 29 2025 brh28/nash-litdevs#3

Closed

		// These subtractions panic on underflow, but this should never happen
		let trimmed_sum_sat = channel_value_satoshis - nondust_htlcs_value_sum_sat - to_broadcaster_value_sat - to_countersignatory_value_sat;

-				// These subtractions panic on underflow, but this should never happen
-				let trimmed_sum_sat = channel_value_satoshis - nondust_htlcs_value_sum_sat - to_broadcaster_value_sat - to_countersignatory_value_sat;
+				// Use checked subtraction to avoid potential panics
+				let trimmed_sum_sat = channel_value_satoshis
+					.checked_sub(nondust_htlcs_value_sum_sat)
+					.and_then(|result| result.checked_sub(to_broadcaster_value_sat))
+					.and_then(|result| result.checked_sub(to_countersignatory_value_sat))
+					.unwrap_or_else(|| {
+						// This should never happen, but we handle it gracefully instead of panicking
+						debug_assert!(false, "Channel value calculation underflowed");
+					});

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

tankyleo commented Sep 4, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

tankyleo Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

tankyleo Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Sep 4, 2025

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

tankyleo commented Sep 5, 2025

Uh oh!

tankyleo commented Sep 7, 2025

Uh oh!

ldk-reviews-bot commented Sep 8, 2025

Uh oh!

ldk-reviews-bot commented Sep 8, 2025

Uh oh!

graphite-app bot Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tankyleo commented Sep 4, 2025 •

edited

Loading

ldk-reviews-bot commented Sep 4, 2025 •

edited

Loading

codecov bot commented Sep 4, 2025 •

edited

Loading