Skip to content

Support manual signaling of channel readiness #2109

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Support manual signaling of channel readiness #2109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 22 additions & 1 deletion lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,14 @@ pub(super) struct Channel<Signer: ChannelSigner> {
/// `accept_inbound_channel`, and `funding_created` should therefore not execute successfully.
inbound_awaiting_accept: bool,

/// A flag that indicates whether the channel requires the user to signal readiness to send
/// the `msgs::ChannelReady` message. This is only set to true if the channel was created with a
/// `ChannelHandshakeConfig::manually_signal_channel_ready` flag set to true.
///
/// When a user signals readiness via `ChannelManager::signal_channel_readiness` this flag is
/// flipped to false.
requires_manual_readiness_signal: bool,

/// The hash of the block in which the funding transaction was included.
funding_tx_confirmed_in: Option<BlockHash>,
funding_tx_confirmation_height: u32,
Expand Down Expand Up @@ -1049,6 +1057,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
target_closing_feerate_sats_per_kw: None,

inbound_awaiting_accept: false,
requires_manual_readiness_signal: config.channel_handshake_config.manually_signal_channel_ready,

funding_tx_confirmed_in: None,
funding_tx_confirmation_height: 0,
Expand Down Expand Up @@ -1393,6 +1402,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
target_closing_feerate_sats_per_kw: None,

inbound_awaiting_accept: true,
requires_manual_readiness_signal: config.channel_handshake_config.manually_signal_channel_ready,

funding_tx_confirmed_in: None,
funding_tx_confirmation_height: 0,
Expand Down Expand Up @@ -4968,10 +4978,11 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
self.channel_update_status = status;
}

