Skip to content

Ensure we wipe pending un-accepted channel requests on err/discon. #2501

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
tnull merged 3 commits into from
Aug 17, 2023
Merged

Ensure we wipe pending un-accepted channel requests on err/discon. #2501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e0255c0
Ensure we wipe pending un-accepted channel requests on err/discon.
TheBlueMatt Aug 15, 2023
1bf27bf
Drop now-unused `outbound_scid_alias` param to channel constructor
TheBlueMatt Aug 15, 2023
ef5a387
Update documentation on `Channel::set_outbound_scid_alias`
TheBlueMatt Aug 17, 2023
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
37 changes: 19 additions & 18 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1000,9 +1000,10 @@ impl<Signer: ChannelSigner> ChannelContext<Signer> {
}

/// Only allowed immediately after deserialization if get_outbound_scid_alias returns 0,
/// indicating we were written by LDK prior to 0.0.106 which did not set outbound SCID aliases.
/// indicating we were written by LDK prior to 0.0.106 which did not set outbound SCID aliases
/// or prior to any channel actions during `Channel` initialization.
pub fn set_outbound_scid_alias(&mut self, outbound_scid_alias: u64) {
assert_eq!(self.outbound_scid_alias, 0);
debug_assert_eq!(self.outbound_scid_alias, 0);
self.outbound_scid_alias = outbound_scid_alias;
}

