Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
145 changes: 145 additions & 0 deletions lightning/src/ln/channel_open_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1750,6 +1750,151 @@ pub fn test_invalid_funding_tx() {
mine_transaction(&nodes[1], &spend_tx);
}

#[xtest(feature = "_externalize_tests")]
pub fn test_open_channel_request_channel_reserve_satoshis() {
// Test that the `channel_reserve_satoshis` field is correctly populated in the
// `OpenChannelRequest` event's `params` field for V1 channels.
let mut manually_accept_conf = UserConfig::default();
manually_accept_conf.manually_accept_inbound_channels = true;

let chanmon_cfgs = create_chanmon_cfgs(2);
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs =
create_node_chanmgrs(2, &node_cfgs, &[None, Some(manually_accept_conf.clone())]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);

let node_a_id = nodes[0].node.get_our_node_id();
let node_b_id = nodes[1].node.get_our_node_id();

// Create channel with 100,000 sats
nodes[0]
.node
.create_channel(node_b_id, 100_000, 10_001, 42, None, Some(manually_accept_conf))
.unwrap();
let open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, node_b_id);

// The channel_reserve_satoshis in the open_channel message is set by the opener
let expected_reserve = open_channel_msg.channel_reserve_satoshis;

nodes[1].node.handle_open_channel(node_a_id, &open_channel_msg);

// Verify the OpenChannelRequest event contains the correct channel_reserve_satoshis
let events = nodes[1].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match &events[0] {
Event::OpenChannelRequest { temporary_channel_id, params, .. } => {
// For V1 channels, channel_reserve_satoshis should be Some with the value from the message
assert_eq!(
params.channel_reserve_satoshis,
Some(expected_reserve),
Copy link
Contributor

Choose a reason for hiding this comment

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

As described elsewhere, let's also add coverage for the V2 case

Copy link
Author

Choose a reason for hiding this comment

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

Done! The new V2 test is added alongside the V1 test.

"channel_reserve_satoshis in OpenChannelRequest params should match the open_channel message"
);

// Verify other params fields are also correctly populated
assert_eq!(
params.dust_limit_satoshis,
open_channel_msg.common_fields.dust_limit_satoshis
);
assert_eq!(
params.max_htlc_value_in_flight_msat,
open_channel_msg.common_fields.max_htlc_value_in_flight_msat
);
assert_eq!(params.htlc_minimum_msat, open_channel_msg.common_fields.htlc_minimum_msat);
assert_eq!(params.to_self_delay, open_channel_msg.common_fields.to_self_delay);
assert_eq!(
params.max_accepted_htlcs,
open_channel_msg.common_fields.max_accepted_htlcs
);

// Accept the channel to clean up
nodes[1]
.node
.accept_inbound_channel(temporary_channel_id, &node_a_id, 0, None)
.unwrap();
},
_ => panic!("Expected OpenChannelRequest event"),
}

// Clear the SendAcceptChannel message event generated by accepting the channel
nodes[1].node.get_and_clear_pending_msg_events();
}

#[xtest(feature = "_externalize_tests")]
pub fn test_open_channel_request_channel_reserve_satoshis_v2() {
// Test that the `channel_reserve_satoshis` field is `None` in the
// `OpenChannelRequest` event's `params` field for V2 (dual-funded) channels.
let mut manually_accept_conf = UserConfig::default();
manually_accept_conf.manually_accept_inbound_channels = true;
manually_accept_conf.enable_dual_funded_channels = true;

let chanmon_cfgs = create_chanmon_cfgs(2);
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs =
create_node_chanmgrs(2, &node_cfgs, &[Some(manually_accept_conf.clone()), Some(manually_accept_conf.clone())]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);

let node_a_id = nodes[0].node.get_our_node_id();
let node_b_id = nodes[1].node.get_our_node_id();

// Get the open_channel message from node 0 to use as a template for the common fields
nodes[0]
.node
.create_channel(node_b_id, 100_000, 10_001, 42, None, Some(manually_accept_conf.clone()))
.unwrap();
let open_channel_v1_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, node_b_id);

// Create an OpenChannelV2 message using the common fields from V1
let open_channel_v2_msg = msgs::OpenChannelV2 {
common_fields: open_channel_v1_msg.common_fields.clone(),
funding_feerate_sat_per_1000_weight: 1000,
locktime: 0,
second_per_commitment_point: open_channel_v1_msg.common_fields.first_per_commitment_point,
require_confirmed_inputs: None,
};

nodes[1].node.handle_open_channel_v2(node_a_id, &open_channel_v2_msg);

// Verify the OpenChannelRequest event contains channel_reserve_satoshis = None for V2 channels
let events = nodes[1].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match &events[0] {
Event::OpenChannelRequest { temporary_channel_id, params, .. } => {
// For V2 channels, channel_reserve_satoshis should be None
assert_eq!(
params.channel_reserve_satoshis,
None,
"channel_reserve_satoshis in OpenChannelRequest params should be None for V2 channels"
);

// Verify other params fields are correctly populated
assert_eq!(
params.dust_limit_satoshis,
open_channel_v2_msg.common_fields.dust_limit_satoshis
);
assert_eq!(
params.max_htlc_value_in_flight_msat,
open_channel_v2_msg.common_fields.max_htlc_value_in_flight_msat
);
assert_eq!(params.htlc_minimum_msat, open_channel_v2_msg.common_fields.htlc_minimum_msat);
assert_eq!(params.to_self_delay, open_channel_v2_msg.common_fields.to_self_delay);
assert_eq!(
params.max_accepted_htlcs,
open_channel_v2_msg.common_fields.max_accepted_htlcs
);

// Accept the channel to clean up
nodes[1]
.node
.accept_inbound_channel(temporary_channel_id, &node_a_id, 0, None)
.unwrap();
},
_ => panic!("Expected OpenChannelRequest event"),
}

