Skip to content

Commit 13281f7

Browse files
TheBlueMattTheBlueMatt
committed
Drop the channel_id argument from convert_channel_err
The `channel_id` argument to `convert_channel_err` allows us to set a different channel ID in the error message around the funding process. However, its not exactly critical to make sure we get it right, and yet another macro argument is annoying to deal with, so here we simply drop it and use the `Channel` value. This means that when force-closing we sometimes send an `error` with the `temporary_channel_id` rather than the funded `channel_id`, but its not that critical to get right (and we don't in all cases anyway), the peer will eventually figure it out when we reconnect or they try to send more messages about the channel.
1 parent 5b2e68e commit 13281f7

File tree

2 files changed

+48
-59
lines changed

2 files changed

+48
-59
lines changed

‎lightning/src/ln/channelmanager.rs‎

Lines changed: 47 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -3167,32 +3167,33 @@ macro_rules! locked_close_channel {
31673167
/// Returns (boolean indicating if we should remove the Channel object from memory, a mapped error)
31683168
#[rustfmt::skip]
31693169
macro_rules! convert_channel_err {
3170-
($self: ident, $peer_state: expr, $err: expr, $chan: expr, $close: expr, $locked_close: expr, $channel_id: expr, _internal) => {
3170+
($self: ident, $peer_state: expr, $err: expr, $chan: expr, $close: expr, $locked_close: expr, $channel_id: expr, _internal) => { {
31713171
match $err {
31723172
ChannelError::Warn(msg) => {
3173-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(msg), *$channel_id))
3173+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(msg), $channel_id))
31743174
},
31753175
ChannelError::WarnAndDisconnect(msg) => {
3176-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::WarnAndDisconnect(msg), *$channel_id))
3176+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::WarnAndDisconnect(msg), $channel_id))
31773177
},
31783178
ChannelError::Ignore(msg) => {
3179-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore(msg), *$channel_id))
3179+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore(msg), $channel_id))
31803180
},
31813181
ChannelError::Close((msg, reason)) => {
31823182
let (mut shutdown_res, chan_update) = $close(reason);
31833183
let logger = WithChannelContext::from(&$self.logger, &$chan.context(), None);
31843184
log_error!(logger, "Closed channel {} due to close-required error: {}", $channel_id, msg);
31853185
$locked_close(&mut shutdown_res, $chan);
31863186
let err =
3187-
MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, chan_update);
3187+
MsgHandleErrInternal::from_finish_shutdown(msg, $channel_id, shutdown_res, chan_update);
31883188
(true, err)
31893189
},
31903190
ChannelError::SendError(msg) => {
3191-
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::SendError(msg), *$channel_id))
3191+
(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::SendError(msg), $channel_id))
31923192
},
31933193
}
3194-
};
3195-
($self: ident, $peer_state: expr, $shutdown_result: expr, $funded_channel: expr, $channel_id: expr, COOP_CLOSED) => { {
3194+
} };
3195+
($self: ident, $peer_state: expr, $shutdown_result: expr, $funded_channel: expr, COOP_CLOSED) => { {
3196+
let chan_id = $funded_channel.context.channel_id();
31963197
let reason = ChannelError::Close(("Coop Closed".to_owned(), $shutdown_result.closure_reason.clone()));
31973198
let do_close = |_| {
31983199
(
@@ -3204,12 +3205,13 @@ macro_rules! convert_channel_err {
32043205
locked_close_channel!($self, $peer_state, funded_channel, shutdown_res_mut, FUNDED);
32053206
};
32063207
let (close, mut err) =
3207-
convert_channel_err!($self, $peer_state, reason, $funded_channel, do_close, locked_close, $channel_id, _internal);
3208+
convert_channel_err!($self, $peer_state, reason, $funded_channel, do_close, locked_close, chan_id, _internal);
32083209
err.dont_send_error_message();
32093210
debug_assert!(close);
32103211
err
32113212
} };
3212-
($self: ident, $peer_state: expr, $err: expr, $funded_channel: expr, $channel_id: expr, FUNDED_CHANNEL) => { {
3213+
($self: ident, $peer_state: expr, $err: expr, $funded_channel: expr, FUNDED_CHANNEL) => { {
3214+
let chan_id = $funded_channel.context.channel_id();
32133215
let mut do_close = |reason| {
32143216
(
32153217
$funded_channel.force_shutdown(reason),
@@ -3219,20 +3221,21 @@ macro_rules! convert_channel_err {
32193221
let mut locked_close = |shutdown_res_mut: &mut ShutdownResult, funded_channel: &mut FundedChannel<_>| {
32203222
locked_close_channel!($self, $peer_state, funded_channel, shutdown_res_mut, FUNDED);
32213223
};
3222-
convert_channel_err!($self, $peer_state, $err, $funded_channel, do_close, locked_close, $channel_id, _internal)
3224+
convert_channel_err!($self, $peer_state, $err, $funded_channel, do_close, locked_close, chan_id, _internal)
32233225
} };
3224-
($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr, UNFUNDED_CHANNEL) => { {
3226+
($self: ident, $peer_state: expr, $err: expr, $channel: expr, UNFUNDED_CHANNEL) => { {
3227+
let chan_id = $channel.context().channel_id();
32253228
let mut do_close = |reason| { ($channel.force_shutdown(reason), None) };
32263229
let locked_close = |_, chan: &mut Channel<_>| { locked_close_channel!($self, chan.context(), UNFUNDED); };
3227-
convert_channel_err!($self, $peer_state, $err, $channel, do_close, locked_close, $channel_id, _internal)
3230+
convert_channel_err!($self, $peer_state, $err, $channel, do_close, locked_close, chan_id, _internal)
32283231
} };
3229-
($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr) => {
3232+
($self: ident, $peer_state: expr, $err: expr, $channel: expr) => {
32303233
match $channel.as_funded_mut() {
32313234
Some(funded_channel) => {
3232-
convert_channel_err!($self, $peer_state, $err, funded_channel, $channel_id, FUNDED_CHANNEL)
3235+
convert_channel_err!($self, $peer_state, $err, funded_channel, FUNDED_CHANNEL)
32333236
},
32343237
None => {
3235-
convert_channel_err!($self, $peer_state, $err, $channel, $channel_id, UNFUNDED_CHANNEL)
3238+
convert_channel_err!($self, $peer_state, $err, $channel, UNFUNDED_CHANNEL)
32363239
},
32373240
}
32383241
};
@@ -3243,9 +3246,7 @@ macro_rules! break_channel_entry {
32433246
match $res {
32443247
Ok(res) => res,
32453248
Err(e) => {
3246-
let key = *$entry.key();
3247-
let (drop, res) =
3248-
convert_channel_err!($self, $peer_state, e, $entry.get_mut(), &key);
3249+
let (drop, res) = convert_channel_err!($self, $peer_state, e, $entry.get_mut());
32493250
if drop {
32503251
$entry.remove_entry();
32513252
}
@@ -3260,9 +3261,7 @@ macro_rules! try_channel_entry {
32603261
match $res {
32613262
Ok(res) => res,
32623263
Err(e) => {
3263-
let key = *$entry.key();
3264-
let (drop, res) =
3265-
convert_channel_err!($self, $peer_state, e, $entry.get_mut(), &key);
3264+
let (drop, res) = convert_channel_err!($self, $peer_state, e, $entry.get_mut());
32663265
if drop {
32673266
$entry.remove_entry();
32683267
}
@@ -4105,7 +4104,7 @@ where
41054104
let reason = ClosureReason::LocallyCoopClosedUnfundedChannel;
41064105
let err = ChannelError::Close((reason.to_string(), reason));
41074106
let mut chan = chan_entry.remove();
4108-
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, chan_id);
4107+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan);
41094108
e.dont_send_error_message();
41104109
shutdown_result = Err(e);
41114110
}
@@ -4302,7 +4301,7 @@ where
43024301
if let Some(mut chan) = peer_state.channel_by_id.remove(&channel_id) {
43034302
let reason = ClosureReason::FundingBatchClosure;
43044303
let err = ChannelError::Close((reason.to_string(), reason));
4305-
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
4304+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan);
43064305
shutdown_results.push((Err(e), counterparty_node_id));
43074306
}
43084307
}
@@ -4366,7 +4365,7 @@ where
43664365
if let Some(mut chan) = peer_state.channel_by_id.remove(channel_id) {
43674366
log_error!(logger, "Force-closing channel {}", channel_id);
43684367
let err = ChannelError::Close((message, reason));
4369-
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan, channel_id);
4368+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan);
43704369
mem::drop(peer_state_lock);
43714370
mem::drop(per_peer_state);
43724371
if is_from_counterparty {
@@ -5842,7 +5841,7 @@ where
58425841
let reason = ClosureReason::ProcessingError { err: e.clone() };
58435842
let err = ChannelError::Close((e.clone(), reason));
58445843
let (_, e) =
5845-
convert_channel_err!(self, peer_state, err, &mut chan, &channel_id);
5844+
convert_channel_err!(self, peer_state, err, &mut chan);
58465845
shutdown_results.push((Err(e), counterparty_node_id));
58475846
});
58485847
}
@@ -7308,7 +7307,7 @@ where
73087307
if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; }
73097308

73107309
if let Err(e) = funded_chan.timer_check_closing_negotiation_progress() {
7311-
let (needs_close, err) = convert_channel_err!(self, peer_state, e, funded_chan, chan_id, FUNDED_CHANNEL);
7310+
let (needs_close, err) = convert_channel_err!(self, peer_state, e, funded_chan, FUNDED_CHANNEL);
73127311
handle_errors.push((Err(err), counterparty_node_id));
73137312
if needs_close { return false; }
73147313
}
@@ -7386,7 +7385,7 @@ where
73867385
let reason = ClosureReason::FundingTimedOut;
73877386
let msg = "Force-closing pending channel due to timeout awaiting establishment handshake".to_owned();
73887387
let err = ChannelError::Close((msg, reason));
7389-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
7388+
let (_, e) = convert_channel_err!(self, peer_state, err, chan);
73907389
handle_errors.push((Err(e), counterparty_node_id));
73917390
false
73927391
} else {
@@ -9202,18 +9201,15 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
92029201
// above so at this point we just need to clean up any lingering entries
92039202
// concerning this channel as it is safe to do so.
92049203
debug_assert!(matches!(err, ChannelError::Close(_)));
9205-
// Really we should be returning the channel_id the peer expects based
9206-
// on their funding info here, but they're horribly confused anyway, so
9207-
// there's not a lot we can do to save them.
92089204
let mut chan = Channel::from(inbound_chan);
9209-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &msg.temporary_channel_id, UNFUNDED_CHANNEL).1);
9205+
return Err(convert_channel_err!(self, peer_state, err, &mut chan).1);
92109206
},
92119207
}
92129208
},
92139209
Some(Err(mut chan)) => {
92149210
let err_msg = format!("Got an unexpected funding_created message from peer with counterparty_node_id {}", counterparty_node_id);
92159211
let err = ChannelError::close(err_msg);
9216-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &msg.temporary_channel_id).1);
9212+
return Err(convert_channel_err!(self, peer_state, err, &mut chan).1);
92179213
},
92189214
None => return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.temporary_channel_id))
92199215
};
@@ -9229,7 +9225,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
92299225
let err = ChannelError::close($err.to_owned());
92309226
chan.unset_funding_info();
92319227
let mut chan = Channel::from(chan);
9232-
return Err(convert_channel_err!(self, peer_state, err, &mut chan, &funded_channel_id, UNFUNDED_CHANNEL).1);
9228+
return Err(convert_channel_err!(self, peer_state, err, &mut chan, UNFUNDED_CHANNEL).1);
92339229
} } }
92349230

