Implement accepting dual-funded channels without contributing #3137

dunxen · 2024-06-20T11:02:30Z

We split this out from #2302 for easier review and to address the common non-public API parts of the V2 channel establishment implementation.

This will allow the holder to be an acceptor, but not initiator of V2 channels. We also don't expose an API for contributing to an inbound channel.

The functionality to initiate V2 channels and fund inbound channels forms part of #2302.

codecov-commenter · 2024-06-24T17:04:11Z

Codecov Report

Attention: Patch coverage is 60.06849% with 583 lines in your changes missing coverage. Please review.

Project coverage is 89.29%. Comparing base (4322b19) to head (8b0f4b5).

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	47.67%	227 Missing and 21 partials ⚠️
lightning/src/ln/channel.rs	59.73%	162 Missing and 18 partials ⚠️
lightning/src/ln/interactivetxs.rs	49.16%	150 Missing and 3 partials ⚠️
lightning/src/ln/dual_funding_tests.rs	99.02%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3137      +/-   ##
==========================================
- Coverage   89.69%   89.29%   -0.41%     
==========================================
  Files         129      130       +1     
  Lines      105437   106910    +1473     
  Branches   105437   106910    +1473     
==========================================
+ Hits        94573    95464     +891     
- Misses       8117     8665     +548     
- Partials     2747     2781      +34

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

optout21

Looks great, mostly localized code improvements suggested.

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/mod.rs

optout21 · 2024-06-26T12:42:52Z

As I see, the dual_funding cfg flag has been removed (not a problem)

optout21 · 2024-06-26T12:43:36Z

Looks to me that #2989 is in fact included in this PR (good!)

lightning/src/events/mod.rs

TheBlueMatt

Some initial comments, sorry this took so long to get back to.

lightning/src/events/mod.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2024-07-08T02:07:19Z

lightning/src/ln/channelmanager.rs

+						if chan.interactive_tx_signing_session.is_some() {
+							let monitor = try_chan_phase_entry!(self,
+								chan.commitment_signed_initial_v2(&msg, best_block, &self.signer_provider, &&logger), chan_phase_entry);
+							if let Ok(persist_status) = self.chain_monitor.watch_channel(chan.context.get_funding_txo().unwrap(), monitor) {


If the persist status is pending we need to handle the later stuff in monitor_updating_restored. Really the whole contents of the block here should be in monitor_updating_restored.

lightning/src/ln/channel.rs

dunxen

Thanks @TheBlueMatt, for review and some good points raised. I'll address these ASAP ❤️

dunxen · 2024-07-10T16:06:44Z

Still working on remaining initial feedback.

TheBlueMatt · 2024-07-15T15:51:54Z

No rush, let me know when you want another pass.

1. InteractiveTxConstructorArgs is introduced to act as a single, more readable input to InteractiveTxConstructor::new(). 2. Various documentation updates.

jkczyz

LGTM. @TheBlueMatt Could you take a look?

lightning/src/ln/channelmanager.rs

TheBlueMatt

Made it most of the way through. I think basically all the comments here can be addressed in a followup, so will get through the rest hopefully soon so we can land it. In the mean time, feel free to squash fixups and fix any of the smaller comments here you want in the process.

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2024-11-18T20:58:37Z

lightning/src/ln/channel.rs

+		HandleTxCompleteResult(Ok(tx_complete))
+	}
+
+	fn funding_tx_constructed<L: Deref>(


Can tx_complete just call this directly when required rather than having channelmanager.rs call tx_complete then call this based only on the return value of tx_complete? Would reduce the logic in channelmanager.rs.

Just note that we'll still want this as a function since it's called by signer_unblocked for async signing.

Fair, still good to consolidate logic in channel.rs where possible.

lightning/src/ln/channel.rs

TheBlueMatt · 2024-11-18T21:27:07Z

lightning/src/ln/channel.rs

@@ -9009,7 +9501,8 @@ impl<SP: Deref> Writeable for Channel<SP> where SP::Target: SignerProvider {
 			(47, next_holder_commitment_point, option),
 			(49, self.context.local_initiated_shutdown, option), // Added in 0.0.122
 			(51, is_manual_broadcast, option), // Added in 0.0.124
-			(53, funding_tx_broadcast_safe_event_emitted, option) // Added in 0.0.124
+			(53, funding_tx_broadcast_safe_event_emitted, option), // Added in 0.0.124
+			(55, self.context.next_funding_txid, option) // Added in 0.1.0


Yea, would be nice to remove this before we release and instead figure out the next_funding_txid field based on the funding transaction in the Channel and the current channel state.

Removed in #3417.

The follow-up PR #3423 should populate it on read.

TheBlueMatt · 2024-11-18T21:30:12Z

lightning/src/ln/channel.rs

@@ -9618,9 +10114,12 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch

 				blocked_monitor_updates: blocked_monitor_updates.unwrap(),
 				is_manual_broadcast: is_manual_broadcast.unwrap_or(false),
+				// If we've sent `commtiment_signed` for an interactively constructed transaction


Mmm, so really this is the "we were done with negotiation" flag in the channel_reestablish message. It does seem like we're missing some startup handling for this - basically we need to check on startup if the ChannelMonitor was persisted (implying we've sent our initial commitment_signed and thus we must not restart negotiation), but (a) it doesn't matter for inbound not-locally-funded channels and (b) maybe its fine just because the Channel will be dropped if its pre-ChannelMonitor (need a test/to check this, but I don't think so, since we mark the channel as funded in internal_tx_complete, at which point we'll persist the Channel even though it doesn't have a monitor yet).

Yeah this was a mistake to mark it funded at that point. Only after receiving an initial commitment_signed and getting a monitor do we need to persist. I'll fix the states up in the follow-up.

lightning/src/ln/channelmanager.rs

TheBlueMatt

Few more comments, got through the bulk of it. Basically the only issue(s) I think need fixing are the persistence of Channels before we have a ChannelMonitor which is gonna cause issues on restart. It can be fixed in a followup, though. Still need to review the tests but I can do that after this lands.

TheBlueMatt · 2024-11-19T13:34:26Z

lightning/src/ln/channelmanager.rs

-			"Dual-funded channels not supported".to_owned(),
-			msg.channel_id.clone())), counterparty_node_id);
+		// Note that we never need to persist the updated ChannelManager for an inbound
+		// tx_complete message - interactive transaction construction does not need to


