Skip to content

Implement accepting dual-funded channels without contributing #3137

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

Merged
merged 15 commits into from
Nov 20, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Handle re-establishment next_funding_txid
  • Loading branch information
dunxen committed Nov 20, 2024
commit c79b49d5f38063eadbbbeb8370ab68055ebbbcc6
105 changes: 87 additions & 18 deletions lightning/src/ln/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,9 @@ use crate::ln::types::ChannelId;
use crate::types::payment::{PaymentPreimage, PaymentHash};
use crate::types::features::{ChannelTypeFeatures, InitFeatures};
use crate::ln::interactivetxs::{
get_output_weight, HandleTxCompleteResult, InteractiveTxConstructor, InteractiveTxConstructorArgs,
InteractiveTxSigningSession, InteractiveTxMessageSendResult, TX_COMMON_FIELDS_WEIGHT,
get_output_weight, HandleTxCompleteValue, HandleTxCompleteResult, InteractiveTxConstructor,
InteractiveTxConstructorArgs, InteractiveTxSigningSession, InteractiveTxMessageSendResult,
TX_COMMON_FIELDS_WEIGHT,
};
use crate::ln::msgs;
use crate::ln::msgs::{ClosingSigned, ClosingSignedFeeRange, DecodeError};
Expand Down Expand Up @@ -901,6 +902,7 @@ pub(super) struct MonitorRestoreUpdates {
pub funding_broadcastable: Option<Transaction>,
pub channel_ready: Option<msgs::ChannelReady>,
pub announcement_sigs: Option<msgs::AnnouncementSignatures>,
pub tx_signatures: Option<msgs::TxSignatures>,
}

/// The return value of `signer_maybe_unblocked`
Expand Down Expand Up @@ -1252,6 +1254,7 @@ pub(super) struct ChannelContext<SP: Deref> where SP::Target: SignerProvider {
monitor_pending_failures: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>,
monitor_pending_finalized_fulfills: Vec<HTLCSource>,
monitor_pending_update_adds: Vec<msgs::UpdateAddHTLC>,
monitor_pending_tx_signatures: Option<msgs::TxSignatures>,

/// If we went to send a revoke_and_ack but our signer was unable to give us a signature,
/// we should retry at some point in the future when the signer indicates it may have a
Expand Down Expand Up @@ -1494,6 +1497,21 @@ pub(super) struct ChannelContext<SP: Deref> where SP::Target: SignerProvider {
/// If we can't release a [`ChannelMonitorUpdate`] until some external action completes, we
/// store it here and only release it to the `ChannelManager` once it asks for it.
blocked_monitor_updates: Vec<PendingChannelMonitorUpdate>,
// The `next_funding_txid` field allows peers to finalize the signing steps of an interactive
// transaction construction, or safely abort that transaction if it was not signed by one of the
// peers, who has thus already removed it from its state.
//
// If we've sent `commtiment_signed` for an interactively constructed transaction
// during a signing session, but have not received `tx_signatures` we MUST set `next_funding_txid`
// to the txid of that interactive transaction, else we MUST NOT set it.
//
// See the spec for further details on this:
// * `channel_reestablish`-sending node: https://github.com/lightning/bolts/blob/247e83d/02-peer-protocol.md?plain=1#L2466-L2470
// * `channel_reestablish`-receiving node: https://github.com/lightning/bolts/blob/247e83d/02-peer-protocol.md?plain=1#L2520-L2531
//
// TODO(dual_funding): Persist this when we actually contribute funding inputs. For now we always
// send an empty witnesses array in `tx_signatures` as a V2 channel acceptor
next_funding_txid: Option<Txid>,
}

/// A channel struct implementing this trait can receive an initial counterparty commitment
Expand Down Expand Up @@ -1710,14 +1728,29 @@ pub(super) trait InteractivelyFunded<SP: Deref> where SP::Target: SignerProvider
}

