Skip to content

Commit e6ea987

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 3f69a55 commit e6ea987

File tree

2 files changed

+74
-37
lines changed

2 files changed

+74
-37
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: 64 additions & 37 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+
let mut short_to_chan_info = $self.short_to_chan_info.write().unwrap();
3194+
if $funded_chan.funding.get_funding_tx_confirmation_height().is_some() || $funded_chan.context.minimum_depth(&$funded_chan.funding) == Some(0) || update_id > 1 {
3195+
let chan_id = $funded_chan.context.channel_id();
31843196
$peer_state.closed_channel_monitor_update_ids.insert(chan_id, update_id);
31853197
}
3186-
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
}
@@ -4310,7 +4331,7 @@ where
43104331
if let Some(mut chan) = peer_state.channel_by_id.remove(&channel_id) {
43114332
let reason = ClosureReason::FundingBatchClosure;
43124333
let err = ChannelError::Close((reason.to_string(), reason));
4313-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, &channel_id);
4334+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
43144335
shutdown_results.push((Err(e), counterparty_node_id));
43154336
}
43164337
}
@@ -4374,7 +4395,7 @@ where
43744395
if let Some(mut chan) = peer_state.channel_by_id.remove(channel_id) {
43754396
log_error!(logger, "Force-closing channel {}", channel_id);
43764397
let err = ChannelError::Close((message, reason));
4377-
let (_, mut e) = convert_channel_err!(self, peer_state, err, chan, channel_id);
4398+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, channel_id);
43784399
mem::drop(peer_state_lock);
43794400
mem::drop(per_peer_state);
43804401
if is_from_counterparty {
@@ -5831,7 +5852,7 @@ where
58315852
let reason = ClosureReason::ProcessingError { err: e.clone() };
58325853
let err = ChannelError::Close((e.clone(), reason));
58335854
let (_, e) =
5834-
convert_channel_err!(self, peer_state, err, chan, &channel_id);
5855+
convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
58355856
shutdown_results.push((Err(e), counterparty_node_id));
58365857
});
58375858
}
@@ -8652,15 +8673,16 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
86528673
let logger = WithChannelContext::from(&self.logger, &inbound_chan.context, None);
86538674
match inbound_chan.funding_created(msg, best_block, &self.signer_provider, &&logger) {
86548675
Ok(res) => res,
8655-
Err((mut inbound_chan, err)) => {
8676+
Err((inbound_chan, err)) => {
86568677
// We've already removed this inbound channel from the map in `PeerState`
86578678
// above so at this point we just need to clean up any lingering entries
86588679
// concerning this channel as it is safe to do so.
86598680
debug_assert!(matches!(err, ChannelError::Close(_)));
86608681
// Really we should be returning the channel_id the peer expects based
86618682
// on their funding info here, but they're horribly confused anyway, so
86628683
// there's not a lot we can do to save them.
8663-
return Err(convert_channel_err!(self, peer_state, err, inbound_chan.context, &inbound_chan.funding, &msg.temporary_channel_id, UNFUNDED_CHANNEL).1);
8684+
let mut chan = Channel::from(inbound_chan);
8685+
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &msg.temporary_channel_id, UNFUNDED_CHANNEL).1);
86648686
},
86658687
}
86668688
},
@@ -8682,7 +8704,8 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
86828704
// Thus, we must first unset the funding outpoint on the channel.
86838705
let err = ChannelError::close($err.to_owned());
86848706
chan.unset_funding_info();
8685-
return Err(convert_channel_err!(self, peer_state, err, chan.context, &chan.funding, &funded_channel_id, UNFUNDED_CHANNEL).1);
8707+
let mut chan = Channel::from(chan);
8708+
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &funded_channel_id, UNFUNDED_CHANNEL).1);
86868709
} } }
86878710

86888711
match peer_state.channel_by_id.entry(funded_channel_id) {
@@ -9223,7 +9246,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
92239246
let err = ChannelError::Close((reason.to_string(), reason));
92249247
let mut chan = chan_entry.remove();
92259248
let (_, mut e) =
9226-
convert_channel_err!(self, peer_state, err, chan, &msg.channel_id);
9249+
convert_channel_err!(self, peer_state, err, &mut chan, &msg.channel_id);
92279250
e.dont_send_error_message();
92289251
return Err(e);
92299252
},
@@ -9280,7 +9303,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
92809303
// fully delete it from tracking (the channel monitor is still around to
92819304
// watch for old state broadcasts)!
92829305
debug_assert!(tx.is_some());
9283-
locked_close_channel!(self, peer_state, chan.context, &chan.funding, close_res);
9306+
locked_close_channel!(self, peer_state, chan, close_res, FUNDED);
92849307
(tx, Some(chan_entry.remove()), Some(close_res))
92859308
} else {
92869309
debug_assert!(tx.is_none());
@@ -10254,7 +10277,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1025410277
};
1025510278
let err = ChannelError::Close((reason.to_string(), reason));
1025610279
let mut chan = chan_entry.remove();
10257-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, &channel_id);
10280+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
1025810281
failed_channels.push((Err(e), counterparty_node_id));
1025910282
}
1026010283
}
@@ -10443,10 +10466,14 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1044310466
_ => unblock_chan(chan, &mut peer_state.pending_msg_events),
1044410467
};
1044510468
if let Some(mut shutdown_result) = shutdown_result {
10446-
let context = &chan.context();
10469+
let context = chan.context();
1044710470
let logger = WithChannelContext::from(&self.logger, context, None);
1044810471
log_trace!(logger, "Removing channel {} now that the signer is unblocked", context.channel_id());
10449-
locked_close_channel!(self, peer_state, context, chan.funding(), shutdown_result);
10472+
if let Some(funded_channel) = chan.as_funded_mut() {
10473+
locked_close_channel!(self, peer_state, funded_channel, shutdown_result, FUNDED);
10474+
} else {
10475+
locked_close_channel!(self, chan.context(), UNFUNDED);
10476+
}
1045010477
shutdown_results.push(shutdown_result);
1045110478
false
1045210479
} else {
@@ -10489,7 +10516,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1048910516
}
1049010517
debug_assert_eq!(shutdown_result_opt.is_some(), funded_chan.is_shutdown());
1049110518
if let Some(mut shutdown_result) = shutdown_result_opt {
10492-
locked_close_channel!(self, peer_state, &funded_chan.context, &funded_chan.funding, shutdown_result);
10519+
locked_close_channel!(self, peer_state, funded_chan, shutdown_result, FUNDED);
1049310520
shutdown_results.push(shutdown_result);
1049410521
}
1049510522
if let Some(tx) = tx_opt {

0 commit comments

Comments
 (0)