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 62.30159% with 570 lines in your changes missing coverage. Please review.

Project coverage is 89.19%. Comparing base (0db051b) to head (a52d5c7).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	58.92%	235 Missing and 18 partials ⚠️
lightning/src/ln/channel.rs	69.46%	154 Missing and 17 partials ⚠️
lightning/src/ln/interactivetxs.rs	51.81%	142 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3137      +/-   ##
==========================================
- Coverage   89.61%   89.19%   -0.42%     
==========================================
  Files         127      127              
  Lines      103534   105156    +1622     
  Branches   103534   105156    +1622     
==========================================
+ Hits        92781    93796    +1015     
- Misses       8053     8614     +561     
- Partials     2700     2746      +46

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

lightning/src/ln/channelmanager.rs

dunxen · 2024-10-30T08:37:40Z

Thanks for the feedback, @jkczyz. Addressing it today.

jkczyz

Mostly minor things while going through a last pass.

lightning/src/ln/interactivetxs.rs

lightning/src/events/mod.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

dunxen · 2024-11-06T07:29:40Z

@TheBlueMatt this ready for another pass when you get a gap 🙏

lightning/src/ln/channel.rs

TheBlueMatt · 2024-11-06T20:07:25Z

lightning/src/ln/channel.rs

+	for (idx, input) in funding_inputs.iter().enumerate() {
+		if let Some(output) = input.1.as_transaction().output.get(input.0.previous_output.vout as usize) {
+			total_input_satoshis = total_input_satoshis.saturating_add(output.value.to_sat());
+			our_contributed_weight = our_contributed_weight.saturating_add(estimate_input_weight(output).to_wu());


Don't we need to account for the weight the witness will have? Presumably users doing dual funding will have to provide that as an input when they provide funding inputs. Luckily doesn't impact this PR because we don't do dual-funding yet but will need to be addressed before we do that.

Sorry for missing this comment which is a super important one 🤦‍♂️

It may be possible if they provide "dummy" witnesses or provide the sizes for the witnesses.

They won't be able to provide valid witnesses (that involve signatures) before construction of the funding transaction is finished.

The issue is since this transaction is always using SIGHASH_ALL for any inputs sigs, we don't know what the sighash we need to sign will be for the funding tx at this point as we have no idea of the inputs or outputs the counterparty will provide (except for the 2-of-2 funding output of course). So the signatures cannot be created by the user before construction is done. At best we can estimate based on the output type for each input provided by the user (without them providing sizes or dummy witnesses).

Thoughts? Or has Fry-day fried my brain? I'm actually hoping that's the case and it's a super trivial fix :P

We only need to care about the outputs provided by the local user, no? We're trying to calculate the weight that we've contributed so we don't care about the counterparty's inputs. I assume we can just make the user provide them.

Yeah we don't care about the weight of the counterparty's inputs/outputs, and we could ask the user to provide their own witness weights or dummy witnesses that use dummy signatures where appropriate.

What they won't be able to provide is valid signatures in witnesses until after the whole tx is constructed as it's SIGHASH_ALL, but we don't need them yet, just the sizes.

We will have to ask the user again to provide those valid witnesses after transaction construction like we do are doing for V2.

So should we just as for weights of witnesses along with the inputs? Then they can provide actual valid witnesses after construction like #2302 already does.

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

TheBlueMatt · 2024-11-06T21:09:23Z

lightning/src/ln/channel.rs

@@ -5298,6 +5625,58 @@ impl<SP: Deref> Channel<SP> where
 		}
 	}

+	pub fn tx_signatures(&mut self, msg: &msgs::TxSignatures) -> Result<(Option<msgs::TxSignatures>, Option<Transaction>), ChannelError> {
+		if let Some(ref mut signing_session) = self.interactive_tx_signing_session {


Don't we need to check the state here? ISTM a peer could send a tx_signatures message to us before sending us the initial commitment_signed leaving us unable to claim our funds if the peer goes away and the funding tx confirms. We'll also need to wait until the initial ChannelMonitor (from commitment_signed handling) has finished persisting before we broadcast.

We should check state and wait for persistence of initial ChannelMonitor here, but in InteractiveTxSigningSession we will only allow our tx_signatures to be sent if we have received a commitment_signed, but looks like something went missing in commitment_signed_initial_v2 along the way.

Nothing broke in the test as we always send an empty witnesses array in tx_signatures since we don't support contributing funds/inputs here. I'll make sure it goes through the full flow in this PR anyway so we can catch that error.

Thanks!

Might be worth adding a test with async persist here too. Set the persistence to InProgress, provide tx_signatures and make sure the tx doesn't broadcast then finish the persist and make sure it goes out then.

dunxen · 2024-11-07T08:30:41Z

lightning/src/ln/channel.rs

@@ -5298,6 +5625,58 @@ impl<SP: Deref> Channel<SP> where
 		}
 	}

