Skip to content

Commit cc3486c

Browse files
TheBlueMattTheBlueMatt
committed
Avoid passing FundingScope around during channel closure
`Channel`'s new `FundingScope` exists to store both the channel's live funding information as well as any in-flight splices. In order to keep the patches which introduced and began using it simple, it was exposed outside of `channel.rs` to `channelmanager.rs`, breaking the `Channel` abstraction somewhat. Here we remove one case of `FundingScope` being passed around `channelmanager.rs` (in the hopes of eventually keeping it entirely contained within `channel.rs`). Specifically, we remove the `FundingScope` parameter from `locked_close_channel`.
1 parent c049d88 commit cc3486c

File tree

2 files changed

+73
-36
lines changed

2 files changed

+73
-36
lines changed

‎lightning/src/ln/channel.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6043,6 +6043,16 @@ where
60436043
SP::Target: SignerProvider,
60446044
<SP::Target as SignerProvider>::EcdsaSigner: EcdsaChannelSigner,
60456045
{
6046+
pub fn context(&self) -> &ChannelContext<SP> {
6047+
&self.context
6048+
}
6049+
6050+
pub fn force_shutdown(
6051+
&mut self, closure_reason: ClosureReason, should_broadcast: bool,
6052+
) -> ShutdownResult {
6053+
self.context.force_shutdown(&self.funding, should_broadcast, closure_reason)
6054+
}
6055+
60466056
#[rustfmt::skip]
60476057
fn check_remote_fee<F: Deref, L: Deref>(
60486058
channel_type: &ChannelTypeFeatures, fee_estimator: &LowerBoundedFeeEstimator<F>,

‎lightning/src/ln/channelmanager.rs

Lines changed: 63 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -3169,22 +3169,33 @@ macro_rules! handle_error {
31693169
/// directly avoids duplicate error messages.
31703170
#[rustfmt::skip]
31713171
macro_rules! locked_close_channel {
3172-
($self: ident, $peer_state: expr, $channel_context: expr, $channel_funding: expr, $shutdown_res_mut: expr) => {{
3172+
($self: ident, $chan_context: expr, UNFUNDED) => {{
3173+
$self.short_to_chan_info.write().unwrap().remove(&$chan_context.outbound_scid_alias());
3174+
// If the channel was never confirmed on-chain prior to its closure, remove the
3175+
// outbound SCID alias we used for it from the collision-prevention set. While we
3176+
// generally want to avoid ever re-using an outbound SCID alias across all channels, we
3177+
// also don't want a counterparty to be able to trivially cause a memory leak by simply
3178+
// opening a million channels with us which are closed before we ever reach the funding
3179+
// stage.
3180+
let alias_removed = $self.outbound_scid_aliases.lock().unwrap().remove(&$chan_context.outbound_scid_alias());
3181+
debug_assert!(alias_removed);
3182+
}};
3183+
($self: ident, $peer_state: expr, $funded_chan: expr, $shutdown_res_mut: expr, FUNDED) => {{
31733184
if let Some((_, funding_txo, _, update)) = $shutdown_res_mut.monitor_update.take() {
31743185
handle_new_monitor_update!($self, funding_txo, update, $peer_state,
3175-
$channel_context, REMAIN_LOCKED_UPDATE_ACTIONS_PROCESSED_LATER);
3186+
$funded_chan.context, REMAIN_LOCKED_UPDATE_ACTIONS_PROCESSED_LATER);
31763187
}
31773188
// If there's a possibility that we need to generate further monitor updates for this
31783189
// channel, we need to store the last update_id of it. However, we don't want to insert
31793190
// into the map (which prevents the `PeerState` from being cleaned up) for channels that
31803191
// never even got confirmations (which would open us up to DoS attacks).
3181-
let update_id = $channel_context.get_latest_monitor_update_id();
3182-
if $channel_funding.get_funding_tx_confirmation_height().is_some() || $channel_context.minimum_depth($channel_funding) == Some(0) || update_id > 1 {
3183-
let chan_id = $channel_context.channel_id();
3192+
let update_id = $funded_chan.context.get_latest_monitor_update_id();
3193+
if $funded_chan.funding.get_funding_tx_confirmation_height().is_some() || $funded_chan.context.minimum_depth(&$funded_chan.funding) == Some(0) || update_id > 1 {
3194+
let chan_id = $funded_chan.context.channel_id();
31843195
$peer_state.closed_channel_monitor_update_ids.insert(chan_id, update_id);
31853196
}
31863197
let mut short_to_chan_info = $self.short_to_chan_info.write().unwrap();
3187-
if let Some(short_id) = $channel_funding.get_short_channel_id() {
3198+
if let Some(short_id) = $funded_chan.funding.get_short_channel_id() {
31883199
short_to_chan_info.remove(&short_id);
31893200
} else {
31903201
// If the channel was never confirmed on-chain prior to its closure, remove the
@@ -3193,11 +3204,11 @@ macro_rules! locked_close_channel {
31933204
// also don't want a counterparty to be able to trivially cause a memory leak by simply
31943205
// opening a million channels with us which are closed before we ever reach the funding
31953206
// stage.
3196-
let alias_removed = $self.outbound_scid_aliases.lock().unwrap().remove(&$channel_context.outbound_scid_alias());
3207+
let alias_removed = $self.outbound_scid_aliases.lock().unwrap().remove(&$funded_chan.context.outbound_scid_alias());
31973208
debug_assert!(alias_removed);
31983209
}
3199-
short_to_chan_info.remove(&$channel_context.outbound_scid_alias());
3200-
for scid in $channel_context.historical_scids() {
3210+
short_to_chan_info.remove(&$funded_chan.context.outbound_scid_alias());
3211+
for scid in $funded_chan.context.historical_scids() {
32013212
short_to_chan_info.remove(scid);
32023213
}
32033214
}}
@@ -3206,7 +3217,7 @@ macro_rules! locked_close_channel {
32063217
/// Returns (boolean indicating if we should remove the Channel object from memory, a mapped error)
32073218
#[rustfmt::skip]
32083219
macro_rules! convert_channel_err {
3209-
($self: ident, $peer_state: expr, $err: expr, $context: expr, $funding: expr, $channel_id: expr, MANUAL_CHANNEL_UPDATE, $channel_update: expr) => {
3220+
($self: ident, $peer_state: expr, $err: expr, $chan: expr, $close: expr, $locked_close: expr, $channel_id: expr, _internal) => {
32103221
match $err {
32113222
ChannelError::Warn(msg) => {
32123223
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(msg), *$channel_id))
@@ -3218,33 +3229,43 @@ macro_rules! convert_channel_err {
32183229
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore(msg), *$channel_id))
32193230
},
32203231
ChannelError::Close((msg, reason)) => {
3221-
let logger = WithChannelContext::from(&$self.logger, &$context, None);
3222-
log_error!(logger, "Closing channel {} due to close-required error: {}", $channel_id, msg);
3223-
let mut shutdown_res = $context.force_shutdown($funding, true, reason);
3224-
locked_close_channel!($self, $peer_state, $context, $funding, &mut shutdown_res);
3232+
let (mut shutdown_res, chan_update) = $close(reason);
3233+
let logger = WithChannelContext::from(&$self.logger, &$chan.context(), None);
3234+
log_error!(logger, "Closed channel {} due to close-required error: {}", $channel_id, msg);
3235+
$locked_close(&mut shutdown_res, $chan);
32253236
let err =
3226-
MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, $channel_update);
3237+
MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, chan_update);
32273238
(true, err)
32283239
},
32293240
ChannelError::SendError(msg) => {
32303241
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::SendError(msg), *$channel_id))
32313242
},
32323243
}
32333244
};
3234-
($self: ident, $peer_state: expr, $err: expr, $funded_channel: expr, $channel_id: expr, FUNDED_CHANNEL) => {
3235-
convert_channel_err!($self, $peer_state, $err, $funded_channel.context, &$funded_channel.funding, $channel_id, MANUAL_CHANNEL_UPDATE, { $self.get_channel_update_for_broadcast(&$funded_channel).ok() })
3236-
};
3237-
($self: ident, $peer_state: expr, $err: expr, $context: expr, $funding: expr, $channel_id: expr, UNFUNDED_CHANNEL) => {
3238-
convert_channel_err!($self, $peer_state, $err, $context, $funding, $channel_id, MANUAL_CHANNEL_UPDATE, None)
3239-
};
3245+
($self: ident, $peer_state: expr, $err: expr, $funded_channel: expr, $channel_id: expr, FUNDED_CHANNEL) => { {
3246+
let mut do_close = |reason| {
3247+
(
3248+
$funded_channel.force_shutdown(reason, true),
3249+
$self.get_channel_update_for_broadcast(&$funded_channel).ok(),
3250+
)
3251+
};
3252+
let mut locked_close = |shutdown_res_mut: &mut ShutdownResult, funded_channel: &mut FundedChannel<_>| {
3253+
locked_close_channel!($self, $peer_state, funded_channel, shutdown_res_mut, FUNDED);
3254+
};
3255+
convert_channel_err!($self, $peer_state, $err, $funded_channel, do_close, locked_close, $channel_id, _internal)
3256+
} };
3257+
($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr, UNFUNDED_CHANNEL) => { {
3258+
let mut do_close = |reason| { ($channel.force_shutdown(true, reason), None) };
3259+
let locked_close = |_, chan: &mut Channel<_>| { locked_close_channel!($self, chan.context(), UNFUNDED); };
3260+
convert_channel_err!($self, $peer_state, $err, $channel, do_close, locked_close, $channel_id, _internal)
3261+
} };
32403262
($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr) => {
32413263
match $channel.as_funded_mut() {
32423264
Some(funded_channel) => {
32433265
convert_channel_err!($self, $peer_state, $err, funded_channel, $channel_id, FUNDED_CHANNEL)
32443266
},
32453267
None => {
3246-
let (funding, context) = $channel.funding_and_context_mut();
3247-
convert_channel_err!($self, $peer_state, $err, context, funding, $channel_id, UNFUNDED_CHANNEL)
3268+
convert_channel_err!($self, $peer_state, $err, $channel, $channel_id, UNFUNDED_CHANNEL)
32483269
},
32493270
}
32503271
};
@@ -4116,7 +4137,7 @@ where
41164137
let reason = ClosureReason::LocallyCoopClosedUnfundedChannel;
41174138
let err = ChannelError::Close((reason.to_string(), reason));
41184139
let mut chan = chan_entry.remove();
4119-
let (_, mut e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
4140+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, chan_id);
41204141
e.dont_send_error_message();
41214142
shutdown_result = Err(e);
41224143
}
@@ -4313,7 +4334,7 @@ where
43134334
if let Some(mut chan) = peer_state.channel_by_id.remove(&channel_id) {
43144335
let reason = ClosureReason::FundingBatchClosure;
43154336
let err = ChannelError::Close((reason.to_string(), reason));
4316-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, &channel_id);
4337+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
43174338
shutdown_results.push((Err(e), counterparty_node_id));
43184339
}
43194340
}
@@ -4377,7 +4398,7 @@ where
43774398
if let Some(mut chan) = peer_state.channel_by_id.remove(channel_id) {
43784399
log_error!(logger, "Force-closing channel {}", channel_id);
43794400
let err = ChannelError::Close((message, reason));
4380-
let (_, mut e) = convert_channel_err!(self, peer_state, err, chan, channel_id);
4401+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, channel_id);
43814402
mem::drop(peer_state_lock);
43824403
mem::drop(per_peer_state);
43834404
if is_from_counterparty {
@@ -5834,7 +5855,7 @@ where
58345855
let reason = ClosureReason::ProcessingError { err: e.clone() };
58355856
let err = ChannelError::Close((e.clone(), reason));
58365857
let (_, e) =
5837-
convert_channel_err!(self, peer_state, err, chan, &channel_id);
5858+
convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
58385859
shutdown_results.push((Err(e), counterparty_node_id));
58395860
});
58405861
}
@@ -8655,15 +8676,16 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
86558676
let logger = WithChannelContext::from(&self.logger, &inbound_chan.context, None);
86568677
match inbound_chan.funding_created(msg, best_block, &self.signer_provider, &&logger) {
86578678
Ok(res) => res,
8658-
Err((mut inbound_chan, err)) => {
8679+
Err((inbound_chan, err)) => {
86598680
// We've already removed this inbound channel from the map in `PeerState`
86608681
// above so at this point we just need to clean up any lingering entries
86618682
// concerning this channel as it is safe to do so.
86628683
debug_assert!(matches!(err, ChannelError::Close(_)));
86638684
// Really we should be returning the channel_id the peer expects based
86648685
// on their funding info here, but they're horribly confused anyway, so
86658686
// there's not a lot we can do to save them.
8666-
return Err(convert_channel_err!(self, peer_state, err, inbound_chan.context, &inbound_chan.funding, &msg.temporary_channel_id, UNFUNDED_CHANNEL).1);
8687+
let mut chan = Channel::from(inbound_chan);
8688+
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &msg.temporary_channel_id, UNFUNDED_CHANNEL).1);
86678689
},
86688690
}
86698691
},
@@ -8685,7 +8707,8 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
86858707
// Thus, we must first unset the funding outpoint on the channel.
86868708
let err = ChannelError::close($err.to_owned());
86878709
chan.unset_funding_info();
8688-
return Err(convert_channel_err!(self, peer_state, err, chan.context, &chan.funding, &funded_channel_id, UNFUNDED_CHANNEL).1);
8710+
let mut chan = Channel::from(chan);
8711+
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &funded_channel_id, UNFUNDED_CHANNEL).1);
86898712
} } }
86908713