fn tx_complete(&mut self, msg: &msgs::TxComplete) -> HandleTxCompleteResult {
HandleTxCompleteResult(match self.interactive_tx_constructor_mut() {
Some(ref mut tx_constructor) => tx_constructor.handle_tx_complete(msg).map_err(
|reason| reason.into_tx_abort_msg(self.context().channel_id())),
None => Err(msgs::TxAbort {
channel_id: self.context().channel_id(),
data: b"No interactive transaction negotiation in progress".to_vec()
}),
})
let tx_constructor = match self.interactive_tx_constructor_mut() {
Some(ref mut tx_constructor) => tx_constructor,
None => {
let tx_abort = msgs::TxAbort {
channel_id: msg.channel_id,
data: b"No interactive transaction negotiation in progress".to_vec(),
};
return HandleTxCompleteResult(Err(tx_abort));
},
};

let tx_complete = match tx_constructor.handle_tx_complete(msg) {
Ok(tx_complete) => tx_complete,
Err(reason) => {
return HandleTxCompleteResult(Err(reason.into_tx_abort_msg(msg.channel_id)))
}
};

if let HandleTxCompleteValue::SendTxComplete(_, ref signing_session) = tx_complete {
self.context_mut().next_funding_txid = Some(signing_session.unsigned_tx.compute_txid());
};

HandleTxCompleteResult(Ok(tx_complete))
}

fn funding_tx_constructed<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.

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

See #3411

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Expand Down Expand Up @@ -2077,6 +2110,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
monitor_pending_failures: Vec::new(),
monitor_pending_finalized_fulfills: Vec::new(),
monitor_pending_update_adds: Vec::new(),
monitor_pending_tx_signatures: None,

signer_pending_revoke_and_ack: false,
signer_pending_commitment_update: false,
Expand Down Expand Up @@ -2170,6 +2204,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
blocked_monitor_updates: Vec::new(),

is_manual_broadcast: false,

next_funding_txid: None,
};

Ok(channel_context)
Expand Down Expand Up @@ -2311,6 +2347,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
monitor_pending_failures: Vec::new(),
monitor_pending_finalized_fulfills: Vec::new(),
monitor_pending_update_adds: Vec::new(),
monitor_pending_tx_signatures: None,

signer_pending_revoke_and_ack: false,
signer_pending_commitment_update: false,
Expand Down Expand Up @@ -2401,6 +2438,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
blocked_monitor_updates: Vec::new(),
local_initiated_shutdown: None,
is_manual_broadcast: false,
next_funding_txid: None,
})
}

Expand Down Expand Up @@ -4955,6 +4993,14 @@ impl<SP: Deref> Channel<SP> where
self.context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new());
self.monitor_updating_paused(false, false, need_channel_ready, Vec::new(), Vec::new(), Vec::new());

if let Some(tx_signatures) = self.interactive_tx_signing_session.as_mut().and_then(
|session| session.received_commitment_signed()
) {
// We're up first for submitting our tx_signatures, but our monitor has not persisted yet
// so they'll be sent as soon as that's done.
self.context.monitor_pending_tx_signatures = Some(tx_signatures);
}

Ok(channel_monitor)
}

Expand Down Expand Up @@ -5628,7 +5674,13 @@ impl<SP: Deref> Channel<SP> where
}
}

