Skip to content

Commit 0a01e41

Browse files
committed
Handle fallible commitment point when getting open_channel message
1 parent 8d6a021 commit 0a01e41

File tree

2 files changed

+59
-40
lines changed

2 files changed

+59
-40
lines changed

lightning/src/ln/channel.rs

Lines changed: 41 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -7425,14 +7425,15 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
74257425
/// If we receive an error message, it may only be a rejection of the channel type we tried,
74267426
/// not of our ability to open any channel at all. Thus, on error, we should first call this
74277427
/// and see if we get a new `OpenChannel` message, otherwise the channel is failed.
7428-
pub(crate) fn maybe_handle_error_without_close<F: Deref>(
7429-
&mut self, chain_hash: ChainHash, fee_estimator: &LowerBoundedFeeEstimator<F>
7428+
pub(crate) fn maybe_handle_error_without_close<F: Deref, L: Deref>(
7429+
&mut self, chain_hash: ChainHash, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
74307430
) -> Result<msgs::OpenChannel, ()>
74317431
where
7432-
F::Target: FeeEstimator
7432+
F::Target: FeeEstimator,
7433+
L::Target: Logger,
74337434
{
74347435
self.context.maybe_downgrade_channel_features(fee_estimator)?;
7435-
Ok(self.get_open_channel(chain_hash))
7436+
self.get_open_channel(chain_hash, logger).ok_or(())
74367437
}
74377438

74387439
/// Returns true if we can resume the channel by sending the [`msgs::OpenChannel`] again.
@@ -7441,7 +7442,9 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
74417442
self.context.holder_commitment_point.transaction_number() == INITIAL_COMMITMENT_NUMBER
74427443
}
74437444

7444-
pub fn get_open_channel(&self, chain_hash: ChainHash) -> msgs::OpenChannel {
7445+
pub fn get_open_channel<L: Deref>(&mut self, chain_hash: ChainHash, logger: &L) -> Option<msgs::OpenChannel>
7446+
where L::Target: Logger
7447+
{
74457448
if !self.context.is_outbound() {
74467449
panic!("Tried to open a channel for an inbound channel?");
74477450
}
@@ -7453,11 +7456,20 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
74537456
panic!("Tried to send an open_channel for a channel that has already advanced");
74547457
}
74557458

7456-
debug_assert!(self.context.holder_commitment_point.is_available());
7457-
let first_per_commitment_point = self.context.holder_commitment_point.current_point().expect("TODO");
7459+
// Note: another option here is to make commitment point a parameter of this function
7460+
// and make a helper method get_point_for_open_channel to check + set signer_pending_open_channel
7461+
// and call that right before anytime we call this function, so this function can remain
7462+
// side-effect free.
7463+
let first_per_commitment_point = if let Some(point) = self.context.holder_commitment_point.current_point() {
7464+
point
7465+
} else {
7466+
log_trace!(logger, "Unable to generate open_channel message, waiting for commitment point");
7467+
self.signer_pending_open_channel = true;
7468+
return None;
7469+
};
74587470
let keys = self.context.get_holder_pubkeys();
74597471

7460-
msgs::OpenChannel {
7472+
Some(msgs::OpenChannel {
74617473
common_fields: msgs::CommonOpenChannelFields {
74627474
chain_hash,
74637475
temporary_channel_id: self.context.channel_id,
@@ -7483,7 +7495,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
74837495
},
74847496
push_msat: self.context.channel_value_satoshis * 1000 - self.context.value_to_self_msat,
74857497
channel_reserve_satoshis: self.context.holder_selected_channel_reserve_satoshis,
7486-
}
7498+
})
74877499
}
74887500

74897501
// Message handlers
@@ -9494,12 +9506,12 @@ mod tests {
94949506

94959507
let node_a_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
94969508
let config = UserConfig::default();
9497-
let node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(&bounded_fee_estimator, &&keys_provider, &&keys_provider, node_a_node_id, &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42, None, &&logger).unwrap();
9509+
let mut node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(&bounded_fee_estimator, &&keys_provider, &&keys_provider, node_a_node_id, &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42, None, &&logger).unwrap();
94989510

94999511
// Now change the fee so we can check that the fee in the open_channel message is the
95009512
// same as the old fee.
95019513
fee_est.fee_est = 500;
9502-
let open_channel_msg = node_a_chan.get_open_channel(ChainHash::using_genesis_block(network));
9514+
let open_channel_msg = node_a_chan.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
95039515
assert_eq!(open_channel_msg.common_fields.commitment_feerate_sat_per_1000_weight, original_fee);
95049516
}
95059517

