Skip to content

Combine common fields for OpenChannel and AcceptChannel V1 & V2 #2871

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 2 commits into from
Feb 8, 2024
Merged

Combine common fields for OpenChannel and AcceptChannel V1 & V2 #2871

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3ddf15
Combine common fields of `OpenChannel` & `OpenChannelV2` into struct
dunxen Feb 2, 2024
0a7a5f9
Combine common fields of `AcceptChannel` & `AcceptChannelV2` into struct
dunxen Feb 2, 2024
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
2 changes: 1 addition & 1 deletion lightning/src/ln/async_signer_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ fn test_async_commitment_signature_for_funding_signed_0conf() {

// nodes[0] <-- accept_channel --- nodes[1]
let accept_channel = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id());
assert_eq!(accept_channel.minimum_depth, 0, "Expected minimum depth of 0");
assert_eq!(accept_channel.common_fields.minimum_depth, 0, "Expected minimum depth of 0");
nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), &accept_channel);

// nodes[0] --- funding_created --> nodes[1]
Expand Down
290 changes: 147 additions & 143 deletions lightning/src/ln/channel.rs
View file
Open in desktop

Large diffs are not rendered by default.

77 changes: 44 additions & 33 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6178,12 +6178,14 @@ where
fn internal_open_channel(&self, counterparty_node_id: &PublicKey, msg: &msgs::OpenChannel) -> Result<(), MsgHandleErrInternal> {
// Note that the ChannelManager is NOT re-persisted on disk after this, so any changes are
// likely to be lost on restart!
if msg.chain_hash != self.chain_hash {
return Err(MsgHandleErrInternal::send_err_msg_no_close("Unknown genesis block hash".to_owned(), msg.temporary_channel_id.clone()));
if msg.common_fields.chain_hash != self.chain_hash {
return Err(MsgHandleErrInternal::send_err_msg_no_close("Unknown genesis block hash".to_owned(),
msg.common_fields.temporary_channel_id.clone()));
}

if !self.default_configuration.accept_inbound_channels {
return Err(MsgHandleErrInternal::send_err_msg_no_close("No inbound channels accepted".to_owned(), msg.temporary_channel_id.clone()));
return Err(MsgHandleErrInternal::send_err_msg_no_close("No inbound channels accepted".to_owned(),
msg.common_fields.temporary_channel_id.clone()));
}

// Get the number of peers with channels, but without funded ones. We don't care too much
Expand All @@ -6196,7 +6198,9 @@ where
let peer_state_mutex = per_peer_state.get(counterparty_node_id)
.ok_or_else(|| {
debug_assert!(false);
MsgHandleErrInternal::send_err_msg_no_close(format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id), msg.temporary_channel_id.clone())
MsgHandleErrInternal::send_err_msg_no_close(
format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id),
msg.common_fields.temporary_channel_id.clone())
Comment on lines +6201 to +6203
Copy link

Choose a reason for hiding this comment

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

The error handling here has been updated to use msg.common_fields.temporary_channel_id, which is consistent with the refactor's goal. However, using format! for error messages is efficient only when necessary. Consider if static messages could be used instead to avoid runtime formatting costs in error paths that are expected to be rare.

- format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id),
+ "Can't find a peer matching the passed counterparty node_id".to_owned(),
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
MsgHandleErrInternal::send_err_msg_no_close(
format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id),
msg.common_fields.temporary_channel_id.clone())
MsgHandleErrInternal::send_err_msg_no_close(
"Can't find a peer matching the passed counterparty node_id".to_owned(),
msg.common_fields.temporary_channel_id.clone())

})?;
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
let peer_state = &mut *peer_state_lock;
Expand All @@ -6210,34 +6214,36 @@ where
{
return Err(MsgHandleErrInternal::send_err_msg_no_close(
"Have too many peers with unfunded channels, not accepting new ones".to_owned(),
msg.temporary_channel_id.clone()));
msg.common_fields.temporary_channel_id.clone()));
}

