Channel Establishment for V3 Channels #3792

carlaKC · 2025-05-22T15:43:09Z

This PR updates the channel establishment flow to allow and validate V3 channels (behind test flag).

ldk-reviews-bot · 2025-05-22T15:43:13Z

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

TheBlueMatt

Cool, basically all LGTM.

TheBlueMatt · 2025-05-22T18:02:13Z

lightning/src/ln/channelmanager.rs

@@ -16153,22 +16154,28 @@ mod tests {
 	}

 	#[test]
-	fn test_anchors_zero_fee_htlc_tx_fallback() {
+	fn test_anchors_zero_fee_htlc_tx_downgrade() {


Congrats, you touched it, now you get to move it out of channelmanager into some other test-specific file that isn't 15000 lines of code 😂

Done this in a follow up in #3797 so that move + format can be reviewed separately.

TheBlueMatt · 2025-05-22T18:04:58Z

lightning/src/util/config.rs

+	/// back to a `anchors_zero_fee_htlc` (if [`Self::negotiate_anchors_zero_fee_htlc_tx`]
+	/// is set) or `static_remote_key` channel.
+	///
+	/// *Implies [`Self::negotiate_anchors_zero_fee_htlc_tx`].*


These are in conflict - one says we'll fall back if its set, the other says that its implied (ie always set) if we set this flag.

ldk-reviews-bot · 2025-05-22T18:13:18Z

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

ldk-reviews-bot · 2025-05-22T18:13:31Z

✅ Added second reviewer: @valentinewallace

wpaulino

LGTM after Matt's comments are addressed

carlaKC · 2025-05-23T18:36:06Z

Removed conflicting docs statement + opened followup for test separation (felt wrong to do move+format in the same PR, happy to include if we want it in here).

ldk-reviews-bot · 2025-05-26T00:00:59Z

🔔 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-05-26T00:01:04Z

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

tankyleo

Sorry for the delay, here's a first pass :)

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

ldk-reviews-bot · 2025-05-28T00:01:29Z

🔔 2nd 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-05-28T00:01:39Z

🔔 2nd 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.

tankyleo

Another pass :)

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

lightning/src/util/config.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

carlaKC · 2025-05-29T15:53:46Z

Major change in push is using get_initial_channel_type in channel type downgrades to DRY up the code a bit.
Otherwise addressed nits + added some tests in fixups (full diff).

tankyleo

Thank you for the downgrade tests ! Just three nits at this point :)

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

carlaKC · 2025-05-30T20:26:33Z

Added test + fixed minor nit.

ldk-reviews-bot · 2025-05-31T00:00:17Z

🔔 3rd 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-05-31T00:00:25Z

🔔 3rd 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-06-02T00:01:32Z

🔔 4th 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.

carlaKC · 2025-06-04T15:42:37Z

Rebased + added additional comment on assert - diff.

carlaKC · 2025-06-04T15:54:38Z

Two followups for this to do some test cleaning while we're here:

Move and Rustfmt Channel Type Negotiation Tests #3827: moves + formats some of the channel.rs tests and adds coverage for get_initial_channel_type as disucssed with @tankyleo
Move and Rustfmt channel acceptance and type downgrade tests #3797: moves + formats some channel downgrades from channelmanager.rs

lightning/src/ln/channel.rs

Useful for the commits that follow where we add more downgrade tests.

carlaKC · 2025-06-04T20:54:57Z

Rebased + addressed nit.

codecov · 2025-06-04T23:10:53Z

Codecov Report

Attention: Patch coverage is 95.78313% with 14 lines in your changes missing coverage. Please review.

Project coverage is 89.93%. Comparing base (7b62ea4) to head (99c645e).

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	93.02%	9 Missing ⚠️
lightning/src/ln/functional_tests.rs	94.11%	2 Missing and 1 partial ⚠️
lightning/src/ln/channelmanager.rs	98.56%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3792      +/-   ##
==========================================
+ Coverage   89.91%   89.93%   +0.02%     
==========================================
  Files         160      160              
  Lines      129307   129579     +272     
  Branches   129307   129579     +272     