86918714
match peer_state.channel_by_id.entry(funded_channel_id) {
@@ -9226,7 +9249,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
92269249
let err = ChannelError::Close((reason.to_string(), reason));
92279250
let mut chan = chan_entry.remove();
92289251
let (_, mut e) =
9229-
convert_channel_err!(self, peer_state, err, chan, &msg.channel_id);
9252+
convert_channel_err!(self, peer_state, err, &mut chan, &msg.channel_id);
92309253
e.dont_send_error_message();
92319254
return Err(e);
92329255
},
@@ -9283,7 +9306,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
92839306
// fully delete it from tracking (the channel monitor is still around to
92849307
// watch for old state broadcasts)!
92859308
debug_assert!(tx.is_some());
9286-
locked_close_channel!(self, peer_state, chan.context, &chan.funding, close_res);
9309+
locked_close_channel!(self, peer_state, chan, close_res, FUNDED);
92879310
(tx, Some(chan_entry.remove()), Some(close_res))
92889311
} else {
92899312
debug_assert!(tx.is_none());
@@ -10257,7 +10280,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1025710280
};
1025810281
let err = ChannelError::Close((reason.to_string(), reason));
1025910282
let mut chan = chan_entry.remove();
10260-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, &channel_id);
10283+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
1026110284
failed_channels.push((Err(e), counterparty_node_id));
1026210285
}
1026310286
}
@@ -10446,10 +10469,14 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1044610469
_ => unblock_chan(chan, &mut peer_state.pending_msg_events),
1044710470
};
1044810471
if let Some(mut shutdown_result) = shutdown_result {
10449-
let context = &chan.context();
10472+
let context = chan.context();
1045010473
let logger = WithChannelContext::from(&self.logger, context, None);
1045110474
log_trace!(logger, "Removing channel {} now that the signer is unblocked", context.channel_id());
10452-
locked_close_channel!(self, peer_state, context, chan.funding(), shutdown_result);
10475+
if let Some(funded_channel) = chan.as_funded_mut() {
10476+
locked_close_channel!(self, peer_state, funded_channel, shutdown_result, FUNDED);
10477+
} else {
10478+
locked_close_channel!(self, chan.context(), UNFUNDED);
10479+
}
1045310480
shutdown_results.push(shutdown_result);
1045410481
false
1045510482
} else {
@@ -10492,7 +10519,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1049210519
}
1049310520
debug_assert_eq!(shutdown_result_opt.is_some(), funded_chan.is_shutdown());
1049410521
if let Some(mut shutdown_result) = shutdown_result_opt {
10495-
locked_close_channel!(self, peer_state, &funded_chan.context, &funded_chan.funding, shutdown_result);
10522+
locked_close_channel!(self, peer_state, funded_chan, shutdown_result, FUNDED);
1049610523
shutdown_results.push(shutdown_result);
1049710524
}
1049810525
if let Some(tx) = tx_opt {

0 commit comments

Comments
 (0)