let best_block_height = self.best_block.read().unwrap().height();
if Self::unfunded_channel_count(peer_state, best_block_height) >= MAX_UNFUNDED_CHANS_PER_PEER {
return Err(MsgHandleErrInternal::send_err_msg_no_close(
format!("Refusing more than {} unfunded channels.", MAX_UNFUNDED_CHANS_PER_PEER),
msg.temporary_channel_id.clone()));
msg.common_fields.temporary_channel_id.clone()));
}

let channel_id = msg.temporary_channel_id;
let channel_id = msg.common_fields.temporary_channel_id;
let channel_exists = peer_state.has_channel(&channel_id);
if channel_exists {
return Err(MsgHandleErrInternal::send_err_msg_no_close("temporary_channel_id collision for the same peer!".to_owned(), msg.temporary_channel_id.clone()));
return Err(MsgHandleErrInternal::send_err_msg_no_close(
"temporary_channel_id collision for the same peer!".to_owned(),
msg.common_fields.temporary_channel_id.clone()));
}

// If we're doing manual acceptance checks on the channel, then defer creation until we're sure we want to accept.
if self.default_configuration.manually_accept_inbound_channels {
let channel_type = channel::channel_type_from_open_channel(
&msg, &peer_state.latest_features, &self.channel_type_features()
).map_err(|e|
MsgHandleErrInternal::from_chan_no_close(e, msg.temporary_channel_id)
MsgHandleErrInternal::from_chan_no_close(e, msg.common_fields.temporary_channel_id)
)?;
let mut pending_events = self.pending_events.lock().unwrap();
pending_events.push_back((events::Event::OpenChannelRequest {
temporary_channel_id: msg.temporary_channel_id.clone(),
temporary_channel_id: msg.common_fields.temporary_channel_id.clone(),
counterparty_node_id: counterparty_node_id.clone(),
funding_satoshis: msg.funding_satoshis,
funding_satoshis: msg.common_fields.funding_satoshis,
push_msat: msg.push_msat,
channel_type,
}, None));
Expand All @@ -6257,17 +6263,21 @@ where
&self.default_configuration, best_block_height, &self.logger, /*is_0conf=*/false)
{
Err(e) => {
return Err(MsgHandleErrInternal::from_chan_no_close(e, msg.temporary_channel_id));
return Err(MsgHandleErrInternal::from_chan_no_close(e, msg.common_fields.temporary_channel_id));
},
Ok(res) => res
};

let channel_type = channel.context.get_channel_type();
if channel_type.requires_zero_conf() {
return Err(MsgHandleErrInternal::send_err_msg_no_close("No zero confirmation channels accepted".to_owned(), msg.temporary_channel_id.clone()));
return Err(MsgHandleErrInternal::send_err_msg_no_close(
"No zero confirmation channels accepted".to_owned(),
msg.common_fields.temporary_channel_id.clone()));
}
if channel_type.requires_anchors_zero_fee_htlc_tx() {
return Err(MsgHandleErrInternal::send_err_msg_no_close("No channels with anchor outputs accepted".to_owned(), msg.temporary_channel_id.clone()));
return Err(MsgHandleErrInternal::send_err_msg_no_close(
"No channels with anchor outputs accepted".to_owned(),
msg.common_fields.temporary_channel_id.clone()));
}

let outbound_scid_alias = self.create_and_insert_outbound_scid_alias();
Expand All @@ -6289,28 +6299,28 @@ where
let peer_state_mutex = per_peer_state.get(counterparty_node_id)
.ok_or_else(|| {
debug_assert!(false);
MsgHandleErrInternal::send_err_msg_no_close(format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id), msg.temporary_channel_id)
MsgHandleErrInternal::send_err_msg_no_close(format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id), msg.common_fields.temporary_channel_id)
Copy link

Choose a reason for hiding this comment

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

The error handling here has been updated to use msg.common_fields.temporary_channel_id, which is consistent with the refactor's goal. However, consider if static messages could be used instead to avoid runtime formatting costs in error paths that are expected to be rare.

- format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id),
+ "Can't find a peer matching the passed counterparty node_id".to_owned(),
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
MsgHandleErrInternal::send_err_msg_no_close(format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id), msg.common_fields.temporary_channel_id)
MsgHandleErrInternal::send_err_msg_no_close("Can't find a peer matching the passed counterparty node_id".to_owned(), msg.common_fields.temporary_channel_id)

})?;
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
let peer_state = &mut *peer_state_lock;
match peer_state.channel_by_id.entry(msg.temporary_channel_id) {
match peer_state.channel_by_id.entry(msg.common_fields.temporary_channel_id) {
hash_map::Entry::Occupied(mut phase) => {
match phase.get_mut() {
ChannelPhase::UnfundedOutboundV1(chan) => {
try_chan_phase_entry!(self, chan.accept_channel(&msg, &self.default_configuration.channel_handshake_limits, &peer_state.latest_features), phase);
(chan.context.get_value_satoshis(), chan.context.get_funding_redeemscript().to_v0_p2wsh(), chan.context.get_user_id())
},
_ => {
return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got an unexpected accept_channel message from peer with counterparty_node_id {}", counterparty_node_id), msg.temporary_channel_id));
return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got an unexpected accept_channel message from peer with counterparty_node_id {}", counterparty_node_id), msg.common_fields.temporary_channel_id));
}
}
},
hash_map::Entry::Vacant(_) => return 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.temporary_channel_id))
hash_map::Entry::Vacant(_) => return 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.common_fields.temporary_channel_id))
}
};
let mut pending_events = self.pending_events.lock().unwrap();
pending_events.push_back((events::Event::FundingGenerationReady {
temporary_channel_id: msg.temporary_channel_id,
temporary_channel_id: msg.common_fields.temporary_channel_id,
counterparty_node_id: *counterparty_node_id,
channel_value_satoshis: value,
output_script,
Expand Down Expand Up @@ -8699,7 +8709,7 @@ where
fn handle_open_channel_v2(&self, counterparty_node_id: &PublicKey, msg: &msgs::OpenChannelV2) {
let _: Result<(), _> = handle_error!(self, Err(MsgHandleErrInternal::send_err_msg_no_close(
"Dual-funded channels not supported".to_owned(),
msg.temporary_channel_id.clone())), *counterparty_node_id);
msg.common_fields.temporary_channel_id.clone())), *counterparty_node_id);
}

fn handle_accept_channel(&self, counterparty_node_id: &PublicKey, msg: &msgs::AcceptChannel) {
Expand All @@ -8715,7 +8725,7 @@ where
fn handle_accept_channel_v2(&self, counterparty_node_id: &PublicKey, msg: &msgs::AcceptChannelV2) {
let _: Result<(), _> = handle_error!(self, Err(MsgHandleErrInternal::send_err_msg_no_close(
"Dual-funded channels not supported".to_owned(),
msg.temporary_channel_id.clone())), *counterparty_node_id);
msg.common_fields.temporary_channel_id.clone())), *counterparty_node_id);
}

fn handle_funding_created(&self, counterparty_node_id: &PublicKey, msg: &msgs::FundingCreated) {
Expand Down Expand Up @@ -11993,14 +12003,15 @@ mod tests {
check_added_monitors!(nodes[0], 1);
expect_channel_pending_event(&nodes[0], &nodes[1].node.get_our_node_id());
}
open_channel_msg.temporary_channel_id = ChannelId::temporary_from_entropy_source(&nodes[0].keys_manager);
open_channel_msg.common_fields.temporary_channel_id = ChannelId::temporary_from_entropy_source(&nodes[0].keys_manager);
}

// A MAX_UNFUNDED_CHANS_PER_PEER + 1 channel will be summarily rejected
open_channel_msg.temporary_channel_id = ChannelId::temporary_from_entropy_source(&nodes[0].keys_manager);
open_channel_msg.common_fields.temporary_channel_id = ChannelId::temporary_from_entropy_source(
&nodes[0].keys_manager);
nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel_msg);
assert_eq!(get_err_msg(&nodes[1], &nodes[0].node.get_our_node_id()).channel_id,
open_channel_msg.temporary_channel_id);
open_channel_msg.common_fields.temporary_channel_id);

// Further, because all of our channels with nodes[0] are inbound, and none of them funded,
// it doesn't count as a "protected" peer, i.e. it counts towards the MAX_NO_CHANNEL_PEERS
Expand Down Expand Up @@ -12048,11 +12059,11 @@ mod tests {
for i in 0..super::MAX_UNFUNDED_CHANNEL_PEERS - 1 {
nodes[1].node.handle_open_channel(&peer_pks[i], &open_channel_msg);
get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, peer_pks[i]);
open_channel_msg.temporary_channel_id = ChannelId::temporary_from_entropy_source(&nodes[0].keys_manager);
open_channel_msg.common_fields.temporary_channel_id = ChannelId::temporary_from_entropy_source(&nodes[0].keys_manager);
}
nodes[1].node.handle_open_channel(&last_random_pk, &open_channel_msg);
assert_eq!(get_err_msg(&nodes[1], &last_random_pk).channel_id,
open_channel_msg.temporary_channel_id);
open_channel_msg.common_fields.temporary_channel_id);

// Of course, however, outbound channels are always allowed
nodes[1].node.create_channel(last_random_pk, 100_000, 0, 42, None, None).unwrap();
Expand Down Expand Up @@ -12088,14 +12099,14 @@ mod tests {
for _ in 0..super::MAX_UNFUNDED_CHANS_PER_PEER {
nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel_msg);
get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id());
open_channel_msg.temporary_channel_id = ChannelId::temporary_from_entropy_source(&nodes[0].keys_manager);
open_channel_msg.common_fields.temporary_channel_id = ChannelId::temporary_from_entropy_source(&nodes[0].keys_manager);
}

// Once we have MAX_UNFUNDED_CHANS_PER_PEER unfunded channels, new inbound channels will be
// rejected.
nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel_msg);
assert_eq!(get_err_msg(&nodes[1], &nodes[0].node.get_our_node_id()).channel_id,
open_channel_msg.temporary_channel_id);
open_channel_msg.common_fields.temporary_channel_id);

// but we can still open an outbound channel.
nodes[1].node.create_channel(nodes[0].node.get_our_node_id(), 100_000, 0, 42, None, None).unwrap();
Expand All @@ -12104,7 +12115,7 @@ mod tests {
// but even with such an outbound channel, additional inbound channels will still fail.
nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel_msg);
assert_eq!(get_err_msg(&nodes[1], &nodes[0].node.get_our_node_id()).channel_id,
open_channel_msg.temporary_channel_id);
open_channel_msg.common_fields.temporary_channel_id);
}

#[test]
Expand Down Expand Up @@ -12140,7 +12151,7 @@ mod tests {
_ => panic!("Unexpected event"),
}
get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, random_pk);
open_channel_msg.temporary_channel_id = ChannelId::temporary_from_entropy_source(&nodes[0].keys_manager);
open_channel_msg.common_fields.temporary_channel_id = ChannelId::temporary_from_entropy_source(&nodes[0].keys_manager);
}

// If we try to accept a channel from another peer non-0conf it will fail.
Expand All @@ -12162,7 +12173,7 @@ mod tests {
_ => panic!("Unexpected event"),
}
assert_eq!(get_err_msg(&nodes[1], &last_random_pk).channel_id,
open_channel_msg.temporary_channel_id);
open_channel_msg.common_fields.temporary_channel_id);

// ...however if we accept the same channel 0conf it should work just fine.
nodes[1].node.handle_open_channel(&last_random_pk, &open_channel_msg);
Expand Down Expand Up @@ -12307,7 +12318,7 @@ mod tests {

nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, 0, 0, None, None).unwrap();
let open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
assert!(open_channel_msg.channel_type.as_ref().unwrap().supports_anchors_zero_fee_htlc_tx());
assert!(open_channel_msg.common_fields.channel_type.as_ref().unwrap().supports_anchors_zero_fee_htlc_tx());

nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel_msg);
let events = nodes[1].node.get_and_clear_pending_events();
Expand All @@ -12322,7 +12333,7 @@ mod tests {
nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &error_msg);

let open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
assert!(!open_channel_msg.channel_type.unwrap().supports_anchors_zero_fee_htlc_tx());
assert!(!open_channel_msg.common_fields.channel_type.unwrap().supports_anchors_zero_fee_htlc_tx());

// Since nodes[1] should not have accepted the channel, it should
// not have generated any events.
Expand Down
6 changes: 3 additions & 3 deletions lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1203,7 +1203,7 @@ pub fn open_zero_conf_channel<'a, 'b, 'c, 'd>(initiator: &'a Node<'b, 'c, 'd>, r
};

let accept_channel = get_event_msg!(receiver, MessageSendEvent::SendAcceptChannel, initiator.node.get_our_node_id());
assert_eq!(accept_channel.minimum_depth, 0);
assert_eq!(accept_channel.common_fields.minimum_depth, 0);
initiator.node.handle_accept_channel(&receiver.node.get_our_node_id(), &accept_channel);

let (temporary_channel_id, tx, _) = create_funding_transaction(&initiator, &receiver.node.get_our_node_id(), 100_000, 42);
Expand Down Expand Up @@ -1257,7 +1257,7 @@ pub fn open_zero_conf_channel<'a, 'b, 'c, 'd>(initiator: &'a Node<'b, 'c, 'd>, r
pub fn exchange_open_accept_chan<'a, 'b, 'c>(node_a: &Node<'a, 'b, 'c>, node_b: &Node<'a, 'b, 'c>, channel_value: u64, push_msat: u64) -> ChannelId {
let create_chan_id = node_a.node.create_channel(node_b.node.get_our_node_id(), channel_value, push_msat, 42, None, None).unwrap();
let open_channel_msg = get_event_msg!(node_a, MessageSendEvent::SendOpenChannel, node_b.node.get_our_node_id());
assert_eq!(open_channel_msg.temporary_channel_id, create_chan_id);
assert_eq!(open_channel_msg.common_fields.temporary_channel_id, create_chan_id);
assert_eq!(node_a.node.list_channels().iter().find(|channel| channel.channel_id == create_chan_id).unwrap().user_channel_id, 42);
node_b.node.handle_open_channel(&node_a.node.get_our_node_id(), &open_channel_msg);
if node_b.node.get_current_default_configuration().manually_accept_inbound_channels {
Expand All @@ -1270,7 +1270,7 @@ pub fn exchange_open_accept_chan<'a, 'b, 'c>(node_a: &Node<'a, 'b, 'c>, node_b:
};
}
let accept_channel_msg = get_event_msg!(node_b, MessageSendEvent::SendAcceptChannel, node_a.node.get_our_node_id());
assert_eq!(accept_channel_msg.temporary_channel_id, create_chan_id);
assert_eq!(accept_channel_msg.common_fields.temporary_channel_id, create_chan_id);
node_a.node.handle_accept_channel(&node_b.node.get_our_node_id(), &accept_channel_msg);
assert_ne!(node_b.node.list_channels().iter().find(|channel| channel.channel_id == create_chan_id).unwrap().user_channel_id, 0);

Expand Down
Loading