Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement accepting dual-funded channels without contributing #3137

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

dunxen
Copy link
Contributor

@dunxen dunxen commented Jun 20, 2024

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.

@dunxen 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 dunxen marked this pull request as ready for review June 20, 2024 16:27
@dunxen dunxen mentioned this pull request Jun 20, 2024
4 tasks
@dunxen dunxen force-pushed the 2024-06-non-public-API-v2-channels branch from 0caf60e to 44821af Compare June 24, 2024 16:55
@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2024

Codecov Report

Attention: Patch coverage is 19.21331% with 1068 lines in your changes missing coverage. Please review.

Project coverage is 88.66%. Comparing base (ca27a85) to head (2216120).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channel.rs 5.66% 466 Missing ⚠️
lightning/src/ln/channelmanager.rs 21.75% 408 Missing and 2 partials ⚠️
lightning/src/ln/interactivetxs.rs 36.63% 191 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3137      +/-   ##
==========================================
- Coverage   89.63%   88.66%   -0.98%     
==========================================
  Files         126      126              
  Lines      102668   105124    +2456     
  Branches   102668   105124    +2456     
==========================================
+ Hits        92028    93205    +1177     
- Misses       7919     9261    +1342     
+ Partials     2721     2658      -63     

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

Copy link
Contributor

@optout21 optout21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, mostly localized code improvements suggested.

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/mod.rs Outdated Show resolved Hide resolved
@optout21
Copy link
Contributor

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

@optout21
Copy link
Contributor

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

@dunxen 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 dunxen requested review from optout21 and TheBlueMatt and removed request for optout21 July 3, 2024 09:51
optout21
optout21 previously approved these changes Jul 3, 2024
lightning/src/events/mod.rs Outdated Show resolved Hide resolved
@dunxen dunxen requested a review from jkczyz July 4, 2024 15:02
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

lightning/src/events/mod.rs Show resolved Hide resolved
lightning/src/events/mod.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.rs Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.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

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh...So the spec changed the definition of commitment_signed to now also include the initial funding commitment tx signing? Even though it implies other things inside the interactive state machine? Is it too late to change this? This seems nuts.

Does this mean that we should support adding HTLCs prior to the initial commitment transaction?

Copy link
Contributor Author

@dunxen dunxen Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous things it implies should really be looked at in the spec and changed there. We don't want to support adding HTLCs prior to initial commitment. So I really need to investigate what is going on with the spec and what might need to be fixed there.

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Copy link
Contributor Author

@dunxen dunxen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

dunxen commented Jul 10, 2024

Still working on remaining initial feedback.

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

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

@dunxen 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
lightning/src/ln/interactivetxs.rs Outdated Show resolved Hide resolved
lightning/src/ln/interactivetxs.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
@dunxen dunxen force-pushed the 2024-06-non-public-API-v2-channels branch from 5febfc9 to 8e70429 Compare September 3, 2024 14:05
@dunxen
Copy link
Contributor Author

dunxen commented Sep 3, 2024

I see there is still a lot of potential DRYing up to do here.

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few quick questions.

@@ -4480,6 +4759,105 @@ impl<SP: Deref> Channel<SP> where
Ok(())
}

pub fn commitment_signed_initial_v2<L: Deref>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be on OutboundChannelV2 since its basically the funding_signed equivalent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we are just adding it for inbound V2 channels, but actually, eventually outbound will also need it once that's enabled (https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#the-commitment_signed-message), and I'll probably share the behaviour via InteractivelyFunded trait or something like that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll want to handle it differently in that case, though, no? The initial commitment signed here is handled ~identical to funding_signed in OutboundV1Channel, which implies we need to DRY this up somewhat (by moving it to ChannelContext), but actually exposing it seems like something we should do on the type so that V1 and V2 work the same as each other as much as possible.

Moving this to Channel is a pretty substantial difference - we're now moving from the Outbound logic to the Channel/funded logic at a different point in the channel's lifecycle, which I'll be honest I don't really want to have to review.

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
Comment on lines +8973 to +8988
pub fn funding_tx_constructed<L: Deref>(
&mut self, signing_session: &mut InteractiveTxSigningSession, logger: &L
) -> Result<(msgs::CommitmentSigned, Option<Event>), ChannelError>
where
L::Target: Logger
{
let (commitment_signed, funding_ready_for_sig_event) = match self.internal_funding_tx_constructed(
signing_session, logger,
) {
Ok(res) => res,
Err(err) => return Err(err),
};

Ok((commitment_signed, funding_ready_for_sig_event))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entire method body can be replaced with:

	self.internal_funding_tx_constructed(signing_session, logger)

So better to just drop this method and rename internal_funding_tx_constructed to funding_tx_constructed?

@@ -8756,6 +9023,85 @@ fn get_initial_channel_type(config: &UserConfig, their_features: &InitFeatures)
ret
}

fn get_initial_counterparty_commitment_signature<SP:Deref, L: Deref>(
context: &mut ChannelContext<SP>, logger: &L
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context doesn't need to be a &mut here.

@@ -8756,6 +9023,85 @@ fn get_initial_channel_type(config: &UserConfig, their_features: &InitFeatures)
ret
}