fn check_get_channel_ready(&mut self, height: u32) -> Option<msgs::ChannelReady> {
pub fn check_get_channel_ready(&mut self, height: u32) -> Option<msgs::ChannelReady> {
// Called:
// * always when a new block/transactions are confirmed with the new height
// * when funding is signed with a height of 0
// * when user calls ChannelManager::signal_channel_readiness
if self.funding_tx_confirmation_height == 0 && self.minimum_depth != Some(0) {
return None;
}
Expand Down Expand Up @@ -5273,6 +5284,10 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
self.inbound_awaiting_accept
}

pub fn requires_manual_readiness_signal(&self) -> bool {
self.requires_manual_readiness_signal
}

/// Sets this channel to accepting 0conf, must be done before `get_accept_channel`
pub fn set_0conf(&mut self) {
assert!(self.inbound_awaiting_accept);
Expand Down Expand Up @@ -6426,6 +6441,8 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for Channel<Signer> {
// versions prior to 0.0.113, the u128 is serialized as two separate u64 values. Therefore,
// we write the high bytes as an option here.
let user_id_high_opt = Some((self.user_id >> 64) as u64);

let requires_manual_readiness_signal = Some(self.requires_manual_readiness_signal);

write_tlv_fields!(writer, {
(0, self.announcement_sigs, option),
Expand All @@ -6452,6 +6469,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for Channel<Signer> {
(23, channel_ready_event_emitted, option),
(25, user_id_high_opt, option),
(27, self.channel_keys_id, required),
(29, requires_manual_readiness_signal, option)
});

Ok(())
Expand Down Expand Up @@ -6723,6 +6741,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch

let mut user_id_high_opt: Option<u64> = None;
let mut channel_keys_id: Option<[u8; 32]> = None;
let mut requires_manual_readiness_signal: Option<bool> = Some(false);

read_tlv_fields!(reader, {
(0, announcement_sigs, option),
Expand All @@ -6743,6 +6762,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
(23, channel_ready_event_emitted, option),
(25, user_id_high_opt, option),
(27, channel_keys_id, option),
(29, requires_manual_readiness_signal, option),
});

let (channel_keys_id, holder_signer) = if let Some(channel_keys_id) = channel_keys_id {
Expand Down Expand Up @@ -6853,6 +6873,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
target_closing_feerate_sats_per_kw,

inbound_awaiting_accept: false,
requires_manual_readiness_signal: requires_manual_readiness_signal.unwrap(),

funding_tx_confirmed_in,
funding_tx_confirmation_height,
Expand Down
52 changes: 50 additions & 2 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1449,6 +1449,22 @@ macro_rules! remove_channel {
}
}

macro_rules! channel_ready_pending {
($self: ident, $pending_msg_events: expr, $channel: expr, $channel_ready_msg: expr) => {{
if $channel.requires_manual_readiness_signal() {
let mut pending_events = $self.pending_events.lock().unwrap();
pending_events.push(events::Event::PendingChannelReady {
Copy link
Contributor

Choose a reason for hiding this comment

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

The send_channel_ready macro doesn't seem like the proper place for this. At this point, we're ready to send out ChannelReady to our counterparty, and if we already received theirs, then we'll actually mark the channel as ready internally (see check_get_channel_ready) and even receive a ChannelReady event immediately after. This may result in a HTLC being routed through the channel, even though we haven't signaled it ready ourselves. If we want to continue using check_get_channel_ready for the manual signal, we'll need to make sure it only returns Some(ChannelReady) once signal_channel_readiness is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, I didn't look carefully enough at check_get_channel_ready and didn't realize it can update channel state.

So the options are to update check_get_channel_ready so that like you said, it only returns the msg if manual is enabled and user has already signaled. This implies adding state to a channel to track whether or not user has signaled already.

Is an alternative to update check_get_channel_ready so that it never returns Some(msg) if manual signaling is enabled and then write new logic in the signal_channel_readiness to make sure it's safe to send and then builds and sends the message?

The latter approach would avoid adding additional state and is the way I'd lean going if you think it's okay?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is an alternative to update check_get_channel_ready so that it never returns Some(msg) if manual signaling is enabled and then write new logic in the signal_channel_readiness to make sure it's safe to send and then builds and sends the message?

This wouldn't allow the ChannelReady message to be retransmitted to the counterparty upon a reconnection and ChannelReestablish, unless another PendingChannelReady event is queued for the consumer to signal. For your use case, it seems you only care about the first ChannelReady that gets sent, excluding any further ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm true, I was thinking it would just continue to fire Pending as needed, requiring the user to continually perform whatever checks they need before sending.

I guess if we wanted to support only one approval from user then there's no way around adding state to the Channel so it knows whether or not it's received approval already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we can solve both problems at once and just add a requires_manual_readiness_signal to Channel that gets set to the new config at creation and then when they manually signal we can flip it to false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm true, I was thinking it would just continue to fire Pending as needed, requiring the user to continually perform whatever checks they need before sending.

We could, but I don't immediately see the use case for it. We already do something similar with our ChannelReady event – we only queue it once.

I guess we can solve both problems at once and just add a requires_manual_readiness_signal to Channel that gets set to the new config at creation and then when they manually signal we can flip it to false.

Yeah that should work. Depending on how/when the event is queued, we may need another bit to make sure it's only queued once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just realized we also potentially send ChannelReady as part of channel reestablishment and monitor update workflows. Seems like everywhere we might return Some(msg::ChannelReady) we would need to make sure it's been signaled already.

The issue then becomes where do we actually queue the PendingChannelReady event. I guess the normal path is part of do_chain_event workflow? Though maybe the first time can also be triggered after funding_created and a node restart via monitor update and 0-conf channel?

I think I need some help figuring out best place to queue it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea, I think we shouldn't concern ourselves with monitors or peer connectivity for generating the event itself - we should generate the event in the 0conf-signed/block-connection path (which all ends up in check_get_channel_ready I think). In those cases, we will want to not set the OurChannelReady/move to the ChannelReady state until the user tells us we're ready to fly.

We'll want to avoid sending the actual ready message if there's a monitor update pending or the peer is disconnected.

We'll also want to generate a fresh event on restart if we see that we're ready to fly (via check_get_channel_ready) but we're blocked on the user. That may be as simple as...doing nothing (but storing a "we generated the event" flag in memory only), as presumably there will (eventually) be another block connected on restart and we'll generate the event then.

channel_id: $channel.channel_id(),
user_channel_id: $channel.get_user_id(),
counterparty_node_id: $channel.get_counterparty_node_id(),
funding_outpoint: $channel.get_funding_txo().unwrap(),
});
} else {
send_channel_ready!($self, $pending_msg_events, $channel, $channel_ready_msg);
}
}}
}

macro_rules! send_channel_ready {
($self: ident, $pending_msg_events: expr, $channel: expr, $channel_ready_msg: expr) => {{
$pending_msg_events.push(events::MessageSendEvent::SendChannelReady {
Expand Down Expand Up @@ -4175,7 +4191,7 @@ where
}

if let Some(msg) = channel_ready {
send_channel_ready!(self, pending_msg_events, channel, msg);
channel_ready_pending!(self, pending_msg_events, channel, msg);
}
if let Some(msg) = announcement_sigs {
pending_msg_events.push(events::MessageSendEvent::SendAnnouncementSignatures {
Expand Down Expand Up @@ -4354,6 +4370,38 @@ where
Ok(())
}

/// Signals channel readiness after a [`Event::PendingChannelReady`].
///
/// The `channel_id` parameter indicates which channel should signal readiness,
/// and the `counterparty_node_id` parameter is the id of the peer the channel is with.
///
/// [`Event::PendingChannelReady`]: events::Event::PendingChannelReady
pub fn signal_channel_readiness(&self, channel_id: &[u8; 32], counterparty_node_id: &PublicKey) -> Result<(), APIError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a test for this.

let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);

let per_peer_state = self.per_peer_state.read().unwrap();
let peer_state_mutex = per_peer_state.get(counterparty_node_id)
.ok_or_else(|| APIError::ChannelUnavailable { err: format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id) })?;
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
let peer_state = &mut *peer_state_lock;
let pending_msg_events = &mut peer_state.pending_msg_events;
match peer_state.channel_by_id.entry(channel_id.clone()) {
hash_map::Entry::Occupied(mut channel) => {
let best_block_height = self.best_block.read().unwrap().height();
let channel = channel.get_mut();
match channel.check_get_channel_ready(best_block_height) {
Some(channel_ready) => send_channel_ready!(self, pending_msg_events, channel, channel_ready),
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 this just going to queue another PendingChannelReady event when manually_signal_channel_ready is set? We need to actually force it through this time.

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 whoops, my bad. Makes sense to pull the conditional logic out of the macro and then the fn on ChannelManager the user uses can just call the macro to send it.

None => return Err(APIError::APIMisuseError { err: "The channel isn't currently in a state where we can signal readiness.".to_owned() })
}
}
hash_map::Entry::Vacant(_) => {
return Err(APIError::ChannelUnavailable { err: format!("Channel with id {} not found for the passed counterparty node_id {}", log_bytes!(*channel_id), counterparty_node_id) });
}
}

Ok(())
}

/// Gets the number of peers which match the given filter and do not have any funded, outbound,
/// or 0-conf channels.
///
Expand Down Expand Up @@ -5953,7 +6001,7 @@ where
HTLCDestination::NextHopChannel { node_id: Some(channel.get_counterparty_node_id()), channel_id: channel.channel_id() }));
}
if let Some(channel_ready) = channel_ready_opt {
send_channel_ready!(self, pending_msg_events, channel, channel_ready);
channel_ready_pending!(self, pending_msg_events, channel, channel_ready);
if channel.is_usable() {
log_trace!(self.logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", log_bytes!(channel.channel_id()));
if let Ok(msg) = self.get_channel_update_for_unicast(channel) {
Expand Down
12 changes: 12 additions & 0 deletions lightning/src/util/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,17 @@ pub struct ChannelHandshakeConfig {
/// Maximum value: 1,000,000, any values larger than 1 Million will be treated as 1 Million (or 100%)
/// instead, although channel negotiations will fail in that case.
pub their_channel_reserve_proportional_millionths: u32,
/// If this is set to true, the user needs to manually signal readiness for an inbound channel.
///
/// When set to true, [`Event::PendingChannelReady`] will be triggered once LDK has seen sufficient
/// confirmations of the funding transaction. In that case, a [`msgs::ChannelReady`] message will not be
/// sent to the counterparty node unless the user explicitly chooses to signal readiness.
///
/// Default value: false.
///
/// [`Event::PendingChannelReady`]: crate::util::events::Event::PendingChannelReady
/// [`msgs::ChannelReady`]: crate::ln::msgs::ChannelReady
pub manually_signal_channel_ready: bool,
#[cfg(anchors)]
/// If set, we attempt to negotiate the `anchors_zero_fee_htlc_tx`option for outbound channels.
///
Expand Down Expand Up @@ -183,6 +194,7 @@ impl Default for ChannelHandshakeConfig {
announced_channel: false,
commit_upfront_shutdown_pubkey: true,
their_channel_reserve_proportional_millionths: 10_000,
manually_signal_channel_ready: false,
#[cfg(anchors)]
negotiate_anchors_zero_fee_htlc_tx: false,
}
Expand Down
76 changes: 76 additions & 0 deletions lightning/src/util/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
//! few other things.

use crate::chain::keysinterface::SpendableOutputDescriptor;
use crate::chain::transaction::OutPoint;
#[cfg(anchors)]
use crate::ln::chan_utils::{self, ChannelTransactionParameters, HTLCOutputInCommitment};
use crate::ln::channelmanager::{InterceptId, PaymentId};
Expand Down Expand Up @@ -817,6 +818,50 @@ pub enum Event {
/// transaction.
claim_from_onchain_tx: bool,
},
/// Indicates a channel has received sufficient confirmations and LDK is ready to send [`msgs::ChannelReady`].
///
/// To signal readiness, call [`ChannelManager::signal_channel_readiness`]. To reject the
/// request, call [`ChannelManager::force_close_without_broadcasting_txn`].
///
/// The event is only triggered when the [`UserConfig::manually_signal_channel_ready`]
/// config flag is set to true.
///
/// [`ChannelManager::signal_channel_readiness`]: crate::ln::channelmanager::ChannelManager::signal_channel_readiness
/// [`ChannelManager::force_close_without_broadcasting_txn`]: crate::ln::channelmanager::ChannelManager::force_close_without_broadcasting_txn
/// [`UserConfig::manually_signal_channel_ready`]: crate::util::config::UserConfig::manually_signal_channel_ready
/// [`msgs::ChannelReady`]: crate::ln::msgs::ChannelReady
PendingChannelReady {
Copy link
Contributor

Choose a reason for hiding this comment

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

SignalChannelReady or MarkChannelReady?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds like the event itself marks the channel ready? I kinda like pending, tbh.

/// The channel ID of the channel.
///
/// When responding to the request, the `channel_id` should be passed
/// back to the ChannelManager through [`ChannelManager::signal_channel_readiness`] to signal,
/// or through [`ChannelManager::force_close_without_broadcasting_txn`] to reject.
///
/// [`ChannelManager::signal_channel_readiness`]: crate::ln::channelmanager::ChannelManager::signal_channel_readiness
/// [`ChannelManager::force_close_without_broadcasting_txn`]: crate::ln::channelmanager::ChannelManager::force_close_without_broadcasting_txn
channel_id: [u8; 32],
/// The `user_channel_id` value passed in to [`ChannelManager::create_channel`] for outbound
/// channels, or to [`ChannelManager::accept_inbound_channel`] for inbound channels if
/// [`UserConfig::manually_accept_inbound_channels`] config flag is set to true. Otherwise
/// `user_channel_id` will be randomized for an inbound channel.
///
/// [`ChannelManager::create_channel`]: crate::ln::channelmanager::ChannelManager::create_channel
/// [`ChannelManager::accept_inbound_channel`]: crate::ln::channelmanager::ChannelManager::accept_inbound_channel
/// [`UserConfig::manually_accept_inbound_channels`]: crate::util::config::UserConfig::manually_accept_inbound_channels
user_channel_id: u128,
/// The node_id of the counterparty requesting to open the channel.
///
/// When responding to the request, the `counterparty_node_id` should be passed
/// back to the `ChannelManager` through [`ChannelManager::signal_channel_readiness`] to
/// signal readiness, or through [`ChannelManager::force_close_without_broadcasting_txn`] to reject the
/// request.
///
/// [`ChannelManager::signal_channel_readiness`]: crate::ln::channelmanager::ChannelManager::signal_channel_readiness
/// [`ChannelManager::force_close_without_broadcasting_txn`]: crate::ln::channelmanager::ChannelManager::force_close_without_broadcasting_txn
counterparty_node_id: PublicKey,
/// The outpoint that holds the channel funds on-chain.
funding_outpoint: OutPoint,
},
/// Used to indicate that a channel with the given `channel_id` is ready to
/// be used. This event is emitted either when the funding transaction has been confirmed
/// on-chain, or, in case of a 0conf channel, when both parties have confirmed the channel
Expand Down Expand Up @@ -1136,6 +1181,15 @@ impl Writeable for Event {
(6, channel_type, required),
});
},
&Event::PendingChannelReady { ref channel_id, ref user_channel_id, ref counterparty_node_id, ref funding_outpoint } => {
31u8.write(writer)?;
write_tlv_fields!(writer, {
(0, channel_id, required),
(2, user_channel_id, required),
(4, counterparty_node_id, required),
(6, funding_outpoint, required)
});
},
// Note that, going forward, all new events must only write data inside of
// `write_tlv_fields`. Versions 0.0.101+ will ignore odd-numbered events that write
// data via `write_tlv_fields`.
Expand Down Expand Up @@ -1471,6 +1525,28 @@ impl MaybeReadable for Event {
};
f()
},
31u8 => {
let f = || {
let mut channel_id = [0; 32];
let mut user_channel_id: u128 = 0;
let mut counterparty_node_id = RequiredWrapper(None);
let mut funding_outpoint = RequiredWrapper(None);
read_tlv_fields!(reader, {
(0, channel_id, required),
(2, user_channel_id, required),
(4, counterparty_node_id, required),
(6, funding_outpoint, required),
});

Ok(Some(Event::PendingChannelReady {
channel_id,
user_channel_id,
counterparty_node_id: counterparty_node_id.0.unwrap(),
funding_outpoint: funding_outpoint.0.unwrap()
}))
};
f()
},
// Versions prior to 0.0.100 did not ignore odd types, instead returning InvalidValue.
// Version 0.0.100 failed to properly ignore odd types, possibly resulting in corrupt
// reads.
Expand Down