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.

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

Jump to bottom

Add more information to OpenChannelRequest Event #3019

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add more information to OpenChannelRequest Event #3019

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions lightning/src/events/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1113,6 +1113,10 @@ pub enum Event {
///
/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
channel_type: ChannelTypeFeatures,
/// True if this channel is (or will be) publicly-announced.
is_public: bool,
/// Opening fields for the channel given by the counterparty.
open_fields: msgs::CommonOpenChannelFields,
Copy link
Contributor

Choose a reason for hiding this comment

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

Mh, if we want to do this, I wonder if we should drop the channel_type field as it's redundant.

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 don't think so, channel type is calculated through a function above, eaiser to give that to the user so they don't need to calculate themselves

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean if we want to do that we should just (finally) require channel type features and then its never negotiated but rather we always use what is in the message :).

That said, there's a lot of extra crap here...I'm fairly confident users don't care about the basepoints of the channel, the temporary_channel_id is redundant, and the chain hash is always matching (cause we check it in LDK). ISTM this is being included primarily for the numerical fields (dust limit, max in-flight value, htlc min, initial feerate, to-self delay and max HTLCs), which makes sense, but its a bit annoying to shove a lot of additional data in here.

Can we instead add like a ChannelParameters struct which contains just these? Can be a part of the wire message struct or a standalone one for the event.

},
/// Indicates that the HTLC was accepted, but could not be processed when or after attempting to
/// forward it.
Expand Down
3 changes: 3 additions & 0 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7169,12 +7169,15 @@ where
MsgHandleErrInternal::from_chan_no_close(e, msg.common_fields.temporary_channel_id)
)?;
let mut pending_events = self.pending_events.lock().unwrap();
let is_public = (msg.common_fields.channel_flags & 1) == 1;
pending_events.push_back((events::Event::OpenChannelRequest {
temporary_channel_id: msg.common_fields.temporary_channel_id.clone(),
counterparty_node_id: counterparty_node_id.clone(),
funding_satoshis: msg.common_fields.funding_satoshis,
push_msat: msg.push_msat,
channel_type,
is_public,
open_fields: msg.common_fields.clone(),
}, None));
peer_state.inbound_channel_request_by_id.insert(channel_id, InboundChannelRequest {
open_channel_msg: msg.clone(),
Expand Down