Skip to content

Commit

Permalink
Handle fallible commitment point when getting open_channel message
Browse files Browse the repository at this point in the history
  • Loading branch information
@alecchendev
alecchendev committed Sep 16, 2024
1 parent bcd5a07 commit c6d76c9
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 43 deletions.
82 changes: 50 additions & 32 deletions lightning/src/ln/channel.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7615,6 +7615,12 @@ impl<SP: Deref> Channel<SP> where
pub(super) struct OutboundV1Channel<SP: Deref> where SP::Target: SignerProvider {
pub context: ChannelContext<SP>,
pub unfunded_context: UnfundedChannelContext,
/// We tried to send a `open_channel` message but our commitment point wasn't ready.
/// This flag tells us we need to send it when we are retried once the
/// commiment point is ready.
///
/// TODO: don't need to persist this since we'll send open_channel again on connect?
pub signer_pending_open_channel: bool,
}

impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
Expand Down Expand Up @@ -7663,7 +7669,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
holder_commitment_point: HolderCommitmentPoint::new(&context.holder_signer, &context.secp_ctx),
};

let chan = Self { context, unfunded_context };
let chan = Self { context, unfunded_context, signer_pending_open_channel: false };
Ok(chan)
}

Expand Down Expand Up @@ -7761,14 +7767,15 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
/// If we receive an error message, it may only be a rejection of the channel type we tried,
/// not of our ability to open any channel at all. Thus, on error, we should first call this
/// and see if we get a new `OpenChannel` message, otherwise the channel is failed.
pub(crate) fn maybe_handle_error_without_close<F: Deref>(
&mut self, chain_hash: ChainHash, fee_estimator: &LowerBoundedFeeEstimator<F>
pub(crate) fn maybe_handle_error_without_close<F: Deref, L: Deref>(
&mut self, chain_hash: ChainHash, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
) -> Result<msgs::OpenChannel, ()>
where
F::Target: FeeEstimator
F::Target: FeeEstimator,
L::Target: Logger,
{
self.context.maybe_downgrade_channel_features(fee_estimator)?;
Ok(self.get_open_channel(chain_hash))
self.get_open_channel(chain_hash, logger).ok_or(())
}

/// Returns true if we can resume the channel by sending the [`msgs::OpenChannel`] again.
Expand All @@ -7777,7 +7784,9 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
self.unfunded_context.transaction_number() == INITIAL_COMMITMENT_NUMBER
}

pub fn get_open_channel(&self, chain_hash: ChainHash) -> msgs::OpenChannel {
pub fn get_open_channel<L: Deref>(&mut self, chain_hash: ChainHash, logger: &L) -> Option<msgs::OpenChannel>
where L::Target: Logger
{
if !self.context.is_outbound() {
panic!("Tried to open a channel for an inbound channel?");
}
Expand All @@ -7789,13 +7798,22 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
panic!("Tried to send an open_channel for a channel that has already advanced");
}

debug_assert!(self.unfunded_context.holder_commitment_point
.map(|point| point.is_available()).unwrap_or(false));
let first_per_commitment_point = self.unfunded_context.holder_commitment_point
.expect("TODO: Handle holder_commitment_point not being set").current_point();
let first_per_commitment_point = if let Some(holder_commitment_point) = self.unfunded_context.holder_commitment_point {
self.signer_pending_open_channel = false;
holder_commitment_point.current_point()
} else {
#[cfg(not(async_signing))] {
panic!("Failed getting commitment point for open_channel message");
}
#[cfg(async_signing)] {
log_trace!(logger, "Unable to generate open_channel message, waiting for commitment point");
self.signer_pending_open_channel = true;
return None;
}
};
let keys = self.context.get_holder_pubkeys();

msgs::OpenChannel {
Some(msgs::OpenChannel {
common_fields: msgs::CommonOpenChannelFields {
chain_hash,
temporary_channel_id: self.context.channel_id,
Expand All @@ -7821,7 +7839,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
},
push_msat: self.context.channel_value_satoshis * 1000 - self.context.value_to_self_msat,
channel_reserve_satoshis: self.context.holder_selected_channel_reserve_satoshis,
}
})
}