fn get_initial_counterparty_commitment_signature<SP:Deref, L: Deref>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to make this a method on ChannelContext?

Likewise for get_initial_commitment_signed. At least there is a get_funding_signed_msg there already.

Comment on lines +9039 to +9045
Ok(ecdsa.sign_counterparty_commitment(&counterparty_initial_commitment_tx, Vec::new(), Vec::new(), &context.secp_ctx)
.map_err(|_| ChannelError::Close(
(
"Failed to get signatures for new commitment_signed".to_owned(),
ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) },
)))?.0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little more idiomatic and readable to map rather than use ?.0 and re-wrap.

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index d9fe0c5dd..dfa87a319 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -9053,12 +9021,13 @@ where
        match context.holder_signer {
                // TODO (taproot|arik): move match into calling method for Taproot
                ChannelSignerType::Ecdsa(ref ecdsa) => {
-                       Ok(ecdsa.sign_counterparty_commitment(&counterparty_initial_commitment_tx, Vec::new(), Vec::new(), &context.secp_ctx)
+                       ecdsa.sign_counterparty_commitment(&counterparty_initial_commitment_tx, Vec::new(), Vec::new(), &context.secp_ctx)
+                               .map(|(signature, _)| signature)
                                .map_err(|_| ChannelError::Close(
                                        (
                                                "Failed to get signatures for new commitment_signed".to_owned(),
                                                ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) },
-                                       )))?.0)
+                                       )))
                },
                // TODO (taproot|arik)
                #[cfg(taproot)]

Comment on lines +4807 to +4964
/// Handles a signed funding transaction generated by interactive transaction construction and
/// provided by the client.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't really clear from the docs when a user would call this and where transaction comes from. Or at least the wording is confusing to me.