@@ -9525,7 +9537,7 @@ mod tests {
95259537

95269538
// Create Node B's channel by receiving Node A's open_channel message
95279539
// Make sure A's dust limit is as we expect.
9528-
let open_channel_msg = node_a_chan.get_open_channel(ChainHash::using_genesis_block(network));
9540+
let open_channel_msg = node_a_chan.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
95299541
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
95309542
let mut node_b_chan = InboundV1Channel::<&TestKeysInterface>::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();
95319543

@@ -9657,7 +9669,7 @@ mod tests {
96579669
let mut node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, node_b_node_id, &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42, None, &&logger).unwrap();
96589670

96599671
// Create Node B's channel by receiving Node A's open_channel message
9660-
let open_channel_msg = node_a_chan.get_open_channel(chain_hash);
9672+
let open_channel_msg = node_a_chan.get_open_channel(chain_hash, &&logger).unwrap();
96619673
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
96629674
let mut node_b_chan = InboundV1Channel::<&TestKeysInterface>::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();
96639675

@@ -9718,7 +9730,7 @@ mod tests {
97189730
// Test that `OutboundV1Channel::new` creates a channel with the correct value for
97199731
// `holder_max_htlc_value_in_flight_msat`, when configured with a valid percentage value,
97209732
// which is set to the lower bound + 1 (2%) of the `channel_value`.
9721-
let chan_1 = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, outbound_node_id, &channelmanager::provided_init_features(&config_2_percent), 10000000, 100000, 42, &config_2_percent, 0, 42, None, &&logger).unwrap();
9733+
let mut chan_1 = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, outbound_node_id, &channelmanager::provided_init_features(&config_2_percent), 10000000, 100000, 42, &config_2_percent, 0, 42, None, &&logger).unwrap();
97229734
let chan_1_value_msat = chan_1.context.channel_value_satoshis * 1000;
97239735
assert_eq!(chan_1.context.holder_max_htlc_value_in_flight_msat, (chan_1_value_msat as f64 * 0.02) as u64);
97249736

@@ -9727,7 +9739,7 @@ mod tests {
97279739
let chan_2_value_msat = chan_2.context.channel_value_satoshis * 1000;
97289740
assert_eq!(chan_2.context.holder_max_htlc_value_in_flight_msat, (chan_2_value_msat as f64 * 0.99) as u64);
97299741

9730-
let chan_1_open_channel_msg = chan_1.get_open_channel(ChainHash::using_genesis_block(network));
9742+
let chan_1_open_channel_msg = chan_1.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
97319743

97329744
// Test that `InboundV1Channel::new` creates a channel with the correct value for
97339745
// `holder_max_htlc_value_in_flight_msat`, when configured with a valid percentage value,
@@ -9803,12 +9815,12 @@ mod tests {
98039815

98049816
let mut outbound_node_config = UserConfig::default();
98059817
outbound_node_config.channel_handshake_config.their_channel_reserve_proportional_millionths = (outbound_selected_channel_reserve_perc * 1_000_000.0) as u32;
9806-
let chan = OutboundV1Channel::<&TestKeysInterface>::new(&&fee_est, &&keys_provider, &&keys_provider, outbound_node_id, &channelmanager::provided_init_features(&outbound_node_config), channel_value_satoshis, 100_000, 42, &outbound_node_config, 0, 42, None, &&logger).unwrap();
9818+
let mut chan = OutboundV1Channel::<&TestKeysInterface>::new(&&fee_est, &&keys_provider, &&keys_provider, outbound_node_id, &channelmanager::provided_init_features(&outbound_node_config), channel_value_satoshis, 100_000, 42, &outbound_node_config, 0, 42, None, &&logger).unwrap();
98079819

98089820
let expected_outbound_selected_chan_reserve = cmp::max(MIN_THEIR_CHAN_RESERVE_SATOSHIS, (chan.context.channel_value_satoshis as f64 * outbound_selected_channel_reserve_perc) as u64);
98099821
assert_eq!(chan.context.holder_selected_channel_reserve_satoshis, expected_outbound_selected_chan_reserve);
98109822

9811-
let chan_open_channel_msg = chan.get_open_channel(ChainHash::using_genesis_block(network));
9823+
let chan_open_channel_msg = chan.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
98129824
let mut inbound_node_config = UserConfig::default();
98139825
inbound_node_config.channel_handshake_config.their_channel_reserve_proportional_millionths = (inbound_selected_channel_reserve_perc * 1_000_000.0) as u32;
98149826

@@ -9844,7 +9856,7 @@ mod tests {
98449856

98459857
// Create Node B's channel by receiving Node A's open_channel message
98469858
// Make sure A's dust limit is as we expect.
9847-
let open_channel_msg = node_a_chan.get_open_channel(ChainHash::using_genesis_block(network));
9859+
let open_channel_msg = node_a_chan.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
98489860
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
98499861
let mut node_b_chan = InboundV1Channel::<&TestKeysInterface>::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();
98509862

@@ -9920,7 +9932,7 @@ mod tests {
99209932
).unwrap();
99219933
let inbound_chan = InboundV1Channel::<&TestKeysInterface>::new(
99229934
&feeest, &&keys_provider, &&keys_provider, node_b_node_id, &channelmanager::provided_channel_type_features(&config),
9923-
&features, &outbound_chan.get_open_channel(ChainHash::using_genesis_block(network)), 7, &config, 0, &&logger, false
9935+
&features, &outbound_chan.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap(), 7, &config, 0, &&logger, false
99249936
).unwrap();
99259937
outbound_chan.accept_channel(&inbound_chan.get_accept_channel_message(), &config.channel_handshake_limits, &features).unwrap();
99269938
let tx = Transaction { version: Version::ONE, lock_time: LockTime::ZERO, input: Vec::new(), output: vec![TxOut {
@@ -10816,13 +10828,13 @@ mod tests {
1081610828

1081710829
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
1081810830
let config = UserConfig::default();
10819-
let node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider,
10831+
let mut node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider,
1082010832
node_b_node_id, &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42, None, &&logger).unwrap();
1082110833

1082210834
let mut channel_type_features = ChannelTypeFeatures::only_static_remote_key();
1082310835
channel_type_features.set_zero_conf_required();
1082410836

10825-
let mut open_channel_msg = node_a_chan.get_open_channel(ChainHash::using_genesis_block(network));
10837+
let mut open_channel_msg = node_a_chan.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
1082610838
open_channel_msg.common_fields.channel_type = Some(channel_type_features);
1082710839
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
1082810840
let res = InboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider,
@@ -10860,13 +10872,13 @@ mod tests {
1086010872
expected_channel_type.set_static_remote_key_required();
1086110873
expected_channel_type.set_anchors_zero_fee_htlc_tx_required();
1086210874

10863-
let channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
10875+
let mut channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
1086410876
&fee_estimator, &&keys_provider, &&keys_provider, node_id_b,
1086510877
&channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42,
1086610878
None, &&logger
1086710879
).unwrap();
1086810880

10869-
let open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network));
10881+
let open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
1087010882
let channel_b = InboundV1Channel::<&TestKeysInterface>::new(
1087110883
&fee_estimator, &&keys_provider, &&keys_provider, node_id_a,
1087210884
&channelmanager::provided_channel_type_features(&config), &channelmanager::provided_init_features(&config),
@@ -10898,14 +10910,14 @@ mod tests {
1089810910
let raw_init_features = static_remote_key_required | simple_anchors_required;
1089910911
let init_features_with_simple_anchors = InitFeatures::from_le_bytes(raw_init_features.to_le_bytes().to_vec());
1090010912

10901-
let channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
10913+
let mut channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
1090210914
&fee_estimator, &&keys_provider, &&keys_provider, node_id_b,
1090310915
&channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42,
1090410916
None, &&logger
1090510917
).unwrap();
1090610918

1090710919
// Set `channel_type` to `None` to force the implicit feature negotiation.
10908-
let mut open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network));
10920+
let mut open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
1090910921
open_channel_msg.common_fields.channel_type = None;
1091010922

1091110923
// Since A supports both `static_remote_key` and `option_anchors`, but B only accepts
@@ -10945,13 +10957,13 @@ mod tests {
1094510957
// First, we'll try to open a channel between A and B where A requests a channel type for
1094610958
// the original `option_anchors` feature (non zero fee htlc tx). This should be rejected by
1094710959
// B as it's not supported by LDK.
10948-
let channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
10960+
let mut channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
1094910961
&fee_estimator, &&keys_provider, &&keys_provider, node_id_b,
1095010962
&channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42,
1095110963
None, &&logger
1095210964
).unwrap();
1095310965

10954-
let mut open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network));
10966+
let mut open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
1095510967
open_channel_msg.common_fields.channel_type = Some(simple_anchors_channel_type.clone());
1095610968

1095710969
let res = InboundV1Channel::<&TestKeysInterface>::new(
@@ -10970,7 +10982,7 @@ mod tests {
1097010982
10000000, 100000, 42, &config, 0, 42, None, &&logger
1097110983
).unwrap();
1097210984

10973-
let open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network));
10985+
let open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
1097410986

1097510987
let channel_b = InboundV1Channel::<&TestKeysInterface>::new(
1097610988
&fee_estimator, &&keys_provider, &&keys_provider, node_id_a,
@@ -11021,7 +11033,7 @@ mod tests {
1102111033
&&logger
1102211034
).unwrap();
1102311035

11024-
let open_channel_msg = node_a_chan.get_open_channel(ChainHash::using_genesis_block(network));
11036+
let open_channel_msg = node_a_chan.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
1102511037
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
1102611038
let mut node_b_chan = InboundV1Channel::<&TestKeysInterface>::new(
1102711039
&feeest,

lightning/src/ln/channelmanager.rs

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2962,7 +2962,7 @@ where
29622962
}
29632963
}
29642964

2965-
let channel = {
2965+
let mut channel = {
29662966
let outbound_scid_alias = self.create_and_insert_outbound_scid_alias();
29672967
let their_features = &peer_state.latest_features;
29682968
let config = if override_config.is_some() { override_config.as_ref().unwrap() } else { &self.default_configuration };
@@ -2977,7 +2977,8 @@ where
29772977
},
29782978
}
29792979
};
2980-
let res = channel.get_open_channel(self.chain_hash);
2980+
let logger = WithChannelContext::from(&self.logger, &channel.context, None);
2981+
let res = channel.get_open_channel(self.chain_hash, &&logger);
29812982

29822983
let temporary_channel_id = channel.context.channel_id();
29832984
match peer_state.channel_by_id.entry(temporary_channel_id) {
@@ -2991,10 +2992,12 @@ where
29912992
hash_map::Entry::Vacant(entry) => { entry.insert(ChannelPhase::UnfundedOutboundV1(channel)); }
29922993
}
29932994

2994-
peer_state.pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
2995-
node_id: their_network_key,
2996-
msg: res,
2997-
});
2995+
if let Some(msg) = res {
2996+
peer_state.pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
2997+
node_id: their_network_key,
2998+
msg,
2999+
});
3000+
}
29983001
Ok(temporary_channel_id)
29993002
}
30003003

@@ -9905,10 +9908,13 @@ where
99059908
}
99069909

99079910
ChannelPhase::UnfundedOutboundV1(chan) => {
9908-
pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
9909-
node_id: chan.context.get_counterparty_node_id(),
9910-
msg: chan.get_open_channel(self.chain_hash),
9911-
});
9911+
let logger = WithChannelContext::from(&self.logger, &chan.context, None);
9912+
if let Some(msg) = chan.get_open_channel(self.chain_hash, &&logger) {
9913+
pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
9914+
node_id: chan.context.get_counterparty_node_id(),
9915+
msg,
9916+
});
9917+
}
99129918
}
99139919

99149920
// TODO(dual_funding): Combine this match arm with above once #[cfg(any(dual_funding, splicing))] is removed.
@@ -10023,7 +10029,8 @@ where
1002310029
let peer_state = &mut *peer_state_lock;
1002410030
match peer_state.channel_by_id.get_mut(&msg.channel_id) {
1002510031
Some(ChannelPhase::UnfundedOutboundV1(ref mut chan)) => {
10026-
if let Ok(msg) = chan.maybe_handle_error_without_close(self.chain_hash, &self.fee_estimator) {
10032+
let logger = WithChannelContext::from(&self.logger, &chan.context, None);
10033+
if let Ok(msg) = chan.maybe_handle_error_without_close(self.chain_hash, &self.fee_estimator, &&logger) {
1002710034
peer_state.pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
1002810035
node_id: *counterparty_node_id,
1002910036
msg,

0 commit comments

Comments
 (0)