// Message handlers
Expand Down Expand Up @@ -9760,12 +9778,12 @@ mod tests {

let node_a_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
let config = UserConfig::default();
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();
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();

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

Expand All @@ -9791,7 +9809,7 @@ mod tests {

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

Expand Down Expand Up @@ -9923,7 +9941,7 @@ mod tests {
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();

// 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 open_channel_msg = node_a_chan.get_open_channel(chain_hash, &&logger).unwrap();
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
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();

Expand Down Expand Up @@ -9984,7 +10002,7 @@ mod tests {
// Test that `OutboundV1Channel::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_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();
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();
let chan_1_value_msat = chan_1.context.channel_value_satoshis * 1000;
assert_eq!(chan_1.context.holder_max_htlc_value_in_flight_msat, (chan_1_value_msat as f64 * 0.02) as u64);

Expand All @@ -9993,7 +10011,7 @@ mod tests {
let chan_2_value_msat = chan_2.context.channel_value_satoshis * 1000;
assert_eq!(chan_2.context.holder_max_htlc_value_in_flight_msat, (chan_2_value_msat as f64 * 0.99) as u64);

let chan_1_open_channel_msg = chan_1.get_open_channel(ChainHash::using_genesis_block(network));
let chan_1_open_channel_msg = chan_1.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();

// 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,
Expand Down Expand Up @@ -10069,12 +10087,12 @@ mod tests {

let mut outbound_node_config = UserConfig::default();
outbound_node_config.channel_handshake_config.their_channel_reserve_proportional_millionths = (outbound_selected_channel_reserve_perc * 1_000_000.0) as u32;
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();
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();

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);
assert_eq!(chan.context.holder_selected_channel_reserve_satoshis, expected_outbound_selected_chan_reserve);

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

Expand Down Expand Up @@ -10110,7 +10128,7 @@ mod tests {

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

Expand Down Expand Up @@ -10187,7 +10205,7 @@ mod tests {
).unwrap();
let inbound_chan = InboundV1Channel::<&TestKeysInterface>::new(
&feeest, &&keys_provider, &&keys_provider, node_b_node_id, &channelmanager::provided_channel_type_features(&config),
&features, &outbound_chan.get_open_channel(ChainHash::using_genesis_block(network)), 7, &config, 0, &&logger, false
&features, &outbound_chan.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap(), 7, &config, 0, &&logger, false
).unwrap();
outbound_chan.accept_channel(&inbound_chan.get_accept_channel_message(), &config.channel_handshake_limits, &features).unwrap();
let tx = Transaction { version: Version::ONE, lock_time: LockTime::ZERO, input: Vec::new(), output: vec![TxOut {
Expand Down Expand Up @@ -11085,13 +11103,13 @@ mod tests {

let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
let config = UserConfig::default();
let node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider,
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();

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

let mut open_channel_msg = node_a_chan.get_open_channel(ChainHash::using_genesis_block(network));
let mut open_channel_msg = node_a_chan.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
open_channel_msg.common_fields.channel_type = Some(channel_type_features);
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
let res = InboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider,
Expand Down Expand Up @@ -11129,13 +11147,13 @@ mod tests {
expected_channel_type.set_static_remote_key_required();
expected_channel_type.set_anchors_zero_fee_htlc_tx_required();

let channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
let mut channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
&fee_estimator, &&keys_provider, &&keys_provider, node_id_b,
&channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42,
None, &logger
).unwrap();

let open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network));
let open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
let channel_b = InboundV1Channel::<&TestKeysInterface>::new(
&fee_estimator, &&keys_provider, &&keys_provider, node_id_a,
&channelmanager::provided_channel_type_features(&config), &channelmanager::provided_init_features(&config),
Expand Down Expand Up @@ -11167,14 +11185,14 @@ mod tests {
let raw_init_features = static_remote_key_required | simple_anchors_required;
let init_features_with_simple_anchors = InitFeatures::from_le_bytes(raw_init_features.to_le_bytes().to_vec());

let channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
let mut channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
&fee_estimator, &&keys_provider, &&keys_provider, node_id_b,
&channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42,
None, &logger
).unwrap();

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

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

let mut open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network));
let mut open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
open_channel_msg.common_fields.channel_type = Some(simple_anchors_channel_type.clone());

let res = InboundV1Channel::<&TestKeysInterface>::new(
Expand All @@ -11239,7 +11257,7 @@ mod tests {
10000000, 100000, 42, &config, 0, 42, None, &logger
).unwrap();

let open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network));
let open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();

let channel_b = InboundV1Channel::<&TestKeysInterface>::new(
&fee_estimator, &&keys_provider, &&keys_provider, node_id_a,
Expand Down Expand Up @@ -11290,7 +11308,7 @@ mod tests {
&logger
).unwrap();

let open_channel_msg = node_a_chan.get_open_channel(ChainHash::using_genesis_block(network));
let open_channel_msg = node_a_chan.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
let mut node_b_chan = InboundV1Channel::<&TestKeysInterface>::new(
&feeest,
Expand Down
29 changes: 18 additions & 11 deletions lightning/src/ln/channelmanager.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3258,7 +3258,7 @@ where
}
}

let channel = {
let mut channel = {
let outbound_scid_alias = self.create_and_insert_outbound_scid_alias();
let their_features = &peer_state.latest_features;
let config = if override_config.is_some() { override_config.as_ref().unwrap() } else { &self.default_configuration };
Expand All @@ -3273,7 +3273,8 @@ where
},
}
};
let res = channel.get_open_channel(self.chain_hash);
let logger = WithChannelContext::from(&self.logger, &channel.context, None);
let res = channel.get_open_channel(self.chain_hash, &&logger);

let temporary_channel_id = channel.context.channel_id();
match peer_state.channel_by_id.entry(temporary_channel_id) {
Expand All @@ -3287,10 +3288,12 @@ where
hash_map::Entry::Vacant(entry) => { entry.insert(ChannelPhase::UnfundedOutboundV1(channel)); }
}

peer_state.pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
node_id: their_network_key,
msg: res,
});
if let Some(msg) = res {
peer_state.pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
node_id: their_network_key,
msg,
});
}
Ok(temporary_channel_id)
}