92359231
match peer_state.channel_by_id.entry(funded_channel_id) {
@@ -9769,8 +9765,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
97699765
let reason = ClosureReason::CounterpartyCoopClosedUnfundedChannel;
97709766
let err = ChannelError::Close((reason.to_string(), reason));
97719767
let mut chan = chan_entry.remove();
9772-
let (_, mut e) =
9773-
convert_channel_err!(self, peer_state, err, &mut chan, &msg.channel_id);
9768+
let (_, mut e) = convert_channel_err!(self, peer_state, err, &mut chan);
97749769
e.dont_send_error_message();
97759770
return Err(e);
97769771
},
@@ -9829,7 +9824,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
98299824
// also implies there are no pending HTLCs left on the channel, so we can
98309825
// fully delete it from tracking (the channel monitor is still around to
98319826
// watch for old state broadcasts)!
9832-
let err = convert_channel_err!(self, peer_state, close_res, chan, &msg.channel_id, COOP_CLOSED);
9827+
let err = convert_channel_err!(self, peer_state, close_res, chan, COOP_CLOSED);
98339828
chan_entry.remove();
98349829
Some((tx, Err(err)))
98359830
} else {
@@ -10881,13 +10876,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1088110876
};
1088210877
let err = ChannelError::Close((reason.to_string(), reason));
1088310878
let mut chan = chan_entry.remove();
10884-
let (_, e) = convert_channel_err!(
10885-
self,
10886-
peer_state,
10887-
err,
10888-
&mut chan,
10889-
&channel_id
10890-
);
10879+
let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan);
1089110880
failed_channels.push((Err(e), counterparty_node_id));
1089210881
}
1089310882
}
@@ -11083,13 +11072,14 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1108311072
let chan_id = context.channel_id();
1108411073
log_trace!(logger, "Removing channel {} now that the signer is unblocked", chan_id);
1108511074
let (remove, err) = if let Some(funded) = chan.as_funded_mut() {
11086-
let err = convert_channel_err!(self, peer_state, shutdown, funded, &chan_id, COOP_CLOSED);
11075+
let err =
11076+
convert_channel_err!(self, peer_state, shutdown, funded, COOP_CLOSED);
1108711077
(true, err)
1108811078
} else {
1108911079
debug_assert!(false);
1109011080
let reason = shutdown.closure_reason.clone();
1109111081
let err = ChannelError::Close((reason.to_string(), reason));
11092-
convert_channel_err!(self, peer_state, err, chan, &chan_id, UNFUNDED_CHANNEL)
11082+
convert_channel_err!(self, peer_state, err, chan, UNFUNDED_CHANNEL)
1109311083
};
1109411084
debug_assert!(remove);
1109511085
shutdown_results.push((Err(err), *cp_id));
@@ -11119,7 +11109,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1111911109
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
1112011110
let peer_state = &mut *peer_state_lock;
1112111111
let pending_msg_events = &mut peer_state.pending_msg_events;
11122-
peer_state.channel_by_id.retain(|channel_id, chan| {
11112+
peer_state.channel_by_id.retain(|_, chan| {
1112311113
match chan.as_funded_mut() {
1112411114
Some(funded_chan) => {
1112511115
let logger = WithChannelContext::from(&self.logger, &funded_chan.context, None);
@@ -11135,7 +11125,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1113511125
if let Some((tx, shutdown_res)) = tx_shutdown_result_opt {
1113611126
// We're done with this channel. We got a closing_signed and sent back
1113711127
// a closing_signed with a closing transaction to broadcast.
11138-
let err = convert_channel_err!(self, peer_state, shutdown_res, funded_chan, channel_id, COOP_CLOSED);
11128+
let err = convert_channel_err!(self, peer_state, shutdown_res, funded_chan, COOP_CLOSED);
1113911129
handle_errors.push((*cp_id, Err(err)));
1114011130

1114111131
log_info!(logger, "Broadcasting {}", log_tx!(tx));
@@ -11145,7 +11135,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1114511135
},
1114611136
Err(e) => {
1114711137
has_update = true;
11148-
let (close_channel, res) = convert_channel_err!(self, peer_state, e, funded_chan, channel_id, FUNDED_CHANNEL);
11138+
let (close_channel, res) = convert_channel_err!(self, peer_state, e, funded_chan, FUNDED_CHANNEL);
1114911139
handle_errors.push((funded_chan.context.get_counterparty_node_id(), Err(res)));
1115011140
!close_channel
1115111141
}
@@ -12399,15 +12389,15 @@ where
1239912389
let mut peer_state_lock = peer_state_mutex.lock().unwrap();
1240012390
let peer_state = &mut *peer_state_lock;
1240112391
let pending_msg_events = &mut peer_state.pending_msg_events;
12402-
peer_state.channel_by_id.retain(|chan_id, chan| {
12392+
peer_state.channel_by_id.retain(|_, chan| {
1240312393
let logger = WithChannelContext::from(&self.logger, &chan.context(), None);
1240412394
if chan.peer_disconnected_is_resumable(&&logger) {
1240512395
return true;
1240612396
}
1240712397
// Clean up for removal.
1240812398
let reason = ClosureReason::DisconnectedPeer;
1240912399
let err = ChannelError::Close((reason.to_string(), reason));
12410-
let (_, e) = convert_channel_err!(self, peer_state, err, chan, chan_id);
12400+
let (_, e) = convert_channel_err!(self, peer_state, err, chan);
1241112401
failed_channels.push((Err(e), counterparty_node_id));
1241212402
false
1241312403
});
@@ -12962,7 +12952,7 @@ where
1296212952
let peer_state = &mut *peer_state_lock;
1296312953
let pending_msg_events = &mut peer_state.pending_msg_events;
1296412954

12965-
peer_state.channel_by_id.retain(|chan_id, chan| {
12955+
peer_state.channel_by_id.retain(|channel_id, chan| {
1296612956
match chan.as_funded_mut() {
1296712957
// Retain unfunded channels.
1296812958
None => true,
@@ -12973,22 +12963,22 @@ where
1297312963
let reason = LocalHTLCFailureReason::CLTVExpiryTooSoon;
1297412964
let data = self.get_htlc_inbound_temp_fail_data(reason);
1297512965
timed_out_htlcs.push((source, payment_hash, HTLCFailReason::reason(reason, data),
12976-
HTLCHandlingFailureType::Forward { node_id: Some(funded_channel.context.get_counterparty_node_id()), channel_id: funded_channel.context.channel_id() }));
12966+
HTLCHandlingFailureType::Forward { node_id: Some(funded_channel.context.get_counterparty_node_id()), channel_id: *channel_id }));
1297712967
}
1297812968
let logger = WithChannelContext::from(&self.logger, &funded_channel.context, None);
1297912969
match funding_confirmed_opt {
1298012970
Some(FundingConfirmedMessage::Establishment(channel_ready)) => {
1298112971
send_channel_ready!(self, pending_msg_events, funded_channel, channel_ready);
1298212972
if funded_channel.context.is_usable() {
12983-
log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", funded_channel.context.channel_id());
12973+
log_trace!(logger, "Sending channel_ready with private initial channel_update for our counterparty on channel {}", channel_id);
1298412974
if let Ok(msg) = self.get_channel_update_for_unicast(funded_channel) {
1298512975
pending_msg_events.push(MessageSendEvent::SendChannelUpdate {
1298612976
node_id: funded_channel.context.get_counterparty_node_id(),
1298712977
msg,
1298812978
});
1298912979
}
1299012980
} else {
12991-
log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", funded_channel.context.channel_id());
12981+
log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", channel_id);
1299212982
}
1299312983
},
1299412984
#[cfg(splicing)]
@@ -13088,8 +13078,8 @@ where
1308813078
// un-confirmed we force-close the channel, ensuring short_to_chan_info
1308913079
// is always consistent.
1309013080
let mut short_to_chan_info = self.short_to_chan_info.write().unwrap();
13091-
let scid_insert = short_to_chan_info.insert(real_scid, (funded_channel.context.get_counterparty_node_id(), funded_channel.context.channel_id()));
13092-
assert!(scid_insert.is_none() || scid_insert.unwrap() == (funded_channel.context.get_counterparty_node_id(), funded_channel.context.channel_id()),
13081+
let scid_insert = short_to_chan_info.insert(real_scid, (funded_channel.context.get_counterparty_node_id(), *channel_id));
13082+
assert!(scid_insert.is_none() || scid_insert.unwrap() == (funded_channel.context.get_counterparty_node_id(), *channel_id),
1309313083
"SCIDs should never collide - ensure you weren't behind by a full {} blocks when creating channels",
1309413084
fake_scid::MAX_SCID_BLOCKS_FROM_NOW);
1309513085
}
@@ -13103,7 +13093,6 @@ where
1310313093
peer_state,
1310413094
err,
1310513095
funded_channel,
13106-
chan_id,
1310713096
FUNDED_CHANNEL
1310813097
);
1310913098
failed_channels.push((Err(e), *counterparty_node_id));

‎lightning/src/ln/functional_tests.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9019,7 +9019,7 @@ pub fn test_duplicate_chan_id() {
90199019
} => {
90209020
// Technically, at this point, nodes[1] would be justified in thinking both
90219021
// channels are closed, but currently we do not, so we just move forward with it.
9022-
assert_eq!(msg.channel_id, channel_id);
9022+
assert_eq!(msg.channel_id, funding_created.temporary_channel_id);
90239023
assert_eq!(node_id, node_a_id);
90249024
},
90259025
_ => panic!("Unexpected event"),

0 commit comments

Comments
 (0)