Skip to content

Add ChannelPending event emitted upon funding_signed #2098

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

Merged
merged 1 commit into from
Apr 3, 2023
Merged

Add ChannelPending event emitted upon funding_signed #2098

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
11 changes: 11 additions & 0 deletions fuzz/src/chanmon_consistency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,18 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
msg.clone()
} else { panic!("Wrong event type"); }
};
let events = $dest.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
if let events::Event::ChannelPending{ ref counterparty_node_id, .. } = events[0] {
assert_eq!(counterparty_node_id, &$source.get_our_node_id());
} else { panic!("Wrong event type"); }

$source.handle_funding_signed(&$dest.get_our_node_id(), &funding_signed);
let events = $source.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
if let events::Event::ChannelPending{ ref counterparty_node_id, .. } = events[0] {
assert_eq!(counterparty_node_id, &$dest.get_our_node_id());
} else { panic!("Wrong event type"); }

funding_output
} }
Expand Down
5 changes: 4 additions & 1 deletion lightning-background-processor/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,7 +710,7 @@ mod tests {
use lightning::chain::keysinterface::{InMemorySigner, KeysManager};
use lightning::chain::transaction::OutPoint;
use lightning::events::{Event, PathFailure, MessageSendEventsProvider, MessageSendEvent};
use lightning::get_event_msg;
use lightning::{get_event_msg, get_event};
use lightning::ln::PaymentHash;
use lightning::ln::channelmanager;
use lightning::ln::channelmanager::{BREAKDOWN_TIMEOUT, ChainParameters, MIN_CLTV_EXPIRY_DELTA, PaymentId};
Expand Down Expand Up @@ -1058,7 +1058,10 @@ mod tests {
($node_a: expr, $node_b: expr, $temporary_channel_id: expr, $tx: expr) => {{
$node_a.node.funding_transaction_generated(&$temporary_channel_id, &$node_b.node.get_our_node_id(), $tx.clone()).unwrap();
$node_b.node.handle_funding_created(&$node_a.node.get_our_node_id(), &get_event_msg!($node_a, MessageSendEvent::SendFundingCreated, $node_b.node.get_our_node_id()));
get_event!($node_b, Event::ChannelPending);

$node_a.node.handle_funding_signed(&$node_b.node.get_our_node_id(), &get_event_msg!($node_b, MessageSendEvent::SendFundingSigned, $node_a.node.get_our_node_id()));
get_event!($node_a, Event::ChannelPending);
}}
}

Expand Down
72 changes: 68 additions & 4 deletions lightning/src/events/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ use crate::util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReada
use crate::util::string::UntrustedString;
use crate::routing::router::{RouteHop, RouteParameters};

use bitcoin::{PackedLockTime, Transaction};
use bitcoin::{PackedLockTime, Transaction, OutPoint};
#[cfg(anchors)]
use bitcoin::{Txid, TxIn, TxOut, Witness};
use bitcoin::blockdata::script::Script;
use bitcoin::hashes::Hash;
use bitcoin::hashes::sha256::Hash as Sha256;
Expand Down Expand Up @@ -608,12 +610,39 @@ pub enum Event {
/// The caveat described above the `fee_earned_msat` field applies here as well.
outbound_amount_forwarded_msat: Option<u64>,
},
/// Used to indicate that a channel with the given `channel_id` is being opened and pending
/// confirmation on-chain.
///
/// This event is emitted when the funding transaction has been signed and is broadcast to the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we have a note that, in rare cases, this event may be missed if a channel is ultimately confirmed quickly prior to the event being processed and then we restart? Its a stupid rare case but its possible I think?

Copy link
Contributor Author

@tnull tnull Mar 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm confused, which scenario are you describing exactly? Wouldn't rather be an edge case that we emit the event twice when we decide to rebroadcast on restart?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can: (a) fund the channel, with an async monitor update, (b) not have that complete, but persist the manager, then (c) complete the monitor update and broadcast the funding tx (d) then restart, replay the chain and see the funding tx confirm, then (e) re-complete the monitor update completed in c, then we wont get the event again but the user will never have used it.

I think with "full" async monitor updates this becomes more realistic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a note that, in rare cases, this event may be missed if a channel is ultimately confirmed quickly prior to the event being processed and then we restart?

Since we plan to leverage it for #2137, seems we should always regenerate it in cases like this? The channel will actually be ready, but as long as it's queued before the ChannelReady event, it should be fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea, that's my biggest concern here.

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 think we can: (a) fund the channel, with an async monitor update, (b) not have that complete, but persist the manager, then (c) complete the monitor update and broadcast the funding tx (d) then restart, replay the chain and see the funding tx confirm, then (e) re-complete the monitor update completed in c, then we wont get the event again but the user will never have used it.

Ugh., I see. In that scenario it would persist the event queue when the event hasn't been emitted yet and wouldn't re-emit it after restart. Mh, instead of leaving a note I went ahead and am now tracking the emission of ChannelPending in the same way as ChannelReady and removed it from internal_funding_created entirely.
This means that we're now only emitting it after CMU completion or reestablish, i.e., would re-emit it if the "emitted" state wasn't persisted before.

/// network. For 0conf channels it will be immediately followed by the corresponding
/// [`Event::ChannelReady`] event.
ChannelPending {
/// The `channel_id` of the channel that is pending confirmation.
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 `temporary_channel_id` this channel used to be known by during channel establishment.
///
/// Will be `None` for channels created prior to LDK version 0.0.115.
former_temporary_channel_id: Option<[u8; 32]>,
/// The `node_id` of the channel counterparty.
counterparty_node_id: PublicKey,
/// The outpoint of the channel's funding transaction.
funding_txo: 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
/// establishment.
ChannelReady {
/// The channel_id of the channel that is ready.
/// The `channel_id` of the channel that is ready.
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
Expand All @@ -624,15 +653,15 @@ pub enum Event {
/// [`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 channel counterparty.
/// The `node_id` of the channel counterparty.
counterparty_node_id: PublicKey,
/// The features that this channel will operate with.
channel_type: ChannelTypeFeatures,
},
/// Used to indicate that a previously opened channel with the given `channel_id` is in the
/// process of closure.
ChannelClosed {
/// The channel_id of the channel which has been closed. Note that on-chain transactions
/// The `channel_id` of the channel which has been closed. Note that on-chain transactions
/// resolving the channel are likely still awaiting confirmation.
channel_id: [u8; 32],
/// The `user_channel_id` value passed in to [`ChannelManager::create_channel`] for outbound
Expand Down Expand Up @@ -931,6 +960,16 @@ impl Writeable for Event {
(6, channel_type, required),
});
},
&Event::ChannelPending { ref channel_id, ref user_channel_id, ref former_temporary_channel_id, ref counterparty_node_id, ref funding_txo } => {
31u8.write(writer)?;
write_tlv_fields!(writer, {
(0, channel_id, required),
(2, user_channel_id, required),
(4, former_temporary_channel_id, required),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: option is a bit more straightforward/saves a few bytes on the wire. It doesn't matter, though, just for future reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's a tiny bit more efficient but to be honest I don't liking mixing implementation type and serialization type semantics if it can be avoided. In my mind option -> field is optional, not field is Option.

(6, counterparty_node_id, required),
(8, funding_txo, 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 @@ -1271,6 +1310,31 @@ impl MaybeReadable for Event {
};
f()
},
31u8 => {
let f = || {
let mut channel_id = [0; 32];
let mut user_channel_id: u128 = 0;
let mut former_temporary_channel_id = None;
let mut counterparty_node_id = RequiredWrapper(None);
let mut funding_txo = RequiredWrapper(None);
read_tlv_fields!(reader, {
(0, channel_id, required),
(2, user_channel_id, required),
(4, former_temporary_channel_id, required),
(6, counterparty_node_id, required),
(8, funding_txo, required),
});

Ok(Some(Event::ChannelPending {
channel_id,
user_channel_id,
former_temporary_channel_id,
counterparty_node_id: counterparty_node_id.0.unwrap(),
funding_txo: funding_txo.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
6 changes: 6 additions & 0 deletions lightning/src/ln/chanmon_update_fail_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1864,6 +1864,7 @@ fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf:
let (outpoint, latest_update, _) = nodes[0].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
nodes[0].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, latest_update);
check_added_monitors!(nodes[0], 0);
expect_channel_pending_event(&nodes[0], &nodes[1].node.get_our_node_id());

let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 0);
Expand All @@ -1885,6 +1886,9 @@ fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf:
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
reconnect_nodes(&nodes[0], &nodes[1], (false, confirm_a_first), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));

// But we want to re-emit ChannelPending
expect_channel_pending_event(&nodes[1], &nodes[0].node.get_our_node_id());
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

Expand Down Expand Up @@ -2808,6 +2812,7 @@ fn do_test_outbound_reload_without_init_mon(use_0conf: bool) {
let funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg);
check_added_monitors!(nodes[1], 1);
expect_channel_pending_event(&nodes[1], &nodes[0].node.get_our_node_id());

let bs_signed_locked = nodes[1].node.get_and_clear_pending_msg_events();
assert_eq!(bs_signed_locked.len(), if use_0conf { 2 } else { 1 });
Expand Down Expand Up @@ -2906,6 +2911,7 @@ fn do_test_inbound_reload_without_init_mon(use_0conf: bool, lock_commitment: boo

nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &funding_signed_msg);
check_added_monitors!(nodes[0], 1);
expect_channel_pending_event(&nodes[0], &nodes[1].node.get_our_node_id());

let as_funding_tx = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
if lock_commitment {
Expand Down
43 changes: 42 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 @@ -499,6 +499,7 @@ pub(super) struct Channel<Signer: ChannelSigner> {
user_id: u128,

channel_id: [u8; 32],
temporary_channel_id: Option<[u8; 32]>, // Will be `None` for channels created prior to 0.0.115.
channel_state: u32,

// When we reach max(6 blocks, minimum_depth), we need to send an AnnouncementSigs message to
Expand Down Expand Up @@ -729,6 +730,9 @@ pub(super) struct Channel<Signer: ChannelSigner> {
// blinded paths instead of simple scid+node_id aliases.
outbound_scid_alias: u64,

// We track whether we already emitted a `ChannelPending` event.
channel_pending_event_emitted: bool,

// We track whether we already emitted a `ChannelReady` event.
channel_ready_event_emitted: bool,

Expand Down Expand Up @@ -991,6 +995,8 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
}
}

let temporary_channel_id = entropy_source.get_secure_random_bytes();

Ok(Channel {
user_id,

Expand All @@ -1004,7 +1010,8 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {

inbound_handshake_limits_override: Some(config.channel_handshake_limits.clone()),

channel_id: entropy_source.get_secure_random_bytes(),
channel_id: temporary_channel_id,
temporary_channel_id: Some(temporary_channel_id),
channel_state: ChannelState::OurInitSent as u32,
announcement_sigs_state: AnnouncementSigsState::NotSent,
secp_ctx,
Expand Down Expand Up @@ -1103,6 +1110,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
latest_inbound_scid_alias: None,
outbound_scid_alias,

channel_pending_event_emitted: false,
channel_ready_event_emitted: false,

#[cfg(any(test, fuzzing))]
Expand Down Expand Up @@ -1350,6 +1358,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
inbound_handshake_limits_override: None,

channel_id: msg.temporary_channel_id,
temporary_channel_id: Some(msg.temporary_channel_id),
channel_state: (ChannelState::OurInitSent as u32) | (ChannelState::TheirInitSent as u32),
announcement_sigs_state: AnnouncementSigsState::NotSent,
secp_ctx,
Expand Down Expand Up @@ -1451,6 +1460,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
latest_inbound_scid_alias: None,
outbound_scid_alias,

channel_pending_event_emitted: false,
channel_ready_event_emitted: false,

#[cfg(any(test, fuzzing))]
Expand Down Expand Up @@ -4550,6 +4560,13 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
self.channel_id
}

// Return the `temporary_channel_id` used during channel establishment.
//
// Will return `None` for channels created prior to LDK version 0.0.115.
pub fn temporary_channel_id(&self) -> Option<[u8; 32]> {
self.temporary_channel_id
}

pub fn minimum_depth(&self) -> Option<u32> {
self.minimum_depth
}
Expand Down Expand Up @@ -4694,6 +4711,21 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
self.prev_config.map(|prev_config| prev_config.0)
}

// Checks whether we should emit a `ChannelPending` event.
pub(crate) fn should_emit_channel_pending_event(&mut self) -> bool {
self.is_funding_initiated() && !self.channel_pending_event_emitted
}

// Returns whether we already emitted a `ChannelPending` event.
pub(crate) fn channel_pending_event_emitted(&self) -> bool {
self.channel_pending_event_emitted
}

// Remembers that we already emitted a `ChannelPending` event.
pub(crate) fn set_channel_pending_event_emitted(&mut self) {
self.channel_pending_event_emitted = true;
}

// Checks whether we should emit a `ChannelReady` event.
pub(crate) fn should_emit_channel_ready_event(&mut self) -> bool {
self.is_usable() && !self.channel_ready_event_emitted
Expand Down Expand Up @@ -6420,6 +6452,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for Channel<Signer> {
if self.holder_max_htlc_value_in_flight_msat != Self::get_holder_max_htlc_value_in_flight_msat(self.channel_value_satoshis, &old_max_in_flight_percent_config)
{ Some(self.holder_max_htlc_value_in_flight_msat) } else { None };

let channel_pending_event_emitted = Some(self.channel_pending_event_emitted);
let channel_ready_event_emitted = Some(self.channel_ready_event_emitted);

// `user_id` used to be a single u64 value. In order to remain backwards compatible with
Expand Down Expand Up @@ -6452,6 +6485,8 @@ 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, self.temporary_channel_id, option),
(31, channel_pending_event_emitted, option),
});

Ok(())
Expand Down Expand Up @@ -6719,10 +6754,12 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
let mut announcement_sigs_state = Some(AnnouncementSigsState::NotSent);
let mut latest_inbound_scid_alias = None;
let mut outbound_scid_alias = None;
let mut channel_pending_event_emitted = None;
let mut channel_ready_event_emitted = None;

let mut user_id_high_opt: Option<u64> = None;
let mut channel_keys_id: Option<[u8; 32]> = None;
let mut temporary_channel_id: Option<[u8; 32]> = None;

read_tlv_fields!(reader, {
(0, announcement_sigs, option),
Expand All @@ -6743,6 +6780,8 @@ 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, temporary_channel_id, option),
(31, channel_pending_event_emitted, option),
});

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

channel_id,
temporary_channel_id,
channel_state,
announcement_sigs_state: announcement_sigs_state.unwrap(),
secp_ctx,
Expand Down Expand Up @@ -6899,6 +6939,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
// Later in the ChannelManager deserialization phase we scan for channels and assign scid aliases if its missing
outbound_scid_alias: outbound_scid_alias.unwrap_or(0),

channel_pending_event_emitted: channel_pending_event_emitted.unwrap_or(true),
channel_ready_event_emitted: channel_ready_event_emitted.unwrap_or(true),

#[cfg(any(test, fuzzing))]
Expand Down
Loading