Expand Down Expand Up @@ -10736,10 +10739,13 @@ where
}

ChannelPhase::UnfundedOutboundV1(chan) => {
pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
node_id: chan.context.get_counterparty_node_id(),
msg: chan.get_open_channel(self.chain_hash),
});
let logger = WithChannelContext::from(&self.logger, &chan.context, None);
if let Some(msg) = chan.get_open_channel(self.chain_hash, &&logger) {
pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
node_id: chan.context.get_counterparty_node_id(),
msg,
});
}
}

// TODO(dual_funding): Combine this match arm with above once #[cfg(any(dual_funding, splicing))] is removed.
Expand Down Expand Up @@ -10854,7 +10860,8 @@ where
let peer_state = &mut *peer_state_lock;
match peer_state.channel_by_id.get_mut(&msg.channel_id) {
Some(ChannelPhase::UnfundedOutboundV1(ref mut chan)) => {
if let Ok(msg) = chan.maybe_handle_error_without_close(self.chain_hash, &self.fee_estimator) {
let logger = WithChannelContext::from(&self.logger, &chan.context, None);
if let Ok(msg) = chan.maybe_handle_error_without_close(self.chain_hash, &self.fee_estimator, &&logger) {
peer_state.pending_msg_events.push(events::MessageSendEvent::SendOpenChannel {
node_id: counterparty_node_id,
msg,
Expand Down

0 comments on commit c6d76c9

Please sign in to comment.