Expand Down Expand Up @@ -6006,7 +6007,7 @@ impl<Signer: WriteableEcdsaChannelSigner> InboundV1Channel<Signer> {
fee_estimator: &LowerBoundedFeeEstimator<F>, entropy_source: &ES, signer_provider: &SP,
counterparty_node_id: PublicKey, our_supported_features: &ChannelTypeFeatures,
their_features: &InitFeatures, msg: &msgs::OpenChannel, user_id: u128, config: &UserConfig,
current_chain_height: u32, logger: &L, outbound_scid_alias: u64, is_0conf: bool,
current_chain_height: u32, logger: &L, is_0conf: bool,
) -> Result<InboundV1Channel<Signer>, ChannelError>
where ES::Target: EntropySource,
SP::Target: SignerProvider<Signer = Signer>,
Expand Down Expand Up @@ -6316,7 +6317,7 @@ impl<Signer: WriteableEcdsaChannelSigner> InboundV1Channel<Signer> {
sent_message_awaiting_response: None,

latest_inbound_scid_alias: None,
outbound_scid_alias,
outbound_scid_alias: 0,

channel_pending_event_emitted: false,
channel_ready_event_emitted: false,
Expand Down Expand Up @@ -7582,7 +7583,7 @@ mod tests {
// Make sure A's dust limit is as we expect.
let open_channel_msg = node_a_chan.get_open_channel(genesis_block(network).header.block_hash());
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
let mut node_b_chan = InboundV1Channel::<EnforcingSigner>::new(&feeest, &&keys_provider, &&keys_provider, node_b_node_id, &channelmanager::provided_channel_type_features(&config), &channelmanager::provided_init_features(&config), &open_channel_msg, 7, &config, 0, &&logger, 42, /*is_0conf=*/false).unwrap();
let mut node_b_chan = InboundV1Channel::<EnforcingSigner>::new(&feeest, &&keys_provider, &&keys_provider, node_b_node_id, &channelmanager::provided_channel_type_features(&config), &channelmanager::provided_init_features(&config), &open_channel_msg, 7, &config, 0, &&logger, /*is_0conf=*/false).unwrap();

// Node B --> Node A: accept channel, explicitly setting B's dust limit.
let mut accept_channel_msg = node_b_chan.accept_inbound_channel();
Expand Down Expand Up @@ -7711,7 +7712,7 @@ mod tests {
// Create Node B's channel by receiving Node A's open_channel message
let open_channel_msg = node_a_chan.get_open_channel(chain_hash);
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
let mut node_b_chan = InboundV1Channel::<EnforcingSigner>::new(&feeest, &&keys_provider, &&keys_provider, node_b_node_id, &channelmanager::provided_channel_type_features(&config), &channelmanager::provided_init_features(&config), &open_channel_msg, 7, &config, 0, &&logger, 42, /*is_0conf=*/false).unwrap();
let mut node_b_chan = InboundV1Channel::<EnforcingSigner>::new(&feeest, &&keys_provider, &&keys_provider, node_b_node_id, &channelmanager::provided_channel_type_features(&config), &channelmanager::provided_init_features(&config), &open_channel_msg, 7, &config, 0, &&logger, /*is_0conf=*/false).unwrap();

// Node B --> Node A: accept channel
let accept_channel_msg = node_b_chan.accept_inbound_channel();
Expand Down Expand Up @@ -7783,12 +7784,12 @@ mod tests {
// Test that `InboundV1Channel::new` creates a channel with the correct value for
// `holder_max_htlc_value_in_flight_msat`, when configured with a valid percentage value,
// which is set to the lower bound - 1 (2%) of the `channel_value`.
let chan_3 = InboundV1Channel::<EnforcingSigner>::new(&feeest, &&keys_provider, &&keys_provider, inbound_node_id, &channelmanager::provided_channel_type_features(&config_2_percent), &channelmanager::provided_init_features(&config_2_percent), &chan_1_open_channel_msg, 7, &config_2_percent, 0, &&logger, 42, /*is_0conf=*/false).unwrap();
let chan_3 = InboundV1Channel::<EnforcingSigner>::new(&feeest, &&keys_provider, &&keys_provider, inbound_node_id, &channelmanager::provided_channel_type_features(&config_2_percent), &channelmanager::provided_init_features(&config_2_percent), &chan_1_open_channel_msg, 7, &config_2_percent, 0, &&logger, /*is_0conf=*/false).unwrap();
let chan_3_value_msat = chan_3.context.channel_value_satoshis * 1000;
assert_eq!(chan_3.context.holder_max_htlc_value_in_flight_msat, (chan_3_value_msat as f64 * 0.02) as u64);

// Test with the upper bound - 1 of valid values (99%).
let chan_4 = InboundV1Channel::<EnforcingSigner>::new(&feeest, &&keys_provider, &&keys_provider, inbound_node_id, &channelmanager::provided_channel_type_features(&config_99_percent), &channelmanager::provided_init_features(&config_99_percent), &chan_1_open_channel_msg, 7, &config_99_percent, 0, &&logger, 42, /*is_0conf=*/false).unwrap();
let chan_4 = InboundV1Channel::<EnforcingSigner>::new(&feeest, &&keys_provider, &&keys_provider, inbound_node_id, &channelmanager::provided_channel_type_features(&config_99_percent), &channelmanager::provided_init_features(&config_99_percent), &chan_1_open_channel_msg, 7, &config_99_percent, 0, &&logger, /*is_0conf=*/false).unwrap();
let chan_4_value_msat = chan_4.context.channel_value_satoshis * 1000;
assert_eq!(chan_4.context.holder_max_htlc_value_in_flight_msat, (chan_4_value_msat as f64 * 0.99) as u64);

Expand All @@ -7807,14 +7808,14 @@ mod tests {

// Test that `InboundV1Channel::new` uses the lower bound of the configurable percentage values (1%)
// if `max_inbound_htlc_value_in_flight_percent_of_channel` is set to a value less than 1.
let chan_7 = InboundV1Channel::<EnforcingSigner>::new(&feeest, &&keys_provider, &&keys_provider, inbound_node_id, &channelmanager::provided_channel_type_features(&config_0_percent), &channelmanager::provided_init_features(&config_0_percent), &chan_1_open_channel_msg, 7, &config_0_percent, 0, &&logger, 42, /*is_0conf=*/false).unwrap();
let chan_7 = InboundV1Channel::<EnforcingSigner>::new(&feeest, &&keys_provider, &&keys_provider, inbound_node_id, &channelmanager::provided_channel_type_features(&config_0_percent), &channelmanager::provided_init_features(&config_0_percent), &chan_1_open_channel_msg, 7, &config_0_percent, 0, &&logger, /*is_0conf=*/false).unwrap();
let chan_7_value_msat = chan_7.context.channel_value_satoshis * 1000;
assert_eq!(chan_7.context.holder_max_htlc_value_in_flight_msat, (chan_7_value_msat as f64 * 0.01) as u64);

// Test that `InboundV1Channel::new` uses the upper bound of the configurable percentage values
// (100%) if `max_inbound_htlc_value_in_flight_percent_of_channel` is set to a larger value
// than 100.
let chan_8 = InboundV1Channel::<EnforcingSigner>::new(&feeest, &&keys_provider, &&keys_provider, inbound_node_id, &channelmanager::provided_channel_type_features(&config_101_percent), &channelmanager::provided_init_features(&config_101_percent), &chan_1_open_channel_msg, 7, &config_101_percent, 0, &&logger, 42, /*is_0conf=*/false).unwrap();
let chan_8 = InboundV1Channel::<EnforcingSigner>::new(&feeest, &&keys_provider, &&keys_provider, inbound_node_id, &channelmanager::provided_channel_type_features(&config_101_percent), &channelmanager::provided_init_features(&config_101_percent), &chan_1_open_channel_msg, 7, &config_101_percent, 0, &&logger, /*is_0conf=*/false).unwrap();
let chan_8_value_msat = chan_8.context.channel_value_satoshis * 1000;
assert_eq!(chan_8.context.holder_max_htlc_value_in_flight_msat, chan_8_value_msat);
}
Expand Down Expand Up @@ -7864,15 +7865,15 @@ mod tests {
inbound_node_config.channel_handshake_config.their_channel_reserve_proportional_millionths = (inbound_selected_channel_reserve_perc * 1_000_000.0) as u32;

if outbound_selected_channel_reserve_perc + inbound_selected_channel_reserve_perc < 1.0 {
let chan_inbound_node = InboundV1Channel::<EnforcingSigner>::new(&&fee_est, &&keys_provider, &&keys_provider, inbound_node_id, &channelmanager::provided_channel_type_features(&inbound_node_config), &channelmanager::provided_init_features(&outbound_node_config), &chan_open_channel_msg, 7, &inbound_node_config, 0, &&logger, 42, /*is_0conf=*/false).unwrap();
let chan_inbound_node = InboundV1Channel::<EnforcingSigner>::new(&&fee_est, &&keys_provider, &&keys_provider, inbound_node_id, &channelmanager::provided_channel_type_features(&inbound_node_config), &channelmanager::provided_init_features(&outbound_node_config), &chan_open_channel_msg, 7, &inbound_node_config, 0, &&logger, /*is_0conf=*/false).unwrap();

let expected_inbound_selected_chan_reserve = cmp::max(MIN_THEIR_CHAN_RESERVE_SATOSHIS, (chan.context.channel_value_satoshis as f64 * inbound_selected_channel_reserve_perc) as u64);

assert_eq!(chan_inbound_node.context.holder_selected_channel_reserve_satoshis, expected_inbound_selected_chan_reserve);
assert_eq!(chan_inbound_node.context.counterparty_selected_channel_reserve_satoshis.unwrap(), expected_outbound_selected_chan_reserve);
} else {
// Channel Negotiations failed
let result = InboundV1Channel::<EnforcingSigner>::new(&&fee_est, &&keys_provider, &&keys_provider, inbound_node_id, &channelmanager::provided_channel_type_features(&inbound_node_config), &channelmanager::provided_init_features(&outbound_node_config), &chan_open_channel_msg, 7, &inbound_node_config, 0, &&logger, 42, /*is_0conf=*/false);
let result = InboundV1Channel::<EnforcingSigner>::new(&&fee_est, &&keys_provider, &&keys_provider, inbound_node_id, &channelmanager::provided_channel_type_features(&inbound_node_config), &channelmanager::provided_init_features(&outbound_node_config), &chan_open_channel_msg, 7, &inbound_node_config, 0, &&logger, /*is_0conf=*/false);
assert!(result.is_err());
}
}
Expand All @@ -7897,7 +7898,7 @@ mod tests {
// Make sure A's dust limit is as we expect.
let open_channel_msg = node_a_chan.get_open_channel(genesis_block(network).header.block_hash());
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
let mut node_b_chan = InboundV1Channel::<EnforcingSigner>::new(&feeest, &&keys_provider, &&keys_provider, node_b_node_id, &channelmanager::provided_channel_type_features(&config), &channelmanager::provided_init_features(&config), &open_channel_msg, 7, &config, 0, &&logger, 42, /*is_0conf=*/false).unwrap();
let mut node_b_chan = InboundV1Channel::<EnforcingSigner>::new(&feeest, &&keys_provider, &&keys_provider, node_b_node_id, &channelmanager::provided_channel_type_features(&config), &channelmanager::provided_init_features(&config), &open_channel_msg, 7, &config, 0, &&logger, /*is_0conf=*/false).unwrap();

// Node B --> Node A: accept channel, explicitly setting B's dust limit.
let mut accept_channel_msg = node_b_chan.accept_inbound_channel();
Expand Down Expand Up @@ -8735,7 +8736,7 @@ mod tests {
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
let res = InboundV1Channel::<EnforcingSigner>::new(&feeest, &&keys_provider, &&keys_provider,
node_b_node_id, &channelmanager::provided_channel_type_features(&config),
&channelmanager::provided_init_features(&config), &open_channel_msg, 7, &config, 0, &&logger, 42, /*is_0conf=*/false);
&channelmanager::provided_init_features(&config), &open_channel_msg, 7, &config, 0, &&logger, /*is_0conf=*/false);
assert!(res.is_ok());
}

Expand Down Expand Up @@ -8777,7 +8778,7 @@ mod tests {
let channel_b = InboundV1Channel::<EnforcingSigner>::new(
&fee_estimator, &&keys_provider, &&keys_provider, node_id_a,
&channelmanager::provided_channel_type_features(&config), &channelmanager::provided_init_features(&config),
&open_channel_msg, 7, &config, 0, &&logger, 42, /*is_0conf=*/false
&open_channel_msg, 7, &config, 0, &&logger, /*is_0conf=*/false
).unwrap();

assert_eq!(channel_a.context.channel_type, expected_channel_type);
Expand Down Expand Up @@ -8819,7 +8820,7 @@ mod tests {
let channel_b = InboundV1Channel::<EnforcingSigner>::new(
&fee_estimator, &&keys_provider, &&keys_provider, node_id_a,
&channelmanager::provided_channel_type_features(&config), &init_features_with_simple_anchors,
&open_channel_msg, 7, &config, 0, &&logger, 42, /*is_0conf=*/false
&open_channel_msg, 7, &config, 0, &&logger, /*is_0conf=*/false
);
assert!(channel_b.is_err());
}
Expand Down Expand Up @@ -8862,7 +8863,7 @@ mod tests {
let res = InboundV1Channel::<EnforcingSigner>::new(
&fee_estimator, &&keys_provider, &&keys_provider, node_id_a,
&channelmanager::provided_channel_type_features(&config), &simple_anchors_init,
&open_channel_msg, 7, &config, 0, &&logger, 42, /*is_0conf=*/false
&open_channel_msg, 7, &config, 0, &&logger, /*is_0conf=*/false
);
assert!(res.is_err());

Expand All @@ -8880,7 +8881,7 @@ mod tests {
let channel_b = InboundV1Channel::<EnforcingSigner>::new(
&fee_estimator, &&keys_provider, &&keys_provider, node_id_a,
&channelmanager::provided_channel_type_features(&config), &channelmanager::provided_init_features(&config),
&open_channel_msg, 7, &config, 0, &&logger, 42, /*is_0conf=*/false
&open_channel_msg, 7, &config, 0, &&logger, /*is_0conf=*/false
).unwrap();

let mut accept_channel_msg = channel_b.get_accept_channel_message();
Expand Down
10 changes: 8 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 @@ -5332,7 +5332,7 @@ where
InboundV1Channel::new(&self.fee_estimator, &self.entropy_source, &self.signer_provider,
counterparty_node_id.clone(), &self.channel_type_features(), &peer_state.latest_features,
&unaccepted_channel.open_channel_msg, user_channel_id, &self.default_configuration, best_block_height,
&self.logger, /*outbound_scid_alias=*/0, accept_0conf).map_err(|e| APIError::ChannelUnavailable { err: e.to_string() })
&self.logger, accept_0conf).map_err(|e| APIError::ChannelUnavailable { err: e.to_string() })
}
_ => Err(APIError::APIMisuseError { err: "No such channel awaiting to be accepted.".to_owned() })
}?;
Expand Down Expand Up @@ -5495,7 +5495,7 @@ where
let user_channel_id = u128::from_be_bytes(random_bytes);
let mut channel = match InboundV1Channel::new(&self.fee_estimator, &self.entropy_source, &self.signer_provider,
counterparty_node_id.clone(), &self.channel_type_features(), &peer_state.latest_features, msg, user_channel_id,
&self.default_configuration, best_block_height, &self.logger, /*outbound_scid_alias=*/0, /*is_0conf=*/false)
&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));
Expand Down Expand Up @@ -7350,6 +7350,9 @@ where
self.issue_channel_close_events(&chan.context, ClosureReason::DisconnectedPeer);
false
});
// Note that we don't bother generating any events for pre-accept channels -
// they're not considered "channels" yet from the PoV of our events interface.
peer_state.inbound_channel_request_by_id.clear();
pending_msg_events.retain(|msg| {
match msg {
// V1 Channel Establishment
Expand Down Expand Up @@ -7493,6 +7496,9 @@ where
if peer_state_mutex_opt.is_none() { return; }
let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap();
let peer_state = &mut *peer_state_lock;
// Note that we don't bother generating any events for pre-accept channels -
// they're not considered "channels" yet from the PoV of our events interface.
peer_state.inbound_channel_request_by_id.clear();
peer_state.channel_by_id.keys().cloned()
.chain(peer_state.outbound_v1_channel_by_id.keys().cloned())
.chain(peer_state.inbound_v1_channel_by_id.keys().cloned()).collect()
Expand Down
4 changes: 2 additions & 2 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7013,7 +7013,7 @@ fn test_user_configurable_csv_delay() {
open_channel.to_self_delay = 200;
if let Err(error) = InboundV1Channel::new(&LowerBoundedFeeEstimator::new(&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }),
&nodes[0].keys_manager, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &nodes[0].node.channel_type_features(), &nodes[1].node.init_features(), &open_channel, 0,
&low_our_to_self_config, 0, &nodes[0].logger, 42, /*is_0conf=*/false)
&low_our_to_self_config, 0, &nodes[0].logger, /*is_0conf=*/false)
{
match error {
ChannelError::Close(err) => { assert!(regex::Regex::new(r"Configured with an unreasonable our_to_self_delay \(\d+\) putting user funds at risks").unwrap().is_match(err.as_str())); },
Expand Down Expand Up @@ -7045,7 +7045,7 @@ fn test_user_configurable_csv_delay() {
open_channel.to_self_delay = 200;
if let Err(error) = InboundV1Channel::new(&LowerBoundedFeeEstimator::new(&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }),
&nodes[0].keys_manager, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &nodes[0].node.channel_type_features(), &nodes[1].node.init_features(), &open_channel, 0,
&high_their_to_self_config, 0, &nodes[0].logger, 42, /*is_0conf=*/false)
&high_their_to_self_config, 0, &nodes[0].logger, /*is_0conf=*/false)
{
match error {
ChannelError::Close(err) => { assert!(regex::Regex::new(r"They wanted our payments to be delayed by a needlessly long period\. Upper limit: \d+\. Actual: \d+").unwrap().is_match(err.as_str())); },
Expand Down