Skip to content

Commit 1f46eab

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 9cbdbb8 commit 1f46eab

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
@@ -6008,6 +6008,16 @@ where
60086008
SP::Target: SignerProvider,
60096009
<SP::Target as SignerProvider>::EcdsaSigner: EcdsaChannelSigner,
60106010
{
6011+
pub fn context(&self) -> &ChannelContext<SP> {
6012+
&self.context
6013+
}
6014+
6015+
pub fn force_shutdown(
6016+
&mut self, closure_reason: ClosureReason, should_broadcast: bool,
6017+
) -> ShutdownResult {
6018+
self.context.force_shutdown(&self.funding, should_broadcast, closure_reason)
6019+
}
6020+
60116021
#[rustfmt::skip]
60126022
fn check_remote_fee<F: Deref, L: Deref>(
60136023
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
@@ -3167,22 +3167,33 @@ macro_rules! handle_error {
31673167
/// directly avoids duplicate error messages.
31683168
#[rustfmt::skip]
31693169
macro_rules! locked_close_channel {
3170-
($self: ident, $peer_state: expr, $channel_context: expr, $channel_funding: expr, $shutdown_res_mut: expr) => {{
3170+
($self: ident, $chan_context: expr, UNFUNDED) => {{
3171+
$self.short_to_chan_info.write().unwrap().remove(&$chan_context.outbound_scid_alias());
3172+
// If the channel was never confirmed on-chain prior to its closure, remove the
3173+
// outbound SCID alias we used for it from the collision-prevention set. While we
3174+
// generally want to avoid ever re-using an outbound SCID alias across all channels, we
3175+
// also don't want a counterparty to be able to trivially cause a memory leak by simply
3176+
// opening a million channels with us which are closed before we ever reach the funding
3177+
// stage.
3178+
let alias_removed = $self.outbound_scid_aliases.lock().unwrap().remove(&$chan_context.outbound_scid_alias());
3179+
debug_assert!(alias_removed);
3180+
}};
3181+
($self: ident, $peer_state: expr, $funded_chan: expr, $shutdown_res_mut: expr, FUNDED) => {{
31713182
if let Some((_, funding_txo, _, update)) = $shutdown_res_mut.monitor_update.take() {
31723183
handle_new_monitor_update!($self, funding_txo, update, $peer_state,
3173-
$channel_context, REMAIN_LOCKED_UPDATE_ACTIONS_PROCESSED_LATER);
3184+
$funded_chan.context, REMAIN_LOCKED_UPDATE_ACTIONS_PROCESSED_LATER);
31743185
}
31753186
// If there's a possibility that we need to generate further monitor updates for this
31763187
// channel, we need to store the last update_id of it. However, we don't want to insert
31773188
// into the map (which prevents the `PeerState` from being cleaned up) for channels that
31783189
// never even got confirmations (which would open us up to DoS attacks).
3179-
let update_id = $channel_context.get_latest_monitor_update_id();
3180-
if $channel_funding.get_funding_tx_confirmation_height().is_some() || $channel_context.minimum_depth($channel_funding) == Some(0) || update_id > 1 {
3181-
let chan_id = $channel_context.channel_id();
3190+
let update_id = $funded_chan.context.get_latest_monitor_update_id();
3191+
let mut short_to_chan_info = $self.short_to_chan_info.write().unwrap();
3192+
if $funded_chan.funding.get_funding_tx_confirmation_height().is_some() || $funded_chan.context.minimum_depth(&$funded_chan.funding) == Some(0) || update_id > 1 {
3193+
let chan_id = $funded_chan.context.channel_id();
31823194
$peer_state.closed_channel_monitor_update_ids.insert(chan_id, update_id);
31833195
}
3184-
let mut short_to_chan_info = $self.short_to_chan_info.write().unwrap();
3185-
if let Some(short_id) = $channel_funding.get_short_channel_id() {
3196+
if let Some(short_id) = $funded_chan.funding.get_short_channel_id() {
31863197
short_to_chan_info.remove(&short_id);
31873198
} else {
31883199
// If the channel was never confirmed on-chain prior to its closure, remove the
@@ -3191,11 +3202,11 @@ macro_rules! locked_close_channel {
31913202
// also don't want a counterparty to be able to trivially cause a memory leak by simply
31923203
// opening a million channels with us which are closed before we ever reach the funding
31933204
// stage.
3194-
let alias_removed = $self.outbound_scid_aliases.lock().unwrap().remove(&$channel_context.outbound_scid_alias());
3205+
let alias_removed = $self.outbound_scid_aliases.lock().unwrap().remove(&$funded_chan.context.outbound_scid_alias());
31953206
debug_assert!(alias_removed);
31963207
}
3197-
short_to_chan_info.remove(&$channel_context.outbound_scid_alias());
3198-
for scid in $channel_context.historical_scids() {
3208+
short_to_chan_info.remove(&$funded_chan.context.outbound_scid_alias());
3209+
for scid in $funded_chan.context.historical_scids() {
31993210
short_to_chan_info.remove(scid);
32003211
}
32013212
}}
@@ -3204,7 +3215,7 @@ macro_rules! locked_close_channel {
32043215
/// Returns (boolean indicating if we should remove the Channel object from memory, a mapped error)
32053216
#[rustfmt::skip]
32063217
macro_rules! convert_channel_err {
3207-
($self: ident, $peer_state: expr, $err: expr, $context: expr, $funding: expr, $channel_id: expr, MANUAL_CHANNEL_UPDATE, $channel_update: expr) => {
3218+
($self: ident, $peer_state: expr, $err: expr, $chan: expr, $close: expr, $locked_close: expr, $channel_id: expr, _internal) => {
32083219
match $err {
32093220
ChannelError::Warn(msg) => {
32103221
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(msg), *$channel_id))
@@ -3216,33 +3227,43 @@ macro_rules! convert_channel_err {
32163227
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore(msg), *$channel_id))
32173228
},
32183229
ChannelError::Close((msg, reason)) => {
3219-
let logger = WithChannelContext::from(&$self.logger, &$context, None);
3220-
log_error!(logger, "Closing channel {} due to close-required error: {}", $channel_id, msg);
3221-
let mut shutdown_res = $context.force_shutdown($funding, true, reason);
3222-
locked_close_channel!($self, $peer_state, $context, $funding, &mut shutdown_res);
3230+
let (mut shutdown_res, chan_update) = $close(reason);
3231+
let logger = WithChannelContext::from(&$self.logger, &$chan.context(), None);
3232+
log_error!(logger, "Closed channel {} due to close-required error: {}", $channel_id, msg);
3233+
$locked_close(&mut shutdown_res, $chan);
32233234
let err =
3224-
MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, $channel_update);
3235+
MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, chan_update);
32253236
(true, err)
32263237
},
32273238
ChannelError::SendError(msg) => {
32283239
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::SendError(msg), *$channel_id))
32293240
},
32303241
}
32313242
};
3232-
($self: ident, $peer_state: expr, $err: expr, $funded_channel: expr, $channel_id: expr, FUNDED_CHANNEL) => {
3233-
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() })
3234-
};
3235-
($self: ident, $peer_state: expr, $err: expr, $context: expr, $funding: expr, $channel_id: expr, UNFUNDED_CHANNEL) => {
3236-
convert_channel_err!($self, $peer_state, $err, $context, $funding, $channel_id, MANUAL_CHANNEL_UPDATE, None)
3237-
};
3243+
($self: ident, $peer_state: expr, $err: expr, $funded_channel: expr, $channel_id: expr, FUNDED_CHANNEL) => { {
3244+
let mut do_close = |reason| {
3245+
(
3246+
$funded_channel.force_shutdown(reason, true),
3247+
$self.get_channel_update_for_broadcast(&$funded_channel).ok(),
3248+
)
3249+
};
3250+
let mut locked_close = |shutdown_res_mut: &mut ShutdownResult, funded_channel: &mut FundedChannel<_>| {
3251+
locked_close_channel!($self, $peer_state, funded_channel, shutdown_res_mut, FUNDED);
3252+
};
3253+
convert_channel_err!($self, $peer_state, $err, $funded_channel, do_close, locked_close, $channel_id, _internal)
3254+
} };
3255+
($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr, UNFUNDED_CHANNEL) => { {
3256+
let mut do_close = |reason| { ($channel.force_shutdown(true, reason), None) };
3257+
let locked_close = |_, chan: &mut Channel<_>| { locked_close_channel!($self, chan.context(), UNFUNDED); };
3258+
convert_channel_err!($self, $peer_state, $err, $channel, do_close, locked_close, $channel_id, _internal)
3259+
} };
32383260
($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr) => {
32393261
match $channel.as_funded_mut() {
32403262
Some(funded_channel) => {
32413263
convert_channel_err!($self, $peer_state, $err, funded_channel, $channel_id, FUNDED_CHANNEL)
32423264
},
32433265
None => {
3244-
let (funding, context) = $channel.funding_and_context_mut();
3245-
convert_channel_err!($self, $peer_state, $err, context, funding, $channel_id, UNFUNDED_CHANNEL)
3266+
convert_channel_err!($self, $peer_state, $err, $channel, $channel_id, UNFUNDED_CHANNEL)
32463267
},
32473268
}
32483269
};
@@ -4114,7 +4135,7 @@ where
41144135
let reason = ClosureReason::LocallyCoopClosedUnfundedChannel;
41154136
let err = ChannelError::Close((reason.to_string(), reason));
41164137
let mut chan = chan_entry.remove();
4117-
let (_, mut e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
4138+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, chan_id);
41184139
e.dont_send_error_message();
41194140
shutdown_result = Err(e);
41204141
}
@@ -4308,7 +4329,7 @@ where
43084329
if let Some(mut chan) = peer_state.channel_by_id.remove(&channel_id) {
43094330
let reason = ClosureReason::FundingBatchClosure;
43104331
let err = ChannelError::Close((reason.to_string(), reason));
4311-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, &channel_id);
4332+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
43124333
shutdown_results.push((Err(e), counterparty_node_id));
43134334
}
43144335
}
@@ -4372,7 +4393,7 @@ where
43724393
if let Some(mut chan) = peer_state.channel_by_id.remove(channel_id) {
43734394
log_error!(logger, "Force-closing channel {}", channel_id);
43744395
let err = ChannelError::Close((message, reason));
4375-
let (_, mut e) = convert_channel_err!(self, peer_state, err, chan, channel_id);
4396+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, channel_id);
43764397
mem::drop(peer_state_lock);
43774398
mem::drop(per_peer_state);
43784399
if is_from_counterparty {
@@ -5805,7 +5826,7 @@ where
58055826
let reason = ClosureReason::ProcessingError { err: e.clone() };
58065827
let err = ChannelError::Close((e.clone(), reason));
58075828
let (_, e) =
5808-
convert_channel_err!(self, peer_state, err, chan, &channel_id);
5829+
convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
58095830
shutdown_results.push((Err(e), counterparty_node_id));
58105831
});
58115832
}
@@ -8623,15 +8644,16 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
86238644
let logger = WithChannelContext::from(&self.logger, &inbound_chan.context, None);
86248645
match inbound_chan.funding_created(msg, best_block, &self.signer_provider, &&logger) {
86258646
Ok(res) => res,
8626-
Err((mut inbound_chan, err)) => {
8647+
Err((inbound_chan, err)) => {
86278648
// We've already removed this inbound channel from the map in `PeerState`
86288649
// above so at this point we just need to clean up any lingering entries
86298650
// concerning this channel as it is safe to do so.
86308651
debug_assert!(matches!(err, ChannelError::Close(_)));
86318652
// Really we should be returning the channel_id the peer expects based
86328653
// on their funding info here, but they're horribly confused anyway, so
86338654
// there's not a lot we can do to save them.
8634-
return Err(convert_channel_err!(self, peer_state, err, inbound_chan.context, &inbound_chan.funding, &msg.temporary_channel_id, UNFUNDED_CHANNEL).1);
8655+
let mut chan = Channel::from(inbound_chan);
8656+
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &msg.temporary_channel_id, UNFUNDED_CHANNEL).1);
86358657
},
86368658
}
86378659
},
@@ -8653,7 +8675,8 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
86538675
// Thus, we must first unset the funding outpoint on the channel.
86548676
let err = ChannelError::close($err.to_owned());
86558677
chan.unset_funding_info();
8656-
return Err(convert_channel_err!(self, peer_state, err, chan.context, &chan.funding, &funded_channel_id, UNFUNDED_CHANNEL).1);
8678+
let mut chan = Channel::from(chan);
8679+
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &funded_channel_id, UNFUNDED_CHANNEL).1);
86578680
} } }
86588681