// Clear the SendAcceptChannelV2 message event generated by accepting the channel
nodes[1].node.get_and_clear_pending_msg_events();
}

#[xtest(feature = "_externalize_tests")]
pub fn test_coinbase_funding_tx() {
// Miners are able to fund channels directly from coinbase transactions, however
Expand Down
21 changes: 20 additions & 1 deletion lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1682,6 +1682,25 @@ pub(super) enum OpenChannelMessageRef<'a> {
V2(&'a msgs::OpenChannelV2),
}

impl<'a> OpenChannelMessageRef<'a> {
pub(super) fn channel_parameters(&self) -> msgs::ChannelParameters {
let (common_fields, channel_reserve_satoshis) = match self {
Self::V1(msg) => (&msg.common_fields, Some(msg.channel_reserve_satoshis)),
Self::V2(msg) => (&msg.common_fields, None),
Copy link
Contributor

Choose a reason for hiding this comment

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

The test suite still passes if I set this to Some(0), let's add coverage for the V2 case.

Copy link
Author

Choose a reason for hiding this comment

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

Done! Added test_open_channel_request_channel_reserve_satoshis_v2 which verifies that channel_reserve_satoshis is None for V2 (dual-funded) channels. This ensures the V2 code path is properly covered.

};
msgs::ChannelParameters {
dust_limit_satoshis: common_fields.dust_limit_satoshis,
max_htlc_value_in_flight_msat: common_fields.max_htlc_value_in_flight_msat,
htlc_minimum_msat: common_fields.htlc_minimum_msat,
commitment_feerate_sat_per_1000_weight: common_fields
.commitment_feerate_sat_per_1000_weight,
to_self_delay: common_fields.to_self_delay,
max_accepted_htlcs: common_fields.max_accepted_htlcs,
channel_reserve_satoshis,
}
}
}

/// A not-yet-accepted inbound (from counterparty) channel. Once
/// accepted, the parameters will be used to construct a channel.
pub(super) struct InboundChannelRequest {
Expand Down Expand Up @@ -10716,7 +10735,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
},
channel_type,
is_announced,
params: common_fields.channel_parameters(),
params: msg.channel_parameters(),
}, None));
peer_state.inbound_channel_request_by_id.insert(channel_id, InboundChannelRequest {
open_channel_msg: match msg {
Expand Down
27 changes: 13 additions & 14 deletions lightning/src/ln/msgs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,20 +244,6 @@ pub struct CommonOpenChannelFields {
pub channel_type: Option<ChannelTypeFeatures>,
}

impl CommonOpenChannelFields {
/// The [`ChannelParameters`] for this channel.
pub fn channel_parameters(&self) -> ChannelParameters {
ChannelParameters {
dust_limit_satoshis: self.dust_limit_satoshis,
max_htlc_value_in_flight_msat: self.max_htlc_value_in_flight_msat,
htlc_minimum_msat: self.htlc_minimum_msat,
commitment_feerate_sat_per_1000_weight: self.commitment_feerate_sat_per_1000_weight,
to_self_delay: self.to_self_delay,
max_accepted_htlcs: self.max_accepted_htlcs,
}
}
}

/// A subset of [`CommonOpenChannelFields`], containing various parameters which are set by the
/// channel initiator and which are not part of the channel funding transaction.
#[derive(Clone, Debug, Hash, PartialEq, Eq)]
Expand All @@ -277,6 +263,19 @@ pub struct ChannelParameters {
pub to_self_delay: u16,
/// The maximum number of pending HTLCs towards the channel initiator.
pub max_accepted_htlcs: u16,
/// The minimum value unencumbered by HTLCs for the non-channel-initiator to keep in the channel.
///
/// For V1 channels (`open_channel`), this is the explicit `channel_reserve_satoshis` value
/// from the channel initiator.
///
/// For V2 channels (`open_channel2`), this is `None` at the time of [`Event::OpenChannelRequest`]
/// because the channel reserve is calculated as `max(1% of total_channel_value, dust_limit_satoshis)`
/// per the spec, where `total_channel_value` includes both the initiator's and acceptor's funding
/// contributions. Since the acceptor's contribution is not yet known when the event is generated,
/// the final reserve value cannot be determined at that point.
///
/// [`Event::OpenChannelRequest`]: crate::events::Event::OpenChannelRequest
pub channel_reserve_satoshis: Option<u64>,
Copy link
Contributor

@tankyleo tankyleo Jan 27, 2026

Choose a reason for hiding this comment

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

A question for @tnull : OpenChannelV2 drops both the channel_reserve_satoshis, and push_msat fields. For the push_msats field, we currently handle this transition in the OpenChannelRequest event with an InboundChannelFunds enum, with PushMsat(u64) as the V1 variant, and DualFunded as the V2 variant.

We also note for InboundChannelFunds that it "allows the differentiation between a request for a dual-funded and non-dual-funded channel."

Seems to me now we can also differentiate V1 vs V2 based on the channel_reserve_satohis field.

To keep things consistent, would it be worth moving push_msats to ChannelParameters too, and set it to None in the V2 case, like we are doing here for channel_reserve_satoshis ?

Copy link
Author

Choose a reason for hiding this comment

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

This is a good point about consistency. I'll leave this design question for @tnull to weigh in on, as it would be a larger change to consolidate push_msats into ChannelParameters as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I kinda feel like we should include a value here for v2 that is the "reserve if you don't contribute" value and call it like likely_channel_reserve_satoshis? Otherwise it just feels weird that we have a value here that we expect to eventually be useless?

}

/// An [`open_channel`] message to be sent to or received from a peer.
Expand Down
Loading