But don't we send signatures in response to tx_complete (if we already received the peer's tx_complete and we're supposed to send first)? More generally, once the tx_completes have been exchanged, don't we at that point want to be able to resume the channel after restart (which implies a persistence, and also some additional handling cause we will currently FC on channel on startup if there's no ChannelMonitor which we won't have yet.

After consecutive tx_complete exchange, we will only send then initial commitment_signed.

We will only send a tx_signatures once we've received an initial commitment_signed if we are up first to send tx_signatures. Once we've received a commitment_signed we persist.

I've fixed this in the follow-up I'll have up soon. No PR yet.

lightning/src/ln/interactivetxs.rs

pending_changelog/3137-accept-dual-funding-without-contributing.txt

dunxen · 2024-11-19T15:14:36Z

Few more comments, got through the bulk of it. Basically the only issue(s) I think need fixing are the persistence of Channels before we have a ChannelMonitor which is gonna cause issues on restart. It can be fixed in a followup, though. Still need to review the tests but I can do that after this lands.

Thanks! Going to address a few more things you brought up throughout tonight and tomorrow morning for me and then I'll open an issue with remaining follow-ups. So unless something major prevents this from landing, we could land it tomorrow?

…ructed txs

dunxen · 2024-11-20T12:30:15Z

I've pushed up a few fixes. There are some nits that could be fixed here, but I'll include them in a followup to start unblocking other PRs.

@jkczyz, if CI and you are happy, we can go ahead and get this in when you're ready. Then I'll create the follow-up issue and tag it to make sure it's a blocker for next release.

jkczyz · 2024-11-20T17:53:53Z

I've pushed up a few fixes. There are some nits that could be fixed here, but I'll include them in a followup to start unblocking other PRs.

@jkczyz, if CI and you are happy, we can go ahead and get this in when you're ready. Then I'll create the follow-up issue and tag it to make sure it's a blocker for next release.

Sure, let's land this now and address the rest in follow-ups. Incremental mutants has been running for five hours, but no need to wait on it.

We want to remove this before release so that we can work on a way to not persist this but rather get it from other persisted data and just free up the TLV. Note that the "added in 0.0.124" comment was incorrect as it was actually added in lightningdevkit#3137 but the comment was stale so it's safe to remove.

dunxen added the Dual-funding label Jun 20, 2024

dunxen force-pushed the 2024-06-non-public-API-v2-channels branch 3 times, most recently from b981cd7 to 0caf60e Compare June 20, 2024 16:14

dunxen marked this pull request as ready for review June 20, 2024 16:27

dunxen mentioned this pull request Jun 20, 2024

Implement V2 channel establishment #2302

Draft

4 tasks

dunxen force-pushed the 2024-06-non-public-API-v2-channels branch from 0caf60e to 44821af Compare June 24, 2024 16:55

optout21 requested changes Jun 26, 2024

View reviewed changes

optout21 mentioned this pull request Jun 27, 2024

Interactive TX negotiation tracks shared outputs #2989

Merged

dunxen force-pushed the 2024-06-non-public-API-v2-channels branch 4 times, most recently from 0db700b to 7dceedd Compare July 3, 2024 09:44

dunxen requested review from optout21 and TheBlueMatt and removed request for optout21 July 3, 2024 09:51

optout21 previously approved these changes Jul 3, 2024

View reviewed changes

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

dunxen requested a review from jkczyz July 4, 2024 15:02

TheBlueMatt reviewed Jul 8, 2024

View reviewed changes

dunxen commented Jul 8, 2024

View reviewed changes

dunxen force-pushed the 2024-06-non-public-API-v2-channels branch from 7dceedd to 64c35f4 Compare July 10, 2024 16:05

dunxen force-pushed the 2024-06-non-public-API-v2-channels branch from 64c35f4 to a46da5a Compare July 11, 2024 14:55

dunxen force-pushed the 2024-06-non-public-API-v2-channels branch 2 times, most recently from 0613f64 to 93896c4 Compare July 16, 2024 10:36

Add features in InteractiveTxConstructor requried for V2 chan accept

f71bbb9

1. InteractiveTxConstructorArgs is introduced to act as a single, more readable input to InteractiveTxConstructor::new(). 2. Various documentation updates.

dunxen force-pushed the 2024-06-non-public-API-v2-channels branch from 90a4695 to cd26443 Compare November 18, 2024 13:20

jkczyz reviewed Nov 18, 2024

View reviewed changes

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

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

dunxen force-pushed the 2024-06-non-public-API-v2-channels branch from cd26443 to 984862e Compare November 18, 2024 18:17

TheBlueMatt reviewed Nov 18, 2024

View reviewed changes

dunxen force-pushed the 2024-06-non-public-API-v2-channels branch 3 times, most recently from 0b75814 to 8b7505d Compare November 19, 2024 13:14

TheBlueMatt reviewed Nov 19, 2024

View reviewed changes

dunxen added 10 commits November 20, 2024 13:57

Implement support for accepting V2 channels

24ba584

Introduce InteractiveTxSigningSession for signing interactively const…

684b3b7

…ructed txs

Handle initial commitment_signed for V2 channels

982e25d

Handle re-establishment next_funding_txid

c79b49d

Add option_dual_fund feature

f793bf8

Temporarily allow some unused dual-funding code

c3896b5

Add manual testing for accepting dual-funded channels

2deb41a

Fix docs to indicate OpenChannelRequest is NOT persisted across restarts

c762c6b

Remove unused muts in channel tests

85d58ba

Add pending release notes for accepting dual-funded channels

dd190ae

dunxen force-pushed the 2024-06-non-public-API-v2-channels branch from 8b7505d to dd190ae Compare November 20, 2024 12:28

jkczyz approved these changes Nov 20, 2024

View reviewed changes

jkczyz merged commit 0c31021 into lightningdevkit:main Nov 20, 2024
16 of 19 checks passed

dunxen deleted the 2024-06-non-public-API-v2-channels branch November 21, 2024 08:16

This was referenced Nov 21, 2024

Follow-ups for PR 3137 #3416

Open

Remove next_funding_txid tlv from Channel read/write #3417

Merged

jkczyz mentioned this pull request Nov 25, 2024

Follow-ups to PR 3137 #3423

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement accepting dual-funded channels without contributing #3137

Implement accepting dual-funded channels without contributing #3137

dunxen commented Jun 20, 2024 •

edited

Loading

codecov-commenter commented Jun 24, 2024 •

edited by codecov bot

Loading

optout21 left a comment •

edited

Loading

optout21 commented Jun 26, 2024

optout21 commented Jun 26, 2024

TheBlueMatt left a comment

TheBlueMatt Jul 8, 2024

dunxen left a comment

dunxen commented Jul 10, 2024

TheBlueMatt commented Jul 15, 2024

jkczyz left a comment

TheBlueMatt left a comment

TheBlueMatt Nov 18, 2024

jkczyz Nov 18, 2024

jkczyz Nov 18, 2024

TheBlueMatt Nov 19, 2024

TheBlueMatt Nov 18, 2024

dunxen Nov 22, 2024

dunxen Nov 22, 2024 •

edited

Loading

TheBlueMatt Nov 18, 2024

dunxen Nov 22, 2024

TheBlueMatt left a comment

TheBlueMatt Nov 19, 2024

dunxen Nov 22, 2024

dunxen commented Nov 19, 2024

dunxen commented Nov 20, 2024

jkczyz commented Nov 20, 2024

Implement accepting dual-funded channels without contributing #3137

Implement accepting dual-funded channels without contributing #3137

Conversation

dunxen commented Jun 20, 2024 • edited Loading

codecov-commenter commented Jun 24, 2024 • edited by codecov bot Loading

Codecov Report

optout21 left a comment • edited Loading

Choose a reason for hiding this comment

optout21 commented Jun 26, 2024

optout21 commented Jun 26, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunxen left a comment

Choose a reason for hiding this comment

dunxen commented Jul 10, 2024

TheBlueMatt commented Jul 15, 2024

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt left a 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

dunxen Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunxen commented Nov 19, 2024

dunxen commented Nov 20, 2024

jkczyz commented Nov 20, 2024

dunxen commented Jun 20, 2024 •

edited

Loading

codecov-commenter commented Jun 24, 2024 •

edited by codecov bot

Loading

optout21 left a comment •

edited

Loading

dunxen Nov 22, 2024 •

edited

Loading