Comment on lines 7957 to 8158
match chan_phase_entry.get_mut() {
ChannelPhase::UnfundedOutboundV2(chan) => {
chan.funding_tx_constructed(&mut signing_session, &self.logger)
},
ChannelPhase::UnfundedInboundV2(chan) => {
chan.funding_tx_constructed(&mut signing_session, &self.logger)
},
_ => Err(ChannelError::Warn(
"Got a tx_complete message with no interactive transaction construction expected or in-progress"
.into())),
}.and_then(|(commitment_signed, funding_ready_for_sig_event_opt)| {
let (channel_id, channel_phase) = chan_phase_entry.remove_entry();
match channel_phase {
ChannelPhase::UnfundedOutboundV2(chan) => {
chan.into_channel(signing_session)
},
ChannelPhase::UnfundedInboundV2(chan) => {
chan.into_channel(signing_session)
},
_ => {
debug_assert!(false); // It cannot be another variant as we are in the `Ok` branch of the above match.
Err(ChannelError::Warn(
"Got a tx_complete message with no interactive transaction construction expected or in-progress"
.into()))
}
}.map(|channel| (channel_id, channel, funding_ready_for_sig_event_opt, commitment_signed))
}).map(|(channel_id, channel, funding_ready_for_sig_event_opt, commitment_signed)| {
peer_state.channel_by_id.insert(channel_id, ChannelPhase::Funded(channel));
if let Some(funding_ready_for_sig_event) = funding_ready_for_sig_event_opt {
let mut pending_events = self.pending_events.lock().unwrap();
pending_events.push_back((funding_ready_for_sig_event, None));
}
peer_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
node_id: counterparty_node_id,
updates: CommitmentUpdate {
commitment_signed,
update_add_htlcs: vec![],
update_fulfill_htlcs: vec![],
update_fail_htlcs: vec![],
update_fail_malformed_htlcs: vec![],
update_fee: None,
},
});
}).map_err(|err| MsgHandleErrInternal::send_err_msg_no_close(format!("{}", err), msg.channel_id))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When possible, it's best to avoid side-effects inside map and and_then calls. It would make the code easier to follow and less indented. Consider the following:

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index b23bd08ff..c2e67287a 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -7955,7 +7955,7 @@ where
                                };
                                if let Some(mut signing_session) = signing_session_opt {
                                        let funding_txid = signing_session.unsigned_tx.txid();
-                                       match chan_phase_entry.get_mut() {
+                                       let (commitment_signed, funding_ready_for_sig_event_opt) = match chan_phase_entry.get_mut() {
                                                ChannelPhase::UnfundedOutboundV2(chan) => {
                                                        chan.funding_tx_constructed(&mut signing_session, &self.logger)
                                                },
@@ -7965,45 +7965,39 @@ where
                                                _ => Err(ChannelError::Warn(
                                                        "Got a tx_complete message with no interactive transaction construction expected or in-progress"
                                                        .into())),
-                                       }.and_then(|(commitment_signed, funding_ready_for_sig_event_opt)| {
-                                                       let (channel_id, channel_phase) = chan_phase_entry.remove_entry();
-                                                       match channel_phase {
-                                                               ChannelPhase::UnfundedOutboundV2(chan) => {
-                                                                       chan.into_channel(signing_session)
-                                                               },
-                                                               ChannelPhase::UnfundedInboundV2(chan) => {
-                                                                       chan.into_channel(signing_session)
-                                                               },
-                                                               _ => {
-                                                                       debug_assert!(false); // It cannot be another variant as we are in the `Ok` branch of the above match.
-                                                                       Err(ChannelError::Warn(
-                                                                               "Got a tx_complete message with no interactive transaction construction expected or in-progress"
-                                                                                       .into()))
-                                                               }
-                                                       }.map(|channel| (channel_id, channel, funding_ready_for_sig_event_opt, commitment_signed))
-                                               }).map(|(channel_id, mut channel, funding_ready_for_sig_event_opt, commitment_signed)| {
-                                                       channel.set_next_funding_txid(&funding_txid);
-                                                       peer_state.channel_by_id.insert(channel_id, ChannelPhase::Funded(channel));
-                                                       if let Some(funding_ready_for_sig_event) = funding_ready_for_sig_event_opt {
-                                                               let mut pending_events = self.pending_events.lock().unwrap();
-                                                               pending_events.push_back((funding_ready_for_sig_event, None));
-                                                       }
-                                                       peer_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
-                                                               node_id: counterparty_node_id,
-                                                               updates: CommitmentUpdate {
-                                                                       commitment_signed,
-                                                                       update_add_htlcs: vec![],
-                                                                       update_fulfill_htlcs: vec![],
-                                                                       update_fail_htlcs: vec![],
-                                                                       update_fail_malformed_htlcs: vec![],
-                                                                       update_fee: None,
-                                                               },
-                                                       });
-                                               }).map_err(|err| MsgHandleErrInternal::send_err_msg_no_close(format!("{}", err), msg.channel_id))
-                               } else {
-                                       // We're not in a signing session yet so we don't need to do anything else.
-                                       Ok(())
+                                       }.map_err(|err| MsgHandleErrInternal::send_err_msg_no_close(format!("{}", err), msg.channel_id))?;
+
+                                       let (channel_id, channel_phase) = chan_phase_entry.remove_entry();
+                                       let mut channel = match channel_phase {
+                                               ChannelPhase::UnfundedOutboundV2(chan) => chan.into_channel(signing_session),
+                                               ChannelPhase::UnfundedInboundV2(chan) => chan.into_channel(signing_session),
+                                               _ => {
+                                                       debug_assert!(false); // It cannot be another variant as we are in the `Ok` branch of the above match.
+                                                       Err(ChannelError::Warn(
+                                                               "Got a tx_complete message with no interactive transaction construction expected or in-progress"
+                                                                       .into()))
+                                               },
+                                       }.map_err(|err| MsgHandleErrInternal::send_err_msg_no_close(format!("{}", err), msg.channel_id))?;
+
+                                       channel.set_next_funding_txid(&funding_txid);
+                                       peer_state.channel_by_id.insert(channel_id, ChannelPhase::Funded(channel));
+                                       if let Some(funding_ready_for_sig_event) = funding_ready_for_sig_event_opt {
+                                               let mut pending_events = self.pending_events.lock().unwrap();
+                                               pending_events.push_back((funding_ready_for_sig_event, None));
+                                       }
+                                       peer_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
+                                               node_id: counterparty_node_id,
+                                               updates: CommitmentUpdate {
+                                                       commitment_signed,
+                                                       update_add_htlcs: vec![],
+                                                       update_fulfill_htlcs: vec![],
+                                                       update_fail_htlcs: vec![],
+                                                       update_fail_malformed_htlcs: vec![],
+                                                       update_fee: None,
+                                               },
+                                       });
                                }
+                               Ok(())
                        },
                        hash_map::Entry::Vacant(_) => {
                                Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.channel_id))

@@ -9530,7 +9542,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.0.125
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add this to the ReadableArgs implementation as discussed offline.

@@ -96,7 +96,7 @@ mod async_signer_tests;
#[cfg(test)]
#[allow(unused_mut)]
mod offers_tests;
#[allow(dead_code)] // TODO(dual_funding): Exchange for dual_funding cfg
#[allow(dead_code)] // TODO(splicing): Exchange for splicing cfg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the dead_code exception no longer needed. This change seems to be search/and/replace false positive.

@TheBlueMatt
Copy link
Collaborator

Needs rebase now :(

@dunxen
Copy link
Contributor Author

dunxen commented Sep 18, 2024

Needs rebase now :(

Sorry, catching up on everything now! Been mostly offline for a week.

@dunxen dunxen force-pushed the 2024-06-non-public-API-v2-channels branch from 9b46d1f to 2b65e1f Compare September 18, 2024 11:04
@dunxen
Copy link
Contributor Author

dunxen commented Sep 18, 2024

Just rebased for now while I'm still addressing past week's feedback.

@dunxen dunxen force-pushed the 2024-06-non-public-API-v2-channels branch from 2b65e1f to 6dc0b7c Compare September 19, 2024 22:27
@dunxen dunxen force-pushed the 2024-06-non-public-API-v2-channels branch from 6dc0b7c to 2216120 Compare September 19, 2024 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants