Skip to content

Commit f609fcf

Browse files
tnulltnull
authored
Merge pull request #2501 from TheBlueMatt/2023-08-err-pre-accept
Ensure we wipe pending un-accepted channel requests on err/discon.
2 parents 9de2a91 + ef5a387 commit f609fcf

File tree

3 files changed

+29
-22
lines changed

3 files changed

+29
-22
lines changed

‎lightning/src/ln/channel.rs‎

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1000,9 +1000,10 @@ impl<Signer: ChannelSigner> ChannelContext<Signer> {
10001000
}
10011001

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

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

63186319
latest_inbound_scid_alias: None,
6319-
outbound_scid_alias,
6320+
outbound_scid_alias: 0,
63206321

63216322
channel_pending_event_emitted: false,
63226323
channel_ready_event_emitted: false,
@@ -7582,7 +7583,7 @@ mod tests {
75827583
// Make sure A's dust limit is as we expect.
75837584
let open_channel_msg = node_a_chan.get_open_channel(genesis_block(network).header.block_hash());
75847585
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
7585-
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();
7586+
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();
75867587

75877588
// Node B --> Node A: accept channel, explicitly setting B's dust limit.
75887589
let mut accept_channel_msg = node_b_chan.accept_inbound_channel();
@@ -7711,7 +7712,7 @@ mod tests {
77117712
// Create Node B's channel by receiving Node A's open_channel message
77127713
let open_channel_msg = node_a_chan.get_open_channel(chain_hash);
77137714
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
7714-
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();
7715+
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();
77157716

77167717
// Node B --> Node A: accept channel
77177718
let accept_channel_msg = node_b_chan.accept_inbound_channel();
@@ -7783,12 +7784,12 @@ mod tests {
77837784
// Test that `InboundV1Channel::new` creates a channel with the correct value for
77847785
// `holder_max_htlc_value_in_flight_msat`, when configured with a valid percentage value,
77857786
// which is set to the lower bound - 1 (2%) of the `channel_value`.
7786-
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();
7787+
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();
77877788
let chan_3_value_msat = chan_3.context.channel_value_satoshis * 1000;
77887789
assert_eq!(chan_3.context.holder_max_htlc_value_in_flight_msat, (chan_3_value_msat as f64 * 0.02) as u64);
77897790

77907791
// Test with the upper bound - 1 of valid values (99%).
7791-
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();
7792+
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();
77927793
let chan_4_value_msat = chan_4.context.channel_value_satoshis * 1000;
77937794
assert_eq!(chan_4.context.holder_max_htlc_value_in_flight_msat, (chan_4_value_msat as f64 * 0.99) as u64);
77947795

@@ -7807,14 +7808,14 @@ mod tests {
78077808

78087809
// Test that `InboundV1Channel::new` uses the lower bound of the configurable percentage values (1%)
78097810
// if `max_inbound_htlc_value_in_flight_percent_of_channel` is set to a value less than 1.
7810-
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();
7811+
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();
78117812
let chan_7_value_msat = chan_7.context.channel_value_satoshis * 1000;
78127813
assert_eq!(chan_7.context.holder_max_htlc_value_in_flight_msat, (chan_7_value_msat as f64 * 0.01) as u64);
78137814

78147815
// Test that `InboundV1Channel::new` uses the upper bound of the configurable percentage values
78157816
// (100%) if `max_inbound_htlc_value_in_flight_percent_of_channel` is set to a larger value
78167817
// than 100.
7817-
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();
7818+
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();
78187819
let chan_8_value_msat = chan_8.context.channel_value_satoshis * 1000;
78197820
assert_eq!(chan_8.context.holder_max_htlc_value_in_flight_msat, chan_8_value_msat);
78207821
}
@@ -7864,15 +7865,15 @@ mod tests {
78647865
inbound_node_config.channel_handshake_config.their_channel_reserve_proportional_millionths = (inbound_selected_channel_reserve_perc * 1_000_000.0) as u32;
78657866

78667867
if outbound_selected_channel_reserve_perc + inbound_selected_channel_reserve_perc < 1.0 {
7867-
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();
7868+
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();
78687869

78697870
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);
78707871

78717872
assert_eq!(chan_inbound_node.context.holder_selected_channel_reserve_satoshis, expected_inbound_selected_chan_reserve);
78727873
assert_eq!(chan_inbound_node.context.counterparty_selected_channel_reserve_satoshis.unwrap(), expected_outbound_selected_chan_reserve);
78737874
} else {
78747875
// Channel Negotiations failed
7875-
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);
7876+
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);
78767877
assert!(result.is_err());
78777878
}
78787879
}
@@ -7897,7 +7898,7 @@ mod tests {
78977898
// Make sure A's dust limit is as we expect.
78987899
let open_channel_msg = node_a_chan.get_open_channel(genesis_block(network).header.block_hash());
78997900
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
7900-
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();
7901+
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();
79017902

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

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

87838784
assert_eq!(channel_a.context.channel_type, expected_channel_type);
@@ -8819,7 +8820,7 @@ mod tests {
88198820
let channel_b = InboundV1Channel::<EnforcingSigner>::new(
88208821
&fee_estimator, &&keys_provider, &&keys_provider, node_id_a,
88218822
&channelmanager::provided_channel_type_features(&config), &init_features_with_simple_anchors,
8822-
&open_channel_msg, 7, &config, 0, &&logger, 42, /*is_0conf=*/false
8823+
&open_channel_msg, 7, &config, 0, &&logger, /*is_0conf=*/false
88238824
);
88248825
assert!(channel_b.is_err());
88258826
}
@@ -8862,7 +8863,7 @@ mod tests {
88628863
let res = InboundV1Channel::<EnforcingSigner>::new(
88638864
&fee_estimator, &&keys_provider, &&keys_provider, node_id_a,
88648865
&channelmanager::provided_channel_type_features(&config), &simple_anchors_init,
8865-
&open_channel_msg, 7, &config, 0, &&logger, 42, /*is_0conf=*/false
8866+
&open_channel_msg, 7, &config, 0, &&logger, /*is_0conf=*/false
88668867
);
88678868
assert!(res.is_err());
88688869

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

88868887
let mut accept_channel_msg = channel_b.get_accept_channel_message();

‎lightning/src/ln/channelmanager.rs‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5332,7 +5332,7 @@ where
53325332
InboundV1Channel::new(&self.fee_estimator, &self.entropy_source, &self.signer_provider,
53335333
counterparty_node_id.clone(), &self.channel_type_features(), &peer_state.latest_features,
53345334
&unaccepted_channel.open_channel_msg, user_channel_id, &self.default_configuration, best_block_height,
5335-
&self.logger, /*outbound_scid_alias=*/0, accept_0conf).map_err(|e| APIError::ChannelUnavailable { err: e.to_string() })
5335+
&self.logger, accept_0conf).map_err(|e| APIError::ChannelUnavailable { err: e.to_string() })
53365336
}
53375337
_ => Err(APIError::APIMisuseError { err: "No such channel awaiting to be accepted.".to_owned() })
53385338
}?;
@@ -5495,7 +5495,7 @@ where
54955495
let user_channel_id = u128::from_be_bytes(random_bytes);
54965496
let mut channel = match InboundV1Channel::new(&self.fee_estimator, &self.entropy_source, &self.signer_provider,
54975497
counterparty_node_id.clone(), &self.channel_type_features(), &peer_state.latest_features, msg, user_channel_id,
5498-
&self.default_configuration, best_block_height, &self.logger, /*outbound_scid_alias=*/0, /*is_0conf=*/false)
5498+
&self.default_configuration, best_block_height, &self.logger, /*is_0conf=*/false)
54995499
{
55005500
Err(e) => {
55015501
return Err(MsgHandleErrInternal::from_chan_no_close(e, msg.temporary_channel_id));
@@ -7350,6 +7350,9 @@ where
73507350
self.issue_channel_close_events(&chan.context, ClosureReason::DisconnectedPeer);
73517351
false
73527352
});
7353+
// Note that we don't bother generating any events for pre-accept channels -
7354+
// they're not considered "channels" yet from the PoV of our events interface.
7355+
peer_state.inbound_channel_request_by_id.clear();
73537356
pending_msg_events.retain(|msg| {
73547357
match msg {
73557358
// V1 Channel Establishment
@@ -7493,6 +7496,9 @@ where
74937496
if peer_state_mutex_opt.is_none() { return; }
74947497
let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap();
74957498
let peer_state = &mut *peer_state_lock;
7499+
// Note that we don't bother generating any events for pre-accept channels -
7500+
// they're not considered "channels" yet from the PoV of our events interface.
7501+
peer_state.inbound_channel_request_by_id.clear();
74967502
peer_state.channel_by_id.keys().cloned()
74977503
.chain(peer_state.outbound_v1_channel_by_id.keys().cloned())
74987504
.chain(peer_state.inbound_v1_channel_by_id.keys().cloned()).collect()

‎lightning/src/ln/functional_tests.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7013,7 +7013,7 @@ fn test_user_configurable_csv_delay() {
70137013
open_channel.to_self_delay = 200;
70147014
if let Err(error) = InboundV1Channel::new(&LowerBoundedFeeEstimator::new(&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }),
70157015
&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,
7016-
&low_our_to_self_config, 0, &nodes[0].logger, 42, /*is_0conf=*/false)
7016+
&low_our_to_self_config, 0, &nodes[0].logger, /*is_0conf=*/false)
70177017
{
70187018
match error {
70197019
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())); },
@@ -7045,7 +7045,7 @@ fn test_user_configurable_csv_delay() {
70457045
open_channel.to_self_delay = 200;
70467046
if let Err(error) = InboundV1Channel::new(&LowerBoundedFeeEstimator::new(&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }),
70477047
&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,
7048-
&high_their_to_self_config, 0, &nodes[0].logger, 42, /*is_0conf=*/false)
7048+
&high_their_to_self_config, 0, &nodes[0].logger, /*is_0conf=*/false)
70497049
{
70507050
match error {
70517051
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())); },

0 commit comments

Comments
 (0)