pub fn tx_signatures(&mut self, msg: &msgs::TxSignatures) -> Result<(Option<msgs::TxSignatures>, Option<Transaction>), ChannelError> {
pub fn tx_signatures<L: Deref>(&mut self, msg: &msgs::TxSignatures, logger: &L) -> Result<(Option<msgs::TxSignatures>, Option<Transaction>), ChannelError>
where L::Target: Logger
{
if !matches!(self.context.channel_state, ChannelState::FundingNegotiated) {
return Err(ChannelError::close("Received tx_signatures in strange state!".to_owned()));
}

if let Some(ref mut signing_session) = self.interactive_tx_signing_session {
if msg.witnesses.len() != signing_session.remote_inputs_count() {
return Err(ChannelError::Warn(
Expand Down Expand Up @@ -5666,9 +5718,17 @@ impl<SP: Deref> Channel<SP> where
}
self.context.funding_transaction = funding_tx_opt.clone();

self.context.next_funding_txid = None;

// Clear out the signing session
self.interactive_tx_signing_session = None;

if tx_signatures_opt.is_some() && self.context.channel_state.is_monitor_update_in_progress() {
log_debug!(logger, "Not sending tx_signatures: a monitor update is in progress. Setting monitor_pending_tx_signatures.");
self.context.monitor_pending_tx_signatures = tx_signatures_opt;
return Ok((None, None));
}

Ok((tx_signatures_opt, funding_tx_opt))
} else {
Err(ChannelError::Close((
Expand Down Expand Up @@ -5911,14 +5971,18 @@ impl<SP: Deref> Channel<SP> where
mem::swap(&mut finalized_claimed_htlcs, &mut self.context.monitor_pending_finalized_fulfills);
let mut pending_update_adds = Vec::new();
mem::swap(&mut pending_update_adds, &mut self.context.monitor_pending_update_adds);
// For channels established with V2 establishment we won't send a `tx_signatures` when we're in
// MonitorUpdateInProgress (and we assume the user will never directly broadcast the funding
// transaction and waits for us to do it).
let tx_signatures = self.context.monitor_pending_tx_signatures.take();

if self.context.channel_state.is_peer_disconnected() {
self.context.monitor_pending_revoke_and_ack = false;
self.context.monitor_pending_commitment_signed = false;
return MonitorRestoreUpdates {
raa: None, commitment_update: None, order: RAACommitmentOrder::RevokeAndACKFirst,
accepted_htlcs, failed_htlcs, finalized_claimed_htlcs, pending_update_adds,
funding_broadcastable, channel_ready, announcement_sigs
funding_broadcastable, channel_ready, announcement_sigs, tx_signatures
};
}

Expand Down Expand Up @@ -5952,7 +6016,7 @@ impl<SP: Deref> Channel<SP> where
match order { RAACommitmentOrder::CommitmentFirst => "commitment", RAACommitmentOrder::RevokeAndACKFirst => "RAA"});
MonitorRestoreUpdates {
raa, commitment_update, order, accepted_htlcs, failed_htlcs, finalized_claimed_htlcs,
pending_update_adds, funding_broadcastable, channel_ready, announcement_sigs
pending_update_adds, funding_broadcastable, channel_ready, announcement_sigs, tx_signatures
}
}

Expand Down Expand Up @@ -7723,10 +7787,7 @@ impl<SP: Deref> Channel<SP> where
next_remote_commitment_number: INITIAL_COMMITMENT_NUMBER - self.context.cur_counterparty_commitment_transaction_number - 1,
your_last_per_commitment_secret: remote_last_secret,
my_current_per_commitment_point: dummy_pubkey,
// TODO(dual_funding): If we've sent `commtiment_signed` for an interactive transaction
// construction but have not received `tx_signatures` we MUST set `next_funding_txid` to the
// txid of that interactive transaction, else we MUST NOT set it.
next_funding_txid: None,
next_funding_txid: self.context.next_funding_txid,
}
}

Expand Down Expand Up @@ -9427,7 +9488,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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in #3417.

Copy link
Contributor Author

@dunxen dunxen Nov 22, 2024

Choose a reason for hiding this comment

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

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

});

Ok(())
Expand Down Expand Up @@ -9717,6 +9779,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
let mut channel_pending_event_emitted = None;
let mut channel_ready_event_emitted = None;
let mut funding_tx_broadcast_safe_event_emitted = None;
let mut next_funding_txid = funding_transaction.as_ref().map(|tx| tx.compute_txid());

let mut user_id_high_opt: Option<u64> = None;
let mut channel_keys_id: Option<[u8; 32]> = None;
Expand Down Expand Up @@ -9777,6 +9840,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
(49, local_initiated_shutdown, option),
(51, is_manual_broadcast, option),
(53, funding_tx_broadcast_safe_event_emitted, option),
(55, next_funding_txid, option) // Added in 0.0.125
});

let (channel_keys_id, holder_signer) = if let Some(channel_keys_id) = channel_keys_id {
Expand Down Expand Up @@ -9950,6 +10014,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
monitor_pending_failures,
monitor_pending_finalized_fulfills: monitor_pending_finalized_fulfills.unwrap(),
monitor_pending_update_adds: monitor_pending_update_adds.unwrap_or_default(),
monitor_pending_tx_signatures: None,

signer_pending_revoke_and_ack: false,
signer_pending_commitment_update: false,
Expand Down Expand Up @@ -10036,6 +10101,10 @@ 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// during a signing session, but have not received `tx_signatures` we MUST set `next_funding_txid`
// to the txid of that interactive transaction, else we MUST NOT set it.
next_funding_txid,
},
interactive_tx_signing_session: None,
})
Expand Down
24 changes: 17 additions & 7 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3164,7 +3164,7 @@ macro_rules! handle_monitor_update_completion {
&mut $peer_state.pending_msg_events, $chan, updates.raa,
updates.commitment_update, updates.order, updates.accepted_htlcs, updates.pending_update_adds,
updates.funding_broadcastable, updates.channel_ready,
updates.announcement_sigs);
updates.announcement_sigs, updates.tx_signatures);
if let Some(upd) = channel_update {
$peer_state.pending_msg_events.push(upd);
}
Expand Down Expand Up @@ -7445,17 +7445,20 @@ where
commitment_update: Option<msgs::CommitmentUpdate>, order: RAACommitmentOrder,
pending_forwards: Vec<(PendingHTLCInfo, u64)>, pending_update_adds: Vec<msgs::UpdateAddHTLC>,
funding_broadcastable: Option<Transaction>,
channel_ready: Option<msgs::ChannelReady>, announcement_sigs: Option<msgs::AnnouncementSignatures>)
-> (Option<(u64, Option<PublicKey>, OutPoint, ChannelId, u128, Vec<(PendingHTLCInfo, u64)>)>, Option<(u64, Vec<msgs::UpdateAddHTLC>)>) {
channel_ready: Option<msgs::ChannelReady>, announcement_sigs: Option<msgs::AnnouncementSignatures>,
tx_signatures: Option<msgs::TxSignatures>
) -> (Option<(u64, Option<PublicKey>, OutPoint, ChannelId, u128, Vec<(PendingHTLCInfo, u64)>)>, Option<(u64, Vec<msgs::UpdateAddHTLC>)>) {
let logger = WithChannelContext::from(&self.logger, &channel.context, None);
log_trace!(logger, "Handling channel resumption for channel {} with {} RAA, {} commitment update, {} pending forwards, {} pending update_add_htlcs, {}broadcasting funding, {} channel ready, {} announcement",
log_trace!(logger, "Handling channel resumption for channel {} with {} RAA, {} commitment update, {} pending forwards, {} pending update_add_htlcs, {}broadcasting funding, {} channel ready, {} announcement, {} tx_signatures",
&channel.context.channel_id(),
if raa.is_some() { "an" } else { "no" },
if commitment_update.is_some() { "a" } else { "no" },
pending_forwards.len(), pending_update_adds.len(),
if funding_broadcastable.is_some() { "" } else { "not " },
if channel_ready.is_some() { "sending" } else { "without" },
if announcement_sigs.is_some() { "sending" } else { "without" });
if announcement_sigs.is_some() { "sending" } else { "without" },
if tx_signatures.is_some() { "sending" } else { "without" },
);

let counterparty_node_id = channel.context.get_counterparty_node_id();
let short_channel_id = channel.context.get_short_channel_id().unwrap_or(channel.context.outbound_scid_alias());
Expand All @@ -7482,6 +7485,12 @@ where
msg,
});
}
if let Some(msg) = tx_signatures {
pending_msg_events.push(events::MessageSendEvent::SendTxSignatures {
node_id: counterparty_node_id,
msg,
});
}

macro_rules! handle_cs { () => {
if let Some(update) = commitment_update {
Expand Down Expand Up @@ -8349,7 +8358,8 @@ where
let channel_phase = chan_phase_entry.get_mut();
match channel_phase {
ChannelPhase::Funded(chan) => {
let (tx_signatures_opt, funding_tx_opt) = try_chan_phase_entry!(self, peer_state, chan.tx_signatures(msg), chan_phase_entry);
let logger = WithChannelContext::from(&self.logger, &chan.context, None);
let (tx_signatures_opt, funding_tx_opt) = try_chan_phase_entry!(self, peer_state, chan.tx_signatures(msg, &&logger), chan_phase_entry);
if let Some(tx_signatures) = tx_signatures_opt {
peer_state.pending_msg_events.push(events::MessageSendEvent::SendTxSignatures {
node_id: *counterparty_node_id,
Expand Down Expand Up @@ -9222,7 +9232,7 @@ where
let need_lnd_workaround = chan.context.workaround_lnd_bug_4006.take();
let (htlc_forwards, decode_update_add_htlcs) = self.handle_channel_resumption(
&mut peer_state.pending_msg_events, chan, responses.raa, responses.commitment_update, responses.order,
Vec::new(), Vec::new(), None, responses.channel_ready, responses.announcement_sigs);
Vec::new(), Vec::new(), None, responses.channel_ready, responses.announcement_sigs, None);
debug_assert!(htlc_forwards.is_none());
debug_assert!(decode_update_add_htlcs.is_none());
if let Some(upd) = channel_update {
Expand Down