==========================================
+ Hits       116262   116539     +277     
+ Misses      10355    10352       -3     
+ Partials     2690     2688       -2

☔ 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

Grrr, one issue sorry.

TheBlueMatt · 2025-06-06T13:52:43Z

lightning/src/ln/channel.rs

+			eligible_features.clear_anchor_zero_fee_commitments();
+			eligible_features.clear_anchors_zero_fee_htlc_tx();
+			assert!(!eligible_features.supports_anchor_zero_fee_commitments());
+			assert!(!eligible_features.supports_anchors_nonzero_fee_htlc_tx());


Isn't this trivially reachable if a peer sets the nonzero-fee-htlc-tx feature bit? Generally, though, all the assertions here seem like they could be debug_assertions or elided entirely (asserting that we don't support something we just cleared probably belongs in a features test, and I believe already exists).

Will remove the feature bit asserts 👍

Isn't this trivially reachable if a peer sets the nonzero-fee-htlc-tx feature bit?

ofc, it should be on channel_type 🤦‍♀️

Rather than duplicating our channel type preference ordering in downgrade logic, make a modified version of the remote peer's supported features and remove our current channel type from it to get the next preferred channel type.

To allow testing along the way in this PR, turn on negotiation of zero fee channels. Co-authored-by: Matt Corallo <[email protected]>

Sender: MUST set `feerate_per_kw` to zero Receiver: MUST fail the channel if `feerate_per_kw` != 0 Co-authored-by: Matt Corallo <[email protected]>

Like anchor channels, these channels require that the user reserves a UTXO to bump the channel. If we automatically accept this channel type and the user does not have such reserve available, they are at risk of losing funds because they cannot fee bump the channel.

carlaKC · 2025-06-06T15:39:20Z

lightning/src/ln/channel.rs

+		// We should never have negotiated `anchors_nonzero_fee_htlc_tx` because it can result in a
+		// loss of funds.
+		let channel_type = &funding.channel_transaction_parameters.channel_type_features;
+		assert!(!channel_type.supports_anchors_nonzero_fee_htlc_tx());


Moved this assert up because we should never negotiate a non-zero-htlc-anchor channel, and it's a risk of lost funds if we do.

Couldn't see a historical reason why we only checked it if the channel type was supports_anchors_zero_fee_htlc_tx - went back a few commits and couldn't find reasoning. cc @TheBlueMatt

carlaKC · 2025-06-06T15:41:07Z

Removed asserts + moved supports_anchors_nonzero_fee_htlc_tx assert, diff here.

lightning/src/ln/channel.rs

carlaKC requested a review from TheBlueMatt May 22, 2025 15:43

carlaKC mentioned this pull request May 22, 2025

Zero Fee Commitments #3789

Open

36 tasks

TheBlueMatt reviewed May 22, 2025

View reviewed changes

ldk-reviews-bot requested a review from valentinewallace May 22, 2025 18:13

wpaulino requested review from wpaulino and removed request for valentinewallace May 22, 2025 18:17

wpaulino reviewed May 23, 2025

View reviewed changes

carlaKC force-pushed the 3789-channelestablishment branch from 3a36506 to b9ec89c Compare May 23, 2025 18:30

carlaKC mentioned this pull request May 23, 2025

Move and Rustfmt channel acceptance and type downgrade tests #3797

Open

carlaKC requested review from wpaulino and TheBlueMatt May 23, 2025 20:54

tankyleo reviewed May 27, 2025

View reviewed changes

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

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

lightning/src/ln/channel.rs Show resolved Hide resolved

tankyleo reviewed May 28, 2025

View reviewed changes

carlaKC force-pushed the 3789-channelestablishment branch from b9ec89c to 892d87a Compare May 29, 2025 15:50