86598682
match peer_state.channel_by_id.entry(funded_channel_id) {
@@ -9194,7 +9217,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
91949217
let err = ChannelError::Close((reason.to_string(), reason));
91959218
let mut chan = chan_entry.remove();
91969219
let (_, mut e) =
9197-
convert_channel_err!(self, peer_state, err, chan, &msg.channel_id);
9220+
convert_channel_err!(self, peer_state, err, &mut chan, &msg.channel_id);
91989221
e.dont_send_error_message();
91999222
return Err(e);
92009223
},
@@ -9251,7 +9274,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
92519274
// fully delete it from tracking (the channel monitor is still around to
92529275
// watch for old state broadcasts)!
92539276
debug_assert!(tx.is_some());
9254-
locked_close_channel!(self, peer_state, chan.context, &chan.funding, close_res);
9277+
locked_close_channel!(self, peer_state, chan, close_res, FUNDED);
92559278
(tx, Some(chan_entry.remove()), Some(close_res))
92569279
} else {
92579280
debug_assert!(tx.is_none());
@@ -10225,7 +10248,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1022510248
};
1022610249
let err = ChannelError::Close((reason.to_string(), reason));
1022710250
let mut chan = chan_entry.remove();
10228-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, &channel_id);
10251+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
1022910252
failed_channels.push((Err(e), counterparty_node_id));
1023010253
}
1023110254
}
@@ -10414,10 +10437,14 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1041410437
_ => unblock_chan(chan, &mut peer_state.pending_msg_events),
1041510438
};
1041610439
if let Some(mut shutdown_result) = shutdown_result {
10417-
let context = &chan.context();
10440+
let context = chan.context();
1041810441
let logger = WithChannelContext::from(&self.logger, context, None);
1041910442
log_trace!(logger, "Removing channel {} now that the signer is unblocked", context.channel_id());
10420-
locked_close_channel!(self, peer_state, context, chan.funding(), shutdown_result);
10443+
if let Some(funded_channel) = chan.as_funded_mut() {
10444+
locked_close_channel!(self, peer_state, funded_channel, shutdown_result, FUNDED);
10445+
} else {
10446+
locked_close_channel!(self, chan.context(), UNFUNDED);
10447+
}
1042110448
shutdown_results.push(shutdown_result);
1042210449
false
1042310450
} else {
@@ -10460,7 +10487,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1046010487
}
1046110488
debug_assert_eq!(shutdown_result_opt.is_some(), funded_chan.is_shutdown());
1046210489
if let Some(mut shutdown_result) = shutdown_result_opt {
10463-
locked_close_channel!(self, peer_state, &funded_chan.context, &funded_chan.funding, shutdown_result);
10490+
locked_close_channel!(self, peer_state, funded_chan, shutdown_result, FUNDED);
1046410491
shutdown_results.push(shutdown_result);
1046510492
}
1046610493
if let Some(tx) = tx_opt {

0 commit comments

Comments
 (0)