+	pub fn tx_signatures(&mut self, msg: &msgs::TxSignatures) -> Result<(Option<msgs::TxSignatures>, Option<Transaction>), ChannelError> {
+		if let Some(ref mut signing_session) = self.interactive_tx_signing_session {


We should check state and wait for persistence of initial ChannelMonitor here, but in InteractiveTxSigningSession we will only allow our tx_signatures to be sent if we have received a commitment_signed, but looks like something went missing in commitment_signed_initial_v2 along the way.

Nothing broke in the test as we always send an empty witnesses array in tx_signatures since we don't support contributing funds/inputs here. I'll make sure it goes through the full flow in this PR anyway so we can catch that error.

Thanks!

lightning/src/ln/channel.rs

We'll only gate public API related to contributing toward an inbound or opening a dual funded channel.

Here we add the `interactive_tx_constructor` field to the `Channel`, `OutboundV2Channel`, and `InboundV2Channel` structs.

For now this is unneeded as we do not provide any inputs as channel acceptor and we do not allow creating outbound channels yet. It will be re-added when that functionality is introduced.

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

…ructed txs

… up funding_tx_constructed

jkczyz · 2024-11-12T00:12:39Z

lightning/src/ln/channel.rs

+			Some(ref mut tx_constructor) => tx_constructor.handle_tx_complete(msg)
+				.inspect(|tx_complete_value| {
+					if let HandleTxCompleteValue::SendTxComplete(_, signing_session) = tx_complete_value {
+						self.context_mut().next_funding_txid = Some(signing_session.unsigned_tx.txid());
+					}
+				})


Can't used inspect as it hasn't made it into our MSRV (1.63.0).

Yeah I've removed it locally and haven't pushed latest changes yet 👍

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

jkczyz reviewed Oct 30, 2024

View reviewed changes

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

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

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 4859142 to fe5e557 Compare October 30, 2024 13:52

dunxen mentioned this pull request Oct 31, 2024

Sort out DiscardFunding event with tests for V2 channels #3392

Open

dunxen force-pushed the 2024-06-non-public-API-v2-channels branch from fe5e557 to 63d4c8f Compare November 1, 2024 08:00

jkczyz reviewed Nov 1, 2024

View reviewed changes

dunxen force-pushed the 2024-06-non-public-API-v2-channels branch from 63d4c8f to be4449a Compare November 5, 2024 11:21

TheBlueMatt reviewed Nov 6, 2024

View reviewed changes

dunxen commented Nov 7, 2024

View reviewed changes

dunxen added 17 commits November 8, 2024 11:30

Add dual-funding spec links

6b4755d

Remove dual_funding cfg attributes

f0a7931

We'll only gate public API related to contributing toward an inbound or opening a dual funded channel.

Add interactive tx constructor to all V2-related channel structs

3e8ee23

Here we add the `interactive_tx_constructor` field to the `Channel`, `OutboundV2Channel`, and `InboundV2Channel` structs.

Remove unused Channel::dual_funding_channel_context

9206856

For now this is unneeded as we do not provide any inputs as channel acceptor and we do not allow creating outbound channels yet. It will be re-added when that functionality is introduced.

Add features in InteractiveTxConstructor requried for V2 chan accept

f1f100b

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

Implement support for accepting V2 channels

1736dc1

Introduce InteractiveTxSigningSession for signing interactively const…

96c5688

…ructed txs

Handle initial commitment_signed for V2 channels

3f9420f

f remove unused their_funding_satoshis field

42065c2

f move get_initial_commitment_signed to ChannelContext method & clean…

eddc69b

… up funding_tx_constructed

f set ChannelContext::funding_transaction in funding_tx_constructed

e09b0d7

Handle re-establishment next_funding_txid

ecdd54a

f set and clear next_funding_txid in channel.rs directly

c0e8501

Add option_dual_fund feature

0492239

Temporarily allow some unused dual-funding code

42d7147

Add manual testing for accepting dual-funded channels

c8ec160

Fix docs to indicate OpenChannelRequest is NOT persisted across restarts

d1823d5

dunxen force-pushed the 2024-06-non-public-API-v2-channels branch from be4449a to d1823d5 Compare November 11, 2024 09:59

dunxen mentioned this pull request Nov 11, 2024

Support async signing for V2 (dual_funded) channels #3404

Open

jkczyz reviewed Nov 12, 2024

View reviewed changes

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

dunxen commented Oct 30, 2024

jkczyz left a comment

dunxen commented Nov 6, 2024

TheBlueMatt Nov 6, 2024

dunxen Nov 8, 2024 •

edited

Loading

TheBlueMatt Nov 8, 2024 •

edited

Loading

dunxen Nov 8, 2024 •

edited

Loading

TheBlueMatt Nov 6, 2024

dunxen Nov 7, 2024

TheBlueMatt Nov 7, 2024

dunxen Nov 7, 2024

jkczyz Nov 12, 2024

dunxen Nov 12, 2024

Implement accepting dual-funded channels without contributing #3137

Are you sure you want to change the base?

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

dunxen commented Oct 30, 2024

jkczyz left a comment

Choose a reason for hiding this comment

dunxen commented Nov 6, 2024

Choose a reason for hiding this comment

dunxen Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

dunxen Nov 8, 2024 • 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

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 8, 2024 •

edited

Loading

TheBlueMatt Nov 8, 2024 •

edited

Loading

dunxen Nov 8, 2024 •

edited

Loading