carlaKC requested a review from tankyleo May 29, 2025 15:53

tankyleo reviewed May 29, 2025

View reviewed changes

lightning/src/ln/channel.rs Show resolved Hide resolved

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

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

carlaKC force-pushed the 3789-channelestablishment branch from 892d87a to e56e32a Compare May 30, 2025 20:23

tankyleo reviewed Jun 4, 2025

View reviewed changes

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

ln/refactor: add helper test function for channel type downgrade

588f490

Useful for the commits that follow where we add more downgrade tests.

carlaKC force-pushed the 3789-channelestablishment branch 2 times, most recently from e430212 to 99c645e Compare June 4, 2025 20:54

tankyleo previously approved these changes Jun 4, 2025

View reviewed changes

carlaKC requested a review from TheBlueMatt June 5, 2025 16:15

carlaKC mentioned this pull request Jun 6, 2025

Move and Rustfmt Channel Type Negotiation Tests #3827

Open

TheBlueMatt reviewed Jun 6, 2025

View reviewed changes

carlaKC and others added 7 commits June 6, 2025 11:14

ln+util: add test option to enable negotiation of zero fee channels

ffddb20

To allow testing along the way in this PR, turn on negotiation of zero fee channels. Co-authored-by: Matt Corallo <[email protected]>

ln/refactor: add helper function for channel type tests

e4465c4

ln: do not set or accept non-zero feerates in open_channel for v3

aab2d29

Sender: MUST set `feerate_per_kw` to zero Receiver: MUST fail the channel if `feerate_per_kw` != 0 Co-authored-by: Matt Corallo <[email protected]>

ln/refactor: add helper function for manual acceptance tests

67e394d

ln: set max_htlcs based on the channel type

23de01c

carlaKC dismissed tankyleo’s stale review via 23de01c June 6, 2025 15:37

carlaKC force-pushed the 3789-channelestablishment branch from 99c645e to 23de01c Compare June 6, 2025 15:37

carlaKC commented Jun 6, 2025

View reviewed changes

carlaKC requested review from TheBlueMatt and tankyleo June 6, 2025 16:37

tankyleo reviewed Jun 6, 2025

View reviewed changes

lightning/src/ln/channel.rs Show resolved Hide resolved

tankyleo approved these changes Jun 6, 2025

View reviewed changes

TheBlueMatt approved these changes Jun 6, 2025

View reviewed changes

TheBlueMatt merged commit 0430b1e into lightningdevkit:main Jun 6, 2025
25 of 26 checks passed

wpaulino removed their request for review June 6, 2025 21:06

Channel Establishment for V3 Channels #3792

Channel Establishment for V3 Channels #3792

Uh oh!

Conversation

carlaKC commented May 22, 2025

Uh oh!

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

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt May 22, 2025

Choose a reason for hiding this comment

Uh oh!

carlaKC May 23, 2025

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt May 22, 2025

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented May 22, 2025

Uh oh!

ldk-reviews-bot commented May 22, 2025

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

carlaKC commented May 23, 2025

Uh oh!

ldk-reviews-bot commented May 26, 2025

Uh oh!

ldk-reviews-bot commented May 26, 2025

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented May 28, 2025

Uh oh!

ldk-reviews-bot commented May 28, 2025

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlaKC commented May 29, 2025

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlaKC commented May 30, 2025

Uh oh!

ldk-reviews-bot commented May 31, 2025

Uh oh!

ldk-reviews-bot commented May 31, 2025

Uh oh!

ldk-reviews-bot commented Jun 2, 2025

Uh oh!

carlaKC commented Jun 4, 2025

Uh oh!

carlaKC commented Jun 4, 2025

Uh oh!

Uh oh!

carlaKC commented Jun 4, 2025

Uh oh!

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

Uh oh!

ldk-reviews-bot commented May 22, 2025 •

edited

Loading

codecov bot commented Jun 4, 2025 •

edited

Loading